[#911] Report the replication connections which used to be dropped in silence

[vharseko]

Fixes #911.

Three paths around the replication port gave up on a connection, or on a peer, without logging anything an administrator could act on. They are the complement of #906, which covered the inbound SSL handshake failure.

The accept loop

ReplicationServer:307 caught every exception which is not a failed SSL handshake and dropped it with no logging at all, not even a trace, so it could not be diagnosed even with the debug log enabled. What reaches there is, in particular, the ConfigException of a trust store which cannot be read: TrustStoreBackend.loadKeyStore() opens the file again on every getKeyManagers(), getTrustManagers() and containsKeyWithAlias() call, so an ads-truststore deleted, truncated or made unreadable after startup makes the server drop every inbound replication connection in silence. A SocketTimeoutException from a peer which connects and then says nothing lands there too, being no SSLException.

The failure of accept() itself is now separated from the failure to build a session, because it is a different problem: the listen socket stays open across it, so the loop came straight back to accept() and, on a process out of file descriptors, span on it at full CPU without a line in any log. The thread now waits 100 ms before accepting again, but only when the failure repeats within that window, so that a connection reset between the handshake and accept() costs the connections behind it nothing.

Two things decide what "repeats" means, and both matter:

• the window is measured from the moment the previous failure was handled, not from the moment it happened. Measured from the failure, the 100 ms wait it was granted put the next failure a whole interval away from the stamp, which read as isolated: half of a continuous run went unwaited and the spin was bounded at twice the intended rate;
• a successful accept() puts the clock back a whole interval. A loop which is serving connections is doing work rather than spinning, and without the reset a stream of connections aborted before accept() — a health check, a port scan — would charge the listen port a wait per probe, and the peers queued behind them would pay it. What that gives up is the failure which alternates with a connection: a process which frees a file descriptor now and then has the accept it lets through reset the clock, and the failure behind it is timed as isolated, so the loop turns as fast as the connections arrive. The warning is throttled either way, so the error log holds one line per five minutes of it on both sides of that trade.

Both are logged through FailureLogThrottle, which is the throttle #906 introduced for the handshake warning, extracted unchanged; ReplSessionSecurity now delegates to it and its test is untouched. Both throttles — the one of accept() and the one of the session which cannot be started on an accepted connection — are runListen locals, alongside the backoff clock and for the same reason: switchListenPort() starts the new listen thread before it stops the old one, and a five minute window opened on the port which was left would otherwise suppress the first failure on the port which replaced it.

The outgoing RS to RS connect

ReplicationServer:490 only traced the failure, so a replication server whose outgoing connection failed logged nothing in logs/errors: the problem was visible solely in the log of the peer, which is the machine whose configuration is right. Success was no more visible, INFO_REPLICATION_SERVER_CONNECTION_TO_RS being written at debug level.

The connect thread retries a failed peer every few seconds for as long as it is down, and a peer being down is a normal state, so ConnectFailureReporter reports it once per outage and per (peer, domain), and reports the connection which ends the outage as well. Three shapes of that bookkeeping needed care, and all three are covered by ConnectFailureReporterTest:

• the peer may connect to this server first, which connect() never sees; the already-connected branch of runConnect() clears the record, otherwise the next outage of that peer would be silenced;
• moving the record on and reporting the move are one step, reportConnectionRestored(), so an outage cannot be closed in the record without being closed in the log — which is how it would be reported open and never reported closed;
• a peer or a domain taken out of the configuration is never connected to again, so nothing would ever clear what was recorded for it; retainAll forgets those on every pass.

What the outage is decides what closes it, and what the handshake did decides what is reported for it. WARN_REPLICATION_SERVER_CONNECT_ERROR is reported for the socket and for the session built on it — ReplicationServerHandler.connect() throws nothing of its own, aborting a handshake it cannot complete by closing the session and returning — so an attempt which reaches the end of connect() is one whose peer answered on its replication port, and that is the outage ending. The outage is closed whatever the handshake did next, because holding it open for a peer which answers reports that peer unreachable while it is answering, and silences its next real outage. What the record holds is not whether an outage is open but what was last reported about the peer, and that is what leaves the session a peer establishes after it answered without one still reportable: a record which is merely present or absent is consumed by the message about the answer, and the handshake which completes seconds later finds nothing left to close. A peer restarting takes exactly that path -- its port answers before its domains are up.

Every narrower reading holes on a peer whose connection this server does not see under the address it dialled, and there is one for each of them:

• the registration with the domain misses a peer which negotiates protocol version 1. ReplicationServerHandler.connect() registers only above V1 — the FIXME there is older than this — so such a peer is connected and never registered;
• the address, and the session left open with it, miss a peer which dials out from an address other than the one it is configured under: multi homing, NAT. Its inbound handler is registered under the source address of its own connection, ServerHandler.toServerAddressURL() taking the host from session.getRemoteAddress(), so the already-connected branch of runConnect() compares the configured address against one it never matches — and the handshake this server offers that same peer aborts on ERR_DUPLICATE_REPLICATION_SERVER_ID, abortStart closing the session, so an open session is never seen at the end of connect() again.

A record left uncleared is not a line too few but a peer gone silent: recordFailure returns false from then on, so the next real outage of that peer is not reported at all.

The handshake is not left to report itself, because it cannot: ReplicationServerHandler.connect() aborts at seven places and three of them pass no message, abortStart logging nothing without one — a StopMsg read where the peer's ReplServerStartMsg was due, a cross connect this server resolves against a peer it is already connected to, and a phase two the peer leaves unanswered. Session.close() publishes a StopMsg for every abort a peer makes, so a peer which stops the handshake — a shutdown under way, a cross connect it resolves against us — reaches us as one of those silent aborts. Reporting the recovery as a connection would leave "connected to replication server" as the last thing said about a domain which has no session for it. The other four have their reason logged here by abortStart, and they are not all of one kind: this server rejecting the peer, its own duplicate server id check included; the peer going away while the handshake ran; and whatever else fails on this side of it.

So ReplicationServerHandler.connect() now returns whether the handshake completed, and connect() reports message 313 for a peer it is connected to and the new message 314 for one which answered and stopped the handshake — the latter saying where a reason was logged rather than naming a side: next to 314 itself where this server ended the handshake, and nothing there where the peer ended it. A side is not what it can name. Two of the four aborts which carry a reason are the peer going away rather than a rejection, and the abort which carries none may have been logged nowhere at all: the peer resolves a cross connect with the same silent abortStart(null), one line into startFromRemoteRS(). The one of the three silent aborts where a session for the domain does exist is the cross connect this server resolves, and 314 denies a session to the attempt rather than to the domain: that session is the connection the peer made, and the already-connected branch reports it on the next pass. It promises the session which follows instead of a standing topology check it cannot fire for: 314 is reported only where an outage was, so two replication servers which have been reachable since start-up and share a server id never reach it. The same outcome reaches runConnect(), which leaves a peer whose handshake aborts alone for a few passes rather than dialling it every second: the reason such a peer logs on its own side is logged about six times less often as well.

Message 312 promised that "the next message about this replication server comes when the connection is established", which is not what closes an outage; it now promises the peer being reachable again, either because this server reached it or because it connected to this server from the address it is configured under — the already-connected branch of runConnect() closes an outage for a connection connect() never dialled, and matches such a connection to the peer by the socket-derived address URL its handler is registered under, which is why the promise is not made for every inbound connection — and the troubleshooting chapter describes both endings.

The data server side

WARN_COULD_NOT_FIND_CHANGELOG and WARN_NO_AVAILABLE_CHANGELOGS name no cause, but the two are not equally blind. The cause is built for every replication server contacted and was logged only for the elected one, and none is ever elected when none of them answers: a rejected certificate, a refused connection and a wrong port then all read as "unable to connect to any replication servers". Every server contacted now reports its own cause, next to the summary which names none of them, reusing WARN_NO_CHANGELOG_SERVER_LISTENING, WARN_TIMEOUT_CONNECTING_TO_RS and WARN_EXCEPTION_STARTING_SESSION_PHASE as they stand.

connectionError bounds the volume: it is set as soon as an attempt reaches no server at all and stays set until a session is established, so the 500 ms loop which retries a total outage reports its first pass only. It is not set while the broker is connected, so the unreachable servers of a topology which still serves the broker are reported again on each reconnection — one line each, so a reconnection costs as many lines as there are servers it could not reach, where it used to cost none. A broker reconnects when its session is lost rather than on a schedule, and a server unreachable across several reconnections is one whose configuration or certificate needs looking at.

The severity says what the failure cost the broker rather than what the message is named: the elected server keeps the error it was reported with, a server which was only contacted is a warning. WARN_EXCEPTION_STARTING_SESSION_PHASE carries the whole stack trace for the elected server, as it did before this PR, and the message alone for the others — that branch is not throttled, an SSLException is not paced by a connect timeout the way a refused connection is, and collectReplicationServersInfo() reruns over every URL on each reconnection.

Where the suppressed records go

A failure the throttle keeps out of the warnings is still recorded, with the information severity. That severity is not invisible: cn=Replication Repair Logger ships enabled with ds-cfg-log-file: logs/replication and ds-cfg-override-severity: SYNC=INFO,ERROR,WARNING,NOTICE, LoggingCategoryNames maps org.opends.messages.replication to SYNC, and TextErrorLogPublisher.isEnabledFor prefers the override. So the throttle bounds what logs/errors holds, while logs/replication holds one record per failure.

Messages 105, 310 and 311 say so, and so do the sample quoting 105 and the troubleshooting chapter, whose instruction for reaching those records in the error log as well names info — the token the default-severity enumeration accepts — rather than information, which it rejects. Message 105 and its sample belong to #906; this PR had already re-worded them, and the corrected paragraph sits three lines under the sample, so leaving them would have shipped a contradiction on one page.

Two things found while there

A session which failed after its handshake leaked. The outer catch of runListen logged and looped without closing the session, so a peer killed after the handshake, or one which stopped answering past the connection timeout, cost one socket and one file descriptor for the life of the process. Closing it there is safe: startFromRemoteDS, startFromRemoteRS and connect each catch everything and abortStart, which closes the session, so a session reaching that catch is owned by nothing else.

logger.debug of a localized message does not go to the debug log. OpenDJLoggerAdapter publishes it to the error log with the information severity, which is what the section above is about.

Message ordinals

The four new messages were 307 to 310 and are now 310 to 313: #892 landed 307, 308 and 309 in replication.properties while this branch was open. No Java change was needed, the generated constants dropping the ordinal suffix. Message 314 was added in the third review round. #945 landed 319 and 320 while this branch was open as well; the merge with master put them beside 310 to 314 rather than over them. replication.properties holds no duplicate ordinal: the highest this branch adds is 314, and the highest in the file is 320.

Verified locally, on the branch merged with master

• mvn -Pprecommit -pl opendj-server-legacy verify -Dit.test='ConnectFailureReporterTest,FailureLogThrottleTest,ReplSessionSecurityTest,ReplicationBrokerConnectFailureTest,ReplicationServerConnectFailureTest,ReplicationServerDynamicConfTest,ReplicationServerTest,TopologyViewTest,HandshakeAbortGenerationIdTest,ReplicationServerFailoverTest,ReplicationServerShutdownSyncTest,DSRSShutdownSyncTest' — 81 tests, no failures, on the branch merged with master at 5d176c6. The last classes cover the interaction with [#900] Time the ReplicaOfflineMsg grace period per replica, and spend it where the message can still be forwarded #919, [#889] Keep a change the replay could not apply out of the ServerState #892, [#908] Wait for the changes being applied before a domain going down saves its ServerState #945 and [#916] Keep an update that lands during a ServerState save out of the saved flag #948, which changed shutdown(), the ServerState save and ReplicationTestCase.
• mvn -Pprecommit -pl opendj-server-legacy verify -Dit.test='ConnectFailureReporterTest,ReplicationServerConnectFailureTest,ReplicationServerDynamicConfTest,ReplicationBrokerConnectFailureTest,FailureLogThrottleTest,ReplSessionSecurityTest' — 36 tests, no failures, at c98fa94, which is the round-5 delta: the classes it changes and the ones which read the messages it rewords. The run above is what covers the rest.
• HandshakeAbortRegistrationTest (3) and GroupIdHandshakeTest (2) on their own: in a long run they collide on the administration connector port, which is the harness rather than this change.
• The precondition itself was checked by putting the earlier, empty topology message back: the case then fails on it, with the IndexOutOfBoundsException of rsInfos.get(0) in the log, where before it passed on the wrong path.
• Each mutation of the delta was applied and reverted, and each was killed but the one named at the end of this section. From the earlier rounds: the round-2 form (if (!session.closeInitiated()) around the report, return true) and reporting every peer which answers as connected each fail both abort tests, and putting either throttle of the listen loop back in a field of the server fails eachListenPassBoundsItsOwnFailures with one warning where two are due. From this one, the three applied together because each falls to a different class: the present-or-absent record — recordReachableWithoutSession consuming the entry the way recordSuccess did — fails aSessionEstablishedAfterAnAnswerWithoutOneIsReported; removing the ERR_DUPLICATE_REPLICATION_SERVER_ID throw from isAlreadyConnectedToRS fails aPeerRegisteredUnderAnotherAddressStillClosesItsOutage with expected:<1> but was:<0> while its other three assertions stay green, which is the finding that case was given; and building a FailureLogThrottle for each failure rather than holding one per listen pass fails eachListenPassBoundsItsOwnFailures with four warnings where two are due. From this round, the two applied together because they fall to different classes: reporting the answer without a session through recordConnected() at the call site of 314 — the pre-fix clearing, with the branch and the log line untouched — fails aSessionEstablishedAfterAnAnswerWithoutOneIsReported on a 313 which never arrives, the run holding the 312, the 314 and then INFO_REPLICATION_SERVER_CONNECTION_TO_RS for a session which is up; and a domain-blind recordConnected, reported.remove(peer) != null, fails aConnectionForOneDomainLeavesTheOtherDomainsOfThePeerRecorded with expected:<false> but was:<true>.

What the tests pin, and what fails without the code under them:

• ReplicationServerConnectFailureTest drives the reporting through ReplicationServer rather than through the bookkeeping alone. Two of its tests drive a peer the already-connected branch of runConnect() can report nothing about, so that connect() is the only place a recovery can come from. aPeerWhichStopsTheHandshakeStillClosesItsOutage has it answer the ReplServerStartMsg with a StopMsg; aPeerRegisteredUnderAnotherAddressStillClosesItsOutage has it complete a full inbound handshake first, registering under an address it is not configured under, so that the outbound handshake really reaches ERR_DUPLICATE_REPLICATION_SERVER_ID — the multi-homing mechanism rather than a stand-in for it. That registration is a precondition of the case rather than an assumption: the test reads getConnectedRSs() and fails unless the handler is there under the address which is not the configured one, because a handshake which ends any other way is an abort too, and message 314 is reported for every abort — the case would otherwise pass while pinning the path of the test next door. Both assert message 314, the absence of 313, and a second outage reported afterwards, which is only reachable because the first was closed: they fail if the recovery is gated on the session left open, on the registration or on the address, and they fail if an aborted handshake is reported as a connection. Those three counts are what an aborted handshake produces whichever way it aborted, so the multi-homing case asserts the ERR_DUPLICATE_REPLICATION_SERVER_ID its name is about as well, which only the registration under another address URL reaches. aPeerWhichIsDownIsReportedOnceAndItsReturnIsReported runs two real replication servers with their own connect threads, so it covers runConnect() and pins that messages 312 and 313 are emitted at all. aSessionEstablishedAfterAnAnswerWithoutOneIsReported is the peer restarting: down, then answering and stopping every handshake, then completing one, all on the socket it answers on. It pins the call site of 314 rather than the bookkeeping — reporting the answer through recordConnected() consumes the record and passes every test of ConnectFailureReporter — and it asserts where the 314 and the 313 sit in the error log rather than only how many there are: a failure to reach the peer between them would open an outage of its own, and a 313 which closes a fresh outage is what the pre-fix form produces as well.
• the accept tests fail if the backoff is granted to the first failure of a row, if it is not granted to the third, if the clock is not put back by a connection served between two failures, and if logThrottledFailure's interval and count arguments are swapped. The second of them serves a connection no session can be started on, which is also what pins message 311 and the order of its arguments; eachListenPassBoundsItsOwnFailures runs two passes of runListen() on one server, each failing once at accept() and serving two connections no session can be started on, and fails if either throttle is a field of the server rather than a local of the listen thread — and, the second connection of a pass being the one its throttle has to suppress, if a throttle is built for each failure instead;
• ReplicationBrokerConnectFailureTest asserts the severity of a cause reported for a server which was only contacted, not only its text;
• the throttle contract and that each throttle counts on its own; the bookkeeping of ConnectFailureReporter, including what retainAll forgets.

Not pinned, and left so knowingly: the call site of reportConnectionRestored inside runConnect() as opposed to the method itself — aPeerWhichIsDownIsReportedOnceAndItsReturnIsReported reports the recovery from whichever of the two call sites wins the race, and the two which pin the one in connect() drive a peer the other cannot see. Nor is the argument connect() passes: reporting every completed handshake as an answer without a session survives all four cases, because the 313 then arrives one pass later from the already-connected branch, which passes the literal true, and the counts of any case whose peer registers are the same either way. Killing that one needs a peer which completes an outbound handshake and registers nothing, which is the V1 shape — ReplicationServerHandler.connect() registers only above V1 — and what it would buy is a warning the next pass corrects.

Every test configures the peer it drives and creates the domain before anything is recorded, and the ones which record from the test thread wait a whole pass of the connect thread first: runConnect() snapshots the domains at the top of a pass and runs retainAll against that snapshot at the bottom, so a pass which started before the domain existed ends by clearing the record.

Left out on purpose

• A peer which answers and aborts every handshake is reported once, by message 314, and then reported again only when it stops answering or when a handshake completes: the state it moved to is not moved to twice. What is on the peer's side of that — a duplicate server id, an address mismatch — is logged by the peer on every attempt, now once per blacklist window rather than once a second.
• The throttles are one per listen thread rather than one per remote address or per cause, as in [#905] Warn when a replication handshake fails and document CA-signed certificates #906. A single prober can therefore claim the five minute window of a genuine failure. Per-peer keying is still the better shape and still wants its own issue.
• Per-domain reporting is degenerate, and this PR does not fix it: runConnect() increments domainTicket once per pass while the blacklist is keyed by address alone, so the first domain to fail for a peer re-blacklists it before the others are reached, and the same domain wins every pass. Message 312 and the troubleshooting chapter now say the domain named is the one the attempt was for rather than the only one affected. Keying the reporter by peer alone would report it honestly but regresses the mixed case — a peer reachable for one domain and failing for another would alternate 312 and 313 every time the blacklist expires — so real per-domain reporting wants the blacklist keyed by (address, baseDN), in its own issue. A handshake which does not complete now blacklists the peer as a failure to connect does, which it did not before this delta, so the same degeneracy covers a peer which aborts durably: the domain whose attempt aborted is the one retried, and the other domains of that peer are served by the connection it makes to this server, replication servers dialling each other both ways. The troubleshooting chapter says so.

[maximthomas]

praise: The diagnosis is right and the shape of the fix is right. Three give-up paths with three
different trigger natures get three different disciplines instead of one blanket rate limit: a time
window where the trigger is external and unbounded (accept, session setup), an edge-triggered
per-outage record where "peer is down" is a state (RS→RS connect), and the existing connectionError
latch on the DS side. Specific things worth keeping:

• Extracting #906's throttle into FailureLogThrottle instead of copy-pasting a second pair of
  AtomicLongs. Verified byte-for-byte behaviour-preserving against the original, and
  ReplSessionSecurityTest needed no edit.
• The backoff fires only on a repeated accept() failure. Better than an unconditional sleep — a
  lone ECONNABORTED no longer delays the connections queued behind it — and simpler than an
  exponential backoff, which would need reset state on the success path.
• close(session) in the outer catch of runListen is a real leak fix, and it is safe:
  startFromRemoteDS/startFromRemoteRS both terminate in catch (Exception) and never propagate,
  so a session owned by a registered handler cannot reach it. Session is declared inside the
  while, so no cross-iteration close either.
• Correcting the "logged at debug level" wording in message 105 and the troubleshooting chapter.
  logger.debug(LocalizableMessage) really does publish to the error log at INFORMATION, and the
  shipped config.ldif really does list only warning/error/notice. Good catch on someone
  else's prose.
• connectionError's set/reset points are untouched, so publish()'s message-dropping behaviour is
  not widened by a logging change. Easy to get wrong; it wasn't.

---

issue (blocking): connect() clears the outage record without connecting, so the recovery notice
it promises is never logged.

opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServer.java:718

if (connectFailures.recordSuccess(remoteServerAddress, baseDN)
    && isConnectedToRS(remoteServerAddress, baseDN))

&& evaluates left first, and recordSuccess mutates — it removes the DN from the peer's failure
set regardless of whether a connection exists.

Pass N: peer down, warning 312, whose text promises the next message comes when the connection is
established. Pass N+1: peer is back and dialled us simultaneously, so
ReplicationServerHandler.connect() takes the "Simultaneous cross connect." path —
abortStart(null); return;, no throw. connect() returns true, the record is erased,
isConnectedToRS is false, nothing is logged. Pass N+2: the already-connected branch at :744
calls recordSuccess again, gets false, and NOTE_REPLICATION_SERVER_CONNECT_RESTORED_313 never
fires. The outage opens in the log and never closes.

if (isConnectedToRS(remoteServerAddress, baseDN)
    && connectFailures.recordSuccess(remoteServerAddress, baseDN))

The comment's worry — a record nothing clears — doesn't follow: the record only survives while the
peer is never seen connected under that address, and then the outage really hasn't ended. The cost
argument is thin too: connect() runs only for peers that are not connected, and
getConnectedRSAddresses(domain) is built once per domain per pass anyway.

---

todo (blocking): the reporter needs one test that goes through ReplicationServer.

opendj-server-legacy/src/test/java/org/opends/server/replication/server/ConnectFailureReporterTest.java:23

The only mention of ReplicationServer in the file is the import. No server is constructed;
connect(), runConnect() and reportConnectionRestored() are never called. All seven tests pass
with every call site deleted — which is exactly why the blocking issue above is invisible to them.
A unit test of the nested class structurally cannot catch a bug in how the nested class is called.

---

suggestion (non-blocking): three more assertions while you are in there.

opendj-server-legacy/src/test/java/org/opends/server/replication/server/ReplicationServerDynamicConfTest.java:597
opendj-server-legacy/src/test/java/org/opends/server/replication/ReplicationBrokerConnectFailureTest.java:67

Each of these currently ships green:

• Delete if (repeated) so every accept failure sleeps 100 ms. Only acceptNanos[2] - acceptNanos[1]
  is asserted; the first interval — the one that must not wait — is never computed.
• Swap or hard-code logThrottledFailure's two count arguments. Records are matched by substring and
  severity=WARNING and then counted; no assertion reads the number out of the message text.
• Turn the new logger.warn(errorMessage) into logger.error. ReplicationBrokerConnectFailureTest
  asserts no severity at all — and that split is the decision the PR body argues for.

---

issue (non-blocking): per-(peer, baseDN) reporting is degenerate — one domain is ever reported
per peer.

opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServer.java:592

runConnect() loops domains-outer / peers-inner. domainTicket increments once per outer pass
(:614 is the only ++), so it is constant across all domains of a pass, and the blacklist is keyed
by address alone while the reporter is keyed by (address, baseDN):

if (blacklistedHosts.getOrDefault(rsAddress, 0L) > domainTicket) { continue; }   // :592
...
if (!connect(rsAddress, domain.getBaseDN())) {
  blacklistedHosts.put(rsAddress, domainTicket + 6);                             // :600
}

Peer P down, domains A/B/C: pass 0 connect(P,A) fails → warning 312 → put(P,6), B and C skipped.
Passes 1–5 skipped entirely. Pass 6 connect(P,A) fails → recordFailure returns false → no log.
After ten passes: one DN in the set, one warning line, connect() called twice, both for A. A always
fails first and re-blacklists the address before B and C are reached, and baseDNs is a plain
HashMap that doesn't change during the outage, so the same domain wins every pass. The operator
reads "could not connect … for domain A" and concludes B and C are fine.

The blacklist is pre-existing and untouched by this diff, so this is inherited, not introduced — not
worth blocking on. But the PR shouldn't ship a message implying coverage it doesn't have. Cheapest
honest fix: key the reporter by peer only and drop baseDN from messages 312/313. Real per-domain
reporting needs Map<HostPort, Map<DN, Long>>, which belongs in its own issue.

---

suggestion (non-blocking): a full stack trace at WARNING, once per contacted replication server,
on the one path left unthrottled.

opendj-server-legacy/src/main/java/org/opends/server/replication/service/ReplicationBroker.java:1164

errorMessage = WARN_EXCEPTION_STARTING_SESSION_PHASE.get(
    getServerId(), serverURL, getBaseDN(), stackTraceToSingleLineString(e));

The fourth %s of message 119 is the whole trace on one line. createClientSession (:1095) throws
SSLException, which is neither ConnectException nor SocketTimeoutException, so it lands here
with hasConnected == false and goes out through the new logger.warn at :1206. One bad
certificate among three RSs with the elected session flapping: connectionError stays false, an
SSL failure is fast (no 5 s connect timeout to pace it), and collectReplicationServersInfo() reruns
over every URL per reconnect — one full stack trace per flap cycle.

Before this PR the same trace reached logger.error only for the elected server. Pass
getExceptionMessage(e) on the warn branch and keep the trace for the elected one, or route this
message through the FailureLogThrottle the PR already adds. Every other new message here is
throttled; this one isn't.

Note the neighbouring claim does not hold and is not part of this: the new per-RS warning is not
unbounded per flap — one extra line per pass, zero while stably connected, and each flap cycle
already logs BADLY/PROPERLY_DISCONNECTED.

---

suggestion (non-blocking): the accept throttle outlives the listen thread it rate-limits.

opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServer.java:166

acceptFailures is an instance field with one unkeyed lastLogNanos; the backoff clock beside it,
lastAcceptFailureNanos (:339), is a runListen local — documented as thread-confined precisely
because a port change runs a second listen thread. Two scopes for the same event.

Server out of file descriptors, accept fails and warns at T. Admin changes the listen port at T+30s.
The new thread's first accept failure gets a negative record() and goes to logger.debug (:542) —
Severity.INFORMATION, unpublished by the shipped config. Silence on the new port for up to five
minutes, right after the admin changed it to get out of trouble.

Make the throttle a runListen local next to lastAcceptFailureNanos — same scoping the code already
chose for the backoff clock, one field-to-local move — or key it by socket address. (The mechanism is
the 5-minute state surviving the handover, not the microsecond thread overlap; and the record does name
the new port correctly, so the operator is under-informed rather than misled.)

---

nitpick (non-blocking): the new warn branch publishes an ERR_-named message at WARNING.

opendj-server-legacy/src/main/java/org/opends/server/replication/service/ReplicationBroker.java:1206

The comment above says contacted servers are reported as "the warnings their messages are named for".
Three of the four errorMessage sources are WARN_; ERR_DS_DN_DOES_NOT_MATCH at :1124 returns
without setting hasConnected (only :1141 does), so it reaches the finally with
keepSession == false and goes out at WARNING. It's the only ERR_-named message that can.

A base-DN mismatch is a permanent misconfiguration, not a transient — it's the one condition here
that won't fix itself, and it's the one getting the softer severity. Either log it at error like its
name says, or amend the comment to say the branch is severity-by-role rather than
severity-by-message-name.

Not a downgrade, to be clear: before this PR that path logged nothing above trace.

---

todo (non-blocking): the doc's dsconfig instruction names a value the configuration rejects.

opendj-doc-generated-ref/src/main/asciidoc/admin-guide/chap-troubleshooting.adoc:799

add information to the default-severity property of the error log publisher to see them

The enumeration is {all, debug, error, info, none, notice, warning}. information is not in it, so
dsconfig set-log-publisher-prop --add default-severity:information is rejected — and this is the
only actionable instruction the PR gives for reaching the suppressed records.

The token is info. It does map to Severity.INFORMATION (Severity.java:43 declares
INFORMATION("INFO"), and TextErrorLogPublisher:426 resolves it via
Severity.parseString(defSev.name())), so the mechanism is right and only the word is wrong. The
surrounding prose and messages 105/310/311 are correct as they stand — they name the severity, not a
config value.

[vharseko]

Addressed in 8b3c4af. Both blocking items are fixed, and the diagnosis behind the first one turned out to reach further than the ordering.

issue (blocking): connect() clears the outage without connecting

Confirmed, including the pass N+2 half: reportConnectionRestored finds nothing left, so the outage opens in the log and never closes. ReplicationServerHandler.connect() reaches abortStart(null); return; without throwing, and recordSuccess mutates, so it was clearing the record of a peer the pass did not connect to.

I did not take the operand swap as written, because keying on the registration has a hole of its own that swapping makes permanent. ReplicationServerHandler.connect() calls replicationServerDomain.register(this) only inside the getProtocolVersion() > REPLICATION_PROTOCOL_V1 branch — the FIXME: i think this should be done for all protocol version !! at ReplicationServerHandler.java:242 is older than this PR. A peer which negotiates V1 is therefore connected and never registered, so isConnectedToRS is false for it forever: with the swap, its record is never cleared, recordFailure returns false from then on, and every outage of that peer after the first is silenced. Same shape for a peer whose handler advertises an address which does not resolve to the configured one — the multi-homing case the HostPort javadoc names its own FIXME for. The old order did not have that hole; it paid for it with the bug you found.

What separates the two states without either hole is the session. abortStart closes it (ServerHandler.java:231) and a handshake which completed leaves it open, so:

if (!session.closeInitiated())
{
  reportConnectionRestored(remoteServerAddress, baseDN);
}

This does what you asked on the path you described — abort, session closed, record kept, and the recovery reported by the already-connected branch a pass later — and it also clears the record for the V1 and multi-homed peers, where the registration never arrives. It costs no name lookup on the success path, which was the other half of the argument in the comment you were reading, and it let isConnectedToRS go away entirely and the tail of connect() call reportConnectionRestored instead of repeating its message and three arguments.

Tell me if you would rather have the swap as written; I have no attachment to the discriminator, only to not trading one silence for another.

todo (blocking): a test that goes through ReplicationServer

Correct — all eight cases passed with every call site deleted. New ReplicationServerConnectFailureTest, two tests:

• aHandshakeThePeerAbortsLeavesTheOutageOpen drives connect() (now package private) against a fake peer which answers the ReplServerStartMsg with a StopMsg, which is the abort path from your pass N+1. Peer down, peer aborting, peer down again: exactly one message 312 and no 313. Without the fix it fails with expected:<1> but was:<2> — the second 312 is the cleared record.
• aPeerWhichIsDownIsReportedOnceAndItsReturnIsReported runs two real replication servers with their own connect threads, so it goes through runConnect() and reportConnectionRestored(), and pins that 312 and 313 are emitted at all — which the PR body admitted was untested.

Both configure the peer they drive and create the domain before anything is recorded, and the first waits a whole pass of the connect thread: runConnect() snapshots the domains at the top of a pass and runs retainAll against that snapshot at the bottom, so a pass which started before the domain existed ends by clearing the record. That cost me a flaky first version which failed on the same assertion as the bug.

suggestion (non-blocking): three more assertions

All three landed green as you said, and all three now fail on the mutation:

mutation	now fails with
delete if (repeated)	104661 microseconds passed between the first attempt and the second
swap logThrottledFailure's two count arguments	the record must read every 5 minutes and (N since the previous warning)
logger.warn(errorMessage) → logger.error	[the cause of a replication server which was only contacted is a warning]

Reading the counts out of the message text turned up a fourth thing, below.

issue (non-blocking): per-(peer, baseDN) reporting is degenerate

The mechanism is exactly as you describe: domainTicket increments once per pass at :614, the blacklist is keyed by address, so connect(P, A) failing re-blacklists P before B and C are reached, and the same domain wins every pass.

I did not key the reporter by peer and drop the domain, because that regresses the mixed case. If P is reachable for domain A and fails for domain B — connect(P, A) succeeded, so A stays in connectedRSAddresses — then with peer-only keying recordFailure(P) from B and reportConnectionRestored(P) from the already-connected branch of A alternate, and the log gets a 312/313 pair every time the blacklist expires. The current Map<HostPort, Set<DN>> is stable there. So I fixed what the message claims instead: 312 now says the domain named is the one the attempt was for and that an unreachable peer is not tried for its other domains while it cannot be reached, and the troubleshooting chapter says the same. Real per-domain reporting still wants its own issue, and it wants the blacklist keyed by (address, baseDN) more than it wants a different map.

suggestion (non-blocking): a full stack trace at WARNING on the unthrottled path

Fixed. keepSession is already the discriminator the finally uses, so it picks the detail at build time:

errorMessage = WARN_EXCEPTION_STARTING_SESSION_PHASE.get(getServerId(), serverURL, getBaseDN(),
    keepSession ? stackTraceToSingleLineString(e) : getExceptionMessage(e));

The elected server keeps the trace it carried before this PR; the ones only contacted get the message, and traceException(e) above still puts the trace in the trace log. I also corrected the comment's volume claim while I was there — it said "as often as the reconnection itself is reported", but it is one line per unreachable server per reconnection, not one line per reconnection.

suggestion (non-blocking): the accept throttle outlives the listen thread

Fixed — acceptFailures is now a runListen local next to lastAcceptFailureNanos, passed to handleAcceptFailure, exactly the scoping the backoff clock already had.

Two more things on that path, both mine:

• The backoff was granted to every second failure of a continuous run. lastAcceptFailureNanos was stamped at the failure, so after a 100 ms sleep the next failure was 100 ms away from the stamp and read as isolated. Measured: the third failure of a row waited 552 µs instead of 100 ms, so the spin was bounded at twice the intended rate. It is now stamped after the failure is handled, and the accept test runs four failures so that two waits in a row are measured.
• A successful accept() now puts that clock back a whole interval. Without it, a stream of connections aborted between the handshake and accept() — a health check, a port scan — would charge the listen port a wait per probe and make the peers queued behind them pay it.
• socket.getLocalSocketAddress() is read before the isClosed() guard rather than after it: a socket closed in between made the warning name the port as null.

nitpick (non-blocking): an ERR_-named message at WARNING

The reading is right — ERR_DS_DN_DOES_NOT_MATCH is set at :1124 without setting hasConnected, which is only set at :1141, so it is the one ERR_ which can reach the warn branch.

I took the second option and amended the comment rather than special-casing the message. The branch's rule is severity by role — the server this broker is electing is the one it cannot work without, so its failure is an error; a server which was only contacted is a warning because the broker found another one. Adding an exception would make the rule "by role, except this one message", which is harder to keep true than to state. A base DN mismatch on the elected server still goes out as an error, and on a contacted one the path logged nothing above trace before this PR.

todo (non-blocking): the doc names a value the configuration rejects

Fixed: info, not information. The enumeration in ErrorLogPublisherConfiguration.xml is {all, none, error, info, warning, notice, debug} and Severity.parseString("INFO") resolves to INFORMATION, so the mechanism was right and only the token was wrong.

Found while re-reading that paragraph: the throttle bounds logs/errors, not logs/replication

The wording those three messages share — "the others are recorded with the information severity, which the error log does not publish by default" — is true of logs/errors and misleading about everything else. cn=Replication Repair Logger ships enabled with

ds-cfg-log-file: logs/replication
ds-cfg-default-severity: none
ds-cfg-override-severity: SYNC=INFO,ERROR,WARNING,NOTICE

LoggingCategoryNames maps org.opends.messages.replication to SYNC, and TextErrorLogPublisher.isEnabledFor prefers definedSeverities.get("SYNC"), which holds INFORMATION. So the suppressed records are published, to logs/replication, one line per failure — and the paragraph telling the reader to add info to the error log publisher "to see them" sits three lines under the line which says replication has its own log file. That is the contradiction on one page this PR set out to avoid, shipped by the PR itself.

Messages 105, 310 and 311 now say the information severity is "which the replication log publishes and the error log does not", the sample quoting 105 was rewrapped to match, the troubleshooting paragraph says where the records are before it says how to get them into the error log as well, and the ReplSessionSecurity javadoc which still said "at debug level" was corrected the same way. Message 105 and its sample are #906's, but this PR had already re-worded them, so leaving them would have shipped the contradiction under a corrected line.

Verified

mvn -Pprecommit -pl opendj-server-legacy verify over 15 classes — 85 tests, no failures:

• FailureLogThrottleTest, ReplSessionSecurityTest, ConnectFailureReporterTest, ReplicationServerConnectFailureTest, ReplicationBrokerConnectFailureTest, ReplicationServerDynamicConfTest
• CryptoManagerTestCase, GroupIdHandshakeTest, ReplicationServerFailoverTest, TopologyViewTest, HandshakeAbortGenerationIdTest, HandshakeAbortRegistrationTest, ReplicationServerShutdownSyncTest, ReplicationServerTest, DSRSShutdownSyncTest

HandshakeAbortRegistrationTest and GroupIdHandshakeTest were run on their own: in a long run they collide on the administration connector port, which is the harness rather than this change.

What the new tests pin, and what they do not: the abort test fails on the ordering, the accept test fails on any of the three mutations above and on the backoff timing, and the two-server test fails if 312 or 313 stops being emitted. Still not pinned: message 311, and the call site of reportConnectionRestored inside runConnect() as opposed to the method itself.

[maximthomas]

praise: The second round answers every item, and three of the answers are better than what was
asked for. Verified this round:

• The V1 half of the argument for reading the session instead of the registration is correct.
  ReplicationServerHandler.connect() registers only inside the > REPLICATION_PROTOCOL_V1 branch
  (ReplicationServerHandler.java:242), so keying on the registration really would have left a V1
  peer's record uncleared for good. Declining the operand swap was right.
• keepSession ? stackTraceToSingleLineString(e) : getExceptionMessage(e) reuses the discriminator
  the finally already has — no new state, and the elected server keeps the trace it had.
• Stamping the backoff clock after the failure is handled rather than at the failure is a real
  defect found and fixed unprompted, and the accept test now runs four failures so two consecutive
  waits are measured rather than one.
• Moving acceptFailures into runListen is the right scoping: switchListenPort() calls
  startListenThread() at :1055 before stopListenThread() at :1065, so the two listen threads
  genuinely overlap.
• The logs/replication finding is the best thing in the round. cn=Replication Repair Logger
  ships with ds-cfg-override-severity: SYNC=INFO,..., so the suppressed records are published;
  correcting 105/310/311 rather than shipping the contradiction was the right call even though 105
  is #906's.
• The mutation table holds. if (true) at ReplicationServer.java:760 reproduces verbatim:
  aHandshakeThePeerAbortsLeavesTheOutageOpen:208, expected:<1> but was:<2>.
• Message ordinals are clean: nothing renumbered or reused, and the placeholder count, type and
  order of 105/310/311/312/313 match every call site.
• The 158 lines added to ReplicationTestCase are purely additive across its 49 subclasses — no
  field, no @BeforeClass, nothing widened.
• The accept timing assertion is not tight: firstIntervalNanos measured at 299/365/539 us against
  a 100 ms bound over three invocations.

---

issue (blocking): on a multi-homed or NAT peer the record is now never cleared, so the peer's
next real outage is not logged at all.

opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServer.java:760

An outage is closed in two places: !session.closeInitiated() here, and the already-connected
branch of runConnect() at :601. That branch compares the configured rsAddress against
HostPort.valueOf(rsHandler.getServerAddressURL()), and the URL is socket-derived:

// ServerHandler.toServerAddressURL
new HostPort(session.getRemoteAddress().getHost(), port)   // host from plainSocket.getInetAddress()

The inbound handler therefore learns the peer's source IP, the outbound one the IP we dialled,
and HostPort.equals compares normalizedHost = InetAddress.getByName(host) — the first address
only. isEquivalentTo (getAllByName, overlap) exists but is not used here.

Peer P is configured as A1 and dials out from A2. P goes down, 312 is recorded. P returns and its
inbound connection wins the race, so startFromRemoteRS registers it as A2:port. From then on,
every pass:

• runConnect():601 misses (A2 vs A1), so the restore is never reported there;
• connect() runs, and ReplicationServerHandler.connect(DN, boolean):188 finds the handler but
  "A2:port".equals("A1:port") is false, so isAlreadyConnectedToRS throws
  ERR_DUPLICATE_REPLICATION_SERVER_ID — caught at :265, abortStart, session.close()
  (ServerHandler.java:231), closeInitiated() true, :760 does not clear;
• connect() returns true, so no blacklist, and this repeats about once a second.

recordSuccess is reachable only from :607 and :762, both blocked; retainAll keeps the key
while the peer stays configured. The next real outage hits recordFailure at :828, which returns
false, and message 312 is never logged again. Round 1's code (f17fd3ee2f:718) evaluated
recordSuccess first and did clear the record here, so this half is a regression introduced by
the delta rather than an inherited defect.

The comment at :755-758 asserts the opposite for exactly this case — "and its session is open
here as well". In the multi-homing shape the session at :760 is closed.

Cheapest fix is to drop the second condition. connect() returns without throwing only after
createClientSession and the start-message exchange succeeded, so the peer is provably reachable
and the outage is over whatever the handshake did next:

if (connectFailures.recordSuccess(remoteServerAddress, baseDN))
{
  logger.info(NOTE_REPLICATION_SERVER_CONNECT_RESTORED, getServerId(), remoteServerAddress, baseDN);
}

That covers the round-1 bug, the V1 peer and the multi-homed peer at once, and depends on neither
the registration nor an address match. Keeping "an aborted handshake is not a connection" instead
means fixing runConnect():601 to use isEquivalentTo — more expensive, and still holed on NAT.

---

todo (blocking): the two new tests pin only the negative direction.

opendj-server-legacy/src/test/java/org/opends/server/replication/server/ReplicationServerConnectFailureTest.java:204

Measured at 8b3c4afb52 with
mvn -Pprecommit -pl opendj-server-legacy verify -Dit.test=ReplicationServerConnectFailureTest:

mutation of ReplicationServer.java:760	result
if (true) — the pre-fix behaviour	killed: :208, expected:<1> but was:<2>
if (false) — connect() can never report a restore	survived, both tests green
the round-1 form isConnectedToRS(...) && recordSuccess(...)	survived, both tests green

So the suite certifies one proposition — an aborted handshake must not clear the record — and
cannot tell the shipped design from the one it replaced. if (false) survives because
aHandshakeThePeerAborts... asserts the restored count is zero, and aPeerWhichIsDown... takes its
single 313 from runConnect():607 instead. The round-1 form survives because both peers in that
test are real V2+ servers that do register. Every argument the design rests on — V1 peers,
multi-homed advertised addresses — is untested, and the issue above lives in that gap: broken and
working code pass identically.

Two cases close it, and both are small:

1. Continue aHandshakeThePeerAbortsLeavesTheOutageOpen with the same fake peer completing the
   handshake on a third connection, and assert exactly one 313. That peer is not configured as an
   RS, so runConnect():607 cannot reach it and only connect() can report — which kills
   if (false) and closes the call site you listed as unpinned.
2. Have the fake peer answer with a ReplServerStartMsg naming an address other than the one it was
   dialled on. That alone makes connectedRSAddresses miss, separates the two designs, and doubles
   as the regression test for the issue above.

---

issue (non-blocking): round-1 item 7 was fixed for one of the two throttles.

opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServer.java:166

acceptFailures became a runListen local; sessionSetupFailures, added by this same PR, is still
an instance field. Its only reader is logSessionSetupFailure() at :523, whose only caller is
runListen() at :392 — used per listen thread, scoped per server. Since switchListenPort()
starts the new thread at :1055 before stopping the old one at :1065, the scenario the comment at
:345 describes applies verbatim: an ads-truststore made unreadable warns 311 at T, the admin
changes the listen port at T+30s, and the new thread's first failure is suppressed by the window
opened on the abandoned port — up to five minutes of silence in logs/errors right after the port
change. One field-to-local move, exactly as handleAcceptFailure already takes its throttle.

Worth noting while you are there: git grep sessionSetupFailures\|WARN_REPLICATION_SERVER_SESSION_SETUP_ERROR
over opendj-server-legacy/src returns the two main-source lines and the properties entry and
nothing else, so 311's argument order and the throttle have no test at all.

---

suggestion (non-blocking): the reset on a successful accept() reverses the rule the previous
round endorsed, without saying so.

opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServer.java:376

// A connection served is not a spin: the failures before it stop pacing the loop.
handledAcceptFailureNanos = System.nanoTime() - ACCEPT_FAILURE_BACKOFF_NANOS;

The comment this delta deleted argued the other way: "a socket which frees a descriptor now and
then lets an accept through between two failures, and counting only consecutive ones would let that
alternation spin". The new line is that alternation: under descriptor exhaustion a freed descriptor
lets one connection through, repeated is false at the next failure, and the 100 ms is never taken
while the throttle still holds the error log to one line per five minutes.

The trade is defensible — a port scan should not charge the listen port a wait per probe — but the
reply presents it as pure gain. Nothing tests it either way: the only runListen test installs a
socket whose accept() throws on every call (ReplicationServerDynamicConfTest:568), so line :376
never executes and can be deleted or inverted with no assertion moving. Either say in the comment
what the reset gives up, or let the accept test succeed once between two failures and pin that no
wait follows it.

[vharseko]

Addressed in 8a38745. Both blocking items are fixed — the first by taking the fix as written rather than by repairing the discriminator, and the second is what shows the first is not round 1 coming back.

issue (blocking): the record is never cleared for a multi-homed peer

Confirmed, and confirmed as mine rather than inherited: f17fd3ee2f:718 evaluated recordSuccess first and did clear the record on that path, 8b3c4afb52 does not.

The chain holds link by link. ServerHandler.toServerAddressURL() builds the host from session.getRemoteAddress(), so an inbound handler is registered under the source address of the connection the peer made. getConnectedRSAddresses() compares that with HostPort.equals, which compares normalizedHost — InetAddress.getByName(host), the first address only (HostPort:349) — while isEquivalentTo, the getAllByName overlap, exists and is not used there. And isAlreadyConnectedToRS throws ERR_DUPLICATE_REPLICATION_SERVER_ID (ReplicationServerDomain:1318) in exactly the case where the server ids match and the two address URLs do not, which lands in catch (DirectoryException) → abortStart(e.getMessageObject()) → session.close(). So recordSuccess was unreachable from both of its call sites, retainAll kept the key, and the next real outage of that peer hit a recordFailure which returns false.

The comment at :755-758 was wrong where you say it is, and worse than wrong — it fused two peers into one sentence. "A peer whose handler advertises an address which does not resolve to the configured one … is the same shape, and its session is open here as well": the V1 peer's session is open here, which is why reading the session survived it, and the multi-homed peer's is not. They are the same shape only with respect to the registration.

Took the fix as written; the second condition is gone.

reportConnectionRestored(remoteServerAddress, baseDN);
return true;

This does not bring round 1's bug back, and the reason is structural rather than a promise. Round 1's bug was a record cleared without the recovery being logged — recordSuccess() mutating and then && isConnectedToRS() short-circuiting. reportConnectionRestored() clears the record and logs 313 in one step, so at this call site "cleared but not reported" is not expressible any more.

What makes it right rather than merely safe is the symmetry with the message it closes. WARN_REPLICATION_SERVER_CONNECT_ERROR is reported from the catch around the socket, createClientSession and rsHandler.connect(), and the last of those throws nothing — it catches everything and abortStarts. So 312 is emitted exactly when the socket or the session could not be built, and 313 now closes it exactly when they could. The handshake's outcome is in neither predicate, which is what it was doing wrong: it decided the fate of a message it is not the subject of. What the handshake did next has messages of its own, and abortStart logs the ones worth logging.

Two costs, on the record rather than found later:

• a peer which answers and always aborts — a genuine duplicate server id — is reported restored once and then goes quiet, while abortStart logs the duplicate id every second, unthrottled. That is master's behaviour and the first bullet of "Left out on purpose";
• a peer alternating between refusing connections and aborting handshakes pays a 312/313 pair per cycle where it used to pay one 312. A peer flapping that way is worth the lines.

todo (blocking): the two new tests pin only the negative direction

Confirmed; your table reproduces. if (false) surviving is the half that mattered — the suite certified one proposition and could not tell the shipped design from the one it replaced.

aHandshakeThePeerAbortsLeavesTheOutageOpen is gone as such, the fix inverting what it asserted. It is now aPeerWhichStopsTheHandshakeStillClosesItsOutage: same fake peer, assertions the other way up — one 313, and two 312, the second outage being reportable only because the first was closed. That is the silence the bug causes, read as a count rather than as an absence.

The second case is aPeerWhichAdvertisesAnotherAddressStillClosesItsOutage, as you asked: the fake peer answers a well-formed ReplServerStartMsg naming a port it is not listening on and leaves phase 2 unanswered. That is the multi-homing mismatch without a second address to bind, and it runs through processStartFromRemote, isAlreadyConnectedToRS and the entry to phase 2, none of which the StopMsg peer reaches.

Measured with mvn -Pprecommit -pl opendj-server-legacy verify -Dit.test=…, each mutation applied on its own and reverted:

mutation of ReplicationServer	result
if (!session.closeInitiated()) back around the call — the shipped round-2 form	killed: both new tests, 313 never arrives
the call deleted — connect() can never report a restore	killed: both new tests, 313 never arrives
the reset at :376 deleted — a connection no longer stops pacing the loop	killed: listenThreadDoesNotWaitAfterAFailureWhichFollowedAConnection, "108248 microseconds passed between the third attempt and the fourth"

Both failures of the first two read the same way, which is the point of the shape: "… connected to replication server 127.0.0.1:65525 for domain "o=test", which it had reported it could not connect to" should have been logged within 30000 ms, but the error log received 2 records. The record which is not there is the one the operator does not get.

Not taken: continuing the abort test with the same peer completing the handshake on a third connection. Under the fix there is no third state left to reach — the StopMsg connection is the one which closes the outage, so it already kills if (false) and pins the call site inside connect(), that peer registering with no domain so runConnect():607 can report nothing about it. A completing handshake would drive the same line to the same assertion.

issue (non-blocking): round-1 item 7 was fixed for one of the two throttles

Fixed. sessionSetupFailures is a runListen local now, passed to logSessionSetupFailure the way handleAcceptFailure already took its own, and the comment above the pair names switchListenPort() starting the new thread before it stops the old one.

Message 311 has a test as well, which it did not: the new accept test below serves one connection no session can be started on, and reads the record back — the peer address rather than the listen port, and which of the two numbers is the interval and which is the count.

suggestion (non-blocking): the reset reverses the rule the previous round endorsed

Took both halves rather than one.

The comment now says what the reset gives up: under descriptor exhaustion a freed descriptor lets one connection through, the failure behind it is timed as isolated, and the loop turns as fast as the connections arrive — against which the warning stays throttled to one line per five minutes either way.

And listenThreadDoesNotWaitAfterAFailureWhichFollowedAConnection pins it: accept() fails, serves one connection, fails again, and the fourth attempt follows the third without a wait. The connection served is a socket connected and then closed, so setTcpNoDelay fails at once where a socket connected to nothing would spend the whole connection timeout inside the SSL handshake — and it still names the peer, which is what makes it the 311 test too.

Verified

mvn -Pprecommit -pl opendj-server-legacy verify over the six classes this touches — FailureLogThrottleTest, ReplSessionSecurityTest, ConnectFailureReporterTest, ReplicationServerConnectFailureTest, ReplicationBrokerConnectFailureTest, ReplicationServerDynamicConfTest — 28 tests, no failures.

[maximthomas]

praise: The fix is taken where it should be. reportConnectionRestored() clearing the record and logging 313 in one step makes "cleared but not reported" inexpressible at that call site — the round-1 bug is structurally gone, not merely avoided.

The mutation table reproduces. Measured in an isolated worktree at 8a38745d36, mvn -Pprecommit -pl opendj-server-legacy -am -Dit.test=ReplicationServerConnectFailureTest verify: baseline 3 cases pass; reportConnectionRestored(...) deleted — killed, 2 of 3 cases; the round-2 form if (!session.closeInitiated()) restored — killed, the same 2 cases, verbatim the same assertion text. The mutant that survived at the round-2 head is dead. Round-2 blocker 2 is closed.

listenThreadDoesNotWaitAfterAFailureWhichFollowedAConnection holds on both claims. Its record filter "accepted a connection from " + servedAddress belongs only to message 311 — 310 reads "could not accept a connection on" — and the address asserted is the served peer's, not the listen port, so substituting the listener's address gives zero matches. contains("every 5 minutes") && contains("(0 since the previous warning)") really does separate the interval argument from the count. And deleting the reset does make attempt 3 read as repeated, sleep 100 ms, and fail with the line quoted.

sessionSetupFailures is a runListen local now, and the comment above the pair names switchListenPort() starting the new thread before it stops the old. The reset comment names what it gives up. Both were asked for and both landed.

---

issue (blocking): When the peer aborts the handshake, the server logs "connected" and then goes silent for good.

opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServer.java:780

Session.close() publishes a StopMsg unconditionally — opendj-server-legacy/src/main/java/org/opends/server/replication/protocol/Session.java:84 sets protocolVersion = getCurrentVersion() and :190 gates only on localSessionError == null && protocolVersion >= V4. So any peer-side abort reaches us as a StopMsg: read before the peer's ReplServerStartMsg it lands on ReplicationServerHandler.java:175, read after it leaves waitAndProcessTopoFromRemoteRS() null and lands on :232. Both pass null, and abortStart(null) logs nothing (opendj-server-legacy/src/main/java/org/opends/server/replication/server/ServerHandler.java:218-225).

The pass then falls through to the unconditional report. A peer that rejects our handshake — its own duplicate-id check, a cross connect it resolves against us, a shutdown in progress — yields one 312, one 313 connected to replication server X for domain "o=test", and then nothing ever again: connect() returned true, so the peer is never blacklisted, and every later pass calls recordSuccess() on an already-cleared record, which returns false and logs nothing. Replication for that domain is dead and the log's last word is "connected".

The disclosed cost leans on "abortStart logs the duplicate id every second, unthrottled". That holds only for our-side aborts (ReplicationServerHandler.java:262, :267, non-null message). On the peer-side abort there is no line at all — and aPeerWhichStopsTheHandshakeStillClosesItsOutage pins that as intended.

The third abortStart(null) site, ReplicationServerHandler.java:191, is not affected: isAlreadyConnectedToRS returns true only when connectedRSs really holds a matching handler, so an inbound connection is up, 313 is true and clearing is correct.

A direction that avoids both discriminators already holed (the session, the registration): let the handshake tell its caller whether it aborted.

// ReplicationServerHandler.connect(...) -> false when it called abortStart()
if (!rsHandler.connect(baseDN, sslEncryption))
{
  // the outage stays truthfully open, and runConnect() blacklists the peer for six passes
  return false;
}
reportConnectionRestored(remoteServerAddress, baseDN);
return true;

A V1 peer completes its handshake, so the V1 hole stays shut. Under multi-homing the abort is ours and carries a non-null message, which is logged.

---

todo (non-blocking): aPeerWhichAdvertisesAnotherAddressStillClosesItsOutage never reaches the mismatch it is named for.

opendj-server-legacy/src/test/java/org/opends/server/replication/server/ReplicationServerConnectFailureTest.java:192

connectedRSs' only writer is ReplicationServerDomain.register() (:2572), called from ReplicationServerHandler.java:242 only above V1 and only after a non-null waitAndProcessTopoFromRemoteRS() — which this peer prevents by leaving phase 2 unanswered. So isAlreadyConnectedToRS takes its oldRsHandler == null -> return false guard (opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerDomain.java:1299-1304) and the address comparison at :1306 never executes. The advertised port is read on no executed path; the case would pass byte-for-byte with the real port advertised.

Measured: both mutants fail both new cases with identical assertion text, so this case kills nothing aPeerWhichStopsTheHandshakeStillClosesItsOutage does not. The multi-homing mechanism — round 2's blocker — stays unpinned. Either drop the case as redundant, or make it real: put a handler for PEER_RS_ID into connectedRSs under a different address URL before the outbound handshake, so isAlreadyConnectedToRS throws ERR_DUPLICATE_REPLICATION_SERVER_ID. Fixing the blocking item above will invert what both cases assert anyway.

---

todo (non-blocking): The sessionSetupFailures move is pinned by no test — restoring the field is a surviving mutant.

opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServer.java:360

runListen has three callers: ReplicationServerListenThread.java:60 and the two test invocations at opendj-server-legacy/src/test/java/org/opends/server/replication/server/ReplicationServerDynamicConfTest.java:585 and :741. Both run once on a freshly constructed server, where the field and the local start equally empty, so the first 311 warns under either form. The asymmetry the move fixes needs two overlapping runListen calls on one instance. Two sequential calls on one server discriminate it:

// same ReplicationServer instance, two listen passes, one unusable connection each
rs.runListen(firstFailingSocket);
rs.runListen(secondFailingSocket);
// instance-field form: the second warning is suppressed by the five-minute window
assertThat(countRecordsOf(records, "accepted a connection from")).isEqualTo(2);

Not measured — the runner's three-run cap went to the connect() call site.

---

issue (non-blocking): Messages 312 and 313 still assert a connection the code no longer guarantees.

opendj-server-legacy/src/messages/org/opends/messages/replication.properties:641, :646

Both keys were authored under the !session.closeInitiated() guard and not revisited when it went: the file's diff over this delta is empty, and in BASE..HEAD both are pure additions. 312 says "This is reported once: the next message about this replication server comes when the connection is established"; 313 says "connected to replication server %s for domain "%s", which it had reported it could not connect to". The delta redefined the closing event to "the peer answered on its replication port".

On the silent sites above, 313 is the only line and it is false. On the logging sites the abort ERROR is emitted immediately before the 313, so the operator gets two adjacent contradictory lines, and anything keyed on msgID 313 reads the link as up. 312 itself nominates 313 as the completion event, so the contract is stated in the log, not only in the docs.

Mostly downstream of the blocking item: if 313 fires only for a completed handshake, both texts are true again. If the unconditional report stays deliberately, rewrite both to say what happened — "answered on its replication port", not "connected" — and check whether opendj-doc-generated-ref/src/main/asciidoc/admin-guide/chap-troubleshooting.adoc repeats the old wording.

---

nitpick (non-blocking): The shared helper's javadoc states a safety property the code does not have.

opendj-server-legacy/src/test/java/org/opends/server/replication/server/ReplicationServerConnectFailureTest.java:228-229 — "The failure which opens each of the two windows blacklists the peer for six passes as well". blacklistedHosts is a Map local to runConnect() (ReplicationServer.java:595), written only at :636-639 from the connect thread's own connect(). The rs.connect(...) calls the test makes from the test thread blacklist nothing. The real protection is ConnectFailureReporter's idempotence in both directions, plus waitConnections() aligning the pass so the end-of-pass retainAll cannot erase the record — worth saying, since that is what makes the exact counts stable.

---

nitpick (non-blocking): A vacuous assertion.

opendj-server-legacy/src/test/java/org/opends/server/replication/server/ReplicationServerDynamicConfTest.java:142 — assertEquals(ReplicationServer.getAllInstances().size(), instancesBefore) cannot fail: allInstances.add(this) is the last statement of the constructor (ReplicationServer.java:278), after catch (ConfigException e) { abortInitialization(); throw e; } (:266-272), and abortInitialization() never touches allInstances. The sibling assertion on the changelog virtual-attribute rules is the one carrying the test. localPorts is added on the same post-catch line and asserted nowhere — that one could catch something:

assertFalse(ReplicationServer.isLocalReplicationServerPort(ports[1]));

---

nitpick (non-blocking): The comment and the PR body name the wrong comparison.

opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerDomain.java:1306 compares getServerAddressURL() with String.equals, not with HostPort.equals/normalizedHost; the HostPort path is used by getConnectedRSAddresses (ReplicationServer.java:678, ReplicationServerDomain.java:1002), not here. No reachable hole today — both sides are socket-derived numeric literals — but the reasoning quoted names a comparison this method does not perform.

[vharseko]

Addressed in 58ea112. The blocking item is fixed, though not with the discriminator it proposed: that one reinstates the blocker of round 2, and the reason it does is the substance of this round.

issue (blocking): a peer-side abort is reported as a connection

Confirmed link by link. Session.close() publishes a StopMsg whenever localSessionError == null and the version is at least V4 (Session.java:190), and the version starts at the current one (:84), so every abort a peer makes reaches us as one — before its ReplServerStartMsg it lands on ReplicationServerHandler.java:175, after it, it leaves waitAndProcessTopoFromRemoteRS() null and lands on :232. Both pass null, and abortStart(null) logs nothing (ServerHandler.java:211-226). connect() then returned true, so the peer was not blacklisted either. And you are right that the cost I put on the record — "abortStart logs the duplicate id every second, unthrottled" — holds only for the aborts this server makes; on the peer-side abort there is no line at all, and aPeerWhichStopsTheHandshakeStillClosesItsOutage pinned that silence as intended.

I did not take if (!rsHandler.connect(...)) { return false; } as written, because gating the record on the handshake is round 2's blocker again, in the very case round 2 blocked on. Multi-homed peer P registered as A2, dialled at A1: isAlreadyConnectedToRS throws ERR_DUPLICATE_REPLICATION_SERVER_ID — an abort of ours, so connect() returns false, recordSuccess is never reached, the already-connected branch of runConnect() misses (A2 against A1), retainAll keeps the key, and the next real outage of P hits a recordFailure which returns false. The duplicate id being logged does not close that gap: what goes missing is message 312 for an outage which happens later, and no line about the abort says anything about it.

What the two rounds ask for is two different things, so the fix does two things rather than picking one:

handshakeCompleted = rsHandler.connect(baseDN, sslEncryption);
...
reportConnectionRestored(remoteServerAddress, baseDN, handshakeCompleted);
return handshakeCompleted;

• the record is cleared whenever the peer answered on its replication port, which is what the outage was about — round 2's blocker stays closed, and so does round 1's, reportConnectionRestored() still clearing and reporting in one step;
• the message is chosen by the handshake: 313 for a peer this server is connected to, and a new message 314 for one which answered and stopped the handshake, which names where the reason is — the error log of the peer — and what to check there, that no two replication servers of the topology share a server id;
• the outcome reaches runConnect(), so the abort is blacklisted for six passes like an unreachable peer. That is the part your version had and mine would otherwise have lost: a peer which aborts every handshake was dialled every second, and the reason it logs on its own side is now logged about six times less often.

The V1 peer completes its handshake, so it takes the 313 branch and stays out of this.

issue (non-blocking): 312 and 313 assert a connection the code does not guarantee

Fixed, and no longer downstream of anything: 313 is reported only for a handshake which completed, so its text is true as it stands. 312 promised "the next message about this replication server comes when the connection is established" and now promises the answer on the replication port which actually closes the outage. The troubleshooting chapter said the same and now describes both endings, the one with a session and the one without.

todo (non-blocking): aPeerWhichAdvertisesAnotherAddressStillClosesItsOutage never reaches the mismatch

Confirmed. connectedRSs is empty for that peer, so isAlreadyConnectedToRS takes the oldRsHandler == null guard and :1306 never runs; the advertised port is parsed by toServerAddressURL() and then read by nothing, so the case would indeed pass with the real port. Made real rather than dropped, as you suggested: aPeerRegisteredUnderAnotherAddressStillClosesItsOutage has the fake peer dial this server first and complete the inbound handshake — start message, topology message, topology message back — which registers it under the address its start message names. The outbound handshake offered to it afterwards then reaches ERR_DUPLICATE_REPLICATION_SERVER_ID for real, and the peer is registered before it starts answering on the configured address, so the connect thread of the server runs into the same registration rather than into the earlier abort.

Both abort tests now assert three things: one 314, zero 313, and two 312 — the second outage being reportable only because the first was closed.

todo (non-blocking): the sessionSetupFailures move is pinned by no test

Confirmed, and it was true of acceptFailures as well, which round 2 endorsed on the argument alone. eachListenPassBoundsItsOwnFailures runs two passes of runListen() on one server, each serving one connection no session can be built on and then failing accept() once before it closes its socket — the failure which closes the socket returns from handleAcceptFailure(), so a pass has to fail once before that one. Two warnings of each kind are due, and one of each arrives with either throttle put back in a field.

nitpick (non-blocking): the helper's javadoc claims a blacklist it does not get

Right — blacklistedHosts is local to runConnect() and written only from its own failures. The javadoc now says what actually keeps the counts exact: ConnectFailureReporter is idempotent in both directions, and waitConnections() aligns the pass so the end-of-pass retainAll cannot erase the record the assertions are about.

nitpick (non-blocking): the vacuous assertion in replServerFailsWhenListenPortIsInUse

The reading is right — allInstances.add(this) is the last statement of the constructor, after the catch which rethrows, and abortInitialization() never touches it, so the assertion cannot fail. It is not part of this delta, though: neither the test nor that line appears in 92d88ca..HEAD. Recording it rather than fixing it here, and it wants portHolder.getLocalPort() rather than ports[1], which that test does not have — it calls findFreePorts(1).

nitpick (non-blocking): the comment names the wrong comparison

isAlreadyConnectedToRS does compare getServerAddressURL() with String.equals, and nothing in the delta says otherwise: HostPort.equals/normalizedHost is named for getConnectedRSAddresses, which is where it is used, and the abort on a duplicate server id is named separately, both in the comment at ReplicationServer.java and in the PR body. git grep normalizedHost over the diff returns nothing. Nothing changed for it, so flagging it rather than arguing it.

Verified

mvn -Pprecommit -pl opendj-server-legacy verify over the classes this touches and the ones around them — ConnectFailureReporterTest, FailureLogThrottleTest, ReplSessionSecurityTest, ReplicationBrokerConnectFailureTest, ReplicationServerConnectFailureTest, ReplicationServerDynamicConfTest, ReplicationServerTest, TopologyViewTest, HandshakeAbortGenerationIdTest, ReplicationServerFailoverTest, ReplicationServerShutdownSyncTest, DSRSShutdownSyncTest — 61 tests, no failures, plus HandshakeAbortRegistrationTest (3) and GroupIdHandshakeTest (2) on their own.

Each mutation applied on its own and reverted:

mutation	result
the round-2 form: if (!session.closeInitiated()) around the report, return true	killed: both abort tests, the second 312 never arrives
report every peer which answers as connected (313 for an abort)	killed: both abort tests, 314 never arrives
sessionSetupFailures back in a field of the server	killed: eachListenPassBoundsItsOwnFailures, 1 warning where 2 are due
acceptFailures back in a field of the server	killed: the same test, the same shape

One thing seen and not chased: under the two killed mutations, where a test waits its full 30 s while the connect thread repeats the duplicate-id abort, the error log picks up severity=ERROR msgID=-1 msg=Index 0 out of bounds for length 0. It does not appear in any green run, and it is on the repeated-abort path rather than in anything this delta touches, so it looks like a defect of its own rather than something to fold in here.

---

Correction

The last paragraph above is wrong, and so was the test it was defending. Index 0 out of bounds for length 0 is not a defect of the repeated-abort path: it is the fake peer of aPeerRegisteredUnderAnotherAddressStillClosesItsOutage sending a TopologyMsg with no RSInfo, which waitAndProcessTopoFromRemoteRS() reads as rsInfos.get(0) above protocol version 4 (ReplicationServerHandler.java:460, "List should only contain RS info for sender"). The inbound handshake ended there, catch (Exception) in startFromRemoteRS turned it into abortStart(LocalizableMessage.raw(...)) -- which is where the msgID=-1 record comes from -- and register() was never reached.

So the outbound handshake which followed aborted on the unanswered second phase rather than on ERR_DUPLICATE_REPLICATION_SERVER_ID: the case pinned the path of aPeerWhichStopsTheHandshakeStillClosesItsOutage while claiming the multi-homing one. That is the finding of this round -- "never reaches the mismatch it is named for" -- reproduced in the replacement I wrote for it. The assertions passed either way, message 314 being reported for any abort.

Fixed in 06961e3d47: the peer sends its own RSInfo, and the registration is waited for and asserted -- waitForRegistrationUnder() reads getConnectedRSs() and checks both that the handler is there and that its address URL is the one which is not configured -- before the outbound handshake is offered.

Measured, -Dit.test=ReplicationServerConnectFailureTest each time:

run	result
fixed	3 cases pass, zero Index 0 out of bounds records
the empty rsInfos put back	fails on the precondition, "the peer should have registered with the domain", 4 such records in the log
the round-2 form	killed, both abort cases, the multi-homing one now on connect() returning true
every peer which answers reported connected	killed, both abort cases, 314 never arrives

What let me miss it, and what I will not repeat: the error log records of these tests reach the build output only inside a failed assertion's text, so "absent from the green runs" was an artefact of the runs being green, not evidence about the server.

[maximthomas]

praise: the round-3 blocker is closed, and closed with the right discriminator.

ReplicationServerHandler.connect() now returns the handshake outcome, so a peer that answers and
aborts no longer produces a lone false "connected". Measured on this head:

• ReplicationServerConnectFailureTest passes at 06961e3d47, 3 cases, 0 failures.
• Clearing the outage record whenever the peer answered at all is the right call, and it is pinned:
  the mutant "log 314 but do not clear the record" is killed, 2 of 3 cases, both on
  expected:<2> but was:<1> for the second 312. That mutant is in neither of your tables — your tests
  are stronger than your own evidence shows.
• Message 314 makes "reachable, no session" a state an operator can read, instead of hiding it under 313.
• Moving both FailureLogThrottles and handledAcceptFailureNanos from fields to runListen() locals
  closes the listen-port-change silence and is what makes the new third caller safe — grep finds no
  FailureLogThrottle field left in the class.
• chap-troubleshooting.adoc no longer promises an established connection; it names 313 and 314 and
  describes each.
• The self-correction about the replacement test in your round-3 reply was right, and saved a round.

---

issue (blocking): after a 314 the recovery is never reported — the WARN stays the operator's last
word about a connected peer.

opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServer.java:815

Both branches sit under one guard, and recordSuccess() (:888) removes the baseDN before either runs:

if (connectFailures.recordSuccess(remoteServerAddress, baseDN))   // removes the baseDN
{
  if (connected) { logger.info(NOTE_REPLICATION_SERVER_CONNECT_RESTORED, ...); }   // 313
  else           { logger.warn(WARN_REPLICATION_SERVER_REACHABLE_NO_SESSION, ...); } // 314 — record now gone
}

Peer restart is the ordinary path into this: port refused → 312; port open but the peer's domain is
not up yet, so it answers with a StopMsg → abortStart(null) → 314; seconds later the handshake
completes → recordSuccess() returns false → no 313. The only line on a completed handshake is
logger.debug(INFO_REPLICATION_SERVER_CONNECTION_TO_RS) (ReplicationServerHandler.java:261), which the
error log does not publish. Log-based alerting is left holding an ERROR and a WARN saying
"no change is replicated over this connection" over a connection that is replicating.

Not a regression of round 2's blocker: a later genuine outage still logs 312 (recordFailure at :742).

Fix — keep the clearing (the mutant above proves it is load-bearing), give ConnectFailureReporter a
last-reported state per peer instead of a present/absent record:

enum Reported { DOWN, REACHABLE_NO_SESSION, CONNECTED }
// 312 -> DOWN; abort -> WARN 314 only when the previous state was DOWN;
// completed handshake -> NOTE 313 whenever the previous state was DOWN or REACHABLE_NO_SESSION.

That also stops a peer aborting every pass from emitting a 314 each time.

---

issue (blocking): 314's text does not match when 314 fires, in either direction. Round-3 finding 4
is not closed — it moved into the new message.

opendj-server-legacy/src/messages/org/opends/messages/replication.properties:648

314 is emitted for the raw handshakeCompleted == false, and ReplicationServerHandler.connect()
(:154-286) has 7 abortStart sites, all reaching it — null at :185, :201, :242; non-null at
:191, :273, :279 (the local ERR_DUPLICATE_REPLICATION_SERVER_ID), :285.

1. The 4 non-null aborts log their reason on this server —
   ServerHandler.java:224, logger.error(reason) — yet 314 says
   "check the error log of the peer, where the reason it rejected the connection is logged". The reason
   is already in the log being read.
2. 314 can only fire when a 312 record stands, and the sole recordFailure is inside the catch at
   :742; an abort throws nothing. Two RSs that have been reachable since start-up and share a server id
   therefore produce no 312 and no 314 at all — while 314's text is a standing topology diagnostic
   ("check that no two replication servers of the topology share a server id"). The code is
   outage-scoped, the message is not.

Text, not control flow: gate the peer-log sentence on the null-reason aborts (pass the reason's presence
down instead of the bare boolean), or drop the topology sentence from a message that cannot fire in the
topology it names.

Also ReplicationServer.java:783: "two of the three aborts of ReplicationServerHandler.connect() pass
no message" — it is 3 of 7, and the 4 omitted are exactly the ones that make the text wrong.

---

issue (blocking): 312's rewritten text over-promises in a new direction.

opendj-server-legacy/src/messages/org/opends/messages/replication.properties:641 now says
"the next message about this replication server comes when it answers again on its replication port".
The already-connected branch clears the record and logs 313 for a connection the peer made inbound,
which connect() never dialled — as its own comment says:

// ReplicationServer.java:616-621
// Skip: already connected. The connection may be the one that peer made to
// this server, which connect() never sees, ...
reportConnectionRestored(rsAddress, domain.getBaseDN(), true);

One-line edit, bundled here because the string is about to be released and translated.

---

issue (blocking): the replacement multi-homing case reaches the duplicate-id path but asserts nothing
that only that path produces.

opendj-server-legacy/src/test/java/org/opends/server/replication/server/ReplicationServerConnectFailureTest.java:288

The three assertions (one 314, zero 313, two 312) are string-for-string the sibling helper's, and 314 is
emitted for any handshakeCompleted == false. Remove the duplicate-id throw from
isAlreadyConnectedToRS and the handshake runs on to phase two, where the fake peer sends no
TopologyMsg → abortStart(null) at ReplicationServerHandler.java:232 → still one 314, zero 313,
two 312, all green. waitForRegistrationUnder asserts the setup, not that the outbound handshake
consumed it. Third appearance of this shape in this file.

One line closes it — the discriminator is clean, the StopMsg path logs nothing and the error-log capture
is server-wide:

assertThat(countRecordsOf(records, ERR_DUPLICATE_REPLICATION_SERVER_ID.get(...)))
    .as("the outbound handshake reached the duplicate server id, not phase two")
    .isEqualTo(1);

---

suggestion (non-blocking): blacklistedHosts is keyed by host but armed by a per-domain handshake
outcome.

opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServer.java:632

if (blacklistedHosts.getOrDefault(rsAddress, 0L) > domainTicket) { continue; }
...
if (!connect(rsAddress, domain.getBaseDN())) { blacklistedHosts.put(rsAddress, domainTicket + 6); }

domainTicket is constant within a pass, so on the pass the entry expires the first durably-aborting
domain re-arms it and every domain iterated after it is skipped again — permanently, not "a few
iterations". New in this delta: before it an aborted handshake returned true and blacklisted nothing.
Bounded in practice (RS↔RS dialling is bidirectional, and a duplicate server id aborts every domain
symmetrically), so: key by (host, baseDN), or state the permanence in the adoc, which currently
discloses only the cross-domain scope.

---

suggestion (non-blocking): the logSessionSetupFailure call site is pinned only at unit level.

eachListenPassBoundsItsOwnFailures (ReplicationServerDynamicConfTest.java:806) makes exactly one
session-setup failure per pass, so a mutant that builds a fresh FailureLogThrottle per failure passes
it unchanged. The acceptFailures call site is pinned — DCT:657 warnings == 1, DCT:659
suppressed == 2. FailureLogThrottleTest covers the class, not the wiring.

---

note (non-blocking): both mutation tables in the round-3 reply credit assertions that are never
evaluated.

Under "the round-2 form, return true" the earlier assertThat(rs.connect(...)).isFalse() fires first;
under "report every peer which answers as connected" the waitForErrorLogRecord for the 314 record spins
its timeout and ends at ReplicationTestCase.java:1018 fail(...). Either way "zero 313" and "two 312" are
never reached, so "both abort tests now assert three things" is true of what is written but unsupported
by the table offered. No code action — the measured mutant in the praise section settles the underlying
question in your favour; worth knowing so the tables are not cited later as proving more than they do.

---

note (non-blocking): the unthrottled ERR_DUPLICATE_REPLICATION_SERVER_ID flood a multi-homed peer
causes is pre-existing (present at base 92d88ca699) and is now tracked as
#1017. It is not charged to this PR — this
delta mitigates it roughly 7x by blacklisting the peer for six passes.

[vharseko]

Addressed in 1b33496 and 100e683, on top of a merge with master (de7fd87) — #945 landed 319 and 320 in replication.properties while this branch was open, and they merged in beside 310 to 314 rather than over them.

All four blocking items are fixed. The blacklist suggestion I took as documentation rather than as a rekey, with the reasoning below.

issue (blocking): after a 314 the recovery is never reported

Confirmed, including the severity half: recordSuccess() was domains.remove(baseDN), present-or-absent with no state, so the 314 branch consumed the record and the handshake which completed seconds later returned false from it. The only line on the completed handshake is logger.debug(INFO_REPLICATION_SERVER_CONNECTION_TO_RS) at ReplicationServerHandler.java:261, which is the information severity — logs/replication publishes it, logs/errors does not — so the error log's last word about a replicating connection really was a WARN saying no change was replicated over it.

Taken as you wrote it. ConnectFailureReporter now holds the last state which was reported rather than whether an outage is open:

private enum Reported { DOWN, REACHABLE_NO_SESSION }
// recordFailure                    -> 312 unless the state is already DOWN
// recordReachableWithoutSession    -> 314 only from DOWN
// recordConnected                  -> 313 from either, and drops the entry

recordConnected dropping the entry rather than storing a third state keeps retainAll and the "nothing was reported, so nothing is reported" case exactly as they were, and it is what makes a peer which was never reported about still cost no line.

One correction to the rationale rather than to the fix: "That also stops a peer aborting every pass from emitting a 314 each time" was already true before this change — the clearing form consumed the record on the first abort, so the second found nothing and returned false. The enum keeps that property, it does not add it. What it adds is the 313 afterwards.

Mutation, measured at 100e683832 with mvn -Pprecommit -pl opendj-server-legacy verify -Dit.test=ConnectFailureReporterTest,ReplicationServerConnectFailureTest,ReplicationServerDynamicConfTest:

mutant	result
recordReachableWithoutSession consuming the entry, the way recordSuccess did	killed: aSessionEstablishedAfterAnAnswerWithoutOneIsReported

Four more cases pin the rest of the contract: the answer without a session is reported only out of an outage, only once, a peer which goes down after one is reported again, and aPeerFailingAgainAfterAConnectionIsReportedAgain still holds through recordConnected. ConnectFailureReporterTest is 13 cases now.

issue (blocking): 314's text does not match when 314 fires

Confirmed, and the count is yours: connect() spans :154 to :292 and aborts at seven places — 185, 201, 242 with no message, 191, 273, 279, 285 with one — so abortStart logs the reason on this server for four of them, including its own ERR_DUPLICATE_REPLICATION_SERVER_ID. The comment at ReplicationServer.java:783 said "two of the three" and now names the seven and describes the three silent ones; the PR body carried the same wrong count and is corrected too.

I took the second of your two options — drop the topology sentence — rather than plumbing the reason's presence down. Passing it down would let 314 name the side per abort, but it widens ReplicationServerHandler.connect()'s return a second time in three rounds for a distinction the text can carry directly, and the standing-topology half of the finding is not fixed by it: 314 is reachable only where a 312 record stands, so two servers reachable since start-up which share a server id still produce neither, whatever 314 says. The text now names both sides and promises what the code can deliver:

Where this server rejected the peer, the reason is logged next to this message; where the peer stopped the handshake it logged nothing here, and the reason is in the error log of the peer, which stops a handshake when it is shutting down and when it resolves a simultaneous connection against this server. Until a handshake completes, no change is replicated over this connection, and the connection which completes one is reported in its turn.

The last clause is only true because of the item above.

issue (blocking): 312's rewritten text over-promises in a new direction

Fixed. 312 now says the next message comes "when it can be reached again, whether it answers on its replication port or connects to this server itself", which is what the already-connected branch of runConnect() closes an outage for. 313 said the peer "had reported it could not connect to", which is not what was reported about one that answered without a session; it now says "had reported it had no replication session with", true after both 312 and 314.

issue (blocking): the multi-homing case asserts nothing only that path produces

Confirmed and fixed. The three counts are string-for-string the sibling's because 314 is reported for any handshakeCompleted == false, so the case now asserts the whole ERR_DUPLICATE_REPLICATION_SERVER_ID, built with the real arguments — both address URLs and the peer's server id — which only the registration under another address URL reaches:

assertThat(countRecordsOf(records, ERR_DUPLICATE_REPLICATION_SERVER_ID.get(
        servers[0].getMonitorInstanceName(), registeredAs, peerAddress, PEER_RS_ID).toString()))
    .as("the outbound handshake should have aborted on the duplicate server id ...")
    .isEqualTo(1);

mutant	result
the duplicate-id throw removed from isAlreadyConnectedToRS	killed: expected:<1> but was:<0> — and the other three assertions stayed green, which is exactly the shape you were naming

suggestion (non-blocking): blacklistedHosts is keyed by host but armed by a per-domain handshake outcome

The premise checks out on both halves: at the base 92d88ca699, connect() ignored rsHandler.connect()'s result and returned true, so an aborted handshake blacklisted nothing — this delta introduced that — and since domainTicket is constant within a pass, the first durably-aborting domain re-arms the entry and every domain iterated after it is skipped for as long as the condition lasts.

I took the adoc option, and I do not think the rekey is the right change here — it would pay for the case this delta introduced with the case that was already the common one.

runConnect() is a single thread dialling every peer for every domain, serially. connect() gives the socket MultimasterReplication.getConnectionTimeoutMS(), 5000 ms by default (MultimasterReplication.java:124), and a peer whose packets are dropped rather than refused — a firewall, a host which went away, the case the blacklist exists for — spends all of it. The pass itself is paced at 1000 + random(100) ms (ReplicationServer.java:658) and the blacklist runs six tickets, so today one down peer costs the connect thread one 5 s dial per six passes, whatever the number of domains.

Keyed by (host, baseDN), that becomes one dial per domain: three domains is 15 s of one thread inside connect() on the pass the entries expire, and during it no other peer is dialled at all. So the rekey would delay every other peer's reconnection by 5 s per domain per cycle in order to keep dialling a peer which has already answered the same way for every domain it serves — and the reason it is bounded in practice is the one you named: RS↔RS dialling is bidirectional, and an abort which is peer-wide (a duplicate server id, a shutdown) aborts every domain symmetrically, so the domains skipped are served by the connection the peer makes to this server.

Round 1 also settled that real per-domain reporting wants its own issue, and it wants (address, baseDN) on the blacklist as part of a change which reconsiders the cadence with it, not on its own. So the chapter now says what the bound costs:

The domain whose attempt failed is also the one retried, so for as long as the condition lasts the other domains of that peer are not dialled from this server again -- they are served by the connection the peer makes to this server, replication servers dialling each other both ways.

"Left out on purpose" says the same, and now names the abort case as this delta's rather than as inherited.

If you read the 5 s × domains stall as acceptable, say so and I will take the rekey — but I would want it as a deliberate trade rather than as a tidier key, because that is what it buys.

suggestion (non-blocking): the logSessionSetupFailure call site is pinned only at unit level

Fixed. eachListenPassBoundsItsOwnFailures serves two connections per pass instead of one, the second being the one the pass's throttle has to suppress, and asserts both the two warnings and the four records:

mutant	result
a FailureLogThrottle built for each failure instead of one held per listen pass	killed: expected [2] but found [4] warnings

The accept() call site keeps the assertions it had.

note: the mutation tables credited assertions never evaluated

Taken, and it stands — under both of those mutants the earlier assertThat(rs.connect(...)).isFalse() or the waitForErrorLogRecord timeout ends the case before the counts are read, so "asserts three things" described the source rather than the run. The tables in the body are rewritten to name, for each mutant, the assertion which actually fails.

note: #1017

Noted, nothing charged here.

Verified

mvn -Pprecommit -pl opendj-server-legacy verify -Dit.test='ConnectFailureReporterTest,FailureLogThrottleTest,ReplSessionSecurityTest,ReplicationBrokerConnectFailureTest,ReplicationServerConnectFailureTest,ReplicationServerDynamicConfTest,ReplicationServerTest,TopologyViewTest,HandshakeAbortGenerationIdTest,ReplicationServerFailoverTest,ReplicationServerShutdownSyncTest,DSRSShutdownSyncTest' — 81 tests, no failures, on the branch merged with master at 5d176c6915. The shutdown classes are up from the earlier round because #945 and #948 added to them; running them is what covers this delta against the ServerState save they changed.

The three mutants above were applied together and reverted, each falling to a different class, so none of them masks another.

[maximthomas]

The Reported enum is the right shape: holding the last state reported rather than whether an outage is open is what lets a 313 follow a 314, and dropping the entry in recordConnected keeps retainAll and the never-reported peer exactly as they were. Three more things landed well — the correction to your own rationale ("the clearing form already suppressed the repeated 314; the enum adds the 313 afterwards") instead of letting the claim stand; aPeerWhichAdvertisesAnotherAddressStillClosesItsOutage now asserting the whole ERR_DUPLICATE_REPLICATION_SERVER_ID built with the real arguments, which is the one thing only that path reaches; and the mutation tables rewritten to name the assertion that actually fails. eachListenPassBoundsItsOwnFailures serving two connections per pass is a real strengthening, and the seven-abort count is now right in the comment, in the message and in the body.

issue (blocking): the fix is pinned by no test — a one-line mutant at its call site survives all 28 cases

opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServer.java:830

Restoring the pre-fix clearing semantics at the 314 call site, branch condition and log line untouched:

// ReplicationServer.java:830, the 314 branch, argument list unchanged:
-    connectFailures.recordReachableWithoutSession(...)
+    connectFailures.recordConnected(...)

survives — 28 cases, 0 failures (ConnectFailureReporterTest 13, ReplicationServerConnectFailureTest 3, ReplicationServerDynamicConfTest 12), measured at 100e683832 with your own invocation, 6:32 wall. That mutant is the blocking item this round was for: after a 314 the entry is gone and the handshake which completes seconds later logs no 313.

ConnectFailureReporterTest never constructs a ReplicationServer and never calls connect(), so its 13 cases pin the class, not the call site. The only integration coverage is the 3 cases of ReplicationServerConnectFailureTest, and none of them drives 312 -> 314 -> a completed handshake.

Two more mutants survive:

// ReplicationServer.java:793 — handshakeCompleted -> false: survives 28/28

// ConnectFailureReporter.recordConnected, made domain-blind: survives 13/13
    return reported.remove(peer) != null;

No unit case uses a second domain of the same peer. The consequence of the domain-blind form is not silence — recordFailure is keyed on the previous value, not on presence — but a repeated 312 plus a spurious 313 on every pass.

Fix: one integration case through connect() covering 312 -> 314 -> success, and one unit case with a second domain.

issue (blocking): 314's two sides do not cover three of the seven aborts

opendj-server-legacy/src/messages/org/opends/messages/replication.properties (message 314), mirrored at opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServer.java:787 and in opendj-doc-generated-ref/src/main/asciidoc/admin-guide/chap-troubleshooting.adoc:795.

Where this server rejected the peer, the reason is logged next to this message; where the peer stopped the handshake it logged nothing here, and the reason is in the error log of the peer

• :273 (IOException) and :285 (Exception) are the peer going away, and the reason is logged here — so "in the error log of the peer" is false for them, and :787's "the remaining four are this server rejecting the peer" counts them on the wrong side.
• :201 (isAlreadyConnectedToRS) is on neither side, and there 314 claims no session while a live inbound session replicates that domain.
• The simultaneous-connection case the sentence names by hand is that same :201 shape, so the text is wrong about the one case it singles out.

Text only, but the ordinal is permanent once released. Restricting the peer-side promise to the two silent aborts (:185, :242) and fixing the count at :787 is enough.

issue (blocking): "the last message about a peer is what is true of it now" is not true

opendj-doc-generated-ref/src/main/asciidoc/admin-guide/chap-troubleshooting.adoc:795

recordConnected drops the entry, so a session lost afterwards while the peer's listener still answers produces no 31x at all: connect() throws nothing, so no 312, and ServerReader logs the loss at information — logs/replication publishes it, logs/errors does not. The last word in the error log stays 313 "connected" while nothing is replicated. That is the silence this PR exists to remove, promoted to an invariant.

The same line still says two replication servers sharing a server id "is reported that way", which needs a standing 312 record — a limit you state in this round's reply while the chapter keeps the claim.

issue (blocking): 312 promises the inbound direction unconditionally

opendj-server-legacy/src/messages/org/opends/messages/replication.properties (message 312)

...whether it answers on its replication port or connects to this server itself

The inbound half closes an outage only through runConnect()'s already-connected branch, which needs a registered handler — registration happens above protocol V1 only — whose socket-derived address URL matches. A V1 peer and a multi-homed or NAT'd peer connect to this server and 313 never comes. Same permanent-ordinal cost as the item above; fix in the same pass.

issue (non-blocking): the new duplicate-id assertion races this server's connect thread

opendj-server-legacy/src/test/java/org/opends/server/replication/server/ReplicationServerConnectFailureTest.java:308

assertThat(countRecordsOf(records, ERR_DUPLICATE_REPLICATION_SERVER_ID.get(
        servers[0].getMonitorInstanceName(), registeredAs, peerAddress, PEER_RS_ID).toString()))
    .isEqualTo(1);

waitConnections() rides one pass, it never stops the connect thread, and blacklistedHosts gives about 6.0-6.6 s of headroom against a 30 s REPORT_TIMEOUT_MS. A second outbound dial inside the window makes the count 2 and the case red. isGreaterThanOrEqualTo(1), or an upper bound on the wait, removes the race without weakening what the assertion proves.

suggestion (non-blocking): the chapter carries half of the blacklist trade-off, and the rekey question is still open

opendj-doc-generated-ref/src/main/asciidoc/admin-guide/chap-troubleshooting.adoc:795

they are served by the connection the peer makes to this server

That holds for a peer which is up and dials back — not for one which is down, which is the very case you cite as the reason not to rekey. The line documents the per-domain cost and leaves the down-peer half of your reasoning in the reply only.

On the rekey itself: your cost argument is sound and this is your call. If you keep the documentation answer, say in the chapter that the skipped domains are covered only where the peer dials back.

[vharseko]

Addressed in c98fa94. All four blocking items are fixed, and two of the findings do not hold as
stated: the answer to each says which.

issue (blocking): the fix is pinned by no test

Confirmed, all three mutants, and the first is the round-4 blocker itself: reporting the answer
without a session through recordConnected() consumes the entry, and the handshake which completes
seconds later reports nothing. ConnectFailureReporterTest calls the class, so it cannot see the
call site, and none of the three integration cases had a completed handshake after a 314.

aSessionEstablishedAfterAnAnswerWithoutOneIsReported is that path: the peer is down (312), answers
and stops every handshake (314), and then its domains come up and the same socket completes the
handshake (313). One socket throughout, so the outage is never reopened in between -- asserted
rather than assumed: the case reads where the 314 and the 313 sit in the error log and fails if any
312 sits between them. A 313 which closes a fresh outage is what the pre-fix form produces as well,
so counts alone would pin nothing. aConnectionForOneDomainLeavesTheOtherDomainsOfThePeerRecorded
is the second domain of one peer, for recordConnected.

Measured at c98fa94 with mvn -Pprecommit -pl opendj-server-legacy verify -Dit.test=ConnectFailureReporterTest,ReplicationServerConnectFailureTest:

mutant	result
recordReachableWithoutSession(...) -> recordConnected(...) at the 314 call site	killed: aSessionEstablishedAfterAnAnswerWithoutOneIsReported, the 313 never logged -- the run has the 312, the 314 and then msgID 205 at information for a session which is up
recordConnected made domain blind, reported.remove(peer) != null	killed: aConnectionForOneDomainLeavesTheOtherDomainsOfThePeerRecorded, expected:<false> but was:<true> on the failure of the other domain
handshakeCompleted -> false	survives, 4 of 4 green, and is left surviving

The third stays unpinned on purpose, and it is worth saying why rather than leaving it out of the
table. Under it every completed outbound handshake reports 314, and the 313 arrives one pass later
from the already connected branch of runConnect(), which passes the literal true: the counts of
any case whose peer registers are the same either way. Killing it needs a peer which completes an
outbound handshake and registers nothing, which is the V1 shape -- ReplicationServerHandler.connect()
registers only above V1. Driving a fake peer at protocol version 1 is a negotiation this file does
not otherwise make, and what it would buy is a warning the next pass corrects.

issue (blocking): 314's two sides do not cover three of the seven aborts

Confirmed on all three counts, and the case you name by hand is worse than the finding says: the
peer resolves a cross connect with abortStart(null) as well -- ReplicationServerHandler.java:311,
in startFromRemoteRS() -- so for that abort the reason is in neither log, and 314 sent the operator
to the peer's for it.

314 no longer names a side. It says where a reason was logged: next to the message where this server
ended the handshake, whether it rejected the peer or the peer went away while it ran, and nothing
next to it where the peer ended it -- which the peer may have logged nothing about either. and no replication session was established is now this attempt established no replication session, which
is what :201 leaves true: the session for that domain is the connection the peer made, and the
already connected branch reports it on the next pass. The comment at ReplicationServer.java:788
names the four the same way, and so does the javadoc of ReplicationServerHandler.connect(), which
carried the same "two of the three aborts" as the comment corrected last round -- in the file the
count is about.

issue (blocking): "the last message about a peer is what is true of it now"

The invariant is gone from the chapter: 313 drops the entry, so an answer without a session after it
is not reported again until the peer cannot be reached at all.

The premise under it does not hold, though, and it decides whether this needs a third state.
ServerReader logs the loss of a session through logger.info(LocalizableMessage) -- :181
NOTE_READER_NULL_MSG, :216 NOTE_READER_EXCEPTION -- and OpenDJLoggerAdapter:218-220
publishes info at Severity.NOTICE, which the shipped error log publisher publishes
(config.ldif:788-790: warning, error, notice). So a session lost while the peer's listener still
answers is not silent in logs/errors; what is true is that these messages do not report it. The
chapter says that now, and names where the loss is logged instead. That is also why the entry stays
dropped: a third state would report from CONNECTED what the reader already reports, and would
report the ordinary cross connect of two servers starting together as a warning.

The shared server id half is fixed in the same sentence: the reason is logged where the handshake
was ended whether or not a 314 accompanies it, and two servers which have been reachable since they
started never opened an outage for one to follow.

issue (blocking): 312 promises the inbound direction unconditionally

Fixed: 312 now says the next message comes when the peer can be reached again, "either because this
server reached it or because it connected to this server from the address it is configured under",
which is what the already connected branch can match.

The V1 half does not hold: a V1 peer dialled from here completes the handshake -- phase two is
skipped at REPLICATION_PROTOCOL_V1, finalizeStart() runs, connect() returns true -- so its
recovery is reported by 313 from connect(). What V1 costs is the registration, which is the hole
the recovery is deliberately not read from. The multi-homed and NAT'd peer is the real one, and
there the message which comes is 314 rather than 313.

issue (non-blocking): the new duplicate id assertion races this server's connect thread

Taken: isGreaterThanOrEqualTo(1). What the assertion separates is the path -- one record against
none -- and the connect thread dials the same peer while it is answering.

suggestion (non-blocking): the chapter carries half of the blacklist trade-off

Taken as documentation, as last round: the chapter now says the skipped domains are served by the
connection the peer makes to this server only where the peer is up to make it, and that a peer which
is down serves none of its domains anyway. The rekey stays out for the reason given last round.

Verified

mvn -Pprecommit -pl opendj-server-legacy verify -Dit.test='ConnectFailureReporterTest, ReplicationServerConnectFailureTest,ReplicationServerDynamicConfTest,ReplicationBrokerConnectFailureTest, FailureLogThrottleTest,ReplSessionSecurityTest' -- 36 tests, no failures. The three mutants above
were applied and reverted, two of them together since they fall to different classes.