Disabling a domain can persist an empty ds-sync-state and lose the replica's position

Symptom

Disabling a replication domain can end with an empty ds-sync-state on the base entry, i.e. the replica loses its replication position. On the next enable() (or on a restart) the domain resumes from nothing: loadState() finds no CSN, and checkAndUpdateServerState() cannot repair it either, because it returns immediately when state.getCSN(serverId) is null. Everything the replication server still holds is then sent again and replayed a second time.

Found while working on #916; it is a separate defect on the disable() path, present before and after the fix for that issue.

The mechanism

disable() empties the in-memory state without keeping the checkpointer away from it:

// LDAPReplicationDomain.java:3867-3873
public void disable()
{
  synchronized (serviceStateLock)
  {
    state.save();
    state.clearInMemory();
    disabled = true;
    ...

clearInMemory() empties the map and marks the state as not saved — which is what PersistentServerState.clear() wants, since it exists to write the empty state out, but not what disable() wants. serviceStateLock is not held by the checkpointer, so between the three statements above the ServerStateFlush thread can enter save(), find the state dirty, snapshot the now-empty map and REPLACE ds-sync-state with an empty attribute.

Two interleavings reach it:

the flush thread evaluates !disabled && !ieRunning() (:564) as true just before disabled = true is set, then takes its snapshot after clearInMemory() has run;
the flush thread's exit path (:576) calls state.save() unconditionally, without consulting disabled or ieRunning(), so a shutdown following a disable() writes the emptied state whatever the flags say.

The window in the first case is a few instructions wide; the second one is not a race at all — a disable() that is not followed by an enable() before shutdown persists emptiness every time.

Impact

The safe direction (changes are replayed rather than skipped), but the whole point of the persisted state is defeated: after a disable/enable cycle or a disable followed by a restart, the replica re-requests everything the replication server still holds. On a busy domain that is a large replay; the changes are applied again as duplicates, which the historical information resolves, but the work and the window of divergence are real.

Suggested direction

The in-memory copy and the flag that says "this must be written" are cleared together by clearInMemory(), which is right for clear() and wrong for disable(). Options, roughly in order of how localized they are:

have disable() set disabled = true before dropping the state, and make the flush thread's exit save() respect it — the exit save exists to flush a last checkpoint, which is pointless once the state has been dropped on purpose;
or give PersistentServerState a "drop the in-memory copy, the backend keeps what it holds" operation distinct from clearInMemory(), which leaves the state marked as saved;
or hold the save lock across the drop, so no snapshot can be taken between emptying the map and marking the domain disabled.

A regression test can drive the second interleaving directly: disable a domain, then let the flush thread run its exit path, and assert that the base entry still carries the CSNs the domain had.

Walked the code for this. Every mechanical claim holds; three things are missing from the description, one of the two interleavings is reachable far more easily than it says, and the Impact section is more generous than the code warrants.

Confirmed

disable() is state.save(); state.clearInMemory(); disabled = true; (LDAPReplicationDomain.java:3868-3873), and clearInMemory() is state.clear(); state.setSaved(false); (PersistentServerState.java:281-285).
serviceStateLock buys nothing here: the checkpointer synchronises on itself — synchronized (this) at :561, the ServerStateFlush instance, which is also the monitor shutdown() notifies on at :2371. The two threads share no lock at all.
The exit save at :576 is unconditional.
checkAndUpdateServerState() returns on state.getCSN(serverId) == null (PersistentServerState.java:314-318) and cannot repair. Worth adding that even without that guard it only ever repairs the local serverId: nothing local records how far the replay of a peer got, so the peers' entries in the state are unrecoverable by construction.
The write is a REPLACE built from state.toASN1ArrayList() (:255), which on an empty state is a zero-value list — ds-sync-state is not written empty, it is removed from the base entry.

The justification for `clearInMemory()`'s semantics does not hold

The description defends setSaved(false) as "what PersistentServerState.clear() wants, since it exists to write the empty state out". clear() has no production caller — the only one in the tree is PersistentServerStateTest.java:86. Both production callers of clearInMemory(), disable() and loadDataState() (:3905), want the opposite.

So this is not "give PersistentServerState a drop operation distinct from clearInMemory()". It is that clearInMemory() has had the wrong semantics all along, and the test-only clear() should clear the flag itself.

Scope, and a third window

disable() has exactly two production callers, both in MultimasterReplication (:638, :664), reached only from ImportTask (:628) and RestoreTask (:263) via DirectoryServer.notifyImportBeginning / notifyRestoreBeginning. The online total update does not go through them. Naming that makes the second interleaving concrete.

It also makes it much easier to reach than "a shutdown following a disable()": both tasks fire notify*Beginning and then, if TaskUtils.disableBackend() fails, return TaskState.STOPPED_BY_ERROR from before the try/finally that fires notify*Ended. The domain is then disabled for the life of the server, holding an empty dirty state, and any shutdown after that writes it out. Filed as #966 — it is a defect in its own right, since replication for that base DN stays silently off.

restore --verifyOnly goes through the same path: notifyRestoreBeginning fires before the verifyOnly branch, so a dry-run verify on a replicated backend disables and re-enables the domain with the data untouched. The only thing at risk there is the position.

And there is a third clear-then-load window the description does not list: loadDataState() (:3903-3907) does the same clearInMemory() then loadState(), and in the total-update path it runs at :4359 with disabled == false. ieRunning() is true across importBackend (ReplicationDomain.java:2522-2551), so the periodic save is gated there — but the exit save at :576 still is not, and the gap is not a few instructions: loadState() runs a base search plus checkAndUpdateServerState()'s historical search.

The impact is worse than "the safe direction"

loadDataState() follows the load with getGenerator().adjust(state.getMaxCSN(getServerId())). On an empty state that argument is null, and CSNGenerator.adjust(null) (CSNGenerator.java:120-130) sets lastTime = TimeThread.getTime(); seqnum = 0 — the only path in the generator that moves lastTime backwards. adjust(CSN) never lowers it (if (lastTime <= rcvdTime) lastTime = ++rcvdTime, :143-145).

So the re-enabled domain can issue CSNs it has already used, by exactly the margin that peers with a faster clock had pushed lastTime past the local clock — which is what adjust exists to do. A change carrying a CSN older than the historical value the peers already hold for that serverId loses conflict resolution and is dropped. That is divergence, not re-replay. It needs clock skew (or two changes inside one millisecond) to bite, but the emptied state is what arms it.

Suggested direction, refined

The second option in the description, but simpler than it is framed: make clearInMemory() leave the state marked as saved — it drops the in-memory copy, the backend still holds the truth — and let the test-only clear() mark it unsaved before saving. That covers disable(), the enable() gap and the total-update gap at :4359 in one place, with no reordering in disable().

Then, belt and braces, give the exit save at :576 the same guard as the loop body. Redundant for this bug once the flag is right, but it is what stops a future clear-then-something from leaking through shutdown.

Interaction with #948

#948 makes ServerState.clear() set saved = false itself, so the fix above becomes state.clear(); state.setSaved(true); inside clearInMemory() — which reads oddly against that new contract and will want a comment saying why.

Its saveLock does not close this race either. It adds an interleaving: the checkpointer passes the !disabled check, enters save(), finds the state dirty, and parks on the lock for the whole duration of the backend write that disable() is doing — then acquires it, re-checks, and finds the state emptied and dirty.

Two additions from walking the task side for #966, both about this issue rather than that one.

The CSN generator regression does not need the race at all

Above I described adjust(null) moving lastTime backwards as the consequence of re-enabling a domain whose state was emptied by an interleaving. It is reached without any interleaving, on every failed import or restore, because the tasks fire the end notification twice and the first one cannot work.

ImportTask.java:673/:688 call notifyImportEnded(..., false) and then return through the finally, which calls notifyImportEnded(..., true) at :732; RestoreTask.java:297/:303 and :338 are the same shape. One processImportBegin, two processImportEnd, two enable()s.

The first enable() runs before the backend is put back — that happens only at ImportTask.java:719:

loadState()'s base search returns NO_SUCH_OBJECT for a deregistered backend (PersistentServerState.java:152-166), so the reload finds nothing and the state stays empty;
getGenerator().adjust(state.getMaxCSN(getServerId())) (LDAPReplicationDomain.java:3907) is therefore adjust(null), and lastTime drops to the local clock with seqnum = 0;
loadGenerationId() then finds nothing either and falls into computeGenerationId() → exportBackend(), whose getBackend() is null while the backend is deregistered, so the exception lands in enable()'s catch (:3931-3937) and the domain stays disabled == true.

The catch does not undo the generator adjustment. The second notification, after the backend is back, is what actually re-enables the domain; its adjust() lifts lastTime again only if the state it reloads is non-empty — and after a failed import the backend may well have been cleared already.

So the emptied in-memory state is not the only way into the CSN reuse hazard: a disabled backend supplies the same emptiness to loadDataState(), deterministically, every time a task fails after its backend was disabled. The clearInMemory() fix does not touch this one; firing the end notification exactly once, from a point where the backend is up, is the task-side half. Filed under #966.

What an empty state costs is more than re-replay

I noted that checkAndUpdateServerState() returns on state.getCSN(serverId) == null and can only ever repair the local entry. The other half of that mechanism is worth naming: the local entry it repairs is what feeds the missing-change publisher. sessionInitiated() starts an RSUpdater only when state.getMaxCSN(getServerId()) is newer than the RS's CSN for this replica (LDAPReplicationDomain.java:4767-4788).

Both guards fail on an empty state, and they are the pair that recovers changes the replication server never received — the local writes a replica took while it was disconnected or disabled. Those are not re-replayed by anyone; they are simply not published. So the impact of a persisted empty ds-sync-state is not only "everything the replication server still holds is sent again", which is the safe direction, but also the silent loss of the changes it does not hold, which is not.

Both are covered by making clearInMemory() leave the state marked as saved — the local CSN survives the drop, so the reload has something to repair from.

A third road to the same symptom, found while reviewing #945 (which is for #908, not for this issue).
Unlike the two interleavings in the description, this one is not a race: it fires every time.

A second `disable()` saves the state the first one cleared

disable() has no idempotence guard, and an online import-ldif on a replicated backend reaches it
twice:

ImportTask.java:628   DirectoryServer.notifyImportBeginning(backend, importConfig)
                        -> MultimasterReplication.java:671   domain.disable()            // #1
ImportTask.java:633   TaskUtils.disableBackend(backend.getBackendID())
                        -> BackendConfigManager
                        -> LDAPReplicationDomain.java:279 performBackendPreFinalizationProcessing
                        -> domain.disable()                                              // #2

LDAPReplicationDomain registers itself as a local backend initialization listener at :716, and the
guard at :279-285 is !ignoreBackendInitializationEvent && !serverShutdownRequested && backendID matches. ignoreBackendInitializationEvent is set in preBackendImport(), which is the total
update road, not this one, and serverShutdownRequested is false. Nothing stops the second call.

disable() #1 runs state.save() — which writes the real state — and then state.clearInMemory()
(:3889-3890), which empties the map and does setSaved(false). Nothing repopulates it in between:
the only reload is loadDataState() from enable(), and that is processImportEnd, after the import.
So disable() #2 runs state.save() on the cleared state — !isSaved() is true, updateStateEntry()
serialises an empty ServerState, and ds-sync-state is REPLACEd with no values.

Where it becomes visible is the road where the import then does not replace the data:
ImportTask:650 fails to take the exclusive lock -> :688 notifyImportEnded(..., false) ->
:719 TaskUtils.enableBackend -> enable() -> loadDataState() reads an empty ServerState over
intact data.

RestoreTask reaches disable() only once — it has no notifyRestoreBeginning counterpart on that
road — so this one is specific to import-ldif. It is the same lines as #966: that issue is about the
domain never being enabled again, this one about the state being erased on the way in.

An idempotence guard on disable() — an early return inside serviceStateLock when disabled is
already true — closes this road on its own and costs nothing on the other two.

What PR #945 does and does not close here

#945 is for #908 and does not claim this issue, but it touches two of the three roads:

it reorders disable() to disabled = true; disableService(); sessionGeneration++; awaitReplayDrained(); state.save(); state.clearInMemory();, i.e. the flag is set before the state is
dropped — the first bullet of "Suggested direction" above;
it guards the exit save at :576 with if (!disabled && !importInProgress()), which closes the
second interleaving deterministically.

It does not close the first interleaving, only narrows it onto the exit save. The guard is two plain
reads outside the flush thread's synchronized (this), and disable() holds serviceStateLock, which
the flush thread never takes, so volatile on disabled buys visibility and not atomicity. The flush
thread can pass both reads, be descheduled, and have a whole disable() run before its state.save()
builds the modify — the exposed span runs to PersistentServerState:231 runUpdateStateEntry reading
the values. shutdown() does not join the flush thread first: it waits on done after the final
save, and flushThread.initiateShutdown() precedes deregisterLocalBackendInitializationListener, so a
concurrent import-ldif / restore / dsconfig backend-disable can run that whole disable() inside
the gap.

And it does not close the double-disable() road above, which does not involve the flush thread at all.

[OpenIdentityPlatform#951] Test that a domain disabled twice keeps th…

Walked this road against the code. It holds, and the flag fix in #970 closes it. Two corrections, and what the pull request now carries.

Confirmed

Both calls are on the lines named. ImportTask.java:628 notifyImportBeginning -> MultimasterReplication.java:671 domain.disable(), then :633 TaskUtils.disableBackend -> applyConfigurationChange with enabled:false -> BackendConfigManager.java:828 deregisterBackend -> :1156-1158 performBackendPreFinalizationProcessing -> disable() again. Worth adding why the second save() can still write: the listener fires before configuredBackends.remove() and deregisterLocalBackend(), so the backend is still registered and the internal modify does reach the base entry. ignoreBackendInitializationEvent is set only by preBackendImport() (:4332), the total update road, and the guard at :281-283 has nothing else to stop.

`restore` takes the same pair

RestoreTask reaches disable() twice as well: :263 notifyRestoreBeginning, and then :270 TaskUtils.disableBackend(backendID) under if (!verifyOnly). Only restore --verifyOnly takes a single disable(), since that is the branch which skips the backend disable. So the road is not specific to import-ldif. What is specific is which failure leaves the data in place: for a restore it is a lockBackend() which does not take or a restoreBackup() which throws, after which :325 TaskUtils.enableBackend re-enables the domain over data the restore never touched.

One line reference

On the lock-failure road the task returns at ImportTask.java:653 and goes straight into the outer finally - :719 TaskUtils.enableBackend, then :732 notifyImportEnded(..., true). :688 is the catch (Exception) around importLDIF, a different road into the same finally. The conclusion stands, only the arrow in the middle points elsewhere.

The guard, and what #970 carries

The idempotence guard is not needed for this symptom. With clearInMemory() leaving the state marked as saved - which is what #970 does - the drop no longer marks it unsaved, so the save() of the second disable() has nothing to write; same for a checkpoint and for the exit save. An early return would be a second mechanism under the same assertion, on the lines #945 rewrites, so #970 does not add it.

What #970 now carries is the regression test for this road, DisabledDomainServerStateTest.aDomainDisabledTwiceKeepsItsPersistedState: a disconnected domain takes a local change, disable() saves the position, and a second disable() must leave ds-sync-state alone. Watched failing against the previous clearInMemory() - the second disable() dropped the position saved in ds-sync-state: lists don't have the same size expected [1] but found [0] - and green with it: DisabledDomainServerStateTest 2/2, PersistentServerStateTest 4/4, InitOnLineTest 10/10, ReSyncTest 2/2 on JDK 11.

On #945

Agreed that its reorder narrows the flush-thread window rather than closing it, and that it leaves the double-disable() road untouched. One detail: the residue is not only on the exit save - the save in the loop has the same check-then-save window, if (!disabled && !ieRunning()) and state.save() being two steps a disable() can run between; what the reorder buys there is that a passing checkpointer now has disableService() and the replay drain to complete inside, rather than two instructions. The two changes touch different files, so whichever lands first, the other still applies.

[OpenIdentityPlatform#951] Keep the persisted position when a domain …

[OpenIdentityPlatform#951] Test that a domain disabled twice keeps th…

[#951] Keep the persisted position when a domain drops its in-memory …

Navigation Menu

Repository navigation

Disabling a domain can persist an empty ds-sync-state and lose the replica's position #951

Symptom

The mechanism

Impact

Suggested direction

Confirmed

The justification for `clearInMemory()`'s semantics does not hold

Scope, and a third window

The impact is worse than "the safe direction"

Suggested direction, refined

Interaction with #948

The CSN generator regression does not need the race at all

What an empty state costs is more than re-replay

A second `disable()` saves the state the first one cleared

What PR #945 does and does not close here

Confirmed

`restore` takes the same pair

One line reference

The guard, and what #970 carries

On #945

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Disabling a domain can persist an empty ds-sync-state and lose the replica's position #951

Description

Symptom

The mechanism

Impact

Suggested direction

Activity

vharseko commented on Sep 8, 2026

Confirmed

The justification for clearInMemory()'s semantics does not hold

Scope, and a third window

The impact is worse than "the safe direction"

Suggested direction, refined

Interaction with #948

vharseko commented on Sep 8, 2026

The CSN generator regression does not need the race at all

What an empty state costs is more than re-replay

maximthomas commented on Sep 10, 2026

A second disable() saves the state the first one cleared

What PR #945 does and does not close here

vharseko commented on Sep 10, 2026

Confirmed

restore takes the same pair

One line reference

The guard, and what #970 carries

On #945

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

The justification for `clearInMemory()`'s semantics does not hold

A second `disable()` saves the state the first one cleared

`restore` takes the same pair