[#986] Give back the changes a replay thread the pool stopped had parked

[vharseko]

A change which waits for another one is parked - RemotePendingChanges.checkDependencies() puts it in dependentChanges - and stays owned by the replay thread which parked it while that thread goes back to the pool and takes the changes which follow. getNextUpdate() is what hands it out again, to whichever replay thread clears the change it was waiting for, and that thread takes it over: the ownership of a parked change is deliberately held by a thread which is not replaying it right now.

That holds for as long as the parking thread comes back to the pool. It does not hold when the pool is stopped. MultimasterReplication.applyConfigurationChange() calls stopReplayThreads(), which sets the shutdown flag on every ReplayThread and joins them, and then createReplayThreads() makes a new pool. A thread which parked a change and came back to the queue is idle when it is stopped, and it ended still recorded as the owner of that change - a thread which does not exist anymore - while putRemoteUpdate() refuses every redelivery of a change a replay thread owns. On a domain which then goes quiet, that change is where this replica's ServerState, and every change behind it from every master, stops.

The trigger is wider than the number of replay threads: applyConfigurationChange() is the change listener of the whole cn=Multimaster Synchronization entry and stops the pool before it looks at what changed, so a modification of ds-cfg-connection-timeout does it too.

Where the give-back of #954 does not reach

It lives in the catch of replay(), and neither road out of a stopped pool throws:

• the idle thread is not inside replay() at all - it parked a change, met a getNextUpdate() which returned nothing, and returned to the queue;
• the thread which is inside one meets replayThreadShutdown at the top of the replay loop and leaves by abandonReplay(), which returns from replay() normally: what it was replaying is handed back, what it parked is not.

The thread gives back what it parked

ReplayThread.run() gives back, in a finally, the changes this thread parked - in every domain of this server, through LDAPReplicationDomain.giveBackChangesParkedByStoppingThread(). Done by the thread which owns them rather than by whoever stopped it, so the rule every road which reads ownership follows holds here as well: a change is given back by the thread it was handed to and by nobody else (#922). Doing it there is also what sees them all - the pool is shared by every domain of this server, and a thread may have parked a change in any of them, while a replay() knows only the domain it was replaying for. It covers the abandoned-replay road at the same time, so that road needs no give-back of its own.

Every domain gets its turn whatever one of them threw. What can throw on that road is an allocation, on the way out of a thread an OutOfMemoryError may be ending - the list of what a domain released, made before anything is released, or the report of a change once it is - and a throw at one domain would otherwise leave the ones after it with changes owned by a thread which does not exist anymore, the state this give-back is for. The first failure is thrown once the loop is over, for the uncaught exception handler of DirectoryThread to write the line and raise the alert; no message is added for it. A domain records that it is waiting for the restart next to the request itself, before its changes are reported, so a report which can not be formatted does not leave a request standing with nobody told to run it.

The loop of run() moves into replayUntilStopped() so that the finally does not re-indent it.

The restart is run for those changes

A change which nobody owns is one only a new delivery brings back, so the session has to be restarted for it. It is run by the thread which stopped the pool, from applyConfigurationChange(), once the new pool is up - a change delivered again while the pool is empty would wait in the replay queue rather than be replayed - and last in that method, so the sessions which start run on the configuration this change carries. A thread on its way out is not held for a session either.

It is run only when a stopping thread actually gave something back - restartSessionForChangesGivenBackByStoppedThreads() acts on a flag of its own. The give-back asks for the restart without the backoff, SessionRestart.NOW - since #981 the wait belongs to the request rather than to the thread which runs it: what went away is a replay thread, not the backend, and these changes were never applied here. The request it runs is the one every road shares, so a restart which a failed replay asked for in the meantime - a change the new pool took off the replay queue and could not apply before the configuration thread got there - is run there as well, once, and with the wait that request was made with: SessionRestartRequests keeps the one which asks for more, so no request is lost, none is answered by less than it asked for, and one restart serves both. What is not run from there is a restart nobody but a failed replay asked for: that one is left to the replay thread which asked for it, which sits through the backoff this domain has reached on the change it can not apply (#889), or to the state checkpointer.

A thread which an OutOfMemoryError is ending was stopped by nobody. The change it was replaying, and what it had parked in that same domain, are given back and asked for again on its way out of replay() - the road of #985 - while what it gives back here, in the domains it was not replaying for, is asked for and left standing: the state checkpointer of #981 runs it within its next tick, or whichever road asks for a restart first does. disable() clears the flag along with the request: the changes are gone with the pending list, and enable() starts the session which would have been restarted.

The changes handed back stay listed and uncommitted, and stay among the changes the newer ones are checked against, the way a change whose replay failed does. They are handed back without a failure counted against them - they were never applied here - as on the road #954 takes. Message 318 is generalised: it is now the report of a parked change whose replay thread went away, whichever way it went.

Tests

UpdateOperationTest.aChangeParkedByAThreadThePoolStoppedIsDeliveredAgain: one replay thread, a change whose replay fails is the barrier, the change which follows it on the same entry is parked behind it by that thread, a change on another entry is applied - so that replayed-updates, the count of the deliveries the session took off, is above zero for a reason other than the give-back - and ds-cfg-num-update-replay-threads is then changed: the pool is stopped and created again, the way an administrator has it. It watches dependent-changes-size go back to zero, which is the give-back, and replayed-updates go back to zero, which is the restart: a session which is started counts from zero, and nothing is delivered over the one which is started, since the changes of this test never travelled a session. Then it has the changes delivered again by hand - nothing else sends them - and checks that the change which was parked is applied.

Measured on this head, one JVM per run: with the loop over restartSessionForChangesGivenBackByStoppedThreads() deleted from applyConfigurationChange() the case is red on the second reading, the session must be restarted for the changes which were given back expected [0] but found [2]; with giveBackParkedChanges() deleted from the finally of ReplayThread.run() it is red on the first, a change parked by a replay thread the pool stopped must be given back expected [0] but found [1].

Run locally on this head, one JVM per class: UpdateOperationTest 33/33, RemotePendingChangesTest 24/24, AssuredReplicationPluginTest 14/14, IsServerFailureTest 23/23, ModifyConflictTest 36/36, NamingConflictTest 21/21, PendingChangesTest 9/9, DependencyTest 3/3, StateMachineTest 5/5 - 168 tests, Skipped: 0.

On master with #981

#981 is merged (c6b483127f), and the stack is rebuilt on it: the [#954] commit as #985 has it on that master (42284c541b), and the [#986] commit over it. What that rebase had to decide is what #985's had to: the flag the give-back set is sessionRestarts.request() now, so giveBackParkedChanges(SessionRestart, boolean byStoppingThread) takes what the restart is asked for as and records, next to the request, that the configuration thread is to run it; restartSessionForChangesGivenBackByStoppedThreads() runs runRequestedSessionRestarts(), which takes no wait anymore. The comment on a request left standing is #981's: the paragraph which bounded what such a request buys, and named its readers, is gone with the flag - the state checkpointer runs what stands, so the bound is not an argument anymore. disable() clears the flag next to sessionRestarts.clear().

Rebuilt on #985 as it stands after its round

#958 is in master (776339a). The branch is rebuilt on the head of #985 as it stands after its round of review - fda30af, one [#954] commit on master at 6dc8f80 - and the [#986] commit goes over it with one conflict, in prose: the javadoc of giveBackParkedChanges(), where that round added the paragraph on a domain which is going away and this branch its own on the thread which is stopping. It carries both.

What that round changed under this branch: giveBackParkedChanges() releases and does nothing more on a domain which is going away or is being imported into - no request, no count, no line - and this branch now records that a restart is to be run next to the request, so that road records nothing either.

The earlier rounds of this rebase stand. The barrier of the test is ModifyMsgWhoseOperationRefusesAControl - an operation which is built and fails before the CSN of the change is read, so the change is kept out of the ServerState and asked for again - since #973 has a modify whose entry DN does not parse stepped over rather than asked for again. The test shortens the replay-give-up-delay budget through setReplayGiveUpDelay(TEST_GIVE_UP_DELAY), in the middle of the test rather than at its head, so that the phase which parks a change behind one whose replay keeps failing runs on the default budget; the reset is in the finally, nested with the reset of the number of replay threads. The class carries two ways of changing the number of replay threads: setNumberOfReplayThreads(), which #941 added and which applies a mocked configuration so that it can be run from a thread of its own, and setNumUpdateReplayThreads(), which modifies ds-cfg-num-update-replay-threads on the configuration entry the way an administrator does. They are left as they are: what each test needs of that road is not the same.

Stacked on #985

This is written on top of #985, which is where the give-back of a parked change lives. Two commits: that PR's [#954] and the [#986] which is new here. Once #985 is merged this PR is that one commit.

Fixes #986

Ordinal

NOTE_REPLAY_PARKED_CHANGE_GIVEN_BACK is 318, which is where #985 has it: the message comes in on that PR's commit, and this branch only generalises its text. Master holds 310-317 (315-317 with #958) and 319-327 (325 with #981). The open PRs which add to replication.properties hold the rest with nothing claimed twice: 318 #985, #988 · 328 #1019 · 329 #1044 · 330 #1045 · 331 #1049.

[vharseko]

Rebased on master at 2a7bb9d and force-pushed - the branch was conflicting, it is mergeable again. Nothing of the change itself moved: the four commits are the ones which were here, and the diff against master is what it was, bar the two lines below.

The one conflict was the import block of UpdateOperationTest, where #941 added TimeoutException and AtomicReference to the class this branch adds Supplier to. All three are there now. What #946 and #960 wrote in LDAPReplicationDomain and PendingChanges is elsewhere in those files and merged on its own, and the ordinals 315 and 316 this branch takes are still claimed by nobody in replication.properties.

The one thing worth a look is that #941 landed a test of the neighbouring road in the same class: aChangeAStoppedReplayThreadHeldIsGivenBackAndDeliveredAgain covers the change a stopped replay thread was holding, and it runs through the applyConfigurationChange() this change adds a session restart to the end of. It is green on this head - the thread it stops parked nothing, so nothing asks for the restart. UpdateOperationTest 23/23, and RemotePendingChangesTest, AssuredReplicationPluginTest, IsServerFailureTest, ModifyConflictTest, NamingConflictTest, PendingChangesTest, StateMachineTest, DependencyTest in one run, 112 in all, green together.

The merge leaves the class with two ways of changing the number of replay threads - setNumberOfReplayThreads() from #941, which applies a mocked configuration so that it can be run from a thread of its own, and setNumUpdateReplayThreads() here, which modifies ds-cfg-num-update-replay-threads on the configuration entry the way an administrator does. I left both: what each test needs of that road is not the same. Say the word if you would rather have one.

The PR description is updated to match. No review comments to answer yet - this is the rebase only.

[vharseko]

Ordinal moved: NOTE_REPLAY_PARKED_CHANGE_GIVEN_BACK 316 → 318 (4d1a83a).

Nothing catches this on the way in. The additions land in different parts of the file, so git merges
every pair of them without reporting a conflict - I merged all ten onto master to check, and the
result carried 310, 311, 315 and 316 twice each. The generator does not check either: it loads the
file into a Properties and keys on name and ordinal (MessagePropertyKey.compareTo), so both
sides compile. What comes out is two unrelated messages carrying one support ID, found by whoever
reads a log rather than by CI.

The open PRs which add to the file now hold 310-325 with nothing claimed twice:

310-313 #935 · 314 #959 · 315-317 #958, #985 · 318 #985, #988 · 319-320 #945 · 321 #964 ·
322 #968 · 323-324 #977 · 325 #981

No Java moved with it: the generated constant is the key name without its ordinal, so the rename is
confined to replication.properties. #935, #958 and #985 keep what they had.

This branch is not one of the six, and its collision is the one which does not merely muddy a log.
#985 - the [#954] work under this - already carries this message at 318. The generated constant is
the key name without its ordinal, so the same name at 316 here and 318 there declares
NOTE_REPLAY_PARKED_CHANGE_GIVEN_BACK twice in ReplicationMessages once both are in, and the
build stops. Git reports nothing on it: the two lines sit in different places in the file.

ERR_ERROR_REPLAYING_CHANGE_315 is the [#922] message and stays put. The description said the
ordinals this branch takes were claimed by nobody and that message 316 is generalised; both lines
are corrected there.

[vharseko]

Rebased, and the branch is mergeable again (8c8323f). Nothing of the change itself moved.

master took 319 and 320 with #945 while this waited, at the end of replication.properties where
this branch adds its own - that is the conflict GitHub was reporting. It is merged rather than
renumbered: 315 to 318 are where the stack has them, 319 and 320 are master's.

The conflict which mattered was reported nowhere

#944 replaced LDAPReplicationDomain.getReplayGiveUpDelay() and setReplayGiveUpDelay(long) with
the configured replay-give-up-delay property, and the two tests of this stack which shorten that
budget - aChangeParkedByAnUnwoundReplayIsDeliveredAgain and
aChangeParkedByAThreadThePoolStoppedIsDeliveredAgain - were still calling the setters. Both sides
merge without a marker and the class stops compiling; a rebase which only answered what git asked
would have pushed that.

They take the road the rest of UpdateOperationTest takes now - setReplayGiveUpDelay(TEST_GIVE_UP_DELAY),
resetReplayGiveUpDelay() in the finally - which is what the two cases of #958 already do. The
budget is still shortened where the setter was called, in the middle of the test rather than at its
head: the phase before it parks a change behind one whose replay keeps failing, and it runs on the
default budget the way it did. The reset is a delete which modifyDomainConfig() treats as done
when the attribute is not there, so a test which fails before the shortening still cleans up without
replacing its own failure.

Rebuilt on the heads of the stack, not on the copies it carried

This branch was carrying the revisions of #958 and #985 as they were when it was written. It is
rebuilt on their current heads instead - #958 at 31e633d, with the rounds of review it has had
since, and the [#954] commit of #985 over it - and then rebased onto master at 21d03d5, where
it applies with no conflict at all. Six commits: four of #958, one of #985, and the one which is new
here.

Moving the [#954] commit onto the current #958 met four conflicts, all of them prose: the javadoc
of getChangeOwnedByCurrentThread() and of
theChangesParkedAsDependenciesAreNotOwnedByTheThreadWhichParkedThem(), the comment in the catch
of replay(), and the block in replication.properties. Each of them now says what both rounds
say - the lookup which allocates nothing, and the parked changes which are given back on a road of
their own.

The [#986] Take an ordinal in replication.properties nothing else claims commit is gone with the
rebase: 318 comes in on #985's commit, and this branch only generalises the text of that message.

Run on this head

UpdateOperationTest 31/31, UpdateOperationTest 31/31, and the classes around it in one run - RemotePendingChangesTest 24/24, AssuredReplicationPluginTest 14/14, IsServerFailureTest 23/23, ModifyConflictTest 36/36, NamingConflictTest 8/8, PendingChangesTest 3/3, DependencyTest 3/3 - 142 in all, green together. StateMachineTest 5/5 on its own: in that run its setUp met the administration connector of another test's server on 0.0.0.0:65534.

The description is updated to match.

[vharseko]

Rebased, and the branch is mergeable again (dd3cafe). Nothing of the change itself moved.

Rebuilt on the heads of the stack

The branch is rebuilt on the current head of #958 - five commits now, the fifth being
[#922] Pin the ownership index on the hand-out and the give-back, and assert the alert an OutOfMemoryError leaves behind - rebased onto master at 13d57e0, and on the [#954]
commit of #985 as it stands there (13be038), which is where the owner of the change is read
before the parked changes are given back. The copy of that commit this branch was carrying
was behind it. Seven commits: five of #958, one of #985, and the one which is new here.

Moving the [#954] commit onto the current #958 met one conflict, in prose: the javadoc of
changeBeingReplayed in RemotePendingChanges, where #958's fifth commit added the paragraph
on who writes an entry of that index and #954 rewrote the sentence on the parked changes.
It carries both now. The [#986] commit applied with no conflict at all.

The conflict which mattered was reported nowhere

#973 landed on master in the meantime: a modify whose entry DN does not parse is now
reported once and stepped over rather than thrown on, so its CSN goes into the ServerState
on the delivery which met it. ModifyMsgWithAnUnparseableOperationDN is that message, and
the two tests of this stack which need a change whose replay fails and stays listed as the
barrier the parked change waits behind - aChangeParkedByAnUnwoundReplayIsDeliveredAgain
and aChangeParkedByAThreadThePoolStoppedIsDeliveredAgain - were built on it. Both sides
merge without a marker and both tests lose their premise: the barrier is committed, nothing
is parked behind it, and dependent-changes-size never reaches 1.

They use ModifyMsgWhoseOperationRefusesAControl now, which #973 introduced for exactly
the case those tests want - an operation which is built and fails before the CSN of the
change is read, so the change is kept out of the ServerState and asked for again - and which
is what the rebase of #958 moved its own failing messages onto. Four lines, the barrier
delivery and the redelivery of it in each test; the rest of both tests is what it was.

Run on this head

UpdateOperationTest 33/33 - both tests of this stack among them - and the classes around it in one run: RemotePendingChangesTest 24/24, AssuredReplicationPluginTest 14/14, IsServerFailureTest 23/23, ModifyConflictTest 36/36, NamingConflictTest 8/8, PendingChangesTest 5/5, DependencyTest 3/3 - 146 in all, green together. StateMachineTest 5/5 in a run of its own.

The description is updated to match.

[maximthomas]

praise: The give-back is done where the rule of #922 can hold, and the road it is on is measured, not asserted.

• ReplayThread.run() gives back in a finally, on the thread which owns the changes, across every domain — the one place which sees them all; deleting that call turns the new case red at UpdateOperationTest.java:3506 exactly as the description says (measured, 65 s).
• The restart is placed after createReplayThreads() and outside the two static synchronized methods, and the comment at MultimasterReplication.java:775-786 says why; a redelivered change never waits in an empty pool.
• On the OOME exit of a replay thread HEAD is strictly better than master: the parked changes end up unowned instead of owned by a dead thread, so any later restart or reconnect brings them back.

---

issue (blocking): The session restart this PR adds on the configuration road is pinned by no case.

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/MultimasterReplication.java:789-792, opendj-server-legacy/src/test/java/org/opends/server/replication/UpdateOperationTest.java:3504-3533

aChangeParkedByAThreadThePoolStoppedIsDeliveredAgain watches dependent-changes-size reach 0 and then redelivers by hand (domain.processUpdate(), :3527-3533), so the flag changesGivenBackByStoppedThreads and the restartSessionForChangesGivenBackByStoppedThreads() loop are observed by nothing. Measured at this head: the case passes with the whole :789-792 loop deleted (survives 1/1, 4.19 s vs 4.26 s); the control with giveBackParkedChanges() deleted from the finally fails at :3506. Same shape as #985 [2].

A session restart takes the domain through NOT_CONNECTED, which resets the replay counters (:1634-1637), while the give-back counts each CSN it hands back (LDAPReplicationDomain.java:3789): after the pool bounce the counter is 0 only if the restart ran.

      setNumUpdateReplayThreads(2);

      assertMonitorAttrValueEventually(baseDN, "dependent-changes-size", 0,
          "a change parked by a replay thread the pool stopped must be given back");
      assertMonitorAttrValueEventually(baseDN, "replayed-updates", 0,
          "the session must be restarted for the changes which were given back");

Pin: with the :789-792 loop deleted the counter keeps the given-back CSN's increment and the second assertion goes red; the hand redelivery below it can stay.

---

issue (non-blocking): restartSessionForChangesGivenBackByStoppedThreads() gates its entry on its own flag and then drains the shared one, so the failed-replay restart is not "left to the replay thread".

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:3666-3676, :3691-3700, :3838

runRequestedSessionRestarts(false) takes sessionRestartRequested, which :3628 sets on every failed replay. The window is reachable: the static updateToReplayQueue survives stopReplayThreads(), the new pool drains it before the configuration thread reaches :789, and a new-pool thread which fails a replay there sets the flag; if the configuration thread wins the CAS at :3691, its getAndSet(false) takes both requests and restarts without waitBeforeSessionRestart() and without bumping consecutiveSessionRestarts (:3838 is under if (wait)). One skipped #889 backoff per configuration change, on a change which fails again at once; no interleaving loses a request. The javadoc at :3660-3665 and the description's "for those changes and no others" say otherwise.

Keeping the code and saying what it does is the smaller change:

   * Run without the backoff: what went away is a replay thread, not the backend. A restart
   * which a failed replay asked for in the meantime is run here as well, once, without the
   * wait that road would have taken: the flags are shared, and one restart serves both.

Or: run the give-back restart without draining sessionRestartRequested, so a pending failed-replay request keeps its backoff — a third state machine next to the two which are there.

---

suggestion (non-blocking): The loop over the domains has no per-domain isolation, unlike its sibling at :2668-2703.

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/ReplayThread.java:200-206, opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:3649-3655, :3786-3791

Only an Error reaches it (list growth at RemotePendingChanges.java:625, the NOTE 318 formatting) — but on that road a throw at domain k skips k+1..n (owned by a dead thread, the master behaviour) and leaves k with sessionRestartRequested set (:3786) and changesGivenBackByStoppedThreads unset (:3653 runs after the return), so the configuration thread skips k now and at the next change.

    for (LDAPReplicationDomain domain : MultimasterReplication.getDomains())
    {
      try
      {
        domain.giveBackChangesParkedByStoppingThread();
      }
      catch (Throwable giveBackFailure)
      {
        // Reported the way :2668-2703 reports its own; the next domain is still visited.
      }
    }

And set the flag before the per-CSN report rather than after the helper returns: the release at :3776 is the mutation, the report loop is where an Error lands.

---

suggestion (non-blocking): On the OOME exit of a replay thread the finally releases every other domain's parked changes and nothing restarts those sessions now.

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/ReplayThread.java:102-105, :183-184, :200-206

MultimasterReplication.java:789 is the only caller of restartSessionForChangesGivenBackByStoppedThreads(), so a domain released on that road waits for its next failed replay, the next configuration change (the flag stays set), or a reconnect. Better than master, but the comment says "restarted by the thread which stopped this one" and nobody stopped it.

       * The session which brings them back is restarted by the thread which stopped this
       * one, once the pool it creates is up. A thread which dies of an OutOfMemoryError
       * was stopped by nobody: the changes it releases wait for the next restart of their
       * domain - a failed replay, a configuration change, a reconnection.

---

suggestion (non-blocking): disable() clears sessionRestartRequested and not changesGivenBackByStoppedThreads.

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:4564, :4709-4715

A flag left set (the OOME road above, or a disable racing the configuration thread's loop) runs one restart with wait=false at the next configuration change, on a session which was started fresh by enable(). Harmless; one line beside :4564.

      sessionRestartRequested.set(false);
      changesGivenBackByStoppedThreads.set(false);

---

suggestion (non-blocking): The two resets in the finally of the new case are not nested.

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

A throw out of resetReplayGiveUpDelay() leaves the pool at 2 threads for every class which follows. Same shape as #985 M4.

    finally
    {
      try
      {
        resetReplayGiveUpDelay();
      }
      finally
      {
        resetNumUpdateReplayThreads();
      }
    }

---

nitpick (non-blocking): The comment on runRequestedSessionRestarts() names two readers; the configuration thread is a third.

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

"Its two readers are the roads out of a failed and of an abandoned replay" and "ends the replay thread it is met on … the uncaught exception handler of DirectoryThread" are false for the reader at :3674: a throw out of restartSession() there ends the modify with an error to the LDAP client (ConfigurationHandler.java:653 has no catch). The request itself survives — the finally at :3703-3729 puts it back.

[vharseko]

Rebuilt on #985 as it stands after its round - fda30af, on master at 6dc8f80 - and then the round; the branch is that PR's [#954] commit and the [#986] one (bef15b3). Moving the commit over met one conflict, in the javadoc of giveBackParkedChanges(), which carries both paragraphs now.

The restart is pinned

The case reads replayed-updates after the pool bounce and waits for 0: the give-back counts what it hands back, a session which is started counts from zero, and nothing is delivered over the one which is started - the changes of this case never travelled a session - so the count is back at zero only if the restart ran. The hand redelivery below it stays.

One thing added to what was proposed: the case applies a change on another entry before the bounce and asserts the count is above zero then. Without it the count before the bounce is whatever the refused duplicates of deliverUntilMonitorReaches() left, which can be 0, and the pin would then ride on the give-back's own incProcessedUpdates() alone - take that count away and 0 before, 0 after, the mutant is green. With it the reading says what it is meant to say: the restart zeroed a count which was not zero.

Measured, one JVM per run: the case with the restartSessionForChangesGivenBackByStoppedThreads() loop deleted from applyConfigurationChange() is red on that reading, the session must be restarted for the changes which were given back expected [0] but found [2]; with giveBackParkedChanges() deleted from the finally of ReplayThread.run() it is red on the reading before it, expected [0] but found [1], as before.

The shared flag

The code stays and the text says what it does. restartSessionForChangesGivenBackByStoppedThreads() is gated on the flag of its own and runs whatever is requested, without the backoff: a restart a failed replay asked for between the new pool draining the queue and the configuration thread getting there is run once, without the wait that road would have taken, and no request is lost. What the javadoc and the description now claim is only what is not run from there: a restart nobody but a failed replay asked for. The third state machine would not have bought that either - the give-back sets the shared flag itself, so a restart which left it standing would be followed by a second one with the backoff.

The loop over the domains

Every domain gets its turn: the give-back of each is under a catch (Throwable) of its own, the first failure is kept - the same instance is not suppressed into itself, an out-of-memory JVM hands the one it prepared out more than once - and it is thrown once the loop is over, for the uncaught exception handler of DirectoryThread to write the line and raise the alert. No message is added for it; on the road where it throws the JVM is out of memory, and the handler reports what it is given.

The flag is set next to sessionRestartRequested, before the report loop, rather than after the helper returns: giveBackParkedChanges(boolean byStoppingThread) now, with the road passed in. That also settles what the round of #985 left for this branch: on a domain which is going away or is being imported into that helper releases and returns true without asking for anything, and the old if (giveBackParkedChanges()) would have recorded a restart to run for a domain which asks for none. Now it records it where the request is made and nowhere else.

The OOME exit, disable(), the finally, the readers

The comment in ReplayThread.run() says that a thread an OutOfMemoryError is ending was stopped by nobody, and where the changes it releases wait; the field's javadoc and the description say the same. disable() clears changesGivenBackByStoppedThreads with sessionRestartRequested. The two resets of the case are nested. The comment on the request left standing names the third reader and what a throw does there: it fails the modification of the configuration entry, and the request stands for the next failed replay or configuration change.

Runs

Run locally on this head, one JVM per class: UpdateOperationTest 33/33, RemotePendingChangesTest 24/24, AssuredReplicationPluginTest 14/14, IsServerFailureTest 23/23, ModifyConflictTest 36/36, NamingConflictTest 21/21, PendingChangesTest 9/9, DependencyTest 3/3, StateMachineTest 5/5 - 168 tests, Skipped: 0.

The description is updated to match: the stack, the ordinals - #964, #968 and #977 are in master - and the sections on the restart and the tests.

[vharseko]

Rebuilt on master at c6b483127f - #981 is in it - and on #985 as it stands there (42284c541b): the branch is that [#954] commit and the [#986] one (d9875ededf). Moving the [#986] commit over met five conflicts in LDAPReplicationDomain, every one of them where this branch touched the flag #981 replaced. What the rebase decided, on top of what #985's did (see there):

The flag of this branch sits next to the request

giveBackParkedChanges(SessionRestart restart, boolean byStoppingThread): the road passed in as before, and what the restart is asked for as with it. giveBackChangesParkedByStoppingThread() asks for NOW - what went away is a replay thread, not the backend - and records that the configuration thread is to run it, next to the request and before the report, as before.

The restart the configuration thread runs carries the wait of what it takes

restartSessionForChangesGivenBackByStoppedThreads() runs runRequestedSessionRestarts(), which takes no argument anymore: since #981 the wait belongs to the request. So what your non-blocking item on the shared flag described is what happens now: a restart a failed replay asked for between the new pool draining the queue and the configuration thread getting there is run there, once, and with the backoff that request was made with - SessionRestartRequests keeps the one which asks for more. What is still not run from there is a restart nobody but a failed replay asked for. The javadoc and the description say so, where they said "without the wait that road would have taken".

What #981 took away

The paragraph on what a request left standing buys, and its readers - the one your nitpick was on - is gone with the flag: the checkpointer runs what stands, so the bound is not an argument anymore, and the comment there is #981's. The field's javadoc and the description say that the changes an OutOfMemoryError road releases in the other domains are asked for and left to the checkpointer, rather than to "the next restart of their domain". disable() clears the flag next to sessionRestarts.clear().

Runs

UpdateOperationTest 35/35 - the cases of #985, of this branch and of #981 among them - and SessionRestartBackoffTest 3/3, in one pass, Skipped: 0. The mutants are not re-measured on this head.

The description is updated to match: the section on the restart, the OOME exit, the ordinals - 325 is master's - and a section on this rebase.

[maximthomas]

praise: The restart on the configuration road is now pinned by what only a restart produces, and the round-1 loop is isolated the way it was asked.

• replayed-updates > 0 before the bounce, == 0 after it: with the MultimasterReplication.java:789-792 loop deleted the case goes red at UpdateOperationTest.java:3579 — "expected [0] but found [2]", 1/1 here as well.
• ReplayThread.java:205-247 — per-domain catch (Throwable), first failure kept, self-suppression guarded, rethrown after the loop.
• changesGivenBackByStoppedThreads is set inside giveBackParkedChanges(boolean byStoppingThread) next to the request, so the "going away" road records nothing; disable() clears it; the fixture resets are nested.
• The javadoc of restartSessionForChangesGivenBackByStoppedThreads() now says what the code does when a failed replay's request is pending.

---

issue (non-blocking): The restart loop in applyConfigurationChange() has no per-domain isolation, unlike the give-back loop this round added.

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/MultimasterReplication.java:789-792

A Throwable at domain k skips k+1..n (their flags stand until the next bounce) and escapes replaceEntry after the entry, config.ldif and the bounce are done — the client gets a server error for a modify which was applied. The only Throwable off restartSession(false) on this thread is an Error (every checked and runtime failure in disableService()/enableService()/ReplicationBroker.start()/stop() is caught), so this is the same road ReplayThread.java:205-247 now isolates.

Throwable failure = null;
for (LDAPReplicationDomain domain : domains.values())
{
  try
  {
    domain.restartSessionForChangesGivenBackByStoppedThreads();
  }
  catch (Throwable restartFailure)
  {
    if (failure == null)
    {
      failure = restartFailure;
    }
    else if (failure != restartFailure)
    {
      failure.addSuppressed(restartFailure);
    }
  }
}
if (failure instanceof Error)
{
  throw (Error) failure;
}
if (failure instanceof RuntimeException)
{
  throw (RuntimeException) failure;
}

Or: one helper taking the per-domain action, called by both loops.

---

issue (non-blocking): The give-back flag is consumed before the restart runs, and nothing puts it back when the restart throws.

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:3847, :3904-3906

compareAndSet(true, false) precedes runRequestedSessionRestarts(false); the finally at :3908 restores sessionRestartRequested only, and the flag has no other true-setter than :3988. For that domain the new comment "…or the next configuration change" is false: the next bounce reaches :3847 with the flag down and runs nothing; what is left is a failed replay in that domain (with its backoff), disable()/enable(), or a later bounce parking there again. Error road only, same as above.

if (changesGivenBackByStoppedThreads.compareAndSet(true, false))
{
  boolean ran = false;
  try
  {
    runRequestedSessionRestarts(false);
    ran = true;
  }
  finally
  {
    if (!ran)
    {
      changesGivenBackByStoppedThreads.set(true);
    }
  }
}

Or: make the comment at :3904-3906 say the next configuration change runs it only for the domains the loop did not reach.

---

suggestion (non-blocking): The pin sees that a restart ran, not that it ran without the backoff.

opendj-server-legacy/src/test/java/org/opends/server/replication/UpdateOperationTest.java:3579-3580, LDAPReplicationDomain.java:3849

runRequestedSessionRestarts(false) → runRequestedSessionRestarts(true) survives 1/1: the case takes 6.19 s instead of 4.23 s (the backoff, taken and unobserved) and stays green. Nothing in the case reads anything wait changes, so "without the backoff: what went away is a replay thread, not the backend" is enforced by no test.

Pin: assert what only wait=false leaves between :3572 and :3580 — consecutiveSessionRestarts unchanged (a package-private getter) or no #889 backoff line in logs/errors. A bound on the case time is not a pin.

---

suggestion (non-blocking): enable() clears neither flag, so a give-back which races disable() runs one no-backoff restart after the domain comes back.

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:3970, :4830-4874, :4989-5038

The gate shutdown.get() || disabled is read with no lock; disable() clears both flags under serviceStateLock; a stopping thread past the gate sets request and flag after that clear, remotePendingChanges.clear() at :4869 has already forgotten what it gave back, and the next bounce after enable() restarts a session for nothing. Harmless, two lines.

// enable(), next to disabled = false
sessionRestartRequested.set(false);
changesGivenBackByStoppedThreads.set(false);

---

nitpick (if-minor): "Run here as well, once, without the wait that road would have taken" holds only when this thread takes the recovery first.

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:3835-3843, :3866, :4053

When the failed replay's thread already holds replayFailureRecovery and sleeps in waitBeforeSessionRestart() outside serviceStateLock, the :3866 CAS fails, this method returns without running anything, that thread's enableService() resends the given-back changes, and its inner re-read at :3870 then runs a second restartSession(true) for the give-back request. Nothing is lost; the changes wait for that backoff and one redundant bounce follows.

 * A restart which a failed replay asked for in the meantime is run here as well, once,
 * without the wait, when this thread takes the recovery first. When that replay's restart
 * is already under way, these changes ride on it: its resend carries them, and the
 * request it finds afterwards costs one more restart, with the backoff.

---

nitpick (if-minor): The comment names the list and the report as the only allocations on the OOME road; the iterator over the domains comes first.

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/ReplayThread.java:207, :216-222

for (… : MultimasterReplication.getDomains()) obtains an iterator over the live domains.values() before the first per-domain try; an allocation refused there releases nothing in any other domain and sets no flag. Multi-domain only — the domain of the failed replay is released at LDAPReplicationDomain.java:2709 before the rethrow. The allocation has no cheaper form; name it.