[#951] Keep the persisted position when a domain drops its in-memory ServerState

[vharseko]

Fixes #951.

The defect

disable() saves the ServerState, empties the copy in memory, and clearInMemory() marks the emptied state as not saved. #945 has since landed and reordered the method — the flag first, then the drain, then the two state calls (LDAPReplicationDomain.java:4015-4020):

synchronized (serviceStateLock)
{
  disabled = true;
  disableService();
  sessionGeneration++;
  awaitReplayDrained();
  state.save();
  state.clearInMemory();

serviceStateLock buys nothing against the checkpointer — it synchronises on itself (:615), so the two threads share no lock. A checkpointer which read !disabled a moment before the flag was set can therefore take its snapshot after the map was emptied, find it dirty, and REPLACE ds-sync-state with a zero-value list, removing the attribute from the base entry.

The last save on the way out of the loop (:647) used to be unconditional, so a shutdown following a disable() wrote the emptied state whatever the flags said, no race required. #945 guards it now, with !disabled && !importInProgress() — which closes that road on its own; the guard is read before save() builds the modify, so what is left of it is the window above rather than a certainty. The road below is untouched by either.

disable() is reached from the online import-ldif and restore tasks (ImportTask.java:628, RestoreTask.java:263), including restore --verifyOnly, which disables and re-enables the domain with the data untouched.

A third road, and this one is not a race

Both tasks reach disable() twice, every time. disable() has no idempotence guard, and after notifyImportBeginning the task disables the backend — which has the domain disabled again through the backend initialization listener it registers at :790:

ImportTask.java:628  notifyImportBeginning  -> MultimasterReplication.java:671  disable()   // #1
ImportTask.java:633  TaskUtils.disableBackend
                     -> BackendConfigManager.java:828  deregisterBackend
                     -> :1150-1158  performBackendPreFinalizationProcessing
                     -> LDAPReplicationDomain.java:281-289  disable()                       // #2

Nothing keeps the second call out: ignoreBackendInitializationEvent is set by preBackendImport() (:4543), which is the total update road, and serverShutdownRequested is false. The listener fires before configuredBackends.remove() and deregisterLocalBackend(), so the backend is still registered and the modify of the second save() does reach the base entry — writing the copy disable() #1 had just dropped. RestoreTask takes the same pair (:263, then :270 TaskUtils.disableBackend under if (!verifyOnly)); only restore --verifyOnly reaches disable() once.

Where it costs the most is the road on which the task then leaves the data alone — an exclusive lock which could not be taken (ImportTask.java:650), a restoreBackup() which threw — since the task enables the backend back from its finally and the domain loads a state which is no longer there over data that still holds every change it had replayed.

Reported by @maximthomas in #951 (comment).

What an empty ds-sync-state costs

The domain comes back with no position at all, and the two mechanisms that would recover from that both give up on it:

• checkAndUpdateServerState() returns on a null CSN for the local server (PersistentServerState.java:417-421); even without that guard it only ever repairs the local entry, since nothing local records how far the replay of a peer got;
• sessionInitiated() starts the missing-change publisher only when the local max CSN is newer than the one the replication server holds (LDAPReplicationDomain.java:5033-5045).

So everything the replication server still holds is replayed again — the safe direction — and the local writes it never received are never published, which is not.

The change

clearInMemory() leaves the emptied state marked as saved: it drops the copy in memory, and the backend keeps what it holds until something loads it back or writes newer CSNs. Both production callers — disable() and loadDataState() — want exactly that. clear(), which empties persistent storage too and whose only caller is PersistentServerStateTest, marks the state as not saved itself.

That is one line of behaviour. It closes the interleaving at disable(), the same clear-then-load gap in loadDataState(), and the double disable() above — the second call finds a state which is not waiting to be written, so its save() does nothing. It also keeps the exit save, which #945 has since guarded, from ever being handed a dirty emptied state.

Testing

All three tests were written first and watched failing against the code as it stood when they were written. Since #945 and #948 have landed in the meantime, here is what they do against origin/master as it stands now, with only this pull request's change to PersistentServerState reverted (JDK 21, mvn -Pprecommit verify):

test	with the change reverted, on current master
PersistentServerStateTest.clearInMemoryLeavesTheBackendStateAlone	fails on both suffixes: csn1 was dropped from persistent storage by clearInMemory() for o=test, then for cn=schema
DisabledDomainServerStateTest.aDomainDisabledTwiceKeepsItsPersistedState	fails: the second disable() dropped the position saved in ds-sync-state: lists don't have the same size expected [1] but found [0]
DisabledDomainServerStateTest.disabledDomainKeepsItsPersistedStateOnShutdown	passes — see below

The third one no longer reproduces anything on its own: #945's guard on the exit save keeps a shutdown which follows a disable() from writing at all, so the emptied dirty state never reaches the backend on that road. It is kept as what it now is — a regression test over the pair, which fails again if either the guard or this flag goes. The second one is the deterministic road with no threads in it: one disable() to save the position, a second one which must leave it alone, and it fails on master today. The setup the two share moved into a helper.

Green with the change in, on the same JDK 21 run: PersistentServerStateTest 11/11 (its own two invocations plus everything #948 added to that class), DisabledDomainServerStateTest 2/2, InitOnLineTest 10/10, ReSyncTest 2/2, GenerationIdTest 4/4, StateMachineTest 5/5 — the domain-level classes because they are the ones which drive disable()/enable() through the import and total-update roads, and none of them had been run against this change on top of #945 before.

Earlier, on the branch before it was rebased: green on JDK 21 and again on JDK 11, with the whole org.opends.server.replication package at 3565 tests and one failure — StateMachineTest.setUp losing the fixed admin port 65534 to another test JVM on the same machine, which is a port collision and not this change.

Not covered here

• No idempotence guard on disable(). With the flag right the second call has nothing to write, so a guard would be a second mechanism under the same assertion — and it is on the lines [#908] Wait for the changes being applied before a domain going down saves its ServerState #945 rewrote.
• An import or restore task that fails after its backend was disabled fires its end notification twice, and the first enable() runs against a deregistered backend — loading nothing and resetting the CSN generator through adjust(null). That is the task side of the same symptom, filed as A failed backend disable leaves a replication domain disabled for the life of the server #966.

Interaction with #948, and #945

Both have landed, and this branch is rebased on them.

#948 was a textual conflict in PersistentServerState.java, resolved the way this description said it would have to be: ServerState.clear() now clears the saved flag itself, so clearInMemory() ends as state.clear(); state.setSaved(true);, and clear() — which does mean to write the emptied state out — sets the flag back to false itself before saving. PersistentServerStateTest carries both sets of tests; #948's five save-race tests and its HookedWrite helper are untouched.

#945 shares no file with this change. Its reorder and its guard on the exit save close the shutdown road, as the table above shows, and narrow the checkpointer window rather than closing it: the flags are read before the modify is built, so a checkpointer already past the guard can still be handed an emptied state. The flag here is what closes that, and neither depends on the other.

[vharseko]

@maximthomas Rebased on master — #948 and #945 have both landed since this branch was cut.

The conflict was with #948, in PersistentServerState.java, and it is resolved the way this description said it would have to be: ServerState.clear() clears the saved flag itself now, so clearInMemory() ends as state.clear(); state.setSaved(true);, and clear() — the one which does mean to write the emptied state out — sets the flag back to false itself before saving. PersistentServerStateTest carries both sets of tests; #948's five save-race tests and its HookedWrite helper are untouched.

One thing the rebase changed for real, and it is on the road you raised

With #945 in, DisabledDomainServerStateTest.disabledDomainKeepsItsPersistedStateOnShutdown no longer fails without this change. The guard #945 put on the exit save — !disabled && !importInProgress() at LDAPReplicationDomain.java:647 — keeps a shutdown which follows a disable() from writing at all, so the emptied dirty state never reaches the backend on that road. I kept the test as what it now is: a regression test over the pair, which fails again if either the guard or this flag goes. Said so in the description rather than leaving the table claiming a failure it no longer produces.

What still fails on master with only this change reverted

Both on JDK 21, mvn -Pprecommit verify, everything else in place:

• PersistentServerStateTest.clearInMemoryLeavesTheBackendStateAlone — csn1 was dropped from persistent storage by clearInMemory(), on both o=test and cn=schema;
• DisabledDomainServerStateTest.aDomainDisabledTwiceKeepsItsPersistedState — the second disable() dropped the position saved in ds-sync-state: lists don't have the same size expected [1] but found [0]. That is the double-disable() road you named in Disabling a domain can persist an empty ds-sync-state and lose the replica's position #951, and [#908] Wait for the changes being applied before a domain going down saves its ServerState #945 does not touch it: the flag is what closes it.

Green with the change in, on the same run: PersistentServerStateTest 11/11, DisabledDomainServerStateTest 2/2, InitOnLineTest 10/10, ReSyncTest 2/2, GenerationIdTest 4/4, StateMachineTest 5/5. The domain-level classes are in because none of them had been run against this change on top of #945 before.

The description is updated with all of it: disable() is quoted as #945 left it, the exit save is no longer described as unconditional, and the line references are re-pinned to master.