[#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 ignores repeated announcements: addDomain() is a no-op for an incorporated domain and incorporateNewCursors() double-checks, since an announcement racing an incorporation can still queue a domain twice; 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 — it unmaps each domain map under its own monitor and only while it is still the mapped value, 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.

mvn -pl opendj-server-legacy verify -P precommit -Dit.test=FileChangelogDBTest: Tests run: 8, Failures: 0, Errors: 0, Skipped: 0 (the class also carries the two removeDomain() race tests of #827, which came in with master). The full -Dit.test=org.opends.server.replication.** sweep is re-running against the second review 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 three commits — the original cleanup, and the fixes of the two 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.