[#821] Unregister the handler when a handshake aborts after registration - #838
Merged
vharseko merged 1 commit intoAug 4, 2026
Conversation
…orts after registration DataServerHandler.startFromRemoteDS() and both ReplicationServerHandler start paths register the handler in the domain before finalizeStart(), which can still fail before the reader and writer threads are started. abortStart() closed the session and rolled back the generation id but never unregistered the handler, and nothing else ever removes it: the normal cleanup is the reader or writer noticing the dead session and calling stopServer(), and neither thread was running. The stale entry inflated the connected DS count fed to the DS-side load balancing, was advertised in every TopologyMsg, kept the generation id from ever being reset and permanently refused reconnection of the same server id with ERR_DUPLICATE_SERVER_ID. abortStart() now asks the domain to unregister a handler that got as far as registering, running the same cleanup stopServer() would have done. The removal is conditional on the map still holding that very handler instance, so aborts that happen before registration (e.g. the duplicate server id rejection) cannot evict a legitimately connected server, and the cleanup runs under the already held domain lock instead of stopServer()'s interruptible lock acquisition, which would silently skip the cleanup when the handshake was aborted by an interrupt. Fixes OpenIdentityPlatform#821.
maximthomas
approved these changes
Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
DataServerHandler.startFromRemoteDS()and bothReplicationServerHandlerstart paths register the handler in the replication server domain beforefinalizeStart(), which can still fail before the reader and writer threads are started (interrupt insession.waitForStartup(); on the RS side the window also spansreceiveTopoInfoFromRS(), which throwsIOException/DirectoryException).abortStart()closed the session and rolled back the generation id but never unregistered the handler — and nothing else ever removes it: the normal cleanup is the reader or writer noticing the dead session and callingstopServer(), and neither thread was running.The stale entry:
ReplServerStartDSMsg, skewing the DS-side weight-based load balancing forever;TopologyMsg;resetGenerationIdIfPossible()from ever resetting the generation id;ERR_DUPLICATE_SERVER_IDon every retry.Fix
abortStart()now asks the domain to unregister a handler that got as far as registering (ReplicationServerDomain.unregisterFailedHandshake()), running the same cleanupstopServer()would have done: removal fromconnectedDSs/connectedRSs, monitoring publisher stop when it was the last server, handler shutdown,resetGenerationIdIfPossible()and topology notification.Two deliberate differences from
stopServer():stopServer()'s interruptible lock acquisition, which would silently skip the cleanup when the handshake was aborted by an interrupt — the very trigger being cleaned up after.Tests
New
HandshakeAbortRegistrationTestpins the contract in three scenarios: an abort after registration unregisters the DS handler (including generation id reset and unblocked reconnection of the same server id), the same for an RS handler, and an abort before registration leaves the legitimately connected handler untouched. Without the fix the two regression scenarios fail with "the aborted handshake left a dead DataServerHandler/ReplicationServerHandler registered"; with the fix all three pass, as does the neighbouringHandshakeAbortGenerationIdTest, which drives real aborted handshakes through the modifiedabortStart()path.Fixes #821.