Repository navigation
[#357] Wait for performed cleanup actions, not an empty queue, in persistit tests - #360
Merged
vharseko merged 1 commit intoOct 18, 2026
Conversation
… empty queue, in persistit tests CleanupManager.poll() drains the queue into a work list before it performs the actions and counts each one only after performing it. A wait on getEnqueuedCount() can therefore end while the drained actions are still pending, and IntegrityCheckTest.testIndexFixHoles then saw a performed count of 0 (expected:<0> but was:<4>, the arguments being swapped). Add PersistitUnitTestCase.waitForCleanupManager(), which waits until the performed and error counts add up to the accepted count, and use it in IntegrityCheckTest and TreeLifetimeTest. Put the arguments of the testIndexFixHoles assertion in order. TransactionAbandonedTest is left as it is: it disables background cleanup and runs the actions on its own thread, so its wait has no such window. Fixes OpenIdentityPlatform#357
vharseko
added
tests
This was referenced Oct 17, 2026
maximthomas
approved these changes
Oct 18, 2026
maximthomas
left a comment
maximthomas
left a comment
Contributor
There was a problem hiding this comment.
praise: The wait now counts what poll() does after an action runs, not the queue that poll() empties first.
PersistitUnitTestCase.waitForCleanupManagerexits oncegetPerformedCount() + getErrorCount()reachesgetAcceptedCount(), andCleanupManager.poll()advances_performed/_errorsonly afterperformAction.- The 60-second failure message carries all three counters and
getLastException(), so a timeout explains itself. IntegrityCheckTest.testIndexFixHolesnow passesholeCounttoassertEqualsas the expected value.
vharseko
added a commit
that referenced
this pull request
Oct 19, 2026
…tent (#368) Fixes #359 Fixes #367 ## Problem - **#359**: `CleanupManager.offer()` put the action into the queue first and counted it as accepted afterwards. `poll()` drains the queue without the monitor that `offer()` holds, so in between it could perform the action and count it as performed. For that moment `getPerformedCount() + getErrorCount()` could exceed `getAcceptedCount()`, or equal it while an accepted action was still pending. The counter-based waits in the tests rely on that not happening. - **#367**: two paths took accepted actions out of the queue without counting them, so the performed and error counts never caught up with the accepted count again: - `poll()` caught only `PersistitException` around each action. A `RuntimeException` left `poll()` (`IOTaskRunnable.run()` only logs it), was neither counted as an error nor posted to the `AlertMonitor`, and the rest of the drained work list (up to 2,000 actions) was lost. - `clear()` discarded the queued actions without counting them. - Found in the review: logging an `AlertMonitor` event replaced a `RuntimeException` among the event's arguments with its formatted stack trace. `PersistitLogMessage.Enabled.logMessage()` formatted the arguments in place, and `Event` passes its own argument array, so after the first log `Event.getFirstArg()` and `getArgs()` returned a `String` instead of the exception. ## Fix - **`offer()`**: checks the `remainingCapacity()` of the queue, counts the action as accepted and only then adds it. Only `offer()` adds to the queue, and it holds the monitor while it does, so the checked capacity is still there when the action is added. Counting first and taking the count back on refusal would make the accepted count briefly too high and let it go down; with the capacity check both the accepted and the refused count stay exact and only ever grow. - **`poll()`**: a `RuntimeException` from an action is handled like a `PersistitException`: the action counts as an error, the alert is posted, and the rest of the work list is performed. An `Error` still ends the loop and the `CLEANUP_MANAGER` thread, as before; the comment says so. - **`clear()`**: drains the queue into a list and counts the discarded actions; `CleanupManagerMXBean` reports them as the new `getDiscardedCount()`. `drainTo()` hands each action either to `clear()` or to a concurrent `poll()`, never to both. - **`PersistitLogMessage.Enabled.logMessage()`**: formats a copy of the arguments, so the event keeps the exception. - New `CLEANUP_OFFER_A`/`B`/`C` sequencer locations and `CLEANUP_OFFER_SCHEDULE`; `offer()` passes `CLEANUP_OFFER_A` between counting the action and adding it to the queue. ## Tests - `testActionAcceptedBeforePerformed`: parks the test's `offer()` at `CLEANUP_OFFER_A` (a `Condition`, set before the schedule is enabled, limits the location to the test's thread) and checks that the action already counts as accepted and is not in the queue yet. A test timeout turns a missed `CLEANUP_OFFER_A` into a failure instead of a hung build. - `testRuntimeExceptionCountedAsError`: an action failing with a `RuntimeException` counts as an error, the action after it is performed, `getLastException()` returns the exception, and the `AlertMonitor` holds one cleanup event carrying it. - `testClearCountsDiscardedActions`: after a `poll()` and a `clear()`, the accepted count equals the performed, error and discarded counts. - `testOverflow`: checks the exact accepted and refused counts instead of `>= 0`. - These four tests use a `CleanupManager` that is never started: no `CLEANUP_MANAGER` thread polls it and no Persistit thread offers to it, so only the test changes its counters. - `AlertMonitorTest.testLoggingKeepsEventArguments`: a logged event still holds its `RuntimeException`, and the log message still contains the stack trace. - `CheckpointCloseRaceTest.copyBackPagesConcurrentWithClose` sets its `Condition` before it enables the schedule as well. - Mutants killed: - accepted counted after the action is added (the #359 order), `add(action); sequence(CLEANUP_OFFER_A); _accepted.incrementAndGet();`: `testActionAcceptedBeforePerformed`, `expected:<1> but was:<0>`; - the hook after both statements, in either order: `testActionAcceptedBeforePerformed`, `expected:<0> but was:<1>`; - `offer()` never reaching `CLEANUP_OFFER_A` (refusing every action, timeout cut to 5 s for the run): `testActionAcceptedBeforePerformed` fails with `TestTimedOutException` and the rest of the class runs; - accepted counted before the capacity check: `testOverflow`, `expected:<100000> but was:<200000>`; - only `PersistitException` caught: `testRuntimeExceptionCountedAsError` fails with the `IllegalStateException` out of `poll()`; - the error counted but not posted: `testRuntimeExceptionCountedAsError`, `the RuntimeException should be posted to the AlertMonitor`; - the old `clear()`: `testClearCountsDiscardedActions`, `expected:<7> but was:<0>`; - arguments formatted in place: `testLoggingKeepsEventArguments` and `testRuntimeExceptionCountedAsError`, `expected same`. - Full `persistit/core` suite on JDK 27 at fb95353: 604 tests, 0 failures, 0 errors, 5 skipped. ## Note `waitForCleanupManager()` from #360 waits until the performed and error counts add up to the accepted count. No test calls `clear()` before such a wait, so it needs no change for the new discarded count.
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
2 participants
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.
Fixes #357
Cause
CleanupManager.poll()drains the queue into a work list withdrainTo(), then performs the actions and increments the performed count after each one. While the work list is being performed,getEnqueuedCount()is already 0.IntegrityCheckTest.waitForCleanupManager()waited only for an empty queue, so it could return before the fourCleanupIndexHoleactions ran, andtestIndexFixHolessaw a performed count of 0 (expected:<0> but was:<4>, with the arguments swapped).Change
PersistitUnitTestCase.waitForCleanupManager()waits until the performed and error counts add up to the accepted count, kicking the manager between checks. It fails after 60 seconds with the counts and the manager's last exception.JournalManagerTestandBug1056489Testalready wait on these counters.IntegrityCheckTest.testIndexFixHolesandtestPruneuse it instead of their private queue-size wait, and thetestIndexFixHolesassertion has its arguments in order.TreeLifetimeTest.testCleanupManagerShouldNotInstantiateTreesuses it as well. Its queue-size wait could end before the action ran, and then the test passed without checking anything.TransactionAbandonedTest.readAndWrite, which the issue also lists, is not changed. It disables background cleanup and runscm.runTask()on its own thread, so its loop ends only after the test thread has performed the actions. Its real problem is that it never writes in the abandoned transaction: #358.Verification
poll()betweendrainTo()and performing the actions, the unchangedtestIndexFixHolesfails with the CI messageIntegrityCheckTest.testIndexFixHoles:242 expected:<0> but was:<4>. With this change,testIndexFixHoles,testPruneandtestCleanupManagerShouldNotInstantiateTreespass, and the last one now takes 2.4 seconds because it waits for the action.persistit/coresuite passes on JDK 11: 600 tests, 0 failures, 5 skipped. After review fixes to the helper (nanoTime deadline, last exception in the message, Javadoc),IntegrityCheckTest,TreeLifetimeTest,TransactionAbandonedTestandCleanupManagerTestpass again.Related: #359 (
CleanupManager.offer()counts an action as accepted afterpoll()can already perform it).