[#792] Fail fast when the replication server cannot bind its listen port

[vharseko]

Fixes #792 — and closes follow-up item 1 recorded in #728.

Problem

ReplicationServer.initialize() only logged ERR_COULD_NOT_BIND_CHANGELOG and returned
normally. A replication server whose listen port was taken was therefore created without
any listener: the constructor "succeeded", the object was registered, and every consumer
(tests, embedded server, production start-up) only learned about it much later, and in an
unrelated place, as a Connection refused. That is exactly how #792 surfaced — the real
error and the reported failure were ~400 lines and one test helper apart.

The port change path of applyConfigurationChange() had the same shape: the old listen
socket is closed first, so a failing rebind leaves the server without a listen thread,
while the log says ERR_COULD_NOT_CLOSE_THE_SOCKET — a message about an error that did
not happen.

Changes

• bindListenPort() retries the bind a few times (5 attempts, 200 ms apart), which makes a
  momentarily occupied port self-healing instead of fatal.
• initialize() propagates the failure as a ConfigException rather than swallowing it,
  and abortInitialization() stops the threads, releases the changelog DB and the external
  changelog so no half initialized instance is left behind (it is never returned to the
  caller, so nothing would ever shut it down). It only deregisters the external changelog
  virtual attribute rules when this instance registered them: they are held globally, by
  attribute name, so releasing them blindly would strip them from a running instance.
• applyConfigurationChange() reports a failed port change through the
  ConfigChangeResult (OPERATIONS_ERROR + message) and goes back to the previous listen
  port, instead of leaving the server deaf with a configuration advertising a port it does
  not listen to. isConfigurationChangeAcceptable() now validates the new port instead of
  accepting every change unconditionally.
• On a bind failure the port is probed once, when the retries are given up, and the error
  message says what was observed on the loopback interface: another socket listening, a
  port which no longer holds anything, or a port which accepts no connection there — which
  covers both a socket bound to another address and a socket which does not listen at all.
  The probe sets SO_REUSEADDR, like every other socket which can land on a replication
  port, so that a probe which self-connects cannot hold the port in TIME_WAIT and defeat
  the retry it is diagnosing.
• ReplicationServerListener creates the replication server before registering itself as a
  configuration listener — a listener registered by an object which is then discarded stays
  in the server wide configuration repository — and applyConfigurationAdd() logs and
  reports the reason of the failure, so dsconfig create-replication-server no longer
  returns a bare error.
• New ReplicationServer.isListening(). stopListen became volatile: it is written by
  the thread applying a configuration change and read by the listen thread.

Upgrade note

A replication server whose listen port is durably occupied now fails to start, and with it
the whole directory server: the ConfigException propagates from
MultimasterReplication.initializeSynchronizationProvider() up to
DirectoryServer.startServer(), which does not catch it, so the connection handlers are
never started.

This is the intended fail-fast and it matches what OpenDJ already does for every other
listener — LDAPConnectionHandler.initializeConnectionHandler() throws an
InitializationException when its port is in use and the server does not start either —
but it differs from the previous behaviour of the replication port, which was to log
ERR_COULD_NOT_BIND_CHANGELOG and come up degraded, without replication. A deployment
whose replication port is held by an orphaned process after an unclean shutdown will now
see the server refuse to start rather than run without replication.

Tests

ReplicationServerDynamicConfTest covers:

• replServerFailsWhenListenPortIsInUse — port held by a socket → ConfigException, the
  failed instance is not left registered, and the external changelog of the replication
  server which is already running (virtual attribute rules, changelog backend) is left
  untouched;
• replServerRetriesToBindItsListenPort — the port is released while the replication
  server is retrying, which must be enough for it to start;
• replServerRollsBackAFailedPortChange — a change to a port in use is rejected by
  isConfigurationChangeAcceptable(), and a change applied anyway reports
  OPERATIONS_ERROR and leaves the replication server listening on its previous port,
  still usable by a broker;
• isListening() assertions before and after a port change in the existing test.

Locally, with -P precommit, the whole replication package:

mvn -o -pl opendj-server-legacy verify -P precommit \
    -Dit.test='**/replication/**/*Test.java,**/replication/**/*TestCase.java'

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

Not in this PR

• Per invocation port allocation in AssuredReplicationServerTest and the ~8 other
  replication tests which pin their ports in @BeforeClass (item 2 of Flaky test: AssuredReplicationServerTest#testSafeDataFromRS — RS silently starts without a listener when the replication port is briefly occupied #792). The retry
  makes the transient case self-healing, so this became optional rather than necessary.
• Whether rerunFailingTestsCount=3 actually applies to TestNG @DataProvider driven
  methods (item 3 of Flaky test: AssuredReplicationServerTest#testSafeDataFromRS — RS silently starts without a listener when the replication port is briefly occupied #792) — still unverified.
• FileChangelogDB.initializeDB() swallows ChangelogException and only logs
  ERR_COULD_NOT_READ_DB, so a replication server with an unreadable changelog still binds
  and starts its threads. Propagating it means changing the ChangelogDB interface and
  every implementation, which belongs to its own change.

[maximthomas]

Good diagnosis of #792 and the right direction — failing fast beats a stochastic Connection refused 400 lines away. Verified the branch compiles (mvn -o -pl opendj-server-legacy test-compile, exit 0) and reproduced the socket behaviour below on Linux 6.8 / JDK 17.

One blocker, plus a behaviour change that needs an explicit decision.

Diagnostic probe can lock the listen port for ~60 s (blocker)

describeBindFailure() creates its probe without SO_REUSEADDR:

// opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServer.java
try (Socket probe = new Socket())          // <-- getReuseAddress() == false
{
  probe.connect(new InetSocketAddress(InetAddress.getLoopbackAddress(), port), LISTEN_PORT_PROBE_TIMEOUT_MS);
  diagnostic = isSelfConnection(probe) ? ... ;
}

If the probe hits the very self-connect this branch exists for, closing it leaves a TIME_WAIT that blocks the rebind. Measured:

new Socket().getReuseAddress()       = false
new ServerSocket().getReuseAddress() = true

self-connect closed, SO_REUSEADDR=false -> port rebindable after 61790 ms
self-connect closed, SO_REUSEADDR=true  -> port rebindable after     0 ms

Because describeBindFailure() is called inside the retry loop (attempts 1–4), a self-connecting probe poisons attempts 2–5, and with the new fail-fast the server then refuses to start for a minute — caused by its own diagnostic.

Every other socket that can land on a replication port already sets the flag, right before the same isSelfConnection check:

• opendj-server-legacy/src/main/java/org/opends/server/replication/service/ReplicationBroker.java:1075-1076
• opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServer.java:406-410
• opendj-server-legacy/src/main/java/org/opends/server/util/StaticUtils.java:1301 (isAddressInUse)
• opendj-server-legacy/src/test/java/org/opends/server/TestCaseUtils.java:764-765, .../server/AssuredReplicationServerTest.java:833-834

The new probe is the only exception. Fix:

try (Socket probe = new Socket())
{
  probe.setReuseAddress(true);
  ...

Worth noting the flip side: because those sockets do set the flag, a self-connect rejected by #729 frees the port in ~0 ms, so the 800 ms retry premise holds — as long as the probe doesn't break it.

Bind failure aborts the entire server, not just replication (high)

ReplicationServer ctor → ReplicationServerListener ctor → MultimasterReplication.initializeSynchronizationProvider() (.../replication/plugin/MultimasterReplication.java:262) → SynchronizationProviderConfigManager.getSynchronizationProvider() rethrows (.../core/SynchronizationProviderConfigManager.java:317-330) → DirectoryServer.startServer():1499.

startServer() (opendj-server-legacy/src/main/java/org/opends/server/core/DirectoryServer.java:1320-1555) has no catch covering that line. Connection handlers are initialized at :1490 but startConnectionHandlers() at :1525 is never reached — so an instance whose replication port is durably taken (orphaned process after an unclean shutdown) now serves no LDAP at all, where it previously came up degraded.

The retry covers the transient case, not this one. Fine as a deliberate choice, but it needs a release/upgrade note rather than arriving as a side effect. The CI motivation in #792 is fully served by failing fast in tests.

ReplicationServerListener leaks itself as a registered config listener (medium)

.../replication/plugin/ReplicationServerListener.java:60-61 registers this before creating the RS at :67. Registration lands in the server-wide config repository (opendj-config/src/main/java/org/forgerock/opendj/config/server/ServerManagedObject.java:1183-1193), keyed by DN — not scoped to the discarded object. finalizeSynchronizationProvider() can't clean up because replicationServerListener was never assigned (null check at MultimasterReplication.java:563).

Harmless when the JVM dies at startup; not harmless for EmbeddedDirectoryServer or a failed-then-retried dsconfig enable, which leaves a dead listener per attempt. Create the RS before registering the listeners, or deregister in a catch.

abortInitialization() can deregister another instance's ECL state (medium)

shutdownExternalChangelog() ends in an unconditional deregisterVirtualAttributeRules(), and deregistration is keyed by attribute name, not identity:

// opendj-server-legacy/src/main/java/org/opends/server/core/VirtualAttributeConfigManager.java:479-488
public void deregister(VirtualAttributeRule rule) { rules.remove(getDummyDN(rule)); }
private DN getDummyDN(VirtualAttributeRule rule) {
  return DN.valueOf("cn=" + rule.getAttributeType().getNameOrOID() + ",cn=Virtual Attributes,cn=config");
}

So an RS that took the enableExternalChangeLog() early-return (ReplicationServer.java:632-637, changelogBackend == null) and then failed to bind will strip lastexternalchangelogcookie / firstchangenumber / lastchangenumber / changelog from a running RS. Reachable via the previous issue: stale listener + live listener both get applyConfigurationAdd, the second RS fails to bind and deregisters the first one's attributes. Guard the abort with a flag recording whether this instance registered anything.

Bind diagnostic reports the opposite of the truth for non-loopback listeners (medium)

Ran the method verbatim against four holder shapes:

port holder	wildcard bind	reported diagnostic
listening on 0.0.0.0	EADDRINUSE	"another socket is listening" ✅
listening on 192.168.x.x	EADDRINUSE	"held by a socket which does not accept connections" ❌
listening on 127.0.0.1	EADDRINUSE	"another socket is listening" ✅
client socket bound, not listening (#728 shape)	EADDRINUSE	"held by a socket which does not accept connections" ✅

A listener on any non-loopback address gets the exact opposite diagnosis, and the whole point of this line is to be trusted in a post-mortem. Either soften the wording to what was actually observed ("nothing accepted a connection on 127.0.0.1:%d") or probe the configured addresses too.

Also: the isSelfConnection(probe) branch describes the probe's own freshly-created socket, so it can't support "the port is held by a TCP self-connect".

Failed port change leaves the RS deaf with a config that lies (medium)

applyConfigurationChange() sets this.config = configuration before the port block, so a failed rebind leaves no listener, a config advertising the new port, and localPorts never updated. Reporting OPERATIONS_ERROR is an improvement, but nothing recovers until a restart. The old port was free a moment ago — a rollback (this.config = oldConfig, setServerURL(), rebind, restart listen thread) would close it.

Separately, the InterruptedException path never resets stopListen, which is only cleared on the success path — any future listen thread would exit its loop immediately.

Test coverage (low)

• assertEquals(getAllInstances().size(), instancesBefore) is a tautology: allInstances.add(this) was already the constructor's last statement on master, after initialize(). abortInitialization() is untested — assert the changelog backend / virtual-attribute state instead.
• The retry — the part that actually fixes the flake — has no test. Hold the port, release it from another thread after ~300 ms, assert isListening().
• No test for the newly-reporting applyConfigurationChange failure (OPERATIONS_ERROR + isListening() == false) or for isConfigurationChangeAcceptable rejecting an occupied port.

Nits

• Diagnostic computed 5×: 4 probes in the retry loop + 1 in initialize()'s catch, evaluated eagerly even when WARN is off. Compute once on final failure and pass it to both messages. (Latency is fine — a refused loopback probe measures ~10 ms, not the 200 ms timeout, so the failure path costs ≈800 ms of sleeps.)
• Retry on non-transient errors: bindListenPort() retries any IOException, including Permission denied on a privileged port. Restrict to BindException.
• FD leak: isConfigurationAcceptable() closes tmpSocket only on the success path; a failing bind skips it. Pre-existing, but the change path now routes through it, so it leaks once per rejected port change.
• Sibling swallow: FileChangelogDB.initializeDB() (.../changelog/file/FileChangelogDB.java:281-299) catches ChangelogException and only logs ERR_COULD_NOT_READ_DB — one line above the swallow this PR fixes. An RS with an unreadable changelog still binds and starts its threads. Worth a follow-up issue.
• Untranslated text in a localized message: describeBindFailure() returns English into ERR_COULD_NOT_BIND_CHANGELOG_6's %s, which has fr/de/es/ja/ko/zh translations. Give the two diagnostics their own message keys, or keep them to logger.trace.
• Double logging: initialize() logs the bind error, then SynchronizationProviderConfigManager logs it again as ERR_CONFIG_SYNCH_ERROR_INITIALIZING_PROVIDER with a stack trace.
• Dead message: ERR_COULD_NOT_CLOSE_THE_SOCKET_70 now has no caller in main code (8 locale files still carry translations).
• Swallowed exception: describeBindFailure()'s inner catch (IOException e) discards e; a logger.traceException(e) would help when the probe itself misbehaves.
• Possible null: cause.getMessage() can be null, yielding "null (…)".

[vharseko]

Thanks for the very precise review — the socket measurements in particular saved a lot of guessing. Everything is addressed in c47f867, except two items which I deliberately left out, and one where I ended up disagreeing with the framing rather than with the fact. Details below, in your order.

Diagnostic probe can lock the listen port for ~60 s

Fixed: the probe now sets SO_REUSEADDR before connecting, like ReplicationBroker, ReplicationServer.connect(), StaticUtils.isAddressInUse() and TestCaseUtils already do.

Worth recording how narrow the window is, because it changed how I fixed the rest: for the probe to self-connect, the port must have been released between the failed bind() and the probe (while the port is genuinely held, the kernel will not hand it out as an ephemeral local port), and the kernel must pick exactly that port out of the ephemeral range. So the failure mode is real but rare — one line to remove it, no argument, and the second half of the fix is that the probe now runs once, when the retries are given up, instead of on every attempt. That also removes 4 spurious connections to whatever legitimate listener holds the port: probing a live RS made it accept a connection and immediately see EOF.

Bind failure aborts the entire server, not just replication

The mechanics are exactly as you describe — no catch covers DirectoryServer.startServer():1521, so startConnectionHandlers() at :1547 is never reached. I disagree only about it being new behaviour worth a design change: LDAPConnectionHandler.initializeConnectionHandler() already throws an InitializationException when its listen port is in use (checkAnyListenAddressInUse()), ConnectionHandlerConfigManager.getConnectionHandler() wraps it into a ConfigException, and the server does not start either. An OpenDJ instance whose LDAP port is taken refuses to start today; this PR makes the replication port behave the same way, rather than inventing a new contract.

So I kept the fail-fast and wrote it up as an explicit Upgrade note in the PR description, naming the case you raised (orphaned process holding the port after an unclean shutdown → server refuses to start instead of running degraded).

ReplicationServerListener leaks itself as a registered config listener

Fixed: the replication server is created first, and addReplicationServerAddListener() / addReplicationServerDeleteListener() only run once it exists. Your diagnosis of why the existing cleanup cannot help was right — SynchronizationProviderConfigManager does call finalizeSynchronizationProvider(), but MultimasterReplication.replicationServerListener is still null at that point, so nothing was ever deregistered.

applyConfigurationAdd() now also logs the failure instead of only putting it in the ConfigChangeResult.

abortInitialization() can deregister another instance's ECL state

Fixed, and thank you — this one was worse than "medium": the new test in the first revision was itself able to trigger it, since replication tests share a JVM and any second RS takes the enableExternalChangeLog() early return.

ReplicationServer now tracks whether this instance registered the virtual attribute rules and only deregisters them in that case; the changelog backend was already guarded by changelogBackend != null, so the two are now symmetric. abortInitialization() also sets shutdown and interrupts the connect/listen threads — unreachable today, since the bind happens before the threads start, but the method claims to release what initialize() acquired, so it should.

There is a regression test: replServerFailsWhenListenPortIsInUse now starts a replication server first and asserts that the aborted one leaves its virtual attribute rules and its changelog backend registered.

Bind diagnostic reports the opposite of the truth for non-loopback listeners

Fixed by reporting only what was observed, and by making the three outcomes real localizable messages instead of English text injected into the %s of a localized one (ERR_COULD_NOT_BIND_CHANGELOG_LISTENING_303, ..._NOT_ACCEPTING_304, ..._PORT_FREE_305):

probe result	message
connect succeeds, not a self-connect	Another socket is listening on 127.0.0.1:%d
connect refused	Nothing accepted a connection on 127.0.0.1:%d : the port is held either by a socket bound to another address, or by a socket which does not accept connections
self-connect	Nothing holds 127.0.0.1:%d anymore : the port was released after the last attempt to bind it

The self-connect branch also stopped lying: as you noted, that self-connect is the probe's own, which means the port was free at probe time — that is now what it says.

Failed port change leaves the RS deaf with a config that lies

Fixed with a rollback. The port change is now stopListenThread() + startListenThread(), and on failure rollbackListenPort() restores the previous configuration, rebinds the old port and restarts the listen thread; only a failed rollback leaves the server without a listener, and it adds its own message to the ConfigChangeResult. localPorts is updated on a successful change (it was never updated at all before, even on success).

stopListen is now cleared inside startListenThread(), after the bind succeeds, which fixes the InterruptedException path you found: the flag can no longer stay set with no listener, and a failed bind can no longer leave a listen thread spinning on a closed socket. It also became volatile — it is written by the config-change thread and read by the listen thread.

replServerRollsBackAFailedPortChange covers all of it: rejection by isConfigurationChangeAcceptable(), OPERATIONS_ERROR from applyConfigurationChange(), the server still listening on its previous port, and a broker still able to connect to it.

Test coverage

You were right that the instance-count assertion was worth little. ReplicationServerDynamicConfTest now has four tests: the existing port change, replServerFailsWhenListenPortIsInUse (fail fast + the ECL assertions above), replServerRetriesToBindItsListenPort (port released from another thread while the server retries), and replServerRollsBackAFailedPortChange.

The retry test releases the port after 300 ms against an 800 ms retry budget, so it has margin; it degrades to "the server starts" rather than to a failure if the scheduler is unkind.

Nits

• Diagnostic computed 5× — fixed, see above.
• Retry on non-transient errors — not done, and I think the suggestion does not achieve its goal: the JDK throws BindException for EACCES ("Permission denied") and EADDRNOTAVAIL too, not only for EADDRINUSE, so restricting the retry to BindException would still retry a privileged-port failure. Discriminating would mean matching on the message text. The cost of getting it wrong is 800 ms once, on a path which then fails anyway, so I left the retry as is.
• FD leak in isConfigurationAcceptable() — fixed, try-with-resources.
• Sibling swallow in FileChangelogDB.initializeDB() — agreed, and left out on purpose: propagating it means adding throws ChangelogException to the ChangelogDB interface and every implementation and caller. It is listed under "Not in this PR" and deserves its own issue.
• Untranslated text in a localized message — fixed, see the table above. (The %s still carries the JDK error text, which is English by nature.)
• Double logging — fixed: initialize() now only throws, and the single log line comes from whoever reports the ConfigException (SynchronizationProviderConfigManager at start-up, ReplicationServerListener for dsconfig).
• Dead message ERR_COULD_NOT_CLOSE_THE_SOCKET_70 — left in place; the constant is generated and the translations are harmless, and removing an ordinal is more churn than it is worth.
• Swallowed exception in the probe's inner catch — fixed, logger.traceException(e).
• Possible null from cause.getMessage() — fixed; all these paths now use getExceptionMessage().

Verification

mvn -o -pl opendj-server-legacy verify -P precommit \
    -Dit.test='**/replication/**/*Test.java,**/replication/**/*TestCase.java'

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

(including AssuredReplicationServerTest 360, ReplicationServerLoadBalancingTest, StateMachineTest, and ChangelogBackendTestCase 30 in a separate run for the external changelog change.)

[maximthomas]

All eight items and nine nits from the previous round are addressed in c47f867, and I agree with both pushbacks: the LDAPConnectionHandler precedent justifies the fail-fast, and BindException does cover EACCES/EADDRNOTAVAIL, so restricting the retry to it would not have discriminated. Verified mvn -pl opendj-server-legacy -am -DskipTests test-compile → exit 0.

Four things the new revision introduced or left.

Rollback leaves a half-applied config, and this.config diverges from what was persisted (medium)

applyConfigurationChange() applies the new config in place before the port block:

// opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServer.java
this.config = configuration;                          // :1128
disconnectRemovedReplicationServers(oldRSAddresses);  // :1130  reads the new list via getConfiguredRSAddresses()
this.changelogDB.setPurgeDelay(getPurgeDelay());      // :1135
this.changelogDB.setComputeChangeNumber(computeCN);   // :1142
cryptoSuite.newParameters(...);                       // :1151

rollbackListenPort() restores this.config (:619) but undoes none of that, and the blocks after the port block (:1199 monitoring period, :1208 group id, :1218 weight, :1226 DB dir) then compare oldConfig against config — the same object — so they no-op. Result: purge delay / compute-CN / crypto and dropped peer connections from the new config, group id / weight / monitoring period from the old one.

The persisted config is already the new one — ConfigurationHandler writes before dispatching:

// opendj-server-legacy/src/main/java/org/opends/server/config/ConfigurationHandler.java
:642   writeUpdatedConfig();
:653   final ConfigChangeResult result = listener.applyConfigurationChange(evaluatedNewEntry);

So config.ldif says 2000 while the RS listens on 1000. Restart fails again, and the next unrelated change (e.g. --set replication-purge-delay:…) arrives with port 2000 against a live 1000 and re-fires the port block: changing the purge delay silently moves the listen port.

The ports differ, so bind the new one before closing the old one — no window without a listener, nothing to undo, rollbackListenPort() disappears. With a bindListenPort(int port) overload there is no config flip-flop either:

final int newPort = configuration.getReplicationPort();
if (newPort != oldConfig.getReplicationPort())
{
  final ServerSocket newSocket;
  try
  {
    newSocket = bindListenPort(newPort);   // old socket still open and serving
  }
  catch (IOException e)
  {
    this.config = oldConfig;               // nothing was touched
    ccr.setResultCode(ResultCode.OPERATIONS_ERROR);
    ccr.addMessage(bindFailureMessage(newPort, e));
    return ccr;
  }
  stopListenThread();                      // safe: the replacement is already bound
  listenSocket = newSocket;
  setServerURL();
  stopListen = false;
  listenThread = new ReplicationServerListenThread(this);
  listenThread.start();
  localPorts.remove(oldConfig.getReplicationPort());
  localPorts.add(newPort);
}

This also makes the InterruptedException case harmless, removes the failed-rollback branch that can still leave the server deaf, and closes the window where setServerURL() has already moved to the new port while nothing is bound there — today that URL is what peers get in topology messages for the ~1 s the failed bind and the rollback take.

Also, the comment at :1285-1286 is stale — it says applyConfigurationChange() "cannot roll back to the old one", in the commit that added the rollback.

replServerRetriesToBindItsListenPort is timing-dependent in both directions (medium)

// opendj-server-legacy/src/test/java/org/opends/server/replication/server/ReplicationServerDynamicConfTest.java
Thread.sleep(300);
close(portHolder);
...
replicationServer = new ReplicationServer(...);   // 5 attempts, 4 × 200 ms = 800 ms budget

Two unsynchronised clocks, so "degrades to the server starts" covers one direction only:

• Releaser early / RS slow — everything the constructor does before the first bind attempt runs inside the 300 ms: enableExternalChangeLog(), crypto suite, ReplSessionSecurity, and changelogDB.initializeDB() (ReplicationServer.java:486-489), which opens a ReplicationEnvironment and replays the changelog state. Overrun that and the port is free at the first attempt — the test passes without exercising the retry.
• Releaser late — a GC pause past 800 ms makes the constructor throw ConfigException and the test fail. A new flake, in the PR that removes one.

Add a package-private attempt counter set by bindListenPort() and assert > 1, so the vacuous-pass direction fails loudly. For the margin, prefer a wall-clock deadline over an attempt count — 800 ms is a guess against an unmeasured transient, and simply raising LISTEN_BIND_ATTEMPTS doubles the worst case on the change path, where a failed bind and a failed rollback bind each pay the full budget.

The rest of the new tests are sound: bindFreePort() binds the wildcard address and leaves the socket in LISTEN (opendj-server-legacy/src/test/java/org/opends/server/TestCaseUtils.java:760-768), so EADDRINUSE is deterministic despite SO_REUSEADDR; and replServerFailsWhenListenPortIsInUse holds whether or not runningServer takes the enableExternalChangeLog() early return, which is exactly the case the new guard fixes.

The ReplicationServerListener reordering trades one leak for its mirror image (low)

Creating the RS before registering the listeners fixes the leaked listener, but both registrations can throw:

// opendj-config/target/generated-sources/.../server/ReplicationSynchronizationProviderCfg.java
:200  void addReplicationServerAddListener(ConfigurationAddListener<ReplicationServerCfg> listener) throws ConfigException;
:222  void addReplicationServerDeleteListener(ConfigurationDeleteListener<ReplicationServerCfg> listener) throws ConfigException;

A throw there now propagates out of the constructor with a fully started RS behind it — listen and connect threads, changelog DB, ECL virtual attributes, entries in allInstances and localPorts. MultimasterReplication.java:262 assigns replicationServerListener only on success and :563-565 guards the shutdown with a null check, so nothing can ever stop it. Same class of bug, other end:

if (configuration.hasReplicationServer())
{
  replicationServer = new ReplicationServer(configuration.getReplicationServer(), dsrsShutdownSync);
}
try
{
  configuration.addReplicationServerAddListener(this);
  configuration.addReplicationServerDeleteListener(this);
}
catch (ConfigException e)
{
  shutdown();   // releases the replication server just created
  throw e;
}

Double logging is back on both ConfigChangeResult paths (low)

handleConfigChangeResult() already logs the ccr messages on a non-SUCCESS result:

// opendj-server-legacy/src/main/java/org/opends/server/config/ConfigurationHandler.java:1801
logger.error(ERR_CONFIG_CHANGE_RESULT_ERROR, className, methodName, entryDN, resultCode, adminActionRequired, messages);

It is called for applyConfigurationAdd (:491) and applyConfigurationChange (:655), and ServerManagedObject.registerChangeListener (:791-803) routes ReplicationServer through that same handler. So logger.error(...) + ccr.addMessage(...) in ReplicationServer.applyConfigurationChange() (:1169-1171, :1179-1182) and in ReplicationServerListener.applyConfigurationAdd() (:88-91) each log twice — the duplication removed from initialize(). Drop the explicit logger.error on those three sites.

Nits

• Localization regression on the hottest path: ERR_COULD_NOT_BIND_CHANGELOG_6 is translated in 7 locales under opendj-server-legacy/src/messages/org/opends/messages/; start-up now emits one of the three new English-only keys instead. "Port in use" is the message operators actually hit.
• Inconsistent exception rendering: ERR_COULD_NOT_STOP_LISTEN_THREAD.get(e) (ReplicationServer.java:1193) passes the exception where the rest of the PR uses getExceptionMessage(e); InterruptedException has a null message, so it renders as bare java.lang.InterruptedException.
• Inconsistent result codes: ReplicationServerListener.applyConfigurationAdd reports CONSTRAINT_VIOLATION for a bind failure while applyConfigurationChange reports OPERATIONS_ERROR for the same condition.
• abortInitialization() NPE landmine: it dereferences changelogDB, assigned at :204, while enableExternalChangeLog() runs at :199 outside the try. Unreachable today, but a null guard would make the method match its javadoc and safe to reuse — enableExternalChangeLog() throwing still leaks a registered changelog backend and any partially registered virtual attribute rules, since registerVirtualAttributeRules() throwing skips externalChangelogRegistered = true.
• isListening() over-promises: returns true between the bind and listenThread.start() (:600-604). Test-only usage, so harmless.
• Test residue: cleanup is ReplicationTestCase.remove() → rs.remove() and rs.getChangelogDB().removeDB(); the aborted instance is never handed to the test, so replServerFailsWhenListenPortIsInUseDb stays on disk. abortInitialization() is otherwise equivalent to shutdown() for a pre-bind failure.
• Closing keyword vs deferred work: Fixes #792 auto-closes it while items 2 (per-invocation ports in AssuredReplicationServerTest and ~8 siblings) and 3 (rerunFailingTestsCount vs @DataProvider) are deferred. File those — and the FileChangelogDB.initializeDB() swallow — as follow-ups, or drop the keyword.

[vharseko]

Thanks again — the second round found the real weakness of the first fix. All four items are
addressed in 07cab85, two of them by dropping the rollback rather than repairing it. Details in
your order.

Rollback leaves a half-applied config

You were right that restoring this.config undoes nothing that ran before the port block, and
that the blocks after it then compare an object with itself. Rather than extend the rollback, the
port change now happens first and without a rollback:

• applyConfigurationChange() starts with the port block, before this.config = configuration,
  so a change which cannot be applied returns with nothing applied at all — not the purge delay,
  not compute-change-number, not the crypto parameters, not the dropped peer connections.
• switchListenPort() binds the new port while the current listen socket is still open and
  serving, and only then stops the listen thread and hands the new socket to a new one. On
  failure the current socket was never touched: there is no window without a listener, no window
  during which setServerURL() advertises a port nothing listens to, and nothing to undo.
• rollbackListenPort() is gone, and with it the branch where a failed rollback left the server
  deaf. The stale comment in isConfigurationChangeAcceptable() is gone too.

Two details from your sketch which needed care: the socket bound for the new port must be closed
if stopListenThread() is interrupted, otherwise the failed change leaks it; and stopListen
must only be cleared after the join, otherwise an interrupted join leaves the previous listen
thread alive and it starts accepting on the new socket — two listen threads on one port. The
InterruptedException path therefore keeps the server stopped and reports OPERATIONS_ERROR.

On reachability, for the record: configChangeIsAcceptable runs before writeUpdatedConfig()
(ConfigurationHandler:624 vs :642), and this PR made isConfigurationChangeAcceptable()
validate the port, so a rejected port change never reaches config.ldif. The divergence you
describe needs the port to be taken between the check and the apply. Narrow, but the bind-first
ordering removes it rather than relying on the window being small.

replServerRollsBackAFailedPortChange became
replServerKeepsItsConfigurationWhenAPortChangeFails and now also changes the weight in the
rejected configuration, asserting that a failed port change applies none of the rest.

replServerRetriesToBindItsListenPort is timing-dependent in both directions

Agreed on the diagnosis, but a plain assert attempts > 1 would trade the silent vacuous pass
for a new flake in the other direction. The port is now released because the bind failed, not
after a delay: ReplicationServer.listenPortBindFailures (package private, AtomicInteger) is
incremented by bindListenPort() on every failed attempt, and the releasing thread waits for it
to move before closing the socket. The test then asserts both that the counter moved — so it
cannot pass without exercising the retry — and that the server is listening. No clock is involved
on either side, so neither direction of the race remains.

The ReplicationServerListener reordering trades one leak for its mirror image

Fixed as suggested: both registrations are wrapped, and a ConfigException shuts down the
replication server which was just created before propagating.

Double logging is back on both ConfigChangeResult paths

Fixed: the explicit logger.error calls are gone from the change path and from
applyConfigurationAdd(); handleConfigChangeResult() logs the ccr messages
(ConfigurationHandler:491 and :655).

Nits

• Localization on the hot path — fixed. bindFailureMessage() now builds
  ERR_COULD_NOT_BIND_CHANGELOG (translated in 7 locales) and appends the diagnostic as its own
  message key, so the "port in use" text operators actually read stays translated and only the
  diagnostic sentence is English.
• ERR_COULD_NOT_STOP_LISTEN_THREAD.get(e) — fixed, getExceptionMessage(e).
• abortInitialization() — enableExternalChangeLog() moved inside the try, so a failure
  there releases the changelog backend too; externalChangelogRegistered is set before
  registerVirtualAttributeRules(), so a partial registration is released as well; and the
  changelogDB dereference is guarded, since the method now really can run on a partially
  constructed instance.
• Test residue — the aborted instance's changelog directory is removed by the test itself.
• Inconsistent result codes — left as is. CONSTRAINT_VIOLATION on a failed add is what the
  rest of the server reports (BackendConfigManager, PasswordPolicyConfigManager,
  AccessControlConfigManager, …), and it predates this PR; OPERATIONS_ERROR on the change
  path matches the other failures of applyConfigurationChange() in this same class.
• isListening() — kept. With the bind-first ordering it reports the current socket while
  the new port is being bound, which is the truth; the remaining window is two statements wide
  and test-only.
• Closing keyword vs deferred work — the deferred items get their own issues (per-invocation
  ports in AssuredReplicationServerTest and its ~8 siblings, rerunFailingTestsCount vs
  @DataProvider, and the FileChangelogDB.initializeDB() swallow), and the PR description is
  updated accordingly.

Verification

mvn -o -pl opendj-server-legacy verify -P precommit \
    -Dit.test='**/replication/**/*Test.java,**/replication/**/*TestCase.java'

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