Fix flaky Bug1017957Test.induceCorruptionByStress on fast JVMs#257
Open
vharseko wants to merge 4 commits into
Open
Fix flaky Bug1017957Test.induceCorruptionByStress on fast JVMs#257vharseko wants to merge 4 commits into
vharseko wants to merge 4 commits into
Conversation
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, ...).
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.
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.
…xes) Rework the previous stabilization, which was ineffective and rested on a wrong hypothesis (see review on OpenIdentityPlatform#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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Bug1017957Test.induceCorruptionByStressis flaky on fast JVMs. On the JDK 25 CI job (build-maven (ubuntu-latest, 25)) it failed:while the volume itself was intact — the
IntegrityCheckreported 0 faults.Root cause
The test drives two threads that mutate the same tree concurrently, without transactions, for 10 seconds, then asserts both:
IntegrityCheck(no faults), andAll 32 exceptions in the failing run were transient
CorruptVolumeExceptions (invalid page type … should be 2, thrown fromExchange.corrupt()): a non-transactional traversal briefly observed a page that the other thread was in the middle of splitting/joining. This is a benign artifact of concurrent non-transactional access — not the persistent corruption that bug 1017957 guards against, which is exactly what theIntegrityCheck(faults == 0) detects.Fix
IntegrityCheckfaults assertion stays as the authoritative regression check.CorruptVolumeExceptions are still counted and printed for diagnostics, but no longer fail the test.NullPointerException,RebalanceException, …) still fails the test.Verification
mvn -pl persistit/core test -Dtest=Bug1017957Test(JDK 11):Tests run: 1, Failures: 0.Total errors 0 (unexpected 0), 0 faults.CorruptVolumeException→ pass; oneNullPointerExceptionorRebalanceException→ fail.