[#1061] Ask for the session restart under an owner as well, and leave it to the state checkpointer

[vharseko]

Fixes #1061.

On master. Rebased onto master at e333af0c8f: d23b4fcd53 is the [#1061] commit, and ed5aad5bf0 is the round commit for the review of 24.09. #1049 ([#1048]) landed over the same three roads, and the PR was conflicting. It made runRequestedSessionRestarts() leave every request standing while ieRunning(), made giveBackParkedChanges() return the changes it handed back, and added NOTE_REPLAY_SESSION_RESTART_HELD_BY_TOTAL_UPDATE. In recoverFromReplayFailure() the request comes first, then the owner arm returns, then #1049's warned / held-restart note. giveBackParkedChanges() asks before the owner read and returns Collections.emptyList() under the owner, so the caller has nothing to run or report. Earlier, #1042 ([#1040]) rewrote the javadoc of sessionHasAnOwner(). Its paragraph on the configuration change (restartService()) stands after this PR's paragraph on the three roads: they are two roads, and restartService() records the suppressed restart for the administrator while restartSession() leaves the request to the checkpointer. The third road, giveBackParkedChanges(), is #954's, on master since the squash of #988.

What was wrong

recoverFromReplayFailure(), abandonReplay() and - with #954 - giveBackParkedChanges() release the change they hand back and, while the session has an owner (sessionHasAnOwner(): the domain shutting down or disabled, or a total update into this replica from the moment it is asked for), asked for no session restart. Two of the three owners forget the pending changes and every request with them - disable() / enable() and the import's finally clear both - so nothing was lost there. The third does not: a total update which is asked for and never begins - initializeFromRemote() refused at publish, an ErrorMsg from the exporter, the two-minute watchdog of InitializeTask (abortStalledInitializeFromRemote()), or the second attempt initialize() makes after a failed import, which re-publishes the request and keeps the context - ends in releaseIEContext() alone, and a change released under it stayed listed, uncommitted and owned by nobody until the next failed replay of this domain restarted the session. On a domain which then goes quiet that is for good: commit() moves the ServerState no further than the oldest uncommitted change, so the state in memory, and the one persisted from it, stop at the change - the shape of #954, one road over.

#968 left it that way on purpose: the request was a flag which only the thread of a failed replay ran, so a flag set under an owner was served by the next failed replay, which would have set it itself. #981 changed that premise - the state checkpointer runs a request nobody ran, and holds it for as long as ieRunning() - and the last resort of replay() relies on it already: its request is made under an owner as well, and only its run is gated. Found in the fourth review of #985, filed as #1061.

What changed

• The three roads ask on every road, as one policy. The request is made once the change is released and before the owner is read - remotePendingChanges.replayFailed(csn) / releaseParkedChangesOwnedByCurrentThread(), then sessionRestarts.request(…), then sessionHasAnOwner() - and the owner arm returns with it standing. What is asked for is what was asked for before: NOW on a thread which is stopping or which an OutOfMemoryError is ending, AFTER_BACKOFF otherwise; abandonReplay() asks NOW as it did. The owners which forget the pending changes forget the request with them (disable() :5248, enable() :5428, the import's finally :5943); the one which does not leaves it to the checkpointer, which runs it within its tick of releaseIEContext() and has the replication server send the change again.
• The owner roads stay silent, and the give-back's count stays off them. WARN_REPLAY_RETRYING_CHANGE (through the throttle of Replication: the replay retry warning is logged once per delivery, with nothing bounding it once the give-up budget is raised #942), NOTE_REPLAY_ABANDONED_CHANGE and NOTE_REPLAY_PARKED_CHANGE_GIVEN_BACK are not logged under an owner, and the parked deliveries are not counted as processed, as before: a server which is shutting down abandons every change in flight, and none of them is delivered again before it is started back, so a line which says the replication server sends the change again would not hold on every owner road. On the never-begun road the change is now asked for again without a line saying so, and a parked delivery given back there is counted by the delivery which replays it; said here rather than papered over with a line of its own.
• The gates on the two runs of replay() stay, as an early exit. They are the parked road and the last resort. They were load-bearing when this PR was written, and are not since [#1048] Hold the session restart a released change asks for while a total update runs #1049: under a total update into this replica runRequestedSessionRestarts() leaves the request standing by itself, because importInProgress() implies ieRunning(). The owners it would still be refused under - the domain disabled or shutting down - forget the request with the pending changes. No test can tell a gate from its removal, so neither is pinned. The javadoc of sessionHasAnOwner(), the comments of the three arms, of the parked road, of enable() (whose account of the disabled = false ordering rested on abandonReplay() asking for nothing) and of the test hook requestSessionRestart() say what holds now.
• A change the owner forgot asks for nothing (round of 24.09). RemotePendingChanges.replayFailed() returns whether it released the change, and abandonReplay() makes no request when it did not. A replay thread which reaches its give-up after the import's clear, or enable()'s, would otherwise leave a NOW request standing past that clear, and the checkpointer would bounce the session the owner had just started. recoverFromReplayFailure() already behaved this way through recordReplayFailure().

What this does not close. A no-owner thread which read no owner, made its request and reached runRequestedSessionRestarts() while the owner arrived spends the merged flag - its own and any request made under the owner in between - on restartSession()'s refusal. That window is the few statements between the read and the lock, the same class as the last resort's today, and it is not a regression (before this, nothing was asked for under the owner at all); closing it for good means restartSession() saying it refused and the caller giving the request back, which is a change to the loop of runRequestedSessionRestarts() and belongs to a follow-up. #1049 closes the import direction of it in passing - its runRequestedSessionRestarts() leaves the requests standing while ieRunning() - and #1045 turns the owner read of restartSession() into a claim. With #1049 in, the import part of the parked road's gate is no longer load-bearing, and the mutant of the table below which pinned it survives by construction.

Tests

All in ReplayDuringImportTest, on the one owner a test can hold open and then take away: initializeFromRemote() with the exporter holding the InitializeRequestMsg, and in the place of the answer abortStalledInitializeFromRemote(0) - the watchdog's road, and the one road out of an unanswered request a test can take at a time of its choosing (an ErrorMsg releases the context only through the task, which the test does not have; production always has one). Each change travels the replication server - published by the exporter's broker, taken off the domain's synchronous queue by the test - which is what has it to deliver again; a message handed to the domain, as the class's other cases do, is one the replication server never had.

case	road	result
aChangeReleasedUnderARequestWhichIsNeverAnsweredIsDeliveredAgain (new)	recoverFromReplayFailure() - the attempts in place spent on an entryUUID search which does not run	green; no retry warning, session up, !cover(csn); after the abort the change is delivered again within the checkpointer's tick + the 1 s backoff, replayed, applied, covered
aChangeAbandonedUnderARequestWhichIsNeverAnsweredIsDeliveredAgain (new)	abandonReplay() - replayed with the stopping flag set, abandoned at the top of its first attempt	green; not applied, no NOTE 309, session up; delivered again after the abort and covered
aParkedChangeGivenBackUnderARequestWhichIsNeverAnsweredIsDeliveredAgain (new)	giveBackParkedChanges() - the parked change waits for one another thread of the test holds before its operation is built (ModifyMsgWhoseOperationWaitsToBeBuilt, new), so nothing has failed and the give-back's request is the only one standing; the replay unwound is the OOME-on-ack fixture of #954	green; dependent-changes-size 1 → 0, held change committed when let go, parked change delivered again after the abort and covered
aParkedChangeGivenBackWhileTheRequestIsOnItsWayLeavesTheSessionToTheOwner (renamed from …IsNotAskedForAgain, assertions unchanged)	the give-back's owner arm: not counted, not reported, session left to the import which then runs to its end	green
aChangeAbandonedOnceTheImportForgotItAsksForNoRestart (new, round of 24.09)	abandonReplay() after the import forgot the change: the test thread owns a delivery across a whole import, then abandons it	green; no restart runs within two ticks of the checkpointer (failNextSessionRestarts(1) unspent), no NOTE 309
the class's other eight cases		green, 13/13

The three new cases were run first against the class as it stands on #985's head (git show HEAD:…LDAPReplicationDomain.java compiled ahead of target/classes): 3/3 red, each on the wait for the redelivery - "was not delivered again once the request gave up … within 30000 ms".

Mutants, each compiled in place of the class and run against the case meant to catch it:

mutant	result
recoverFromReplayFailure() asks only when the session has no owner (the request moved back below the owner arm)	red on the released case: "was not delivered again once the request gave up"
abandonReplay() asks only when the session has no owner	red on the abandoned case: "was not delivered again once the request gave up"
giveBackParkedChanges() asks only when the session has no owner	red on the parked case
the parked road's gate forgets the import: !shutdown.get() && !disabled in place of !sessionHasAnOwner()	measured red before #1049; equivalent since, because the give-back returns nothing under the owner and runRequestedSessionRestarts() holds the request itself while ieRunning()
round: abandonReplay() ignores what replayFailed() returns	red on aChangeAbandonedOnceTheImportForgotItAsksForNoRestart: "expected [1] but found [0]"
round: recordReplayFailure(csn, now) inside abandonReplay()	red on the abandoned case: "counted against its budget … expected [0] but found [1]"
round: the owner arm of abandonReplay() calls restartSession(false), and restartSession() stops refusing	red on the abandoned case: the restart hook throws out of the replay

The three owner cases count the restarts run on the thread of the test with failNextSessionRestarts(1) / getSessionRestartFailuresLeft(). They used to read isConnected(), but a restart run inline is back up before that read. The abandoned case also asserts that the delivery was handed to the thread and that changes-with-failed-replay is 0. RemotePendingChangesTest pins what replayFailed() returns.

Run on ed5aad5bf0, one JVM per class: ReplayDuringImportTest 13/13, RemotePendingChangesTest 25/25, ParkedChangeGiveBackTest 4/4, ReplayDuringExportTest 2/2, DisabledDomainServerStateTest 2/2, SessionRestartBackoffTest 3/3, UpdateOperationTest 41/41, Skipped: 0. On d23b4fcd53 also ConfigChangeDuringImportTest 4/4 and SessionRestartRequestsTest 7/7. Before the rebases, on 25f1034981: ReplayDuringImportTest 9/9, ParkedChangeGiveBackTest 3/3 (as of #985's fifth round), SessionRestartBackoffTest 3/3, SessionRestartTest 2/2, DisabledDomainServerStateTest 2/2, RemotePendingChangesTest 24/24, SessionRestartRequestsTest 7/7, ServerStateFlushTest 4/4, LDAPReplicationDomainConfigChangeTest 9/9, DependencyTest 3/3, UpdateOperationTest 40/40, InitOnLineTest 10/10, GenerationIdTest 4/4, ReSyncTest 2/2, NamingConflictTest 21/21, AssuredReplicationPluginTest 14/14, ReplicationDomainTest 12/12 - the classes which restart a session, drive a total update or read the requests, 169 tests, Skipped: 0.

[vharseko]

@maximthomas restacked on master at 80481f756d: the squash of #988 took #985's [#954] commit in with it, so the [#954] this branch carried is gone and the branch is the one [#1061] commit, 25f1034981 - the diff to review is what it was. 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 javadoc of giveBackParkedChanges(), where this change rewrote the paragraph on a domain whose session has an owner - the changes are released and asked for, and the request is left to the state checkpointer - and #986 added a paragraph on the thread which is stopping right under it. Both are there now, this change's first. The body of the method, ReplayDuringImportTest, ModifyMsgWhoseOperationWaitsToBeBuilt and TestSynchronousReplayQueue merged on their own. git range-diff against d047625c57 shows the code byte for byte the same.

Re-run after the restack, one JVM per class: ReplayDuringImportTest 9/9, UpdateOperationTest 41/41 - the cases of #954 and #986 included - ParkedChangeGiveBackTest 3/3. The four mutants are not re-measured on this head. The description says "on master" where it said "stacked on #985".

[vharseko]

@maximthomas rebased onto master at 26defc28ef - the branch was conflicting - and the one [#1061] commit goes over it (529afd16af). Nothing new is proposed here; this only says what the conflict was and how it was taken.

The conflict was the javadoc of sessionHasAnOwner(), which #1042 rewrote as well: this PR replaces the paragraph saying a change given back under an owner is asked for by nobody, and #1042 added a paragraph after it on the configuration change, whose restart restartService() refuses under an owner and records for the administrator. Both are kept, this PR's paragraph first. They describe two roads which do not meet - the configuration change goes through restartService(), the three roads of this PR request a restart the checkpointer runs through restartSession() - so neither text needed a word changed. restartService() itself and the rest of LDAPReplicationDomain, ReplayDuringImportTest, TestSynchronousReplayQueue and the new fixture merged on their own; #1037's changes to the domain sit away from the three arms.

On this head: ReplayDuringImportTest 9/9, ParkedChangeGiveBackTest 3/3, ConfigChangeDuringImportTest 4/4 - #1042's class, the other reader of that javadoc - RemotePendingChangesTest 24/24, Skipped: 0. The description says so; its line references to the three sessionRestarts.clear() calls now point at them (:5248, :5428, :5943), and the list of 169 tests is marked as run on the head before the rebase.

Not measured again: the four mutants of the table. The three arms and the parked road's gate are the lines they were measured on, unchanged by the rebase.

[maximthomas]

praise: The fix sits where #1061 is: every road which releases a change now asks for the restart before it reads the owner, so a total update which never begins leaves the request to the state checkpointer.

• The request moves above sessionHasAnOwner() in all three roads, recoverFromReplayFailure() (:4004), giveBackParkedChanges() (:4234) and abandonReplay() (:4278). The owner arms stay silent and uncounted, and the three roads follow one policy.
• Each new ReplayDuringImportTest case sends its change through the replication server and takes the watchdog road, abortStalledInitializeFromRemote(0). The description reports each arm's revert mutant red on its own case.

---

issue (non-blocking): abandonReplay() asks for a restart when its change is no longer listed, so a request made after an owner's clear outlives that owner.

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:4259-4285, :5942-5946, :5428

replayFailed(csn) does nothing for an unlisted CSN, and request(NOW) now comes before the owner read. Two owners clear the requests while they still own the session:

• The import's finally clears them, and importInProgress() holds until ReplicationDomain.initialize() runs broker.reStart(false) and then releaseIEContext().
• enable() clears them, then runs loadDataState(), and only then sets disabled = false.

A thread can read the flag as set and reach abandonReplay() only after that clear: after processUpdateDone()'s ack publish, or as a straggler past the bounded awaitReplayDrained(). Its NOW request survives, and the checkpointer then stops and restarts the session the owner has just started, for a change the clear already dropped. Nothing is lost: it is one extra bounce, and the reloaded state absorbs the redelivery. At the base the owner arm returned before asking. The same window contradicts enable()'s "every request standing here is that one". recoverFromReplayFailure() does not hit it, because recordReplayFailure() returns null for an unlisted CSN.

// RemotePendingChanges
public boolean replayFailed(CSN csn)
{
  pendingChangesWriteLock.lock();
  try
  {
    final PendingChange change = pendingChanges.get(csn);
    if (change != null && !change.isCommitted() && change.isOwnedBy(Thread.currentThread()))
    {
      change.setOwner(null);
      changeBeingReplayed.remove(Thread.currentThread(), csn);
      return true;
    }
    return false;
  }
  finally
  {
    pendingChangesWriteLock.unlock();
  }
}

// LDAPReplicationDomain.abandonReplay(CSN)
if (!remotePendingChanges.replayFailed(csn))
{
  // No longer listed: the owner which cleared the pending changes starts the session again
  // from the state it reloaded, which asks for everything that state does not cover.
  return;
}
sessionRestarts.request(SessionRestart.NOW);

Or: keep the code as is and say in the enable() and import-finally comments that this window exists.

---

suggestion (non-blocking): The abandon case never checks that the change took the uncounted abandon road, and it drops the result of markInProgress().

opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/ReplayDuringImportTest.java:509, :496-528

The javadoc says the change is abandoned "without being counted against its budget", but the case reads no failure record, budget or counter. Two mutants keep it green:

• recordReplayFailure(csn, now) added inside abandonReplay().
• A stopping thread's change sent to recoverFromReplayFailure() under an owner only. That road also asks NOW for a stopping thread and logs nothing under the owner.

replayed-updates-failed counts only give-ups, so it cannot tell the two roads apart. The redelivery half of the case is pinned.

assertTrue(domain.markInProgress(delivered), "the delivery was not handed to this thread");
domain.replay(delivered, new AtomicBoolean(true));

Pin: after the replay, assert that no replay failure is recorded for csn, reading RemotePendingChanges the way RemotePendingChangesTest does. Or drop the "not counted" half from the javadoc.

---

suggestion (non-blocking): Nothing can kill the removal of the last-resort gate in replay(), which the description calls load-bearing.

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

The gate is reached only from catch (Throwable recoveryFailure). Under an owner the recovery has no throw a test can reach: the three arms ask and return, and failSessionRestartIfATestAskedFor() throws only inside restartSession(), after its owner check. So every test which enters this catch has no owner, for example ParkedChangeGiveBackTest, UpdateOperationTest and SessionRestartBackoffTest. The mutant below survives. The gate is unchanged from the base apart from its comment, and the mutant table pins only the parked-road gate at :2826.

// in place of: if (!sessionHasAnOwner()) { runRequestedSessionRestarts(); }
runRequestedSessionRestarts();

Pin: add a hook which throws an Error out of the recovery, for example out of giveBackParkedChanges(), while the exporter holds the InitializeRequestMsg. Then call abortStalledInitializeFromRemote(0) and awaitDelivery(csn, …), as the parked case does. The mutant spends the request on the refusal, and nothing is delivered again.

---

suggestion (non-blocking): The isConnected() checks right after the in-thread replay() cannot see a restart that ran on the test's own thread.

opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/ReplayDuringImportTest.java:470, :517

A restart run on that thread goes through restartSession() inline: disableService(), the backoff, enableService(), then the broker's connect. So the session is back up before the read. The message says the check catches a session stopped under the owner, but it passes either way. The neighbouring WARN/NOTE checks and the redelivery wait kill every single mutant. A double mutant survives: the owner arm calls restartSession(true) itself and the refusal at :4304 is removed. The :517 read, which follows a NOW restart with no backoff, may also race the replication server unregistering the replica (not traced).

assertTrue(domain.isConnected(), "the session is not up after the replay under the owner");

Pin: to say that no restart ran under the owner, assert that a restart count or the broker's session identity is unchanged across the replay() call.

---

nitpick (non-blocking): The import finally still says its clear is for a restart asked for "before the total update owned the session".

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

With this PR, every give-up under the import owner asks before it returns. So the requests this clear forgets are mostly ones made while the import owned the session, and the new sessionHasAnOwner() javadoc ("the import at its end forget[s] the request") relies on that.

 * listed a change meanwhile: the listener thread is the one running this import,
 * and the replay threads gave up every attempt while the flag was set. The restarts
 * they asked for on their way out, while the total update owned the session, and
 * any asked for before it did, go with them, as do the deliveries folded into no
 * warning: the caller starts the session again from the reloaded state.

[vharseko]

@maximthomas thank you. Two things in this push, head ed5aad5bf0: the branch is rebased onto master at e333af0c8f, since #1049 landed over the same three roads and the PR was conflicting (d23b4fcd53), and a round commit takes four of the five points (ed5aad5bf0).

The rebase over #1049. #1049 made runRequestedSessionRestarts() leave every request standing while ieRunning(), returned the changes giveBackParkedChanges() handed back, and added NOTE_REPLAY_SESSION_RESTART_HELD_BY_TOTAL_UPDATE. They are merged like this:

• recoverFromReplayFailure(): the request is made first, then the owner arm returns (this PR), and then comes [#1048] Hold the session restart a released change asks for while a total update runs #1049's warned / held-restart note.
• giveBackParkedChanges(): it asks before the owner read and returns Collections.emptyList() under the owner, so the caller has nothing to run or report and the owner roads stay silent.

The [#1061] commit message no longer says the two gates of replay() are load-bearing (see point 3).

1. abandonReplay() asking for an unlisted change: taken. RemotePendingChanges.replayFailed() now returns whether it released the change, and abandonReplay() asks for nothing when it did not, the way recoverFromReplayFailure() already asks for nothing on a null from recordReplayFailure(). The other way to get false is a change another thread took over, and that needs no restart either, because that thread replays it or gives it back. Pinned by the new aChangeAbandonedOnceTheImportForgotItAsksForNoRestart: the test thread owns a delivery, an import runs to its end, and then the stopping replay abandons the change. No restart runs within two checkpointer ticks, which failNextSessionRestarts(1) counts. The mutant which ignores the return value is red on it: "expected [1] but found [0]". RemotePendingChangesTest pins the return value itself: true for the owner, false for another thread, a second release, a forgotten change and a committed one.

2. The abandon case does not pin "not counted": taken. assertTrue(domain.markInProgress(delivered)), and changes-with-failed-replay is 0 after the replay. The mutant recordReplayFailure(csn, now) inside abandonReplay() is red: "counted against its budget … expected [0] but found [1]". Your second mutant, the stopping change routed to recoverFromReplayFailure(), goes through recordReplayFailure() too, so the same line should catch it. That one is reasoned, not measured.

3. The last-resort gate: not pinned, because after #1049 the mutant is equivalent. importInProgress() implies ieRunning(), since both read the same context, so under a total update into this replica runRequestedSessionRestarts() leaves the request standing itself, whether or not the gate is there. The other owners, the domain disabled or shutting down, would refuse the restart and spend the request, but they also forget the pending changes and every request with them (enable(), and a server which is going down). So no test can tell the gate from its removal, and the same now holds for the parked road's gate that the description's mutant table pinned. The PR body predicted this ("whichever lands second re-checks"). The gates stay as a cheap early exit, and the commit message and the description no longer call them load-bearing.

4. isConnected() cannot see a restart run inline: taken. The three owner cases now set failNextSessionRestarts(1) before the replay and assert getSessionRestartFailuresLeft() == 1 after it, in place of the isConnected() reads. The hook sits past restartSession()'s owner check, so any restart that gets through counts. Your double mutant (the owner arm of abandonReplay() calls restartSession(false), and the refusal is removed) is red on the abandon case: the hook throws out of the replay ("could not be started again, as a test asked").

5. The comment on the import's clear: taken, in your words, plus one sentence: a replay thread that reaches its give-up after this clear finds its change unlisted and asks for nothing.

Run on the round's head, one JVM per class: ReplayDuringImportTest 13/13, RemotePendingChangesTest 25/25, ParkedChangeGiveBackTest 4/4, ReplayDuringExportTest 2/2, DisabledDomainServerStateTest 2/2, SessionRestartBackoffTest 3/3, UpdateOperationTest 41/41, with Skipped: 0. The rebased [#1061] commit alone also passed ConfigChangeDuringImportTest 4/4 and SessionRestartRequestsTest 7/7.

[maximthomas]

praise: The round commit closes the window from round 1 with a return type and no new state.

• RemotePendingChanges.replayFailed() now returns whether it released the change. abandonReplay() (LDAPReplicationDomain.java:4360) asks for nothing when it did not, and RemotePendingChangesTest pins every arm of that result.
• The owner cases count restarts with failNextSessionRestarts(1) / getSessionRestartFailuresLeft(). This sees a restart run inline, which isConnected() could not.
• The rebase over #1049 keeps the owner roads quiet: giveBackParkedChanges() returns Collections.emptyList() under an owner, so the caller has nothing to run or report.