Skip to content

Stabilize flaky TreeTransactionalLifetimeTest.createRemoveByStep#260

Closed
vharseko wants to merge 2 commits into
OpenIdentityPlatform:masterfrom
vharseko:fix/treetxnlifetime-flaky
Closed

Stabilize flaky TreeTransactionalLifetimeTest.createRemoveByStep#260
vharseko wants to merge 2 commits into
OpenIdentityPlatform:masterfrom
vharseko:fix/treetxnlifetime-flaky

Conversation

@vharseko

Copy link
Copy Markdown
Member

Problem

TreeTransactionalLifetimeTest.createRemoveByStep is flaky. Its crash/restart sub-cases reopen Persistit and return without waiting for recovery and timely-resource pruning to settle:

if (crash || restart) {
    _persistit = new Persistit(_config);
    _persistit.initialize();
}

Recovery and the pruning of timely tree resources 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:

org.junit.ComparisonFailure: Expected contents at steps
  expected:<0[],1:,2:a=step2,3,4:b=step4>
  but was:<0[:],1:,2:a=step2,3,4:b=step4>
    at com.persistit.TreeTransactionalLifetimeTest.createRemoveByStepHelper(TreeTransactionalLifetimeTest.java:215)

Observed on build-maven (ubuntu-latest, 21); it does not reproduce on macOS (40 local runs green).

Fix

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 (updateActiveTransactionCache() + pruneTimelyResources()). The change adds nothing but a settle point; it weakens no assertion (the expected2 check after restart is unaffected — pruning removes only obsolete versions).

Not caused by this PR's parent change

Unrelated to the PR it was blocking (#257, which fixes Bug1017957Test.induceCorruptionByStress): that PR touches only Bug1017957Test.java, a different test.

Verification

  • Happy path with the fix: createRemoveByStep 7/7 green; full TreeTransactionalLifetimeTest class 5/5.
  • The flake does not reproduce locally (macOS), so the fix is validated by mechanism analysis and happy-path stability; the two settle calls are idempotent no-ops when the state is already quiesced. CI is the real confirmation.

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.
@vharseko vharseko requested a review from maximthomas July 10, 2026 06:20
@vharseko vharseko added the tests Test code changes label Jul 10, 2026
… the ATC updater and CleanupManager pruning

Per review on OpenIdentityPlatform#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.
@vharseko

Copy link
Copy Markdown
Member Author

Closing in favor of #266: it adds the same two calls (updateActiveTransactionCache() + pruneTimelyResources()) at the end of Persistit.initialize(), so the settle happens for every caller — including this test's crash/restart helper — rather than only inside TreeTransactionalLifetimeTest. With #266 merged, the test-side quiesce added here would be redundant.

@vharseko vharseko closed this Jul 10, 2026
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