[#1048] Hold the session restart a released change asks for while a total update runs

[vharseko]

Fixes #1048.

On master. #981 is merged, and the branch is the one commit [#1048] on origin/master (45794c50d0), 36fe165, which is the diff to review. This change is only safe on top of #981, as the issue says: a request the two synchronous roads leave standing is run by nobody but the state checkpointer #981 adds.

Round 2 takes seven of the nine points of the round-1 review: the parked give-back road says the hold as well, the new note is folded with the warning it qualifies, both negative arms of that note are pinned, the comment which described the import direction as unreachable no longer claims it, the javadoc of the disclosed window says how wide it is, and the test's finally prints where it asserted. The rebase came first, since the branch was conflicting, and the conflict was one hunk of replication.properties - #1044 took 329 next to the 331 this adds, both kept - with git range-diff showing the code of round 1 unchanged.

Round 3 takes the three points of the round-2 review: the last resort of replay() no longer reports a change whose road has already run as one whose give-back failed, the cover() assertion which could not fail is moved to where it can, and the restart the give-back runs before the export is counted rather than inferred from the session being up. The rebase came first again - the branch was conflicting on one hunk of replication.properties, where #1019 took 328 next to 331, both kept - with git range-diff showing the code of round 2 unchanged.

Rebased over #1045 after round 3, since #1045 landed and the branch was conflicting. The textual conflict was one hunk of replication.properties: #1045 took 330 next to 331, and both are kept. The semantic one was the question the review of #1045 asked: which contract stands once both are in. It is this PR's, where a replay restart waits out an export. Two changes follow from that, and both are made here because this PR is merged second:

• [#1041] Claim the import context for the length of a session restart, and refuse the total update which lands across it #1045's ReplayDuringImportTest.aRestartDecidedWhileAnExportRunsStopsTheSessionItStreamsOver is removed. runRequestedSessionRestarts() is the only replay-road caller of restartSession(), so the stop that case waits for is no longer reached, and the export direction is pinned by ReplayDuringExportTest.
• The javadoc of disableServiceUnlessImportInProgress() now says an export reaches its "not an owner" arm only when it begins between this PR's ieRunning() read and the claim.

The round-3 commit is otherwise unchanged.

What was wrong

A change which could not be replayed is given back and the session restarted for it, so that the replication server sends it again - and the restart is run by the thread which released the change, on two roads: recoverFromReplayFailure() once the attempts in place are spent, and the last resort of replay(). On both restartSession() stops the session whatever it is carrying. An export from this replica - the initialization of another replica from this one - publishes its entries over that very broker: disableService() is broker.stop() on the same object, exportLDIFEntry() falls out of its flow-control loop on broker.shuttingDown(), publish() answers false, and the export is given up as ERR_INIT_RS_DISCONNECTION_DURING_EXPORT. The replica being initialized is left to be initialized again: minutes on a large backend, spent for a change which would have waited. (The NewAttempt retry in initializeRemote() does not save it: while (!done && ++attempt < 2) leaves the loop on the continue, so NOTE_RESENDING_INIT_TARGET is logged and nothing is resent - pre-existing, and not touched here.)

#968 decided this on purpose for its own change: an export is not an owner of the session, because owning it would leave the change given back with nothing to restart the session for it - an export reloads no state, so no import-end reset would ask again. #981 is what changes that premise: the request outlives the thread which made it, and the checkpointer runs what stands, already holding its own restarts back while ieRunning(). The two synchronous roads were left restarting mid-export, and the round-1 review of #981 filed them as #1048.

What changed

• runRequestedSessionRestarts() leaves the requests standing while a total update is being processed, in either direction, and returns whether it did. It is the one method every road runs its restarts through - the replay thread's own restart, the last resort of replay(), and the checkpointer - so the rule holds for all of them in one place: the checkpointer's pre-check loses its ieRunning(), and the paragraph which said why moves to the method which now decides it. On the import direction the request is not made at all (sessionHasAnOwner() returns before it) except by the last resort, which asks for one before it hands the change back and does not run it there - as it did not at base, under the same owner guard.
• recoverFromReplayFailure() says the restart is waiting, with NOTE_REPLAY_SESSION_RESTART_HELD_BY_TOTAL_UPDATE (331): the WARN_REPLAY_RETRYING_CHANGE line it has just logged ends in "restarting the session to the replication server so that it is sent again", and for the length of an export that is not what happens - a change not delivered again for minutes should not read as a change nobody asked for. The line qualifies that warning, so it is written where the warning was written and folded where the warning was folded: logReplayRetryWarning() now says whether it wrote its line, and a backend which fails every delivery of a long export gets one pair rather than one 331 per delivery. Neither is built on the road out of a JVM which has run out of memory, for the reason the warning is not built there. abandonReplay()'s hold, which [#925] Keep asking for a session restart until it has run #981 accepted without a line, is a second long; this one is the export.
• The give-back of the parked changes says it for them too. giveBackParkedChanges() returns the changes it handed back and asked the restart for - the list the release already allocated, so the answer costs no allocation on the road out of a JVM which has run out of memory - and the last resort of replay() writes one 331 per change when the restart it runs for them is left standing. They are reported as given back to a replication server "which still owns it and sends it again", and during a total update it does not send them yet; these are the changes no 307 covers (nothing owned, or an owned change whose give-up budget had fired). Outside the throttle, the way NOTE_REPLAY_PARKED_CHANGE_GIVEN_BACK is: that road writes one line per change either way, and 331 stands next to those.
• The last resort's own restart is unchanged: it asks NOW, calls the same method, and a restart held there is left to the checkpointer like a restart which threw; its comment says so.
• The last resort speaks only for a give-back which did not run (round 3). It forgets the change it owned once recoverFromReplayFailure() or abandonReplay() has returned, so a throw out of what follows - the restart run for the parked changes, or the 331 lines this adds there - no longer writes ERR_REPLAY_GIVE_BACK_FAILED ("released without its failure being counted") for a change which was counted and given back, or given up on. The throw out of that restart could already do it at base. A restart which throws inside recoverFromReplayFailure() itself still reaches the last resort with the change set, as at base: clearing it by ownership instead would leave a change released by replayFailed() with no request standing if logReplayRetryWarning(), which runs between the release and the request, threw - the request(NOW) of the last resort is what saves that one.

What stays open, deliberately. The read of ieRunning() and the export's claim of its context share no lock, so an export which begins between the read and a stop this call makes is still cut - the same window #1041 describes on the import side, where the whole export was before. The window is the call rather than a few statements of it: restartSession() stops the session before it waits its backoff out, so a request taken later in the loop is stopped up to ten seconds after the read - including the restart of a request another thread has just reported as held. #1045 (#1041), now on master, turned the owner read of restartSession() into a claim on importExportContext, and a claim which fails against an export still stops the session. That claim could give the request back instead of stopping, which would close the window for good. The rebase over #1045 does not do it, since it would change behaviour rather than resolve a conflict, so it is left for a follow-up. restartService() during an export - a configuration change accepted before the export began - is #1040's shape (#1042), and isConfigurationChangeAcceptable() already refuses one while ieRunning().

On the import side of that window the message says more than that road does: an import which claims the context between the two reads reaches the line, and importBackend() clears the request in its finally rather than running it, so no restart follows. What an operator reads the line for still holds - the change is not lost, and comes back once the total update is over - because the import loads the ServerState of the exporter and the session started at its end asks for everything that state does not cover. The comment says as much where it used to call the import direction impossible.

Three consequences worth stating. The ServerState of this domain stands behind the change for the length of the export, as it does behind the checkpointer's hold in #981; the ds-sync-state the export carries does not cover the change, so the replica being initialized asks the replication server for it and everything after it - nothing is lost. And replay-give-up-delay (300 000 ms by default) is measured from the first failure of the change, so an export which outlasts it has the next failed delivery give the change up at once: the budget is a wall-clock one by its documented design, an administrator who runs a long export on a domain whose backend is failing a change can raise it, and subtracting the hold from ReplayFailure would complicate it for that one combination. Left as is, said here. Third, the hold has no upper bound of its own: waitForRemoteEndOfInit() loops while (!done && !broker.shuttingDown()) with no deadline, so a peer which never leaves FULL_UPDATE_STATUS keeps ieRunning() true and the standing restart with it - and the 10 s which looks like a bound is not one, since int reconnectWait = 0 is declared inside the do body and the test reads 1 < 10 on every pass. Both are at base and untouched here; what does bound itself is the streaming phase (ERR_INIT_HEARTBEAT_LOST_DURING_EXPORT) and a failed import, which leaves the status from its own finally.

Tests

ReplayDuringExportTest.aReplayWhichFailsDuringTheExportLeavesTheSessionToTheExport	green. The importer is a broker of the test which asks for the total update, enters the full update status and withholds its acknowledgement: the exporter publishes up to the initialization window and waits, and the change - published through the replication server beforehand, replayed by the test from its synchronous queue - fails its attempts in place there. A second change fails during the same export (round 2): its warning is folded into the count the next one carries, and no 331 is written for it; once the first change's redelivery has committed, the ServerState is asserted not to cover it (round 3 - read earlier, the uncommitted first change made that assertion unable to fail). The export is then acknowledged and read to the DoneMsg with every entry of the domain in it, NOTE_REPLAY_SESSION_RESTART_HELD_BY_TOTAL_UPDATE names the first change, and that change is delivered again over the session the checkpointer restarts once the export is over, applied, and covered by the ServerState
ReplayDuringExportTest.theParkedChangesGivenBackDuringTheExportAreReportedAsHeld (new, round 2)	green. A change is parked behind one whose operation is refused, and a replay is then unwound by the ack of a change it had applied: the give-back on the way out hands the parked change back. Walked twice - before the export, where the restart runs - the generation of the session is read under serviceStateLock before the replay and the moment it returns, and must have moved (round 3), since a request left standing would be run by the checkpointer a tick later with the session found up all the same - and no 331 is written, and during it, where the request is left standing, 331 names the change, and the export streams to its end
ParkedChangeGiveBackTest (round 2)	green, 4/4. The case where the give-back runs its own restart pins both negative arms of 331: neither the change whose replay failed nor the change which was parked is reported as held, because no total update is being processed over the session
ParkedChangeGiveBackTest.aThrowAfterTheChangeWasTakenCareOfIsNotReportedAsAFailedGiveBack (new, round 3)	green. A change fails and the ack of its delivery runs out of memory, so the replay is unwound with the change still owned; its give-up budget is spent at once (the case lowers it through the configuration the domain reads at every decision), so its road gives it up and runs no restart; the restart the parked change was given back with runs after that and is asked to fail. ERR_REPLAY_SKIPPING_CHANGE names the change, the injected failure comes out with the error, and no ERR_REPLAY_GIVE_BACK_FAILED names it

Two things about the fixture, since neither is written down anywhere else. An EntryMsg is a buffer of the export stream (BufferedOutputStream, 8 KiB), not an entry, so the data has to outgrow (window + 1) × 8 KiB before the exporter waits: the test says so in a constant and asserts the arithmetic. And waitForRemoteEndOfInit() loops for as long as the importer is in the full update status, ieRunning() with it: the broker leaves that status in a finally - reStart(true), as InitOnLineTest does - whatever the case found before, or the export never ends. That finally prints where it used to assert (round 2): an AssertionError out of it would replace the failure of the stream above it, which is the one worth reading, and its wait is 30 s so that it and the 60 s receive fit inside the 120 s of the case - a case which ends on the TestNG timeout prints neither message. The export is held across one tick of the checkpointer before it is released, so whichever thread comes for the standing request while the export runs is the one under test.

TestSynchronousReplayQueue is backed by a ConcurrentLinkedDeque rather than a LinkedList: the redelivery is offered by the listener thread of the domain and taken by the thread of the test, which no case did before; take() still throws on an empty queue, which is what makes it the synchronous queue it is.

Mutants, each compiled in place of the class and run against the tests named:

mutant	result
the ieRunning() gate of runRequestedSessionRestarts() removed (the fix reverted, the message kept)	red, both cases of ReplayDuringExportTest: the export is cut, ERR_INIT_RS_DISCONNECTION_DURING_EXPORT, and the rest of the entries never arrive
the checkpointer's call to runPendingSessionRestart() removed	red, the first case: the export streams to its end and the change is not delivered again within 30 s of it
warned dropped from the gate of 331	red, the first case: 331 is written for the second change of the export, whose warning was folded into a count
the 331 of the parked give-back road removed	red, the second case: the parked change waits for the export with nothing saying so
the ! dropped from the gate of 331 in recoverFromReplayFailure()	red, all three cases of ParkedChangeGiveBackTest: the restart ran and is reported as held
the 331 of the parked give-back road written whatever the restart did	red, the second case: the give-back before the export runs its restart and is reported as held
the change owned by the last resort not cleared once its road ran (round 3)	red, the new case of ParkedChangeGiveBackTest: ERR_REPLAY_GIVE_BACK_FAILED is written for a change which was given up on
a change whose warning was folded recorded as replayed rather than asked for again (round 3)	red, the first case of ReplayDuringExportTest: the ServerState covers the second change once the first one has committed
the give-back's request left standing, 331 written only under ieRunning() (round 3, the reviewer's mutant)	red, the second case: the generation of the session has not moved when the replay before the export returns

Run on this head through Maven (-Pprecommit verify, one JVM per class): ReplayDuringImportTest 9/9 (this PR's six cases and three of #1045's), ReplayDuringExportTest 2/2, ParkedChangeGiveBackTest 4/4, SessionRestartTest 4/4, SessionRestartRequestsTest 7/7, SessionRestartBackoffTest 3/3, 29 tests with nothing skipped, and the javadoc gate is green. The rest of the 16-class list and the mutants were not run again on this head.

Run on be972e42c7, the round-3 head before the rebase over #1045, one JVM per class, all green: ReplayDuringExportTest 2/2, ReplayDuringImportTest 6/6, ParkedChangeGiveBackTest 4/4, SessionRestartBackoffTest 3/3, SessionRestartRequestsTest 7/7, SessionRestartTest 4/4, DisabledDomainServerStateTest 2/2, LDAPReplicationDomainConfigChangeTest 10/10, ServerStateFlushTest 4/4, NamingConflictTest 21/21, RemotePendingChangesTest 24/24, ConfigChangeDuringImportTest 4/4, UpdateOperationTest 41/41, DependencyTest 3/3, InitOnLineTest 11/11, GenerationIdTest 4/4 - the classes which restart a session, give a change back or drive a total update, 150 tests, nothing skipped. ReplayDuringExportTest was run through Maven (-Pprecommit verify) as well, and the javadoc gate is green.

Ordinal

NOTE_REPLAY_SESSION_RESTART_HELD_BY_TOTAL_UPDATE_331: master holds up to 330 (#1045 took 330, #1044 329, #1019 328). 331 is claimed by nothing on master or on any open PR head.

[vharseko]

@maximthomas rebuilt on master at c6b483127f, now that #981 is merged: the branch is the one commit [#1048], 34cab95de9, and the three [#925] commits it carried are gone. The copy of #981 it was stacked on (c2455f8) was one round behind what master took - [#925] Time disable() against the woken backoff, count the wakes, and pin the clears and the give-back level - and the commit goes over the merged head without a conflict: that round is in the wait of restartSession() and in enable(), away from the ieRunning() gate this change puts on runRequestedSessionRestarts(). The add/add on SessionRestartBackoffTest GitHub was reporting was between that stale copy and master, not this commit's.

UpdateOperationTest 33/33, ReplayDuringExportTest 1/1, ReplayDuringImportTest 4/4, SessionRestartBackoffTest 3/3 in one pass on this head, Skipped: 0; the mutants are not re-measured. The description says "on master" where it said "stacked on #981", and that 325 is master's.

[vharseko]

@maximthomas rebased onto master at 80481f756d - the squash of #988, which brought #954 and #986 in - now that the branch was conflicting: the one [#1048] commit is 014e9e1466. Nothing new is proposed here; this only says what the conflict was and how it was taken.

The conflict was in one place, and it was prose: the comment of the last resort of replay(), where #954's fifth round spelled out which restarts are left to the state checkpointer - one which threw, one a domain whose session has an owner would refuse - and this change adds the one a total update holds. The list carries all three now. runRequestedSessionRestarts() with its ieRunning() gate, the checkpointer's pre-check it took the gate from, recoverFromReplayFailure() with the NOTE, replication.properties and the two test files merged on their own; TestSynchronousReplayQueue is byte for byte the change #1062 makes to the same file, so the two merge in either order. git range-diff against 34cab95de9 shows the code the same.

Re-run after the rebase, one JVM per class: UpdateOperationTest 41/41 - the cases of #954 and #986 included - ReplayDuringExportTest 1/1, ReplayDuringImportTest 6/6, SessionRestartBackoffTest 3/3. The two mutants are not re-measured on this head. The description says so.

[maximthomas]

praise: The gate sits in the one place every restart road runs through, so the rule holds for all of them at once.

• runRequestedSessionRestarts() (LDAPReplicationDomain.java:4097-4099) is what the replay thread, the last resort of replay() and the state checkpointer all call, so the ieRunning() refusal did not have to be repeated three times — and the checkpointer's own pre-check could drop its copy.
• The new case is measured, not asserted: the gate reverted turns ReplayDuringExportTest red with ERR_INIT_RS_DISCONNECTION_DURING_EXPORT, and the checkpointer's runPendingSessionRestart() removed turns it red on the redelivery — two different mutants, two different failures.
• Ordinal 331 was checked against every open PR head rather than against the descriptions (#1019 328, #1044 329, #1045 330); replication.properties has no duplicate check to catch a collision.

---

issue (non-blocking): The parked give-back road logs "sends it again" during an export and nothing says the restart waits.

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:2826-2845, :4268-4273

giveBackParkedChanges() requests the restart at :4268 and then writes one NOTE_REPLAY_PARKED_CHANGE_GIVEN_BACK per released CSN — "given back to the replication server, which still owns it and sends it again". The caller at :2826 (parkedGivenBack && !replayThreadShutdown.get() && !sessionHasAnOwner(), the arm a non-OOM Throwable takes on a live replay thread) then calls the same gated runRequestedSessionRestarts() at :2845 and drops its return. During an export those changes wait for its length with no counterpart of the :4035-4046 held line — the very gap this PR closes for WARN_REPLAY_RETRYING_CHANGE. The sub-arm where no 331 covers them at all is owned == null (nothing owned, so recoverFromReplayFailure() is never reached from :2823) or an owned change whose give-up budget already fired and returned at :3976. The body's "abandonReplay()'s hold is a second long" argues the :4067 call site, not this one: :2826 requires !replayThreadShutdown.get().

  // :4250 — an overload that says what was released; the old signature delegates with a throwaway list
  private boolean giveBackParkedChanges(SessionRestart restart)
  {
    return giveBackParkedChanges(restart, new ArrayList<CSN>());
  }

  private boolean giveBackParkedChanges(SessionRestart restart, List<CSN> released)
  {
    ...
    sessionRestarts.request(restart);
    for (CSN csn : parked)
    {
      incProcessedUpdates();
      logger.info(NOTE_REPLAY_PARKED_CHANGE_GIVEN_BACK, csn, getBaseDN());
    }
    released.addAll(parked);
    return true;
  }

  // :2845 — the same line the recovery road writes, for the changes this road handed back
  if (!runRequestedSessionRestarts())
  {
    for (CSN csn : released)
    {
      logger.info(NOTE_REPLAY_SESSION_RESTART_HELD_BY_TOTAL_UPDATE, csn, getBaseDN());
    }
  }

Behaviour is already right here — the checkpointer runs the standing request once the export ends — so this is the operator-facing half only.

---

issue (non-blocking): The new note escapes the throttle that folds the warning it qualifies.

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:4035-4046, :3827-3846

logReplayRetryWarning() at :4016 writes WARN_REPLAY_RETRYING_CHANGE only when now - lastLogged >= replayRetryWarningIntervalInMs and the CAS takes, and counts the rest into foldedReplayRetryWarnings at :3846. The 331 line at :4046 is unconditional. On a backend failing every delivery during a long export the log then carries one 331 naming a CSN whose 307 was folded into a count — the throttle exists to fold exactly that repetition. The rate is not bounded by IN_PLACE_REPLAY_ATTEMPTS: that bounds the attempts inside one delivery, and the session stays up throughout the hold, so deliveries keep arriving; the bound is the topology's change rate.

  // :3827 — say whether the warning was written
  private boolean logReplayRetryWarning(CSN csn, RemotePendingChanges.ReplayFailure failure)

  // :4016 / :4035 — the held line follows the line it qualifies; the next 307 carries the folded count for the rest
  final boolean warned = logReplayRetryWarning(csn, failure);
  ...
  if (!runRequestedSessionRestarts() && !outOfMemory && warned)

Or: leave it and say in the body that the held line is deliberately outside the throttle. The comment at :4036-4045 argues the line must stand on its own; it does not address the folding.

---

suggestion (non-blocking): 331 is pinned on its held arm only — a mutant which logs it on every give-back stays green.

opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/ReplayDuringExportTest.java:216-218

git grep NOTE_REPLAY_SESSION_RESTART_HELD_BY_TOTAL_UPDATE -- src/test returns exactly one hit, and it is isNotEmpty(). Drop the ! from :4035 (or flip the tail return true at :4142, whose value feeds only this log) and 331 is written on every give-back: ParkedChangeGiveBackTest, UpdateOperationTest and SessionRestartBackoffTest all filter ERROR_TEXT_WRITER by ordinal (325/307, 307, …) and none of 67, 76, 307, 321, 325 is a prefix of 331, so nothing reads the extra record and nothing reads the writer's size. An operator then reads "the restart is held" on a domain with no total update anywhere.

  // in an existing give-back case with no total update running — ParkedChangeGiveBackTest already
  // reads the writer by ordinal and CSN
  assertThat(errorLogRecordsOf(
      NOTE_REPLAY_SESSION_RESTART_HELD_BY_TOTAL_UPDATE.ordinal(), csn))
      .as("the restart was not held: no total update is being processed")
      .isEmpty();

Pin: the negative arm of the new message, in a class that already gives a change back without an export.

---

question (non-blocking): Was the import direction meant to be unreachable at :4046, or is the #1041 window accepted there too?

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

sessionHasAnOwner() is read at :3988 and ieRunning() at :4099, under no shared lock. An import that claims the context between them (ReplicationDomain.java:2561 acquireIEContext(true), then :2580 TO_FULL_UPDATE_STATUS) reaches :4046, and on that road importBackend()'s finally clears the request at :5962 — nothing ever runs it, so the message's "The restart runs once the total update is over" is literally false there. No data is lost: the import reloads the ServerState and the session started at its end asks for everything that state does not cover. If the window is accepted — as the body accepts it for the export side — the comment should not claim a guarantee the earlier read does not give; if it was meant to be unreachable, that is a different change. Either way this is text only, so the severity does not turn on the answer.

       * The line above said the session is being restarted for the change, and it is not
       * yet: a total update is being processed over that session - almost always an export
       * from this replica, since a total update into it owns the session and is refused
       * above, except by an import which claims its context between that read and this one
       * (issue #1041) - and the restart waits for it.

---

suggestion (non-blocking): The test's finally asserts, so a double failure reports the wrong cause.

opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/ReplayDuringExportTest.java:335-346

finishExport()'s finally runs assertTrue(System.currentTimeMillis() < deadline, "the export did not end") in a 60 s loop. An AssertionError thrown from a finally replaces the try block's exception without suppression, so on the double-failure road — the export cut and the context not released within 60 s — the red run says "the export did not end" and the diagnostic the mutant table relies on ("the export was cut instead of streaming to its end", the missing entries) is gone. The same finally is why the 60 s receive in the try plus the 60 s wait here meet @Test(timeOut = 120_000) exactly: on the silent road the TestNG timeout fires inside Thread.sleep() and neither message is printed.

    finally
    {
      leaveTheFullUpdateStatus();
      final long deadline = System.currentTimeMillis() + 60_000;
      while (domain.ieRunning() && System.currentTimeMillis() < deadline)
      {
        Thread.sleep(50);
      }
      // printed, not thrown: a failure of the try block above is the one worth reading
      if (domain.ieRunning())
      {
        System.err.println("the export did not end within 60 s");
      }
    }

The PR's own mutant runs confirm the single-failure road is unaffected: the context is released there, so the try's message propagates.

---

nitpick (non-blocking): The comment on the return from the held arm still names only the import.

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

"the replication server delivers it again, now or - while a total update owns the session - after the import restarts it" was accurate at base, where only sessionHasAnOwner() could refuse the restart. It is reached from the new held arm (:3541 → recoverFromReplayFailure() → :4046 → return true at :4048), which adds the export case. The sibling comment in the same file was rewritten for exactly this (@@ -2856,7 +2856,8 @@); this one was missed.

        // The ack has been published and the change is given back: the replication server
        // delivers it again, now or - while a total update is being processed over the
        // session - once it is over. There is nothing left to replay here.

---

nitpick (non-blocking): "a few statements wide" understates the window on the drain loop.

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

The javadoc describes the disclosed race as the read at :4099 against the stop that follows it. On the drain loop the read precedes a later take()'s stop: restartSession() disables before it waits (:4341) and re-enables after, and a SessionRestart.AFTER_BACKOFF wait runs up to MAX_REPLAY_RETRY_DELAY_IN_MS = 10 000, so one call can stop the session seconds after the ieRunning() that let it through — including a request another thread has just reported as held at :4046. Same shape, same consequence as the disclosed window; only the description of its width is off.

   * A total update which begins between this read and any of the stops this call makes is
   * not seen here, and is cut by it: the read and the claim of the import/export context
   * share no lock, which is issue #1041 on the import side. A request taken later in the
   * loop is stopped a backoff wait after the read, not a few statements after it.

---

note (non-blocking): One sentence of the description describes a pre-rebase base.

• LDAPReplicationDomain.java:2909 at base — "except by the last resort, whose request now stands until importBackend() clears it where before restartSession() consumed it under the owner guard" is not what changed: at base 80481f75 the last resort already skipped runRequestedSessionRestarts() under if (!sessionHasAnOwner()), so the import road of the last resort is unchanged by this PR rather than changed as stated. The code does what the rest of the description says.

---

note (non-blocking): The hold has no upper bound when the peer never leaves the full update status — pre-existing, and worth one sentence in the body.

• ReplicationDomain.java:2009 — while (!done && !broker.shuttingDown()); // infinite wait has two exits only: every waited-for replica out of FULL_UPDATE_STATUS, or broker.stop(). There is no deadline, so ieRunning() stays true for as long as a hung peer stays in that status, and the standing restart with it.
• ReplicationDomain.java:1955-1956 — the 10 s grace that looks like a bound is not one: int reconnectWait = 0 is declared inside the do body, so ++reconnectWait < reconnectMaxDelayInSec is 1 < 10 on every pass and a crashed importer never times out. Present at base, untouched here; a separate issue.
• Nothing is asked of this PR's code: the streaming phase does bound itself (ERR_INIT_HEARTBEAT_LOST_DURING_EXPORT, :2301-2308), a failed import leaves the status from its own finally (:2600-2617), and at base the checkpointer already held standing restarts under the same flag. Only the body's "for as long as the export takes" is worth qualifying.

[vharseko]

@maximthomas round 2, 2506e2a8aa: seven of the nine taken, two answered. Rebased onto master at b960e40edc first - the branch was conflicting - and the conflict was one hunk of replication.properties: #1044 took 329 next to the 331 this adds, both kept, git range-diff shows the code of the round-1 commit unchanged.

issue - the parked give-back road says nothing while an export holds its restart. Taken. giveBackParkedChanges() returns the changes it handed back and asked the restart for - the list the release allocated, so nothing is allocated for the answer on the road out of a JVM which has run out of memory - and the caller at the last resort writes one 331 per change when runRequestedSessionRestarts() leaves the request standing. The sub-arms you name (owned == null, or an owned change whose budget fired) are the ones which now say it.

issue - the new note escapes the throttle. Taken, with the shape you wrote: logReplayRetryWarning() says whether it wrote its line, and 331 follows only where it did. !outOfMemory is gone from the gate - warned is false on that road, since the warning is not built there - and the parked road above is deliberately outside the throttle, the way NOTE_REPLAY_PARKED_CHANGE_GIVEN_BACK is: it writes one line per change either way, and 331 stands next to those.

suggestion - 331 is pinned on its held arm only. Taken. ParkedChangeGiveBackTest pins both negative arms now, in the case where the give-back runs its own restart: the change whose replay failed and the change which was parked, neither reported as held. ReplayDuringExportTest pins the folded arm as well - a second change failing during the same export is not reported as held, because its warning was folded - and the new case pins the parked road's positive arm.

question - was the import direction meant to be unreachable at :4046? The window is accepted, as it is on the export side, and the comment no longer claims a guarantee it does not give: "almost always an export ... except by an import which claims its context between that read and this one (issue #1041)", and then what that road does - importBackend() clears the request rather than running it, and the change is delivered again all the same, because the import loads the ServerState and the session started at its end asks for everything that state does not cover. The message itself is left alone: what an operator reads it for - the change is not lost, it comes back once the total update is over - holds on both roads.

suggestion - the test's finally asserts. Taken: it prints, and the wait is 30 s rather than 60 s so that the receive above it and this one fit inside the 120 s of the case with room to spare - a case which ends on the TestNG timeout prints neither message. What the export being stuck costs is read on the assertions which follow, which wait for the change the restart brings back.

nitpick - the comment on the return names only the import. Taken: it names the total update in either direction now, and what ends the wait on each - the restart which was held, or the session started from the reloaded state.

nitpick - "a few statements wide". Taken: the javadoc says the window is the call rather than a few statements of it, and names the backoff wait a request taken later in the loop is stopped after - including the restart of a request another thread has just been told is held.

note - one sentence describes a pre-rebase base. Right, and it was wrong at round 1 too: at base the last resort already skipped runRequestedSessionRestarts() under if (!sessionHasAnOwner()), so its import road is unchanged by this PR. The sentence is gone from the description.

note - the hold has no upper bound when the peer never leaves the full update status. Recorded in the description, as the pre-existing shape it is: while (!done && !broker.shuttingDown()) has no deadline, and the 10 s which looks like one is not - int reconnectWait = 0 is declared inside the do body, so the test is 1 < 10 on every pass. Nothing of this PR's code changes with it; say the word and I file it.

Runs on this head, one JVM per class: 16 classes of the replication suite - the ones which restart a session, give a change back or drive a total update - 149 tests, nothing skipped; ReplayDuringExportTest and ParkedChangeGiveBackTest through Maven (-Pprecommit verify) as well, 5/5, and the javadoc gate green. Six mutants, each red on an arm of its own, including the two your points added: 331 written for a change whose warning was folded, and 331 written on a give-back whose restart ran. Two pins were tautologies before they were mutants: a modify of a DN which is in the data makes no entryUUID search, and ShortCircuitPlugin forgets its count on both register and deregister - the count is read inside the try now, and the failure of that replay is asserted rather than assumed.

[maximthomas]

praise: Round 1's two operator-facing gaps are closed where the lines are written, not beside them.

• giveBackParkedChanges() now returns the CSNs it handed back (LDAPReplicationDomain.java:4302), oldest first as its @return says (dependentChanges is a TreeSet), and the give-back tail writes NOTE_REPLAY_SESSION_RESTART_HELD_BY_TOTAL_UPDATE for each of them when the restart is held (:2846-2864).
• The held note moved inside logReplayRetryWarning() (:3852, written at :4087), so it is folded with the warning it qualifies instead of repeating past the throttle.
• theParkedChangesGivenBackDuringTheExportAreReportedAsHeld (ReplayDuringExportTest.java:285) walks the parked give-back on both sides of the export, and a second failure during the same export pins the fold.

---

issue (non-blocking): A throw from the new held-note loop is reported as a failed give-back of a change that was given back.

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:2861-2864, :2866-2900

The loop runs inside the try whose catch (Throwable recoveryFailure) treats any throw as a failed give-back of owned. It runs after recoverFromReplayFailure(owned, ...) has already released and counted owned, and owned is not cleared. The comment at :2856-2858 says the loop is built on the OutOfMemoryError road on purpose. Take an OOME-unwound replay during an export, with parked changes. If the loop's allocation throws, the catch calls replayFailed(owned), which does nothing because RemotePendingChanges:342 checks the owner, and merges request(NOW). It then logs ERR_REPLAY_GIVE_BACK_FAILED ("released without its failure being counted") for a change that was released and counted. No state is harmed, but the one error line an operator acts on is false. At the base a throwing runRequestedSessionRestarts() could already reach this catch; the loop adds a second unguarded throw site after the release.

          if (!runRequestedSessionRestarts())
          {
            try
            {
              for (CSN csn : parkedGivenBack)
              {
                logger.info(NOTE_REPLAY_SESSION_RESTART_HELD_BY_TOTAL_UPDATE, csn, getBaseDN());
              }
            }
            catch (Throwable reportFailure)
            {
              // the changes are given back and their restart stands; only the report is lost
            }
          }

Or: clear owned once recoverFromReplayFailure() returns, so the last resort only speaks for a give-back that did not run.

---

suggestion (non-blocking): The cover() assertion in failAReplayOf() cannot fail at its only call site.

opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/ReplayDuringExportTest.java:359-360, :230

failAReplayOf() is called only at :230. At that point the first failing change, from the same CSNGenerator(IMPORTER_ID, 0) and older, is still listed and uncommitted. RemotePendingChanges.commit() drains only the committed prefix, and state.update at RemotePendingChanges:293 is the only ServerState writer on that road. So the state holds nothing for server id 2, and assertFalse(cover(csn)) passes whether the second change replayed or not. The short-circuit count at :350-353 is what pins the failure. This line is redundant, and its message claims more than it checks.

Pin: drop the assertion, or assert cover() for the second change only after the first change's redelivery has committed (:255), where the prefix has drained and a replayed change would be covered.

---

suggestion (non-blocking): The pre-export arm's text claims a restart that the case does not observe.

opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/ReplayDuringExportTest.java:298-303, :415-422

awaitConnected() polls isConnected(), which is already true when the session was never stopped. Take a mutant that leaves the give-back's request standing at LDAPReplicationDomain.java:2846 instead of running it, and writes the held note only under ieRunning(). It passes :297-303, because the checkpointer runs the request within its tick. The requester's own run at that tail was already unpinned before this PR. What this PR adds is the "... processed: it ran" and "brought back by the restart the give-back ran" text, which claims that run.

Pin: either reword both messages to what is checked ("no hold was reported", "the session is up"), or take a count of session starts before unwindTheReplayOf and assert that it moved before awaitConnected() returns. Only the second version kills the mutant.

[vharseko]

@maximthomas round 3, be972e42c7: all three taken. Rebased onto master at 26defc28ef first - the branch was conflicting - and the conflict was one hunk of replication.properties: #1019 took 328 at the end of the file where this adds 331, both kept; git range-diff shows the code of the round-2 commit unchanged.

issue - a throw from the new held-note loop is reported as a failed give-back. Taken, with your second shape: owned is cleared once recoverFromReplayFailure() - or abandonReplay() - has returned, so the last resort only speaks for a give-back which did not run. That closes the throw site this PR added and the one you point at the base as well: a throw out of the parked road's runRequestedSessionRestarts() after the owned change's road has run. The comment of the last resort lists the held line among the roads which leave a request standing with no change of this thread's to hand back.

Not taken further, on purpose: a restart which throws inside recoverFromReplayFailure() still reaches the last resort with owned set, as at base. Clearing it by ownership rather than by return - "speak only if this thread still owns the change" - would look like the general form, but it is wrong on one road: logReplayRetryWarning() runs after replayFailed() released the change and before sessionRestarts.request(...), so a throw there would leave a released change with no request standing, and the request(NOW) of the last resort is what saves it today.

Pinned in ParkedChangeGiveBackTest.aThrowAfterTheChangeWasTakenCareOfIsNotReportedAsAFailedGiveBack: a change fails and its ack runs out of memory, so the replay is unwound with the change still owned; its budget is spent at once (the case lowers it through the configuration the domain reads at every decision), so its road gives it up and runs no restart; the restart the parked change was given back with runs after that and is asked to fail. With the line which clears owned removed the case is red on ERR_REPLAY_GIVE_BACK_FAILED written for a change which was skipped. ModifyMsgWhoseOperationRefusesAControl loses its final for the fixture.

suggestion - the cover() assertion in failAReplayOf() cannot fail. Right: the first change is still listed and uncommitted there, so the state covers nothing newer from that server whatever became of the second. The assertion moves to the end of the case, after the first change's redelivery has committed - the second one was handed to the domain rather than published, so nothing delivers it again and it holds the state where it stands. A mutant which records a change whose warning was folded as replayed is red there and nowhere else.

suggestion - the pre-export arm's text claims a restart the case does not observe. Taken with the pin rather than the rewording: the generation of the session is read under serviceStateLock - the way SessionRestartTest reads it - before the replay is unwound and the moment it returns, and it must have moved. The restart of that give-back runs on the test's own thread before the replay returns, and one left standing is run by the state checkpointer a tick later, so the read tells the two apart where awaitConnected() could not. Your mutant - the give-back's request left standing, 331 written only under ieRunning() - is red on it. The two messages stay as they were, since they now say what is checked.

Runs on this head, one JVM per class: 16 classes of the replication suite - the ones which restart a session, give a change back or drive a total update - 150 tests, nothing skipped; ReplayDuringExportTest through Maven (-Pprecommit verify) as well, and the javadoc gate green. Three mutants, each red on its own assertion: owned not cleared, a folded change recorded as replayed, and the give-back's restart left standing.

[vharseko]

@maximthomas rebased onto master at 45794c50d0 - #1045 landed and the branch was conflicting - and the one [#1048] commit is 36fe165b38. There are two things to report: the textual conflict, and the semantic one you raised on #1045.

The textual conflict was one hunk of replication.properties: #1045 took 330 at the end of the file where this adds 331. Both are kept, and there are no duplicate ordinals.

The semantic one is the question you asked on #1045 ("once #1049 is in, which contract stands"). #1045 was answered there, and this is the PR merged second, so the change lands here. The contract is this PR's: a replay restart waits out an export. runRequestedSessionRestarts() is the only replay-road caller of restartSession(), so aRestartDecidedWhileAnExportRunsStopsTheSessionItStreamsOver would hold nothing reachable and fail on stopHeld.await.

• The case is removed from ReplayDuringImportTest. The export direction is pinned by ReplayDuringExportTest, which this PR adds.
• The javadoc of disableServiceUnlessImportInProgress() still says an export is not an owner there, since that is what the method does. It now says the replay restart does not reach it while a total update runs (Replication: a session restart run by the thread which released a change cuts the export the session carries #1048), so an export only gets there when it begins between that read and the claim. That is the window the javadoc of runRequestedSessionRestarts() already describes.
• The setServiceStopHook seam stays: aRestartDecidedBeforeTheImportIsClaimedRefusesTheImport still uses it.

Nothing else of this PR's code moved in the rebase. LDAPReplicationDomain, ParkedChangeGiveBackTest, ReplayDuringExportTest and TestSynchronousReplayQueue merged on their own, and the commit message is the round-3 one, unchanged.

Run on this head through Maven (-Pprecommit verify, one JVM per class): ReplayDuringImportTest 9/9 (this PR's six cases and three of #1045's), ReplayDuringExportTest 2/2, ParkedChangeGiveBackTest 4/4, SessionRestartTest 4/4, SessionRestartRequestsTest 7/7, SessionRestartBackoffTest 3/3, 29 tests with nothing skipped, and the javadoc gate is green. The rest of the 16-class list and the mutants were not run again on this head.