[#952] Keep a failed state write from killing the checkpointer and hanging the shutdown

[vharseko]

Fixes #952.

The hang

ServerStateFlush.run() guarded only against interruption, so any unchecked exception from a state write ended the thread with done still false - PersistentServerState.save() runs a modify, and ModifyOperationBasis.run() lets everything but a CanceledOperationException out. shutdown() waited on that flag with no bound, and it is called from MultimasterReplication.finalizeSynchronizationProvider(), that is from DirectoryServer.shutDown() itself: the shutdown never went on to finalizePlugins(), shutdownLocalBackends() or the release of server.lock, so the backends were never closed and the JVM stayed up. ServerShutdownMonitor does not help - it excludes the thread which initiated the shutdown, which is exactly the one waiting. The domain also stopped checkpointing for the rest of the life of the server, with nothing but the uncaught-exception alert to say so.

The exception does not have to be exotic. A pluggable backend in an error state only produces a result code, because BackendImpl converts StorageRuntimeException into a DirectoryException - but an exception which is not a StorageRuntimeException goes straight through. CASStorage.execute() calls session.execute() directly, so an AllNodesFailedException or a DriverTimeoutException on a Cassandra-backed domain lands in the checkpointer.

This is independent of #948, which has landed since and changes save() itself: the exception still comes out of save() after it.

The change

• Both state.save() calls of the checkpointer go through saveState(), which catches RuntimeException, logs it, and keeps the loop going. The state is left marked as unsaved, so the next checkpoint writes it again - and when the failed write was the exit save, which has no next checkpoint, the domain comes back with the last state it did write, its own CSNs repaired from ds-sync-hist by checkAndUpdateServerState() and those of the other replicas as they were, and replays the changes since: duplicates, not loss. This trades the ALERT_TYPE_UNCAUGHT_EXCEPTION the death of the thread used to raise for an ERR_CHECKPOINTING_STATE_FAILED line per failed checkpoint and no alert at all, and that is deliberate: the alert reported a dead thread, which is the very thing this removes, and a state write which fails with a result code rather than an exception - a backend in an error state, which BackendImpl turns into a DirectoryException - has always been a logged DEBUG_ERROR_UPDATING_RUV and a retry, with no alert. The exception now takes the road the result code always took.
• The write is run outside the monitor of the thread. shutdown() takes that monitor to wake the checkpointer up, so a write which does not come back used to block the shutdown before it ever reached the wait it does for the thread.
• done is gone, and with it the hand-rolled join it stood for. shutdown() now waits with flushThread.join() and logs when the thread is still alive afterwards. join() covers a thread killed by an Error - which no catch in the loop can be expected to hold - a thread stuck in a write, and a thread which was never started, which is what the initial done = true stood for. The 30 s it waits is the budget ServerShutdownMonitor gives a thread before it starts interrupting them. It is spent by every call of shutdown(): once per domain when the server goes down - finalizeSynchronizationProvider() shuts the domains down one after another - and again by a second caller of a domain whose checkpointer is stuck. It bounds the shutdown of the domain, not of the server: a write which ignores the interrupt still holds the quiescence of a pluggable backend, so shutdownLocalBackends() waits for it again in BackendImpl.waitUntilQuiescent(); SchemaBackend.closeBackend() has no such wait, so for the cn=schema domain this bound is the last one. Never worse than before, when the same write hung the shutdown here instead. What going on without the checkpointer costs: shutdown() sets no flag the checkpointer reads, so a write which comes back after the bound still lands, a REPLACE of ds-sync-state with the state as it was before the shutdown - one write, since the exit save after it finds the state saved and writes nothing. A domain re-added on the same base DN before then, or a backend which comes back, has that state written over whatever was saved since, and the replica replays from an older CSN: duplicates, not loss.
• The if (flushThread != null) around the wake-up goes with it. The field is final and the one constructor of the class assigns it unconditionally, so the guard could never fire - and it was the only reason the join() below it looked like the dereference of a nullable field, which is what the CodeQL alert on this PR reported.

Rebased on master

Twice since it was opened. Neither rebase moved anything of the change itself: the diff of the branch against master is the same line for line before and after each of them, apart from context.

First onto f3076a0, over #948 and #908. Two files conflicted:

• LDAPReplicationDomain.java - [#916] Keep an update that lands during a ServerState save out of the saved flag #948 put the final save of run() under a !disabled && !importInProgress() guard, where this branch replaces that same call with saveState(). Both are kept: the guard and the comment which explains it stay, and the call inside goes through saveState(). An exception from that last write is exactly the one which used to end run() with done still false, so it is the one which least deserves to be let out.
• replication.properties - [#908] Wait for the changes being applied before a domain going down saves its ServerState #945 landed 319-320 right where this branch adds its own. Merged by keeping both sides, without renumbering anything.

Then onto 13d57e0, over #935, #959, #972, #973, #975 and #976. Only replication.properties conflicted, and only because #972 put its 326-327 at the end of the file, where this branch's two messages sit. Merged by keeping both sides again: master's 326-327 stay where they are and this branch's 323-324 follow them. LDAPReplicationDomain.java merged on its own - #972 made eclDomain volatile a few lines above the done flag this branch removes, and #976 reworded a trace message far from the checkpointer.

Testing

ServerStateFlushTest drives a domain over a memory backend whose write of ds-sync-state throws, dies of an Error, or blocks on a latch. Every test was watched failing against master first:

test	on master
checkpointerKeepsCheckpointingAfterAStateWriteThatThrows	the checkpointer terminates abnormally (msgID=140, through LDAPReplicationDomain:566 and ModifyOperationBasis:337) and ds-sync-state is never written again
shutdownCompletesWhenEveryStateWriteThrows	hangs in shutdown()
shutdownCompletesWhenTheCheckpointerDiedOfAnError	hangs in shutdown()
shutdownCompletesWhileAStateWriteIsStuck	hangs in shutdown()

PersistentServerStateTest, ReplicationServerLoadBalancingTest and StateMachineTest were run alongside it and pass.

Re-run after each rebase: ServerStateFlushTest is green on the branch rebased onto f3076a0 and again on the one rebased onto 13d57e0 (2830d3b), Tests run: 4, Failures: 0, Errors: 0, Skipped: 0 both times.

The other direction was watched again on master at f3076a0, the commit this branch sat on after the first rebase, and it
does not even get past the first test. checkpointerKeepsCheckpointingAfterAStateWriteThatThrows fails
the same way it did before - the checkpointer terminates abnormally (msgID=140, now through
LDAPReplicationDomain:620 and ModifyOperationBasis:337) and ds-sync-state is never written again -
and then the tear-down of that very test never returns. The surefire thread dump taken 40 minutes in has
main parked in LDAPReplicationDomain.shutdown() on the Thread.sleep(50) of the while (!done)
loop at LDAPReplicationDomain:2495, under MultimasterReplication.deleteDomain(), with the
checkpointer long dead and done false for good. The run was still there eleven hours later and had to
be killed - which is the hang of this issue, with a test suite in the place of a server shutdown.

Left alone on purpose, and worth an issue of its own: when the checkpointer is interrupted while no shutdown has been initiated, the catch puts the interrupt flag back and the next wait(1000) throws at once - a hot loop which never writes the state. Nothing in the tree interrupts that thread today.

After the review

The non-blocking items of the review, all taken (7457c49):

• shutdownCompletesWhenEveryStateWriteThrows pins the saveState() of the exit save, which no case did: a checkpointer which lets the exception of its last write out is as dead as one which held it, so isAlive() alone could not tell them apart. The case installs an uncaught-exception handler on the thread - it outranks the one of the thread group, which would log the exception and raise an alert - and asserts that nothing reached it, and that a failed write was made after the checkpointer was asked to stop. That count is taken in the backend, from isShutdownInitiated() of the writing thread, rather than from a snapshot of the counter before deleteDomain(): the wake-up of the checkpointer runs the in-loop save once more before the loop condition is read, so a snapshot cannot tell that write from the exit save, and a tick landing between the snapshot and the shutdown would count as well. Watched red with the exit save reverted to state.save(): Total tests run: 4, Failures: 1 - shutdownCompletesWhenEveryStateWriteThrows, AssertionError: the last state write let an exception out of the checkpointer expected [null] but found [java.lang.IllegalStateException: injected failure of a replication state write] at ServerStateFlushTest:260, the other three cases green as they were for the reviewer.
• tearDownDomain() stops the domain before it waits for the checkpointer. The other way round, the first case - which leaves the domain to the tear-down - spent the whole 20 s wait on a thread which was alive by design. The class runs in 47 s instead of 67 s under the same TestNG launcher, 37 s of it in the cases - 31 s of those the budget the stuck-write case spends on purpose.
• tearDownDomain() has a timeOut of its own. The one of a @Test covers the case body, and for that first case the shutdown under test runs in the tear-down - which is where the run on master hung, for eleven hours. 90 s rather than 60: the 30 s budget of the shutdown plus the 20 s wait for the thread is the longest tear-down which is not a failure. Watched firing with the bound set to 1 ms: ThreadTimeoutException: Method ...tearDownDomain() didn't finish within the time-out 1, one configuration failure, the three cases left skipped and exit code 3 - a red run rather than a hung one.
• A comment on the join() outside the CAS says that every caller waits, the one which lost the race included. There are two roads to a second shutdown() of a domain, both a race with finalizeSynchronizationProvider() while the provider is STOPPING - applyConfigurationAdd(), and applyConfigurationDelete() through deleteDomain() - and the second caller waited on done before this branch just the same. Moving the join inside the CAS would let it return while the checkpointer is still writing, which is worse. What the loser gets is the budget from its own arrival, which starts before the winner has asked the checkpointer to stop - the winner may still be in awaitReplayDrained(), up to 19 s - so on a stuck write it can log the budget as spent and return before the winner; the comment says so (round 2).
• The javadoc of FLUSH_THREAD_SHUTDOWN_TIMEOUT_IN_MS and the description say what the bound bounds, what going on without the checkpointer costs, and that the alert the dead thread used to raise is gone on purpose.

ServerStateFlushTest through the single reactor on 7457c49: Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 45.95 s.

After round 2

The three non-blocking items, all text (b298baf):

• ERR_CHECKPOINTING_STATE_FAILED said "the next checkpoint writes it again", which is false at the exit save. The text proposed in the review - "or the next start" - is not true either: loadState() reads the last ds-sync-state which was written, and checkAndUpdateServerState() repairs only the CSNs of this server from ds-sync-hist; those of the other replicas stay as the last write left them, and the replica replays from there. The message, and the javadoc of saveState() which made the same claim, now say that a domain which was stopping comes back with the last state it did write and replays the changes since.
• The javadoc of FLUSH_THREAD_SHUTDOWN_TIMEOUT_IN_MS no longer says "once per domain" against the comment at the join(): the budget is spent by every call of shutdown(), once per domain when the server goes down and again by a second caller. The quiescence wait is named for what it is, a BackendImpl fact: SchemaBackend.closeBackend() has no waitUntilQuiescent(), and ds-sync-state of the cn=schema domain lives there.
• The comment at the join() says what the loser of the CAS actually gets, as above. The comment rather than the latch: nothing after the second shutdown() on either road depends on the checkpointer having stopped - applyConfigurationAdd() has nothing after it, deleteDomain() goes on to removeECLDomainCfg() and stopReplayThreads() - so the cost of the inaccuracy is one ERR_STATE_CHECKPOINTER_NOT_STOPPED line in a race inside the shutdown of the server, and a latch counted down by the winner would leave the loser waiting twice the budget if the winner threw before reaching it.

No code moved: the diff is three comments, one message and the description. ServerStateFlushTest through the single reactor on b298baf: Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 49.27 s.

Ordinals

ERR_CHECKPOINTING_STATE_FAILED_323 and ERR_STATE_CHECKPOINTER_NOT_STOPPED_324, moved off 310 and
311 in 2830d3b. Six open branches had each read 310 as the first ordinal free in master and taken
it, and git merges those additions without reporting a conflict - they land in different places in
the file - so the duplicate would only have surfaced afterwards, as two unrelated messages sharing
one support ID. The generator does not check either: it keys on name and ordinal together, so both
compile.

#935, #945 and #959 have landed since, so 310-314 and 319-320 are in master now, and #972 took
326-327 on top of them. 323-324 are still claimed by no other open PR which adds to
replication.properties.

[vharseko]

Ordinals moved: ERR_CHECKPOINTING_STATE_FAILED 310 → 323, ERR_STATE_CHECKPOINTER_NOT_STOPPED 311 → 324 (c1cdeb3).

Six open branches had each read 310 as the first ordinal free in master and taken it - #935, #945,
#959, #964, #968, #977.

Nothing catches this on the way in. The additions land in different parts of the file, so git merges
every pair of them without reporting a conflict - I merged all ten onto master to check, and the
result carried 310, 311, 315 and 316 twice each. The generator does not check either: it loads the
file into a Properties and keys on name and ordinal (MessagePropertyKey.compareTo), so both
sides compile. What comes out is two unrelated messages carrying one support ID, found by whoever
reads a log rather than by CI.

The open PRs which add to the file now hold 310-325 with nothing claimed twice:

310-313 #935 · 314 #959 · 315-317 #958, #985 · 318 #985, #988 · 319-320 #945 · 321 #964 ·
322 #968 · 323-324 #977 · 325 #981

No Java moved with it: the generated constant is the key name without its ordinal, so the rename is
confined to replication.properties. #935, #958 and #985 keep what they had.

[vharseko]

Rebased onto master (13d57e0): the branch was conflicting.

Only replication.properties conflicted, and only because #972 put its _326 and _327 messages at the end of the file, right where the two messages of this branch sit. Resolved by keeping both sides: master's 326-327 stay where they are and this branch's 323-324 follow them. Nothing was renumbered - master now holds 310-314, 319-320 and 326-327, and 323-324 are still claimed by no other open PR which adds to the file.

LDAPReplicationDomain.java merged on its own: #972 made eclDomain volatile a few lines above the done flag this branch removes, and #976 reworded a trace message far from the checkpointer. The diff of the branch against master is the same line for line before and after the rebase, apart from those context lines.

The commits are the same three, re-parented: 3141ae7, 6dcd250, 2830d3b. Code scanning reports the flushThread alert (#1285) fixed on the merge ref, which is what 6dcd250 (formerly 52a36c6) was for.

The description is updated to match.

[maximthomas]

praise: The change is exactly what the description says, and the description says why: catch (RuntimeException) keeps the #923 Error contract, the write moved out of synchronized (this) so a stuck modify no longer holds the shutdown's wake-up, the dead null guard leaves with an explanation, and both rebases are line-identical. The test class injects the failures at the backend — the real write path, no mock — and the BLOCKS case runs the 30 s bound for real. The ordinal check across the open PRs in the comments is what the message generator should do and does not.

---

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:674
opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/ServerStateFlushTest.java:216-226

issue (non-blocking): The exit save's saveState() at :674 is pinned by no case.

I ran ServerStateFlushTest with that one line reverted to state.save() (the in-loop call at :646 untouched): 4/4 green, same as HEAD. shutdownCompletesWhenEveryStateWriteThrows asserts only that the thread is dead after the shutdown — an unwrapped exit save kills it too, and join() returns at once. Three lines make the case tell the two apart:

backend.stateWrite = StateWrite.THROWS_RUNTIME_EXCEPTION;
domain.getServerState().update(newCSN());
waitForFailedStateWrites(1);

final AtomicReference<Throwable> uncaught = new AtomicReference<>();
checkpointer.setUncaughtExceptionHandler((t, e) -> uncaught.set(e)); // outranks the group's handler
final int writesBeforeShutdown = backend.failedStateWrites.get();

deleteDomain();

assertFalse(checkpointer.isAlive(), "the state checkpointer is still running after the shutdown");
assertNull(uncaught.get(), "the exit save let an exception out of the checkpointer");
assertTrue(backend.failedStateWrites.get() > writesBeforeShutdown, "the checkpointer did not try the exit save");

---

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:2556

suggestion (non-blocking): Say in the description what the 30 s bound costs. shutdown() sets no flag the checkpointer reads (disabled belongs to disable()), so a write that returns after the bound completes as a REPLACE of ds-sync-state (PersistentServerState.updateStateEntry()), and the loop then runs the exit save once more with the same in-memory state. A domain re-added on the same base DN, or a backend that comes back, gets the pre-shutdown state written over anything saved since — the replica replays from an older CSN (duplicates, not loss). Narrow, and the right trade-off; one sentence is all it needs.

---

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:2513-2556

suggestion (non-blocking): join() sits outside the CAS that guards the wake-up, so a second shutdown() on a stuck checkpointer pays the 30 s again and logs ERR_STATE_CHECKPOINTER_NOT_STOPPED again. I found no road that calls shutdown() twice on one domain, so a one-line comment that every caller waits is enough — moving the join inside the CAS would let the second caller return while the first is still waiting, which is worse.

---

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:540-548

suggestion (non-blocking): The javadoc of FLUSH_THREAD_SHUTDOWN_TIMEOUT_IN_MS and the description read as if the bound were the server's. It is per domain — finalizeSynchronizationProvider() shuts domains down one after another, so a backend that answers nobody costs 30 s × domains — and it bounds this domain's shutdown, not the server's: a JE/PDB write that ignores ServerShutdownMonitor's interrupt still hangs shutDown(), now in BackendImpl.waitUntilQuiescent() (the parked replaceEntry holds threadTotalCount) before the server.lock release, instead of here. Never worse than before; just not "completes".

 * ... This is the budget {@code ServerShutdownMonitor} gives a thread before it starts
 * interrupting them. It is spent once per domain, and it bounds the shutdown of this domain,
 * not of the server: a write which ignores the interrupt still holds its backend's quiescence.

---

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:692-700

question (non-blocking): With RuntimeException caught the checkpointer no longer dies, so ALERT_TYPE_UNCAUGHT_EXCEPTION is never raised for it — alert handlers (JMX, SMTP) go silent and the once-a-second ERR_CHECKPOINTING_STATE_FAILED line is the only sign. Deliberate? If yes, a sentence in the description. If not, an alert on the first failure of a run (a flag cleared by the next successful save) keeps the log cadence and gives the handlers one notification.

---

opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/ServerStateFlushTest.java:173-181

suggestion (non-blocking): tearDownDomain() joins the checkpointer for 20 s before deleteDomain() stops it. In checkpointerKeepsCheckpointingAfterAStateWriteThatThrows the thread is alive by design, so the join spends its whole budget and the delete stops the thread a tick later: 20 s of dead wall time per run, hidden by TestNG's per-case time (156 s suite, 37 s in cases). Delete first, join after:

if (domain != null && !domainDeleted)
{
  deleteDomain(); // stops the checkpointer within a tick
}
if (checkpointer != null)
{
  checkpointer.join(SECONDS.toMillis(CHECKPOINT_TIMEOUT_IN_SECS));
  checkpointer = null;
}

---

opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/ServerStateFlushTest.java:163

nitpick (non-blocking): @Test(timeOut = 120000) covers the case body only. For the first case tearDownDomain() is where shutdown() runs — the path under test — so a regression there shows up as a CI timeout, not as a red case. @AfterMethod(timeOut = 60000).

[vharseko]

Review round 2: all seven non-blocking items taken in 7457c49, the description updated to match.

Exit save (:674) - pinned in shutdownCompletesWhenEveryStateWriteThrows with the uncaught-exception handler on the thread, as proposed. One change to the count: it is taken in the backend, from isShutdownInitiated() of the writing thread, rather than from a snapshot before deleteDomain(). The wake-up runs the in-loop saveState() once more before isShutdownInitiated() is read, so a snapshot cannot tell that write from the exit save, and a tick landing between the snapshot and the shutdown would count as well - the handler assertion is the one which pins the line, the count only says the premise held. Watched red with the line reverted to state.save(): Failures: 1, the last state write let an exception out of the checkpointer ... found [java.lang.IllegalStateException: injected failure of a replication state write] at :260, the other three green as in your run.

30 s cost - in the description, with one correction to the mechanism: it is one late write, not two. The exit save which follows a write that came back after the bound finds the state saved - PersistentServerState.save() returns on isSaved(), and a successful late write does not clear the flag - so it writes nothing. The late REPLACE itself, and what it does to a domain re-added on the same base DN or to a backend which comes back, is as you describe: an older CSN, duplicates rather than loss.

join() outside the CAS - a comment, as suggested. There is one road to a second shutdown() of a domain: applyConfigurationAdd() sees the provider STOPPING after createNewDomain() put the domain in the map, and finalizeSynchronizationProvider() iterates that map - a race during server shutdown. The second caller waited on done before this branch just the same, so the comment records the semantics rather than changes them.

Javadoc of the bound - your sentence, plus that the server shutdown waits for such a write again when it closes the backend: shutdownLocalBackends() → BackendImpl.waitUntilQuiescent(), which spins on threadTotalCount and ignores the interrupt. "Completes" in the description is softened accordingly.

Alert - deliberate, and now said in the description. The alert reported a dead thread, which is the thing this removes; and a state write which fails with a result code rather than an exception - a backend in an error state, which BackendImpl turns into a DirectoryException - has always been a logged DEBUG_ERROR_UPDATING_RUV and a retry, with no alert. The exception now takes the road the result code always took. A rate-limited alert of its own, on the pattern of sendUnreplayedChangeAlert() from #892, is a reasonable follow-up if one is wanted operationally; not in this PR.

Tear-down order - swapped. 47 s instead of 67 s for the class under the same TestNG launcher.

@AfterMethod timeOut - 90 s rather than 60: the 30 s budget of the shutdown plus the 20 s wait for the thread is the longest tear-down which is not a failure. Watched firing with the bound at 1 ms: ThreadTimeoutException: Method ...tearDownDomain() didn't finish within the time-out 1, one configuration failure, three cases skipped, exit 3.

ServerStateFlushTest through the single reactor on 7457c49: Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 45.95 s.

[maximthomas]

suggestion (non-blocking): "the next checkpoint writes it again" is false at the one site where the state is really lost: saveState() also serves the exit save (LDAPReplicationDomain.java:677), and nothing checkpoints after it. Make the text true at both sites:

ERR_CHECKPOINTING_STATE_FAILED_323=Could not write the replication state of domain "%s" : %s. \
 The state stays unsaved: the next checkpoint writes it again, or the next start if the \
 checkpointer was stopping

[vharseko]

Taken in b298baf, with a different second half. "or the next start if the checkpointer was stopping" is not what happens: loadState() reads the last ds-sync-state which was written, and checkAndUpdateServerState() repairs only the CSNs of this server from ds-sync-hist - those of the other replicas stay as the last write left them, and the replica replays from there. The message now says that a domain which was stopping comes back with the last state it did write and replays the changes since. The javadoc of saveState() made the same "next checkpoint" claim and says the same now.

[maximthomas]

suggestion (non-blocking): Two things in this javadoc. "It is spent once per domain" contradicts the comment at :2558-2559 ("a second shutdown() … pays the budget again") — both new in this round; the join comment is the accurate one. And "holds its backend's quiescence" is a BackendImpl fact: SchemaBackend.closeBackend() has no waitUntilQuiescent(), and the cn=schema domain's ds-sync-state lives there, so for that domain this 30 s is the last wait.

   * before it starts interrupting them. It is spent once per shutdown() call - a second
   * caller of a domain whose checkpointer is stuck pays it again - and it bounds the
   * shutdown of this domain, not of the server: a write which ignores the interrupt still
   * holds a pluggable backend's quiescence, so the server shutdown waits for it again when
   * it closes that backend; SchemaBackend has no such wait.

[vharseko]

Both in b298baf. The "once per domain" fact is kept next to the correction rather than replaced - it is what makes a backend which answers nobody cost 30 s × domains on a server shutdown: "spent by every call of shutdown(): once per domain when the server goes down, which shuts the domains down one after another, and again by a second caller of a domain whose checkpointer is stuck". The quiescence sentence is yours: the quiescence of a pluggable backend, and SchemaBackend has no such wait. The description says the same.

[maximthomas]

suggestion (non-blocking): "none of them returns while the checkpointer is still writing" is more than the loser has. Its join(30 s) at :2563 starts at its arrival, while the winner may still be in awaitReplayDrained() (bounded, up to 19 s) before initiateShutdown(): up to 19 s of the loser's budget go by before the checkpointer is even asked to stop, and on a stuck write the loser logs ERR_STATE_CHECKPOINTER_NOT_STOPPED with the full 30 s and returns first. Still strictly better than the done wait before this PR. Either say so:

     * Every caller waits, the one which lost the race above included. The loser's wait starts
     * before the winner has asked the checkpointer to stop, so it may end - and log the whole
     * budget as spent - while the winner is still waiting: a second shutdown() of a domain
     * whose checkpointer is stuck can return before the first one.

or have the loser wait for the flag first — a CountDownLatch counted down right after initiateShutdown(), awaited with the same budget before the join.

[vharseko]

The comment, in b298baf - your text, kept after the reason for waiting at all, which is what the old sentence was for. Not the latch: nothing after a second shutdown() on either road depends on the checkpointer having stopped - applyConfigurationAdd() has nothing after rd.shutdown(), deleteDomain() goes on to removeECLDomainCfg() and stopReplayThreads() - so what the inaccuracy costs is one ERR_STATE_CHECKPOINTER_NOT_STOPPED line in a race inside the shutdown of the server, and a latch counted down by the winner would have the loser wait twice the budget if the winner threw before reaching it. Two roads rather than the one I named in round 1, while at it: applyConfigurationDelete() through deleteDomain() races finalizeSynchronizationProvider() just the same. The description says so.

[maximthomas]

praise: Every round-1 item is in, and the one that mattered is pinned the right way — the per-thread UncaughtExceptionHandler + assertNull in shutdownCompletesWhenEveryStateWriteThrows catches the exit save losing its catch, and the unwrapped mutant fails exactly there. The join outside the CAS, the budget explained in one place, @AfterMethod(timeOut) bounding a hung teardown, and the test saying what each wait is for are all better than round 1. Approving; the comments below are non-blocking.

[vharseko]

Review round 3: the three non-blocking items of round 2 taken in b298baf - three comments and one message, no code moved. One departure, on the message: "or the next start" is not what happens - checkAndUpdateServerState() repairs only the CSNs of this server - so it says that a domain which was stopping comes back with the last state it did write and replays the changes since; the threads have the detail. The description is updated to match.

ServerStateFlushTest through the single reactor on b298baf: Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 49.27 s.