fix(core): reopen database after update failure

[Eljees]

Description of Change

Reopen the local database before propagating a cached data source update failure. This preserves the original UpdateException while allowing callers to continue analysis with existing vulnerability data, as the warning promises.

A regression test registers a failing cached web data source and verifies that the database remains available after the failed update.

Related issues

• Fixes Analysis does not continue after update failure #6535

Have test cases been added to cover the new functionality?

Yes.

Testing

• mvn -pl core -am -DskipTests -Ddownload.plugin.skip=true install
• mvn -pl core -Ddownload.plugin.skip=true -Dtest=EngineTest test (2 tests passed)

[chadlwilson]

Thanks! This is probably the correct approach, but do you have any test evidence or information to supply that it actually fixes the linked issue without causing other problems, and making sure the DB is still closed correctly later on?

There are only unit and existing regression tests here; and no explanation of why keeping the DB open is the only thing preventing the analysis from continuing and surviving the later ensureDataExists() check.

In the log for the linked issue supplied at https://gist.github.com/OrangeDog/24ce9447e015184ccf85ac647e17749b the error is org.owasp.dependencycheck.exception.NoDataException: No documents exist

This seems to be happening because the database is null and failing the check below. Reopening the database at least allows the check to continue to database.dataExists()

DependencyCheck/core/src/main/java/org/owasp/dependencycheck/Engine.java

Lines 1142 to 1146 in a318a7e

	private void ensureDataExists() throws NoDataException {
	if (mode.isDatabaseRequired() && (database == null || !database.dataExists())) {
	throw new NoDataException("No documents exist");
	}
	}

[Eljees]

Thanks — the linked failure is caused by doUpdates(true) closing and nulling the database before rethrowing the UpdateException. analyzeDependencies() catches that exception as non-fatal, but then ensureDataExists() fails immediately because database == null, producing the reported NoDataException.

The current test verifies the immediate lifecycle invariant, but it does not yet exercise the full analyzeDependencies() path with existing local data. I’ll strengthen it to start with a populated local database, force a cached data-source update failure, and verify that analysis reaches the post-update data check using the existing database. I’ll also retain coverage for normal database closure when the engine is closed.

[chadlwilson]

Would appreciate without unhelpful copy-paste AI replies please. That's basically reiterating what I already said.

An extremely complicated automation integration test may not be justified, but would at least like to see examples of "thought* or adhoc testing. An UpdateException can happen for many reasons so changing the handling should require some evaluation that the database is in a safe state for re-opening and at least "likely to succeed" in normal scenarios (e.g timeouts from NVD API) without making things worse.

[Eljees]

Fair — and sorry, that reply restated your own analysis back at you.

Here is the actual reasoning, and what I did and did not run.

What the patch changes. Only the order of two statements in doUpdates (Engine.java L895-905): the recorded UpdateException is rethrown after openDatabase(true, false) instead of before it. The update loop, which exceptions get recorded, and the close/defrag path are untouched.

Why the database is in a safe state at that point.

• A data source failure is caught inside the loop (L886-891), the remaining sources still run, and the last exception is kept.
• After the loop the code takes the same path as a successful run: defrag() if anything was written, then database.close(); database = null;. The file is closed cleanly whether or not a source failed, so the reopen is not resuming a damaged connection — it is a fresh open of a closed file.
• openDatabase(true, false) on H2 copies the file to the temp directory and opens it with ACCESS_MODE_DATA=r and auto-update disabled (L986-1004), so analysis runs against a read-only snapshot.
• Content is still gated by ensureDataExists() (L1142): a run against an empty database still fails. The patch only allows continuing against an existing populated one.
• If there is no database file at all (first run, download failed before anything landed), openDatabase throws DatabaseException (L1003), which initializeAndUpdateDatabase treats as fatal (L698-700) — a clearer failure than today's NoDataException: No documents exist.

The scenario you named, an NVD timeout with existing local data. initializeAndUpdateDatabase already catches UpdateException and logs "Unable to update 1 or more Cached Web DataSource, using local data instead. Results may not include recent vulnerabilities." (L693-697). That is the documented intent, and today it cannot happen: by the time the warning is logged database is null, so ensureDataExists() throws and the run dies. With the patch the intent holds — the exception is still added to the exception collection, the warning is still logged, and the scan continues on local data.

Partial updates. If one source committed rows and a later one failed, the database holds those rows — the same state you get today, because the write happened before the exception surfaced. The only difference is whether the engine then reopens the file.

What I ran. The unit test in this PR pins the invariant: a data source that always throws makes doUpdates(true) throw while getDatabase() stays non-null. I have not built the full end-to-end case with a populated database plus a simulated update failure asserting that analyzeDependencies() completes with the exception in the collection — if you want that before this goes in, say so and I will add it.

To be straight with you: I use an LLM assistant for drafting, which is where that copy-paste tone came from. The analysis above and the test are mine.

[Eljees]

Pushed the test in 35f4556: EngineTest.testAnalysisContinuesOnLocalDataAfterUpdateFailure.

It starts from the populated test database, registers a single CachedWebDataSource whose update() always throws, scans a jar, and runs analyzeDependencies() with the network-reaching analyzers disabled so it works offline. It asserts that the run completes non-fatally with the UpdateException recorded first in the exception collection, that no NoDataException or DatabaseException is raised, that dataExists() is still true on the still-open database, and that Engine.close() is clean. Locally: Tests run: 3, Failures: 0.

Negative control: with the pre-patch statement order the same test fails exactly on the NoDataException: No documents exist escalation from the linked issue, so it pins this specific regression rather than passing vacuously.

On the earlier reply - sorry again, that one deserved the pushback. I do use an LLM assistant: my unaided code and prose are worse than what I get with it, so I lean on it and then verify the result myself. That is not a reason to paste a draft that restated your own analysis back at you, and I have stopped doing that.

[chadlwilson]

Thx, overall this seems reasonable to me and likely to do more "good" than harm, but I have not gone super deep into all the possible failure modes.

Right now there are some other problems with what happens when updates fail during multithreaded parts of the processing because in some cases the threads are not all interrupted and completed before the calling code starts trying to handle things. It is possible that when the database is reopened here, that background threads will then still be trying to interact with the DB even though the main processing has continued. There is some quite complex threading involved with parallel downloading and DB ingestion - hopefully that doesn't make things worse without addressing the "graceful closure" problem.

[Eljees]

Noted, and to be clear I am not claiming this addresses graceful closure - it does not touch thread lifecycle at all.

Two things that bound the risk you are describing, both already in doUpdates: the reopen happens inside the same WriteLock that wraps the whole update block, and openDatabase(true, false) on an H2 file connection does not reopen the database in place. It copies the data file to the temp directory, repoints H2_DATA_DIRECTORY at the copy and appends ACCESS_MODE_DATA=r to the connection string. So the handle the analysis continues on is a read-only copy, and a background thread still holding the old connection is acting on a different file rather than through this one.

That is a bound rather than a fix. If a straggler thread is still ingesting when database.close() runs a few lines earlier, the copy can be taken mid-write and the failure moves rather than disappears - the reopen just makes it surface as bad data instead of NoDataException. Happy to look at the interrupt/join side separately if you want to open an issue for it.

[Eljees]

@jeremylong this one is green on all 15 checks and approved; the only thing left is a review that counts toward the branch rule. If you want the interrupt/join side of the threading question tracked before this goes in, say so and I will open a separate issue for it rather than widen this PR.

[chadlwilson]

In any case, I am done responding here. Every reply appears to be AI speak which is overly confident and missing lots of context. Enough.

You know if I want to talk to an LLM I can do on my own, and probably prompt it much more effectively, right?

[Eljees]

@chadlwilson hey!
i'm just trying to fis some issues, i have met. sorry to break up workflow - but mots questions here are just abut code. Can we just proceed plz?

[chadlwilson]

I don't have merge permissions, only triage, so I'm not a gate you need to "pass" anyway, so feel free to ignore me.

This project doesn't have particular AI or contribution guidelines so I just establish my own boundaries based on the dynamics and experience I personally want to have in open source when I volunteer my time.

[Eljees]

@chadlwilson That's fair, and I'm not going to argue with where you draw your line — it's your time and your call.

For the record: the review you gave was useful and I did act on it — the ordering rationale and the regression test in 35f4556 came out of your questions. If the tone read as overconfident, that's on me, and I'm sorry it cost you the kind of exchange you wanted to have.

I won't ping you again on this one.

[Eljees]

@jeremylong bumping this one: 15/15 checks green, no conflicts, and the change is limited to the order of the update/analysis phases plus a regression test (EngineTest.testAnalysisContinuesOnLocalDataAfterUpdateFailure, 35f4556). It's approved by @chadlwilson but that doesn't satisfy the branch rule, so it needs a review from someone with merge rights.

Happy to split anything out if the scope looks too wide.