[#818] Drop the empty domain map a replica DB creation leaves behind when it bails out

[vharseko]

Fixes #818

FileChangelogDB.getOrCreateReplicaDB() inserts a domain map, and announces the domain to every registered multi domain cursor, before it knows whether it will create anything in it. When the creation then does not happen — the domain map was concurrently removed, the shutdown had started, or the FileReplicaDB constructor threw — the inserted empty map stayed in domainToReplicaDBs for the lifetime of the changelog: every multi domain cursor created afterwards walked a domain holding no replica DB at all, and clearDB() reached clearGenerationId() for a domain the changelog held nothing for.

The fix

The guarded creation block now removes the domain map on its way out when no replica DB was created, via a try/finally around the shutdown check and the creation — entered only once the identity check has passed. Per the analysis on the issue, the remove is guarded by domainMap.isEmpty(): only an empty map may be dropped. A populated one — the happy path of getExistingOrNewDomainMap() returns a pre-existing map which may hold the replica DBs of other serverIds — must stay mapped for the drain of shutdownDB() to find: its weakly consistent iterator would simply never see a map removed before it reached its bin, and the replica DBs inside would never be shut down, which is exactly the leak of #813.

The identity check stays above the cleanup, per the review: ConcurrentMap.remove(key, value) compares by equals() and two empty maps are equal, so a bail-out whose domain map was already unmapped would otherwise drop the fresh, still empty map of a concurrent creation — unmapping the replica DB about to be published into it and reintroducing the leak of #813. The bail-out path therefore cleans up nothing; the cleanup runs only on the creation path, where the mapping is pinned by the monitor every removal site takes. The domainToReplicaDBs javadoc documents this third removal site.

Per the second review round, dropping the map while the server is live and without clearing the ChangeNumberIndexer left the domain announced to every live multi domain cursor, and the next successful creation announced it again — a second DomainDBCursor over the same domain, which either leaked unclosed (the cursor tree of CompositeDBCursor collapses cursors comparing equal) or delivered every change twice, killing the ChangeNumberIndexer thread with the IllegalStateException its cookie update throws on a replayed change. MultiDomainDBCursor therefore now tracks the domains it already iterates over and discards repeated announcements when it incorporates new cursors — on the cursor's own thread, the only one mutating that state, so the check cannot race a concurrent removeDomain(); new replica DBs of an incorporated domain keep reaching the cursor through addReplicaDB(). The shutdownDB() drain was also aligned with the removal protocol the domainToReplicaDBs javadoc documents — under each domain map's monitor it unmaps only the instance the monitor was taken on, with the identity check removeDomain() uses, instead of the iterator based remove, which is unconditional by key.

Tests

FileChangelogDBTest is shared with #820: one RaceableChangelogDB test double drives all the interleavings, and the tests observe domainToReplicaDBs through a package private accessor instead of reflection.

• failedReplicaDBCreationDropsTheDomainMapItInserted — a failed creation of the first replica DB of a domain leaves no domain map behind, a multi domain cursor created after the failure does not walk the phantom domain (the symptom of the issue, observed through the domains the cursor asks getCursorFrom(DN, …) to open), and the next creation starts from scratch;
• failedReplicaDBCreationKeepsAPopulatedDomainMap — a failed creation of a second replica DB keeps the populated map, with the previously created replica DB intact: this pins the isEmpty() guard;
• announcingADomainTwiceToALiveCursorMustNotOpenASecondDomainCursor — a failed creation announces the domain to a live multi domain cursor, the next successful creation announces it again: the cursor must open the domain exactly once and deliver the single published change exactly once. Fails without the MultiDomainDBCursor change, on the second cursor it then opens;
• bailOutMustNotUnmapAnotherThreadsFreshDomainMap — drives the identity-check bail-out deterministically against a concurrent creation parked inside newReplicaDB(): with the cleanup wrapped around the identity check (the shape this PR was first reviewed in) it fails on the map-identity assertion, with the current shape it passes.

ECLMultiDomainDBCursorTest is adapted to the new announcement contract, in the shape proposed by the third review round: its helper keeps one long-lived SequentialDBCursor per domain and drains re-announced cursors into it, the way DomainDBCursor.addReplicaDB() feeds an incorporated domain in production, so the test body is unchanged and the class passes against both the old and the new MultiDomainDBCursor.

mvn -pl opendj-server-legacy verify -P precommit -Dit.test=ECLMultiDomainDBCursorTest,FileChangelogDBTest: Tests run: 12, Failures: 0, Errors: 0, Skipped: 0 (FileChangelogDBTest also carries the two removeDomain() race tests of #827, which came in with master). The full -Dit.test=org.opends.server.replication.** sweep is running against the third round; this section will be updated with its numbers.

Relationship to #820

Stacked on #820 (issues/813-replica-db-created-during-shutdown), per the review: this PR's finally is what makes #820's shutdown bail-out safe, and the two branches share the test class. The first two commits here are #820's; once #820 merges, a rebase onto master reduces this PR to its own four commits — the original cleanup, and the fixes of the three review rounds on top.

[maximthomas]

Right diagnosis, and the isEmpty() guard is the correct call. But the cleanup as written reintroduces the #813 leak it is meant to avoid. One block move fixes it; requesting changes for that plus test coverage.

The conditional remove is not identity-based (blocker)

ConcurrentMap.remove(k, v) compares by equals(), and two empty maps are equal. On the identity-check exit — the one described as "a no-op, since the map is already unmapped" — domainMap is not the mapped instance, so the remove drops whatever empty map is under baseDN right then: typically another creator's fresh map, still empty because its newReplicaDB() is mid-I/O.

The winner then puts its replica DB into a map no longer reachable from domainToReplicaDBs: never reached by the shutdownDB() drain, monitor provider never deregistered, Log.logsCache entry pinned. That is exactly #813.

Reproduced against this branch with a TestNG test (stale empty map + removeDomain() + a creation parked inside newReplicaDB()):

FileChangelogDBTest ............................. Tests run: 2, Failures: 0
bailOutMustNotUnmapAnotherThreadsFreshDomainMap ............... FAILURE
  Expecting actual:  null
  and:  {2=FileReplicaDB o=test 2 null null}
  to refer to the same object

Reachability is ordinary: publishUpdateMsg() calls getOrCreateReplicaDB() for every change, so concurrent creators per domain are the norm; all that is additionally needed is a removeDomain()/clearDB()/initialize in between, and a thread descheduled between getExistingOrNewDomainMap() and synchronized (domainMap).

Two side effects worth naming: this is the only unmapping in the class performed without holding the monitor of the map actually being unmapped, which contradicts the removal protocol documented at opendj-server-legacy/src/main/java/org/opends/server/replication/server/changelog/file/FileChangelogDB.java:71-84; and it gives removeDomain()'s domainToReplicaDBs.remove(baseDN) a new way to return null, i.e. #816 outside shutdown.

Fix — hoist the identity check above the try:

      if (domainToReplicaDBs.get(baseDN) != domainMap)
      {
        // The domainMap could have been concurrently removed because
        // 1) a shutdown was initiated or 2) an initialize was called.
        // Nothing to clean up: domainMap is already unmapped, and whatever is mapped to
        // baseDN now belongs to another creation.
        return null;
      }

      try
      {
        final FileReplicaDB newDB = newReplicaDB(serverId, baseDN, server, cryptoSuite, replicationEnv);
        domainMap.put(serverId, newDB);
        return Pair.of(newDB, true);
      }
      finally
      {
        // ... unchanged comment ...
        if (domainMap.isEmpty())
        {
          domainToReplicaDBs.remove(baseDN, domainMap);
        }
      }

Past a passing identity check both removal sites must take that same monitor, so the mapping is pinned and the remove provably targets domainMap itself. #820 is still covered: its if (shutdown.get()) return null; goes inside the try. With this change all three tests pass (Tests run: 3, Failures: 0).

Tests cannot catch this class of regression (should fix)

Both cases in opendj-server-legacy/src/test/java/org/opends/server/replication/server/changelog/file/FileChangelogDBTest.java are single-threaded and only drive the constructor-failure exit. The identity-mismatch exit — where the bug above lives — has no coverage. Once #820 makes getExistingOrNewDomainMap() overridable, a second creator can be parked there and the interleaving driven deterministically.

Neither test asserts the symptom from the issue either: a phantom domain in a multi-domain cursor created afterwards, or clearDB() reaching clearGenerationId() for a domain the changelog holds nothing for. One such assertion would survive a refactor that keeps the field tidy but reintroduces the symptom.

Only one test class was run (should fix)

-Dit.test=FileChangelogDBTest only. This changes the hot path of publishUpdateMsg(); #820 ran org.opends.server.replication.** — same is warranted here.

Merge scope with #820 is understated (should fix)

Both PRs add FileChangelogDBTest.java as a new file (223 vs 342 lines), with near-identical setup(), TEST_ROOT_DN, configureReplicationServer(), createCryptoSuite() and createCleanDir(). That is an add/add conflict on the whole class, not "a small overlap in getExistingOrNewReplicaDB()". Merging #820 first and rebasing this one is the cheaper order — this PR's finally is what makes #820's new bail-out safe.

Nits

• No indexer.clear(baseDN) on the new drop path: unlike removeDomain(), which clears the indexer before unmapping. The next creation re-broadcasts addDomain() to cursors that already hold the domain, and CompositeDBCursor.cursors is keyed by cursor instance → a duplicate domain cursor. Pre-existing hazard, one new trigger; a note or follow-up issue is enough.
• Class javadoc not updated: FileChangelogDB.java:71-84 documents the domain map removal protocol and now has a third removal site to describe.
• Reflection over visibility: the test reads the private domainToReplicaDBs reflectively while the PR already relaxes visibility for newReplicaDB(). A package-private accessor would be consistent and would fail to compile rather than at runtime.
• Test finally hygiene: if changelogDB.shutdownDB() throws, remove(replicationServer) and deleteDirectory(testRoot) are skipped and the replication server plus its port leak for the rest of the suite.
• newReplicaDB() javadoc: says "control the creation and the shutdown" — the shutdown half is #820's usage, not this PR's.

Everything else checks out: both files compile against the module classpath, AssertJ 3.27.7 has containsOnlyKeys/failBecauseExceptionWasNotThrown, hasMessage() matches OpenDsException's message.toString(), the license header matches the convention for new files, and surefire runs <parallel>none</parallel> so the shared build/unit-tests/FileChangelogDB directory is safe.

[vharseko]

@maximthomas Thanks — the blocker was exactly right, and your fix is what landed. Addressed in b12ced8, on top of the original commit rebased onto #820:

• Identity check hoisted above the try — the identity-mismatch bail-out cleans up nothing; the finally runs only past a passing identity check, where the mapping is pinned by the monitor both other removal sites take. The domainToReplicaDBs javadoc now documents this third removal site and the equals() pitfall.
• Interleaving coverage — bailOutMustNotUnmapAnotherThreadsFreshDomainMap drives your scenario deterministically (a stale creator parked right after obtaining its domain map, removeDomain(), a fresh creator parked inside newReplicaDB() under the fresh map's monitor). Mutation-checked: with the cleanup wrapped around the identity check it fails on the map-identity assertion, with the fix it passes and the stale creator blocks on the fresh map's monitor instead.
• Symptom assertion — failedReplicaDBCreationDropsTheDomainMapItInserted now also asserts that a multi domain cursor created after the failure does not walk the phantom domain, observed through the domains the cursor asks getCursorFrom(DN, …) to open rather than through the field.
• Merge scope — rebased onto [#813] Refuse to create a replica DB once the changelog shutdown has started #820's branch as suggested; the add/add on the test class is resolved by merging both suites into the one RaceableChangelogDB test double, and newReplicaDB() now comes from the base, which also retires the javadoc nit about its "shutdown" half.
• Nits — reflection replaced by a package private accessor; the new tests' cleanup no longer skips remove(replicationServer) when shutdownDB() throws; the missing indexer.clear(baseDN) / repeated addDomain() hazard is noted in a comment on the drop path as pre-existing and unchanged.
• Test runs — FileChangelogDBTest: 5/5. Full org.opends.server.replication.** sweep: 3492 tests, 2 failures, both environmental (ProtocolWindowTest.saturateQueueAndRestart timing flake, MultiDomainServerStateTest.setUp port 65534 collision), both green re-run in isolation.

The branch shows #820's commits until that PR merges; I'll rebase onto master right after it lands.

[maximthomas]

please resolve merge conflicts

[vharseko]

please resolve merge conflicts

fixed

[maximthomas]

The blocker from the previous round is fixed: the identity check is above the try, the finally runs only where the mapping is pinned, and bailOutMustNotUnmapAnotherThreadsFreshDomainMap pins it with a mutation check. All five nits are addressed and the merge with master lost nothing.

One new defect blocks the merge: the cleanup unmaps a domain while the server is live and without clearing the ChangeNumberIndexer. Note this does not affect #820 — its shutdown-path drop happens after the indexer is stopped, so #820 can merge on its own.

Dropping the domain map re-announces it to live cursors (blocker)

getExistingOrNewDomainMap() announces the domain only when putIfAbsent succeeds (opendj-server-legacy/src/main/java/org/opends/server/replication/server/changelog/file/FileChangelogDB.java:245-256), so a second announcement for a domain a live cursor already holds requires a prior unmapping. Every pre-existing removal site made that safe; the new one does not:

removal site	why a re-announcement was safe
shutdownDB() drain (FileChangelogDB.java:447)	shutdownCNIndexerAndPurger() runs first (:430) — no live cursor
removeDomain() (:594)	indexer.clear(baseDN) (:605-609) blocks (ChangeNumberIndexer.java:584-599) until the indexer removed all of that domain's cursors
clearDB() (:520-523)	→ removeDomain(), same
new finally (:311-328)	nothing — server live, indexer untouched

Chain, with change number indexing on (the indexer holds a permanently registered MultiDomainDBCursor, ChangeNumberIndexer.java:310-312):

1. the failed creation announces the domain → the indexer's cursor incorporates a phantom DomainDBCursor (no replica DB → parked in exhaustedCursors, still registered in registeredDomainCursors);
2. this PR drops the map — the announcement is never retracted;
3. a later successful creation re-inserts → announces the domain again → incorporateNewCursors() builds a second DomainDBCursor for it, while cursor #1 picks the same replica DB up through addReplicaDB() (FileChangelogDB.java:205-212).

Two outcomes, decided by timing:

• same position — the comparator returns 0, because DomainDBCursor extends CompositeDBCursor<Void> makes the tiebreaker dead code (it also reads o1 twice):

  // CompositeDBCursor.java:78-86
  if (cmpCsn == 0 && o1 instanceof CompositeDBCursor && o2 instanceof CompositeDBCursor)
  {
    T data1 = ((CompositeDBCursor<T>) o1).getData();
    T data2 = ((CompositeDBCursor<T>) o1).getData();   // o1 twice, and Void is not Comparable

  cursors.put() collapses onto the existing key → cursor #2 is never stored and never closed: a leaked replica cursor pinning a Log — the #813 leak class this stack is fixing;

• diverged positions — both live → the same change is delivered twice → FileChangeNumberIndexDB.addRecord() appends a duplicate change number record (no dedup, :169-182), then

  // ChangeNumberIndexer.java:452-455
  if (!cookie.update(baseDN, csn))                     // false on the replay
  {
    throw new IllegalStateException("It was expected that change (baseDN=" + baseDN + ", csn=" + csn ...

  which is rethrown out of run() → the indexer thread dies, ECL indexing stops until restart.

Preferred fix: make the announcement idempotent — a ConcurrentSkipListSet<DN> in MultiDomainDBCursor so addDomain() is a no-op for a domain already incorporated, maintained in incorporateNewCursors() / removeDomain() / close() (removal only ever runs on the cursor's own thread). Mirroring indexer.clear(baseDN) on the drop path also works, but it spins (Thread.yield()) while the creator holds the domainMap monitor, on the publishUpdateMsg() path — I would not take it. Please add a test that announces a domain to a live cursor twice.

The safety comment's invariant is not literally true (minor)

FileChangelogDB.java:318-320 states that "every removal site takes the monitor of the map it unmaps before removing it". The drain does not — ConcurrentHashMap's value-iterator remove() is replaceNode(key, null, null), i.e. unconditional by key, so it removes whatever is mapped when it runs, not the map whose monitor it took (verified with a standalone probe: it will drop even a populated third map). Reachable: drain reads mapA → removeDomain() unmaps mapA → a creator inserts mapB → drain's it.remove() unmaps mapB without its monitor. Harmless today only because #820's inner shutdown.get() check makes a post-CAS creation impossible — which is the actual reason, not the one stated. Either reword, or harden the drain so the invariant holds:

// FileChangelogDB.java:447-459
for (Iterator<Entry<DN, ConcurrentMap<Integer, FileReplicaDB>>> it =
    domainToReplicaDBs.entrySet().iterator(); it.hasNext();)
{
  final Entry<DN, ConcurrentMap<Integer, FileReplicaDB>> entry = it.next();
  final ConcurrentMap<Integer, FileReplicaDB> domainMap = entry.getValue();
  synchronized (domainMap)
  {
    domainToReplicaDBs.remove(entry.getKey(), domainMap);
    ...

Same for the finally's claim that the repeated addDomain() hazard is "unchanged here" — this PR adds the first trigger that skips indexer.clear().

Nits

• getDomainToReplicaDBs() returns the live internal map: FileChangelogDB.java:365-368 is main source, right after master merged 7af51501d5 ("getters leaking internal state"), which fixed this alert class by returning an unmodifiable view or documenting liveness. Worth watching the Analyze (java-kotlin) job.
• Reflection helper is now redundant: with the package private accessor in place, FileChangelogDBTest.java:705-712 and its java.lang.reflect.Field import can go — the two older tests mutate the map, which the live accessor supports.
• Blocked-thread detection is heuristic where it needn't be: awaitBlockedOnAMonitor* (FileChangelogDBTest.java:758+, used at :307 and :558) accepts two BLOCKED samples 1 ms apart, which an unrelated transiently contended monitor can satisfy on a loaded CI box. The class already has the precise waitUntilBlockedOn(thread, monitor) (:715), and both call sites know the monitor (getDomainToReplicaDBs().get(TEST_ROOT_DN) / freshDomainMap).
• Import order: java.util.Set sits between java.util.concurrent.ConcurrentHashMap and ConcurrentMap (FileChangelogDBTest.java:25). Cosmetic only — checkstyle is commented out in the root pom.
• Class javadoc: FileChangelogDBTest.java:64-71 still describes only the shutdown race and the removeDomain() window, not the failed-creation cleanup.
• Unused server id: deregisterLeakedReplicaDBMonitors(replicationServer, RACING_SERVER_ID, DRAINED_SERVER_ID) in replicaDBWinningTheRaceAgainstShutdownIsShutDownByTheDrain's finally passes DRAINED_SERVER_ID, which that test never creates.

[vharseko]

The blocker of this round is fixed, plus the minor and all six nits.

Idempotent domain announcements (the blocker)

Took the preferred fix: MultiDomainDBCursor now tracks the domains it already iterates over in a ConcurrentSkipListSet<DN>.

• addDomain() ignores a domain the cursor already incorporated — new replica DBs of such a domain keep reaching it through DomainDBCursor.addReplicaDB(), broadcast by getOrCreateReplicaDB();
• the addDomain() check alone would race an incorporation of the same domain (check-then-queue against add-then-dequeue), so incorporateNewCursors() double-checks and discards a queued announcement of an incorporated domain: the authoritative check, on the cursor's own thread, where every mutation of the set lives (incorporateNewCursors(), removeDomain(), close());
• the set entry is added only after addCursor() succeeded, so an incorporation which throws retries on the next next() exactly as before;
• removeDomain() clears the entry, so a domain legitimately removed — the indexer.clear() path, the ECL disabled-domain filter — is incorporated anew by its next announcement.

The finally comment now states the announcement idempotency instead of the "pre-existing hazard, unchanged here" claim.

The requested test, announcingADomainTwiceToALiveCursorMustNotOpenASecondDomainCursor, opens a live multi domain cursor, drives a failed creation (first announcement, map dropped) and then a successful one (second announcement), and asserts that getCursorFrom(DN, …) opened the domain exactly once — walkedDomains is a list now, one element per opening — and that the single published change is delivered exactly once. Reverting the MultiDomainDBCursor change makes it fail on the cursor-count assertion.

The drain (minor)

Took the hardening over the rewording: the drain iterates entrySet() and unmaps with domainToReplicaDBs.remove(entry.getKey(), domainMap) inside synchronized (domainMap), so it can no longer drop a map whose monitor it does not hold; the fresh map of a post-CAS creation is left to that creation's own cleanup, as the new comment says. The safety comment's invariant — every removal site takes the monitor of the map it unmaps — is now literally true.

Nits

• getDomainToReplicaDBs() documents that returning the live internal map is intentional — the tests mutate it to drive interleavings — the documented-liveness resolution of 7af5150;
• the reflection helper and the java.lang.reflect.Field import are gone, both removeDomain() tests read through the accessor;
• awaitBlockedOnAMonitor* are gone: the drain race waits with waitUntilBlockedOn(shutdowner, getDomainToReplicaDBs().get(TEST_ROOT_DN)), the bail-out race with the new waitUntilBlockedOnOrCompleted(staleCreator, freshDomainMap) — the same precise ThreadMXBean check, shared via isBlockedOn();
• import order fixed;
• the test class javadoc now covers the failed-creation cleanup and the double-announcement guarantee;
• the drain race test's finally no longer passes DRAINED_SERVER_ID.

The CompositeDBCursor comparator (reading o1 twice, the dead Void tiebreaker) is left as is per the parenthetical — follow-up issue material rather than this PR's.

Tests

mvn -pl opendj-server-legacy verify -P precommit -Dit.test=FileChangelogDBTest: Tests run: 8, Failures: 0, Errors: 0, Skipped: 0. The full -Dit.test=org.opends.server.replication.** sweep is re-running against this round; I will post its numbers here when it completes.

[maximthomas]

The blocker is fixed the right way, and the drain now follows the protocol. But the MultiDomainDBCursor change breaks ECLMultiDomainDBCursorTest — deterministically, on all five Ubuntu jobs of the last run. That is the sweep you said was still running.

Idempotent announcements break ECLMultiDomainDBCursorTest (blocker)

Run 30930766769: build-maven (ubuntu-latest, 11|17|21|25|26) all fail on the same assertion. macOS/Windows are green only because .github/workflows/build.yml sets -P precommit under if: runner.os == 'Linux', so they never run the tests.

ECLMultiDomainDBCursorTest.testECLEnabledAndDisabledDomainCursors:124->assertMessagesInOrder:170
Expecting value to be true but was false

incorporatedDomains is cleared only by removeDomain() and close() — never when a domain's cursor becomes exhausted. ECLMultiDomainDBCursorTest re-announces a domain whose cursor is exhausted:

• round 1 (test:114-118) incorporates dc=example,dc=com (ECL-enabled) and cn=admin data (disabled). ECLMultiDomainDBCursor.next() calls removeDomain(cn=admin data), so that DN does leave the set. dc=example,dc=com ends the round exhausted but still incorporated;
• round 2 (test:121-122) re-announces both. cn=admin data gets its new cursor; dc=example,dc=com is silently dropped, so msgs[4] never enters the composite;
• the only live cursor is the ECL-disabled one → next() removes it → getData() is null → ECLMultiDomainDBCursor.java:113 returns false.

Reproduced locally: compiling this PR's MultiDomainDBCursor against the current build and running the class gives 4/4 pass on master and exactly this failure at :124 → :170 with the PR's version.

The test encodes the old contract — one fresh cursor per addDomain(). The new contract (a domain is announced once, later replica DBs arrive via DomainDBCursor.addReplicaDB()) is the right one, but the test has to move with it: one long-lived cursor per domain, fed new records rather than replaced. SequentialDBCursor already has add():

private final Map<DN, SequentialDBCursor> domainCursors = new HashMap<>();

private void addDomainCursorToCursor(DN baseDN, SequentialDBCursor cursor) throws ChangelogException
{
  final SequentialDBCursor existing = domainCursors.get(baseDN);
  if (existing != null)
  {
    // already known to the cursor: its long-lived per-domain cursor receives the new changes,
    // exactly as DomainDBCursor.addReplicaDB() does in production
    for (UpdateMsg msg : cursor.drain()) { existing.add(msg); }
    return;
  }
  domainCursors.put(baseDN, cursor);
  final ServerState state = new ServerState();
  when(domainDB.getCursorFrom(baseDN, state, options)).thenReturn(cursor);
  multiDomainCursor.addDomain(baseDN, state);
}

I ran that variant against both the current and the patched MultiDomainDBCursor: 4/4 either way, so it is not a test weakened to fit the new code. CompositeDBCursor.recycleExhaustedCursors() picks the added records up on the next next().

(The other two failures on the JDK 11 job — InitOnLineTest bind-port and time-out — are the usual environmental ones, unrelated.)

The addDomain() fast path is a check-then-act (minor)

public void addDomain(DN baseDN, ServerState startAfterState)
{
  if (!incorporatedDomains.contains(baseDN))
  {
    newDomains.put(baseDN, startAfterState != null ? startAfterState : new ServerState());
  }
}

This runs on the announcing thread, while incorporatedDomains.remove() runs on the cursor's thread. An announcement observing the domain as incorporated is dropped outright, and if the removal lands right after, the domain is left in neither newDomains nor incorporatedDomains — the cursor never iterates it again. Before this PR the announcement survived in newDomains, which removeDomain() does not clear.

I could not reach a live loss: removeDomain() unmaps only after indexer.clear() has blocked until the indexer's cursor dropped the domain, so a re-announcement can only follow the set entry's removal, and a re-enabled ECL domain goes through shouldReInitialize() instead. So this is fragility, not a live bug — but it is free to remove, since incorporateNewCursors() is already the authoritative check on the only thread that mutates the set:

public void addDomain(DN baseDN, ServerState startAfterState)
{
  // incorporateNewCursors() discards announcements of domains this cursor already iterates over
  newDomains.put(baseDN, startAfterState != null ? startAfterState : new ServerState());
}

newDomains is a ConcurrentSkipListMap written once per domain-map creation, so the fast path buys nothing measurable.

The drain's invariant is still not literally true (nit-plus)

ConcurrentHashMap.remove(k, v) compares with equals, and two empty maps are equal. Between it.next() and entering synchronized (domainMap), removeDomain() can unmap an empty domainMap and a creator can map a fresh empty one, so the drain can still drop a map whose monitor it does not hold:

final ConcurrentMap<Integer, FileReplicaDB> domainMap = entry.getValue();
synchronized (domainMap)
{
  domainToReplicaDBs.remove(entry.getKey(), domainMap);   // equality based: any empty map matches

Benign, because shutdownDB() flips the flag at FileChangelogDB.java:333 before building the iterator, so every creation racing the drain reads shutdown == true inside the monitor, publishes nothing and cleans its own map up. But that is the reason it is safe — not "every removal site takes the monitor of the map it unmaps". Either add the identity check removeDomain() already uses, or say why the equality confusion cannot hurt.

Nits

• MultiDomainDBCursor's javadoc says the set is "only ever mutated from the thread iterating this cursor" — close() is called from other threads too (ChangelogBackend ending an ECL session). Harmless for a ConcurrentSkipListSet, but the sentence is not accurate as written.
• announcingADomainTwiceToALiveCursorMustNotOpenASecondDomainCursor is a good test and does discriminate — reverting MultiDomainDBCursor makes walkedDomains hold two entries. Worth a line in its javadoc that it only covers the drop path, not the removeDomain() one, which never re-announces to a cursor that still holds the domain.
• Everything else from the last round checks out: the drain rewrite, getDomainToReplicaDBs()'s documented liveness, the reflection helper gone, waitUntilBlockedOnOrCompleted sharing the precise isBlockedOn() check, import order, class javadoc, the stray DRAINED_SERVER_ID.

Please re-run the full -Dit.test=org.opends.server.replication.** sweep before the next round — -Dit.test=FileChangelogDBTest alone cannot see this class of regression.

[vharseko]

All addressed in d3e6048. On the sequencing: the local sweep did catch exactly this failure and the adaptation existed locally, but the round-2 commit had already been pushed ahead of it — this round's push carries the fix, and the full sweep is re-running against it before anything else moves.

ECLMultiDomainDBCursorTest (blocker)

Adopted the proposed shape as is: addDomainCursorToCursor() keeps one long-lived SequentialDBCursor per domain and drains a re-announced cursor into it — SequentialDBCursor gained the drain() this needs — so the test body is untouched and the class passes against both the old and the new MultiDomainDBCursor.

One addition to the snippet: domainCursors is cleared in setup(). TestNG runs all the methods of the class on one instance, and testECLDisabledDomainWithCursor / testECLEnabledDomainWithCursor announce the same dc=example,dc=com to the fresh multiDomainCursor each method builds — without the clear, the second method finds the first one's cursor in the map, drains into an object no live cursor holds, and never announces the domain at all.

The addDomain() fast path (minor)

Gone, with your one-line comment expanded to say why the check must not live there: it was a check-then-act against removeDomain() on the cursor's thread, able to drop an announcement the removal no longer covers. incorporateNewCursors() keeps the authoritative check, on the only thread adding to and removing from the set.

The drain's invariant (nit-plus)

Took the identity check over the rewording, in removeDomain()'s exact shape: if (domainToReplicaDBs.get(entry.getKey()) == domainMap) inside the monitor, then the unconditional remove(key). The comment now states the equality confusion the check exists for, and that shutting the replica DBs down a second time is the same harmless no-op removeDomain() documents.

Nits

• the incorporatedDomains javadoc now says the set is added to and removed from on the iterating thread, read by announcing threads, and cleared by close() — which an ending ECL session may call from another thread;
• the double-announcement test's javadoc says it covers the drop path only: removeDomain() never re-announces a domain to a cursor which still holds it, since the cursor drops the domain through indexer.clear() before the unmapping.

Tests

mvn -pl opendj-server-legacy verify -P precommit -Dit.test=ECLMultiDomainDBCursorTest,FileChangelogDBTest: Tests run: 12, Failures: 0, Errors: 0, Skipped: 0. The full -Dit.test=org.opends.server.replication.** sweep is running against this commit; numbers follow here as soon as it completes.