Follow-ups from #5764: CHECK DATABASE materialises every record twice, two arms still say "removing it" when nothing is removed, and three smaller checker/test cleanups

[lvca]

Discoveries from #5764 (merged as c7349f8b7, PR #5767) that were out of that issue's scope. Grouped into one issue because four of the five live in the same two files. Line numbers are against c7349f8b7.

Ordered by value. Item 1 is the only one with a measurable payoff; the rest are correctness-of-message and cleanup.

---

1. CHECK DATABASE materialises every vertex and edge record twice

This fell out of proving item 4 of #5764 (that the RECORD-scoped arm running one pass is equivalent to the type-wide arm running two). The corollary is the interesting half: if one pass is enough for the scoped arm, the type-wide arm's first pass is doing work its second pass already does.

GraphDatabaseChecker.checkVertices (and checkEdges, identically) runs:

• pass 1 — GraphDatabaseChecker.java:484 / :1277: a bucket scan that materialises each record with newImmutableRecord(database, type, rid, view, null) and calls asVertex(true) / asEdge(true);
• pass 2 — database.scanType(typeName, false, record -> checkConnectivity.accept(record), errorCallback).

But LocalDatabase.scanType (LocalDatabase.java:657) is itself b.scan(...) → newImmutableRecord(wrappedDatabaseInstance, type, rid, view, null) — the same construction, from the same raw page view — and checkConnectivity opens with the same asVertex(true), which is what forces the buffer decode.

Failure detection is equally identical: newImmutableRecord is invoked inside scanType's callback, which sits inside LocalBucket.scan's per-slot try, so a materialisation failure is routed to the errorRecordCallback that already warns and flags the record corrupted. Pass 1 therefore detects nothing pass 2 misses.

The cost is not hypothetical — the progress total is written as exactly that:

progressBegin(progressStepName, 2 * database.countType(typeName, false));   // :477 and :1270

Dropping pass 1 should roughly halve the record materialisation of a full CHECK DATABASE on a graph.

Two things to settle before doing it:

• Benchmark first. The equivalence above is verified statically only. Nobody has measured what fraction of a real CHECK DATABASE is materialisation versus the connectivity walk and the edge sweep, so "halves the check" is an upper bound on the pass, not a claim about wall clock.
• It changes the output. The two passes emit different warning strings for the same record — "vertex <rid> cannot be loaded, removing it" from pass 1 versus "vertex <rid> cannot be loaded (error: ...)" from pass 2's error callback. corruptedRecords is a set so the corrupted total is unaffected, but totalWarnings would drop by one per corrupt record and one message would disappear. That is arguably an improvement (see item 2), but it is user-visible.

2. checkVertices and checkEdges still say "removing it" when nothing is removed

GraphDatabaseChecker.java:487 and :1280:

addWarning(warnings, totalWarnings, maxWarnings, "vertex " + rid + " cannot be loaded, removing it");
addWarning(warnings, totalWarnings, maxWarnings, "edge " + rid + " cannot be loaded, removing it");

Removal happens only under fix (the for (final RID rid : corruptedRecords) delete loop), so a plain CHECK DATABASE reports "removing it" and removes nothing.

#5764 fixed exactly this wording on the document arm, because that was all it asked for — there the message was wrong unconditionally, since nothing on that path ever removes anything. The vertex and edge arms have the conditional version of the same defect. Either drop the clause or make it conditional on fix.

Note the interaction with item 1: if pass 1 goes away, these two messages go with it, since the surviving pass-2 wording is already correct.

3. Four tests reflect on LogManager's private logger field, which has a public accessor

engine/src/test/java/com/arcadedb/schema/LocalSchemaOrphanIndexSelfHealTest.java:99
engine/src/test/java/com/arcadedb/index/vector/LSMVectorIndexBruteForceScanTest.java:132
engine/src/test/java/com/arcadedb/index/vector/LSMVectorIndexRecoveryTest.java:1656
engine/src/test/java/com/arcadedb/query/QueryEngineManagerPoolTest.java:250

All four do readField(LogManager.instance(), "logger") to keep the original logger before swapping in a capturing one. LogManager.getLogger() is public and documented for precisely this ("so a caller that temporarily replaces it ... can put the original back"). The reflection would break on a field rename with no compile error. #5764's new test uses the accessor; these four predate it.

While there, worth recording in those tests what the swap actually is: LogManager is a singleton, so installing a capturing logger is process-wide for the duration. All four restore it in a finally and are safe today only because surefire runs test classes sequentially within a fork. If class-level parallelism is ever enabled in the engine module, a concurrent test's WARNING output lands in the capturing list and the delegate becomes whatever that test installed. There is no per-invocation seam to capture through — the log call sites go straight to the singleton — so the constraint is worth a comment rather than a fix.

4. The two addCorrupted implementations disagree past the cap

• GraphDatabaseChecker.addCorrupted (:1377): at/over maxCorrupted it increments unconditionally, so a RID still present in the retained set is counted again.
• DatabaseChecker.addCorrupted (added by Follow-ups from #5680: point the repair advice at CHECK DATABASE RECORD, keep the conflict's cause, and two checker asymmetries #5764): checks corrupted.contains(rid) first, so a RID still in the set is deduped even past the cap.

The second is the better behaviour and its Javadoc explains the limit (exact under the cap, degraded past it; the only exact fix is a second unbounded set, which is the memory the cap exists to refuse). Neither is a bug on any path reachable today. The concern is just that two near-identical helpers with different semantics invite a future "consistency fix" in the wrong direction. Align them on the contains-checking version.

5. totalWarnings counts occurrences while the retained warnings is a Set

DatabaseChecker publishes warnings as a LinkedHashSet<String>, so two findings that render to the same string collapse to one message, while totalWarnings increments for each. The totals can therefore exceed the retained set size well before maxWarnings is reached.

Pre-existing and not introduced by #5764, but that PR routed the type-wide document path through the same counter, so it now feeds it too. Harmless unless someone asserts the totals for exactness. Smallest of the five — fold into item 4 rather than fix separately.

---

Not a follow-up, but worth knowing

#5764 changed CHECK DATABASE output for anyone parsing it programmatically: the unloadable-document message is now document <rid> cannot be loaded (was vertex <rid> cannot be loaded, removing it), and totalWarnings/totalCorruptedRecords are non-zero on a type-wide run that finds one (they were previously bypassed). Documented in docs/release-26.8.1.md.

[lvca]

Fixed by #5776, merged as 876635c43.

All five items landed. Two of them did not turn out the way this issue predicted, so recording that here rather than only in the release notes:

Item 1 - the payoff is ~7%, not "roughly halves". This issue asked for a benchmark before claiming one, and it was right to. On a warm 200k-vertex / 200k-edge graph with no super-node: vertices 173 ms -> 162 ms, edges 155 ms -> 142 ms, full CHECK DATABASE 509 ms -> 472 ms. The record-materialisation count does halve, but materialisation was never what the step spent its time on - asVertex(true) parses the edge pointers, not the properties. (A super-node fixture is useless for measuring this: containsEdge()'s O(degree) walk costs 108 s against 0.5 s for the same record count, and swamps everything.)

The pass was still worth deleting, for a reason under-weighted above: it passed a null error callback, so it saw less than the pass that survives it - a failure inside the bucket machinery (placeholder resolution, a multi-page chain read) was only logged, never reported. Dropping it improves detection.

Item 2 needed no wording decision. The issue offered "either drop the clause or make it conditional on fix". Neither: both "removing it" messages were emitted only by the pass item 1 removes, so they left with it. The surviving wording already names the failure.

Item 4 got a better answer than alignment. Aligning the two addCorrupted helpers would have left two lookalikes free to drift again - which is what item 4 was actually worried about. Instead the rule now exists once, in CollectionUtils.addBounded, returning a three-valued BoundedAdd so no caller re-derives any part of it. That collapsed four hand-written copies, not two; item 5 folded into the same extraction.

Item 3 replaced the reflection with LogManager.getLogger() at all four sites and recorded the process-wide-singleton caveat at each.

Two output changes for anyone parsing CHECK DATABASE, both in docs/release-26.8.1.md: totalWarnings now counts distinct messages rather than occurrences, and a capped and quiet run no longer logs the warnings it had to drop from the graph arm (the document arm always honoured verboseLevel 0; they now agree). A default CHECK DATABASE runs at verbosity 1 and is unaffected.

Spawned #5777 - checkEdges flags a missing IN endpoint's vertex RID as corrupted but a missing OUT endpoint's not, and flagging either contradicts the "a RID that is not there is not corruption" principle from #5680/#5764. Left out of that PR because it changes what FIX rebuilds.