[#813] Refuse to create a replica DB once the changelog shutdown has started

[vharseko]

Fixes #813.

Problem

FileChangelogDB.getOrCreateReplicaDB() reads the shutdown flag in its loop condition
(:193), and everything the creation needs happens afterwards: the domain map is inserted by
getExistingOrNewDomainMap() (:230) and the replica DB is created and registered by
getExistingOrNewReplicaDB() (:275-277). Meanwhile shutdownDB() flips the flag with a CAS
(:332) and drains domainToReplicaDBs (:354-366).

A caller which read false just before that CAS therefore inserts its domain map into a map
which has already been drained, and creates a FileReplicaDB which nothing will ever shut
down. FileReplicaDB registers its monitor provider in its constructor and deregisters it in
shutdown() (FileReplicaDB.java:118-119 and :221), and that shutdown() is only reached
from the drain in shutdownDB() and from removeDomain(). So the monitor provider of a
replication server which has stopped stays registered for the lifetime of the process, and the
log of that replica stays referenced: its entry in the static Log.logsCache is pinned,
together with the file handles of its log files, and the next replication server which opens
the same path in the same JVM — the test suite does this all the time — gets that stale Log
instance back out of the cache, whose files a removeDB() may have deleted in between.

The window is narrow but real: the drain has to be in progress, since a creation which reaches
Log.openLog() after replicationEnv.shutdown() (:367-370) is refused by
ReplicationEnvironment.checkShutDownBeforeOpening() anyway.

Changes

• The shutdown flag is read again inside the synchronized (domainMap) block which already
  guards the creation, next to the check which covers a concurrently removed domain map.
  Reading false under that monitor means the CAS in shutdownDB() has not run yet, hence its
  iterator over domainToReplicaDBs does not exist yet either: it will see the domain map,
  which was inserted before the monitor was taken, and will have to block on that same monitor
  to drain it. Reading true returns null, and the loop in getOrCreateReplicaDB() then
  throws ERR_CANNOT_CREATE_REPLICA_DB_BECAUSE_CHANGELOG_DB_SHUTDOWN, which is what a caller
  racing a shutdown is meant to get.
• getExistingOrNewDomainMap() is package private, and the creation of a FileReplicaDB moves
  into a package private newReplicaDB(). Both are overridable so that the tests can hold a
  thread at each end of the race; neither is overridden in the product.

There is one insertion site and two removal sites for domainToReplicaDBs, so the case
analysis is closed.

Tests

FileChangelogDBTest drives both branches of the race step by step, without a single sleep.

replicaDBLosingTheRaceAgainstShutdownIsNotCreated — the branch a caller loses:

1. a replica DB is created for DS(814) — the one the drain will be held in — and its monitor
   provider is asserted to be registered, so that the test cannot pass by looking for a
   registration it would not see anyway;
2. the creator thread asks for DS(813), reads the shutdown flag, sees false, and is held
   there, before it inserts the domain map it needs;
3. the shutdown thread flips the flag and drains domainToReplicaDBs, and is held inside the
   shutdown of DS(814), i.e. once that domain map has been removed and while the replication
   environment is still open — held after replicationEnv.shutdown() instead, the unfixed code
   would fail for the wrong reason and the test would pass without testing anything;
4. the creator is released into that window.

replicaDBWinningTheRaceAgainstShutdownIsShutDownByTheDrain — the branch the fix relies on,
which the first test cannot see: a drain refactored to no longer traverse domainToReplicaDBs
as it existed when the flag was flipped — snapshotting the keys beforehand, shutting the
replication environment down first — would silently reintroduce the leak while the first test
stayed green.

1. the creator thread creates its replica DB and is held once the monitor provider is
   registered but before the DB is published into the domain map, still under the domain map
   monitor — the exact state the unfixed code leaked from;
2. the shutdown starts, flips the flag, and is observed to block on the domain map monitor the
   creator holds;
3. the creator is released: it publishes the replica DB and exits the monitor, and the drain
   must shut that replica DB down, deregistering the monitor provider asserted registered in
   step 1.

The first test, checked against the unfixed code, reports both symptoms:

1) [a replica DB created while the changelog is being drained is released by nobody]
Expecting actual not to be null
3) [monitor providers of the replica DBs created during the shutdown]
Expecting empty but was: ["changelog for ds(813),cn=replication server rs(2) ...,cn=o_test,cn=replication"]

mvn -o -pl opendj-server-legacy verify -P precommit -Dit.test='FileChangelogDBTest'

Tests run: 2, Failures: 0, Errors: 0, Skipped: 0

The whole replication package passed as well at the initial revision of this PR — the review
round changed only the tests and a comment:

mvn -o -pl opendj-server-legacy verify -P precommit -Dit.test='org.opends.server.replication.**.*Test'

Tests run: 3478, Failures: 0, Errors: 0, Skipped: 0

Out of scope

Three adjacent defects found while analysing this one, filed separately rather than folded in
here: #816 (removeDomain() throws a NullPointerException when it races shutdownDB()),
#818 (an empty domain map is left behind by a creation which bails out — the residue this fix
adds one more path to) and #819 (FileReplicaDB leaks its log reference when its constructor
cannot read the CSN limits).

The issue attributed msgID 274 (ERR_CHANGELOG_UNREFERENCED_LOG_WHILE_RELEASING) to this race
as well. That message is logged when Log.releaseLog() finds no entry in logsCache, i.e. on
an unbalanced release, whereas this race produces the opposite imbalance — an open which is
never released. What it does explain is the monitor provider which outlives its replication
server, and the pinned cache entry described above.

[maximthomas]

Analysis holds up end to end. I reproduced both directions locally: as submitted the test passes 7/7; with only the 11-line guard in getExistingOrNewReplicaDB() reverted (test untouched) it fails deterministically with exactly the two symptoms you report. Also checked: one insertion site and two removal sites for domainToReplicaDBs, both removals under synchronized (domainMap) — the case analysis really is closed; the merge with #805 is clean; the new test is picked up by failsafe's default includes. Your correction of the issue's msgID 274 attribution is right (274 is an over-release, this race is an under-release).

One change requested, plus nits.

Winning branch is untested (medium)

The test covers only the losing branch (shutdown reads true → ChangelogException). The branch that carries the correctness argument — shutdown reads false under the monitor, so the drain must block and shut the new DB down — has no test.

That branch holds only because shutdownDB() builds its iterator after the CAS and ConcurrentHashMap guarantees iterators "traverse elements as they existed upon construction of the iterator". Any future refactor of the drain in opendj-server-legacy/src/main/java/org/opends/server/replication/server/changelog/file/FileChangelogDB.java:354-366 — snapshotting keySet(), moving replicationEnv.shutdown() earlier, parallelising it — silently reintroduces the leak while replicaDBLosingTheRaceAgainstShutdownIsNotCreated stays green.

The seams you added make the second test cheap: hold the creator inside newReplicaDB() instead of getExistingOrNewDomainMap(), start the shutdown, assert it is blocked, release, then assert the monitor provider is gone.

Monitor assertions read JVM-global state by prefix (medium)

replicaDBMonitorNames() in opendj-server-legacy/src/test/java/org/opends/server/replication/server/changelog/file/FileChangelogDBTest.java matches any replication server and any domain in the JVM:

final String prefix = "changelog for ds(" + serverId + ")";

TestNG runs many classes per JVM. Server ids 813/814 make a collision unlikely today, but the assertion is stronger and self-documenting scoped to the full registered name (FileReplicaDB.java:277-281):

toLowerCase("Changelog for DS(" + serverId + "),cn=" + domain.getMonitorInstanceName())

Nits

• Cite the CHM guarantee: the new comment in getExistingOrNewReplicaDB() says the drain "will see this domainMap" without saying why. One clause — ConcurrentHashMap iterators traverse elements as they existed upon construction — makes the load-bearing step self-contained for the next reader.
• deregisterMonitorProvider() bypassed: the finally block's DirectoryServer.getMonitorProviders().remove(monitorName) leaves the JMXMBean behind (opendj-server-legacy/src/main/java/org/opends/server/core/DirectoryServer.java:2734-2760). Pull the provider out and pass it to DirectoryServer.deregisterMonitorProvider(provider).
• Cleanup misses DRAINED_SERVER_ID: if softly.assertAll() fails, the final assertion never runs and the ds(814) monitor leaks into the rest of the JVM run — the finally clears only RACING_SERVER_ID.
• join() swallows its timeout: a hung creator/shutdowner leaks a live changelog with open logs into later test classes with no signal.
• newReplicaDB() duplicates the constructor signature: will rot silently if FileReplicaDB's constructor gains a parameter.
• Helper duplication: createCleanDir(), createCryptoSuite(), configureReplicationServer() and the cipherTransformation / keyLength / TEST_ROOT_DN fields are copied from FileReplicaDBTest in the same package.
• #818 becomes more likely: this adds a third path that leaves an empty domain map behind, and on the shutdown path that map outlives shutdownDB() entirely while still firing cursor.addDomain(baseDN, null). Your own test exercises exactly that residue, so worth landing #818 soon after.

[vharseko]

Addressed in 8773e2e.

Winning branch is untested — added replicaDBWinningTheRaceAgainstShutdownIsShutDownByTheDrain, on the seams you pointed at: the creator is held inside newReplicaDB(), once the monitor provider is registered but before the DB is published into the domain map — the exact state the unfixed code leaked from. The shutdown is then started and waited for until it blocks acquiring a monitor: the domain map monitor is the only one it can stay blocked on, since the locks on its way to the drain (the purger notify, the thread joins) are only transiently contended — hence two consecutive BLOCKED observations, and a fail-fast if the shutdown completes without ever blocking. The creator is released into the drain, which must shut the new replica DB down; the test asserts the registration before the shutdown starts, so the final deregistration assertion cannot pass vacuously.

Monitor assertions read JVM-global state by prefix — replicaDBMonitorName() now rebuilds the full registered name, toLowerCase("Changelog for DS(" + serverId + "),cn=" + domain.getMonitorInstanceName()), and every assertion matches it exactly.

Nits:

• the comment in getExistingOrNewReplicaDB() now cites the guarantee: ConcurrentHashMap iterators traverse the elements as they existed upon construction of the iterator;
• the cleanup pulls the provider out of the map and passes it to DirectoryServer.deregisterMonitorProvider(), so the JMXMBean is released with it, and covers both server ids;
• join() no longer swallows its timeout: a thread which outlives it is interrupted and reported to stderr with its stack trace — throwing from the finally would mask the primary failure, so a signal is the most it can leave behind;
• createCleanDir(), createCryptoSuite(), configureReplicationServer() and the cipher constants moved to FileChangelogTestFixtures, shared with FileReplicaDBTest;
• newReplicaDB() duplicating the constructor signature — left as is. The parameters come from the call site, so a factory cannot avoid restating them, and the rot is not silent: a constructor gaining a parameter stops newReplicaDB() (and the test override) from compiling. What that does not catch is a new overload added next to the old one — but then the old constructor keeps working and the seam stays correct;
• An empty domain map is left behind by a replica DB creation which bails out #818 — agreed, it is next in line; this fix indeed adds a third path leaving an empty domain map behind, and the shutdown path fires cursor.addDomain(baseDN, null) on a map nothing will drain.

mvn -o -pl opendj-server-legacy verify -P precommit -Dit.test='FileChangelogDBTest'

Tests run: 2, Failures: 0, Errors: 0, Skipped: 0

[maximthomas]

@vharseko please fix merge conficts

[vharseko]

@vharseko please fix merge conficts

fixed

[maximthomas]

LGTM. Both requested changes are in, and the merge is clean.

Before merging

• Wait for a Linux leg to go green — they are the only ones that set -P precommit, so macOS and Windows never execute FileChangelogDBTest. At review time only build-maven (macos-latest, 11) had reported.
• Land #818 soon after: this adds a third path leaving an empty domain map behind, and on the shutdown path that map now outlives shutdownDB() entirely while still firing cursor.addDomain(baseDN, null).