From 779cb81c40074106286034f52942bcfbf92eb081 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Thu, 9 Jul 2026 17:47:08 +0300 Subject: [PATCH 1/8] Fix flaky Bug1017957Test.induceCorruptionByStress on fast JVMs Two threads mutate the same tree concurrently without transactions for 10s; the test then asserted both an IntegrityCheck pass and zero thrown exceptions. On the JDK 25 CI job the latter failed with 32 exceptions while the volume was intact (0 faults). All 32 were transient CorruptVolumeExceptions: a non-transactional traversal briefly observed a page that the other thread was splitting/joining (Exchange.corrupt(), "invalid page type ... should be"). That is a benign artifact of concurrent non-transactional access, not the persistent corruption bug 1017957 guards against, which the IntegrityCheck (faults == 0) detects. Count and print transient CorruptVolumeExceptions for diagnostics but no longer fail on them; keep failing on IntegrityCheck faults or on any other exception type (NPE, RebalanceException, ...). --- .../java/com/persistit/Bug1017957Test.java | 44 +++++++++++++++++-- 1 file changed, 41 insertions(+), 3 deletions(-) diff --git a/persistit/core/src/test/java/com/persistit/Bug1017957Test.java b/persistit/core/src/test/java/com/persistit/Bug1017957Test.java index 61418e481..3abf007bc 100644 --- a/persistit/core/src/test/java/com/persistit/Bug1017957Test.java +++ b/persistit/core/src/test/java/com/persistit/Bug1017957Test.java @@ -1,6 +1,8 @@ /** * Copyright 2012 Akiban Technologies, Inc. - * + * + * Portions Copyrighted 2026 3A Systems, LLC. + * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at @@ -16,6 +18,7 @@ package com.persistit; +import com.persistit.exception.CorruptVolumeException; import org.junit.Test; import java.io.PrintWriter; @@ -112,6 +115,7 @@ protected Properties doGetProperties(final boolean cleanup) { public void induceCorruptionByStress() throws Exception { final long expiresAt = System.nanoTime() + STRESS_NANOS; final AtomicInteger totalErrors = new AtomicInteger(); + final AtomicInteger unexpectedErrors = new AtomicInteger(); final Thread t1 = new Thread(new Runnable() { @Override public void run() { @@ -131,6 +135,15 @@ public void run() { e.printStackTrace(); } totalErrors.incrementAndGet(); + // A CorruptVolumeException here is a transient read of a page that + // the other thread is concurrently splitting/joining (this test + // does non-transactional structural stress). It does not imply + // persistent corruption -- that is verified separately by the + // final IntegrityCheck. Only other exception types count as an + // unexpected failure of the bug-1017957 fix. + if (!(e instanceof CorruptVolumeException)) { + unexpectedErrors.incrementAndGet(); + } } } } catch (final Exception e) { @@ -158,6 +171,15 @@ public void run() { e.printStackTrace(); } totalErrors.incrementAndGet(); + // A CorruptVolumeException here is a transient read of a page that + // the other thread is concurrently splitting/joining (this test + // does non-transactional structural stress). It does not imply + // persistent corruption -- that is verified separately by the + // final IntegrityCheck. Only other exception types count as an + // unexpected failure of the bug-1017957 fix. + if (!(e instanceof CorruptVolumeException)) { + unexpectedErrors.incrementAndGet(); + } } } } catch (final Exception e) { @@ -177,9 +199,25 @@ public void run() { icheck.setMessageLogVerbosity(Task.LOG_VERBOSE); icheck.setMessageWriter(new PrintWriter(System.out)); icheck.checkVolume(_persistit.getVolume("persistit")); - System.out.printf("\nTotal errors %d", totalErrors.get()); + System.out.printf("%nTotal errors %d (unexpected %d)%n", totalErrors.get(), unexpectedErrors.get()); + /* + * The regression this test guards against (bug 1017957) manifests as + * *persistent* volume corruption -- a dangling index pointer to a reused + * garbage page, or a stale LevelCache after an unbumped tree generation. + * The authoritative assertion is therefore that the volume passes the + * IntegrityCheck with no faults after the concurrent stress run. + */ assertEquals("Corrupt volume", 0, icheck.getFaults().length); - assertEquals("Exception occurred", 0, totalErrors.get()); + /* + * The two worker threads mutate the same tree concurrently without + * transactions, so a traversal can transiently observe a page that another + * thread is in the middle of splitting/joining and defensively raise a + * CorruptVolumeException. On fast JVMs this happens occasionally even + * though the volume ends up structurally sound (asserted above), so those + * transient exceptions are counted and printed for diagnostics but are not + * treated as failures. Any other exception type still fails the test. + */ + assertEquals("Unexpected exception occurred", 0, unexpectedErrors.get()); } /** From 83a5d2ff62cc97b6c8bebe6f3bce5e4d67ef00a1 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 10 Jul 2026 09:19:31 +0300 Subject: [PATCH 2/8] Stabilize flaky TreeTransactionalLifetimeTest.createRemoveByStep The crash/restart branches of createRemoveByStepHelper reopen Persistit and return without waiting for recovery and timely-resource pruning to settle. Those run asynchronously after initialize(), so the next helper iteration can begin a transaction against not-yet-settled state, and residual tree-version state from the previous iteration's crash/restart leaks into the new transaction's step-based MVCC visibility -- a tree created at step 1 becomes visible at step 0. That produced an intermittent ComparisonFailure ("expected:<0[]...> but was:<0[:]...>") on CI (ubuntu-latest JDK 21); it does not reproduce on macOS. Quiesce the MVCC state after the restart by updating the active-transaction cache and pruning timely resources -- the same idiom the sibling tests in this class already use before asserting. --- .../TreeTransactionalLifetimeTest.java | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/persistit/core/src/test/java/com/persistit/TreeTransactionalLifetimeTest.java b/persistit/core/src/test/java/com/persistit/TreeTransactionalLifetimeTest.java index 2f9109dbc..3fde3522d 100644 --- a/persistit/core/src/test/java/com/persistit/TreeTransactionalLifetimeTest.java +++ b/persistit/core/src/test/java/com/persistit/TreeTransactionalLifetimeTest.java @@ -1,6 +1,8 @@ /** * Copyright 2013 Akiban Technologies, Inc. - * + * + * Portions Copyrighted 2026 3A Systems, LLC. + * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at @@ -235,6 +237,19 @@ private void createRemoveByStepHelper(final String treeName, final boolean primo if (crash || restart) { _persistit = new Persistit(_config); _persistit.initialize(); + /* + * Recovery and the pruning of timely tree resources run + * asynchronously after initialize(). If the next helper iteration + * starts a transaction before that settles, residual tree-version + * state from this iteration's crash/restart can leak into its + * step-based MVCC visibility -- e.g. a tree created at step 1 shows + * up at step 0 -- producing an intermittent ComparisonFailure + * ("expected:<0[]...> but was:<0[:]...>") seen on CI. Settle the + * active-transaction cache and prune timely resources here, the same + * way the sibling tests quiesce MVCC state before asserting. + */ + _persistit.getTransactionIndex().updateActiveTransactionCache(); + _persistit.pruneTimelyResources(); } assertEquals("Expected contents at steps", expected2, computeCreateRemoveState(treeName, 1)); } From 09dd1a7c8fb0e0bda6a23e38e6b0ad2e1c1c294f Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Thu, 9 Jul 2026 18:58:34 +0300 Subject: [PATCH 3/8] Make WarmupTest.readOrderIsSequential deterministic The test records the buffer inventory at shutdown, restarts, and asserts (via a TrackingFileChannel injected into the volume channel) that preload reads pages back from the volume file. That only holds once copyBackPages() has fully drained the journal into the volume; otherwise, on restart the recorded pages are served from the journal instead of the volume and the channel records zero reads: java.lang.AssertionError: Preload should have loaded pages from journal file at com.persistit.WarmupTest.readOrderIsSequential(WarmupTest.java:122) Background CLEANUP_MANAGER / page-writer / checkpoint activity racing with copyBackPages() can leave the drain incomplete, producing an intermittent failure that shows up on the CI Windows runner but does not reproduce on Linux/macOS. This is the same background-eviction/cleanup interleaving that made the sibling testWarmup flaky. Call disableBackgroundCleanup() at the start of the test so the journal drain is deterministic. The overflow/scramble phase relies on synchronous eviction during store(), which is unaffected, so the rest of the test is unchanged. --- .../src/test/java/com/persistit/WarmupTest.java | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/persistit/core/src/test/java/com/persistit/WarmupTest.java b/persistit/core/src/test/java/com/persistit/WarmupTest.java index 1d0141aae..2a076d0c8 100644 --- a/persistit/core/src/test/java/com/persistit/WarmupTest.java +++ b/persistit/core/src/test/java/com/persistit/WarmupTest.java @@ -66,6 +66,21 @@ public void testWarmup() throws Exception { @Test public void readOrderIsSequential() throws Exception { + /* + * Stop background cleanup/pruning for this session so the buffer-pool state + * is deterministic. The assertion below requires that the pages recorded by + * the buffer inventory at shutdown are actually read back from the *volume* + * file (the injected TrackingFileChannel only sees volume reads). That holds + * only once copyBackPages() has fully drained the journal into the volume. + * When the background CLEANUP_MANAGER / page-writer / checkpoint activity + * races with copyBackPages(), the drain can be left incomplete, so on + * restart some recorded pages are served from the journal instead of the + * volume and TrackingFileChannel records zero reads -- an intermittent + * failure that reproduces on the CI Windows runner but not on Linux/macOS. + * This is the same background-eviction/cleanup interleaving that made the + * sibling testWarmup flaky. Disabling it makes the drain deterministic. + */ + disableBackgroundCleanup(); Exchange ex = _persistit.getExchange("persistit", "WarmupTest", true); BufferPool pool = ex.getBufferPool(); From fc86a2d98a52bb7b4482c181bb4cda529de6496a Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 10 Jul 2026 10:30:58 +0300 Subject: [PATCH 4/8] Assert source-agnostic read count in readOrderIsSequential (review fixes) Rework the previous stabilization, which was ineffective and rested on a wrong hypothesis (see review on #259): - disableBackgroundCleanup() bound the first Persistit instance, which the test then replaces with new Persistit(); the second instance -- the one that runs preload and the injected channel -- kept background cleanup on. - The real fragility is the read source: a recorded page may be served from the volume file or the journal (VolumeStorageV2.readPage() consults readPageFromJournal() first), non-deterministically across a restart, so counting reads on a TrackingFileChannel injected into the volume channel can legitimately see zero. TrackingFileChannel.assertOrdered was also a no-op (its `previous` cursor never advanced) and its read list was mutated without synchronization by background reads. Assert on the buffer pool's miss counter instead: getMissCounter() increments whenever get() loads a page from disk regardless of source, so it is the source-agnostic signal that preload actually read the recorded pages back. This drops the fragile channel injection and the dead order check. --- .../test/java/com/persistit/WarmupTest.java | 34 +++++++------------ 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/persistit/core/src/test/java/com/persistit/WarmupTest.java b/persistit/core/src/test/java/com/persistit/WarmupTest.java index 2a076d0c8..ea5caea08 100644 --- a/persistit/core/src/test/java/com/persistit/WarmupTest.java +++ b/persistit/core/src/test/java/com/persistit/WarmupTest.java @@ -66,22 +66,6 @@ public void testWarmup() throws Exception { @Test public void readOrderIsSequential() throws Exception { - /* - * Stop background cleanup/pruning for this session so the buffer-pool state - * is deterministic. The assertion below requires that the pages recorded by - * the buffer inventory at shutdown are actually read back from the *volume* - * file (the injected TrackingFileChannel only sees volume reads). That holds - * only once copyBackPages() has fully drained the journal into the volume. - * When the background CLEANUP_MANAGER / page-writer / checkpoint activity - * races with copyBackPages(), the drain can be left incomplete, so on - * restart some recorded pages are served from the journal instead of the - * volume and TrackingFileChannel records zero reads -- an intermittent - * failure that reproduces on the CI Windows runner but not on Linux/macOS. - * This is the same background-eviction/cleanup interleaving that made the - * sibling testWarmup flaky. Disabling it makes the drain deterministic. - */ - disableBackgroundCleanup(); - Exchange ex = _persistit.getExchange("persistit", "WarmupTest", true); BufferPool pool = ex.getBufferPool(); @@ -129,12 +113,20 @@ public void readOrderIsSequential() throws Exception { _persistit.initialize(); final Volume volume = _persistit.getVolume("persistit"); - final MediatedFileChannel mfc = (MediatedFileChannel) volume.getStorage().getChannel(); - final TrackingFileChannel tfc = new TrackingFileChannel(); - mfc.injectChannelForTests(tfc); pool = volume.getStructure().getPool(); + /* + * Verify that preload actually read the recorded pages back from disk. + * A recorded page may be served from either the volume file or the journal + * -- VolumeStorageV2.readPage() consults readPageFromJournal() first -- and + * which one is used is non-deterministic across a restart, so counting reads + * on an injected volume channel is racy (the original assertion + * intermittently saw zero reads on CI). The buffer pool's miss counter + * increments whenever get() loads a page from disk regardless of source, so + * it is the source-agnostic signal that preload did its work. + */ + final long missesBefore = pool.getMissCounter(); pool.preloadBufferInventory(); - assertTrue("Preload should have loaded pages from journal file", tfc.getReadPositionList().size() > 0); - tfc.assertOrdered(true, true); + assertTrue("Preload should have read the recorded pages back from disk", + pool.getMissCounter() > missesBefore); } } From 13382daa53be045652712f422c313b57de6aea5d Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 10 Jul 2026 17:21:17 +0300 Subject: [PATCH 5/8] Rework readOrderIsSequential per review: drain to volume, fix assertOrdered The getMissCounter() assertion was vacuous: preloadBufferInventory reads the shared _buffer_inventory_ tree before loading any recorded page, and that read alone bumps the pool's single _missCounter, so the test passed even when zero recorded pages loaded. Make the channel-based check deterministic through setup instead: after the first close, reopen with buffer inventory recording disabled, run copyBackPages() to drain the journal into the volume, and close again, so the recorded pages (including the inventory tree's own pages the closing checkpoint flushed to the journal) live in the volume file. Preload then reads them through the injected volume channel -- verified as 18 reads in strictly ascending page order. A strict getPageMapSize() == 0 precondition turned out unachievable -- the final close always leaves one checkpoint page in the journal (not a data page) -- so the assertions are the channel read count plus read order. Also fix TrackingFileChannel.assertOrdered, which never advanced `previous` and so only checked `position > -1`; it now verifies read order. --- .../com/persistit/TrackingFileChannel.java | 3 +- .../test/java/com/persistit/WarmupTest.java | 42 ++++++++++++------- 2 files changed, 29 insertions(+), 16 deletions(-) diff --git a/persistit/core/src/test/java/com/persistit/TrackingFileChannel.java b/persistit/core/src/test/java/com/persistit/TrackingFileChannel.java index dd4d9fc0c..41243761b 100644 --- a/persistit/core/src/test/java/com/persistit/TrackingFileChannel.java +++ b/persistit/core/src/test/java/com/persistit/TrackingFileChannel.java @@ -163,13 +163,14 @@ public List getReadPositionList() { public void assertOrdered(final boolean read, final boolean forward) { final List list = read ? _readPositions : _writePositions; - final long previous = forward ? -1 : Long.MAX_VALUE; + long previous = forward ? -1 : Long.MAX_VALUE; for (final Long position : list) { if (forward) { assertTrue("Position should be larger", position > previous); } else { assertTrue("Position should be smaller", position < previous); } + previous = position; } } } diff --git a/persistit/core/src/test/java/com/persistit/WarmupTest.java b/persistit/core/src/test/java/com/persistit/WarmupTest.java index ea5caea08..8e3de26af 100644 --- a/persistit/core/src/test/java/com/persistit/WarmupTest.java +++ b/persistit/core/src/test/java/com/persistit/WarmupTest.java @@ -106,27 +106,39 @@ public void readOrderIsSequential() throws Exception { _persistit.copyBackPages(); _persistit.close(); - _persistit = new Persistit(); + /* + * recordBufferInventory() at the close above interleaves its own tree stores + * with the buffer scan, so some recorded pages are the inventory tree's own + * pages; the closing checkpoint flushes those to the journal with no + * copyBack(), and on restart they would be served from the journal + * (VolumeStorageV2.readPage consults readPageFromJournal first), bypassing + * the injected volume channel. Reopen with inventory recording disabled, + * drain the journal into the volume, and close again so every recorded page + * lives in the volume file; the preload below then reads them through the + * tracked channel deterministically instead of racing the journal. + */ _config.setBufferInventoryEnabled(false); _config.setBufferPreloadEnabled(false); - _persistit.setConfiguration(_config); - _persistit.initialize(); + _persistit = new Persistit(_config); + _persistit.copyBackPages(); + _persistit.close(); + + _persistit = new Persistit(_config); final Volume volume = _persistit.getVolume("persistit"); + final MediatedFileChannel mfc = (MediatedFileChannel) volume.getStorage().getChannel(); + final TrackingFileChannel tfc = new TrackingFileChannel(); + mfc.injectChannelForTests(tfc); pool = volume.getStructure().getPool(); + pool.preloadBufferInventory(); /* - * Verify that preload actually read the recorded pages back from disk. - * A recorded page may be served from either the volume file or the journal - * -- VolumeStorageV2.readPage() consults readPageFromJournal() first -- and - * which one is used is non-deterministic across a restart, so counting reads - * on an injected volume channel is racy (the original assertion - * intermittently saw zero reads on CI). The buffer pool's miss counter - * increments whenever get() loads a page from disk regardless of source, so - * it is the source-agnostic signal that preload did its work. + * With the recorded pages copied back to the volume above, preload reads + * them through the injected volume channel rather than the journal, so the + * channel sees a non-empty, strictly ascending sequence of reads (warmup + * issues them in page order via PageNode.READ_COMPARATOR). */ - final long missesBefore = pool.getMissCounter(); - pool.preloadBufferInventory(); - assertTrue("Preload should have read the recorded pages back from disk", - pool.getMissCounter() > missesBefore); + assertTrue("Preload should have read the recorded pages from the volume file", + tfc.getReadPositionList().size() > 0); + tfc.assertOrdered(true, true); } } From 7bc8cc995a1ca7483d1a88916ecc1f6d696163a6 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 10 Jul 2026 18:11:52 +0300 Subject: [PATCH 6/8] Revert "Fix flaky Bug1017957Test.induceCorruptionByStress on fast JVMs" This reverts commit 779cb81c40074106286034f52942bcfbf92eb081. --- .../java/com/persistit/Bug1017957Test.java | 44 ++----------------- 1 file changed, 3 insertions(+), 41 deletions(-) diff --git a/persistit/core/src/test/java/com/persistit/Bug1017957Test.java b/persistit/core/src/test/java/com/persistit/Bug1017957Test.java index 3abf007bc..61418e481 100644 --- a/persistit/core/src/test/java/com/persistit/Bug1017957Test.java +++ b/persistit/core/src/test/java/com/persistit/Bug1017957Test.java @@ -1,8 +1,6 @@ /** * Copyright 2012 Akiban Technologies, Inc. - * - * Portions Copyrighted 2026 3A Systems, LLC. - * + * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at @@ -18,7 +16,6 @@ package com.persistit; -import com.persistit.exception.CorruptVolumeException; import org.junit.Test; import java.io.PrintWriter; @@ -115,7 +112,6 @@ protected Properties doGetProperties(final boolean cleanup) { public void induceCorruptionByStress() throws Exception { final long expiresAt = System.nanoTime() + STRESS_NANOS; final AtomicInteger totalErrors = new AtomicInteger(); - final AtomicInteger unexpectedErrors = new AtomicInteger(); final Thread t1 = new Thread(new Runnable() { @Override public void run() { @@ -135,15 +131,6 @@ public void run() { e.printStackTrace(); } totalErrors.incrementAndGet(); - // A CorruptVolumeException here is a transient read of a page that - // the other thread is concurrently splitting/joining (this test - // does non-transactional structural stress). It does not imply - // persistent corruption -- that is verified separately by the - // final IntegrityCheck. Only other exception types count as an - // unexpected failure of the bug-1017957 fix. - if (!(e instanceof CorruptVolumeException)) { - unexpectedErrors.incrementAndGet(); - } } } } catch (final Exception e) { @@ -171,15 +158,6 @@ public void run() { e.printStackTrace(); } totalErrors.incrementAndGet(); - // A CorruptVolumeException here is a transient read of a page that - // the other thread is concurrently splitting/joining (this test - // does non-transactional structural stress). It does not imply - // persistent corruption -- that is verified separately by the - // final IntegrityCheck. Only other exception types count as an - // unexpected failure of the bug-1017957 fix. - if (!(e instanceof CorruptVolumeException)) { - unexpectedErrors.incrementAndGet(); - } } } } catch (final Exception e) { @@ -199,25 +177,9 @@ public void run() { icheck.setMessageLogVerbosity(Task.LOG_VERBOSE); icheck.setMessageWriter(new PrintWriter(System.out)); icheck.checkVolume(_persistit.getVolume("persistit")); - System.out.printf("%nTotal errors %d (unexpected %d)%n", totalErrors.get(), unexpectedErrors.get()); - /* - * The regression this test guards against (bug 1017957) manifests as - * *persistent* volume corruption -- a dangling index pointer to a reused - * garbage page, or a stale LevelCache after an unbumped tree generation. - * The authoritative assertion is therefore that the volume passes the - * IntegrityCheck with no faults after the concurrent stress run. - */ + System.out.printf("\nTotal errors %d", totalErrors.get()); assertEquals("Corrupt volume", 0, icheck.getFaults().length); - /* - * The two worker threads mutate the same tree concurrently without - * transactions, so a traversal can transiently observe a page that another - * thread is in the middle of splitting/joining and defensively raise a - * CorruptVolumeException. On fast JVMs this happens occasionally even - * though the volume ends up structurally sound (asserted above), so those - * transient exceptions are counted and printed for diagnostics but are not - * treated as failures. Any other exception type still fails the test. - */ - assertEquals("Unexpected exception occurred", 0, unexpectedErrors.get()); + assertEquals("Exception occurred", 0, totalErrors.get()); } /** From 8f9ead3d550c73f48ef3461c13c0229e49138328 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 10 Jul 2026 18:10:33 +0300 Subject: [PATCH 7/8] Validate the page before re-inserting an index pointer in fixIndexHole (bug 1017957 class race) rebalanceSplit() links a new page only through its sibling chain and defers the parent index entry to a background CleanupIndexHole action. By the time the CleanupManager runs it, the action may be arbitrarily stale: a covering removeKeyRange can have unlinked the page onto a garbage chain (content and type intact, or retyped PAGE_TYPE_GARBAGE if it became a chain root), or the page may have been reused. fixIndexHole() performed no validation and inserted a key-pointer pair for whatever the page now holds, planting a dangling index entry to a garbage or reused page -- the bug 1017957 failure mode. Concurrent descents following such an entry throw transient CorruptVolumeExceptions ("invalid page type 30: should be 2" observed on CI under Bug1017957Test.induceCorruptionByStress; "walked right more than 50 pages" and "LONG_RECORD chain is invalid" are the same race's other signatures). The volume looks consistent afterwards because later covering removes delete the dangling entry before IntegrityCheck runs, which is why the corruption appeared transient. Faster JVMs widen the window: more iterations produce more rebalance splits racing the background queue. Guard the insertion. Under the tree reader claim (structure deletes require the writer claim, so reachability cannot change while it is held): (a) drop the action if the page is no longer this level's type or is empty; (b) verify the page is still reachable in this tree -- a search for its current first key at that level must land on the page itself via the B-link right-walk -- and drop the stale action otherwise. No claim is held across the verification descent, preserving the parent-then-child claim order. Verified: full persistit/core suite 565/565 green; IntegrityCheckTest testIndexFixHoles confirms valid holes are still repaired; Bug1017957Test.induceCorruptionByStress with its original strict zero-exception assertion green across repeated runs. The race itself reproduces only on CI runners, which is where this fix is ultimately validated. --- .../src/main/java/com/persistit/Exchange.java | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/persistit/core/src/main/java/com/persistit/Exchange.java b/persistit/core/src/main/java/com/persistit/Exchange.java index 3842f42e9..897c7ba2d 100644 --- a/persistit/core/src/main/java/com/persistit/Exchange.java +++ b/persistit/core/src/main/java/com/persistit/Exchange.java @@ -4076,12 +4076,49 @@ boolean fixIndexHole(final long page, final int level) throws PersistitException return false; } try { + /* + * This action was enqueued by rebalanceSplit() when the page was + * reachable only through its sibling chain, and may be arbitrarily + * stale by the time the background CleanupManager runs it: a covering + * removeKeyRange may since have unlinked the page onto a garbage chain + * (leaving its content and type intact, or retyping it PAGE_TYPE_GARBAGE + * if it became a chain root), or the page may have been reused by an + * unrelated allocation. Blindly inserting a key-pointer pair for such a + * page plants a dangling index entry to a garbage or reused page -- the + * bug 1017957 failure mode, observed as transient + * CorruptVolumeExceptions ("invalid page type 30") under concurrent + * structural stress. Structure deletes hold the tree writer claim, so + * under the reader claim held here the page's reachability cannot + * change; validate it before inserting the pointer. + */ buffer = _pool.get(_volume, page, false, true); + if (buffer.getPageType() != level + PAGE_TYPE_DATA + || buffer.getKeyBlockEnd() <= buffer.getKeyBlockStart()) { + // Retyped (garbage-chain root or reused elsewhere) or empty: the + // index hole this action was created for no longer exists. + return true; + } buffer.nextKey(_spareKey2, buffer.toKeyBlock(0)); _value.setPointerValue(page); _value.setPointerPageType(buffer.getPageType()); buffer.release(); buffer = null; + /* + * A page sitting on a garbage chain keeps its old type and content, so + * the type check above is not sufficient. Verify the page is still + * reachable in this tree: a search for its current first key at this + * level must land on the page itself (via the B-link right-walk, since + * the hole means it has no parent entry yet). If the search lands + * elsewhere the action is stale and inserting the pointer would corrupt + * the index, so drop it. + */ + searchTree(_spareKey2, level, false); + final LevelCache lc = _levelCache[level]; + final long landedOn = lc._page; + lc._buffer.releaseTouched(); + if (landedOn != page) { + return true; + } storeInternal(_spareKey2, _value, level + 1, StoreOptions.NONE); return true; } finally { From c7d6ae028ab692a5fac295b24d7717edfd72d19c Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 10 Jul 2026 18:18:11 +0300 Subject: [PATCH 8/8] Reword quiesce comment: recovery is synchronous; the async actors are the ATC updater and CleanupManager pruning Per review on #257: initialize() applies recovered transactions inline, so blaming asynchronous recovery was wrong and would send a maintainer hunting a race that does not exist. The asynchronous actors that make the settle calls necessary are the TransactionIndex active-transaction-cache updater thread and the CleanupManager's timer-driven pruneTimelyResources(). Comment-only change. --- .../TreeTransactionalLifetimeTest.java | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/persistit/core/src/test/java/com/persistit/TreeTransactionalLifetimeTest.java b/persistit/core/src/test/java/com/persistit/TreeTransactionalLifetimeTest.java index 3fde3522d..55e3a49c2 100644 --- a/persistit/core/src/test/java/com/persistit/TreeTransactionalLifetimeTest.java +++ b/persistit/core/src/test/java/com/persistit/TreeTransactionalLifetimeTest.java @@ -238,15 +238,17 @@ private void createRemoveByStepHelper(final String treeName, final boolean primo _persistit = new Persistit(_config); _persistit.initialize(); /* - * Recovery and the pruning of timely tree resources run - * asynchronously after initialize(). If the next helper iteration - * starts a transaction before that settles, residual tree-version - * state from this iteration's crash/restart can leak into its - * step-based MVCC visibility -- e.g. a tree created at step 1 shows - * up at step 0 -- producing an intermittent ComparisonFailure - * ("expected:<0[]...> but was:<0[:]...>") seen on CI. Settle the - * active-transaction cache and prune timely resources here, the same - * way the sibling tests quiesce MVCC state before asserting. + * Recovery itself completes synchronously inside initialize(), but + * two maintenance tasks are asynchronous after it: the + * TransactionIndex's active-transaction-cache updater thread and + * the CleanupManager's timer-driven pruneTimelyResources(). If the + * next helper iteration starts a transaction before those settle, + * residual tree-version state from this iteration's crash/restart + * can leak into its step-based MVCC visibility -- e.g. a tree + * created at step 1 shows up at step 0 -- producing an intermittent + * ComparisonFailure ("expected:<0[]...> but was:<0[:]...>") seen on + * CI. Force both here, the same way the sibling tests quiesce MVCC + * state before asserting. */ _persistit.getTransactionIndex().updateActiveTransactionCache(); _persistit.pruneTimelyResources();