Skip to content

[#924] Clear the disabled flag before a domain enables its session - #971

Merged
vharseko merged 1 commit into
OpenIdentityPlatform:masterfrom
vharseko:issues/924-enable-clears-disabled-before-session
Sep 9, 2026
Merged

vharseko merged 1 commit into
OpenIdentityPlatform:masterfrom
vharseko:issues/924-enable-clears-disabled-before-session

Conversation

@vharseko

@vharseko vharseko commented Sep 8, 2026

Copy link
Copy Markdown
Member

Fixes #924.

LDAPReplicationDomain.disable() sets disabled before it stops the session; enable() cleared it two statements after starting one:

// disable()
disabled = true;
disableService();
...
// enable()
enableService();
sessionGeneration++;

disabled = false;

enableService() ends with startListenService(), so the listener it starts can take a delivery, list it in remotePendingChanges and hand it to a replay thread while the flag still says the domain is going away. That thread reads the guard at the top of its replay loop, gives the change up - and abandonReplay() returns without asking for it again, because a domain on its way down owns its session.

The change is then listed, uncommitted and owned by nobody, and nothing brings it back: the replication server only sends a change again over a session which is restarted. Until something else restarts that session,

  • commit() advances the ServerState from the head of the pending list, so it stops in front of that change - for every master, not only the one it came from;
  • the change stays in activeAndDependentChanges, so the changes which depend on it are parked in dependentChanges and getNextUpdate() only hands them out once every older change has committed: they are never applied at all;
  • pendingChanges keeps growing for the life of the session.

It heals on the next session restart - the redelivery is taken over by putRemoteUpdate(), which accepts a listed change no replay thread owns - so this is a domain which quietly stops recording its progress rather than one which loses data.

What changed

The flag is cleared before the session is started, and put back if it could not be started: a domain whose session did not come up owns that session the way a disabled one does, so the replay threads must not believe there is one of theirs to restart. sessionGeneration is bumped where it was; it is only ever read under serviceStateLock, which enable() holds throughout.

Reachability

The window is two statements wide, and nothing in the protocol can be made to land inside it - the delivery has to travel from the socket through the listener, the replay queue and createOperation() while the enabling thread executes an increment and a volatile write. It takes the enabling thread being preempted right there. This is an invariant to hold rather than a bug to reproduce, and there is no seam in enable() which would let a test hold that thread, so it comes with a comment rather than with a test.

Merge order

Worth landing after #945. On its own this closes a two-statement window on the new-session side and opens one as long as enableService() - a broker connect and a handshake - on the side of #908: a replay thread which survived disable() reads the same flag, and with the flag cleared early it resumes and applies a change from before the import or the restore into the data which has just replaced it. #945 drains the replay of a domain under serviceStateLock before it saves its ServerState, so once it is in, no thread of this domain can be inside an attempt when disable() returns and this change is a clean win. The two do not touch the same lines - #945 leaves enable() alone.

Testing

mvn -Pprecommit verify -Dit.test=ReSyncTest - the restore and import tasks are what call disable() and enable() on a replicated backend: 2 tests, no failures.

…ables its session

disable() sets the flag before it stops the session; enable() cleared it two
statements after starting one. enableService() ends with startListenService(),
so the listener can list a delivery and hand it to a replay thread while the
flag still says the domain is going away: that thread gives the change up at
the top of its replay loop, and abandonReplay() does not ask for it again - a
domain on its way down owns its session. The change is left listed,
uncommitted and owned by nobody, so this domain's ServerState, and every
change parked behind it, is held back until something else restarts the
session.

The flag is now cleared before the session is started, and put back if it
could not be started: a domain which has no session owns it the way a
disabled one does.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug concurrency Thread-safety / race-condition bugs replication

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replication: enable() clears "disabled" after starting the session, where disable() sets it before stopping one

2 participants