[#290] Report leftover MVV mark bits in IntegrityCheck#291
Conversation
maximthomas
left a comment
maximthomas
left a comment
There was a problem hiding this comment.
The detection half is solid — countMarkedVersions terminates safely (getLength() masks to 0x7FFF, so from advances by at least LENGTH_PER_VERSION), the dedicated scan is the right call over extending VersionVisitor, and MVVTest (40/40) + IntegrityCheckTest (10/10) are green on the branch.
The problem is the remediation the PR prescribes. Requesting changes on that.
icheck -p remediation silently resurrects deleted keys (high)
The PR says "icheck -p remains the remediation path … marks are reported first and then rewritten by the prune." That is not safe: MVV.prune uses the same bit as its private "keep this version" flag and never clears pre-existing marks before pass 1. A leftover mark is therefore read as prune's own decision to keep:
persistit/core/src/main/java/com/persistit/MVV.java:530-546— the primordial branch promotes the first marked version it finds. With a stale mark on an older version, the obsolete version wins. Note the branch is still entered:markedcounts only prune's ownmark()calls (1), while two versions actually carry the bit.persistit/core/src/main/java/com/persistit/MVV.java:508-524— pass 2 registers only!isMarkedversions inprunedVersionList. The marked version that pass 3 doesn't promote is therefore discarded without ever being registered, so its long-record page (lines 515-520) is never deallocated. Observed:prunedVersionListis empty in the stale-mark run vs. one entry in the control.
Reproduced on this branch at unit level (two committed versions, mark on the older one, MVV.prune called directly):
leftoverMark=false survivor=[22 22] prunedVersionList.size=1 <- correct, newest version
leftoverMark=true survivor=[11 11 11] prunedVersionList.size=0 <- stale version promoted
And end-to-end through the exact path the PR recommends — testPruneClearsLeftoverMarkBits plus a before/after snapshot of the visible key set, with a control run that calls corrupt5's ex.store() but skips MVV.mark:
>>> doMark=false markedVersions=0 faults=0 keysBefore=500 keysAfter=500
>>> doMark=true markedVersions=10 faults=10 keysBefore=500 keysAfter=505
icheck -p undeleted 5 transactionally removed keys. The mark sat on the value version, so prune promoted it instead of the AntiValue. The control rules out corrupt5's non-transactional store as the cause.
The bug lives in MVV.prune and predates this PR, but this PR is what tells operators to run icheck -p on affected volumes, and testPruneClearsLeftoverMarkBits asserts remediation "works" while only checking counters. Suggested:
- Make
prunedefensive about its own invariant — clear any pre-existing marks before pass 1 (you already have the scan):
// MVV.prune, before the first pass
if (countMarkedVersions(bytes, offset, length) > 0) {
int from = offset + 1;
while (from + LENGTH_PER_VERSION <= offset + length) {
final int vlength = getLength(bytes, from);
unmark(bytes, from);
from += vlength + LENGTH_PER_VERSION;
}
}- Add the data-preservation assertion to
testPruneClearsLeftoverMarkBitsin
persistit/core/src/test/java/com/persistit/IntegrityCheckTest.java:132— snapshot the visible key/value set before corruption, compare after remediation. That single assertion is what surfaces this.
icheck -P (pruneAndClear) regresses on affected volumes (medium)
_faults is never cleared (reset() at persistit/core/src/main/java/com/persistit/IntegrityCheck.java:786 doesn't touch it), and the new fault is added during buffer.verify — i.e. before the prune in the same verifyPage call. So the gate at persistit/core/src/main/java/com/persistit/IntegrityCheck.java:315 can never pass on an affected volume: resetMVVCounts is skipped even though the prune fully rewrote the marks.
Confirmed by running the real runTask() path (icheck trees=* -u -P -v), against a control that stores the same records without setting the mark bit:
>>> doMark=false marked=0 faults=0 -> "0 aborted transactions were cleared by pruning"
>>> doMark=true marked=10 faults=10 -> "PruneAndClear failed to remove all aborted MMVs"
Before this PR the same volume ran -P to completion.
Options: keep marked-version faults in a separate list excluded from the -P gate; re-verify after pruning and drop the fault when the count reaches zero; or at minimum document "run icheck -P twice".
Marked-version count runs on an unvalidated MVV (low)
persistit/core/src/main/java/com/persistit/IntegrityCheck.java:206 calls countMarkedVersions before visitAllVersions validates the record, so a structurally corrupt MVV is walked with garbage lengths. Confirmed — an MVV with only a damaged length field and no mark bit anywhere:
intact -> marked=0
corrupt -> marked=1
That yields a bogus MVV has 1 version with a leftover mark bit fault and an inflated MarkedVersions counter next to the genuine CorruptValueException fault — misleading exactly when an operator is using the counter to answer "am I affected by #286?". The inline comment says the count "must be counted before visitAllVersions", but there is no such constraint — visitAllVersions does not mutate bytes. Counting after a clean traversal (or sharing one traversal) fixes it.
Nits
- Doc drift:
persistit/doc/Management.rst:136documents theicheck -cCSV header verbatim and isn't updated withMarkedVersions. Worth noting in the PR description that both the CSV columns and the plain-report format changed — breaking for anything parsing icheck output. - Redundant page tracking:
verifyPagealready holdspageatpersistit/core/src/main/java/com/persistit/IntegrityCheck.java:1187; a field set there would remove thevisitPageoverride and the duplicated_currentPagestate. - Copyright header:
MVV.javaandMVVTest.javacarryPortions Copyrighted 2026 3A Systems, LLC;persistit/core/src/test/java/com/persistit/IntegrityCheckTest.javais modified without it (recent commits3f8c98909,cca1422d7do add it). corrupt5duplication: it is a byte-for-byte copy ofcorrupt2except for the mutation — a shared helper taking the mutation would be cleaner. Both also leakignoreMVCCFetch(true)if the loop throws (nofinally).- Test granularity:
testLeftoverMarkBitsReportedmarks one version per record, sofaults == markedVersionCountby construction. Marking two versions in one record would cover the counter/fault distinction and theplural()branch. - Line length:
persistit/core/src/main/java/com/persistit/IntegrityCheck.java:209is 121 chars, one over the file's 120-col wrap (cosmetic — no formatter config in the repo).
MVV.prune's finally-block safety net used the wrong loop bound (index < length instead of index < offset + length), so a prune that exited via an exception - e.g. PersistitInterruptedException while resolving a commit status under interrupt - left mark bits set in the live buffer page whenever the MVV sat at a non-zero in-page offset. The fetch paths (visitAllVersions, fetchVersion, fetchVersionByOffset) then read the version length signed and with the mark bit included, driving the scan offset negative and crashing with ArrayIndexOutOfBoundsException, as seen in TransactionTest2.transactionsWithInterrupts on Windows JDK 11. - Make the finally-block unmark loop in MVV.prune mirror the first pass exactly - same advance, same guard, with the array-end bound covering the unguarded first iteration - and remove its early break on zero-length versions (a legal undefined value) - Tighten the first pass's corruption guard so a misaligned traversal throws CorruptValueException before it can read or mark outside the MVV region - Read version lengths unsigned with the mark bit stripped in visitAllVersions, fetchVersion and fetchVersionByOffset - Throw CorruptValueException instead of AIOOBE when a version header or value extends past the MVV bounds - Fix storeVersion reading target[0] instead of target[targetOffset] and exactRequiredLength's loop bound ignoring targetOffset when the MVV sits at a non-zero offset - Add deterministic regression tests for marked-version reads and for the interrupted-prune unmark safety net; make transactionsWithInterrupts assert no worker died with an unexpected exception, reporting the first failure, with shared counters reset in @before Fixes OpenIdentityPlatform#286
69b3e44 to
d9ed39f
Compare
MVV.prune uses the 0x8000 mark bit in the version length field as transient private state and assumes no version is marked on entry. A volume corrupted by a prune interrupted before the OpenIdentityPlatform#288 fix violates that assumption: a leftover mark on an obsolete version won the primordial-conversion scan, resurrecting the old value (or an undefined value) while silently dropping the most recent committed version with no PrunedVersion accounting for it — leaking the MVV count and, for long records, the page chain. In the multi-version path a stale-marked dead version survived one extra prune round with its accounting deferred. Sweep all mark bits in the region clean before the first pass, so mark state is guaranteed consistent regardless of what is on disk. The sweep never touches bytes outside the MVV region even on corrupt input. Fixes OpenIdentityPlatform#292
A prune interrupted before the issue OpenIdentityPlatform#286 fix could leave 0x8000 mark bits behind in on-disk version length fields. Since PR OpenIdentityPlatform#288 the read paths strip the mark bit silently, so such versions read normally and icheck reported affected volumes as clean. Add MVV.countMarkedVersions and scan every data record during icheck: marked versions are counted in a new MarkedVersions counter (report, CSV and getMarkedVersionCount) and reported as a fault so affected volumes can be identified. The scan runs only after visitAllVersions has validated the version structure, so a structurally corrupt MVV cannot produce a bogus marked-version fault. icheck -p remains the remediation: pruning rewrites the marked versions (issue OpenIdentityPlatform#292 makes prune clear stale marks unconditionally). With icheck -P the marked-version faults are reported but do not block clearing the TransactionIndex, since the prune pass has already rewritten the marks. The icheck -c CSV example in Management.rst is updated for the new MarkedVersions column. Fixes OpenIdentityPlatform#290
d9ed39f to
79b2cd7
Compare
|
Thanks for the thorough review — the prune reproduction especially. All points addressed; the branch is force-pushed as 79b2cd7 on a reordered stack.
Count on an unvalidated MVV (low) — fixed: Nits:
Local runs on the new head: |
|
@maximthomas ready for another look — all review points are addressed, see the point-by-point summary above (#291 (comment)). New head is 79b2cd7. Highlights:
#293 would need your review as well — it carries the actual |
2 participants
Summary
Implements the proposal from #290 (split out of PR #288, suggested by @maximthomas in the #288 review):
IntegrityChecknow detects and reports MVV versions whose length field still carries a leftover0x8000mark bit — the on-disk residue of a prune interrupted before the #286 fix. Since PR #288 the read paths strip the mark bit silently, so such versions read normally andicheckreported affected volumes as clean; the mark survived until some future prune rewrote that version.Changes
MVV.countMarkedVersions(bytes, offset, length): a dedicated scan counting versions with the mark bit set. A separate scan rather than an extension ofMVV.VersionVisitor: the interface has six implementations, andsawVersionhands out the post-header value offset (with no header at all for primordial values), so a visitor cannot see the mark anyway.IntegrityCheck:visitDataRecordscans each record for marks aftervisitAllVersionshas validated the version structure, so a structurally corrupt MVV cannot produce a bogus marked-version fault next to its genuineCorruptValueExceptionfault. Marked versions feed a newMarkedVersionscounter (plain report, CSV, and agetMarkedVersionCount()getter) and add aFault("MVV has N versions with a leftover mark bit") with the page address and record position, sohasFaults()flags affected volumes.icheck -premains the remediation path:verifyPagerunsbuffer.verifybefore pruning, so marks are reported first and then rewritten by the prune ([persistit] MVV.prune can promote a stale-marked old version during primordial conversion #292 makes prune clear stale marks unconditionally).icheck -P(pruneAndClear): marked-version faults are reported but do not blockresetMVVCounts— by the time the run completes, the prune pass has already rewritten the marks, so only other faults and incomplete pruning count as failures. Without this,-Pcould never succeed on an affected volume.Tests
MVVTest:countMarkedVersions(mark/unmark transitions over a two-version MVV) andcountMarkedVersionsNonMVV(non-MVV, zero and negative lengths).IntegrityCheckTest:corrupt2/corrupt5share acorruptMVVshelper taking the mutation as a callback, withtry/finallyaroundignoreMVCCFetch.corrupt5marks every version of each corrupted MVV, so the marked-version count exceeds the fault count (one fault per record) and the plural fault message is exercised.testLeftoverMarkBitsReportedasserts a clean tree reports nothing and a corrupted tree reports exactly one fault per corrupted record plus the exact marked-version total.testPruneClearsLeftoverMarkBitssnapshots the visible key/value contents before corruption and asserts remediation (icheckwith pruning) leaves them unchanged, then that a subsequenticheckis clean.testPruneAndClearWithLeftoverMarkBitsruns the real task path (CLI.parseTask("icheck trees=* -u -P")) and asserts the faults are reported while the TransactionIndex is still cleared ("aborted transactions were cleared by pruning", not "PruneAndClear failed").Local runs:
IntegrityCheckTest11/11,MVVTest44/44,MVCCBasicTest+MVCCPruneTest+MVCCPruneBufferTest+TransactionIndexTest+Bug1017957Testall green (1 pre-existing skip).Fixes #290