[#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.
• 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.
• 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.

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.