Skip to content

Stabilize flaky TransactionTest2.transactionsConcurrentWithPersistitClose#255

Open
vharseko wants to merge 3 commits into
OpenIdentityPlatform:masterfrom
vharseko:fix/transactiontest2-flaky-close
Open

Stabilize flaky TransactionTest2.transactionsConcurrentWithPersistitClose#255
vharseko wants to merge 3 commits into
OpenIdentityPlatform:masterfrom
vharseko:fix/transactiontest2-flaky-close

Conversation

@vharseko

@vharseko vharseko commented Jul 9, 2026

Copy link
Copy Markdown
Member

Problem

TransactionTest2.transactionsConcurrentWithPersistitClose is flaky. It starts worker threads that run transactions in a loop, closes Persistit, and then immediately asserts that no worker thread was left stranded:

_persistit.close();
assertEquals("All threads should have exited correctly", 0, _strandedThreads.get());

Each worker increments _strandedThreads on entry and only decrements it once it catches one of the expected exceptions thrown when Persistit is closed. That unwinding is asynchronous: after close() returns, a worker still in the middle of a transaction needs a moment to reach its next Persistit operation, receive the exception, and decrement the counter. There is no wait between close() and the assertion, so on a loaded CI runner a worker that has not yet caught its exception leaves the counter at 1:

java.lang.AssertionError: All threads should have exited correctly expected:<0> but was:<1>
    at com.persistit.TransactionTest2.transactionsConcurrentWithPersistitClose(TransactionTest2.java:271)

Observed on build-maven (ubuntu-latest, 17); the same module passed on JDK 11/21/25/26 in the same run, confirming it is a timing flake rather than a real regression.

Fix

  • Keep references to the worker threads and join() them with a single bounded deadline (TIMEOUT, 10s) before the assertion, so the threads get a chance to observe the closure and exit. On success the join returns almost immediately; on a genuine problem (a stuck worker, or one that exited via an unexpected path and therefore did not decrement the counter) the assertion still fails, so the test keeps its diagnostic value.
  • Flatten the now-unnecessary launcher thread so the worker threads can be joined directly.
  • Reset the shared static _strandedThreads counter at the start of the test for isolation from the other methods in the class.

Verification

  • transactionsConcurrentWithPersistitClose run 5×: 5/5 pass.
  • Full TransactionTest2 class: 5 tests, 0 failures.

…lose

The test closed Persistit and immediately asserted that no worker threads
were left stranded, but worker threads unwind from the closure
asynchronously. On a loaded CI runner a worker that had not yet caught its
expected exception left _strandedThreads at 1, producing a spurious
"expected:<0> but was:<1>" failure (seen on ubuntu-latest JDK 17).

Join the worker threads with a bounded deadline before the assertion so
they can observe the closure and exit, and reset the shared
_strandedThreads counter at the start of the test for isolation.
@vharseko vharseko requested a review from maximthomas July 9, 2026 14:16
@vharseko vharseko added the tests Test code changes label Jul 9, 2026
vharseko added 2 commits July 10, 2026 09:24
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests Test code changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants