[#889] Keep a change the replay could not apply out of the ServerState

[vharseko]

Fixes #889.

A change whose replay fails with anything other than NO_OPERATION, BUSY or UNAVAILABLE is recorded as replayed: the ServerState advances past it, the replication server never sends it again, and an assured (SAFE_READ) ack goes back to the originating master as if the change had been applied. The replica silently diverges while reporting itself fully caught up, with unresolved-naming-conflicts at 0 and a single line in the error log. This is storage-agnostic: JE, PersistIt and JDBC all reach it on any StorageRuntimeException.

What changed

The four solveNamingConflict() overloads no longer collapse two outcomes into one return true. They report a ConflictResolution (REPLAY_AGAIN / NOTHING_TO_DO / FAILED), so replay() can tell "the operation became a no-op after conflict resolution" from "the operation failed". The four copies of the ERR_ERROR_REPLAYING_OPERATION log move to the single place which decides what to do with the failure.

A failure of the server itself is no longer recorded as applied. UNAVAILABLE, BUSY and the server-error-result-code - the code BackendImpl.createDirectoryException() puts on every StorageRuntimeException, 80 by default - are retried in place, and if they keep failing the change is deliberately left out of the ServerState. The replication server still owns it, so the domain restarts its session and the change is delivered and replayed again on a backend which has hopefully recovered. The codes conflict resolution knows how to solve (NO_SUCH_OBJECT, ENTRY_ALREADY_EXISTS, NOT_ALLOWED_ON_RDN, NOT_ALLOWED_ON_NONLEAF, and UNWILLING_TO_PERFORM / OBJECTCLASS_VIOLATION which solveNamingConflict(ModifyDNOperation) solves too) are excluded from that test: server-error-result-code is configurable and is not validated as a result code, and it must never take a change away from solveNamingConflict(). Conflict resolution getting its chance first is all that exclusion means, though: a change which comes back FAILED on the configured server-error-result-code is retried as the server failure it is, rather than recorded as replayed after one attempt - and retried in place as many times as an unavailable backend is, so a storage busy for a moment does not cost a session restart. Which road it takes once those attempts are spent is decided by the result of the attempt which spent the last of them rather than by an earlier one: a change whose later attempts kept coming back on something conflict resolution rewrote is the replay loop, and is reported and counted as one.

A change this replica can never turn into an operation is given up on, not left in the way. A message which fails to decode has no operation to retry and no delivery which would decode any better, so it is skipped where it is reported - counted, alerted on, and out of the way. Left listed, uncommitted and owned by the replay thread which failed on it, it was the barrier holding this domain's ServerState - and every change behind it, from every master - back for good, since the delivery which would replace it is turned down while a replay thread owns it. The wedge is as old as the decode path; what is new is that ownership closes the accidental way out master had, where putRemoteUpdate() overwrote the listed copy and an unrelated redelivery let the listener push the CSN through.

A change whose replay failed is released, not forgotten. Ownership is a mark on the change itself. replayFailed(csn) drops the mark and leaves everything else alone, so the change stays listed and uncommitted - it is the barrier which holds the ServerState back, which is what makes the replication server send it again - and it stays among the changes newer ones are checked against, because a change which is not in the data is exactly what the changes which follow it must depend on. putRemoteUpdate() then takes over an unowned change: the delivery which comes next is the one to replay, while a change a replay thread still owns is refused as the duplicate check of OPENDJ-1115 wants. The copy which the previous delivery left in the shared replay queue is dropped when a replay thread takes it out, because markInProgress() only accepts the delivery which is listed as pending; it is counted as processed all the same, so replication-processed-updates does not drift.

The changes another replay thread is applying at that moment are left alone: they commit as usual, and the head-of-line barrier keeps their CSNs - and the failed one - out of the ServerState until the failed change is applied. Forgetting them would have their commit() fail, the ServerState stay behind them and the replication server replay them a second time, which is how a child Add resent while its parent is not in the data ends up renamed to a conflict RDN for good.

Two things follow from keeping the pending changes: putRemoteUpdate() no longer overwrites the copy which is listed with the one a new delivery came with (which lost the fact that it had been replayed), and a duplicate delivery no longer has the listener thread push its CSN to the ServerState - the copy which is listed owns the change and records it once it really has been replayed, while the ack and the window credit stay per delivery.

Every failed replay is reported, including the one a replay thread abandons when it is stopped - the ack says the change is not in the data here, rather than being the plain ack master sent, and the change is given back to the replication server only once that ack has been published, since handing it back stops and starts the session the delivery came over. replayErrorMsg is set on all failure paths, so the SAFE_READ ack carries the replay error instead of telling the originating master that the write is durable here. The ack belongs to the delivery and states what that delivery did: a change which failed is not in the data at that point, whether or not the delivery which follows manages to apply it. Holding it back until the change is resolved would not be more truthful - the session it came over is torn down a moment later, so nothing would reach the server waiting for it and an assured write would wait out its timeout instead of being told what happened. The new replayed-updates-failed monitor attribute counts the changes this replica gave up on - once each, like replayed-updates-ok counts the ones it applied.

A change which can never be applied here does not stop the replica for good. The failures are counted on the PendingChange itself - the change which stays listed as the barrier - so they are kept for as long as the change is not in the data, whichever delivery is replaying it, and they go away with it when it commits or when a disabled domain forgets it. A backend which is failing fails every change in flight, so a single counter would be reset by each of them in turn, and a bounded map on the side would have a change evicted between two of its own failures and its budget restarted. The budget is a duration rather than a number of attempts: what isServerFailure() reports is measured in minutes, because a backend which is being rebuilt, imported into or restored (OPENDJ-49) serves nothing while it works, and a handful of attempts would have this replica give up on every change of a maintenance window it only had to wait out. After five minutes of failing, the change is skipped, but loudly: ERR_REPLAY_SKIPPING_CHANGE plus the new UnreplayedChange alert telling the administrator that this replica has diverged and must be reinitialized. The attempts in between are logged as WARN_REPLAY_RETRYING_CHANGE, and the session is left down for a moment before the change is asked for again, so that a backend which keeps failing is not asked for every change as fast as the replication server can send them. That backoff counts the session restarts of the domain rather than the failures of whichever change opened the recovery - with an outage failing everything in flight, a change being delivered for the first time would otherwise keep the wait at its shortest - and it is capped at ten seconds, because it runs on a replay thread every domain of this server shares. Giving up on a change does not start that backoff over either: whatever made that one unreplayable is still failing the changes in flight with it, so a replica which gives up on a change now and then would otherwise ask for the whole backlog of an outage at the shortest wait. Only a change which was really applied resets it, and only when it is the last one which was failing: RemotePendingChanges counts the changes with a failure recorded against them, because a change which can never be applied here fails alone, among changes which replay perfectly well. Resetting on any success at all had a single poison change tear the session down and rebuild it once a second for the whole give-up window, which is the hammering this backoff exists to prevent; an outage still resets it on the first change which gets through. Both messages count deliveries - each of which is attempted ten times in place - and the give-up prints how long the change has been failing in milliseconds, so that a give-up shorter than a second does not read as zero. Both durations are measured on a clock which only moves forward, so a wall-clock step does not decide whether a replica diverges.

The session is stopped and started in one place. disable(), enable(), shutdown(), the fractional configuration being changed and the recovery serialise on one lock, and every one of them bumps a session counter, so the recovery only starts a session back if the session it stopped is still the one which is down - a configuration change, or the end of an import, cannot be undone by a replay thread which was sleeping through it. readFractionalConfig(), changeConfig() and readAssuredConfig() are held under that lock over the whole stop, mutate and start: a session brought up in between would read a configuration half way through being changed. The recovery checks both the counter and the listener still being down, since a restart made outside the lock leaves the counter untouched. A disabled domain also forgets its pending changes, once its listener is stopped rather than while it can still list one: its ServerState is saved and read again from the backend when it is enabled back, and a change which stayed listed would be discarded as a duplicate with nothing left to replay it. Finally, a replay thread which is stopped - when the number of them is changed - hands the change it was replaying back to the replication server instead of leaving it listed as owned by a thread which is gone.

What deliberately did not change

Failures which are not the server's fault - a schema or constraint violation, or the conflict-resolution loop of #798 - keep being skipped, as they always were: redelivering them cannot help, and stopping the domain on them would turn a one-entry divergence into an outage. What is new for them is that they now carry the same honest ack, counter and alert, so the divergence is visible instead of silent: a ConflictResolution.FAILED - OBJECTCLASS_VIOLATION, CONSTRAINT_VIOLATION, INSUFFICIENT_ACCESS_RIGHTS, ADMIN_LIMIT_EXCEEDED - now raises UnreplayedChange and fails the SAFE_READ ack after one try, where master only logged ERR_ERROR_REPLAYING_OPERATION. Restarting the session on those failures was tried first and is what UpdateOperationTest.infiniteReplayLoop and namingConflicts rightly rejected.

Tests

test	covers
RemotePendingChangesTest (new)	an uncommitted change holds back the ServerState; a change whose replay failed stays listed as the barrier, keeps holding back a change another thread commits in the meantime, and lets the next delivery take over from the one which failed, while a change a replay thread owns is refused; only the delivery which is listed as pending is replayed; a disabled domain forgets everything
RemotePendingChangesTest (new)	failures are counted per change; the budget survives the delivery which takes over; however long a delivery takes to fail, its failures belong to the same run; a change which is replayed or forgotten keeps none of them; and a change keeps its budget while 1500 other changes fail in between
UpdateOperationTest.failedReplayIsNotRecordedAsReplayed (new)	the change is delivered again after a failed replay - which takes more short circuits than the in-place attempts of a single delivery, and is impossible if the ServerState had advanced - the entry is untouched, and the replica eventually gives up, counting the change once - and the count is then watched to stay put, since a counter bumped once per attempt passes through the expected value on its way up - and raising the UnreplayedChange alert
UpdateOperationTest.everyChangeWhichCanNotBeReplayedIsGivenUpOn (new)	two changes failing at once - what a backend outage looks like - are both given up on, which a counter kept for the last failed change only never reaches, and are counted once each - a count which is watched to stay put rather than only to be reached
UpdateOperationTest.transientReplayFailureIsRetriedAndTheChangeApplied (new)	a backend which stays unavailable, or a lock which stays taken (BUSY), for longer than the in-place attempts, then serves the operation, has its change applied exactly once - only reachable through a session restart and a second delivery - with nothing counted as failed and no alert raised. The exactly-once assertions read the counter until it settles, so a change applied twice fails them rather than passing on the way through
UpdateOperationTest.changeConflictResolutionCanNotSolveOnTheServerErrorCodeIsRetried (new)	with server-error-result-code set to a code conflict resolution owns, a change which fails with it is retried and applied rather than recorded as replayed after one attempt
UpdateOperationTest.aChangeWhichCanNotBeDecodedIsNotLeftHoldingTheServerStateBack (new)	a message which travels the protocol intact and can not be turned into an operation - the encoded modifications are only read by createOperation() - is skipped rather than left holding the ServerState back, counted once and alerted on
RemotePendingChangesTest.aChangeKeepsFailingWhileTheChangesAroundItAreReplayed (new)	a change stays counted as failing while the changes around it are applied, which is what keeps the session restart backoff climbing through a poison change
RemotePendingChangesTest.failingChangesAreCountedOnceAndForgottenWithTheChange (new)	however many times one change fails it is counted once, and it stops being counted when it is applied or when a disabled domain forgets it
AssuredReplicationPluginTest.testSafeReadModeReplyWithReplayError (new)	the SAFE_READ ack carries hasReplayError and failedServers=[1]

Each of the three new tests was checked against a mutation of the code it covers, so that none of them is a restatement of the implementation: removing the give-up on the undecodable change, dropping the first-failure guard on the counter, and clearing the count on any commit each fail exactly one of them.

The AssuredReplicationPluginTest one closes a TODO which has been in the tree since upstream: "make the domain return an error: use a plugin? The resolution code does not generate any error so we need to find a way to have the replay not working to test this...". The short circuit has to be set at the pre-parse plugin point - LocalBackendDeleteOperation and friends skip the pre-operation plugins for synchronization operations.

UpdateOperationTest, RemotePendingChangesTest and AssuredReplicationPluginTest: 42/42 green on the current head - the three classes which cover the replay decision, the give-up budget and the assured ack this round changes. GenerationIdTest, InitOnLineTest, MonitorTest, StateMachineTest and ChangelogBackendTestCase were green on the previous head and are run by CI on every push. (ReplayFailuresTest went with ReplayFailures when the budget moved onto the PendingChange.)

Left for later

• The ReplicaOfflineMsg grace period is spent on the first message of the process: DSRSShutdownSync latches its timestamp and never resets it #900 - DSRSShutdownSync.stopInstanceTimestamp is latched by the first ReplicaOfflineMsg a process ever sends, so the grace period which lets a collocated replication server forward it is already spent at the shutdown it is meant to cover. Any online import trips it, with or without this PR.
• Disabling a replication domain can drop a change a replay thread is applying #908 - disable() can drop a change a replay thread is mid-apply: the replay path takes no serviceStateLock and disableService() joins only the listener, so a thread can be between markInProgress() and commit() when state.save() persists a watermark excluding it. Pre-existing - master had no clear() there and replayed the change twice silently, where this PR logs ERR_OPERATION_NOT_FOUND_IN_PENDING.
• No test for the change a stopped replay thread hands back to the replication server #909 - the change a stopped replay thread hands back has no test: parking a replay thread inside op.run() while num-update-replay-threads is reconfigured needs a blocking test plugin, which would cover Disabling a replication domain can drop a change a replay thread is applying #908 as well.
• No test for a ModifyDN conflict solved while server-error-result-code is one of the conflict codes #910 - a ModifyDN conflict solved while server-error-result-code is one of the codes conflict resolution owns has no test; the new test covers the other half, the change conflict resolution can not solve.
• the abandon on a live num-update-replay-threads change acks every in-flight assured SAFE_READ write with a replay error, for changes this replica applies moments later: either a replay thread finishes the change it owns before it exits, or the cost is written down in the javadoc.
• abandonReplay() logs nothing when the domain is disabled for an import or a restore, while the ack still carries the error - the peer records a replay error the local log never explains.
• a poison change still withholds the ServerState for the whole give-up window. The backoff now climbs to its cap through it rather than being reset by the changes which replay around it, so the cost is a session restart every ten seconds rather than every second, but telling a storage failure from a change which can never be applied here wants the marker exception the review asked for rather than a configurable result code.
• changeConflictResolutionCanNotSolveOnTheServerErrorCodeIsRetried does not pin conflict resolution being reached: putting UNWILLING_TO_PERFORM back into isServerFailure() gives the same observables. Neither does any test cover the isCommitted() half of recordReplayFailure()'s guard - a change committed behind an older uncommitted one - nor sessionGeneration and readFractionalConfig() under serviceStateLock.
• Make the replay retry budget of a replication domain configurable instead of a constant with a test-only setter #901 - the retry budget and the attempt counts are constants with a @VisibleForTesting setter; they belong in ReplicationDomainCfg, next to replay-thread-number, so that an operator whose maintenance windows are longer than five minutes can say so.

[maximthomas]

The diagnosis is right and the direction is right — a replay failure must not advance the ServerState. But in the
exact scenario this targets, a backend outage failing several in-flight changes, the new recovery path restarts the
session without bound and re-applies changes that were already applied. Two blockers, three majors below. Nothing
was executed; all of it is traced from source.

The MAX_REPLAY_ATTEMPTS bound is unreachable (blocker)

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

The attempt count lives in two scalar fields, not a per-CSN map:

lastFailedCSNAttempts = csn.equals(lastFailedCSN) ? lastFailedCSNAttempts + 1 : 1;
lastFailedCSN = csn;

A failure for a different CSN resets the count to 1, so attempts > MAX_REPLAY_ATTEMPTS (:2644) is only reached
when one CSN fails four times with nothing else failing in between. A backend outage fails every change in flight.
With two failing changes:

recover(c1)  lastFailedCSN=null -> attempts=1 -> disableService/clear/enableService
recover(c2)  c2 != c1           -> attempts=1 -> restart
   RS resumes from a ServerState covering neither, resends both
recover(c1)  c1 != c2           -> attempts=1 -> restart ...

The counter never leaves 1 and there is no sleep or backoff anywhere in :2623-2671. The escape hatch the PR
describes — "a change which can never be applied here does not stop the replica for good" — is dead code, and the
replica restarts its replication session indefinitely.

This holds single-threaded too; two CSNs alternating through one thread reset each other identically.
synchronized (replayFailureLock) at :2644 does not help: it makes the read-modify-write atomic, but one slot is
still one slot.

UpdateOperationTest.failedReplayIsNotRecordedAsReplayed misses it because it publishes exactly one DeleteMsg, so
the counter walks 1,2,3,4 and the skip fires as intended. A test with two concurrent failing CSNs would catch it.

Suggested: key the count on the CSN (a bounded map or small LRU), dropping entries on successful replay and on skip.

RemotePendingChanges.clear() re-arms OPENDJ-1115 (blocker)

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/RemotePendingChanges.java:212

clear() empties pendingChanges unconditionally, but commit() flushes only the contiguous committed prefix —
it breaks at the first uncommitted entry. This fix deliberately leaves the failing change uncommitted, so every
later change that another replay thread applies successfully stays in the map as committed-but-unflushed, with its
CSN absent from the ServerState. clear() drops those too.

c1 fails, stays uncommitted
c2, c3 applied OK but sit behind c1  -> ServerState covers none of the three
clear()                              -> all three forgotten
enableService() -> RS resends c2, c3 -> putRemoteUpdate() now returns true (map is empty)
                                     -> c2, c3 replayed and acked a second time

That putRemoteUpdate() check at LDAPReplicationDomain.java:4425 is the OPENDJ-1115 guard, and its own comment
says surviving pendingChanges is exactly what makes session failover safe. Correctness now rests entirely on
conflict resolution being idempotent for a replayed Add/Modify/Delete — the reliance OPENDJ-1115 was filed to remove.
The pre-existing session restarts (disable() at :3304, the fractional reconnect at :702/:729) do not call
clear().

Only the uncommitted changes need forgetting. Keeping the committed ones is still correct: once the failing CSN
commits, the prefix flushes and the state advances over them. A selective removal gets the stated benefit without
reopening OPENDJ-1115.

clear() runs against a replay queue nobody drained (major)

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/RemotePendingChanges.java:234

recoverFromReplayFailure does disableService(); clear(); enableService();. disableService() stops the broker
and joins the listener thread only — it never stops the replay threads, and updateToReplayQueue is a static
10 000-entry queue shared by every domain. Messages for this domain are still in it, pollable, while the map is
empty. Then:

// markInProgress, no null guard; activeAndDependentChanges is a ConcurrentSkipListSet
activeAndDependentChanges.add(pendingChanges.get(msg.getCSN()));

Two outcomes, depending on whether the redelivered copy has re-run putRemoteUpdate() yet:

• not yet — get() returns null, add(null) throws NPE into ReplayThread's catch-all. replay() never runs:
  no replay, no processUpdateDone(), no assured ack, so the originating master waits out its SAFE_READ
  timeout instead of getting the honest error ack this PR adds. One ERR_EXCEPTION_REPLAYING per queued change.
• already — the stale copy and the redelivered copy are both replayed: same CSN, two threads, one map entry. The
  loser hits commit() -> NoSuchElementException -> ERR_OPERATION_NOT_FOUND_IN_PENDING and returns early, so it
  is applied but never recorded.

switchQueueLock does not serialise this — it is released before domain.replay(), and clear() is reached from
inside replay(). A null guard alone would turn the first case into a silent drop; the queue needs draining for
this domain before clear().

The shutdown guard reads the wrong flag (major)

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

private boolean recoverFromReplayFailure(CSN csn, AtomicBoolean shutdown) {
  if (shutdown.get() || disabled) return true;

That shutdown is the parameter threaded down from replay(LDAPUpdateMsg, AtomicBoolean) — the ReplayThread's
flag — and it shadows the domain field at :352. disabled is set only inside disable(), and shutdown() (:2242)
calls disableService() directly rather than disable(), so it stays false. Neither guard sees domain shutdown.

MultimasterReplication.finalizeSynchronizationProvider() runs domain.shutdown() for every domain at :572 and
stopReplayThreads() only at :576, so the thread flag is false throughout — and that is precisely when replays fail,
because the backend is going offline and returns a code isServerFailure() now routes here. Both guards pass,
enableService() runs, ReplicationBroker.start() clears its own shutdown flag and reconnects: a broker and a fresh
listener thread come back up on a domain whose alert generator, flush thread and RSUpdater are already gone.

Before this PR no replay failure ever called enableService(). Checking the domain's own state (and renaming the
parameter — the shadowing is what hid this) is enough.

isServerFailure() keys off ResultCode.OTHER (major)

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

return result == ResultCode.UNAVAILABLE
    || result == getServerContext().getCoreConfigManager().getServerErrorResultCode();

server-error-result-code defaults to ResultCode.OTHER — not a storage-specific code, but what the server returns
whenever nothing more specific applies. ReferentialIntegrityPlugin.java:1155 does
catch (Exception de) { stopProcessing(ResultCode.OTHER, ...) }, reached from doPreOperation(add):1054 and
doPreOperation(modify):985, neither guarded by isSynchronizationOperation(); SaltedSHA1PasswordStorageScheme
and SaltedSHA512PasswordStorageScheme do the same at :447/:451.

So a deterministic referential-integrity failure now costs, per delivery, ten rounds of Thread.sleep(50) at :2375
plus a full session restart, three times over — where before it fell into solveNamingConflict, was logged, and was
stepped over in one pass. It is also a realistic input for the first blocker: one misconfiguration fails many
changes at once.

Separately, the branch sits ahead of conflict resolution (:2375 vs :2386) and the knob is unvalidated —
GlobalConfiguration.xml:181 constrains it only with <adm:integer lower-limit="0"/> and CoreConfigManager does a
bare ResultCode.valueOf(). Setting it to 32, 66, 67 or 68 makes those codes bypass solveNamingConflict entirely.

Detecting the storage failure at its source — a marker exception type, or the exception cause — would be sturdier
than matching a configurable result code. Failing that, move the branch after conflict resolution.

Nits

• replayed-updates-failed counts attempts, not changes: the three numFailedReplayedUpdates.incrementAndGet()
  sites (:2441, :2467, :2512) are all inside replay(), re-entered on every redelivery, while replayed-updates-ok
  is incremented once per committed change (:2018). One unreplayable change contributes up to 4 — and unboundedly
  once the first blocker fires.
• Off-by-one in the skip: attempts > MAX_REPLAY_ATTEMPTS with attempts starting at 1 skips on the fourth
  delivery, and ERR_REPLAY_SKIPPING_CHANGE_308 ("after %d attempts") prints 4 while the constant and the PR
  description both say 3.
• The attempt counter is never reset: lastFailedCSN / lastFailedCSNAttempts are assigned only at :2639-2640
  — not on a successful replay, not after a skip. A CSN skipped once is skipped again with zero retries if
  redelivered; a change that fails, succeeds, then fails much later starts at attempt 2.
• The new alert type is undocumented: org.opends.server.replication.UnresolvedConflict is listed in
  opendj-doc-generated-ref/src/main/asciidoc/admin-guide/chap-monitoring.adoc:2179 and the docbkx copy at
  chap-monitoring.xml:980; UnreplayedChange appears in neither. enabled-alert-type is an allow-list — "if
  there are any values for this attribute, then only alerts with one of the specified types are allowed" — so an
  operator with a non-empty list gets no notification of a diverged replica and no documented string to add.
• A self-healing retry is logged at ERROR: ERR_REPLAY_RETRYING_CHANGE_307 fires via logger.error on every
  attempt of a path the code expects to recover from; the neighbouring retry messages are
  WARN_RETRYING_BIND_CHANGELOG_301 / NOTE_BOUND_CHANGELOG_AFTER_RETRY_302. Worth a WARN_ prefix — and worth
  mentioning in the description, which announces only ERR_REPLAY_SKIPPING_CHANGE.
• The advertised retry-in-place success is untested: both new tests keep the short circuit registered for the
  whole run, so all ten in-loop attempts fail and only the give-up path is observed — nothing shows that a transient
  failure clearing within the window is replayed exactly once and committed. Both inject ResultCode.OTHER, so the
  UNAVAILABLE half of isServerFailure() is unexercised, and the alert assertion is
  assertThat(DummyAlertHandler.getAlertCount()).isGreaterThan(initialAlerts) — any alert emitted during the three
  session restarts satisfies it, including one of a different type.

[vharseko]

Thanks - the review was traced from the source and it holds. Both blockers and all three majors are fixed in 8d3a0cc, together with every nit. Two of the fixes turned out to need more than what was suggested, and one of the majors is answered rather than applied; details below.

The MAX_REPLAY_ATTEMPTS bound (blocker) - fixed as suggested

The two scalar fields are gone. The count is kept per CSN in a ConcurrentSkipListMap<CSN, Integer>, dropped as soon as the change is replayed (synchronize()) or given up on, and bounded at 1000 entries with the oldest evicted - only failing changes are listed, so the bound is never reached in practice. The session is also left down for 1s * attempts before the change is asked for again, so a backend which keeps failing is not asked for every change as fast as the RS can send them.

UpdateOperationTest.everyChangeWhichCanNotBeReplayedIsGivenUpOn is the regression test you asked for: two CSNs failing at once, which is the case the single slot could never carry to the give up - each of them resets the count of the other.

clear() re-arming OPENDJ-1115 (blocker) - fixed, plus two things it uncovered

clearUncommitted() now removes only the changes which were not replayed, exactly as you suggested. Writing the unit test for it turned up two more holes on that path:

• putRemoteUpdate() overwrote the entry it found. pendingChanges.put(csn, new PendingChange(...)) == null reports the duplicate but has already replaced the committed-but-unflushed change with a fresh uncommitted one, so nothing would ever commit it and the ServerState would stay behind it for good. It is putIfAbsent() now.
• A duplicate delivery had the listener push its CSN to the ServerState. processUpdate() returned true for a duplicate, and ReplicationDomain's listener does processUpdateDone(msg, null); state.update(msg.getCSN()) for everything which returns true (ReplicationDomain.java:3246-3256). With the committed changes now kept, the resent copy of c2 would push c2 into the ServerState over c1, which is still failing - the very bug this PR is about. It returns false now and calls processUpdateDone(msg, null) itself, so the ack and the window credit stay per delivery while the recording of the change stays with the copy which is listed.

clear() against an undrained replay queue (major) - fixed, by a different means

I first did what you suggested - MultimasterReplication.dropQueuedUpdates(domain) holding switchQueueLock. It works, but it is unusably slow: instrumenting the recovery showed disable=1-3ms, enable=64-329ms, drain+clear=19527ms / 61786ms / 83782ms. switchQueueLock is non-fair and ten replay threads re-take it in a tryLock(1s) / poll(1s) loop, so the draining thread starves for tens of seconds. That is what made everyChangeWhichCanNotBeReplayedIsGivenUpOn time out at 120s.

The queue is not drained at all now. markInProgress() returns whether this message is the delivery which is listed as pending, and ReplayThread skips the message when it is not:

final PendingChange change = pendingChanges.get(msg.getCSN());
if (change == null || change.getLDAPUpdateMsg() != msg) { return false; }

change == null is the first outcome you described - the NPE - and the identity check is the second one: the stale copy and the redelivered copy are never both replayed, because only the one the domain is listing gets through. No lock, and a stale message costs one poll().

The shutdown guard (major) - fixed, with one correction

The parameter is replayThreadShutdown now and the guard reads the state of the domain (shutdown.get() || disabled), re-read after the wait and before enableService(). One correction to the reasoning: at server shutdown the synchronization providers are finalized before the backends (DirectoryServer.java:4170-4172), so "the backend is going offline" is not the usual trigger - the window is real for any replay failing in flight, and for disable()/delete() from a configuration change.

isListenerShuttingDown() looked like a good extra guard and is not: during a recovery started by another replay thread the listener is already gone, so a change failing at that moment took the "domain is going away" exit and never counted its attempt. The count now happens before the guard.

isServerFailure() keying off ResultCode.OTHER (major) - partly

The premise is right: server-error-result-code defaults to 80 = OTHER, CoreConfigManager does a bare valueOf() on an unvalidated integer, and the code is used all over the server for "internal error".

The two examples do not reach a replay, though. Pre-operation plugins are not invoked for synchronization operations - LocalBackendModifyOperation.java:323-333, LocalBackendAddOperation.java:436-439, LocalBackendDeleteOperation.java:262-265 - and ReferentialIntegrityPlugin registers only pre-operation, post-operation and subordinate types (isConfigurationAcceptable() at :255-280), no pre-parse, so its stopProcessing(ResultCode.OTHER, ...) cannot fire for a replayed change. SaltedSHA* :447/:451 is "the JVM has no SHA-1 MessageDigest", not a per-change failure. So the "ten Thread.sleep(50) plus three session restarts for a deterministic referential-integrity failure" does not happen.

What is real is the misconfiguration you point at second, and that is now closed: the codes conflict resolution owns (NO_SUCH_OBJECT, ENTRY_ALREADY_EXISTS, NOT_ALLOWED_ON_RDN, NOT_ALLOWED_ON_NONLEAF) are excluded from isServerFailure(), so setting the knob to 32/66/67/68 can no longer take a change away from solveNamingConflict().

Detecting the storage failure at its source is the better design and I did not do it: Operation carries neither the exception nor a marker, so it means plumbing a flag from BackendImpl.createDirectoryException() through setResponseData() and the Operation interface (and OperationWrapper, and every implementor) - a change to a core API well outside this fix. Worth its own issue if you want it.

Nits

All applied.

• replayed-updates-failed counts changes, not attempts: the increment moved into skipUnreplayableChange(), and it only counts when the change was really recorded as skipped (updateError() reports whether it committed), so a change skipped while another thread was clearing the pending list is counted when it is skipped for good rather than twice.
• Off-by-one: attempts >= MAX_REPLAY_ATTEMPTS, so the change is skipped on the 3rd attempt and the message prints 3.
• The attempt count is dropped on a successful replay and on a skip.
• org.opends.server.replication.UnreplayedChange is documented in chap-monitoring.adoc and the docbkx chap-monitoring.xml, next to UnresolvedConflict.
• ERR_REPLAY_RETRYING_CHANGE_307 is WARN_REPLAY_RETRYING_CHANGE_307 and is logged with logger.warn.
• Tests: transientReplayFailureIsRetriedAndTheChangeApplied covers the UNAVAILABLE half and the retry in place - the backend serves the operation again after three attempts and the change is applied exactly once, with nothing counted as failed and no alert; failedReplayIsNotRecordedAsReplayed now counts the redeliveries through the short circuit rather than through the monitor attribute, asserts replayed-updates-failed is +1 exactly, and asserts the alert count of that type.

One thing not in the review: the alert. Whatever makes one change unreplayable makes every change in flight unreplayable, and the alert was raised per skipped change. It is now raised at most once a minute per domain, while every skipped change is still logged.

Tests

UpdateOperationTest 11/11, AssuredReplicationPluginTest 14/14, RemotePendingChangesTest 6/6, NamingConflictTest 6/6, ModifyConflictTest 36/36, DependencyTest 3/3, IsolationTest 1/1, StateMachineTest 5/5 - 82/82 green.

[maximthomas]

Round 1's five findings are genuinely addressed: the per-CSN map is real, the shutdown guard now
re-reads a volatile field after the wait, and everyChangeWhichCanNotBeReplayedIsGivenUpOn does
fail against e62617b. The CONFLICT_RESULT_CODES exclusion closes the misconfiguration case as
described. Two new blockers below, four majors, eight minors. Nothing executed; all traced from source.

---

issue (blocking): clearUncommitted() drops changes another replay thread is executing right now

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/RemotePendingChanges.java:225

final PendingChange change = it.next();
if (!change.isCommitted())
{
  dependentChanges.remove(change);
  activeAndDependentChanges.remove(change);
  it.remove();
}

A change is uncommitted for the whole duration of its backend operation. disableService() stops the
broker and joins listenerThread only — the replay threads are MultimasterReplication's shared pool
and are never signalled or joined. markInProgress() guards the queue take, which an in-flight replay
has already passed. The default pool is >= 16 threads and an outage fails everything at once, so this
is the normal case, not a race.

That operation then succeeds:

• synchronize():2058 → remotePendingChanges.commit(curCSN) → NoSuchElementException
• :2064 logs ERR_OPERATION_NOT_FOUND_IN_PENDING and returns
• nothing else advances the state — the listener's state.update is dead now that processUpdate()
  always returns false
• the RS resends, putRemoteUpdate() finds no entry, the change is replayed a second time

The double replay itself is absorbed (solveNamingConflict for Add/Delete/ModDN,
AttrHistoricalMultiple.processAddConflict for Modify). What is not absorbed: the change is also
dropped from activeAndDependentChanges, so its dependency ordering is lost. A child Add resent while
the parent Add is still in op.run() sees no dependency, gets NO_SUCH_OBJECT, and
solveNamingConflict:3200-3216 renames it to a conflict RDN. That divergence is permanent.

This is round 1's blocker moved rather than removed — committed entries are protected, in-progress ones
are not.

Suggested: skip changes present in activeAndDependentChanges (markInProgress() already puts them
there) and let their own replay commit normally; a resend then hits putIfAbsent → duplicate →
processUpdate false, which is the intended path.

---

issue (blocking): an orphan in dependentChanges makes getNextUpdate() throw for the life of the domain

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/RemotePendingChanges.java:313

checkDependencies() reads under the read lock, releases it, then registers under a different lock:

private PendingChange getPendingChange(CSN csn) {
  pendingChangesReadLock.lock();
  try { return pendingChanges.get(csn); }
  finally { pendingChangesReadLock.unlock(); }   // <-- released here
}

private void addDependency(PendingChange dependentChange) {
  dependentChangesLock.lock();                    // <-- pendingChanges not held
  try { dependentChanges.add(dependentChange); }
  finally { dependentChangesLock.unlock(); }
}

clearUncommitted() holds both locks but cannot see a change not yet inserted. It removes the change
from pendingChanges; addDependency() then puts it into dependentChanges → an entry absent from
pendingChanges. Then, at :293:

if (!dependentChanges.isEmpty())
{
  PendingChange firstDependentChange = dependentChanges.first();
  if (pendingChanges.firstKey().isNewerThanOrEqualTo(firstDependentChange.getCSN()))

pendingChanges is typically empty right after clearUncommitted() (only committed-unflushed entries
survive), so firstKey() throws NoSuchElementException. It is thrown at
LDAPReplicationDomain.java:2576, outside the try, so it escapes replay() into ReplayThread's
catch (Exception) → ERR_EXCEPTION_REPLAYING_REPLICATION_MESSAGE. Nothing removes the orphan, so it
recurs on every later replay() that ends with an empty map. If the map is non-empty and
firstKey >= orphan, getNextUpdate() instead returns the stale message and replay() runs it
without markInProgress() — bypassing the new identity check.

Not pre-existing: the invariant dependentChanges ⊆ pendingChanges held before this PR (commit()
removes only committed entries, and a change is uncommitted while its own thread is in
checkDependencies). That invariant is exactly what made the unguarded firstKey() safe.
clearUncommitted() is the first remover that can race an uncommitted entry.

Suggested: hold pendingChangesReadLock across getPendingChange + addDependency, or re-check
membership inside addDependency. Guard firstKey() with isEmpty() as well — but not instead, or
a permanent break becomes a silent stale replay.

---

issue (non-blocking): the total retry budget is ~4.5 s, for an outage the javadoc measures in minutes

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

Per change: 10 in-place iterations × sleep(50) ≈ 500 ms, then attempt 1 → wait 1 s → restart →
≈ 500 ms, attempt 2 → wait 2 s → restart → ≈ 500 ms, attempt 3 → skip. ≈ 4.5 s, plus the 64–329 ms per
enableService() you measured. skipUnreplayableChange() then advances the ServerState over a change
that was never applied.

isServerFailure()'s own javadoc names the case it exists for:

the backend being offline or rebuilt (OPENDJ-49), or the storage failing to serve the operation

An index rebuild, an import, a restore or a storage failover lasts minutes. So for the exact scenario
this PR targets, every change in the outage window is permanently skipped and the replica diverges —
with the once-a-minute-per-domain throttle collapsing a mass divergence into one alert.

Not a regression against master (which committed after ~500 ms, silently). But "a change which can never
be applied here does not stop the replica for good" currently means never = 4.5 seconds.

Suggested: a time-based budget (give up after N minutes of continuous failure), or make
MAX_REPLAY_ATTEMPTS / REPLAY_RETRY_DELAY_IN_MS configurable, or exempt UNAVAILABLE from give-up
and bound only per-change failures. Whichever — restate it in the description.

---

issue (non-blocking): neither headline test exercises the new machinery

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

assertTrue(ShortCircuitPlugin.getShortCircuitCount(OperationType.DELETE, "PreParse") >= 2,
           "the change was not sent again after its replay failed");

That is the test's proof of redelivery, and the in-place retry loop already satisfies it. :2426 does
Thread.sleep(50); continue; without reassigning nextOp, so the same operation object is re-run;
DeleteOperationBasis.run():189 resets the result to UNDEFINED and :203 invokes
invokePreParseDeletePlugins unconditionally. Delivery #1 alone drives the count to 10 in ~500 ms —
before recoverFromReplayFailure() has run once. Delete clearUncommitted(), putIfAbsent() and the
whole redelivery path and this assertion still passes.

.../UpdateOperationTest.java:1606 — transientReplayFailureIsRetriedAndTheChangeApplied registers
maxTimes=3 against retryCount=10. Attempts 1–3 short-circuit and sleep/continue; attempt 4 passes
through and succeeds. Four of ten slots used, so control never leaves the while loop:
countReplayFailure(), failedReplayAttempts, MAX_REPLAY_ATTEMPTS, waitBeforeSessionRestart(),
clearUncommitted() and enableService() are all unreached. That UNAVAILABLE → sleep/continue branch
already exists on master (:2306, :2357-2364), so the test passes against unmodified code.

It also does not assert exactly-once — only assertNull(getEntry(...)). A second application returns
NO_SUCH_OBJECT → solveNamingConflict → NOTHING_TO_DO → updateError, incrementing nothing and
invisible to every assertion.

Net: fail → session restart → applied has no test, and neither blocker above sits in code these tests
touch. 82/82 green is worth less than it reads.

Suggested: set maxTimes above the 10 in-place slots (12) so success requires a real restart; count
deliveries (distinct operation identity, or a restart counter), not plugin invocations.

---

issue (non-blocking): committed-but-unflushed changes are stranded if the domain is disabled mid-recovery

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

waitBeforeSessionRestart(attempts);
if (shutdown.get() || disabled)
{
  // The domain went away while this thread was waiting.
  return true;
}
enableService();

commit() is now the only thing that advances the ServerState for a replayed change — state.update
is nowhere else in RemotePendingChanges, and the listener's state.update cannot fire because
processUpdate() returns false for every LDAPUpdateMsg. clearUncommitted() removes the blocking
head and deliberately keeps the committed entries, but does not flush.

If disabled was set by processImportBegin / processRestoreBegin → disable(), enableService()
is never reached and nothing clears remotePendingChanges on disable/enable. Those committed entries
survive the cycle, so every later resend hits putIfAbsent → false → processUpdate false → no
state.update. The ServerState never covers them for the life of the domain object,
remote-pending-changes-size stays non-zero, and the RS re-ships them on every reconnect.

Suggested: clear remotePendingChanges in disable() — it already does state.save(); state.clearInMemory(), so the pending map is the one piece of that state left behind.

Not the fix: flushing the prefix after clearUncommitted(). The ServerState is a per-serverId
watermark, so flushing c2/c3 after dropping the failing c1 advances past c1 and reintroduces #889.

---

question (non-blocking): should a retried change still send an error ack on every attempt?

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

} finally
{
  if (!dependency)
  {
    processUpdateDone(msg, replayErrorMsg);
  }
}

if (replayFailed && recoverFromReplayFailure(csn, replayThreadShutdown))
{
  // The ack has been published and the change, still owned by the replication
  // server, is being delivered again: there is nothing left to replay here.

The comment says this is deliberate, so this is a question rather than a defect claim. In SAFE_READ the
originating client's result derives from those acks. Before this PR a failed replay was never retried,
so an error ack was final and truthful. Now a change that fails once and succeeds on attempt 2 reports
a replay failure at this replica to the client that made it — a new false negative — and one CSN emits
up to three AckMsg across three sessions.

Was the false negative part of the trade? If not: publish processUpdateDone once the change is
resolved (replayed / skipped / given up), or at least don't set replayErrorMsg on an attempt that will
be retried. Either touches AssuredReplicationPluginTest.testSafeReadModeReplyWithReplayError, whose
5000 ms timeout now covers a path containing a full disable / 1 s sleep / enable cycle.

---

nitpick (non-blocking): the 1000-entry bound evicts the entry it exists to protect

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

while (failedReplayAttempts.size() > MAX_FAILED_REPLAY_ATTEMPTS_TRACKED)
{
  // Every change in flight is failing: keep the counts of the most recent ones,
  // the oldest ones have been given up on long ago.
  failedReplayAttempts.pollFirstEntry();
}

pollFirstEntry() on a ConcurrentSkipListMap<CSN, Integer> removes the lowest CSN — the oldest,
i.e. the head-of-line change the RS resends first and the one closest to MAX_REPLAY_ATTEMPTS. The
comment's premise ("given up on long ago") is what the eviction prevents. Past 1000 tracked CSNs the
blocking change never reaches 3 and the domain restarts indefinitely — round 1's blocker re-armed.

I could not close whether >1000 CSNs are ever tracked at once (broker window vs. the shared 10 000-entry
replay queue), so this is a nitpick rather than an issue. Evicting by insertion order removes the doubt.

---

nitpick (non-blocking): a stale count outlives the failure it counted

.../LDAPReplicationDomain.java:2700 — failedReplayAttempts.remove() exists at exactly two sites:
:2057 (result == SUCCESS) and :2647 (skipUnreplayableChange). recoverFromReplayFailure returns
early at the shutdown || disabled guard and at the lost-CAS path after countReplayFailure() has
incremented, and the NO_OPERATION path never removes. Counts survive a disable/enable cycle, so a CSN
that burned 2 attempts before an import is skipped on its first failure afterwards — against a backend
that is healthy again.

---

nitpick (non-blocking): a dropped delivery skips processUpdateDone() entirely

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/ReplayThread.java:102

if (!domain.markInProgress(updateMsg))
{
  continue;
}

No ack is owed — the delivery necessarily arrived on a session already torn down, and rcvWindow is
reset on reconnect. But incProcessedUpdates() is skipped, so replication-processed-updates
undercounts, and ReplicationBroker.updateDoneCount is not reset at reconnect the way rcvWindow is,
so a residual carries into the new session.

---

nitpick (non-blocking): both give-up tests race the counter they assert

.../LDAPReplicationDomain.java:2645

failedReplayAttempts.remove(csn);
if (updateError(csn))
{
  numFailedReplayedUpdates.incrementAndGet();

updateError(csn) is what makes cover(csn) true, and both tests poll on exactly that before asserting
replayed-updates-failed. In the two-CSN test one thread's commit() head-walk pushes both CSNs
while only one increment has landed → assertEquals(..., initialFailures + 2) fails. Increment before
updateError.

---

nitpick (non-blocking): ShortCircuitPlugin state can silently defang the transient test

opendj-server-legacy/src/test/java/org/opends/server/plugins/ShortCircuitPlugin.java:739

deregisterShortCircuit() clears shortCircuits and shortCircuitLimits but not shortCircuitCounts;
the 3-arg registerShortCircuit() clears neither. More importantly the registration is keyed only on
operationType + section and is consulted for every delete in the JVM, so unrelated deletes can eat
the maxTimes=3 budget of transientReplayFailureIsRetriedAndTheChangeApplied — eat all three and the
replayed delete succeeds on its first attempt with every assertion still green. The other five callers
use the request-control path and are unaffected.

---

note (non-blocking): two behaviour changes not in the description

• .../LDAPReplicationDomain.java:2473 — ConflictResolution.FAILED now sets replayErrorMsg and
  calls skipUnreplayableChange(), so OBJECTCLASS_VIOLATION, CONSTRAINT_VIOLATION,
  INSUFFICIENT_ACCESS_RIGHTS, ADMIN_LIMIT_EXCEEDED raise the UnreplayedChange alert and fail the
  SAFE_READ ack after one try, with no retry budget. Master only did
  logger.error(ERR_ERROR_REPLAYING_OPERATION, ...). Probably right — worth stating.
• .../LDAPReplicationDomain.java:2407 — NO_OPERATION now does replayDone = true; updateError(csn);. postOperation commits only on SUCCESS, so previously such a change stayed
  uncommitted forever and blocked the prefix push. A real fix for a state stall, in neither commit
  subject.

---

chore (non-blocking): four small ones

• !replayFailed in the while at :2366 is dead — the field is assigned only after the loop and in
  the catch.
• waitBeforeSessionRestart() (:2802) catches InterruptedException, re-sets the flag, and returns as
  if it had waited; enableService() then runs on an interrupted thread and the backoff is skipped.
• resetUnreplayedChangeAlertThrottle() (:2678) is public on a production class for a test in the
  same package — package-private reaches it.
• RemotePendingChangesTest extends ReplicationTestCase: a full server start/stop for six in-memory
  unit tests. Nothing covers the eviction bound.

[vharseko]

Thanks - the review holds where it counts, and one of the two blockers turned out to be the
one worth redesigning around rather than patching. Everything is in 0f6ba70. Two findings
are answered rather than applied and two are, I believe, mistaken; details below.

clearUncommitted() dropping changes another thread is applying (blocker) - fixed, but not as suggested

The suggestion - skip the changes listed in activeAndDependentChanges - does not survive
its own scenario: nothing removes a change from that set when its replay fails
(commit() at :185 and clearUncommitted() at :238 are the only removals), so the
change which opened the recovery would have been kept, its redelivery turned down by
putIfAbsent and nothing would ever replay it. Same for every thread which loses the
replayFailureRecovery CAS: the comment there relies on clearUncommitted() having
forgotten their changes.

There is also a second reason the "keep the in-progress ones" shape needs care: the
ServerState is a watermark. Keeping a newer in-flight change while dropping the older
failed one means the newer one's commit() walks the head of an empty-in-front map and
records a CSN which covers the failed change - #889 again, by another route.

So clearUncommitted() is gone and ownership became explicit instead. PendingChange has
an owned mark; replayFailed(csn) drops the mark and leaves everything else alone, so:

• the change stays listed and uncommitted - it is the barrier which holds the ServerState
  back, which is what makes the replication server send it again;
• it stays in activeAndDependentChanges, so the changes which follow it still depend on
  it. This is the divergence you described - a child Add resent while its parent is not in
  the data - and it is now impossible by construction rather than by timing;
• putRemoteUpdate() takes over an unowned change: the new delivery replaces the message,
  and the stale copy in the shared replay queue is dropped by markInProgress()'s identity
  check. A change a thread still owns is refused exactly as OPENDJ-1115 wants.

Nothing is applied twice and no ERR_OPERATION_NOT_FOUND_IN_PENDING is logged on this path
anymore. replayFailedKeepsTheChangeAsABarrier in RemotePendingChangesTest is the
regression test: a change is failed while a newer one is mid-replay, the newer one commits,
and neither reaches the ServerState until the failed one is delivered again and applied.

Writing it turned up a hole the suggestion would not have closed either: a change released
after a restart began gets its redelivery refused, and if the thread which released it
lost the CAS, nobody asks again. sessionRestartRequested now carries the request across
the restart, and the recovery loop re-checks it after giving up the CAS.

The dependentChanges orphan (blocker) - fixed as suggested

addDependency() holds pendingChangesReadLock and only lists a change which
pendingChanges still has; getNextUpdate() guards firstKey() with isEmpty() as well,
not instead. With clearUncommitted() gone the invariant is back to what made the
unguarded firstKey() safe in the first place, and both guards keep it that way.

The retry budget (issue) - fixed

It is a duration now, not a count: REPLAY_GIVE_UP_DELAY_IN_MS is five minutes, which is
the order of magnitude the isServerFailure() javadoc talks about. The backoff counts the
session restarts of the domain rather than the failures of whichever change opened the
recovery - with an outage failing everything in flight, a change being sent for the first
time would otherwise keep the wait at one second forever - and it is capped at ten seconds
because it runs on a replay thread every domain shares. The count resets as soon as
anything is replayed.

The tests (issue) - fixed, with one correction

You are right about the >= 2 assertion: DeleteOperationBasis.run() re-invokes the
pre-parse plugins, so the ten in-place attempts satisfy it on the first delivery. It is
> IN_PLACE_REPLAY_ATTEMPTS now, referencing the production constant so the two cannot
drift apart.

transientReplayFailureIsRetriedAndTheChangeApplied was worse than you described in one
respect and I have made it prove the opposite: it fails IN_PLACE_REPLAY_ATTEMPTS + 2
times, so the change can only be applied after a real session restart, and it asserts
replayed-updates-ok grew by exactly one - the exactly-once claim you noted was missing.

One correction: failedReplayIsNotRecordedAsReplayed as a whole was not vacuous. Its
give-up assertion is only reachable through three deliveries, so removing clearUncommitted()
or putIfAbsent() made it fail on the 120 s timeout rather than pass. Only that one
assertion was redundant.

Stranded committed-but-unflushed changes (issue) - fixed as suggested

disable() forgets the pending changes, after disableService() rather than before it so
that a delivery in flight is not re-listed on the way down. And you were right that
flushing the prefix is not the fix: the watermark would jump the failed change.

disable(), enable() and shutdown() now serialise with the recovery on one lock, and
the recovery only starts a session back if the session it stopped is still the one which is
down - so a configuration change or the end of an import cannot be undone by a replay
thread which was sleeping through it.

The ack on every attempt (question) - answered, not changed

Deliberate, and the code now says why. The ack belongs to the delivery and states what that
delivery did: the change is not in the data at that point, whether or not the delivery
which follows manages to apply it. Holding it back would not be more truthful - the session
it came over is torn down a moment later, so nothing would reach the server waiting for it
and an assured write would wait out its timeout instead of being told what happened. The
false negative is real; a silent timeout looked worse.

Nits

• Eviction: the failures live in a small ReplayFailures class now, ordered on the last
  failure, so what gets evicted past the bound is the change which stopped failing the
  longest ago - never the one which is being retried. ReplayFailuresTest covers it.
• Stale counts: dropped on success, on give-up, when the recovery bails out and when the
  domain is disabled. A change which stops failing is forgotten outright, so there is no
  wall-clock staleness heuristic left to get wrong.
• Dropped delivery: counted as processed. No ack is owed - a delivery is only dropped
  when the session it came over is gone.
• Counter race in the tests: the exact value is waited for rather than read once
  (assertMonitorAttrValueEventually), which also survives the monitor entry being
  deregistered while the session is down.
• ShortCircuitPlugin: both registerShortCircuit() overloads and
  deregisterShortCircuit() now clear the counts and the limit of the key they touch.

Two findings I think are mistaken

• NO_OPERATION "previously stayed uncommitted forever": master committed it too. The
  branch sets replayDone = true and falls into if (replayDone) { updateError(csn); } at
  0b9c0f63f5:2399-2404. Moving the call into the branch was a refactor, not a fix - which
  is why it is in no commit subject.
• resetUnreplayedChangeAlertThrottle() could be package-private: UpdateOperationTest
  is in org.opends.server.replication and the domain is in
  org.opends.server.replication.plugin, so package-private does not compile. It stays
  public with @VisibleForTesting.

ConflictResolution.FAILED raising the alert and failing the ack after one try is indeed a
behaviour change; it is in the description now, along with the budget and the ack.

Also fixed while in there, both found by re-reading the paths this touches rather than by
the review: BUSY no longer ends in a silent skip when the ten in-place yields are spent
under lock contention (it is as transient as a storage failure, and the change is just as
absent from the data), solveNamingConflict(ModifyDNOperation) keeps the two result codes
it solves - UNWILLING_TO_PERFORM and OBJECTCLASS_VIOLATION were missing from the
exclusion set - and a replay thread which is stopped, when the number of them is changed,
hands the change it was replaying back to the replication server instead of leaving it
listed as owned by a thread which is gone.

Two things I did not do here and filed instead: #900, DSRSShutdownSync.stopInstanceTimestamp
is latched by the first ReplicaOfflineMsg ever sent, which any import already trips; and
#901, the retry policy belongs in ReplicationDomainCfg rather than in constants with a
test-only setter.

Ran green: UpdateOperationTest, RemotePendingChangesTest, ReplayFailuresTest,
AssuredReplicationPluginTest, GenerationIdTest, InitOnLineTest, MonitorTest,
StateMachineTest (the one which changes the replay thread count) and
ChangelogBackendTestCase - 87 tests.

[maximthomas]

praise: The redesign is the right call, and the counter-argument to the round-2 suggestion holds: nothing removes a change from activeAndDependentChanges when its replay fails, so "keep the listed ones" would have stranded the change that opened the recovery — its redelivery refused, nobody left to replay it. The watermark point is right too; keeping a newer in-flight change while dropping the older failed one is #889 by another route. Explicit ownership beats inferring it from map membership. sessionRestartRequested closes a hole the review never raised, and disable() clearing after disableService() is correct as argued. Four candidates from this round died on tracing because the code was already right in non-obvious ways: the dropped delivery owes no ack and leaks no window credit (rcvWindow is reset at every handshake); replayFailed() cannot see a committed CSN, because every commit path sets replayDone and breaks; a stopping thread's zero-delay restart cannot coexist with a requester owed a backoff; and abandonReplay() cannot deadlock stopReplayThreads(), since nothing on that path takes the class monitor.

---

issue (blocking): ReplayFailures eviction restarts the give-up budget, so a large outage never gives up.

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/ReplayFailures.java:96-129

Runs leave the map only via forget()/clear() — success, give-up, shutdown/disabled — so every surviving entry is a change that is still failing. The eldest is not "stopped failing longest ago", it is the still-failing change that has waited longest for its next turn. Evict it and its next failure allocates a new Run with firstFailureTimeMs = nowMs, resetting the clock.

new LinkedHashMap<CSN, Run>(16, 0.75f, true) { removeEldestEntry -> size() > maxTracked }

With more than MAX_FAILED_REPLAY_ATTEMPTS_TRACKED (1000) distinct changes failing, every change is evicted between two of its own failures, so failure.getFailingForMs() >= replayGiveUpDelayInMs (LDAPReplicationDomain.java:2857) is never true for any of them: no give-up, no ERR_REPLAY_SKIPPING_CHANGE, no alert, a session restart every ≤10 s forever. Reachable — the shared replay queue is LinkedBlockingQueue<>(10000) (MultimasterReplication.java:109) and window-size defaults to 100000, ten times the bound.

Suggested: carry firstFailureTimeMs on the PendingChange that is already the barrier, and leave only the attempt count in ReplayFailures. If the side map stays, eviction must drop the run's detail and keep its deadline. The class javadoc at :33-38 also states an invariant that cannot hold — a change that stopped failing has already been forget()-ten.

---

issue (blocking): The give-up releases the change before it decides to give up, so it can commit a change another thread is applying.

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

remotePendingChanges.replayFailed(csn);

final ReplayFailures.Failure failure = replayFailures.recordFailure(csn, monotonicNowInMs());
if (failure.getFailingForMs() >= replayGiveUpDelayInMs)
{
  final LocalizableMessage message =
      ERR_REPLAY_SKIPPING_CHANGE.get(csn, getBaseDN(), failure.getAttempts());
  logger.error(message);
  skipUnreplayableChange(csn, message);
  return false;
}

Between the release and the commit the change is listed, uncommitted and unowned — exactly what putRemoteUpdate() accepts as a takeover (RemotePendingChanges.java:175-180), and nothing re-checks isOwned(). T1 un-owns A, then sits in the synchronized recordFailure and a logger.error — both slow in the scenario that produced a 5-minute run. Another thread restarts the session, the RS resends A (the ServerState is still held back by A), a third thread applies it. T1 resumes and commits A: the watermark advances over a change being applied right then. If that thread's replay then fails, replayFailed(A) is a no-op because A is committed — A is in the ServerState, absent from the data, no alert.

Suggested: record the failure and test the budget before replayFailed(csn) — skip while still owned, or release to be retried, never both.

---

issue (blocking): abandonReplay acks the delivery as successfully replayed.

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

if (replayThreadShutdown.get() || shutdown.get())   // master: shutdown.get() only
{
  abandonReplay(msg.getCSN());
  return;                                            // -> finally: processUpdateDone(msg, replayErrorMsg)
}

dependency is the enclosing while-condition and replayErrorMsg is still null, so ReplicationDomain.java:3465 publishes a plain new AckMsg(csn). An assured SAFE_READ writer is told the change is in the data at the moment this replica declares it absent and asks for it again. Master reached this only on server shutdown; the replayThreadShutdown disjunct makes it fire whenever num-update-replay-threads is reconfigured on a live domain. This is the opposite polarity to the false negative the description defends.

Suggested: set replayErrorMsg before the return, or suppress the ack as markInProgress()'s drop path at :2394 already does.

---

issue (blocking): readFractionalConfig's restart is a fourth disableService()/enableService() pair and is not under serviceStateLock.

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:792,819

if (needRestart) { disableService(); }
...                                     // :794-814 mutate the fractional configuration
if (needRestart) { enableService(); }

The lock is taken at :2365, :2954, :2973, :3668, :3715 — not here. A replay thread waking in restartSession() takes the uncontended lock, finds isListenerShuttingDown() true (the config thread already nulled listenerThread), so its guard does not fire and it runs enableService(). The session is up mid-reconfiguration: setFractional() has run, the exclusive flag and the attribute sets at :801-813 have not. The inner synchronized (sessionLock) does not help — it makes each call atomic, not the pair.

Also a description fix: "disable(), enable() and shutdown() now serialise with the recovery on one lock" is true of those three and not of this pair.

---

issue (non-blocking): restartSession() guards only its enable side.

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:2954-2962 vs :2975

if (shutdown.get() || disabled) { return; }                          // teardown: no identity check
disableService();
...
if (shutdown.get() || disabled || !isListenerShuttingDown()) { return; }   // enable: has one

disable() never clears sessionRestartRequested, so after an import the loop at :2914 reads a leftover request and the teardown block kills the session enable() just restored — for a redelivery that cannot come, clear() having forgotten every pending change. Bounded by MAX_REPLAY_RETRY_DELAY_IN_MS and self-healing, but it falsifies "the recovery only starts a session back if the session it stopped is still the one which is down": nothing records which session it stopped.

Suggested: clear sessionRestartRequested in disable(); properly, a session generation counter captured before the wait and checked on both sides.

---

issue (non-blocking): The "applied exactly once" assertion cannot detect a double apply.

opendj-server-legacy/src/test/java/org/opends/server/replication/UpdateOperationTest.java:1662-1665,1693

assertMonitorAttrValueEventually("replayed-updates-ok", initialReplayed + 1,
    "a change which was delivered again must be applied exactly once");
assertMonitorAttrValueEventually("replayed-updates-failed", initialFailures, ...);

The helper is TestTimer.repeatUntilSuccess, which returns on the first passing poll and never re-checks — a change applied twice passes as long as the counter transits through +1, which is the OPENDJ-1115 regression the takeover exists to prevent. The second call expects the value already read into initialFailures, so it succeeds at t=0: a snapshot, not a wait.

Suggested: assert stability after a settle, or assert on a value only reachable once.

---

issue (non-blocking): RemotePendingChangesTest lost its TestNG groups.

opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/RemotePendingChangesTest.java:35

public class RemotePendingChangesTest extends DirectoryServerTestCase   // was ReplicationTestCase

ReplicationTestCase carries @Test(groups = { "precommit", "replication" }); the new parent does not, and the methods use a bare @Test. Failsafe's filename include still runs the class, so any group-filtered invocation silently stops running the #889 regression test.

---

todo (non-blocking): Three behaviour changes announced in the description have no test: the BUSY reclassification (LDAPReplicationDomain.java:2593), the solveNamingConflict(ModifyDNOperation) exclusion-set additions, and abandonReplay()'s hand-back (:2935). StateMachineTest changes the replay-thread count but asserts nothing about the hand-back.

---

issue (non-blocking): disable() can drop a change a replay thread is mid-apply.

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

The replay path takes serviceStateLock nowhere and disableService() joins only the listener, so a thread can be between markInProgress() and commit() with the operation already in the backend when state.save() persists a watermark excluding it and clear() wipes it. Pre-existing at 0b9c0f63f5 (which had no clear() there) — the PR turns a silent double replay into a logged ERR_OPERATION_NOT_FOUND_IN_PENDING. Worth a follow-up issue rather than a fix here.

---

issue (non-blocking): Two monitor attributes changed meaning silently.

• changes-in-progress-size — replayFailed() never removes from activeAndDependentChanges (only commit() and clear() do), so a failed change counts as "actively being replayed" until redelivery or give-up. RemotePendingChanges.java:250-262, published at LDAPReplicationDomain.java:4875.
• replication-processed-updates — the dropped-delivery path calls incProcessedUpdates() (LDAPReplicationDomain.java:2404), so it now counts deliveries taken off the session, not updates handed to the replay path. Not in the doc diff.

---

question (non-blocking): Widening CONFLICT_RESULT_CODES also changes isServerFailure(), its only reader (LDAPReplicationDomain.java:407-412,2726-2733). With ds-cfg-server-error-result-code = 53 or 65, does a genuine backend failure now route to solveNamingConflict(), return FAILED for non-ModifyDN ops and land in skipUnreplayableChange() — recorded as replayed after one attempt? Deliberate?

---

nitpick (non-blocking): ERR_REPLAY_SKIPPING_CHANGE_308 says "after %d attempts" but the trigger is a duration; the count printed is not the threshold that fired. opendj-server-legacy/src/messages/org/opends/messages/replication.properties:616

nitpick (non-blocking): putRemoteUpdate()'s takeover tests !isCommitted() && !isOwned() while its javadoc and LDAPReplicationDomain.java:4822 both call it "a change whose replay failed" — it also matches a change between putRemoteUpdate() and markInProgress(). Benign today; the comment overstates the guard. opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/RemotePendingChanges.java:175-180

nitpick (non-blocking): disable() and the recovery bail-out clear replayFailures but not sessionRestartRequested or consecutiveSessionRestarts, contra "stale counts … dropped when the recovery bails out and when the domain is disabled".

[vharseko]

Thanks - every blocker holds, and two of them turned out to have the same fix. Everything is in 5a0297e. One finding is answered rather than applied; details below.

ReplayFailures eviction restarting the give-up budget (blocker) - fixed, by removing the map

You are right, and the shape was wrong rather than the bound: runs only leave the map through forget()/clear(), so every entry in it is a change which is still failing, and the eldest is just the one whose turn to fail has not come round yet. Evict it and its next failure starts a new Run.

So the side map is gone. The failures live on the PendingChange which is already the barrier - recordReplayFailure(nowMs) / getReplayFailingForMs(nowMs) - and RemotePendingChanges.recordReplayFailure() hands out a snapshot of them. That takes the eviction with it:

• the budget is kept for exactly as long as the change is not in the data, whichever delivery is replaying it - the takeover replaces the message and leaves the failures alone;
• it goes away with the change, when it commits or when a disabled domain forgets it, so there is nothing to expire and no staleness heuristic left;
• it can not grow without end either: it is bounded by the pending changes, which the window and the ServerState barrier already bound.

ReplayFailures and ReplayFailuresTest are deleted; the scenarios that test carried are now in RemotePendingChangesTest, against the real bookkeeping - failures counted per change, kept across a takeover, dropped with the change, and aFailingChangeKeepsItsBudgetHoweverManyOtherChangesAreFailing, which fails 1500 other changes in between and is the regression test for exactly what you found.

This also closes the CodeQL alert on :103: the anonymous LinkedHashMap whose size() shadowed the enclosing one no longer exists.

The give-up releasing the change before deciding (blocker) - fixed as suggested

recordReplayFailure() and the budget test now run while this thread still owns the change, and only the retry path calls replayFailed(). A change which is given up on is committed by the thread which owned it all along, so there is no window for putRemoteUpdate() to hand it to anyone.

abandonReplay acking the delivery as replayed (blocker) - fixed, and the first attempt was wrong

replayErrorMsg is set and the ack carries the error: NOTE_REPLAY_ABANDONED_CHANGE. You are right that master only reached this on server shutdown and that replayThreadShutdown widened it to a live num-update-replay-threads change; the polarity is the one to fix, not to keep.

Setting the message and calling abandonReplay() where the old return was is not enough, though, and I had to redo it: the hand-back stops and starts the session, and the ack is published by the finally, so the error ack was being sent on a session which had just been torn down and replaced - the replayFailed/recoverFromReplayFailure pair sits below the finally for exactly that reason. The abandon path now sets a flag and breaks out of the loop, and abandonReplay() runs after the ack has been published on the session the delivery came over. The log line moved into abandonReplay() past its shutdown check as well: a server which is shutting down abandons every change in flight and asks for none of them again, so one INFO line per change would have said otherwise.

readFractionalConfig()'s restart outside serviceStateLock (blocker) - fixed as suggested

The stop, the mutation of the fractional configuration and the start are one block under serviceStateLock now. Your trace is right, including why the guard did not fire: disableService() nulls listenerThread, so isListenerShuttingDown() reads true and the replay thread went on to enableService() mid-reconfiguration.

restartSession() guarding only its enable side (issue) - fixed, with the session counter

disable() clears sessionRestartRequested and consecutiveSessionRestarts, and so does the give up - the ServerState moved past the change, so the change which fails next must not inherit the backoff this replica had reached. The identity check is real now: sessionGeneration is bumped every time this domain stops or starts its session under serviceStateLock - disable(), enable(), the shutdown path, the configuration paths and restartSession() itself - and the recovery captures it before the wait and only starts a session back if it is still the one it stopped.

isListenerShuttingDown() is kept next to it rather than replaced by it, because readFractionalConfig() is not the only pair: changeConfig() (through restartService()) and readAssuredConfig() restart the session in the same applyConfigurationChange(), and neither can bump a counter it does not know about. All three now run under serviceStateLock, which also closes the window where a session came up between readAssuredConfig()'s disableService() and its assuredConfig = config; the listener check catches whatever else brings a session up outside the lock.

The "applied exactly once" assertion (issue) - fixed as suggested

assertMonitorAttrValueStays() reads the value five times over a second, so a counter which transits through +1 on its way to +2 fails it. The replayed-updates-failed assertion uses it too, which is what makes it a wait rather than the snapshot you spotted.

RemotePendingChangesTest groups (issue) - fixed

@Test(groups = { "precommit", "replication" }, sequential = true) on the class.

The three untested behaviour changes (todo) - one and a half of them

• BUSY: transientReplayFailureIsRetriedAndTheChangeApplied is a data provider over UNAVAILABLE and BUSY now, so the reclassification is covered by the same assertions as the storage failure.
• The CONFLICT_RESULT_CODES additions: covered indirectly by the new test below, which sets server-error-result-code to 53. What is not covered is a ModifyDN conflict which is still solved with that setting in place - the scenario needs a naming conflict which fails with 53/65 on a ModifyDN specifically, and I did not build one.
• abandonReplay()'s hand-back: not covered, and I do not think it can be without new test machinery, which is also why the ack ordering above was found by re-reading rather than by a test. The trigger needs a replay thread to be inside the retry loop at the moment the thread count changes; a short circuit which keeps failing spends most of its time in the backoff instead, so the test would pass whether or not the hand-back happened - the change is released by the recovery either way. The mechanics it relies on - a released change taken over by the next delivery - are covered by replayFailedLetsTheNextDeliveryTakeOverTheChange.

Widening CONFLICT_RESULT_CODES also changing isServerFailure() (question) - fixed rather than answered

You are right that it traded one hole for another: with ds-cfg-server-error-result-code = 53 or 65, a storage failure carrying that code was left to conflict resolution, came back FAILED and was recorded as replayed after one attempt. Conflict resolution still gets its chance first, but when it returns FAILED on the configured server error code the change is now retried as the server failure it is, rather than skipped - and retried in place the same IN_PLACE_REPLAY_ATTEMPTS times an UNAVAILABLE gets, so a storage busy for a moment does not cost a session restart, with the error logged once below the loop rather than once per attempt. isServerFailure() is expressed in terms of the new predicate, so the configured code is read once and the rule lives in one place.

changeConflictResolutionCanNotSolveOnTheServerErrorCodeIsRetried is the test: with the code set to 53, a change which keeps failing for longer than the in-place attempts is delivered again and applied, with nothing counted as failed and no alert.

recordReplayFailure() returning null (not raised, found while re-reading the path) - it is not only the disabled domain

It also returns null for a change which is already committed, which is reachable without any disable - the give up commits the change, and anything throwing after that lands in the catch-all with replayFailed set. The recovery carries on with the changes which were waiting rather than returning as if the domain were going away, so the dependent changes are not left sitting until the next change of the domain happens to be replayed, and the javadoc names both reasons.

The two monitor attributes (issue) - documented

Neither is in the admin guide - only the alert types are - so the meaning is stated where it is read: changesInProgressSize() says a change whose replay failed counts until it is applied or given up on, and the drop path says replication-processed-updates counts the deliveries taken off the session.

disable() dropping a change mid-apply (issue) - agreed, follow-up

Pre-existing and worth its own issue rather than a fix here, as you say.

Nitpicks

• ERR_REPLAY_SKIPPING_CHANGE prints the duration which fired the give-up as well as the attempts.
• putRemoteUpdate()'s javadoc now says the guard also matches a change between putRemoteUpdate() and markInProgress(), and why that is harmless.
• disable() clears the recovery state, as above.

Tests

UpdateOperationTest 13/13 (including both data provider rows, the new test, and namingConflicts / infiniteReplayLoop / modifyConflicts), RemotePendingChangesTest 12/12, AssuredReplicationPluginTest 14/14, StateMachineTest 5/5, NamingConflictTest 6/6, ModifyConflictTest 36/36, DependencyTest 3/3, GenerationIdTest 4/4 and MonitorTest 1/1 - 94 tests, all on 5a0297e.

[maximthomas]

praise: The rework is the right shape, and three things stand out.

Deleting ReplayFailures instead of raising its bound. The eviction was a symptom — every entry in
that map was a change still failing, so the eldest was just the one whose turn had not come round.
Moving the budget onto PendingChange takes the eviction, the staleness heuristic and the growth
question with it in one move.

Re-doing the abandon fix after the first attempt. Setting replayErrorMsg where the old return
was looked right; catching that the hand-back stops and starts the session, so the error ack was
being published on a session that had just been torn down, needed re-reading the path rather than
trusting the diff. The flag-and-break shape is correct.

aFailingChangeKeepsItsBudgetHoweverManyOtherChangesAreFailing fails 1500 changes against an old
bound of 1000, and changeConflictResolutionCanNotSolveOnTheServerErrorCodeIsRetried genuinely
fails on the previous head. Both are real regression tests, not restatements of the new code.
Self-reporting the recordReplayFailure()-returns-null-for-a-committed-change case, found while
re-reading rather than by a test, is the same instinct.

---

issue (blocking): Giving up on a change resets the session-restart backoff for every other
change still failing.

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

consecutiveSessionRestarts is domain-wide and is the only input to waitBeforeSessionRestart() —
min(1000ms * restarts, 10000ms), so a 1s floor climbing to a 10s cap after ten restarts.

private void skipUnreplayableChange(CSN csn, LocalizableMessage cause) {
  if (updateError(csn)) {
    numFailedReplayedUpdates.incrementAndGet();
    sendUnreplayedChangeAlert(cause);
    ...
    consecutiveSessionRestarts.set(0);   // <-- here
  }
}

Backend goes UNAVAILABLE. The backoff climbs correctly for 300s. Then the head change's budget
expires, skipUnreplayableChange runs, and the counter goes to 0 — while the whole backlog is still
failing. Those changes restart the session from the 1s floor again. Give-ups then recur at the
write-arrival rate, and one per 55s is enough that the cap is never reached: a sustained outage
restarts the session every ~1-2s instead of every 10s, each restart rewinding the RS cursor to the
frozen ServerState and re-streaming the backlog. The field's own javadoc at 364-370 names this
failure mode as the reason it exists.

It is also reached from 2598 (FAILED) and 2656 (LOOP), so one thread skipping an unrelated
change resets a backoff another thread is climbing.

Giving up is not progress — the backend is still down. Delete the line; 2119 (postOperation)
already resets on a change that actually applied.

---

issue (blocking): serverFailedOnAConflictResultCode is set once and decides the fate of all
ten attempts.

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

The flag is declared once per delivered message (2436), set inside the retry loop, never cleared,
and tested after it:

// 2583, inside case FAILED:
serverFailedOnAConflictResultCode = true;
Thread.sleep(50);
break;

// 2632, after the loop
if (isServerFailure(lastResult) || ResultCode.BUSY.equals(lastResult)
    || serverFailedOnAConflictResultCode)

With ds-cfg-server-error-result-code = UNWILLING_TO_PERFORM — a CONFLICT_RESULT_CODES member,
which is this branch's whole premise. Attempt 1 returns it, solveNamingConflict(Modify) falls to
the terminal FAILED at 3310, flag set. Attempt 2 finds the entry renamed concurrently:
NO_SUCH_OBJECT, findEntryDN hits, msg.setDN, REPLAY_AGAIN. Attempts 3-10 ping-pong
REPLAY_AGAIN until retryCount is spent. lastResult is NO_SUCH_OBJECT — neither a server
failure nor BUSY — but the stale flag still takes the server-failure road: withheld from the
ServerState, session restarted, re-delivered every round for the full 300s before being skipped
anyway. The intended road at 2651-2656 never runs, so unresolved-naming-conflicts stays flat for
what is exactly an unresolved naming conflict, and ERR_ERROR_REPLAYING_OPERATION at 2640 names
NO_SUCH_OBJECT rather than the code that set the flag.

Drop the flag and test the last attempt's own result, so the branch and the log agree:

if (isServerFailure(lastResult) || ResultCode.BUSY.equals(lastResult)
    || isConfiguredServerErrorResultCode(lastResult))

---

issue (blocking): The "exactly once" fix landed at one of three sites; the two give-up tests
still cannot catch the regression their own messages name.

opendj-server-legacy/src/test/java/org/opends/server/replication/UpdateOperationTest.java:1486
and :1576

assertMonitorAttrValueEventually is a TestTimer.repeatUntilSuccess — the first poll that matches
wins:

// 1486
assertMonitorAttrValueEventually("replayed-updates-failed", initialFailures + 1,
    "a change which could not be replayed must be counted once, not once per attempt");
// 1576
assertMonitorAttrValueEventually("replayed-updates-failed", initialFailures + 2,
    "both changes must be counted as failed, once each");

A counter ending at +2 passes through +1, and whichever 200ms poll lands on the transit value
satisfies the assertion. The likeliest break — a retry loop incrementing per attempt — is precisely a
counter climbing past the expected value, so these two tests are blind to it. The correct pattern is
already three lines below at 1687-1689:

assertMonitorAttrValueEventually("replayed-updates-failed", initialFailures + 1, "...");
assertMonitorAttrValueStays     ("replayed-updates-failed", initialFailures + 1, "...");

Worth sweeping the construct rather than these two lines — it survived at two of the three places it
appears.

---

suggestion (non-blocking): The flagship conflict test does not pin conflict-resolution-first.

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

changeConflictResolutionCanNotSolveOnTheServerErrorCodeIsRetried asserts only that the change is
retried and applied. Putting UNWILLING_TO_PERFORM back into isServerFailure() — never calling
conflict resolution at all — gives identical observables. Assert that solveNamingConflict was
reached (unresolved-naming-conflicts, or a conflict side effect).

---

suggestion (non-blocking): The isCommitted() half of the new null guard is untested.

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/RemotePendingChanges.java:332

if (change == null || change.isCommitted())

In replayFailuresGoAwayWithTheChange the committed CSN is the first generated, so commit()
flushes it off the head of the map and the null comes from change == null. Deleting
|| change.isCommitted() keeps every test green. The case named in your comment — the give-up
commits a change sitting behind an older uncommitted one — has no test. Commit the second change
while the first is still uncommitted.

---

issue (non-blocking): sessionGeneration's documented invariant is false.

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

Javadoc says "Bumped every time the session of this domain is stopped or started". It is bumped at
798/826, 2375, 3056/3084, 3774, 3839 — but not around changeConfig() or
readAssuredConfig(), both of which stop and start the session
(ReplicationDomain.java:3378, :3838-3843). Not live today, since both sides now hold
serviceStateLock. But dropping the isListenerShuttingDown() half of the 3071 guard on the
strength of that javadoc would reintroduce the double-start this commit fixes. Either bump it in
those two paths or narrow the javadoc.

---

question (non-blocking): Was the peer-visible cost of abandoning on a live thread-count change
intended?

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

The polarity fix is right. But the condition is replayThreadShutdown.get() || shutdown.get(), and
MultimasterReplication:767-771 does stopReplayThreads(); createReplayThreads(); on a
ds-cfg-num-update-replay-threads change. So a benign tuning action now makes every in-flight
assured SAFE_READ change ack with setHasReplayError(true): the origin logs
NOTE_DS_RECEIVED_ACK_ERROR and bumps assuredSrReplayErrorUpdates, for a change this replica
applies moments later. A permanent false negative on the origin.

Either let a thread finish the change it owns before exiting, or accept it and say so in the
javadoc.

---

issue (non-blocking): abandonReplay() is silent when the domain is disabled.

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

if (shutdown.get() || disabled) { return; }
// ...comment justifying the shutdown case only
logger.info(NOTE_REPLAY_ABANDONED_CHANGE, csn, getBaseDN());

A change abandoned during an import/restore is handed back with no local log line, while the ack
still carries the error. The peer records a replay error the local log never explains. Log it, or
extend the comment.

---

suggestion (non-blocking): The two give-up messages misreport attempts and duration.

opendj-server-legacy/src/messages/org/opends/messages/replication.properties:613-621,
opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:2938-2939

getAttempts() counts delivery rounds; each round already burned up to
IN_PLACE_REPLAY_ATTEMPTS = 10 in-place tries. Both WARN_REPLAY_RETRYING_CHANGE_307 and
ERR_REPLAY_SKIPPING_CHANGE_308 print it as (attempt %d), understating by up to 10x. And:

ERR_REPLAY_SKIPPING_CHANGE.get(csn, getBaseDN(), failure.getFailingForMs() / 1000, ...)

truncates, so a sub-second give-up delay — which is how the tests set it — prints "failing for 0
seconds". Say "delivery", and print sub-second durations in ms.

---

note (non-blocking): The TestNG-groups fix changes nothing in this build, but keep it.

opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/RemotePendingChangesTest.java:35

There is no <groups>/<excludedGroups> in any surefire/failsafe config, no testng*.xml, and no
-Dgroups in any workflow — selection is by **/*Test.java filename only, so the class was already
running and a normal build never noticed its absence from the groups. The annotation still earns its
place: the class extends DirectoryServerTestCase, not ReplicationTestCase, so without it any
group-filtered invocation — -Dgroups=replication, an IDE group run, downstream CI that filters —
silently skips the regression test for the issue this PR exists to fix. sequential = true is inert
under <parallel>none</parallel>, and as a class-level @Test attribute it would not be inherited by
the methods' own @Test annotations if parallelism were ever enabled.

---

todo (non-blocking): The test story overstates coverage.

git grep -l "sessionGeneration\|ABANDONED_CHANGE\|readFractionalConfig\|abandonReplay" over
opendj-server-legacy/src/test returns nothing. Round-1 fixes #4 (readFractionalConfig under
serviceStateLock) and #5 (the sessionGeneration guard) ship untested; the comment acknowledges
only #3. #5 is a generation counter compared across a wait — exactly the shape a unit test pins.

---

nitpick (non-blocking): Six small ones.

• opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/RemotePendingChanges.java:112-122 —
  changes-in-progress-size now counts a failed change for the whole give-up window with nothing
  actively replaying it. The new meaning is in javadoc only; the doc delta adds the alert type but
  not this. Dashboards only — no flow control reads it.
• opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/PendingChange.java:44-56 —
  committed, owned, replayFailures, firstReplayFailureTimeMs carry no @GuardedBy. All five
  write sites do hold pendingChangesWriteLock, so no live race; the annotation is what stops the
  next caller adding one.
• opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/RemotePendingChangesTest.java:362 —
  the magic 1500 no longer relates to any surviving constant (MAX_FAILED_REPLAY_ATTEMPTS_TRACKED
  = 1000 went with ReplayFailures). Valid today; a future bound above 1500 reintroduces the bug
  green. Worth a comment saying why 1500.
• opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/RemotePendingChangesTest.java:328 —
  the re-homed forget test records one failure where the deleted aChangeWhichIsForgottenStartsOver
  recorded two and asserted the count restarted at 1. A clear() resetting to 1 instead of 0 now
  passes.
• opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:2662-2680 —
  the catch (Exception e) does not restore the interrupt flag, unlike 2388 and 3109 in the
  same file.
• opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/PendingChange.java:186-189 —
  getReplayFailingForMs() cannot distinguish "never failed" from "failed just now"; both return 0.
  Dead guard on the only call path, latent for a future caller.

[vharseko]

Thanks - all three blockers hold, and the first one is a straight reversal of what I argued last round. Fixed in 3c0f24f, together with two of the smaller findings. Six are acknowledged and left; details and reasons below, so that what is not in the commit is not in it by decision rather than by omission.

Giving up resetting the session backoff (blocker) - fixed, and I had it backwards

You are right, and this is the finding I should not have needed twice: last round I defended that reset with "the ServerState moved past the change, so the change which fails next must not inherit the backoff". That reads well and is wrong. Giving up is not progress - the backend which made this change unreplayable is still failing every change in flight with it - and the field's own javadoc names the failure mode: a change delivered for the first time keeping the wait at its shortest for as long as the outage lasts. Your arithmetic is the whole argument, and I had written it myself: the ramp is 1+2+...+10 = 55s, so a give-up more often than once a minute means the ten second cap is never reached, and give-ups arrive at the write rate once the first budget expires.

The line is gone. What resets the backoff now is a change which was really applied - postOperation and recordChangeResolved - which is the only thing that says the backend is serving again. The reasoning is in the skipUnreplayableChange() javadoc rather than only in this thread, so the next reader does not re-derive the reset.

Your second point holds too: the reset was reached from the FAILED and LOOP skips, which never increment that counter, so one thread skipping a schema violation was resetting a backoff another thread was climbing through an outage.

serverFailedOnAConflictResultCode deciding for all ten attempts (blocker) - fixed as suggested

The flag is gone and the branch reads lastResult, so the road taken and the code named in ERR_ERROR_REPLAYING_OPERATION are the same one. The ping-pong you traced is the reachable case, and the general shape is worse than the case: any attempt setting the flag decided for the nine after it.

One consequence to flag rather than bury, because it is not a strict no-op. The flag was only ever set inside case FAILED; isConfiguredServerErrorResultCode(lastResult) also fires when the last attempt came back on that code and conflict resolution answered REPLAY_AGAIN. That case used to take the LOOP road (skip, unresolved-naming-conflicts), and now takes the server-failure road (withhold, ask again). I think that is right - the change is not in the data and the last thing the server said about it is the code it puts on an internal error - but it is a behaviour change on your suggestion, not just a de-staling of it. Say the word if you would rather have it narrowed to FAILED.

assertMonitorAttrValueEventually blind to a counter climbing past (blocker) - fixed, and swept

Both sites now assert Stays as well. Swept the construct as you asked: all three assertMonitorAttrValueEventually calls in the class are followed by a assertMonitorAttrValueStays on the same attribute (:1486/:1493, :1583/:1589, :1692/:1700), so the pattern is uniform rather than right in the place it was last fixed. getMonitorAttrValue() retries the monitor read for ten seconds on its own, so the added Stays does not turn a deregistered monitor entry into a flake.

sessionGeneration's javadoc (issue) - fixed by narrowing the javadoc

You are right that the invariant is false, and the reason is the one I gave last round in the same breath as the wrong claim: changeConfig() and readAssuredConfig() cannot bump a counter they do not know about. Rather than bump it in LDAPReplicationDomain around calls whose restart lives in ReplicationDomain, the javadoc now says what the counter actually counts - sessions started under serviceStateLock by this class - and names isListenerShuttingDown() as the half of the guard which catches the rest. That is the sentence which stops the next reader dropping it.

The two give-up messages (suggestion) - fixed

Both now say "delivery", both note that each delivery is attempted several times in place, and the give-up prints the duration in milliseconds, so a sub-second give-up no longer reads as "0 seconds". ReplayFailure.getAttempts()'s javadoc says deliveries too, since that is what was misleading the call sites.

Left, with reasons

• The flagship conflict test not pinning conflict-resolution-first - accepted, not written. The assertion has to observe that solveNamingConflict() was reached, and for a Delete on the configured code the conflict path leaves no side effect the test can read: unresolved-naming-conflicts is only bumped on the LOOP road, which this test deliberately does not take. It needs either a counter on the conflict path or a ModifyDN variant, which is No test for a ModifyDN conflict solved while server-error-result-code is one of the conflict codes #910. Queued there rather than guessed at here.
• The isCommitted() half of the null guard - accepted, not written. Committing the second change while the first is uncommitted is the case, and it is three lines; it belongs with the RemotePendingChangesTest pass that No test for a ModifyDN conflict solved while server-error-result-code is one of the conflict codes #910 will touch.
• Abandoning on a live thread-count change (question) - not intended as a cost, no. The ack is right for the delivery, but a ds-cfg-num-update-replay-threads change is not a reason to tell the origin its assured write failed, and the check sits at the top of the loop, so a thread that has not even attempted the change abandons it. I lean to your first option - let a thread finish the change it owns and only bail between changes - but that is a change to the shutdown path, and parking a replay thread inside op.run() to test it is exactly the blocking plugin No test for the change a stopped replay thread hands back to the replication server #909 is filed for. Not guessing at it in a round which is otherwise three deletions.
• abandonReplay() silent when disabled - accepted, not written; it belongs with the above, since both are about that path.
• The test story overstating coverage - the description is corrected: it no longer implies sessionGeneration, readFractionalConfig and the hand-back are covered, and it lists them with the rest of what is left. ReplayFailuresTest was also still named there after being deleted; that is gone too.
• The nitpicks - all six read true. The @GuardedBy annotations on PendingChange, the interrupt flag in the catch (Exception e), the comment on the 1500, and the getReplayFailingForMs() guard are one-liners I would rather land in one pass with the tests above than scatter across two rounds. changes-in-progress-size's new meaning and the clear()-resets-to-1 gap in the re-homed forget test are the two worth doing properly rather than quickly.

Note on the TestNG groups

Agreed on all counts, including that a normal build never noticed. Keeping it.

Tests

UpdateOperationTest (13), RemotePendingChangesTest (12) and AssuredReplicationPluginTest (14): 39/39 green on 3c0f24f. AssuredReplicationPluginTest matters here because the second fix changes which road a change takes with a configured server-error-result-code, and that road ends in the ack.

[vharseko]

A second commit, 27e29b9. Nothing here comes from your review - these are from a pass I ran over the whole diff after the last push, so they are mine to have missed rather than yours to have raised. Two of them are real bugs, one of which is older than this PR and which this PR was about to make permanent.

A change which can not be decoded wedged the domain for good

logDecodingOperationError() set replayErrorMsg and nothing else: replayFailed stayed false, so recoverFromReplayFailure() never ran, commit() never ran, and the change stayed listed, uncommitted and owned. It is the barrier, so this domain's ServerState - and every change behind it, from every master - stopped there for as long as the server was up.

The catch is identical in master, so the wedge is not new. What is new is that master had a way out by accident: putRemoteUpdate() overwrote the listed copy unconditionally, so a redelivery caused by some other failing change let the listener push the CSN through. The ownership check this PR adds is what closes that door, which makes this PR's job to open a real one. There is no operation to retry and no delivery which would decode any better, so the change is now skipped where it is reported - counted, alerted on, and out of the way of everything behind it.

The session restart backoff was reset by every successful replay

Last round you had me delete the reset in skipUnreplayableChange(), correctly. The reset in synchronize() at what was :2119 is the one we both left standing, and it has the same hole from the other side: a change which can never be applied here fails alone, among changes which replay perfectly well, and each of those was setting consecutiveSessionRestarts back to 0. So the backoff only ever worked during a total outage. With one poison change and a domain taking ordinary writes, the wait stayed at its one second floor and the domain tore its session down and rebuilt it roughly once a second for the whole 300s give-up window - the hammering the field exists to prevent, just reached by a different route than the one you found.

RemotePendingChanges now counts the changes which have a replay failure recorded against them, and the backoff is reset only when that count is zero. An outage still resets it on the first change which gets through; a poison change now lets it climb to the ten second cap. This also softens - it does not close - the ResultCode.OTHER concern from your round 1: a deterministic plugin failure still withholds the ServerState for the give-up window, but it costs a restart every ten seconds rather than every second. Telling a storage failure from a change which can never be applied here still wants the marker exception you asked for, and that is listed with the rest of what is left.

Smaller, same commit

• The in-place retry loop reads disabled now. A domain disabled for an import cleared its pending changes and saved its ServerState, but the replay threads already inside replay() were told nothing: they kept applying changes into the backend the import owns, and every commit failed on the empty map with ERR_OPERATION_NOT_FOUND_IN_PENDING. This is the #908 shape from your round 3, and while the full fix still needs the blocking plugin, the loop guard is worth having on its own.
• Each attempt runs an operation of its own. Re-running the one which already ran stacked a ManageDsaIT control - and an access log record - per attempt, up to ten. Pre-existing on the UNAVAILABLE path; this PR widened the set of codes that reach it, so it is this PR's to fix. The conflict resolution branch was already creating a fresh operation for the same reason, and now does not need to.
• The markInProgress() drop path javadoc claimed "the delivery which took over from it carries the ack", which is true of the takeover and not of the clear()-on-disable case, where no delivery takes over. Your round 3 trace that no ack is owed still holds for both - the session is gone either way - but for two different reasons, and the comment now says which.
• serviceStateLock says what it costs. enableService() connects under it, so a shutdown, an import or a configuration change which arrives mid-restart waits for that connect. That is the lock you asked for in round 3 and I am not narrowing it; each of those callers stops the session as its first act, so what they wait for is a session about to be stopped again.
• chap-monitoring.xml was modified without its 3A Systems copyright year being extended, while its asciidoc sibling in the same commit was.

Tests, and how much they are worth

Three new ones, and each was checked against a mutation of the code it covers rather than taken on trust - twice now you have found tests here which pass against the bug they name, so it seemed the least I could do:

test	mutation it fails against
aChangeWhichCanNotBeDecodedIsNotLeftHoldingTheServerStateBack	the give-up removed from logDecodingOperationError() - the CSN never reaches the ServerState and the test times out
failingChangesAreCountedOnceAndForgottenWithTheChange	the first-failure guard dropped, so one change failing twice is counted twice
aChangeKeepsFailingWhileTheChangesAroundItAreReplayed	the count cleared on any commit - which is exactly the wrong belief the synchronize() reset was built on

The first builds a ModifyMsg which travels the protocol intact and cannot be turned into an operation: the encoded modifications are carried as an opaque byte array and only read by createOperation(), so the message is decoded, listed and handed to a replay thread before it fails. It asserts the operation cannot be built before publishing, so it cannot quietly stop testing anything.

Worth saying plainly: the third one passed against the first two mutations. I only caught that by working out what each mutation actually does to it, which is why it has a mutation of its own rather than sharing theirs.

Two test fixes came out of the same pass. testSafeReadModeReplyWithReplayError registered an unlimited failure without shortening the give-up delay, so the domain restarted its session against the FakeReplicationServer while the assertions and the teardown ran; it gives up at once now. And changeConflictResolutionCanNotSolveOnTheServerErrorCodeIsRetried restored a hard-coded 80 in its finally rather than the code it found.

UpdateOperationTest, RemotePendingChangesTest and AssuredReplicationPluginTest: 42/42 green on 27e29b9.

[maximthomas]

praise: The two bugs in 27e29b9 are both real, and the reasoning behind the first one is the
good part. Spotting that master only escaped the decode wedge by accident — putRemoteUpdate()
overwriting the listed copy unconditionally, so an unrelated failing change was what pushed the CSN
through — and that this PR's ownership check is what closes that door, is the kind of thing a diff
review does not surface. The backoff hole found from the other side (one poison change among healthy
replays, each of which was resetting consecutiveSessionRestarts) is a genuine second route to the
same hammering. Fresh operation per attempt kills the stacked ManageDsaIT control and the ten
access-log records. And reporting that the third test passed against the first two mutations, rather
than quietly giving it a mutation of its own, is worth more than the test.

---

issue (blocking): logDecodingOperationError() has two callers, and the give-up is only correct
for one of them.

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

// logDecodingOperationError(), :2795-2802 — new in 27e29b9
skipUnreplayableChange(msg.getCSN(), message);

Caller two is the generic catch, reached whenever csn is still null — and csn is assigned at
:2544, 66 lines into the try:

// :2736-2737
catch (Exception e) {
  if (csn != null) { ... replayFailed = true; }
  else { replayErrorMsg = logDecodingOperationError(msg, e); }   // any unchecked throw before :2544
}

Reachable thrower, :2538 — ModifyOperationBasis.getEntryDN() returns null after an unparseable
DN (it catches LocalizedIllegalArgumentException, sets INVALID_DN_SYNTAX, returns null):

if (modifyOperation.getEntryDN().equals(SET_PERMISSIVE_MODIFY_FOR_DN))   // NPE

That NPE now runs skipUnreplayableChange → updateError → commit() → state.update(csn). The
ServerState covers a change that never reached the backend, replayFailed stays false so
recoverFromReplayFailure (:2765) never runs — no retry, no redelivery, no give-up budget — and
the ack still says ERR_EXCEPTION_DECODING_OPERATION. The origin server is told it failed; this
server's state says it was applied. At the previous commit this method only logged.

Move the skip into the DecodeException | LDAPException | DataFormatException catch at :2719, the
only caller for which the new javadoc's "no delivery would decode any better" is true. Leave the
csn == null path logging, or route it through the normal failure road.

---

todo (non-blocking): the guard this commit adds to commit() is not pinned by any of the three
new tests.

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/RemotePendingChanges.java:243-247

if (pendingChange.getReplayFailures() > 0) { failingChanges--; }
it.remove();

Drop the guard and all three pass. In aChangeKeepsFailingWhileTheChangesAroundItAreReplayed the
closing commit(failing) drains failing(1) and replayed(0): real code 1 → 0, mutant
1 → 0 → -1, and the closing assertFalse reads failingChanges > 0, false for both. In
failingChangesAreCountedOnceAndForgottenWithTheChange every removed change has failures, so the two
are identical. The decode test records no failure at all. A negative counter is not harmless:
hasFailingChanges() then reads false while a change is genuinely failing, restoring the
once-a-second restart loop this commit exists to stop.

Same test, RemotePendingChangesTest.java:416 — the middle assertTrue is vacuous. commit(replayed)
marks it committed, then breaks at the uncommitted head failing (:233-236), never reaching the
decrement. The counter is still the 1 written on line 409, so :416 re-asserts :410.

One line closes both:

// after the closing assertFalse(...)
// real code: 0 + 1 = 1 -> true ;  guard dropped: -1 + 1 = 0 -> false
pendingChanges.recordReplayFailure(/* a fresh listed change */);
assertTrue(pendingChanges.hasFailingChanges());

---

todo (non-blocking): setReplayGiveUpDelay(0) took the assured test off the road the PR is about,
and the comment left behind describes the old road.

opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/AssuredReplicationPluginTest.java:1224

With the delay at 0 the first failure gives up — recordReplayFailure sets
firstReplayFailureTimeMs = now, so getFailingForMs() is 0:

// LDAPReplicationDomain.java:3017
if (failure.getFailingForMs() >= replayGiveUpDelayInMs) {   // 0 >= 0, first failure
  ...
  skipUnreplayableChange(csn, ...);
  return false;        // before replayFailed(:3033), sessionRestartRequested(:3054), restartSession
}

So the retained comment at :1237-1240 — "the domain restarts its session to have the change sent
again, which takes its monitor entry away" — no longer holds; the monitor entry survives and those
assertions are now available. And the assured ack behaviour under a withheld change is no longer
covered by anything. Either keep 0 and rewrite the comment plus add the assertions it was excusing,
or use a short non-zero delay to keep the withhold road.