[#286] Fix MVV marked-version corruption after interrupted prune

[vharseko]

Summary

Fixes the residual interrupt-corruption failure from #286: TransactionTest2.transactionsWithInterrupts intermittently crashed with ArrayIndexOutOfBoundsException: Index -32745 out of bounds in MVV.visitAllVersions during an MVCC fetch (Windows / JDK 11, interrupt stress).

Root cause

Two cooperating defects in MVV:

1. Write side (the actual corruption). MVV.prune's finally-block safety net, which is supposed to remove transient mark bits when prune exits via an exception, used the wrong loop bound: while (index < length) instead of while (index < offset + length). For in-page pruning (Buffer.pruneMvvValuesHelper passes a large in-page offset), the loop body never ran, so an interrupted prune (PersistitInterruptedException out of TransactionIndex.commitStatus, a TimeoutException, or a CorruptValueException) left 0x8000 mark bits set in the version length fields of the live buffer page. The exclusive claim held during pruning means this leftover state — not any transient race — is the only way readers can ever observe marks. The observed stack (the crash surfaced in the main thread's final balance scan, after all workers had died) confirms the marks persisted in the page.

2. Read side (the crash). The fetch paths — visitAllVersions, fetchVersion, fetchVersionByOffset — read the per-version length with signed Util.getShort and without stripping the mark bit, unlike the canonical getLength() accessor (Util.getChar & MAX_LENGTH_MASK). A marked version therefore produced a negative length (L | 0x8000 sign-extended), drove the scan offset negative, and the next Util.getLong threw a raw AIOOBE. The observed index -32745 matches this arithmetic exactly (offset_ver + 10 + L - 32768 with offset_ver + L = 13).

Changes

• MVV.prune: make the finally-block unmark loop mirror the first pass exactly (same advance, same guard, with the array-end bound covering the unguarded first iteration) and tighten the first pass's corruption guard so a misaligned traversal throws CorruptValueException before it can read or mark outside the MVV region; drop the early break on zero-length versions (a zero-length version is a legal undefined value, and the loop always advances by at least LENGTH_PER_VERSION).
• MVV.visitAllVersions / fetchVersion / fetchVersionByOffset: read version lengths unsigned with the mark bit stripped, matching getLength(). Leftover marks (including any already persisted to disk) become harmless — the true length is read.
• Add bounds checks to the scan loops and the by-offset copy: a version header or value extending past the MVV bounds now throws a clean CorruptValueException instead of escaping as an AIOOBE.
• MVV.storeVersion: fix a latent bug found while writing the regression test — the existing-MVV detection read target[0] instead of target[targetOffset], so at a non-zero offset an existing MVV was treated as a primordial value and nested. (Unreachable in production today: Exchange merges MVVs in spare buffers at offset 0 and writes pages via RawValueWriter.)

Tests

• MVVTest: six new deterministic regression tests — marked-version reads through all three fetch paths, corrupt-length inputs now throwing CorruptValueException, and two prune safety-net tests: pruneExceptionUnmarksVersionsAtNonZeroOffset reproduces the interrupted-prune scenario (two uncommitted versions at a non-zero offset → prune throws after marking) and asserts the byte array is left unchanged, while pruneExceptionMustNotUnmarkPastRegionEnd covers the misaligned-MVV case where the net must stop at the same point as the first pass's corruption guard instead of writing past the region end. All six fail on the previous code; visitMarkedVersion reproduces the CI signature (ArrayIndexOutOfBoundsException: Index -32747).
• TransactionTest2.transactionsWithInterrupts: reset the shared static counters at the start and assert _failedTransactionCount == 0 at the end, so a worker dying with an unexpected exception fails the test loudly instead of only printing a stack trace.

Local runs: MVVTest 39/39, MVCCBasicTest + MVCCPruneTest + MVCCPruneBufferTest + MVCCConcurrentTest + TransactionIndexTest 97 green (1 pre-existing skip), TransactionTest2 5/5.

Fixes #286

[maximthomas]

Good fix — root cause is correct and I verified it against the tree: Buffer.pruneMvvValuesHelper (persistit/core/src/main/java/com/persistit/Buffer.java:3697) does pass a large in-page offset, so the old while (index < length) ran zero iterations there. No other file in core/src/main reads MVV lengths with signed Util.getShort, so the read-side fix is complete.

One issue I'd like addressed before merge, plus some smaller items.

---

1. The new finally bound is stricter than pass 1 — it can now leave a mark behind

persistit/core/src/main/java/com/persistit/MVV.java — prune()

Pass 1 iterates on from < offset + length; the new safety net iterates on index + LENGTH_PER_VERSION <= offset + length. Those are not the same set of indices, and the safety net must undo exactly what pass 1 did.

For a misaligned (corrupt) MVV — precisely the case this path exists for — pass 1's guard only rejects from > offset + length, so from can legally land inside the last 10 bytes. Example, offset = 0, length = 20, first version has vlength = 4:

• pass 1: from = 1 + 4 + 10 = 15; 15 > 20 is false, so it iterates again, and may mark(bytes, 15) (writing bytes 23–24, i.e. outside the MVV region) before throwing "Multiple uncommitted versions".
• new finally: 15 + 10 <= 20 is false → loop exits → mark left set.
• old code: 15 < 20 was true → it did unmark. So this is a regression at offset == 0.

Suggested bound — mirrors pass 1's iteration, still never reads past the array:

if (marked > 0) {
    int index = offset + 1;
    while (index < offset + length && index + LENGTH_PER_VERSION <= bytes.length) {
        final int vlength = getLength(bytes, index);
        unmark(bytes, index);
        index += vlength + LENGTH_PER_VERSION;
    }
}

Alternative (bigger, but removes the whole class of problem): tighten pass 1's guard so a misaligned traversal is impossible, then both bounds agree:

from += vlength + LENGTH_PER_VERSION;
if (from != offset + length && from + LENGTH_PER_VERSION > offset + length) {
    throw new CorruptValueException("MVV Value is corrupt at index: " + from);
}

Note Debug.ENABLED == false, so the Debug.$assert0.t(...) in that block is a no-op and catches none of this.

---

2. Unfixed twin of the storeVersion bug

persistit/core/src/main/java/com/persistit/MVV.java:170 — exactRequiredLength has the same non-zero-offset defect you just fixed in storeVersion:

int offset = targetOffset + 1;
while (offset < targetLength) {          // should be: offset < targetOffset + targetLength

…and the return targetLength - valueLength + newVersionLength likewise ignores targetOffset.

Same reachability caveat as storeVersion: only via ValueHelper.MVVValueWriter, and Exchange._mvvValueWriter (persistit/core/src/main/java/com/persistit/Exchange.java:302) is declared but never used anywhere — I checked case-insensitively, declaration only. But persistit/core/src/main/java/com/persistit/Buffer.java:1636/1681/1991 do pass non-zero offsets into requiredLength/storeVersion through the ValueHelper interface; it's safe only because the MVV implementation is dead. Since you're already fixing one half, please fix both — or leave a // requires targetOffset == 0 note.

---

3. fetchVersionByOffset — reuse the canonical accessor

persistit/core/src/main/java/com/persistit/MVV.java

// current
final int length = (offset == 0) ? sourceLength : (Util.getChar(source, offset - LENGTH_VALUE_LENGTH) & MAX_LENGTH_MASK);

// suggested
final int length = (offset == 0) ? sourceLength : getLength(source, offset - LENGTH_PER_VERSION);

Exactly equivalent (getLength reads at o + LENGTH_VERSION, and offset - 10 + 8 == offset - LENGTH_VALUE_LENGTH), and it uses the accessor whose consistent use is the whole point of this PR. Also drops the line from ~129 chars back under the file's ~120-col norm — it's currently the longest line in MVV.java.

---

4. visitAllVersions — trailing check is now dead

if (offset != end) {
    throw new CorruptValueException("invalid length in MVV at offset/length=" + ...);
}

The new per-version guard enforces offset + valueLength <= end, and the loop exits only when offset >= end, so offset == end always. Harmless, but it's no longer what catches truncation — worth a comment or removal so the next reader doesn't assume it's live.

---

5. visitOverrunningLengthThrows doesn't fail on the old code

persistit/core/src/test/java/com/persistit/MVVTest.java

The description says all five new tests fail before the fix; this one doesn't. Old code: offset += 50 → 61 != 14 → the trailing check threw the same CorruptValueException. (TestVisitor.sawVersion only writes to a map, so nothing throws earlier.)

The behaviour actually fixed is that sawVersion(version, 11, 50) is no longer invoked with an out-of-bounds length before throwing — which matters, because Exchange.MvvVisitor records that offset/length and fetchVersionByOffset would then copy 50 bytes. Suggest asserting that:

final TestVisitor visitor = new TestVisitor();
try {
    MVV.visitAllVersions(visitor, source, 0, source.length);
    fail("expected CorruptValueException");
} catch (final CorruptValueException expected) {
}
assertTrue("visitor must not see an out-of-bounds version", visitor.versions.isEmpty());

---

6. TransactionTest2 — make the failure diagnosable

persistit/core/src/test/java/com/persistit/TransactionTest2.java

The new assertion is a genuine tightening — I confirmed _failedTransactionCount is incremented only for unexpected throwables (lines 317, 324), not for ordinary interrupt handling, so it shouldn't be flaky on its own.

Two small things:

• "see stack traces above" isn't much help in a CI log. Record the first throwable so the assertion carries it:

  static AtomicReference<Throwable> _firstFailure = new AtomicReference<Throwable>();
  // in the catch blocks that count a failure:
  _firstFailure.compareAndSet(null, throwable);
  // in the assertion:
  assertEquals("Worker threads died with unexpected exceptions; first was " + _firstFailure.get(),
          0, _failedTransactionCount.get());

• The counters are static and shared; transactions() and the stranding test don't reset them. An @Before reset (or instance fields) stops the next person re-stepping on this.

---

7. Optional follow-up — IntegrityCheck loses sight of leftover marks

persistit/core/src/main/java/com/persistit/IntegrityCheck.java:188 goes through visitAllVersions, which now strips marks silently. Pages already on disk carrying stale marks will read clean and icheck reports nothing; the mark survives in the length field until some future prune rewrites that version. Making marks readable is right; making them invisible is a small step back. Consider counting MVV.isMarked versions as a fault so affected volumes can be identified.

[vharseko]

Thanks for the thorough review — all points addressed in 19fb065...b669a2f (amended into the single commit). Point by point:

1. finally bound stricter than pass 1 — fixed, both variants applied. The safety net now mirrors pass 1 exactly (index < offset + length), bounded only by the array end past which mark() could not have written (index + LENGTH_PER_VERSION <= bytes.length). I also took your alternative: pass 1's guard is tightened to from != offset + length && from + LENGTH_PER_VERSION > offset + length, so a misaligned traversal now throws before the next iteration can read or mark outside the MVV region. The two are complementary — the tightened guard cannot protect the very first iteration (a "MVV" shorter than one version header), so the safety net keeps the mirrored bound.

2. exactRequiredLength — loop bound fixed (while (offset < targetOffset + targetLength)). I left return targetLength - valueLength + newVersionLength as is: all branches of this method return lengths, not offsets, and that expression is pure length arithmetic, so it holds for any targetOffset once the loop bound is right. Let me know if I'm missing a case.

3. fetchVersionByOffset — done. Now getLength(source, offset - LENGTH_PER_VERSION).

4. Trailing check in visitAllVersions — kept, with a comment. Marked as defensive-only/unreachable so the next reader doesn't assume it's live; I'd rather keep it as a tripwire in case the traversal logic changes than delete it.

5. visitOverrunningLengthThrows — reworked as suggested. It now asserts visitor.versions.isEmpty() after the expected throw, which does fail on the pre-fix code (the visitor used to record the out-of-bounds version before the trailing check threw).

6. TransactionTest2 — both done. Added AtomicReference<Throwable> _firstFailure, recorded in the two counting catch blocks and included in the assertion message; the shared counter resets moved to an @Before so every test in the class starts clean.

7. IntegrityCheck — agreed, filed as #290. Counting MVV.isMarked versions as a fault touches the icheck reporting (likely a VersionVisitor extension plus a new counter), so I'd keep this PR read-side only.

Verified locally on JDK 11: MVVTest (38), MVCCPruneTest, MVCCBasicTest, TransactionTest2, and the full persistit/core suite — 575 tests, 0 failures, 5 skipped.

[maximthomas]

Pass 1's tightened guard means it can't mark past offset + length, so the bytes.length
bound now lets this loop unmark() — write — outside the MVV region. At
offset=0, length=20, vlength=4: pass 1 marks at 1 and throws at from=15, then the net
writes bytes 23–24 (ff -> 7f) — the next record in the page. Mirror pass 1's
advance-then-check instead; the array-end bound is only needed for its unguarded first
iteration. 98 tests green locally, byte 23 untouched.

Suggested change

	/*
	* Make sure all marks are removed even if this method exits via an
	* Exception.
	* Exception. Marks left behind in a live buffer page are read back
	* later as part of the version length and corrupt the MVV (see
	* issue #286). A zero-length version is legal (undefined value),
	* so the loop must not stop on vlength == 0; it always advances by
	* at least LENGTH_PER_VERSION. The iteration must mirror the first
	* pass exactly — on a corrupt MVV that pass can visit a version
	* starting inside the trailing LENGTH_PER_VERSION bytes, so this
	* loop may not stop earlier; the only hard bound is the array end,
	* past which mark() could not have written.
	*/
	if (marked > 0) {
	int index = offset + 1;
	while (index < length) {
	while (index < offset + length && index + LENGTH_PER_VERSION <= bytes.length) {
	final int vlength = getLength(bytes, index);
	Debug.$assert0.t(vlength + index + LENGTH_PER_VERSION <= offset + length);
	unmark(bytes, index);
	index += vlength + LENGTH_PER_VERSION;
	if (vlength <= 0) {
	break;
	}
	}
	}
	/*
	* Remove marks left by pass 1 even when this method exits via an
	* exception: a mark left in a live page is read back as part of
	* the version length and corrupts the MVV (issue #286). Mirrors
	* pass 1 - same advance, same guard - so it touches exactly the
	* indices that pass could have marked, and nothing else. A
	* zero-length version is legal, hence no break on vlength == 0.
	* The array-end bound covers pass 1's unguarded first iteration.
	*/
	if (marked > 0) {
	int index = offset + 1;
	while (index + LENGTH_PER_VERSION <= bytes.length) {
	final int vlength = getLength(bytes, index);
	unmark(bytes, index);
	index += vlength + LENGTH_PER_VERSION;
	if (index + LENGTH_PER_VERSION > offset + length) {
	break;
	}
	}
	}

[vharseko]

Applied as suggested — amended into d6ce5c9. You're right: the tightened pass-1 guard made the mirrored bound unsafe in the opposite direction — pass 1 now throws before marking the misaligned index, while the net still visited it, so its unmark() became a stray write outside the region. The advance-then-check net touches exactly the indices pass 1 could have marked.

Also pinned the scenario with a regression test, pruneExceptionMustNotUnmarkPastRegionEnd: one uncommitted version (so pass 1 marks it) plus a claimed region length that leaves the next version header straddling the region end, in a 0xFF-filled array. On the previous net it fails with the stray write at the region end (arrays first differed at element [139]: 0xff -> 0x7f); with this fix prune leaves the array byte-for-byte unchanged. Local runs: MVVTest 39/39; with MVCCBasicTest, MVCCPruneTest, MVCCPruneBufferTest, MVCCConcurrentTest, TransactionIndexTest, TransactionTest2 — 103 tests, 0 failures, 1 pre-existing skip.

[vharseko]

@maximthomas Round 2 addressed in d6ce5c9 (details in the inline thread): the safety net is now the advance-then-check mirror of pass 1 you suggested, and a new regression test pruneExceptionMustNotUnmarkPastRegionEnd pins the misaligned-MVV scenario — on the previous net it fails with the stray write at the region end (0xff -> 0x7f just past the region), with the fix prune leaves the array byte-for-byte unchanged. The PR description is updated to match.

Local runs on the amended commit: MVVTest 39/39; together with MVCCBasicTest, MVCCPruneTest, MVCCPruneBufferTest, MVCCConcurrentTest, TransactionIndexTest, TransactionTest2 — 103 tests, 0 failures, 1 pre-existing skip. CodeQL and the analyzers are green on the new head; the build matrix is still running. Please take another look.