[#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, 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 identity check and the creation. 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 finally covers all the exits at once: the identity-check bail-out (where the conditional remove is a no-op, since the map is already unmapped), a FileReplicaDB constructor failure, and — once #820 lands — the shutdown bail-out of #813, which is the path where the leftover map would outlive shutdownDB() altogether.

Tests

FileChangelogDBTest drives the constructor-failure path through a newReplicaDB() hook (added in the same shape as in #820, so the hunks coincide on merge):

• failedReplicaDBCreationDropsTheDomainMapItInserted — a failed creation of the first replica DB of a domain leaves no domain map behind, 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.

With the guard disabled, the first test fails on exactly the reported symptom — {o=test={}} left in domainToReplicaDBs; with the fix, mvn -pl opendj-server-legacy verify -P precommit -Dit.test=FileChangelogDBTest passes: Tests run: 2, Failures: 0, Errors: 0, Skipped: 0.

Relationship to #820

Standalone on master. The shutdown bail-out this cleanup also protects arrives with #820 (the fix of #813); whichever merges second resolves a small overlap in getExistingOrNewReplicaDB() — the newReplicaDB() hook and its test-side use are identical on both branches on purpose.

[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.