[#292] Clear stale MVV mark bits before pruning

[vharseko]

Fixes #292. Stacked on #288 (now merged) — this PR is a clean diff on master. Intended merge order: this PR → #291, so that by the time #291 documents icheck -p as the remediation path, pruning is already safe on affected volumes.

MVV.prune uses the 0x8000 mark bit in the version length field as transient private state: pass 1 marks the versions to keep, pass 3 removes the unmarked ones. This assumes no version is marked on entry. A volume corrupted by a prune interrupted before the #288 fix violates that assumption, and the marked counter only counts marks set by the current run while isMarked() sees any bit:

1. Wrong-version promotion. In the primordial-conversion path the scan promotes the first marked version, so a stale mark on an obsolete version resurrected the old value (or a zero-length undefined value) while the most recent committed version was silently dropped.
2. Leaked accounting. The dropped keeper was marked in pass 1, so pass 2 never added it to prunedVersionList — the MVV count was never decremented and a long record's page chain was never deallocated.
3. Milder variant in the multi-version path. A stale-marked dead version was treated as a keeper and survived one extra prune round with its accounting deferred, while the first prune after corruption reported a clean result.

The fix sweeps all mark bits in the region clean before pass 1, mirroring the finally-block safety net, so mark state is guaranteed consistent regardless of what is on disk. Following review, the sweep is gated on MVV.verify, hoisted from the existing debug assert — its argument was already evaluated on every prune call, so the check adds no cost. A stale-marked but well-formed MVV still passes verify (the length accessors mask the mark bit) and still gets swept; a malformed MVV is never swept — it is rejected by pass 1's guard with a CorruptValueException and left byte-for-byte unchanged. Without the gate, a corrupt length field could misalign the sweep's traversal and unmark() would clear bit 15 of a value-payload byte inside the region.

Four regression tests: three cover each scenario above (with the sweep disabled they fail exactly as described in the issue — obsolete value promoted, undefined promoted, dead version surviving the prune), and pruneMustNotSweepMisalignedMvv covers the misaligned case (with the verify gate removed it fails with a payload byte flipped 0xFF → 0x7F). The tests are self-contained — the no-marks-remain assertion walks the versions with MVV.isMarked directly, so this PR does not depend on the countMarkedVersions scan introduced in #291. MVVTest 43/43; related suites (IntegrityCheckTest, MVCCBasicTest, MVCCPruneTest, MVCCPruneBufferTest, TransactionIndexTest, Bug1017957Test) all green.

Pre-existing limitation, unchanged by this PR: Buffer.pruneMvvValuesHelper dirties the page only when prune changes the value's size (newSize != oldSize), so a mark-only repair is not persisted until the next size-changing prune — icheck keeps reporting the volume until then. Pass 3 behaved the same way before the sweep.

Detection of the corruption is covered by #290/#291 (icheck fault + MarkedVersions counter); this PR makes the remediation path itself safe.

[maximthomas]

The diagnosis and the fix are right, and the three regression tests are real: with the sweep deleted they fail exactly as described (expected:<3> but was:<2>, expected:<3> but was:<0>, both dead versions must be pruned in one round expected:<14> but was:<25>). MVVTest is 42/42 with it. One change requested before merge.

Sweep writes into value bytes of a misaligned MVV (moderate)

persistit/core/src/main/java/com/persistit/MVV.java:453-456 traverses with no alignment validation, unlike pass 1. On a corrupt MVV whose length field misaligns the traversal, unmark() clears bit 15 of a byte that is value payload, not a version header — inside the region, so #288's pruneExceptionMustNotUnmarkPastRegionEnd does not catch it.

This is a new exposure, not inherited. The old finally net is gated on marked > 0; the sweep is unconditional. Probe: offset=7, v1 = 4 bytes, v2 = 20 bytes of 0xFF, both versions ABORTED so pass 1 marks nothing, then MVV.putLength(bytes, offset + 1, 9) to misalign.

	result
base d6ce5c9c6	CorruptValueException, array unchanged
this PR 14f35f8f4	CorruptValueException, byte at index 35 changed 0xFF → 0x7F

Trace: from = 8 → unmark (no-op) → from = 8 + 9 + 10 = 27; 27 + 10 <= 52 passes, so unmark(bytes, 27) writes the char at bytes[35..36], which is v2's data. getLength(27) then reads 0x7FFF and from jumps to 32804, where pass 1's guard finally throws — after the write.

Buffer.pruneMvvValuesHelper runs on every cleanup pass and on icheck -p, so an already-corrupt record gets a data byte flipped before the corruption is reported.

MVV.verify (persistit/core/src/main/java/com/persistit/MVV.java:628) is exactly the needed check — it requires exact alignment and masks the mark bit, so a stale-marked but well-formed MVV still passes and still gets swept. prune already calls it at line 427 under Debug.$assert0.t, so hoisting it costs nothing in a Debug build:

final boolean wellFormed = verify(bytes, offset, length);
Debug.$assert0.t(wellFormed);   // replaces the existing assert at MVV.java:427

...

if (wellFormed) {
    int sweep = offset + 1;
    while (sweep + LENGTH_PER_VERSION <= offset + length) {
        unmark(bytes, sweep);
        sweep += getLength(bytes, sweep) + LENGTH_PER_VERSION;
    }
}

Skipping the sweep on a malformed region just defers to pass 1's guard, which throws anyway. Worth a test alongside pruneExceptionMustNotUnmarkPastRegionEnd covering intra-region misalignment, since that one only guards the region end.

Nits

• from reuse: persistit/core/src/main/java/com/persistit/MVV.java:453-457 borrows pass 1's loop variable and resets it with from = offset + 1;. A dedicated local (sweep above) reads better and drops the reset line.
• Comment should state why the sweep is separate: folding unmark(bytes, from) into pass 1's loop head would be free — pass 1 visits every version and getLength already masks the mark. The separate pass buys one thing: it repairs the whole region even when pass 1 throws early. Worth saying, since it is the only reason not to inline it.
• PR description overstates the bound: "the sweep never touches bytes outside the MVV region even on corrupt input" is true of the region end only; see above.
• Stale assertion message: pruneExceptionUnmarksVersionsAtNonZeroOffset still asserts "prune must leave the byte array unchanged when it throws". It passes (nothing is marked on entry) but now claims more than the amended javadoc promises.
• Pre-existing, not this PR — worth a line in the description: Buffer.pruneMvvValuesHelper:3700 sets changed only when newSize != oldSize, and pruneMvvValues:3624 dirties the page only when changed. When every version is a keeper, prune clears the stale mark but returns the same size, so the repair is never persisted and icheck keeps reporting the volume. Pass 3 already behaved this way before the sweep (MVV.java:585-587), so nothing regresses — but since #291 will point users here as the remediation, the limitation is worth naming.

[vharseko]

All points addressed in d65c96c.

Sweep writes into value bytes of a misaligned MVV — confirmed your probe exactly as described before fixing: with the unconditional sweep, unmark(bytes, 27) flipped the payload byte at index 35 (0xFF → 0x7F) before pass 1's guard threw at index 32804; on the base the same input threw with the array untouched. Fixed as suggested: verify is hoisted into final boolean wellFormed and the sweep is gated on it. One note — the hoist is free in any build, not just a Debug one: Debug.$assert0.t(verify(...)) evaluates its argument eagerly, so verify already ran on every prune call and only the boolean was discarded. A stale-marked but well-formed MVV still passes verify (the length accessors mask the mark bit) and still gets swept — the three #292 regression tests are unchanged and green.

Test — added pruneMustNotSweepMisalignedMvv alongside the region-end test: two aborted versions, the first version's length field corrupted 4 → 9, asserting CorruptValueException with a byte-for-byte unchanged array. With the gate removed it fails exactly per your table: arrays first differed at element [35]; expected:<-1> but was:<127>.

Nits — the sweep uses a dedicated sweep local (no from reset); the block comment now states why the sweep is a separate pass (it repairs the whole region even when pass 1 throws early) and why it is gated; the assertion message in pruneExceptionUnmarksVersionsAtNonZeroOffset claims only what the amended javadoc promises (unchanged when no stale marks are present on entry); the PR description drops the overstated region bound and names the pre-existing pruneMvvValuesHelper newSize != oldSize limitation.

MVVTest 43/43; IntegrityCheckTest, MVCCBasicTest, MVCCPruneTest, MVCCPruneBufferTest, TransactionIndexTest, Bug1017957Test all green on the branch.