[#916] Keep an update that lands during a ServerState save out of the saved flag

[vharseko]

Fixes #916.

The race

PersistentServerState.save() decided whether to write from a flag that the write itself cleared. runUpdateStateEntry() serialises the state once, at its top, and the flag was only set after the modify — which rewrites 99-user.ldif and takes hundreds of milliseconds — had completed. An update landing in between was lost: its CSN was not in the attribute being written, and the saved = false it had set was overwritten by the setSaved(true) of a write that did not carry it. Nothing set the flag again, so ds-sync-state stayed stale until the next meaningful update on the domain — which, on a domain as quiet as cn=schema, may never come.

The CSNs in the CI failure of #916 pin the window: the persisted state carries the replayed change from 19:38:31.846 but not the local one from 19:38:32.247, so the snapshot was taken between the two and the write completed after the later one, marking the state saved on behalf of a write that did not carry it.

The change

• save() marks the state as saved before taking the snapshot, so an update landing during the write clears the flag again and the next checkpoint writes it — at the cost of at most one redundant write per race.
• That ordering only holds if the dirty marker is published after the mutation it advertises, so ServerState.update() clears the flag once the new CSN is visible in the map rather than before touching it. As a side effect a duplicate or older CSN no longer marks the state dirty for nothing.
• The flag is restored when the write does not go through — a failure reported by the modify, or an exception on its way to the backend. The previous code left the state dirty in that case; the new ordering would otherwise have marked it saved for good.
• Only one save writes at a time. Two saves each took their own snapshot, and the write of the older one landing last left a state on disk that was both stale and marked as saved. The lock is taken with tryLock() and never waited for: a save which finds another one writing gives up its turn. Waiting would close a lock cycle — a write which goes to the domain configuration entry ends up in LDAPReplicationDomain.applyConfigurationChange(), which takes the very lock disable() holds while calling save(). Giving up the turn loses nothing, because the state is marked as saved before the snapshot in flight is taken: whatever the save which stepped aside had to write is either already in that snapshot, or has cleared the flag again after it was set, in which case the flag is still clear when that write completes and the next save writes it.
• ServerState.clear() marks the state it empties as not saved itself — and, like update(), only when it actually removed something. Loading a state that came in holding CSNs of its own no longer leaves it looking saved: the load only merges in what the backend holds. The old unconditional clearing in update() covered both by accident.

runModify() is split out of runUpdateStateEntry() as a seam, so a test can reach the point where the snapshot has been taken but the write has not gone through yet.

Testing

Every production change was watched failing first, through a direct mutation of the committed code:

disabled	fails
the flag ordering in save()	updateLandingDuringSaveIsWrittenByTheNextSave
saved = false after the map mutation	updateThatChangesNothingKeepsTheStateSaved
if (!written) setSaved(false)	writeThatFailsLeavesTheStateUnsaved, writeThatThrowsLeavesTheStateUnsaved, writeWithNoBaseEntryAndNoConfigEntryLeavesTheStateUnsaved, and the pre-existing persistentServerStateTest
saved = false in clear(), and its emptiness guard	clearMarksTheStateUnsaved
tryLock() replaced by a blocking lock()	aSaveGivesUpItsTurnWhileAnotherOneIsWriting
the guard in loadState()	stateLoadedOverCSNsOfItsOwnIsNotConsideredSaved

aSaveGivesUpItsTurnWhileAnotherOneIsWriting holds its write open until the second save has run and been checked, so the exclusion is a constructed fact rather than a timing window: the second save must return while the first write is provably still in flight, must not have reached the write, and must leave the state for the next save — which then puts the newer CSN on disk. The write in flight releases itself after 30 s, so a save which waits reports rather than wedging the suite.

Green on JDK 21 (mvn -Pprecommit verify): PersistentServerStateTest 9/9 (the race test runs against both o=test and cn=schema), ServerStateTest 6/6, SchemaReplicationTest 3/3 — including pushSchemaFilesChange, the intermittently failing test from the issue — ReplicationDomainTest 12/12, GenerationIdTest 4/4, InitOnLineTest 10/10, UpdateOperationTest 15/15. The setUp failures seen locally were another test run on the same machine holding the fixed test ports, and each of those classes passes on its own.

Not covered here

Three pre-existing hazards this change does not touch, all unchanged by it:

• Disabling a domain can persist an empty ds-sync-state and lose the replica's position #951 — disable() runs save() and clearInMemory() non-atomically, and the flush thread's exit save() is unconditional, so an emptied state can reach ds-sync-state. clearInMemory()'s javadoc now says so.
• An exception from a state write kills the checkpointer and hangs server shutdown #952 — an exception out of the write kills ServerStateFlush, and shutdown() then waits on while (!done) with no timeout.
• An import which disables a replication domain can deadlock against a write to that domain's configuration entry #967 — serviceStateLock and configLock deadlock against each other on the fallback path, and did so before this PR: a write to the domain configuration entry reaches applyConfigurationChange() — and so serviceStateLock — with configLock held, while disable() holds serviceStateLock across a save() which wants configLock. It is one lock ordering over two other files, so it is not touched here.

The second write of the baseDN -> configuration-entry fallback has no test. Reaching it needs a ds-cfg-replication-domain entry over the suffix, which would start a live domain with a checkpointer of its own writing the same state under every other method of PersistentServerStateTest; it belongs on a class which already runs a domain.

[maximthomas]

praise: The diagnosis is exact and the ordering contract is now right at both ends.

setSaved(true) is taken before the snapshot and saved = false is published after the map
mutation, in both CAS branches — so an update landing mid-write re-dirties the flag instead of being
swallowed. That makes all three ServerState mutators consistent (removeCSN() already had this
ordering). The runModify() seam turns a thread race into a deterministic test, the PR body's
"revert this line → this test fails" table is accurate (I checked writeThatFails* /
writeThatThrows*: they really do pin the finally), and the fix stays minimal on a 20-year-old
critical path. Filing #951 and #952 separately rather than folding them in is the right call.

---

issue (blocking): saveLock closes an AB-BA cycle with serviceStateLock.

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/PersistentServerState.java:127

The backend modify now runs inside saveLock. When the baseDN entry is missing, it falls through
to the domain's cn=config entry — still inside the lock:

ResultCode result = runUpdateStateEntry(baseDN);
if (result == ResultCode.NO_SUCH_OBJECT)
{
  SearchResultEntry configEntry = searchConfigEntry();
  if (configEntry != null)
  {
    result = runUpdateStateEntry(configEntry.getName());   // under saveLock
  }
}

That write goes ConfigurationBackend.replaceEntry → synchronized (configLock) →
ConfigurationHandler.replaceEntry, which loops every ConfigChangeListener synchronously and
diffs nothing — so LDAPReplicationDomain.applyConfigurationChange runs, and its body is
synchronized (serviceStateLock).

T1  ServerStateFlush : saveLock          -> config modify -> WANTS serviceStateLock
T2  disable()        : serviceStateLock  -> state.save()  -> WANTS saveLock

disable() holds serviceStateLock across state.save(), and sets disabled only afterwards, so
the flusher's !disabled guard does not help. Reachable on import/restore into an empty backend:
processImportBegin / processRestoreBegin call disable(), and a missing baseDN entry is exactly
what routes the save to the config entry. shutdown() then blocks forever on the flushThread
monitor. At base save() took no lock, so T2 never queued.

Fix: take the snapshot under the lock, do the write outside it — or use
ReentrantLock.tryLock() and let a losing saver skip the tick, since the flag already guarantees
the next tick retries.

---

issue (blocking): the save lock's only test can pass with the lock deleted.

opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/PersistentServerStateTest.java:267

Exclusion is inferred from a latch that must time out:

secondWriteGotIn.set(secondWriteStarted.await(2, SECONDS));
...
assertFalse(secondWriteGotIn.get(), "the second save reached its write while the first one was still writing");
assertEquals(mostWritersInside.get(), 1, "two saves wrote the state at the same time");

If the main thread needs more than 2 s to reach its own runModify (loaded CI, a GC pause), the
first write's await expires by itself, writersInside returns to 0, the second save takes the
else branch — and both assertions hold with the lock never contended. The failure direction is a
false green.

Blocking because the locking is going to be rewritten for the issue above, and this is the only test
that would catch the rewrite going wrong. Assert a positive observation (the second save found the
lock held), and fail when the contention window was missed rather than greening.

Two more in the same method, fix while you are there: the happy path always burns the full 2 s
(success is the timeout), and the main thread's state.save() blocks on saveLock untimed, so a
wedged lock hangs the suite instead of reporting.

---

note (non-blocking): the loadState() guard is unreachable in production.

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/PersistentServerState.java:166

final boolean hadCSNs = state.iterator().hasNext();
...
if (hadCSNs) { state.setSaved(false); }

Both src/main entries pass an empty state: LDAPReplicationDomain:3905 is
state.clearInMemory(); state.loadState();, and the constructor gets the ServerState that
ReplicationDomain created empty. Only the new unit test reaches it.

Keep it as defence in depth if you like — but a comment should say it is unreachable today, so it
does not read as the thing that makes the update() change safe. On the shipped paths that change
is safe for a different reason: merging into an empty map always mutates.

---

suggestion (non-blocking): clean up unconditionally.

opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/PersistentServerStateTest.java:305

checkpointer.join(SECONDS.toMillis(30));
if (!checkpointer.isAlive())
{
  state.clear();
}

A wedged checkpointer skips the clear and leaves a serverId-1 CSN in o=test's ds-sync-state.
ReplicationTestCase is sequential and the sibling methods hard-code serverId 1, so one real
failure cascades into exact-CSN failures that report the wrong defect. Clear through a fresh
PersistentServerState, as the other finally blocks already do.

---

suggestion (non-blocking): cover the baseDN → config-entry fallback.

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/PersistentServerState.java:285

writeThatFails injects UNWILLING_TO_PERFORM, never NO_SUCH_OBJECT, so runModify is called
exactly once in every new test. The fallback takes a second snapshot, written reflects only the
second write, and the hook fires twice. It is also the path the deadlock above rides on.

---

nitpick (non-blocking): doc and API residue.

• PersistentServerState.java:59 — the saveLock javadoc justifies the dedicated lock by not
  queueing behind checkAndUpdateServerState()'s monitor, but save() never took this at base
  either; the rationale describes a constraint that never existed.
• ServerState.java:47 — the new saved javadoc encodes PersistentServerState's protocol into a
  shared value class in common that enforces none of it.
• ServerState.java:70 — clear() dirties an already-empty, already-saved state, the opposite of
  the discipline this same commit imposes on update(CSN):

  public void clear() { serverIdToCSN.clear(); saved = false; }

• PersistentServerState.java:166 — state.iterator().hasNext() where ServerState.isEmpty()
  exists.

[vharseko]

Both blocking findings hold, and both are mine: the lock I added closes a cycle that was not there before, and the test I wrote to guard that lock cannot fail in the direction it was meant to. The lock is now taken with tryLock() and never waited for, and the test asserts a positive observation instead of a timeout.

issue (blocking): saveLock closes an AB-BA cycle with serviceStateLock

Confirmed, including the callback the finding names. ConfigChangeListenerAdaptor.applyConfigurationChange() (opendj-config) hands the entry to the registered listener unconditionally — it diffs nothing — so every ds-sync-state write which lands on the domain configuration entry runs LDAPReplicationDomain.applyConfigurationChange() (:4555, synchronized (serviceStateLock) at :4565) with configLock held (ConfigurationBackend.java:495). The domain is registered on exactly the entry searchConfigEntry() finds — configuration.addChangeListener(this) at LDAPReplicationDomain.java:710.

Fixed by never waiting on it:

// PersistentServerState.save()
if (!saveLock.tryLock())
{
  return;
}

Skipping the turn loses nothing, and the flag is what proves it. Let A be the save which holds the lock — it has done setSaved(true) at t_A and taken its snapshot at t_A' > t_A — and B the save which finds the lock held after reading saved == false at t_B:

• t_B < t_A: the mutation which cleared the flag published itself after the map was mutated, so it precedes t_B, hence t_A'. It is in A's snapshot.
• t_B > t_A: the flag was cleared after A set it, and A does not set it again — on success it leaves the flag alone, on failure it clears it. So the flag is still clear when A returns, and the next save writes it.

At most one redundant write per race, which is the cost the fix already carried.

I took this over the other option in the finding — snapshot under the lock, write outside — because that one does not keep what the lock is for. With the flag ordering, a second save only gets past the outer check when an update cleared the flag after the first setSaved(true); its snapshot is then strictly newer, and two writes in flight can still land in the order of their snapshots reversed. That is #916 again, one window narrower. Never waiting keeps the writes ordered and takes the cycle out.

clear() is the one caller which now depends on winning the lock, and it has no production caller — only tests, which is why the cleanup below goes through a state of its own.

The cycle which is left, and which I did not introduce

serviceStateLock and configLock already deadlock at base, on this same fallback path and without saveLock in it:

T1  ServerStateFlush : state.save() -> config entry modify -> configLock
                                    -> applyConfigurationChange -> WANTS serviceStateLock
T2  disable()        : serviceStateLock -> state.save() -> config entry modify
                                    -> WANTS configLock

disable() holds serviceStateLock across state.save() (:3869-3871) and sets disabled only afterwards, so the flusher's !disabled && !ieRunning() guard (:564) does not keep T1 out of a save which is already in flight. It is one lock ordering over two files I have not touched, so I have left it alone here rather than growing this PR. Say the word and I file it the way #951 and #952 were filed.

issue (blocking): the save lock's only test can pass with the lock deleted

Right, and the failure direction was a false green. The test is rewritten around a write which is held open until the second save has run and been checked, so the contention is not inferred from a window — it is constructed:

state.save();

assertFalse(writeInFlightWasReleased.get(),
    "the second save only returned once the write in flight had been released");
assertEquals(writesStarted.get(), 1, "the second save wrote while the first one was writing");
assertFalse(serverState.isSaved(),
    "a save which gave up its turn must leave the state to the next one");

The first two cover the two things the fix has to do — not write, and not wait — and the third with the two assertions after the release cover why not writing is safe: the state is left dirty and the next save puts the newer CSN on disk.

The three points you raised about it are all gone with the rewrite: success is no longer a timeout (the happy path releases the write as soon as the assertions are through), and nothing here blocks untimed — the write in flight holds itself for at most 30 s, so a save which waits reports rather than wedging the suite. Watched it fail: with tryLock() replaced by lock(), it fails at 30.97 s - when the write in flight lets go of itself - with

AssertionError: the second save only returned once the write in flight had been released
  expected [false] but found [true]

note (non-blocking): the loadState() guard is unreachable in production

Checked, and it is: both src/main entries come in with an empty state. Kept as it is — the guard belongs to what loadState() promises rather than to what today's two callers happen to pass — with the comment saying so, so it does not read as the thing which makes the update() change safe:

/*
 * ...
 * No shipped path comes in holding anything - the constructor is handed the
 * state a ReplicationDomain has just created, and loadDataState() empties
 * it first - so this guards a caller which does not exist yet rather than
 * one which does.
 */
final boolean hadCSNs = !state.isEmpty();

suggestion (non-blocking): clean up unconditionally

Done, and through a fresh PersistentServerState as you suggested — which the tryLock makes necessary rather than merely tidy: a clear() on the state the writer thread may still hold would give up its turn and clear nothing at all.

finally
{
  releaseTheWrite.countDown();
  writer.join(SECONDS.toMillis(30));
  new PersistentServerState(baseDn, 1, new ServerState()).clear();
}

suggestion (non-blocking): cover the baseDN -> config-entry fallback

Half done, and I will say which half. writeWithNoBaseEntryAndNoConfigEntryLeavesTheStateUnsaved drives NO_SUCH_OBJECT for the base entry and pins that the fallback wrote nowhere and left the state unsaved:

assertEquals(writtenTo, Collections.singletonList(baseDn.toString()),
    "the fallback wrote somewhere although no configuration entry holds this suffix");
assertFalse(serverState.isSaved(), "a state which reached no entry at all must be left unsaved");

The second write is still not covered, and not for want of trying: it needs a ds-cfg-replication-domain entry over the suffix, and adding one to this class starts a live LDAPReplicationDomain with a checkpointer of its own writing the same o=test state under every other method here. It is worth a test, on a class which already runs a domain rather than on this one.

nitpick (non-blocking): doc and API residue

All four:

• the saveLock javadoc no longer justifies itself with a monitor save() never took — it now says what the lock is and that it is never waited for;
• the saved javadoc in common states the ordering the field guarantees and stops there, without PersistentServerState's protocol in it;
• clear() leaves an already-empty state alone, the way update() leaves a duplicate CSN alone, and ServerStateTest.clearMarksTheStateUnsaved pins both halves;
• state.isEmpty().

Verification

mvn -Pprecommit -pl opendj-server-legacy verify, JDK 21:

• PersistentServerStateTest 9, ServerStateTest 6, SchemaReplicationTest 3
• ReplicationDomainTest 12, GenerationIdTest 4, InitOnLineTest 10, UpdateOperationTest 15

The setUp failures seen locally were another test run on this machine holding the fixed test ports; each of those classes passes on its own.

Each production change watched failing first, by disabling it on the committed code:

disabled	fails
tryLock() -> lock()	aSaveGivesUpItsTurnWhileAnotherOneIsWriting
the emptiness guard in clear()	clearMarksTheStateUnsaved

[maximthomas]

Reviewed at ec1d98aa against base 92d88ca6. The shape of the fix is right: deciding the flag from
a value the write itself invalidates is exactly the bug in #916, and setting saved before the
snapshot, restoring it in a finally, and moving the eager saved = false out of
ServerState.update()'s pre-loop into the successful branches all follow from that. tryLock()
rather than lock() is also right — applyConfigurationChange() reaches serviceStateLock with
configLock held, so a save that waited would close an AB-BA cycle.

My conclusion first: I would merge this as it stands. It removes a loss that fires on every save
overlapping an update — continuously on a busy domain, and permanently on one that goes quiet after a
burst, since the exit save then skips on the same flag — and it introduces no new failure class:
saveLock is tryLock-only, so nothing ever waits on it and no existing lock order can get worse,
and everything I found below is bounded by a resend and a duplicate replay rather than by divergence.
The only thing I would ask for before the merge is a correction to one row of the mutation table
(point 2 below), which is a change to the description and not to the code.

The rest is follow-up material. The first item is the one I care about, and I think it belongs in
the #951 fix rather than in this PR.

Follow-up: the save that steps aside is never made good at the callers that have no next save

PersistentServerState.java:134 and :139. Not a merge blocker — the closing paragraph of this
section says why.

save() can now return having written nothing, two ways:

• if (state.isSaved()) { return; } is true for the whole duration of another save's in-flight
  write, because setSaved(true) at :160 precedes updateStateEntry();
• if (!saveLock.tryLock()) { return; }.

The javadoc answers this with "the next save writes it". That holds for the ServerStateFlush
checkpoint loop, which comes round every second. It does not hold for the other three callers, and
LDAPReplicationDomain.java is not in this diff, so the change reads as safe locally and is not:

• disable() (:3869-3873) is state.save(); state.clearInMemory(); disabled = true; — what this
  save does not write is dropped from memory one statement later, and disabled is set only
  afterwards, so the checkpointer is not held off while it happens;
• backupStart() (:4110-4113) is a single state.save(); under the javadoc "We need to make sure
  that the serverState is correctly save.";
• the ServerStateFlush exit save (:576) is the last write of the process.

Concrete interleaving on disable():

1. the checkpointer takes saveLock, sets saved = true, and freezes its snapshot S on the first
   line of runUpdateStateEntry();
2. a replay thread commits and calls state.update(C) from RemotePendingChanges.commit() — under
   its own lock, not serviceStateLock — so the flag goes back to false and C is in memory but
   not in S;
3. disable() calls save(), sees the state dirty, loses tryLock(), and returns without writing;
4. clearInMemory() drops C;
5. the checkpointer's write lands S, without C.

On re-enable, loadDataState() reads the older watermark. Usually that is absorbed — the RS resends
the window and the historical information resolves the duplicates — but if the changelog has purged
it, the replica is declared out of date and needs a full reinitialisation.

How much worse than base this is, precisely: base had no lock, so disable()'s save and the
checkpointer's save ran concurrently as two REPLACEs of the same attribute. disable() always
issued a write carrying C, but which of the two landed last was not guaranteed by construction —
only by the checkpointer's operation having started earlier and therefore usually taking the entry
lock first. So base preserved C in the common interleaving and lost it in the reordered one, while
the head loses it in every contended case. The regression is that a conditional loss becomes a
certain one, not that a safe path became unsafe. Related and smaller: when the lock holder's write
fails, base gave a second attempt (disable()'s own); the head gives none.

Suggested fix, keeping tryLock() and adding no lock edge — either give save() a boolean return
and have disable() clear memory only when it is true, or have the save that steps aside set a
saveRequested flag that the lock holder re-reads before unlock() and answers with a second pass.
Either way the javadoc's "Giving up the turn loses nothing" needs qualifying for the terminal
callers.

Why I would not hold the merge on it: it needs a rare administrative operation — disable(),
backupStart() or shutdown — to land inside the checkpointer's few-millisecond write out of a
one-second period, with a replay update in flight; and the worst of the three legs is already broken
deterministically today by #951, where a disable() not followed by an enable() before
shutdown erases ds-sync-state outright. Both directions suggested there — set disabled before
dropping the state, or hold the save lock across the drop — close this leg as well, which is why the
make-good belongs in that fix. #945 also reorders disable() so the drain and disabled = true
precede state.save(), narrowing the same leg. backupStart() and the exit save are untouched by
either.

Smaller points

1. clear() / clearInMemory() javadoc no longer matches the code. ServerState.clear() resets
   saved only when the map was non-empty, so on an already-empty state PersistentServerState.clear()
   writes nothing while its javadoc says "Empty the ServerState and write the emptied state to
   persistent storage", and clearInMemory()'s new javadoc says the marking happens unconditionally.
   Latent — clear() has no caller in main today — but the two javadocs are what a future caller
   will read.

2. One row of the mutation table credits the wrong test. For
   if (!written) { state.setSaved(false); } the table lists the pre-existing
   persistentServerStateTest. Running that mutant: the cases that fail are
   writeThatFailsLeavesTheStateUnsaved, writeThatThrowsLeavesTheStateUnsaved and
   writeWithNoBaseEntryAndNoConfigEntryLeavesTheStateUnsaved — exactly three of the nine —
   while persistentServerStateTest passes in both parametrisations ([o=test] and [cn=schema]).
   This is the one point I would ask for before merging, since it is the PR's own evidence record and
   costs no code change.

3. The inner re-check is reached by no test. if (state.isSaved()) { return; } at :146 has no
   row in the table and no case reaches it: the second save always loses tryLock() first, and every
   other method in the class is single-threaded. So "every production change was watched failing" is
   one hunk short. Deleting the line costs one redundant modify, so this is a claim/coverage point
   rather than a defect — but as it stands the line is the only unpinned production change in the PR.

4. The ordering the fix rests on is pinned by nothing. A mutant that keeps the no-op semantics
   but moves saved = false back above the putIfAbsent/replace CAS in ServerState.update()
   passes both changed test files. No test anywhere drives a single ServerState from two threads —
   the racing update in updateLandingDuringSaveIsWrittenByTheNextSave runs on the saving thread,
   inside runModify. And update((CSN) null) in updateThatChangesNothingKeepsTheStateSaved
   passes at base as well, since the removed saved = false sat below the null guard; only that
   case's duplicate-CSN assertion actually fails at base. One test that drives two threads over one
   ServerState would pin the property the whole change is about.

Two issues this PR does not close

• #951 — disable() followed by a shutdown persists an empty ds-sync-state, because
  clearInMemory() leaves the state marked dirty and the checkpointer's exit save at :576 is
  unconditional. Unchanged here, in either direction; mentioning it because a reader of this PR may
  reasonably expect the clearInMemory() rework to have touched it.
• #952 — the new finally { if (!written) { state.setSaved(false); } } does leave the state
  correctly dirty when the write throws, and writeThatThrowsLeavesTheStateUnsaved pins that. The
  throw still escapes save(), and the ServerStateFlush loop catches only InterruptedException,
  so the checkpointer thread still dies on it.

For the record on the test side: PersistentServerStateTest is 9/9 green at this head (102.98 s),
and aSaveGivesUpItsTurnWhileAnotherOneIsWriting completes in 0.038 s, nowhere near its 30 s
self-release — no wedge risk there.