[#730] Fix import/export context leak on failed initializeRemote validation#731
Merged
vharseko merged 2 commits intoJul 11, 2026
Merged
Conversation
…nitializeRemote validation
ReplicationDomain.initializeRemote() acquired the import/export context
before validating the request: when validation failed ("no remotes" for an
initialize-all, or the target replica unknown to the domain - e.g. an
InitializeTarget task racing topology propagation), the DirectoryException
escaped without releasing the context. ieRunning() then stayed true forever
and the domain rejected every subsequent total update as a simultaneous
import/export until restart. The release site even carried an upstream
"FIXME should not this be in a finally?".
Validate the request before acquiring the context, and release the context
in a finally around the extracted export body, resolving the FIXME.
Also fix InitOnLineTest, where this leak cascaded over the whole class on CI
(3 of the last tests failing in afterTest on the ubuntu-latest/11 leg of PR
714's run): afterTest asserted ieRunning() before any cleanup, leaking the
domain config, brokers and three replication servers into the following
tests. Assert after the cleanup instead, wait for the target replica to
appear in the topology before scheduling InitializeTarget tasks, and add
initializeTargetUnknownRemote which reproduces the leak deterministically -
verified failing without the product fix and green with it (full class
10/10).
vharseko
added
bug
java
…dentityPlatform#731 CI) addTask() waits for the added task to reach RUNNING, tolerating a task fast enough to have already COMPLETED_SUCCESSFULLY - but not one fast enough to have already STOPPED_BY_ERROR. initializeTargetUnknownRemote's task fails its validation within milliseconds (by design), so on a fast CI runner the RUNNING phase may never be observed: the ubuntu-latest/11 leg failed with "Expected task state:RUNNING ... found [STOPPED_BY_ERROR]". Accept both terminal states in the RUNNING wait; callers interested in the final state already wait for it explicitly afterwards (the only direct RUNNING waiter outside addTask does exactly that).
Member
Author
|
@maximthomas please review |
maximthomas
approved these changes
Jul 11, 2026
Contributor
|
LGTM |
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
2 participants
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.
Fixes #730.
Problem
InitOnLineTestfailed on CI with a 3-test cascade (ubuntu-latest/11 leg ofPR #714's run, unrelated to it), all in per-test cleanup:
The trigger was a benign topology race inside
initializeTargetExportMultiSS:the
InitializeTargettask started before DS(1) had learned about the justconnected DS(2), and failed with "Cannot start total update … the remote
directory server DS(2) is unknown". What was not benign is what it left
behind.
Root cause 1 (product): import/export context leak
ReplicationDomain.initializeRemote()acquired the import/export contextbefore validating the request, and both validation errors
(
ERR_FULL_UPDATE_NO_REMOTES,ERR_FULL_UPDATE_MISSING_REMOTE) escapedwithout releasing it. The only release site was not exception-safe and even
carried an upstream FIXME:
Once leaked,
ieRunning()staystrueforever andacquireIEContext()'scompareAndSetcan never succeed again: the domain rejects everysubsequent total update as
ERR_SIMULTANEOUS_IMPORT_EXPORT_REJECTEDuntilthe server is restarted. In production a single
dsreplication initializepointed at a not-yet-known server id permanently poisons the domain.
Fix: validate the request before acquiring the context, and release the
context in a
finallyaround the extracted export body (resolving theFIXME). The export body itself is unchanged — moved to a private overload so
the existing logic keeps its shape.
Root cause 2 (test): afterTest asserted before cleaning up
InitOnLineTest.afterTest()assertedieRunning()before any cleanup;on failure it leaked the domain config, both brokers and all three
replication servers into the following tests — turning one flake into the
observed 3-test cascade. The assertion now runs after the cleanup, and the
tests that schedule
InitializeTargettasks first wait for the targetreplica to appear in
getReplicaInfos(), closing the trigger race as well.Tests
InitOnLineTest.initializeTargetUnknownRemote(new) — schedules anInitializeTargettask for a server id unknown to the domain, waits forSTOPPED_BY_ERROR, and asserts the import/export context was released.Verified failing without the product fix with exactly the CI signature
(
expected [false] but found [true], deterministic), green with it.InitOnLineTestclass: 10/10, 0 failures locally (rerunsdisabled).