Skip to content

[#1017] Recognise a replication server by the address it names, not by the one its session came from - #1020

Merged
vharseko merged 4 commits into
OpenIdentityPlatform:masterfrom
vharseko:feature/multi-homed-peer-recognition
Sep 23, 2026
Merged

vharseko merged 4 commits into
OpenIdentityPlatform:masterfrom
vharseko:feature/multi-homed-peer-recognition

Conversation

@vharseko

@vharseko vharseko commented Sep 10, 2026

Copy link
Copy Markdown
Member

Fixes #1017.

What was wrong

A remote replication server was identified by the address the socket of one of its sessions happened to carry: ServerHandler.toServerAddressURL() takes the host of a handler from session.getRemoteAddress() and its port from the start message that handler received. That address is the interface a connection used, not an identity, so a peer reachable at more than one address — a multi homed host, or a NAT where the address a peer connects from is not the address it is configured as — is registered under an address nothing in the configuration ever names.

Three places read that address, and all three were wrong for such a peer:

  • runConnect() skipped a configured peer only when a handler was registered under that exact address, so it dialled a peer it was already connected to on every pass, about once a second, for as long as that peer stayed where it was;
  • isAlreadyConnectedToRS() compared the address URLs of the two handlers as strings, so the second session with a peer already connected read as two replication servers sharing a server id — a misconfiguration this topology does not have;
  • stopReplicationServers() compared the addresses removed from ds-cfg-replication-server against the registered one, so a peer an administrator took out of the topology kept its session and went on replicating. This one is not in the issue, which is about the log volume; it is the same defect at a third call site and it is a functional one, so it is fixed here rather than left to be found again.

Which message the flood carries depends on the peer, and the issue names only one of the two. The inbound path checks isAlreadyConnectedToRS() before it answers (ReplicationServerHandler.startFromRemoteRS(), ahead of its sendStartToRemote()), so a peer which already holds a handler for this server never answers the start message it is offered: the dialling side then reads EOF and logs ERR_RS_DISCONNECTED_DURING_HANDSHAKE (the IOException branch of ReplicationServerHandler.connect()), while ERR_DUPLICATE_REPLICATION_SERVER_ID is logged by whichever end has the mismatched pair of addresses. Both are ERR, neither is throttled, and both are one line per pass.

The fix

A remote server is now known by both of the addresses it comes with — the one it names in its start message, which is the address it is configured under, and the one its session came from — and either of them identifies it. ReplicationServer.setServerURL() builds what a peer names from the configured address which is local to it, so the named address is the one the rest of the topology configures that peer at.

The addresses are built once, when the start message names them, rather than on each comparison: the connect thread compares them on every one of its passes, and HostPort logs ERR_COULD_NOT_SOLVE_HOSTNAME each time one is built from a name it cannot resolve — which the fall back of setServerURL() to the host name of the machine makes an ordinary thing for a peer to name. Building them per pass would have traded one unthrottled line per second for another. The comparison itself does resolve, on every call, and reports nothing above trace when it cannot.

The comparison is HostPort.isEquivalentTo(), which is what the data server side already uses for this question in ReplicationBroker.isSameReplicationServerUrl(), and HostPort.equals() beside it. Both are needed, and for opposite reasons. isEquivalentTo() resolves both hosts, which is what sees through a name with several A records: equals() compares the normalized hosts, which hold the first address a name maps to, and the multi homing that leaves it blind to is written down in a FIXME of its own in normalizeHost(). But a host name neither end can resolve makes InetAddress.getAllByName() throw, and isEquivalentTo() reads the exception as “not the same server” — the same name is not equivalent to itself. That name is what setServerURL() falls back to when none of the addresses a peer is configured with is local to it, which is the peer of this change, and normalizedHost holds it as the name it is where resolution failed. So isServerAt() answers on either: address.equals(known) || address.isEquivalentTo(known).

What this gives up

The peer whose configuration was copied whole: two servers which share a server id and name the same address are now read as one, and the second session is dropped in silence where the source addresses used to tell them apart. The equals() arm carries that to the name which resolves nowhere: two servers which share a server id and both name one unresolvable name, on one port, are read as one as well.

Two servers which name different addresses are still reported — with one exception, and it is the behaviour of the base rather than of this change: two peers behind one gateway, dialling in on one port, carry one and the same address on the connected arm of both handlers, and the second is dropped in silence, exactly as it was when that address was compared as a string. twoServersSharingAServerIdAreStillReported pins the reporting of the pair an administrator can act on, and aSecondSessionFromTheAddressAPeerIsConnectedOnIsReadAsThatPeer pins the silent drop, so what is given up here is readable from a test rather than from this paragraph alone.

And the address a peer names is what that peer says of itself, which now decides runConnect() as well: a peer which names an address this configuration gives to another replication server hides that one from the connect thread, which then dials nothing for it and reports it connected while it is down. It takes two servers configured at one address — a configuration copied whole, or two sites whose private ranges overlap — and the peer it hides is still the one which dials this server. ReplicationServerDomain.isConnectedToServerAt() says so.

Left out on purpose

  • The throttle on abortStart(). logger.error(reason) in ServerHandler.abortStart() is still unthrottled. [#911] Report the replication connections which used to be dropped in silence #935, which this branch now sits on, is where FailureLogThrottle lives, and it also has connect() blacklist a peer whose handshake aborted for six connect passes, where an abort used to count as a connection; putting a second throttle here would collide with it. This change removes the mis-dialling that produced the volume, rather than bounding the volume.
  • The addresses ERR_DUPLICATE_REPLICATION_SERVER_ID prints. The line names the two connected addresses, while the decision is now made on the named ones as well. After the equals() arm above, no pair it reports carries one address twice, so what is left is a line which does not show everything the decision read — a change of the message and of the case which pins its text, and one for a change of its own.
  • A complete skip test. An address cannot be mapped to a server id without dialling it, so runConnect() can never skip a peer whose configuration names it by something this server's configuration does not use. What that costs is bounded by the blacklist of [#911] Report the replication connections which used to be dropped in silence #935, not by this change.

The test

MultiHomedPeerTest drives the mechanism rather than a stand in for it. The fake peer dials the server under test over the loopback interface and names a documentation address (RFC 5737) in its start message, which is what a peer behind a NAT looks like to the server it dials: it registers under 127.0.0.1:P while it is configured, and names itself, as 192.0.2.1:P. The handshake the server then offers it runs on a session which reports the address it dialled, which is what reaching that peer at its configured address gives. The addresses named are documentation addresses (RFC 5737): nothing answers at them, which is what the connect thread of the server under test finds when it dials one — a host stack sends that SYN to its default route, and the connect fails, once per pass until the peer has registered.

The registration under the other address is asserted as a precondition rather than assumed, because a handshake which ends any other way registers nothing at all and would leave the cases asserting something else.

Without the fix, three of the cases fail, and on the mechanism rather than beside it (measured on the first commit, so the line numbers are the ones it had):

[ERROR] aPeerRegisteredUnderTheAddressItDialledFromIsFoundByItsConfiguredAddress:128
        Expecting value to be true but was false
[ERROR] aPeerReachedAtItsConfiguredAddressIsNotReportedAsADuplicateServerId:172
        Expecting empty but was: ["... category=SYNC severity=ERROR msgID=55 msg=In Replication
        server Replication Server 65528 8251: replication servers 127.0.0.1:65527 and
        192.0.2.1:65527 have the same ServerId : 8252", ...]
[ERROR] aPeerRemovedFromTheConfigurationIsDisconnectedByItsConfiguredAddress:258
        Expecting [8252] not to contain [8252]

Round two of review: a mutant per road

The run above reaches the predicate and the three call sites together, which left each of them pinned by the same thing. The cases now stand apart, and each one is measured against a mutant of its own — every mutant run against the whole class, all six cases:

Mutant Case which goes red
address.equals(known) dropped from isServerAt() aPeerWhichNamesAnUnresolvableHostIsNotReportedAsADuplicateServerId (the duplicate id line)
address.getPort() == known.getPort() in place of the comparison twoServersSharingAServerIdAreStillReported
addresses = singletonList(namedAddress) aPeerWhichNamesAnAddressThisConfigurationDoesNotUseIsFoundByTheOneItDialledFrom
both HostPorts rebuilt inside isServerAt() on every call aPeerWhichNamesAnUnresolvableHostIsNotReportedAsADuplicateServerId (the ERR_COULD_NOT_SOLVE_HOSTNAME line)
the base comparison back at ReplicationServer.java:612, isConnectedToServerAt() untouched aPeerRegisteredUnderTheAddressItDialledFromIsFoundByItsConfiguredAddress (the connect thread line)

The last of those is why the first case now records an outage for the configured address (rs.connect(peerAddress, baseDN)) and waits for NOTE_REPLICATION_SERVER_CONNECT_RESTORED: that record is written by the already connected branch of runConnect() and by nothing else, while a second dial writes nothing at all — ConnectFailureReporter records a peer as DOWN once, and connect() has no debug branch under it.

Round three of review: the entry of this server, and the arms no case read

HostPort.normalizeHost() folds every address local to this machine to localhost, so the entry of this server and the loopback address a peer names for itself are one address on one port: a peer whose own configuration lists localhost:P for itself names exactly that in its start messages, and this server reads that name as its own entry. disconnectRemovedReplicationServers() handed that entry to the domain like any other, so removing it — a configuration which does not list this server is supported, and runConnect() says so — stopped the session of a healthy peer, which then had to dial again. At the base the comparison read the connected address of that peer, its real one, which never folds that way. The entry of this server is now skipped there as runConnect() skips it before it dials, and by the same test: !oldRSAddress.equals(HostPort.localAddress(getReplicationPort())), which covers every address local to this machine on the port this server listens on rather than the literal entry.

Three roads the cases reached without reading are pinned, one mutant each:

Mutant Case which goes red
the guard above dropped from disconnectRemovedReplicationServers() removingTheEntryOfThisServerStopsNoPeer
isSameServerAs() walking the named arm of the other handler only aSecondSessionFromTheAddressAPeerIsConnectedOnIsReadAsThatPeer
nonexistent.invalid replaced by a name which resolves the premise of aPeerWhichNamesAnUnresolvableHostIsNotReportedAsADuplicateServerId

removingTheEntryOfThisServerStopsNoPeer registers two peers — one naming 127.0.0.1:ports[0], the address this server listens on, and one naming its own entry at 127.0.0.1:ports[1] — and removes both entries in a single applyConfigurationChange(), so the guard alone tells the two apart: without it the first peer loses its session, while the second, which does go, is what says the road reached stopReplicationServers() rather than returning early. aSecondSessionFromTheAddressAPeerIsConnectedOnIsReadAsThatPeer answers a handshake from the address the registered peer is connected on while naming another address, which only the connected arm of isSameServerAs() can match — the arm which carries what "What this gives up" says of two peers behind one gateway. And the unresolvable case now asserts the ERR_COULD_NOT_SOLVE_HOSTNAME record its premise rests on before it clears the log: a resolver which answers a name which does not exist would have isEquivalentTo() carry that case on its own, leaving the equals() arm unread and the count of that record vacuous, with nothing red to say so.

One row of the table of round two moves with them: addresses = singletonList(namedAddress) now kills two cases, its own and the new connected arm one, which reads the same road from the other side.

The javadoc says three things it did not. isServerAt() resolves a pair the names alone do not answer on every call, and both handshake roads hold the domain lock across isAlreadyConnectedToRS(), so what a resolver which does not answer holds is the domain rather than one comparison — and only replication servers reach it, a data server handshake making no such comparison. isConnectedToServerAt() is hidden from by any named address which resolves here to one this configuration gives to another replication server, the fall back host name of setServerURL() included, not only by two servers configured at one address. And the pair the equals() arm reads as one is read as one under the other arm as well, which is this change rather than the base. In the test, toServerAddressURL() is named in the past tense as the method this change removes, and a dial of an address nothing answers at is one per six passes, the domainTicket + 6 blacklist of #935 holding the five in between.

The branch was rebased on the current master before this round, so the round commit carries the round and nothing else: the three commits replayed with no conflict, and git range-diff reports = for all three. Two of the commits which landed since do touch these files — #1019, which reports a ReplicaOfflineMsg as forwarded only to a peer which can decode it, and #987, which interrupts the listen thread after those messages are forwarded — and neither touches what this change owns.

Rebased on master after #935 and #976

The one conflict was in runConnect(): #935 hoisted getConfiguredRSAddresses() into a local it reads again further down, for connectFailures.retainAll(), while this change replaced the connectedRSAddresses set next to it. The resolution keeps that local and puts domain.isConnectedToServerAt() where connectedRSAddresses.contains() was, which is the same two-line change as before against the new base; getConnectedRSAddresses() is gone as before. Nothing else #935 or #976 touched overlaps with this change in code: the skip branch of runConnect() now also reports the peer as reachable again, and connect() now returns whether the handshake completed, and both are as right for a peer recognised by the address it names as for one recognised by the address it came from.

What did overlap is a comment. #935 justifies closing an outage on the connection alone, in ReplicationServer.connect(), with the multi homed peer runConnect() could never match, and it describes that peer by what the old comparison did with it -- registered under the source address of its session, never matched, its handshake aborted on a duplicate server id. The second commit rewrites the two paragraphs which said that: what is left of that reading is the peer of "A complete skip test" above, the one which names an address this configuration does not use, and its handshake is resolved as a cross connect rather than reported as a duplicate. The last paragraph said the pass after a cross connect says what is true, which holds for the peer runConnect() can match and not for this one: nothing more is said of it where no outage was reported, the one warning stays where one was, and the blacklist bounds the dialling.

Verified locally, on the branch rebased on master

  • The same 13 classes as before — MultiHomedPeerTest, the classes [#917] Wait for every peer replication server to forward the ReplicaOfflineMsg #947 rewrote, and the one [#911] Report the replication connections which used to be dropped in silence #935 added for the connect thread, whose skip branch this change decides — 91 tests, no failures (80 before the rebase, the current master carrying more of them). Run as three invocations of mvn -Pprecommit -pl opendj-server-legacy verify -Dit.test=… over four, four and five classes rather than one, because this machine was building other branches at the same time and the single invocation was killed for memory before any test ran.
  • mvn -pl opendj-server-legacy package -Dmaven.test.skip=true — the attach-javadocs execution, with doclint=all,-missing and failOnWarnings=true, passes, the new {@link ReplicationServerDomain#isAlreadyConnectedToRS(ReplicationServerHandler)} included.
  • Each mutant of the two tables above against the whole class: eight of them, each red, and each killing the case named beside it — singletonList(namedAddress) killing two, as the round three section says. Two of those runs failed in setUp on binding a port of the embedded server, another JVM on this machine holding 0.0.0.0:65528 and 0.0.0.0:65534, and were green when re-run.
  • The three failures above are the run of the four cases against the old comparison, with the new ReplicationServerDomain.isConnectedToServerAt() present but still comparing what getConnectedRSAddresses() used to compare — so the cases fail on the comparison rather than on a missing method. Each of them reaches one of the three call sites and no other, so that run pins all three: the first the one in runConnect(), the second the one in isAlreadyConnectedToRS(), the third the one in stopReplicationServers().

@vharseko vharseko added bug replication tests Test suites: fixing, enabling, un-disabling labels Sep 10, 2026
@vharseko
vharseko force-pushed the feature/multi-homed-peer-recognition branch from f0acce5 to 44f98ce Compare September 11, 2026 18:36
@vharseko

Copy link
Copy Markdown
Member Author

@maximthomas rebased on master after #935 and #976 landed; the branch had gone CONFLICTING.

The one conflict was in ReplicationServer.runConnect(), where #935 hoisted getConfiguredRSAddresses() into a local it reuses further down and this change had replaced the connectedRSAddresses set next to it. The resolution keeps #935's local and puts domain.isConnectedToServerAt() where connectedRSAddresses.contains() was — the same two-line change as before against the new base. Nothing else the two landed PRs touched overlaps with this one; the diff against master is still the same four files.

Re-run on the rebased branch: the 12 classes from the description plus ReplicationServerConnectFailureTest from #935 — 78 tests, no failures — and the attach-javadocs gate of opendj-server-legacy. The description is updated to match (a "Rebased on master" section, and the line numbers it cited in ReplicationServerHandler replaced with method names, since #935 moved them).

@vharseko

Copy link
Copy Markdown
Member Author

@maximthomas one more commit, 58c7218197, comment only.

Checking the rebase for overlap with #935 turned up one in prose rather than code: the comment in ReplicationServer.connect() which justifies closing an outage on the connection alone names the multi homed peer runConnect() could never match as one of its two reasons, and describes that peer by what the old comparison did with it — registered under the source address of its session, never matched, its handshake aborted on a duplicate server id. With this change both addresses identify it, so that description is no longer what happens: what is left of the reason is the peer which names an address this configuration does not use (the "complete skip test" the description leaves out), and its handshake is now resolved as a cross connect rather than reported as a duplicate. The two paragraphs which said otherwise are rewritten; the reasoning they support is unchanged. The description carries the same note under "Rebased on master".

The duplicate server id check itself is not touched: isAlreadyConnectedToRS() still reports two servers which share an id and answer to different addresses, which twoServersSharingAServerIdAreStillReported pins.

@maximthomas maximthomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: The change sits where the bug is, and one predicate serves every reader of the address.

  • The three call sites the issue and the description name all go through one predicate: ReplicationServer.runConnect:612, ReplicationServerDomain.isAlreadyConnectedToRS:1431 and stopReplicationServers:1080 all end in ReplicationServerHandler.isServerAt().
  • The comparison is the one the data-server side already uses for this question, ReplicationBroker.isSameReplicationServerUrl()HostPort.isEquivalentTo(), and getConnectedRSAddresses() is gone rather than widened.
  • MultiHomedPeerTest asserts its precondition (getServerAddressURL() = the loopback address, getServerURL() = the named one) before the case, so a handshake that ends any other way fails on the precondition, not beside it.

issue (blocking): A peer that names a host this server cannot resolve is still reported as ERR_DUPLICATE_REPLICATION_SERVER_ID, where the rewritten connect() comment says its second session is dropped as a cross connect.

opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerHandler.java:783, :60-64, opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServer.java:758-767, opendj-server-legacy/src/main/java/org/opends/server/types/HostPort.java:489-490, :525-532

isServerAt() decides only through HostPort.isEquivalentTo(), which calls InetAddress.getAllByName() on both sides and returns false on UnknownHostException — a name that does not resolve from this server is equivalent to nothing, not even to the identical string. The addresses javadoc calls such a name "an ordinary thing for a peer to name" (the setServerURL() fallback of a peer whose configured address is not local to it — the NAT peer of #1017). For that peer, inbound handler [H:p, A2:p], outbound [H:p, A1:p]: isSameServerAs() compares H/H (throws → false), H/A2, A1/H, A1/A2 — all false — and isAlreadyConnectedToRS() throws the duplicate-id message, once per dial, at the #935 blacklist rate, plus the ERR_COULD_NOT_SOLVE_HOSTNAME line each outbound handler's HostPort.valueOf(H) logs. Not a regression, and the multi-homed peer (names the address it is configured under) is fixed; but the comment at ReplicationServer.java:765 promises the cross connect for exactly this peer, and the code does not deliver it. HostPort.equals() compares normalizedHost, which is the raw name when resolution failed (HostPort.java:363-364), so one short circuit closes it:

  boolean isServerAt(HostPort address)
  {
    for (HostPort known : addresses)
    {
      // equals() matches a name neither side can resolve; isEquivalentTo() answers false for it.
      if (address.equals(known) || address.isEquivalentTo(known))
      {
        return true;
      }
    }
    return false;
  }

Pin: a MultiHomedPeerTest case whose fake peer names nonexistent.invalid:ports[1] (RFC 6761, resolvers answer NXDOMAIN) and dials the server at it; assert the second session is dropped with no ERR_DUPLICATE_REPLICATION_SERVER_ID record — red at the head, green with the short circuit.


issue (non-blocking): The runConnect() call site is pinned by no case: restoring the base branch's skip at :612 survives 4/4 (measured), while the description says the first case pins that site.

opendj-server-legacy/src/test/java/org/opends/server/replication/server/MultiHomedPeerTest.java:126, opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServer.java:612-619

aPeerRegisteredUnderTheAddressItDialledFromIsFoundByItsConfiguredAddress asserts domain.isConnectedToServerAt(peerAddress) directly and never observes the connect thread; the run the description reports mutated the comparison inside the predicate, which the case reaches by calling it, not through runConnect(). Mutant at :612, isConnectedToServerAt() untouched: if (domain.getConnectedRSs().values().stream().map(h -> HostPort.valueOf(h.getServerAddressURL())).anyMatch(rsAddress::equals)) — survives 4/4 (bytecode checked). Under it the connect thread dials 192.0.2.1:p1 on every pass — the symptom the class javadoc names first — and nothing in the class moves: cases 2–4 never read the connect-failure records, and errorLogRecordsOfHandshakeWith() filters to the duplicate-id text.

      final ReplicationServerHandler registered = waitForRegistration(domain);
      // Two passes of the connect thread after the registration: the already connected
      // branch reports a session (ReplicationServer.java:619) and dials nothing.
      rs.waitConnections();
      rs.waitConnections();
      assertThat(connectFailureRecordsFor(peerAddress, afterRegistration))
          .as("a registered peer must not be dialled again")
          .isEmpty();

Pin: at the head the already-connected branch takes reportConnectionRestored(rsAddress, baseDN, true) and never dials; under the mutant every pass dials and the first failure is logged after the registration timestamp.


suggestion (non-blocking): The host half of isServerAt() is pinned by no case: a port-only comparison survives 4/4 (measured).

opendj-server-legacy/src/test/java/org/opends/server/replication/server/MultiHomedPeerTest.java:201, opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerHandler.java:783

The one negative case, twoServersSharingAServerIdAreStillReported, names the second server at 198.51.100.1:ports[2] while every positive case uses ports[1], so isEquivalentTo() answers false on the port before any host is read. Mutant if (address.getPort() == known.getPort()) in place of address.isEquivalentTo(known) — 4/4 green (javap: no isEquivalentTo in the class).

    // Same port as the registered peer: only the host tells the two apart.
    final HostPort otherAddress = documentationAddress(OTHER_ADDRESS, ports[1]);

Pin: nothing binds ports[2]; at the head isEquivalentTo(198.51.100.1:p1, 192.0.2.1:p1) is false (distinct literals) and vs 127.0.0.1:p1 false (one local, one not), so the case still reports; the port-only mutant drops the session in silence and isNotEmpty fails.


suggestion (non-blocking): The connected-address arm of addresses is pinned by no case: singletonList(namedAddress) survives 4/4 (measured).

opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerHandler.java:123, :769-773, opendj-server-legacy/src/test/java/org/opends/server/replication/server/MultiHomedPeerTest.java:112, :159, :210, :250

Every case has the fake peer name the address the server is configured with, so every match lands on the named arm, and the getServerAddressURL() preconditions read serverAddressURL, which the mutant still sets. The javadoc of isServerAt() calls the connected arm "the only one known of a server which names an address this configuration does not use"; that road is asserted nowhere. Mutant addresses = Collections.singletonList(namedAddress); — 4/4 green.

    // The peer is configured at the address it dials from and names one this
    // configuration does not use: only the connected arm can match it.
    final HostPort peerAddress = loopbackAt(ports[1]);
    final HostPort namedAddress = documentationAddress(OTHER_ADDRESS, ports[1]);
    rs = startServerWithPeerConfiguredAt(ports[0], "multiHomedPeerConnectedArmDb", peerAddress);
    inbound = registerPeerFrom(ports[0], namedAddress, baseDN);
    ...
    assertThat(domain.isConnectedToServerAt(peerAddress)).isTrue();

Pin: at the head isConnectedToServerAt(127.0.0.1:p1) matches on the connected arm; the mutant's isEquivalentTo(127.0.0.1:p1, 198.51.100.1:p1) is false (one local, one not) and the case goes red.


suggestion (non-blocking): The resolve-once design is pinned by nothing: every address the cases hand in is an IP literal.

opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerHandler.java:60-64, opendj-server-legacy/src/test/java/org/opends/server/replication/server/MultiHomedPeerTest.java:423-430

The reason given for building the two HostPorts once is the ERR_COULD_NOT_SOLVE_HOSTNAME line HostPort logs per construction from a name it cannot resolve; documentationAddress() and loopbackAt() build literals only, for which normalizeHost() logs nothing, and no case reads that message. A mutant rebuilding HostPort.valueOf(serverURL) inside isServerAt() on every call re-logs per connect pass and stays green (traced, not run).

Pin: the same nonexistent.invalid:ports[1] case as the blocking issue — assert exactly one ERR_COULD_NOT_SOLVE_HOSTNAME record for that name over two waitConnections() passes after registration.


suggestion (non-blocking): The choice of isEquivalentTo() over equals() is unpinned by construction.

opendj-server-legacy/src/test/java/org/opends/server/replication/server/MultiHomedPeerTest.java:430, opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerHandler.java:783

The two differ only on a name with several A records (a hosts-file fixture) or on an unresolvable name — where equals() is the one that answers true, see the blocking issue. On the literals the cases use they agree, so address.equals(known) alone is green on every case. Recorded once; nothing portable to ask for.


suggestion (non-blocking): ERR_DUPLICATE_REPLICATION_SERVER_ID prints the two connected addresses, while the decision is now made on the named ones too.

opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerDomain.java:1444-1447, opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerHandler.java:122

serverAddressURL is the connected address only. A pair reported at the head differs in what it names as well as where it came from; the line shows the latter, which for two peers behind one gateway is the same string twice. Pre-existing shape; twoServersSharingAServerIdAreStillReported pins the current text, so this is a test change too.

    LocalizableMessage message = ERR_DUPLICATE_REPLICATION_SERVER_ID.get(
        localReplicationServer.getMonitorInstanceName(),
        oldRsHandler.getServerURL() + " (" + oldRsHandler.getServerAddressURL() + ")",
        rsHandler.getServerURL() + " (" + rsHandler.getServerAddressURL() + ")",
        rsHandler.getServerId());

nitpick (non-blocking): The addresses javadoc and the description say the addresses are "resolved once … rather than on each comparison"; the comparison resolves per call.

opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerHandler.java:60-64, opendj-server-legacy/src/main/java/org/opends/server/types/HostPort.java:489-490

isEquivalentTo() calls getAllByName() on both hosts on every call; what is once is the construction — normalizeHost() and its ERR line. The stated outcome (no ERR line per pass) holds, but because isEquivalentTo() traces its UnknownHostException instead of logging it; the connect thread does resolve every connected handler's named host per pass, per configured address with a matching port, bounded by the JVM resolver cache (10 s negative TTL).

  /**
   * The addresses the remote replication server is known by, built once, when its start
   * message names it: {@link HostPort} logs a name it cannot resolve each time it is built
   * from one -- which the fall back of {@code ReplicationServer.setServerURL()} to the host
   * name of the machine makes an ordinary thing for a peer to name -- while the comparison
   * the connect thread makes on every pass reports nothing above trace.
   */

nitpick (non-blocking): "Nothing the tests do can leave the machine" / "which nothing routes" — the server under test dials 192.0.2.1:P.

opendj-server-legacy/src/test/java/org/opends/server/replication/server/MultiHomedPeerTest.java:72, :427, :270-272, opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServer.java:711

The helper's own javadoc says the connect thread "dials it, fails, and comes back to it". Any pass that finds the domain with the peer not yet registered runs socket.connect(192.0.2.1:p1, 5000) under connectThreadLock; a host stack does not drop a SYN to TEST-NET-1, it sends it to the default route. A race (pass 1 usually precedes the domain), fails nothing, costs up to 5 s of connect-thread wall time and a thread that outlives remove(rs) by up to the timeout (shutdown() interrupts only, socket.connect is not interruptible). The claim to keep is "nothing answers there", not "nothing leaves".


nitpick (non-blocking): "Two servers which name different addresses are still reported" (description, "What this gives up") is not so for two peers behind one gateway on the same port.

opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerHandler.java:753-761, :120-121

Two peers sharing a server id, dialling in from one source S on the same listen port, carry S:port on the connected arm of both handlers (connectedAddress takes the named port); isSameServerAs() matches S/S and drops the second in silence, and stopReplicationServers(S:port) stops both. Same as the base (string compare on S:port); the code's own javadoc ("either of the two addresses … identifies it") is consistent with it — the over-promise is only in the description.


nitpick (non-blocking): ReplicationServerConnectFailureTest still documents the pre-#1020 identity, and both test javadocs misname the old method.

opendj-server-legacy/src/test/java/org/opends/server/replication/server/ReplicationServerConnectFailureTest.java:312-330, :345-347, opendj-server-legacy/src/test/java/org/opends/server/replication/server/MultiHomedPeerTest.java:56

The javadoc of aPeerRegisteredUnderAnotherAddressStillClosesItsOutage says "This is the multi homed peer", names toServerAddressURL() and says the already-connected branch "compares the configured address against one which never matches it". At the head the method is gone from src/main, a multi-homed peer is matched by its named address, and the fixture (127.0.0.1:ports[2] vs configured ports[1]) is the port-mismatch peer of the rewritten connect() comment — it reaches ERR_DUPLICATE_REPLICATION_SERVER_ID only because the port differs. The second commit rewrote the two connect() paragraphs that said the same and left this one. Also, ServerHandler.toServerAddressURL() (both javadocs, the description) was a private method of ReplicationServerHandler.


note (non-blocking): Not run.

  • The three mutants against the 13 classes the description lists: measured against MultiHomedPeerTest only (all three survive there); the sibling suites' replication servers name localhost:port, which isEquivalentTo() answers on the named arm, so they do not reach the arms in question (reasoned, not run).
  • Cases 2 and 4 red on the base code with isConnectedToServerAt() shimmed: taken from the description's own run, not re-run.

@vharseko

Copy link
Copy Markdown
Member Author

@maximthomas round 2, 9e51b42bf8.

issue (blocking) -- the peer whose named host this server cannot resolve. Taken.
isServerAt() now answers on HostPort.equals() as well as on isEquivalentTo(): equals()
compares the normalized hosts, which for a name resolution failed on is the name itself, while
isEquivalentTo() resolves both sides and answers false for such a name even against itself.
Pinned by aPeerWhichNamesAnUnresolvableHostIsNotReportedAsADuplicateServerId, whose fake peer
names nonexistent.invalid:ports[1] (RFC 6761) in both of its start messages: red at the head
on :317, green with the short circuit.

Two things the change carries, and the description now says both:

  • it does not make that peer quiet. Each outbound handshake still builds the named address and
    logs one ERR_COULD_NOT_SOLVE_HOSTNAME from normalizeHost(); what goes away is the
    duplicate id line beside it, so the rate halves rather than drops to nothing, and the line
    which is left names a configuration problem an operator can act on.
  • two servers which share a server id and both name one unresolvable name on one port are now
    read as one. That is the trade of "What this gives up" widened, not a new one: two servers
    which name the same resolvable address were already read as one.

issue (non-blocking) -- the runConnect() call site. The diagnosis is right and the
description was wrong: the first case reached the predicate by calling it, and the run the
description reported mutated the comparison inside the predicate.

The pin as proposed does not hold, and not only under the mutant. ConnectFailureReporter
records a peer as DOWN once: recordFailure() returns false for a second consecutive failure
and connect() has no debug branch, so a repeated dial writes nothing to the error log at all;
and the first dial of 192.0.2.1:p1 is in flight before the peer registers -- five seconds of
MultimasterReplication.getConnectionTimeoutMS() -- so its
WARN_REPLICATION_SERVER_CONNECT_ERROR lands after the registration at the head as well. The
assertion would be red at the head and green under the mutant once the blacklist holds.

What the case asserts instead is the other side of that same branch: an outage recorded from the
test thread (rs.connect(peerAddress, baseDN), false), and then
NOTE_REPLICATION_SERVER_CONNECT_RESTORED for that address, which only the already connected
branch of runConnect() logs -- the dialling branch reports a connection for a handshake it
completed, which a documentation address never gives.

suggestion -- the host half of isServerAt(). Taken: otherAddress moved to ports[1],
so the host is what tells the two servers apart, and the case needs two ports rather than three.

suggestion -- the connected arm. Taken, as
aPeerWhichNamesAnAddressThisConfigurationDoesNotUseIsFoundByTheOneItDialledFrom: the peer is
configured at the address it dials from and names one this configuration does not use.

suggestion -- the resolve once design. Taken inside the unresolvable case: three
isConnectedToServerAt() calls must add no ERR_COULD_NOT_SOLVE_HOSTNAME record for that name.

suggestion -- equals() against isEquivalentTo(). Pinned by the blocking case, which is
the one place the two differ.

suggestion -- ERR_DUPLICATE_REPLICATION_SERVER_ID prints the connected addresses.
Recorded, not taken here. After the short circuit above the pair which reaches that message
differs in what it names as well as in where it came from, so the line is no longer the same
string twice for any pair it reports; the change is a test change as you say, and it would put
the runs of this round back on the bench. Say the word and it goes in the next one.

nitpick -- "resolved once". Taken: built once. The javadoc now says what the building buys
(the error log) and what the comparison does on every pass (resolves, and reports nothing above
trace).

nitpick -- "nothing can leave the machine". Taken: nothing answers there. The class javadoc
now says what the connect thread does with such an address -- a SYN to the default route and a
connect which fails, once per pass until the peer has registered.

nitpick -- "two servers which name different addresses are still reported". Taken in the
description: two peers behind one gateway on one port carry one address on the connected arm of
both handlers and the second is dropped in silence, as it was before this change.

nitpick -- the stale javadoc. Taken: ReplicationServerConnectFailureTest no longer calls
its fixture the multi homed peer -- it is the peer whose handler is known by two addresses the
configured one matches neither of, the port being what differs -- and
ServerHandler.toServerAddressURL() is ReplicationServerHandler.toServerAddressURL() in both
javadocs and in the description.

One more from the same seam, recorded in isConnectedToServerAt(): the address a peer names
is what that peer says of itself, so a peer which names an address this configuration gives to
another replication server hides that one from the connect thread, which then dials nothing for
it and reports it connected while it is down. It takes two servers configured at one address --
a configuration copied whole, or two sites whose private ranges overlap -- and the peer it hides
is still the one which dials this server.

Runs, on the head of this round:

  • MultiHomedPeerTest, six cases, green.
  • Five mutants, one per road, each against the whole class: dropping address.equals(known) kills
    the unresolvable case on the duplicate id line; getPort() == getPort() in place of the
    comparison kills twoServersSharingAServerIdAreStillReported; singletonList(namedAddress) kills
    the connected arm case; rebuilding both HostPorts inside isServerAt() kills the unresolvable
    case on its ERR_COULD_NOT_SOLVE_HOSTNAME line; the base comparison back at
    ReplicationServer.java:612, with isConnectedToServerAt() untouched, kills the first case on
    the connect thread line. Each kills the case named and no other.
  • The 13 classes of the description -- 80 tests, no failures, in four invocations rather than one:
    the machine was building several branches at once, and a class whose embedded server cannot bind
    0.0.0.0:65534 fails in setUp for reasons which have nothing to do with this change. The three
    which did that are green on their own.
  • mvn -pl opendj-server-legacy package -DskipTests -- the attach-javadocs gate.

@maximthomas maximthomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: The fix sits where the bug is, and its trade-off is written down beside it.

  • ReplicationServerHandler.isServerAt():795address.equals(known) || address.isEquivalentTo(known): the unresolvable name the setServerURL() fallback produces now matches itself, and aPeerWhichNamesAnUnresolvableHostIsNotReportedAsADuplicateServerId goes red without the equals() arm.
  • ReplicationServerHandler.java:776-785 — the javadoc says what the equals() arm gives up (two servers sharing an id and one unresolvable name on one port), so the next reader does not rediscover it.
  • aPeerWhichNamesAnAddressThisConfigurationDoesNotUseIsFoundByTheOneItDialledFrom — the connected arm of addresses has its own case, and the description's mutant table names which case kills which mutant.

issue (non-blocking): stopReplicationServers() drops a healthy peer which names a loopback address when this server's own entry is removed from ds-cfg-replication-server.

opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerDomain.java:1083-1093, opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServer.java:2017-2027, :625

HostPort.normalizeHost() folds every address local to this machine to localhost, so HostPort.valueOf("<own hostname>:8989").equals(HostPort.valueOf("localhost:8989")) is true (measured on the compiled class, unchanged by this PR). A peer whose configuration lists localhost:P or 127.0.0.1:P for itself names exactly that in its start message (setServerURL() takes the first configured entry which isLocalAddress() on its port), and on this server that named address is this server's own configured entry on the same port. disconnectRemovedReplicationServers() puts every removed entry into serversToDisconnect, this server's own included — the connect road skips self at :625, the disconnect road does not — so removing this server's own entry from its configuration (:604-609 say a list without self is supported) has isServerAt(hostA:P) answer true on that peer's handler and stopServer() its session once. At the base the comparison read the connected address, the peer's real IP, which never folds. Cost: one spurious drop and a reconnect on the peer's next pass; the precondition (a loopback self-entry on the peer, one port, removal of the own entry) is why this is non-blocking.

// ReplicationServer.disconnectRemovedReplicationServers()
final Set<HostPort> newRSAddresses = getConfiguredRSAddresses();
// This server holds no session with itself, and a peer naming a loopback address on this
// port reads as this server under HostPort.equals(): skip self, as runConnect() does.
final HostPort localAddress = HostPort.localAddress(getReplicationPort());
for (HostPort oldRSAddress : oldRSAddresses)
{
  if (!newRSAddresses.contains(oldRSAddress) && !oldRSAddress.equals(localAddress))
  {
    serversToDisconnect.add(oldRSAddress);
  }
}

Pin: the fold needs one port on two machines, which the in-JVM fixture cannot give; a case whose fake peer names the server's own address (loopbackAt(ports[0])) and whose removal list holds only that entry asserts the registered handler survives — it pins the guard, not the fold.


suggestion (non-blocking): The equals()-arm pin turns on the resolver answering NXDOMAIN for nonexistent.invalid, and nothing in the case checks that premise.

opendj-server-legacy/src/test/java/org/opends/server/replication/server/MultiHomedPeerTest.java:284, :301-306

On a resolver that synthesises an A record for NXDOMAIN (consumer ISPs, captive portals, some corporate DNS) the name resolves to one address on both handlers: HostPort.normalizeHost() never logs, so the isEmpty at :306 holds vacuously, and isEquivalentTo() answers true on its own, so the equals()-dropped mutant survives while the case stays green. The record which proves the premise — ERR_COULD_NOT_SOLVE_HOSTNAME, logged once when the address is built at registration — lands before ERROR_TEXT_WRITER.clear() at :301 and is asserted nowhere. The kill in the mutant table is a property of the box it ran on; the case never goes red, so this is an environment-dependent pin, not a flake.

// before TestCaseUtils.ERROR_TEXT_WRITER.clear() at :301
assertThat(recordsContaining(TestCaseUtils.ERROR_TEXT_WRITER.getMessages(),
    ERR_COULD_NOT_SOLVE_HOSTNAME.get(UNRESOLVABLE_HOST).toString()))
    .as("the resolver of this box must not resolve " + UNRESOLVABLE_HOST)
    .isNotEmpty();

Or: a SkipException on a resolving name; the assertion is the one that says which network is wrong, and it doubles as the positive control the :306 isEmpty lacks.


suggestion (non-blocking): The connected-arm match of isSameServerAs() — two peers behind one gateway read as one — is pinned by no case.

opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerHandler.java:755-763

isSameServerAs() walks both of the other handler's addresses through isServerAt(). Every case where isAlreadyConnectedToRS() must answer true has the two handlers naming the same address (192.0.2.1:P twice, or nonexistent.invalid:P twice), and twoServersSharingAServerIdAreStillReported differs on both arms, so a mutant that iterates only the named arm of other.addresses answers the same on all six cases (reasoned from the inputs, not run). The description names the road as inherited from the base and pins its reporting counterpart; whether the silent drop on the connected arm is intended is readable from no test. The singletonList(namedAddress) row of the mutant table is killed through isConnectedToServerAt(), which leaves this call site's arm unmeasured.

// after registerPeerFrom(ports[0], peerAddress, baseDN) and waitForRegistration(domain):
// a second session from the address the peer is connected on, naming another address
final List<String> records =
    errorLogRecordsOfHandshakeWith(rs, baseDN, loopbackAt(ports[1]), otherAddress);
assertThat(duplicateServerIdRecords(records, rs, loopbackAt(ports[1]), otherAddress))
    .as("a second session from the address a peer is connected on is that peer")
    .isEmpty();

Pin: the named-arm-only mutant goes red on it — 198.51.100.1:P against 192.0.2.1:P and 127.0.0.1:P is false under both equals() and isEquivalentTo(), and only the connected arm carries 127.0.0.1:P.


suggestion (non-blocking): isAlreadyConnectedToRS() resolves the other handler's named host under the domain lock on the road where the two handlers name different hosts.

opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerHandler.java:185-219, :326-330, opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerDomain.java:1440

lockDomainNoTimeout() at :185processStartFromRemote()isAlreadyConnectedToRS()isSameServerAs()isEquivalentTo()InetAddress.getAllByName() on both named hosts, per pair, whenever equals() fails on a pair; the accept road holds the lock at :328 for :330 the same way. Only RS-RS handshakes resolve there (DataServerHandler holds no such call); DS handshakes on the domain wait on the lock. On a genuine duplicate id, or a round-robin name whose first address differed at the two constructions, the old handler's name is cold once per JVM TTL, and a slow or dead resolver then holds the domain lock for the resolver timeout: lockDomainWithTimeout() callers on that domain fail with WARN_TIMEOUT_WHEN_CROSS_CONNECTION (3-8 s), lockDomainNoTimeout() callers wait. The base had the same shape (toServerAddressURL() resolved the peer's named host under this lock), so this widens it by one name on a rarer road rather than adding a class; the description states the per-call resolve and its log cost, not that the handshake road does it under the lock. No code change asked — a clause in the isServerAt() javadoc ("resolves under the domain lock on the handshake road") is the cheap fix.


nitpick (non-blocking): The isConnectedToServerAt() javadoc bounds the hide narrower than the code: the fallback host name a peer names matches whatever it resolves to on this server, no address configured twice needed.

opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerDomain.java:1051-1056

setServerURL() falls back to InetAddress.getLocalHost().getHostName() for the NAT peer this PR is for (ReplicationServer.java:1884-1887); setServerAddresses() builds that name here, and isServerAt() matches whatever this server's resolver gives it — a clone host name, split DNS or a stale /etc/hosts line pointing at another RS's address hides that RS with nothing configured twice. Suggested: "so that takes a peer whose named address resolves here to one this configuration gives to another replication server: two of them configured at one address, or the host name setServerURL() falls back to mapped there by this server's resolver".


nitpick (non-blocking): "as two servers which name the same resolvable address already are" reads as base behaviour; the base compared connected addresses only.

opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerHandler.java:782-784

At the base isAlreadyConnectedToRS() compared getServerAddressURL() strings over socket-derived URLs, so two servers naming one address were told apart by their source addresses; "already" is true of the isEquivalentTo() arm beside it, which is this PR's, and the description says "are now read as one". Suggested: "as two servers which name the same resolvable address are under the other arm".


nitpick (non-blocking): The class javadoc says the connect thread dials the documentation address "once per pass until the peer has registered"; the blacklist makes it one dial per six passes.

opendj-server-legacy/src/test/java/org/opends/server/replication/server/MultiHomedPeerTest.java:72-74, :417-418

ReplicationServer.runConnect() blacklists a failed address for six tickets (:632, :640): a dial at ticket T is skipped at T+1..T+5 and repeated at T+6. Suggested at :74: "once per six passes -- a failed dial is blacklisted for the five after it -- until the peer has registered"; at :417-418: "comes back to it six passes later".


nitpick (non-blocking): The class javadoc names ReplicationServerHandler.toServerAddressURL() in the present tense; this PR removes it.

opendj-server-legacy/src/test/java/org/opends/server/replication/server/MultiHomedPeerTest.java:56-57

git grep toServerAddressURL at the head returns this line alone; the method is replaced by setServerAddresses() (ReplicationServerHandler.java:118-126). The paragraph narrates the base, but the tense says it is current. Suggested: "ReplicationServerHandler.toServerAddressURL(), which this change removes, took the host of a handler from ...".

…ress it names, not by the one its session came from

A remote replication server was identified by the address the socket of one of its
sessions happened to carry, which `ServerHandler.toServerAddressURL()` takes from
`session.getRemoteAddress()`. That address is the interface a connection used rather
than an identity, so a peer reachable at more than one address -- a multi homed host,
or a NAT where the address a peer connects from is not the address it is configured
as -- was never recognised as the peer configured at its own address.

A remote server is now known by both of the addresses it comes with, the one it names
in its start message and the one its session came from, and either identifies it. They
are resolved once, when the start message names them, because the connect thread
compares them on every one of its passes and `HostPort` logs a name it cannot resolve
each time one is built from it.

Three places read the address, and all three were wrong for such a peer:

* `runConnect()` skipped a configured peer only when a handler was registered under that
  exact address, so it dialled a peer it was already connected to about once a second,
  and every one of those handshakes aborted and logged
  `ERR_DUPLICATE_REPLICATION_SERVER_ID` or `ERR_RS_DISCONNECTED_DURING_HANDSHAKE` with no
  throttle, for as long as the peer stayed where it was;
* `isAlreadyConnectedToRS()` compared the address URLs of the two handlers as strings, so
  a second session with a peer already connected read as two replication servers sharing
  a server id, which is a misconfiguration this topology does not have;
* `stopReplicationServers()` compared the addresses removed from
  `ds-cfg-replication-server` against the registered one, so a peer an administrator took
  out of the topology kept its session.

The comparison is `HostPort.isEquivalentTo()`, which is what the data server side already
uses for the same question in `ReplicationBroker`: `HostPort.equals()` resolves only the
first address a name maps to, and the multi homing that leaves it blind to is noted in a
FIXME of its own in `normalizeHost()`.

What this gives up is the peer whose configuration was copied whole: two servers which
share a server id and name the same address are now read as one and the second session is
dropped in silence, where the source addresses used to tell them apart. Two servers which
name different addresses are still reported, which is every duplicate id an administrator
can act on.
…never sees connected now costs

The comment OpenIdentityPlatform#935 left in ReplicationServer.connect() justified closing an outage on the
connection alone with the multi homed peer runConnect() could never match, and described
that peer by what the old comparison did with it: registered under the source address of
its session, never matched, its handshake aborted on a duplicate server id. Both addresses
now identify it, so what is left of that reading is the peer which names an address this
configuration does not use, and its handshake is resolved as a cross connect rather than
reported as a duplicate. The last paragraph said the pass after a cross connect says what
is true, which holds for the peer runConnect() can match and not for this one.
…can resolve, and pin each call site

The comparison a remote replication server is recognised by answers false for a host name
this server cannot resolve: InetAddress.getAllByName() throws for it and
HostPort.isEquivalentTo() reads that as "not the same server", even against the same name.
So the peer of the rewritten connect() comment -- the one which falls back to naming the
host name of its machine, which the rest of the topology has no reason to resolve -- was
still reported as two servers sharing a server id, once per dial. It is now compared as the
name it is, which is what HostPort.equals() does with it. What that gives up is two servers
which share a server id and both name one unresolvable name on one port: they are read as
one, as two which name the same resolvable address already were.

Each of the three call sites is now pinned by a case of its own:

* runConnect(): the first case records an outage for the configured address and asserts
  NOTE_REPLICATION_SERVER_CONNECT_RESTORED, which the already connected branch writes and
  nothing else does -- a second dial writes nothing at all, ConnectFailureReporter recording
  a peer as DOWN once and connect() having no debug branch under it;
* the host half of isServerAt(): the second server of the negative case moved to the port of
  the first, so the host is what tells the two apart;
* the connected arm of the addresses: a case whose peer names an address this configuration
  does not use and is found by the one it dialled from.

Also the javadoc: "built once" rather than "resolved once", and what the comparison resolves
on every pass; what the connect thread does with a documentation address, which is a SYN to
the default route rather than nothing leaving the machine; the fixture of
ReplicationServerConnectFailureTest, which is the peer whose two addresses the configured
one matches neither of rather than the multi homed peer; the name of
ReplicationServerHandler.toServerAddressURL(); and what trusting the address a peer names
costs, in ReplicationServerDomain.isConnectedToServerAt().
…disconnect list, and pin the arms no case read

HostPort normalises every address local to this machine to localhost, so the entry of this
server and the loopback address a peer names for itself are one address on one port: a peer
whose own configuration lists localhost:P for itself names exactly that in its start messages,
and this server reads that name as its own entry. disconnectRemovedReplicationServers() handed
that entry to the domain like any other, so removing it -- a configuration which does not list
this server is supported -- stopped the session of a healthy peer, which then had to dial again.
At the base the comparison read the connected address, the real address of the peer, which never
normalises that way. The entry of this server is now skipped there, as runConnect() skips it
before it dials.

Three roads the cases reached without reading are pinned:

* removingTheEntryOfThisServerStopsNoPeer registers two peers, one naming the address this
  server listens on and one naming its own entry, and removes both entries in a single
  applyConfigurationChange(): the guard alone tells the two apart, so the case is red without it
  and the peer whose own entry went is the positive control of the road.
* aSecondSessionFromTheAddressAPeerIsConnectedOnIsReadAsThatPeer answers a handshake from the
  address the registered peer is connected on while naming another address, which only the
  connected arm of isSameServerAs() can match. That arm carries what this change gives up -- two
  servers behind one gateway read as one -- and no case stated it.
* The unresolvable case now asserts the ERR_COULD_NOT_SOLVE_HOSTNAME record its premise rests on
  before it clears the log. A resolver which answers a name which does not exist would have
  isEquivalentTo() carry that case on its own, leaving the equals() arm unread and the count of
  that record vacuous, with nothing red to say so.

The javadoc says three things it did not: isServerAt() resolves under the domain lock on the
handshake road, whose cost is the domain rather than one comparison; isConnectedToServerAt() is
hidden from by any named address which resolves here to another replication server, the fall
back host name of setServerURL() included, not only by two servers configured at one address;
and the pair the equals() arm reads as one is read as one under the other arm too, which is this
change rather than the base. In the test, toServerAddressURL() is named in the past tense, and
a dial of an address nothing answers at is one per six passes, the blacklist of OpenIdentityPlatform#935 holding the
five in between.
@vharseko
vharseko force-pushed the feature/multi-homed-peer-recognition branch from 9e51b42 to cde2f96 Compare September 23, 2026 09:30
@vharseko

Copy link
Copy Markdown
Member Author

@maximthomas round 3, cde2f9690c. Every point is taken; the branch is rebased on the current
master first, so the round is the last commit alone.

issue (non-blocking) -- stopReplicationServers() drops a healthy peer which names a loopback
address.
Taken, and the road is this change rather than the base, as you say:
HostPort.normalizeHost() folds every address local to this machine to localhost, so the entry
of this server and the loopback address such a peer names for itself are one address on one port,
while at the base the comparison read the connected address of that peer, its real one, which
never folds that way. disconnectRemovedReplicationServers() now skips the entry of this server
before it builds the list, as runConnect() skips it before it dials, and with the same test:
!oldRSAddress.equals(HostPort.localAddress(getReplicationPort())), which covers every address
local to this machine on the port this server listens on rather than the literal entry.

The port it reads is the new one, which is the right reading of the one case where the two
differ: after a port change an entry on the old port is no longer this server, and a peer which
names it is a second replication server on this machine, whose removal is a removal like any
other.

Pinned by removingTheEntryOfThisServerStopsNoPeer, which is your pin with the positive control
the guard needs beside it: two fake peers register, one naming 127.0.0.1:ports[0] -- the
address this server listens on, which is what the fold makes of the loopback address a peer names
for itself -- and one naming its own entry at 127.0.0.1:ports[1]; both entries then go in a
single applyConfigurationChange(), so the guard alone tells the two apart. Without it the first
peer loses its session and the case is red; the second peer, which does go, is what says the road
reached stopReplicationServers() at all rather than returning early. Nothing had to open up for
it: a ReplServerFakeConfiguration which differs only in its list of replication servers takes
no other branch of applyConfigurationChange().

suggestion -- the equals() arm pin turns on the resolver answering NXDOMAIN. Taken, as the
assertion rather than the SkipException. Before ERROR_TEXT_WRITER.clear() the case now
asserts the ERR_COULD_NOT_SOLVE_HOSTNAME record for nonexistent.invalid, which is the record
each HostPort built from that name logs, so the premise is read where it was assumed -- and it
doubles as the positive control the isEmpty below it lacked. A skip would leave the only pin of
the equals() arm silently unmeasured on such a network, which is the thing worth being loud
about; the assertion names the resolver as what is wrong.

Measured: with UNRESOLVABLE_HOST pointed at a name which resolves, that assertion is the one
which goes red, and it is the only failure in the class.

suggestion -- the connected arm of isSameServerAs() is pinned by no case. Taken as
aSecondSessionFromTheAddressAPeerIsConnectedOnIsReadAsThatPeer, which is your snippet: the
handshake is answered from loopbackAt(ports[1]), the address the registered peer is connected
on, while what answers names 198.51.100.1:ports[1], an address neither handler carries
otherwise, so the named addresses of the two handlers differ and only the address the two
sessions have in common can match them. The duplicate id line it asserts the absence of names
127.0.0.1:ports[1] twice, because the message prints the connected addresses of both handlers,
which for such a pair is one string twice -- and that is the shape of the trade the case is
there to state. A mutant which walks the named arm of other.addresses only goes red on it.

suggestion -- isAlreadyConnectedToRS() resolves under the domain lock. Taken as the
javadoc clause you asked for, in isServerAt(): a pair the names alone do not answer is resolved
on every call, both startFromRemoteRS() and the connect road hold the domain lock across
isAlreadyConnectedToRS(), so a resolver which does not answer holds that domain for its own
timeout on the road where two handlers of one server id name different hosts -- and only
replication servers reach it, a data server handshake making no such comparison. No code change,
as you said.

nitpick -- the isConnectedToServerAt() javadoc bounds the hide narrower than the code.
Taken, close to your wording: what it takes is a named address which resolves here to one this
configuration gives to another replication server -- two of them configured at one address, or
the host name setServerURL() falls back to mapped there by the resolver of this server, which a
clone host name, split DNS or a stale hosts file gives.

nitpick -- "as two servers which name the same resolvable address already are". Taken: "are
under the other arm". The base told such a pair apart by their source addresses, so "already" was
saying of the base what this change does.

nitpick -- the class javadoc says the connect thread dials once per pass. Taken in both
places: once per six passes, a failed dial being blacklisted for the five after it by the
domainTicket + 6 of #935, and the helper now says the connect thread comes back to that address
six passes later.

nitpick -- toServerAddressURL() in the present tense. Taken: the paragraph narrates the
base in the past tense and says the method is what this change removes.

One correction to the table of round two. addresses = singletonList(namedAddress) now kills
two cases rather than one -- its own,
aPeerWhichNamesAnAddressThisConfigurationDoesNotUseIsFoundByTheOneItDialledFrom, and the new
connected arm case, which reads the same road from the other side. The other six mutants each
kill one case and no other.

Runs, on the head of this round:

  • MultiHomedPeerTest, eight cases, green.
  • Eight mutants, each against the whole class, each red: the guard above dropped →
    removingTheEntryOfThisServerStopsNoPeer; isSameServerAs() walking the named arm only →
    aSecondSessionFromTheAddressAPeerIsConnectedOnIsReadAsThatPeer; nonexistent.invalid
    a name which resolves → the premise of the unresolvable case; and the five of round two,
    unchanged but for the row above -- equals() arm dropped and both HostPorts rebuilt inside
    isServerAt() → the unresolvable case (the duplicate id line and the
    ERR_COULD_NOT_SOLVE_HOSTNAME line); getPort() == getPort()
    twoServersSharingAServerIdAreStillReported; the base comparison back in runConnect()
    aPeerRegisteredUnderTheAddressItDialledFromIsFoundByItsConfiguredAddress.
  • The 13 classes of the description -- 91 tests, no failures (80 before the rebase; the
    current master carries more of them). In three invocations of four, four and five classes
    rather than one: this machine was building other branches at the same time, and the single
    invocation was killed for memory before any test ran. Two of the mutant runs failed in setUp
    on binding a port of the embedded server -- another JVM on this machine held 0.0.0.0:65528
    and 0.0.0.0:65534 -- and both were green when re-run, which is how the two rows above were
    measured.
  • mvn -pl opendj-server-legacy package -Dmaven.test.skip=true -- the attach-javadocs gate,
    which the new {@link ReplicationServerDomain#isAlreadyConnectedToRS(ReplicationServerHandler)}
    goes through.

Rebased on the current master before the round, so the round commit carries the round and
nothing else: the three commits replayed unchanged, with no conflict, and git range-diff
reports = for all three. Two of the commits which landed since do touch these files -- #1019,
which reports a ReplicaOfflineMsg as forwarded only to a peer which can decode it, and #987,
which interrupts the listen thread after those messages are forwarded -- and neither touches what
this change owns.

Still not taken, and unchanged from round two: the addresses
ERR_DUPLICATE_REPLICATION_SERVER_ID prints. After the equals() arm no pair it reports carries
one address twice, and the case which pins its text would move with the message; say the word and
it goes in a round of its own.

@maximthomas maximthomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: Every new pin was measured red against the mutant it is there for.

  • ReplicationServer.disconnectRemovedReplicationServers():2049: removing the guard turns removingTheEntryOfThisServerStopsNoPeer red ("Expecting [] to contain [8252]"). That case removes both entries in one change, so the guard alone tells the two peers apart.
  • aSecondSessionFromTheAddressAPeerIsConnectedOnIsReadAsThatPeer kills the named-only isSameServerAs() mutant and addresses = singletonList(namedAddress). Restoring the base skip in runConnect() now turns aPeerRegisteredUnderTheAddressItDialledFromIsFoundByItsConfiguredAddress red. Both mutants survived 4/4 at 58c7218.
  • MultiHomedPeerTest:309-313: the unresolvable case now asserts its own premise before it clears the log.

@vharseko
vharseko merged commit 0a92115 into OpenIdentityPlatform:master Sep 23, 2026
17 checks passed
@vharseko
vharseko deleted the feature/multi-homed-peer-recognition branch September 23, 2026 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug replication tests Test suites: fixing, enabling, un-disabling

Projects

None yet

2 participants