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

[vharseko]

Fixes #1048.

On master, over #981. #981 is merged (c6b483127f), and the branch is the one commit [#1048] on origin/master, 34cab95, 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. The copy of #981 this branch carried (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 gate this change adds.

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. The import direction is unchanged in effect: on it the request is not made at all (sessionHasAnOwner() returns before it), except by the last resort, whose request now stands until importBackend() clears it where before restartSession() consumed it under the 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. Not built on the road out of a JVM which has run out of memory, for the reason the warning is not built there either. 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 last resort of replay() is unchanged in code: 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.

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 disableService() is still cut - the same window #1041 describes on the import side, a few statements wide where the whole export was. #1045 (#1041) turns the owner read of restartSession() into a claim on importExportContext, and keeps "a claim which fails against an export stops the session, as before" for this issue; once both are in, that claim failing against an export can give the request back instead of stopping, which closes the window for good. Filed against whichever lands second rather than stacked on two open PRs. restartService() during an export - a configuration change accepted before the export began - is #1040's shape (#1042), and isConfigurationChangeAcceptable() already refuses one while ieRunning().

Two 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.

Tests

ReplayDuringExportTest.aReplayWhichFailsDuringTheExportLeavesTheSessionToTheExport (new)

green, ~5 s. 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. 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 change, and the change is delivered again over the session the checkpointer restarts once the export is over, applied, and covered by the ServerState

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. 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 new test:

mutant	result
the ieRunning() gate of runRequestedSessionRestarts() removed (the fix reverted, the message kept)	red: Starting total update: exporting 10 entries, the retry warning, connection to Replication Server ... is lost from the export, and the rest of the entries never arrive
the checkpointer's call to runPendingSessionRestart() removed	red: the export streams to its end, and the change is not delivered again within 30 s of it

The class was run on the unfixed tree first, before the fix was written, and failed the same way as the first mutant.

Run on this head, one JVM per class, all green: ReplayDuringExportTest 1/1 (three runs), ReplayDuringImportTest 3/3, NamingConflictTest 18/18, SessionRestartBackoffTest 2/2, SessionRestartTest 2/2, DisabledDomainServerStateTest 2/2, LDAPReplicationDomainConfigChangeTest 9/9, SessionRestartRequestsTest 7/7, ServerStateFlushTest 4/4, UpdateOperationTest 32/32, RemotePendingChangesTest 21/21, AssuredReplicationPluginTest 14/14, DependencyTest 3/3, InitOnLineTest 10/10, GenerationIdTest 4/4, ReSyncTest 2/2 - the classes which restart a session or drive a total update, 134 tests, nothing skipped. On the head over master: UpdateOperationTest 33/33, ReplayDuringExportTest 1/1, ReplayDuringImportTest 4/4, SessionRestartBackoffTest 3/3 in one pass, Skipped: 0.

Ordinal

NOTE_REPLAY_SESSION_RESTART_HELD_BY_TOTAL_UPDATE_331: master holds up to 327 (325 with #981); of the open PRs which add to replication.properties, #1019 holds 328, #1044 329 and #1045 330 - checked on every open PR head, not on the descriptions. 331 is claimed by nothing.

[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.