[#908] Wait for the changes being applied before a domain going down saves its ServerState

[vharseko]

Fixes #908.

disable() saved the ServerState, cleared it from memory and forgot the pending changes
while a replay thread could be half way through applying one of them: disableService()
stops the broker and joins the listener thread, and the replay threads are
MultimasterReplication's shared pool, which nothing signalled or waited for. A change
which reached the backend in that window was recorded nowhere, so the replication server
sent it again when the domain was enabled back and a change already in the data was
replayed a second time.

disable() is reached from an import-ldif task, from a restore task and from a
backend being taken offline by a configuration change. The first two replace the data
wholesale, so the lost watermark costs little there; the third does not, and that is where
the replica stays behind while reporting itself up to date.

What changed

A read/write lock spans the replay attempt. A replay thread takes the read lock around
every attempt it makes on the backend - the flags which tell it to stop are read under
that same lock - and a domain on its way down sets a flag and then takes the lock
exclusively: a change is either recorded in the ServerState which is about to be saved or
was never started. disable() also stops the replay before it saves the state rather than
after; the old order saved first, so it lost the change even on the interleavings the wait
does not need.

The flags are read once before the lock as well. The replay threads are a pool shared by
every domain, so a thread which took a change of a domain which is going down should not
queue behind that domain's wait - the changes of every other domain are behind it in the
same pool. That read is a scheduling optimisation for the common case rather than a
guarantee: the flag can be set and the writer can queue between it and the lock, and a
reader which arrives behind a queued writer blocks even on a lock which is not the fair
kind. The read under the lock is the one which decides.

The wait is bounded by REPLAY_DRAIN_TIMEOUT_IN_MS, and that bound is derived from the
ceiling the server itself puts on an operation which is waiting for an entry rather than
picked: LockManager gives the subtree lock and the entry lock
DEFAULT_LOCK_TIMEOUT each, so a replayed change whose target is held by a concurrent
local operation is inside its attempt for twice that before it gives up with BUSY. A
shorter bound would be spent by ordinary lock contention - no import, no index rebuild and
no wedged backend needed - and the change applied after it is exactly the one this barrier
exists to keep out of that window. It is a ceiling for the two locks of the target entry
rather than a guarantee: an operation also waits for the subtree lock of every entry above
it, one timeout each.

The bound is there at all because it is paid in three places: it is held under
serviceStateLock, it runs inside BackendConfigManager's write lock when a backend is
being deregistered, and a server going down spends it once per domain. It is only ever
spent in full by a replay which is genuinely stuck - the wait ends the moment the attempt
does - and a warning says what giving up costs. A wait which is interrupted rather than
spent says so in a message of its own, so the timeout it never spent is not read as a slow
backend. All of it stays under serviceStateLock so that disable() and enable() remain
the mutually exclusive pair they have always been: an enable() which ran in the middle
would clear the flag and bring a session up, and disable() would go on to cut that
session and clear a ServerState which the flush thread - reading a flag which says the
domain is enabled - would write back empty.

The same window is on the way to a shutdown: shutdown() stops the flush thread whose
final state.save() is the last thing which saves the ServerState, and a replay which
commits after that save is in the data and in no state. It takes the same wait, right after
the flag is set. That final save is now guarded, but on the direction of a total update
rather than on one running at all: a disabled domain cleared its ServerState from memory,
and saving it was a REPLACE of ds-sync-state with no value at all, so stopping a server
during an import or a restore left the replica with no ServerState rather than with the one
it saved on its way down. The disabled flag does not catch the total update this replica
is the target of - preBackendImport() sets ignoreBackendInitializationEvent, so
disable() is not called there - which is why the import is asked for. An export is not:
it leaves the data and the ServerState of this domain alone while the replay keeps running,
and the save in the loop above is skipped for either direction, so this is the only thing
which persists what was replayed while an export ran. ReplicationDomain.importInProgress()
is the accessor for that, next to ieRunning(), which cannot tell the two apart.

What this does not close

A change which reached the backend can still be left out of the saved ServerState by two
other roads, and the javadoc of the lock says so. commit() only advances the state over
the changes committed from the head of the pending list, so a change applied while an older
one is still to be replayed is forgotten by the clear() on the way down and sent again -
the barrier of #889 seen from the other side, and a thing to fix where that barrier is
rather than here. A change this replica made itself is the other, and the one the
ServerState recovery in PersistentServerState.loadState() already repairs, since it looks
for the CSNs of this server.

The PendingChange.owned mark added in #892 is not the predicate for the wait, although
#908 suggested it: owned stays set on a change parked in dependentChanges -
getNextUpdate() neither sets nor clears it - so it means "a replay thread is responsible
for this change", not "a replay thread is applying it right now". Waiting on it would never
finish on a domain with a dependency chain in flight.

Tests

aChangeBeingAppliedIsRecordedBeforeTheDomainIsDisabled parks the replay inside op.run()
and disables the domain around it, then reads the ServerState which was saved - not the
in-memory one, which disable() clears, and before the domain is enabled back, or the
change being sent again would make the state cover it either way. The parked replay is
released a fixed delay after the session was cut, which puts the release inside the wait
for a domain which waits and long after the save for one which does not: releasing on the
disconnection itself handed the replay the listener join as a head start, which is enough
for a domain which never waited to record it anyway. Taking the read lock out of the replay
attempt, and restoring the old order of disable(), both fail it on the assertion about
the saved state.

theDomainStopsWaitingForAReplayWhichDoesNotFinish holds the replay for good and checks
that the domain comes down all the same, reading the parked count rather than the log: only
the test releases the pause, so an operation still parked when disable() returns is one
the domain did not wait for. It asserts both ends of the bound - that the domain came down
short of the default, and that it did spend the budget it was given, which is what tells a
bounded wait from no wait at all.

Both budgets are counted against disable() as a whole rather than against the wait inside
it - the only moment either test can see is the session being cut, which is that method's
first act - so each of them has to dominate the rest of the method: the listener thread
asked to stop and joined with no bound on the join, and the ServerState saved with an
internal modify. That remainder has no ceiling on it - 3 ms here, hundreds of milliseconds
on the machine of the review - and a budget of its order would have the assertion satisfied
by the remainder alone, or hand the released replay a race against the save rather than a
loss to it. Both are 2 s: the wait the domain is told to take, and the delay the parked
replay is held for.

The park is a new PausePreParsePlugin: the pre-parse point is the one a replayed operation
goes through - the pre-operation plugins are not invoked for synchronization operations - so
it holds a replay thread between the moment it starts applying a change and the moment the
change reaches the backend.

ReplicationDomainTest.exportAndImport and exportAndImportAcross2ReplServers pin the
predicate the final save now turns on: FakeReplicationDomain records what
importInProgress() said while it was inside importBackend() and inside
exportBackend(), and the exporter must not report a total update into itself. Reducing
the accessor to "a context is held" fails them.

The shutdown half is not covered by a test of its own: the domain of the test class is
shared by every test in it, so shutting it down would take the rest of them with it. What
the tests above pin of the final save is its predicate, not the save.

Ordinals

WARN_REPLAY_NOT_DRAINED_319 and WARN_REPLAY_DRAIN_INTERRUPTED_320, moved off 310 and 311 in
edc056d. 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. The open PRs which add to replication.properties now hold 310-325 with nothing claimed
  twice.

[maximthomas]

praise: the barrier is correctly placed, correctly bounded, and honestly scoped

A few things worth naming, because they are the reason this design holds:

• The invariant is real, not assumed. remotePendingChanges.commit(curCSN) is reached from
  synchronize(PostOperationOperation), i.e. it runs inside op.run(). That is what makes a read
  lock around the attempt mean "applied to the backend ⇒ recorded in the ServerState about to be
  saved", rather than "probably recorded".
• Flag first, then lock — and the flag read twice. Reading goingDown before
  replayReadLock.lock() keeps a thread of the shared replay pool from parking on a dying domain
  while other domains' changes wait behind it; re-reading it under the lock is what actually decides.
  The two reads have different jobs and both are needed.
• Bounded, not unbounded. tryLock(timeout) is the right call: the drain is paid under
  serviceStateLock and, on the backend-offline road, inside BackendConfigManager's write lock.
  An unconditional wait there would hang an administrative operation on a wedged backend.
• PendingChange.owned rejected with a reason. owned stays set on a change parked in
  dependentChanges, so it means "a replay thread is responsible for this change", not "a replay
  thread is applying it right now". Waiting on it would never terminate on a domain with a
  dependency chain. Good call, and documented.
• The read lock is released on every exit path — continue, break, and an exception out of
  op.run() all pass through the finally. No return inside the guarded region.
• The PR says what it does not close. commit() advancing only over the committed prefix of the
  pending list is the honest residual, and naming it is worth more than closing it here.
• Pre-parse is the right plugin point. Replayed (synchronization) operations never reach
  pre-operation plugins, so a PreOperation fixture would have been dead code.
• setReplayDrainTimeout follows the existing seam — getReplayGiveUpDelay/
  setReplayGiveUpDelay in the same class already has the identical get / override /
  restore-in-finally shape.
• The second bug found on the way is a real one. The unconditional final state.save() writing
  an empty ds-sync-state over the state a disabled domain had just saved is a genuine, independent
  defect, and it deserved the guard it got.

---

issue (blocking): the new guard also suppresses the final save during an initialize-remote export

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

// base: unconditional
state.save();

// head
if (!disabled && !ieRunning())
{
  state.save();
}

ieRunning() is importExportContext.get() != null, and the export direction sets it too
(ReplicationDomain.initializeRemote -> acquireIEContext(false)).

Why that matters:

• Replay keeps running for the whole export — that is why a remote-requested export is dispatched to
  exportThreadPool in the first place — so the in-memory ServerState keeps advancing.
• The in-loop save was already skipped by the same predicate, and no other path persists
  ds-sync-state during an export (the only callers of PersistentServerState.save() are the two
  flush-thread sites, disable() and backupStart()). The final save was the only saver.
• checkAndUpdateServerState() repairs only CSNs whose serverId is our own, so nothing recovers
  the remote changes afterwards.

So: stop the server while an export is in flight — minutes to hours on a large backend — and on
restart the RS resends everything replayed since the export began, and every one of those changes is
replayed a second time. That is the #908 failure mode, on a different road. The base persisted it.

!ieRunning() cannot simply be dropped — it is the only net that catches the total-update import,
because preBackendImport sets ignoreBackendInitializationEvent = true, so disable() is not
called there and disabled is false. Guard on the direction instead; ImportExportContext already
records it:

if (!disabled && !importInProgress())
{
  state.save();
}

---

issue (non-blocking): the 5000 ms drain is shorter than the server's own entry-lock timeout

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java
(REPLAY_DRAIN_TIMEOUT_IN_MS) vs opendj-server-legacy/src/main/java/org/opends/server/types/LockManager.java

LockManager.DEFAULT_LOCK_TIMEOUT is 9 seconds, and tryWriteLockEntry spends it twice — once for
the subtree lock, once for the entry lock — before returning null:

private static final long DEFAULT_LOCK_TIMEOUT = 9; // SECONDS
...
lock.tryLock(DEFAULT_LOCK_TIMEOUT, TimeUnit.SECONDS);

So a replayed delete or modify whose target is locked by a concurrent local operation — a client
subtree delete over the parent, say — holds replayReadLock for 9-18 s and only then returns BUSY.
The drain gives up at 5 s, logs the warning, and disable() / shutdown() walk into exactly the
window this PR closes. No import, no index rebuild, no 60 s PersistIt write-write conflict required —
ordinary lock contention is enough. And the replay thread already inside op.run() cannot see
disabled; it is only re-read at the top of the next iteration.

Suggest deriving the bound from that constant rather than picking a round number:

private static final long REPLAY_DRAIN_TIMEOUT_IN_MS =
    2 * SECONDS.toMillis(LockManager.DEFAULT_LOCK_TIMEOUT) + 1000;

or, if 5 s is deliberate, say in the javadoc why giving up below the entry-lock ceiling is the right
trade.

---

issue (non-blocking): no test pins the barrier itself

opendj-server-legacy/src/test/java/org/opends/server/replication/UpdateOperationTest.java

theDomainStopsWaitingForAReplayWhichDoesNotFinish asserts only an upper bound:

Assertions.assertThat(PausePreParsePlugin.parkedCount(OperationType.DELETE)).isEqualTo(1);
assertTrue(waitedMs < drainTimeout, ...);

Delete awaitReplayDrained() entirely and both still pass: waitedMs is ~0 (below 5000) and the
replay is still parked. The test pins "the override is honoured", not "a wait happened". The missing
half is one line:

assertTrue(waitedMs >= 200, "the domain did not wait for the replay at all");

aChangeBeingAppliedIsRecordedBeforeTheDomainIsDisabled pins the statement reordering
deterministically — the base saved before disableService(), so the releaser can never beat it — but
it pins the lock only probabilistically: releaseWhenDisconnected fires on !domain.isConnected(),
which broker.stop() makes false at the start of disableService(), before listenerThread.join()
and well before the drain. With the lock removed, the released replay gets that whole join window as
a head start on state.save().

Also uncovered by design or by omission: the drain in shutdown(), and the new guard on the flush
thread's final save (which is where the blocking issue above lives).

---

suggestion (non-blocking): the interrupt path logs the timeout message

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java
(awaitReplayDrained)

The catch re-arms the interrupt and assigns nothing, so drained stays false and the code falls
into the warning:

Domain "%s" is going down and gave up on waiting up to %d ms for the replay ...

after waiting ~0 ms. An operator reads that as "the backend is slow". Either use a distinct message
or skip the warning when the wait was interrupted.

---

nitpick (non-blocking): two comments state things the code does not do

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

1. On the pre-lock flag read — "the read under the lock is the one which decides, the one above it
   only keeps this thread out of the queue". It cannot keep the thread out of the queue: the flag can
   be set and the writer can queue between the read and replayReadLock.lock(), and a non-fair
   ReentrantReadWriteLock blocks a reader arriving after a queued writer. It is a common-case
   scheduling optimisation, which is still worth having — just say that.

2. In the InterruptedException catch — the rationale that the caller is a task being cancelled and
   needs the interrupt. Task cancellation here is cooperative: ImportTask and RestoreTask only
   call setTaskInterruptState and importConfig.cancel(), and the whole backends/task package
   contains a single .interrupt(), aimed at the scheduler thread. Keep the re-arm — it is correct
   hygiene — but not for that reason.

---

nitpick (non-blocking): traps in the new test fixture

opendj-server-legacy/src/test/java/org/opends/server/plugins/PausePreParsePlugin.java,
opendj-server-legacy/src/test/java/org/opends/server/replication/UpdateOperationTest.java

• The comment above getEntry(dn, 30000, false) credits the redelivery, but the finally released
  the pause before enable(), so the released replay deletes the entry itself. Only the
  getServerState().cover(csn) timer below actually evidences redelivery.

• awaitPaused() conflates "no pause registered" with "timed out":

  return pause != null && pause.reached.await(timeout, unit);

  A wrong OperationType at the pause() call site therefore reports "the replay thread never
  started applying the change" after burning the full 60 s. Failing loudly on a missing pause would
  save the next person that hour.

• parkedCount() after release() is not guaranteed to read 0 — the decrement happens in the parked
  thread's finally. No current call site depends on it, but the javadoc invites the read.

[vharseko]

@maximthomas e50b4bda6b. The blocker is fixed, and the predicate it now turns on is pinned by a test. The six other items are in as well. What is still uncovered is named at the end rather than left implied.

issue (blocking): the guard also suppresses the final save during an initialize-remote export

Confirmed in the shape you described. initializeRemote() acquires the context through acquireIEContext(false), so ieRunning() is true for an export just the same; PersistentServerState.save() has no caller which runs during one - the two flush-thread sites, disable() and backupStart() - and checkAndUpdateServerState() repairs only CSNs whose serverId is ours. The final save really was the only thing which persisted what the domain replayed while the export ran.

Fixed as you suggested, by asking for the direction rather than for the context:

      if (!disabled && !importInProgress())
      {
        state.save();
      }

ImportExportContext.importInProgress() is package private in replication.service, so the plugin package cannot reach it. The accessor is a protected boolean importInProgress() on ReplicationDomain, next to ieRunning(), which is where the asymmetry belongs: ieRunning() cannot tell an export from an import, and anything guarding the data of this replica has to.

!ieRunning() stays on the save in the loop. That one is pre-existing on both directions and this PR has no reason to widen it - the final save is the one your reading is about.

One thing worth writing down now that the predicate is narrower: there is no window on the import road where the context is gone and the ServerState is still empty. initialize() releases it in completeInitializeTask() from its own finally, after importBackend() returned, and LDAPReplicationDomain.importBackend() runs loadDataState() in a finally of its own before that.

Test. ReplicationDomainTest.exportAndImport and exportAndImportAcross2ReplServers now assert the direction each side reported while it was in it: FakeReplicationDomain records importInProgress() at the top of importBackend() and of exportBackend(). With the accessor reduced to ieCtx != null - the predicate this fix replaces - both fail on the exporter, three failures over the two data-provider rows and the two-RS case:

[ERROR] ReplicationDomainTest.exportAndImport:369->assertExportSucessful:800 the replica which is
exporting must not report a total update into itself: its data and its ServerState are left alone
expected [false] but found [true]

issue (non-blocking): the 5000 ms drain is shorter than the server's own entry-lock timeout

Right, and it is the ordinary case rather than the pathological one. LockManager.tryLock() spends DEFAULT_LOCK_TIMEOUT on the subtree lock and again on the entry lock, and the server always builds its manager with the default - new LockManager() in DirectoryServer is the only construction in the tree. Taken as you wrote it:

  private static final long REPLAY_DRAIN_TIMEOUT_IN_MS =
      2 * LockManager.DEFAULT_LOCK_TIMEOUT_UNITS.toMillis(LockManager.DEFAULT_LOCK_TIMEOUT) + 1000;

The two constants had to become public for that; they were private, with the number restated in prose on the constructor's javadoc.

The javadoc now says what the bound buys and what it does not. It is a ceiling for the two locks of the target entry, not a guarantee: tryAcquireParentSubtreeReadLock() spends one timeout per ancestor as well, so a deep contended chain outlasts it. The cost argument moved with it - 19 s is only ever spent in full by a replay which is genuinely stuck, since the wait ends the moment the attempt does, and what is on the other side of that trade is the loss this PR exists to close.

issue (non-blocking): no test pins the barrier itself

Both halves confirmed, and both fixed.

Your one line, against the override rather than a repeat of its value:

        assertTrue(waitedMs >= TEST_REPLAY_DRAIN_TIMEOUT_IN_MS,
            "the domain came down in " + waitedMs + " ms, so it did not wait the "
                + TEST_REPLAY_DRAIN_TIMEOUT_IN_MS + " ms it was given ...");

The second half was worse than probabilistic: the head start made the mutant pass rather than fail. Releasing on !domain.isConnected() gave the replay the listener join to finish in, and a replay which finishes before state.save() is recorded whether or not anything waited for it. The release now happens a fixed 500 ms after the disconnection - inside the wait for a domain which waits, and long after the save for one which does not.

Watched failing, twice:

mutation	UpdateOperationTest
awaitReplayDrained() out of disable()	aChangeBeingApplied... "a change which reached the backend must be recorded in the saved ServerState expected [true] but found [false]"; theDomainStopsWaiting... "the domain came down in 0 ms"
the write lock taken on a lock object of its own - the barrier gone, the wait kept	the same two, "came down in 1 ms"

Before this round the first mutation left theDomainStopsWaiting... green, and the second left both green.

suggestion (non-blocking): the interrupt path logs the timeout message

Distinct message, WARN_REPLAY_DRAIN_INTERRUPTED, on a flag set in the catch. It says the wait was interrupted and repeats the consequence; the timeout it never spent is not in it, which was the thing an operator would have read a slow backend into.

nitpick (non-blocking): two comments state things the code does not do

Both right.

• The pre-lock read says what it is now - a scheduling optimisation for the common case - and says why it cannot be more: the flag can be set and the writer can queue between the two reads, and a reader arriving behind a queued writer blocks even without fairness. (The lock's own javadoc already said that second part; the comment at the call site contradicted it.)
• The InterruptedException rationale no longer claims task cancellation. Checked: ImportTask and RestoreTask only call setTaskInterruptState() and importConfig.cancel(), and the single .interrupt() in backends/task is aimed at the scheduler thread. The re-arm stays, for the reason it is actually good hygiene - whoever interrupted this thread is still waiting for it, and this is not the last thing it does.

nitpick (non-blocking): traps in the new test fixture

All three.

• The comment above getEntry(dn, 30000, false) says what that line evidences - the change is in the data, applied by the first replay after the give-up - and the redelivery is credited to the cover(csn) timer below it, which is what actually shows it.
• awaitPaused() throws IllegalStateException naming the operation type when no pause is registered, instead of reporting a wrong OperationType at the call site as "the replay thread never started applying the change" an hour later.
• parkedCount()'s javadoc says it has to be read before the release, and why: the decrement is in the parked thread's finally.

Still not covered

• The final save itself, and the drain in shutdown(). What the new assertions pin is the predicate the guard turns on, not the guard. The shutdown road still has no test of its own for the reason the description gives - the domain of UpdateOperationTest is shared by every test in the class, so shutting it down takes the rest of them with it - and a test which held an export open on a real LDAPReplicationDomain and then shut the domain down would be a test class of its own. Worth doing; not in this round.
• The barrier tests kill the barrier being removed and the wait being removed. They do not pin the order of state.save() against state.clearInMemory(), or the remotePendingChanges.clear() which follows both.

Verification

mvn -Pprecommit -pl opendj-server-legacy verify - 76 tests, no failures:

• UpdateOperationTest 17, ReplicationDomainTest 12, InitOnLineTest 10, GenerationIdTest 4, LockManagerTest 33

The three mutations above were run one at a time against this head; each produced exactly the failures listed and no others.

[vharseko]

Ordinals moved: WARN_REPLAY_NOT_DRAINED 310 → 319, WARN_REPLAY_DRAIN_INTERRUPTED 311 → 320 (edc056d).

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.

[maximthomas]

Approving. All seven items of the previous round are closed in code, and the delta broke nothing that
was clean. Round 2 scoped itself to the authored delta 4ce8365d..edc056d8 plus the four files the
previous round never saw — ReplicationDomain, types/LockManager, FakeReplicationDomain,
ReplicationDomainTest.

The blocker fix is right, and narrowing on the direction rather than dropping the guard is the version
that keeps working: importInProgress() is exactly ieRunning() for the whole lifetime of an import
context, and importBackend() runs loadDataState() in its own finally before initialize()'s
finally releases the context, so there is no window where the context is gone and the state is still
empty. The FALSE-on-exporter assertion does kill the ieCtx != null mutant — the recording fields are
volatile, written at the top of export/importBackend, and read only after initializeFromRemote
returned.

REPLAY_DRAIN_TIMEOUT_IN_MS derived from LockManager's own constants is the right shape, and
DirectoryServer is indeed the only production new LockManager(), so "the one the server runs with"
holds.

Ran at edc056d8 in a detached worktree: UpdateOperationTest 17/0/0 twice (158.7 s, 160.5 s),
ReplicationDomainTest 12/0/0 (136.6 s). Matches the counts in the thread. Note the ten build-maven
cells are still pending on this head at the time of writing.

One request, not a blocker

Both constants that carry the barrier's mutation pin are measured against disable(), not against
the drain. isConnected() flips false at setConnectedRS(ConnectedRS.stopped()) inside
ReplicationBroker.stop(), which is disableService()'s first statement. Everything after it — the
rest of stop(), listenerThread.initiateShutdown(), the unbounded listenerThread.join(),
sessionGeneration++ — runs before awaitReplayDrained() is entered.

So:

• assertTrue(waitedMs >= TEST_REPLAY_DRAIN_TIMEOUT_IN_MS) brackets the whole of disable(). Whenever
  disableService() + state.save() alone cost more than the 200 ms override, the mutant with
  awaitReplayDrained() deleted still satisfies it. Measured here, the case takes 483 ms in total
  against a 200 ms drain, which bounds the whole non-drain remainder of the method at 283 ms — a margin
  of the same order as the quantity it has to dominate.
• SETTLE_BEFORE_RELEASE_IN_MS = 500 is spent against that same unbounded join, not against the drain.
  If the join outlasts it, the parked replay is freed while disable() is still inside
  disableService(), and the barrier-less mutant races state.save() instead of losing to it.

Neither can fail the honest build, which is the problem: if the margin goes, nothing says so. Raising
TEST_REPLAY_DRAIN_TIMEOUT_IN_MS to ~2000 (costs 1.8 s) and SETTLE_BEFORE_RELEASE_IN_MS to ~2000
(free — that test does not override the 19 s budget) restores it. Timing the drain from a probe inside
awaitReplayDrained() would be the structural version, if it is worth the seam.

[vharseko]

@maximthomas dda75bb3. Taken as asked - both budgets are 2 s now - and the reason they were wrong is written where they are declared rather than left to be re-derived.

One request: both constants are measured against disable(), not against the drain

Confirmed in the shape you described. isConnected() falls inside setConnectedRS(ConnectedRS.stopped()) and broker.stop() is the first statement of disableService(), so everything between the only moment either test can see and the drain being entered - the rest of stop(), listenerThread.initiateShutdown(), the join with no bound on it, sessionGeneration++ - is inside the quantity being measured. waitedMs brackets that plus state.save(), and SETTLE_BEFORE_RELEASE_IN_MS is spent against the same remainder rather than against the wait.

One thing to add to it: that remainder has no ceiling at all, rather than a high one. The join is unbounded by construction, state.save() is an internal modify through the whole operation pipeline, and entering disable() is taking serviceStateLock - which a replay thread restarting its session holds across an enableService().

TEST_REPLAY_DRAIN_TIMEOUT_IN_MS 200 → 2000, SETTLE_BEFORE_RELEASE_IN_MS 500 → 2000. The first costs the run 1.8 s; the second is spent inside the 19 s that test leaves at its default and adds 1.5 s of wall clock.

Measured, since the two numbers in this thread are two orders apart. With awaitReplayDrained() deleted from disable() at this head, both barrier tests fail:

UpdateOperationTest.aChangeBeingAppliedIsRecordedBeforeTheDomainIsDisabled:2177
  a change which reached the backend must be recorded in the saved ServerState expected [true] but found [false]
UpdateOperationTest.theDomainStopsWaitingForAReplayWhichDoesNotFinish:2286
  the domain came down in 3 ms, so it did not wait the 2000 ms it was given for the replay of a change which was still being applied

3 ms, and that under a load average of ~35 with two other builds on the box. The remainder is cheap here by construction: setConnectedRS() closes the old session before it publishes the stopped state, so the listener is already awake by the time isConnected() falls and the join costs nothing. Which is exactly why your 483 ms matters - the same construction is outrun by two orders of magnitude on another machine, and sizing a budget by what the fastest box measures is how the margin goes without anything reporting it.

Honest build at the same head: UpdateOperationTest 17/0/0.

The structural version, not taken. Saying so rather than leaving it implied. A probe timing the wait from inside awaitReplayDrained() does pin the second test exactly, but it has to be read-and-reset - the domain is shared by every test in the class, so the first test's drain would satisfy the second test's assertion on its own - and the first test needs a seam of its own on top of it: replayLock.hasQueuedThreads() exposed, so that the release waits for the domain to be provably queued behind the parked replay instead of for a delay. Two accessors into the production class, against a margin which is now 7x on the slower of the two machines this has been measured on. If you would rather have the seams anyway, say so and they go in.