From 9084867d22d222ad971ff6c5529833771982ae2c Mon Sep 17 00:00:00 2001 From: Luca Garulli Date: Sun, 16 Aug 2026 21:24:02 -0400 Subject: [PATCH 1/5] fix(#6267): a peer address that identifies nobody says so, and a membership test stops poisoning its own teardown Five follow-ups from #6221 / #6226. 1. A withheld peer-to-peer endpoint is now reported. getUnambiguousPeerHttpAddress and its HTTPS twin refuse an address two peers resolve to by returning null (#6202), and every caller decided for itself whether to say anything. Neither existing warning covers it: the derive warnings fire whenever an address is derived at all, which a healthy homogeneous StatefulSet also does. There is now a one-time WARNING per protocol - modelled on warnAmbiguousRouting (#6183), but for the peer-to-peer endpoints - naming the peers that could not be told apart, the address they share and the field to declare. HTTP and HTTPS have separate latches. 2. GET /api/v1/cluster reports each peer's httpAddress and, only when it is not that peer's alone, httpAddressAmbiguous: true. Studio renders it as a warning line on the node's card, and the OpenAPI spec documents both. A correctly declared cluster carries neither. 3. The presence matrix (?presence=true) dialled the best-effort address and attributed the answer to the peer - the same unattended dial #6221 fixed for the verify endpoint, with the same failure: every peer was queried on the leader's own endpoint and reported the leader's databases as its own, so the matrix showed every database present everywhere. It resolves through PeerDialAddress now and reports a peer it cannot identify as unreachable, with the reason logged. RaftClusterStatusExporter.exportClusterStatus(), a second cluster-status JSON builder that nothing called, is removed rather than taught about this: the live endpoint is GetClusterHandler, and an unreachable second view of one cluster is how two views drift apart. 4. RESYNC_RETRY_TIMEOUT_MS drops from 120s to 30s, set from the instrument #6226 added rather than from a suspicion: across nine full ha-integration-tests runs (235 tests each) not one wait exceeded the 10s report threshold, and the slowest of the ten classes that use those helpers took 53s wall-clock for the whole class, cluster startup and teardown included. 30s is what the rest of the class already treats as long enough - waitForReplicationIsCompleted, waitAllReplicasAreConnected and the leader-election wait all use it. The report threshold drops to 5s to keep the same resolution for the next cut. 5. DynamicMembershipTest no longer leaves its own teardown holding a peer it evicted to a replica's contract. Each method already gets its own cluster (@BeforeEach/@AfterEach, Raft storage under the deleted database directory), so the issue's shared-cluster premise does not apply and two of the three reported failures were the leaderless-window assert that #6226 already fixed by making findLeaderIndex wait. The third is real and now has a regression test: the base class waits for, and compares, exactly the servers getServerToCheck() names, turning a 30s-per-evicted-server timeout and a DatabaseAreNotIdentical charged to endTest into neither (152s -> 77s for the class, with one more test). Seven await().until(() -> findLeaderIndex() >= 0) wrappers that #6226 made redundant are gone, and three copies of "only the servers still running" collapse into BaseRaftHATest.startedServers(). Tests: new Issue6267AmbiguousPeerAddressWarningTest (3, warning fires once per protocol, names the peers, stays silent on a correct cluster, HTTPS not muted by HTTP) and Issue6267AmbiguousAddressVisibilityIT (3, the status flag on an ambiguous and on a correct cluster, and the presence matrix reporting a peer it cannot identify as unreachable - confirmed failing against the pre-fix dial). New DynamicMembershipTest#removedPeerIsNotHeldToTheClusterConsistencyCheck confirmed failing without the getServerToCheck fix with exactly the reported "DatabaseAreNotIdentical Types: DB1 5 <> DB2 6" at endTest. Run locally: full ha-raft unit lane (918), the 18 IT classes reached by these changes (35 tests) including all ten that use the shortened budget, server PluginApiSpecTest, and a full reactor build. --- docs/release-26.9.1.md | 48 +++++ .../server/ha/raft/GetClusterHandler.java | 27 ++- .../ha/raft/RaftClusterStatusExporter.java | 112 +----------- .../arcadedb/server/ha/raft/RaftHAServer.java | 52 +++++- .../server/ha/raft/BaseRaftHATest.java | 102 ++++++++--- .../server/ha/raft/CapturingTestLogger.java | 52 +++++- .../server/ha/raft/DynamicMembershipTest.java | 81 ++++++++- ...Issue6267AmbiguousAddressVisibilityIT.java | 169 ++++++++++++++++++ ...ue6267AmbiguousPeerAddressWarningTest.java | 131 ++++++++++++++ .../ha/raft/RaftLeaderDown2NodesIT.java | 15 +- .../server/ha/raft/RaftLeaderFailoverIT.java | 16 +- .../server/ha/raft/RaftQuorumLostIT.java | 15 +- .../raft/RaftTimeSeriesOversizedSealedIT.java | 2 - .../RaftTimeSeriesReplication3NodesIT.java | 18 +- .../raft/SuperNodeAppendHAConsistencyIT.java | 3 - .../SuperNodeConcurrentAppendHABenchmark.java | 3 - .../http/handler/openapi/PluginApiSpec.java | 7 + .../handler/openapi/PluginApiSpecTest.java | 13 +- .../resources/static/js/studio-cluster.js | 14 ++ 19 files changed, 663 insertions(+), 217 deletions(-) create mode 100644 ha-raft/src/test/java/com/arcadedb/server/ha/raft/Issue6267AmbiguousAddressVisibilityIT.java create mode 100644 ha-raft/src/test/java/com/arcadedb/server/ha/raft/Issue6267AmbiguousPeerAddressWarningTest.java diff --git a/docs/release-26.9.1.md b/docs/release-26.9.1.md index 305a0b0f94..705b017f38 100644 --- a/docs/release-26.9.1.md +++ b/docs/release-26.9.1.md @@ -2234,3 +2234,51 @@ purpose ("more seeds than nodes" reads as "as many as exist" and is clamped to t nameless `NegativeArraySizeException`. [#6216](https://github.com/ArcadeData/arcadedb/issues/6216) + +## A peer address that identifies nobody says so, instead of waiting for an operation to refuse (#6267) + +Five follow-ups from #6221 / #6226. Four are visible to an operator; the rest are test hygiene. + +**A withheld peer-to-peer endpoint is now reported.** `getUnambiguousPeerHttpAddress` and its HTTPS twin refuse +an address two peers both resolve to by returning `null` (#6202), and every caller then decided for itself +whether to say anything - so the refusal was visible only where one happened to log it, and invisible everywhere +else. Neither existing warning covered it: the derive warnings fire whenever an address is derived **at all**, +which is also what a perfectly healthy homogeneous Kubernetes StatefulSet does, so they cannot distinguish +"deriving, and fine" from "deriving, and two peers just collapsed onto one address". There is now a one-time +WARNING per protocol - modelled on the `warnAmbiguousRouting` of #6183, but for the peer-to-peer endpoints rather +than the client routing tables - naming the peers that could not be told apart, the address they share, and the +`host:{raft:..,http:..}` field to declare in `arcadedb.ha.serverList`. HTTP and HTTPS have separate latches: a +cluster that declares distinct `http` ports and shares an `https` one must still hear about the second. + +**Observable change in `GET /api/v1/cluster`.** Each peer entry now carries `httpAddress` and, only when it is +not the peer's alone, `httpAddressAmbiguous: true`. Before this, the status endpoint and the Studio HA panel +displayed a plausible address for every peer with nothing to say that it named none of them, and an operator +found out when a snapshot resync or a cluster verify refused to dial. A correctly declared cluster carries +neither field's flag, so nothing changes for one. Studio renders the flag as a warning line on the node's card. + +**The presence matrix dialled the address nothing had checked.** `GET /api/v1/cluster?presence=true` asks every +peer which databases it holds and attributes the answer to that peer - the same unattended dial the verify +endpoint was making before #6221, with the same failure: on a cluster whose peers collapse onto one derived +address, every peer was queried on the leader's own endpoint and reported the leader's database list as its own, +so the matrix showed every database present on every node. It resolves through `PeerDialAddress` now, so a peer +it cannot identify is reported in `unreachable` - with the reason logged - rather than answered for by whoever +picked up. + +`RaftClusterStatusExporter.exportClusterStatus()`, a second cluster-status JSON builder that nothing has called, +was removed rather than taught about any of this: the live endpoint is `GetClusterHandler`, and an unreachable +second view of one cluster is how two views drift apart. + +**Test-only, in the HA lane.** `BaseRaftHATest.RESYNC_RETRY_TIMEOUT_MS` drops from 120 s to 30 s. #6226 added an +instrument rather than guessing, and it has now reported: across nine full `ha-integration-tests` runs (235 tests +each) not one wait exceeded the 10 s report threshold, and the slowest of the ten classes that use those helpers +took 53 s wall-clock for the whole class, cluster startup and teardown included. 30 s is what the rest of that +class already treats as long enough for a cluster to do anything it is going to do - `waitForReplicationIsCompleted`, +`waitAllReplicasAreConnected` and the leader-election wait all use it - so the one budget with no measurement +behind it was also the only one four times larger than its siblings. The report threshold drops to 5 s with it, to +keep the same resolution for the next cut. `DynamicMembershipTest` no longer leaves its own teardown holding a +peer it evicted to a replica's contract: the base class now waits for, and compares, exactly the servers +`getServerToCheck()` names, which turned a 30 s-per-evicted-server timeout and a `DatabaseAreNotIdentical` charged +to `endTest` into neither. Seven `await().until(() -> findLeaderIndex() >= 0)` wrappers that #6226 made redundant +are gone, and three copies of "only the servers still running" collapse into one helper. + +[#6267](https://github.com/ArcadeData/arcadedb/issues/6267) diff --git a/ha-raft/src/main/java/com/arcadedb/server/ha/raft/GetClusterHandler.java b/ha-raft/src/main/java/com/arcadedb/server/ha/raft/GetClusterHandler.java index 19d783a637..a8db14e8f0 100644 --- a/ha-raft/src/main/java/com/arcadedb/server/ha/raft/GetClusterHandler.java +++ b/ha-raft/src/main/java/com/arcadedb/server/ha/raft/GetClusterHandler.java @@ -123,6 +123,19 @@ public ExecutionResponse execute(final HttpServerExchange exchange, final Server final String peerId = peer.getId().toString(); peerJson.put("id", peerId); peerJson.put("address", peer.getAddress()); + // The peer's HTTP endpoint as resolved, plus whether that endpoint identifies this peer and no other. + // With no 'http' port declared in arcadedb.ha.serverList a peer's endpoint is derived as its Raft host + // plus THIS node's port, so on a cluster whose nodes differ by port every peer collapses onto one + // address. Reporting the address alone would show an operator a plausible endpoint per peer with nothing + // to say that it names none of them, and they would find out only when a resync or a verify refuses to + // dial. The flag is present only when set, so a correctly declared cluster carries no extra field + // (issue #6267). + final String httpAddress = raftHAServer.getPeerHttpAddress(peer.getId()); + if (httpAddress != null) { + peerJson.put("httpAddress", httpAddress); + if (raftHAServer.getUnambiguousPeerHttpAddress(peer.getId()) == null) + peerJson.put("httpAddressAmbiguous", true); + } final boolean peerIsLeader = leaderId != null && peer.getId().equals(leaderId); peerJson.put("role", peerIsLeader ? "LEADER" : "FOLLOWER"); @@ -250,15 +263,21 @@ private JSONObject buildPresenceMatrix(final RaftHAServer raftHAServer, final Ra if (!dbName.startsWith(ArcadeDBServer.RESERVED_DATABASE_PREFIX)) dbNames.add(dbName); } else { - final String httpAddr = raftHAServer.getPeerHttpAddress(peerId); - if (httpAddr == null) { + // The guarded address, not the best-effort one (issue #6267). This fan-out attributes whatever comes back + // to peerIdStr, so an address that resolves to the wrong node - or to this one - fills the matrix with a + // reassuring answer nobody asked for: on a cluster whose peers collapse onto one derived address, every + // peer would report the local node's databases and the matrix would show them present everywhere. Same + // guard the resync and verify paths use, so the three cannot drift apart. + final PeerDialAddress dial = PeerDialAddress.resolve(raftHAServer, peerId, "peer"); + if (dial.refused()) { + LogManager.instance().log(this, Level.WARNING, + "Presence matrix: not querying peer '%s': %s", peerIdStr, dial.refusal()); unreachable.add(peerIdStr); continue; } - final String httpsAddr = raftHAServer.getPeerHttpsAddress(peerId); try { final List infos = - LeaderDatabaseQuery.fetch(httpAddr, httpsAddr, clusterToken, timeoutMs, server); + LeaderDatabaseQuery.fetch(dial.httpAddress(), dial.httpsAddress(), clusterToken, timeoutMs, server); for (final LeaderDatabaseQuery.DatabaseInfo info : infos) dbNames.add(info.name()); } catch (final InterruptedException e) { diff --git a/ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftClusterStatusExporter.java b/ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftClusterStatusExporter.java index c0c1660d4b..14c49c8a74 100644 --- a/ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftClusterStatusExporter.java +++ b/ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftClusterStatusExporter.java @@ -34,8 +34,12 @@ import java.util.logging.Level; /** - * Exports cluster status as JSON, prints cluster configuration tables, and manages - * the replication lag monitor. + * Prints the cluster configuration table and manages the replication lag monitor. + *

+ * It used to also build a cluster-status JSON, which nothing called: the live status endpoint is + * {@link GetClusterHandler}, which assembles its own. A second, unreachable builder of the same document is how + * two views of one cluster drift apart - and it did, since the reachable one reports the peer-address ambiguity + * of issue #6267 and this one never would have - so it was removed rather than kept in step by hand. */ class RaftClusterStatusExporter { @@ -52,110 +56,6 @@ class RaftClusterStatusExporter { this.clusterMonitor = clusterMonitor; } - // -- Status Export -- - - JSONObject exportClusterStatus() { - final var haJSON = new JSONObject(); - - haJSON.put("protocol", "ratis"); - haJSON.put("clusterName", haServer.getClusterName()); - haJSON.put("leader", haServer.getLeaderName()); - // Raw role vs. ability to serve: a freshly elected leader rejects writes until it has committed its - // current-term no-op (issue #5453). Both come from one snapshot so the pair is never contradictory. - final RaftHAServer.LeadershipState leadership = haServer.getLeadershipState(); - haJSON.put("isLeader", leadership.leader()); - haJSON.put("leaderReady", leadership.leaderReady()); - haJSON.put("localPeerId", haServer.getLocalPeerId().toString()); - haJSON.put("configuredServers", haServer.getConfiguredServers()); - haJSON.put("quorum", haServer.getQuorum().name()); - haJSON.put("currentTerm", haServer.getCurrentTerm()); - haJSON.put("commitIndex", haServer.getCommitIndex()); - haJSON.put("lastAppliedIndex", haServer.getLastAppliedIndex()); - - // Peer list with replication state (follower indices available only on leader) - final var followerStates = haServer.getFollowerStates(); - final var replicationLatencies = haServer.getReplicationLatencies(); - final var peers = new JSONArray(); - final RaftPeerId leaderId = haServer.getLeaderId(); - for (final RaftPeer peer : haServer.getLivePeers()) { - final var peerJSON = new JSONObject(); - final String peerId = peer.getId().toString(); - peerJSON.put("id", peerId); - peerJSON.put("address", peer.getAddress()); - peerJSON.put("httpAddress", haServer.getPeerHttpAddress(peer.getId())); - peerJSON.put("isLocal", peer.getId().equals(haServer.getLocalPeerId())); - peerJSON.put("role", leaderId != null && peer.getId().equals(leaderId) ? "LEADER" : "FOLLOWER"); - - for (final var fs : followerStates) - if (peerId.equals(fs.get("peerId"))) { - peerJSON.put("matchIndex", fs.get("matchIndex")); - peerJSON.put("nextIndex", fs.get("nextIndex")); - // Time since the leader last heard from this follower (issue #5314): the honest meaning of the - // value the CLUSTER CONFIGURATION table used to mislabel as "LATENCY". - peerJSON.put("lastContactMs", fs.get("lastRpcElapsedMs")); - // Real measured appendEntries/heartbeat round-trip latency (issue #5314), load-independent. - final RaftHAServer.ReplicationLatency rtt = replicationLatencies.get(peerId); - if (rtt != null) { - peerJSON.put("replicationRttMs", rtt.meanMs()); - peerJSON.put("replicationRttP99Ms", rtt.p99Ms()); - } - if (clusterMonitor != null) { - final var lags = clusterMonitor.getReplicaLags(); - final Long lag = lags.get(peerId); - if (lag != null) - peerJSON.put("lagging", lag > clusterMonitor.getLagWarningThreshold() - && clusterMonitor.getLagWarningThreshold() > 0); - // Studio renders this as a colored badge in the cluster view, so a STALLED follower - // jumps out at the operator without having to compare numbers in their head. - peerJSON.put("replicaStatus", clusterMonitor.getReplicaStatus(peerId).name()); - } - break; - } - - peers.put(peerJSON); - } - haJSON.put("peers", peers); - - // Database list - final var databases = new JSONArray(); - final var stateMachineForBaseline = haServer.getStateMachine(); - for (final String dbName : haServer.getServer().getDatabaseNames()) { - // Never expose reserved internal databases (e.g. the Raft control directory '.raft'). - if (ArcadeDBServer.isReservedDatabaseName(dbName)) - continue; - final var databaseJSON = new JSONObject(); - databaseJSON.put("name", dbName); - databaseJSON.put("quorum", haServer.getQuorum().name()); - - // Surface the bootstrap baseline applied via BOOTSTRAP_FINGERPRINT_ENTRY (#4147 phase 7). - // Null when no bootstrap entry has been committed for this database yet, which is the - // normal case for clusters that pre-date #4147 or that never engaged the bootstrap path. - final var baseline = stateMachineForBaseline != null - ? stateMachineForBaseline.getBootstrapBaseline(dbName) : null; - if (baseline != null) { - databaseJSON.put("bootstrapLastTxId", baseline.lastTxId()); - databaseJSON.put("bootstrapFingerprint", baseline.fingerprint()); - } - databases.put(databaseJSON); - } - haJSON.put("databases", databases); - - // Metrics - final var stateMachine = haServer.getStateMachine(); - final var metricsJSON = new JSONObject(); - metricsJSON.put("electionCount", stateMachine.getElectionCount()); - metricsJSON.put("lastElectionTime", stateMachine.getLastElectionTime()); - metricsJSON.put("startTime", stateMachine.getStartTime()); - metricsJSON.put("lagWarningThreshold", clusterMonitor.getLagWarningThreshold()); - haJSON.put("metrics", metricsJSON); - - // Required by RemoteHttpComponent for cluster configuration - haJSON.put("leaderAddress", haServer.getLeaderHttpAddress()); - haJSON.put("replicaAddresses", haServer.getReplicaAddresses()); - - return haJSON; - } - // -- Cluster Configuration Printing -- /** diff --git a/ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftHAServer.java b/ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftHAServer.java index c33d680f02..9437624cc9 100644 --- a/ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftHAServer.java +++ b/ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftHAServer.java @@ -153,6 +153,11 @@ public class RaftHAServer implements HealthMonitor.HealthTarget { private final AtomicBoolean httpFallbackWarned = new AtomicBoolean(false); // Logged at most once: notes that peer HTTPS endpoints are derived from this node's local HTTPS port. private final AtomicBoolean httpsFallbackWarned = new AtomicBoolean(false); + // Logged at most once per protocol: warns that a peer-to-peer endpoint was WITHHELD because two peers + // resolved to it. Distinct from the two latches above, which fire whenever an address is derived at all - + // which a healthy homogeneous StatefulSet also does (issue #6267). + private final AtomicBoolean httpAmbiguityWarned = new AtomicBoolean(false); + private final AtomicBoolean httpsAmbiguityWarned = new AtomicBoolean(false); // Client-reachable Bolt endpoints (optional object-form 'bolt' field in HA_SERVER_LIST). Advertised // in the Bolt ROUTE routing table so neo4j:// drivers can discover leader/followers. private final Map boltAddresses = new HashMap<>(); @@ -1939,8 +1944,10 @@ private String unambiguousPeerAddress(final RaftPeerId peerId, final boolean htt // occupies a slot beyond it - the same sizing getRoutingTable uses, for the same reason. final String[] addresses = new String[peers.size() + 1]; final boolean[] fromConfig = new boolean[addresses.length]; + final RaftPeerId[] owners = new RaftPeerId[addresses.length]; addresses[0] = address; fromConfig[0] = declared.containsKey(peerId); + owners[0] = peerId; int resolved = 1; for (final RaftPeer peer : peers) { @@ -1951,11 +1958,54 @@ private String unambiguousPeerAddress(final RaftPeerId peerId, final boolean htt continue; addresses[resolved] = other; fromConfig[resolved] = declared.containsKey(peer.getId()); + owners[resolved] = peer.getId(); ++resolved; } final Map claims = claimsByAddress(addresses, fromConfig, resolved); - return identifiesOnePeer(claims.get(address), fromConfig[0]) ? address : null; + if (identifiesOnePeer(claims.get(address), fromConfig[0])) + return address; + + warnAmbiguousPeerAddress(https, address, addresses, owners, resolved); + return null; + } + + /** + * Tells the operator, once per protocol, that a peer-to-peer endpoint was withheld - which peers + * could not be told apart, and what to write to fix it. + *

+ * Without this the refusal is silent (issue #6267): {@link #unambiguousPeerAddress} answers {@code null} and + * every caller decides for itself whether to say anything, so the misconfiguration is visible only where one + * happens to log it. Neither existing warning covers it. {@link #deriveHttpAddressWithWarning} and its HTTPS + * twin fire whenever an address is derived at all, which is also what a perfectly healthy homogeneous + * Kubernetes StatefulSet does, so they cannot distinguish "deriving, and fine" from "deriving, and two peers + * just collapsed onto one address"; {@link #warnAmbiguousRouting} says exactly this, but about the client + * routing tables of issue #6183, not about the peer-to-peer endpoints a resync and a cluster verify dial. + *

+ * Once per protocol rather than once per attempt: the resync and verify paths ask on every attempt, and a + * misconfiguration that does not change between attempts should not be re-reported on each one. + */ + private void warnAmbiguousPeerAddress(final boolean https, final String address, final String[] addresses, + final RaftPeerId[] owners, final int count) { + if (!(https ? httpsAmbiguityWarned : httpAmbiguityWarned).compareAndSet(false, true)) + return; + + final StringBuilder shared = new StringBuilder(); + for (int i = 0; i < count; i++) + if (address.equals(addresses[i])) { + if (!shared.isEmpty()) + shared.append(", "); + shared.append(owners[i]); + } + + final String protocol = https ? "HTTPS" : "HTTP"; + final String field = https ? "https" : "http"; + LogManager.instance().log(this, Level.WARNING, + "HA %s peer endpoints are ambiguous: peers %s all resolve to %s, which two listening sockets cannot both own. " + + "Their %s endpoint is withheld rather than guessed, so a snapshot resync and a cluster verify refuse to " + + "dial them and report them unverified instead of answering for the wrong node. Declare each node's %s port " + + "explicitly with the 'host:{raft:..,%s:..}' object syntax in %s.", + protocol, shared, address, protocol, protocol, field, GlobalConfiguration.HA_SERVER_LIST.getKey()); } /** Tells the operator, once per protocol, that peers shared an address and what to write to fix it. */ diff --git a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/BaseRaftHATest.java b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/BaseRaftHATest.java index 4213b9951b..08f0e86768 100644 --- a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/BaseRaftHATest.java +++ b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/BaseRaftHATest.java @@ -31,8 +31,10 @@ import org.apache.ratis.protocol.RaftPeerId; import java.io.File; +import java.util.Arrays; import java.util.Map; import java.util.function.Function; +import java.util.function.IntPredicate; import java.util.function.LongSupplier; import java.util.logging.Level; @@ -44,28 +46,38 @@ public abstract class BaseRaftHATest extends BaseGraphServerTest { private static final int BASE_RAFT_PORT = 2434; - // 120s: two independent CI runs of this PR (runs 31578870619 and 31587763511) each showed - // Issue5410AbandonedTicketReleaseIT stall for the ENTIRE poll budget - once at 30s, once at 60s - - // immediately after RaftMigrationCompactionRaceIT (a 72k-record, 24-compaction-round, 12-writer- - // thread IT) ran in the same shared, reused JVM fork (failsafe reuseForks=true/forkCount=1 for the - // whole module). Both stalls were confirmed real via the test's own embedded log timestamps, not a - // log-flush artifact. Not reproducible locally in isolation, nor under an emulated 4-CPU/3.9GB-heap - // constraint matching the CI runner - only in the full-suite adjacency. The suspected cause was - // ArcadeStateMachine.notifyInstallSnapshotFromLeader running its download on the JDK common - // ForkJoinPool, which is also shared with JDK GC/reference handler internals - exactly the - // "long-running [GC-heavy] work starves engine work" failure shape this looks like. That caller has - // its own executor since issue #6202, so half of the suspicion is gone; the timeout stays until a - // CI run shows it can come down, and the other half of the fix - isolating heavy ITs into their own - // fork - is still a follow-up. The bump costs nothing when nothing stalls: withResyncRetry(), - // awaitValue() and awaitCountOn() all return as soon as the condition is met. + // 30s, down from 120s, and set from a measurement rather than from a suspicion (issue #6267). // - // #6221 asked whether it can come down now that the download has its own executor, and the honest answer is - // that nobody knows: a budget nothing consumes leaves no trace of how much of it was needed, and this lane is - // chronically red (#5668, #5702) so a single green run proves nothing either way. So the waits now report what - // they actually cost (see reportSlowWait): a handful of CI runs turn "lower it and see" into a measurement. - private static final long RESYNC_RETRY_TIMEOUT_MS = 120_000; - /** Above this, a wait is worth a line in the log: it is evidence about the budget above, not noise. */ - private static final long SLOW_WAIT_REPORT_MS = 10_000; + // The 120s was a reaction to two CI runs (31578870619, 31587763511) in which + // Issue5410AbandonedTicketReleaseIT stalled for the ENTIRE poll budget - once at 30s, once at 60s - + // immediately after RaftMigrationCompactionRaceIT (a 72k-record, 24-compaction-round, 12-writer-thread IT) + // ran in the same shared, reused JVM fork (failsafe reuseForks=true/forkCount=1 for the whole module). The + // stalls were real, not a log-flush artifact, and never reproduced in isolation. The suspected cause was + // ArcadeStateMachine.notifyInstallSnapshotFromLeader running its download on the JDK common ForkJoinPool, + // shared with JDK GC/reference-handler internals - the "long-running work starves engine work" shape. That + // caller has had its own executor since issue #6202, and #6221/#6226 added the instrument below rather than + // guessing: a budget nothing exhausts leaves no trace of how much of it was needed. + // + // The measurement, over nine full ha-integration-tests runs since #6226 merged (31968696717, 31969178061, + // 31969810563, 31972218219, 31975575222, 31977924355, 31980155942, 31980224898 and the merge run itself), + // 235 tests each: NOT ONE wait exceeded the 10s report threshold. The corroboration is the per-class + // elapsed time - all ten classes that call these helpers ran in every one of those runs, and the slowest + // of them took 53s WALL CLOCK for the whole class, cluster startup and teardown included, so no single + // wait inside it can have approached even half the old budget. + // + // 30s is what the rest of this class already treats as "long enough for the cluster to do anything it is + // going to do": waitForReplicationIsCompleted, waitAllReplicasAreConnected and LEADER_ELECTION_TIMEOUT_MS + // all use it. The budget that had no measurement behind it was also the only one four times larger than + // its siblings; it is now one of them. It stays generous - three times the largest wait the instrument can + // prove any of those runs needed - while a genuine hang costs 90s less before it is reported. + private static final long RESYNC_RETRY_TIMEOUT_MS = 30_000; + /** + * Above this, a wait is worth a line in the log: it is evidence about the budget above, not noise. Lowered + * to 5s with the budget (issue #6267) to keep the same resolution: at the old 10s a wait could consume a + * third of the new budget and still say nothing, which is the blindness that let the 120s stand unmeasured + * for as long as it did. + */ + private static final long SLOW_WAIT_REPORT_MS = 5_000; /** * How long {@link #findLeaderIndex()} waits for an election before answering "no leader". An election in these * in-process clusters settles in a second or two; the budget is for the loaded CI runner where the question @@ -269,17 +281,49 @@ protected RaftHAPlugin getRaftPlugin(final int serverIndex) { return null; } + /** + * Ensures the replicas about to be compared have caught up, then compares them. The base {@code endTest()} + * calls the comparison directly, without a Raft-aware wait. + *

+ * The wait covers exactly {@link #getServerToCheck()} - the servers the comparison will look at - rather than + * every configured one. The two sets differ only for a test that takes a server out of the Raft group + * ({@code DynamicMembershipTest}): a peer that is no longer a member never applies another entry, so waiting + * for it to reach the leader's last-applied index can only burn the full 30 s budget per evicted server and + * then log a timeout, and comparing it can only report the divergence the eviction was asking for. Both are + * charged to {@code endTest}, which is the wrong place to read about a peer some earlier line removed on + * purpose (issue #6267). + */ @Override protected void checkDatabasesAreIdentical() { - // Ensure all Raft replicas have caught up before comparing pages. - // The base endTest() calls this directly without a Raft-aware wait. - for (int i = 0; i < getServerCount(); i++) { + for (final int i : getServerToCheck()) if (getServer(i) != null && getServer(i).isStarted()) waitForReplicationIsCompleted(i); - } super.checkDatabasesAreIdentical(); } + /** + * The subset of server indexes {@code keep} accepts, in index order, as the {@code int[]} + * {@link #getServerToCheck()} is declared to return. The one place that turns a per-server predicate into that + * array, so an override does not have to hand-roll the two-pass count-then-fill each time. + */ + protected int[] serversMatching(final IntPredicate keep) { + final int count = getServerCount(); + final int[] buffer = new int[count]; + int found = 0; + for (int i = 0; i < count; i++) + if (keep.test(i)) + buffer[found++] = i; + return found == count ? buffer : Arrays.copyOf(buffer, found); + } + + /** + * The servers that are currently running. The default set for a test that deliberately stops one: a stopped + * server has no database to compare, and nothing to wait for. + */ + protected int[] startedServers() { + return serversMatching(i -> getServer(i) != null && getServer(i).isStarted()); + } + /** * Waits for every running server in the cluster to apply entries up to the current * leader's last-applied index. Use this after a write before reading from all servers @@ -464,8 +508,10 @@ protected long awaitCountOn(final int serverIndex, final String typeName, final /** * Records how much of {@link #RESYNC_RETRY_TIMEOUT_MS} a wait actually consumed, when it consumed enough to be * worth knowing. A budget that is never exhausted leaves no evidence of how much of it was needed, which is - * exactly why the 120 s above has stood on a suspicion rather than on a measurement (issue #6221): a wait that - * satisfies in 300 ms and one that satisfies at 95 s are indistinguishable in a green run. + * exactly why the old 120 s stood on a suspicion rather than on a measurement (issue #6221): a wait that + * satisfies in 300 ms and one that satisfies at 95 s are indistinguishable in a green run. Nine runs of + * silence are what let it come down to 30 s (issue #6267), and the instrument stays for the same reason it + * was added - the next cut, or the case for putting it back, has to come from evidence too. *

* Logged, not asserted: a slow wait is not a failure, and a test that failed the moment a CI runner was busy * would be a worse trade than the timeout it was meant to justify. @@ -475,7 +521,7 @@ private void reportSlowWait(final String what, final long startMs, final boolean if (elapsed < SLOW_WAIT_REPORT_MS) return; LogManager.instance().log(this, Level.WARNING, - "TEST: %s %s after %d ms of the %d ms budget (issue #6221: evidence for whether that budget can come down)", + "TEST: %s %s after %d ms of the %d ms budget (issue #6267: evidence for whether that budget can come down further)", what, satisfied ? "satisfied" : "GAVE UP", elapsed, RESYNC_RETRY_TIMEOUT_MS); } } diff --git a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/CapturingTestLogger.java b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/CapturingTestLogger.java index e7090d306f..03c6e319b8 100644 --- a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/CapturingTestLogger.java +++ b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/CapturingTestLogger.java @@ -22,6 +22,7 @@ import com.arcadedb.log.LogManager; import com.arcadedb.log.Logger; +import java.util.Arrays; import java.util.List; import java.util.concurrent.CopyOnWriteArrayList; import java.util.logging.Level; @@ -35,7 +36,9 @@ */ final class CapturingTestLogger implements Logger { - private final List messages = new CopyOnWriteArrayList<>(); + private final List messages = new CopyOnWriteArrayList<>(); + /** The same messages with their arguments substituted; see {@link #countFormattedContaining}. */ + private final List formatted = new CopyOnWriteArrayList<>(); static CapturingTestLogger install() { final CapturingTestLogger logger = new CapturingTestLogger(); @@ -48,8 +51,22 @@ void uninstall() { } int countContaining(final String... needles) { + return countIn(messages, needles); + } + + /** + * Like {@link #countContaining}, but over the messages with their arguments substituted - for an assertion + * about what a warning actually said (which peers, which address) rather than that it fired at all. + * A message whose arguments cannot be substituted is counted in its raw form, so a format that changes shape + * fails the assertion instead of silently vanishing from the list. + */ + int countFormattedContaining(final String... needles) { + return countIn(formatted, needles); + } + + private static int countIn(final List where, final String... needles) { int n = 0; - for (final String m : messages) { + for (final String m : where) { boolean all = true; for (final String needle : needles) if (!m.contains(needle)) { @@ -67,15 +84,38 @@ public void log(final Object iRequester, final Level iLevel, final String iMessa final String context, final Object arg1, final Object arg2, final Object arg3, final Object arg4, final Object arg5, final Object arg6, final Object arg7, final Object arg8, final Object arg9, final Object arg10, final Object arg11, final Object arg12, final Object arg13, final Object arg14, final Object arg15, final Object arg16, final Object arg17) { - if (iMessage != null) - messages.add(iMessage); + record(iMessage, arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8, arg9, arg10, arg11, arg12, arg13, arg14, arg15, + arg16, arg17); } @Override public void log(final Object iRequester, final Level iLevel, final String iMessage, final Throwable iException, final String context, final Object... args) { - if (iMessage != null) - messages.add(iMessage); + record(iMessage, args); + } + + private void record(final String message, final Object... args) { + if (message == null) + return; + messages.add(message); + formatted.add(substitute(message, args)); + } + + /** + * Substitutes {@code args} into {@code message}. Trailing nulls are dropped first: the fixed-arity overload + * above always passes 17 slots, and {@code String.format} would render the unused ones as literal "null". + */ + private static String substitute(final String message, final Object[] args) { + int used = args.length; + while (used > 0 && args[used - 1] == null) + --used; + if (used == 0) + return message; + try { + return String.format(message, Arrays.copyOf(args, used)); + } catch (final RuntimeException e) { + return message; + } } @Override diff --git a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/DynamicMembershipTest.java b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/DynamicMembershipTest.java index fbc090eac1..67db261edc 100644 --- a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/DynamicMembershipTest.java +++ b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/DynamicMembershipTest.java @@ -24,18 +24,49 @@ import org.junit.jupiter.api.Test; import java.util.Collection; +import java.util.HashSet; +import java.util.Set; +import com.arcadedb.database.Database; import com.arcadedb.exception.ConfigurationException; import static org.assertj.core.api.Assertions.assertThat; +/** + * Cluster membership changes: reading the group, removing a peer, the quorum guard that refuses a removal, and + * the peer display-name registry. + *

+ * Every method here runs against its own 3-node cluster - {@code beginTest}/{@code endTest} are + * {@code @BeforeEach}/{@code @AfterEach} and the Raft storage lives under the database directory the setup + * deletes - so a removal never reaches the next method. What it does reach is this method's teardown: + * the base class waits for every configured server to catch up to the leader and then compares their databases, + * and a peer this test evicted does neither. It never applies another entry, so the wait can only burn its full + * budget and log a timeout, and the comparison can only report the divergence the eviction asked for - both + * charged to {@code endTest}, which is the wrong place to read about a peer some earlier line removed on + * purpose. {@link #getServerToCheck()} below takes the evicted servers out of both (issue #6267). + * + * @author Luca Garulli (l.garulli@arcadedata.com) + */ class DynamicMembershipTest extends BaseRaftHATest { + /** Servers this test took out of the Raft group; see {@link #getServerToCheck()}. */ + private final Set evictedServers = new HashSet<>(); + @Override protected int getServerCount() { return 3; } + /** + * The cluster as this test left it: the servers still in the Raft group. An evicted peer keeps running and + * keeps its database open - it is simply no longer a replica, and holding it to a replica's contract at + * teardown reports a failure nobody can act on. + */ + @Override + protected int[] getServerToCheck() { + return serversMatching(i -> !evictedServers.contains(i)); + } + @Test void getLivePeersReturnsAllConfiguredPeers() { final int leaderIndex = findLeaderIndex(); @@ -53,15 +84,42 @@ void removePeerDecreasesClusterSize() { // Pick a non-leader peer to remove, since Ratis requires the leader to process the change final int targetIndex = leaderIndex == 0 ? 2 : 0; - final String targetPeerId = peerIdForIndex(targetIndex); final RaftHAServer raftServer = getRaftPlugin(leaderIndex).getRaftHAServer(); assertThat(raftServer.getLivePeers()).hasSize(3); - raftServer.removePeer(targetPeerId); + evict(raftServer, targetIndex, false); assertThat(raftServer.getLivePeers()).hasSize(2); } + /** + * A peer removed from the group stops replicating, and the cluster's remaining members must not be held to + * matching it. Reproduces the {@code endTest > checkDatabasesAreIdentical} failure this class used to leave + * behind: without {@link #getServerToCheck()} the write below reaches the two members and not the evicted + * peer, and the teardown comparison fails in a method that did nothing wrong. + */ + @Test + void removedPeerIsNotHeldToTheClusterConsistencyCheck() { + final int leaderIndex = findLeaderIndex(); + assertThat(leaderIndex).isGreaterThanOrEqualTo(0); + + final int targetIndex = leaderIndex == 0 ? 2 : 0; + final RaftHAServer raftServer = getRaftPlugin(leaderIndex).getRaftHAServer(); + evict(raftServer, targetIndex, false); + + // A write the remaining members commit and the evicted peer, by definition, never sees. + final Database leaderDB = getServer(leaderIndex).getDatabase(getDatabaseName()); + leaderDB.transaction(() -> { + leaderDB.command("sql", "CREATE DOCUMENT TYPE AfterEviction"); + leaderDB.command("sql", "INSERT INTO AfterEviction SET id = 1"); + }); + + assertThat(leaderDB.getSchema().existsType("AfterEviction")).isTrue(); + assertThat(getServer(targetIndex).getDatabase(getDatabaseName()).getSchema().existsType("AfterEviction")) + .as("an evicted peer must not receive entries committed after its removal") + .isFalse(); + } + @Test void removePeerRefusedWhenItWouldBreakQuorum() { final int leaderIndex = findLeaderIndex(); @@ -72,7 +130,7 @@ void removePeerRefusedWhenItWouldBreakQuorum() { // 3 -> 2 is allowed (quorum of 3 is 2). Remove a non-leader so the leader can commit the change. final int firstTarget = leaderIndex == 0 ? 2 : 0; - raftServer.removePeer(peerIdForIndex(firstTarget)); + evict(raftServer, firstTarget, false); assertThat(raftServer.getLivePeers()).hasSize(2); // 2 -> 1 would drop below quorum: must be refused without force. @@ -81,14 +139,27 @@ void removePeerRefusedWhenItWouldBreakQuorum() { .isInstanceOf(ConfigurationException.class) .hasMessageContaining("quorum"); - // The cluster configuration is untouched by the refused removal. + // The cluster configuration is untouched by the refused removal, so the peer is still a member. assertThat(raftServer.getLivePeers()).hasSize(2); // With force=true the same removal proceeds. - raftServer.removePeer(peerIdForIndex(secondTarget), true); + evict(raftServer, secondTarget, true); assertThat(raftServer.getLivePeers()).hasSize(1); } + /** + * Removes a peer from the group and records that it is no longer a replica. Recorded only once the removal + * has returned: a refused removal leaves the peer a member, and excluding it from the teardown check would + * hide exactly the divergence the refusal exists to prevent. + */ + private void evict(final RaftHAServer raftServer, final int serverIndex, final boolean force) { + if (force) + raftServer.removePeer(peerIdForIndex(serverIndex), true); + else + raftServer.removePeer(peerIdForIndex(serverIndex)); + evictedServers.add(serverIndex); + } + private int pickRemainingNonLeader(final RaftHAServer raftServer, final int leaderIndex, final int alreadyRemoved) { final String localPeer = raftServer.getLocalPeerId().toString(); for (int i = 0; i < getServerCount(); i++) { diff --git a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/Issue6267AmbiguousAddressVisibilityIT.java b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/Issue6267AmbiguousAddressVisibilityIT.java new file mode 100644 index 0000000000..6f6ec3b66a --- /dev/null +++ b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/Issue6267AmbiguousAddressVisibilityIT.java @@ -0,0 +1,169 @@ +/* + * Copyright © 2021-present Arcade Data Ltd (info@arcadedata.com) + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * SPDX-FileCopyrightText: 2021-present Arcade Data Ltd (info@arcadedata.com) + * SPDX-License-Identifier: Apache-2.0 + */ +package com.arcadedb.server.ha.raft; + +import com.arcadedb.serializer.json.JSONArray; +import com.arcadedb.serializer.json.JSONObject; +import org.apache.ratis.protocol.RaftPeerId; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; + +import java.net.HttpURLConnection; +import java.net.URI; +import java.nio.charset.StandardCharsets; +import java.util.Base64; +import java.util.HashMap; +import java.util.Map; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * {@code GET /api/v1/cluster} on a cluster whose peer addresses identify nobody (issue #6267). + *

+ * With no {@code http} port declared in {@code arcadedb.ha.serverList} every peer's HTTP endpoint is derived as + * its Raft host plus this node's port, so on a cluster whose nodes differ by port they all collapse onto + * one address. Two things used to hide that from the operator, and both pointed the reassuring way: + *

+ * The ambiguity is injected by emptying the leader's resolved HTTP address map for the duration of one test, as + * {@link Issue6221VerifyFanOutGuardIT} does, rather than by starting the cluster misconfigured: it reproduces the + * production condition at the point where it matters without making cluster startup part of what is under test. + * + * @author Luca Garulli (l.garulli@arcadedata.com) + */ +class Issue6267AmbiguousAddressVisibilityIT extends BaseRaftHATest { + + @Override + protected int getServerCount() { + return 3; + } + + @Test + @Timeout(120) + void anAmbiguousPeerAddressIsFlaggedInTheClusterStatus() throws Exception { + final int leader = findLeaderIndex(); + assertThat(leader).as("a Raft leader must be elected").isGreaterThanOrEqualTo(0); + + final RaftHAServer raft = getRaftPlugin(leader).getRaftHAServer(); + // The live map, by contract (see getHttpAddresses): writing to it is how this test puts the running node + // into the misconfigured state. + final Map httpAddresses = raft.getHttpAddresses(); + final Map declared = new HashMap<>(httpAddresses); + try { + httpAddresses.clear(); // nothing declared: every peer now derives to this node's own endpoint + + final JSONArray peers = getCluster(leader, "").getJSONArray("peers"); + assertThat(peers.length()).isEqualTo(getServerCount()); + for (int i = 0; i < peers.length(); i++) { + final JSONObject peer = peers.getJSONObject(i); + assertThat(peer.getBoolean("httpAddressAmbiguous", false)) + .as("every peer resolves to this node's endpoint, so none of them is identified by it: %s", peer) + .isTrue(); + assertThat(peer.getString("httpAddress", "")) + .as("the resolved address is still reported - it is what the flag is about") + .isNotEmpty(); + } + } finally { + httpAddresses.putAll(declared); + } + } + + /** + * Control, on the same cluster: with the addresses it actually has, every peer owns its endpoint and no flag + * is emitted. Without it the assertion above would also pass against a build that flagged every cluster. + */ + @Test + @Timeout(120) + void aCorrectlyAddressedClusterCarriesNoAmbiguityFlag() throws Exception { + final int leader = findLeaderIndex(); + assertThat(leader).as("a Raft leader must be elected").isGreaterThanOrEqualTo(0); + + final JSONArray peers = getCluster(leader, "").getJSONArray("peers"); + assertThat(peers.length()).isEqualTo(getServerCount()); + for (int i = 0; i < peers.length(); i++) { + final JSONObject peer = peers.getJSONObject(i); + assertThat(peer.has("httpAddressAmbiguous")) + .as("a declared, distinct endpoint per peer carries no flag: %s", peer) + .isFalse(); + } + } + + /** + * The presence matrix asks each peer which databases it holds and attributes the answer to that peer, which + * makes it exactly the kind of unattended dial {@code PeerDialAddress} exists to guard. Unguarded, every peer + * was queried on the leader's own address and reported the leader's databases as its own. + */ + @Test + @Timeout(120) + void thePresenceMatrixReportsAPeerItCannotIdentifyAsUnreachable() throws Exception { + final int leader = findLeaderIndex(); + assertThat(leader).as("a Raft leader must be elected").isGreaterThanOrEqualTo(0); + + final RaftHAServer raft = getRaftPlugin(leader).getRaftHAServer(); + final Map httpAddresses = raft.getHttpAddresses(); + final Map declared = new HashMap<>(httpAddresses); + try { + httpAddresses.clear(); + + final JSONObject presence = getCluster(leader, "?presence=true").getJSONObject("databasePresence"); + final JSONArray unreachable = presence.getJSONArray("unreachable"); + + assertThat(unreachable.length()) + .as("the two peers the leader cannot identify were not queried: %s", presence) + .isEqualTo(getServerCount() - 1); + for (int i = 0; i < unreachable.length(); i++) + assertThat(unreachable.getString(i)).isNotEqualTo(raft.getLocalPeerId().toString()); + + // And nothing was attributed to them: a peer that was never asked holds no databases in the matrix. + final JSONArray databases = presence.getJSONArray("databases"); + for (int i = 0; i < databases.length(); i++) { + final JSONObject database = databases.getJSONObject(i); + final JSONArray present = database.getJSONArray("present"); + assertThat(present.length()) + .as("only the local node answered for '%s': %s", database.getString("name"), present) + .isEqualTo(1); + assertThat(present.getString(0)).isEqualTo(raft.getLocalPeerId().toString()); + } + } finally { + httpAddresses.putAll(declared); + } + } + + /** GETs the cluster endpoint of one server as the root operator, returning the parsed 200 body. */ + private JSONObject getCluster(final int serverIndex, final String query) throws Exception { + final HttpURLConnection conn = (HttpURLConnection) new URI( + "http://localhost:" + getServer(serverIndex).getHttpServer().getPort() + "/api/v1/cluster" + query) + .toURL().openConnection(); + try { + conn.setRequestMethod("GET"); + conn.setRequestProperty("Authorization", "Basic " + Base64.getEncoder() + .encodeToString(("root:" + DEFAULT_PASSWORD_FOR_TESTS).getBytes(StandardCharsets.UTF_8))); + + assertThat(conn.getResponseCode()).isEqualTo(200); + return new JSONObject(new String(conn.getInputStream().readAllBytes(), StandardCharsets.UTF_8)); + } finally { + conn.disconnect(); + } + } +} diff --git a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/Issue6267AmbiguousPeerAddressWarningTest.java b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/Issue6267AmbiguousPeerAddressWarningTest.java new file mode 100644 index 0000000000..633c0b58ea --- /dev/null +++ b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/Issue6267AmbiguousPeerAddressWarningTest.java @@ -0,0 +1,131 @@ +/* + * Copyright © 2021-present Arcade Data Ltd (info@arcadedata.com) + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * SPDX-FileCopyrightText: 2021-present Arcade Data Ltd (info@arcadedata.com) + * SPDX-License-Identifier: Apache-2.0 + */ +package com.arcadedb.server.ha.raft; + +import com.arcadedb.ContextConfiguration; +import com.arcadedb.GlobalConfiguration; +import com.arcadedb.server.ArcadeDBServer; + +import org.apache.ratis.protocol.RaftPeerId; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * A withheld peer-to-peer endpoint says so, once (issue #6267). + *

+ * {@code getUnambiguousPeerHttpAddress} / {@code getUnambiguousPeerHttpsAddress} refuse an address two peers + * resolve to by answering {@code null}, and each caller then decided for itself whether to say anything - so the + * misconfiguration was visible only where one happened to log it. Neither pre-existing warning covers it: the + * derive warnings fire whenever an address is derived at all, which a healthy homogeneous Kubernetes + * StatefulSet also does, and {@code warnAmbiguousRouting} says exactly this but about the client routing tables + * of issue #6183. + *

+ * Driven against a real {@link RaftHAServer} built from a server list with Ratis never started - the peer group + * and the declared addresses are both populated by the constructor, which is all the resolver reads. + * + * @author Luca Garulli (l.garulli@arcadedata.com) + */ +class Issue6267AmbiguousPeerAddressWarningTest { + + private static final String AMBIGUOUS_HTTP = "HA HTTP peer endpoints are ambiguous"; + private static final String AMBIGUOUS_HTTPS = "HA HTTPS peer endpoints are ambiguous"; + + private CapturingTestLogger log; + + @BeforeEach + void installLogger() { + log = CapturingTestLogger.install(); + } + + @AfterEach + void restoreLogger() { + log.uninstall(); + } + + /** + * Two peers deriving to one {@code host:port}: the address is withheld from both, and the operator is told + * which peers could not be told apart, on which address, and what to write instead. + */ + @Test + void aWithheldHttpAddressIsReportedOnceNamingThePeersThatShareIt() { + final RaftHAServer raft = newDetachedServer("localhost:2434:2480,localhost:2435:2490,localhost:2436:2490"); + + assertThat(raft.getUnambiguousPeerHttpAddress(RaftPeerId.valueOf("localhost_2435"))).isNull(); + assertThat(raft.getUnambiguousPeerHttpAddress(RaftPeerId.valueOf("localhost_2436"))).isNull(); + // The resync and verify paths ask on every attempt; a misconfiguration that has not changed since the last + // attempt must not be re-reported on each one. + assertThat(raft.getUnambiguousPeerHttpAddress(RaftPeerId.valueOf("localhost_2435"))).isNull(); + + assertThat(log.countFormattedContaining(AMBIGUOUS_HTTP, "localhost_2435", "localhost_2436", "localhost:2490", + GlobalConfiguration.HA_SERVER_LIST.getKey())) + .as("one warning, naming both peers, the address they share and the setting that fixes it") + .isEqualTo(1); + } + + /** The address every peer owns is handed out, and nothing is logged: a correct cluster stays quiet. */ + @Test + void anUnambiguousClusterIsNotWarnedAbout() { + final RaftHAServer raft = newDetachedServer("localhost:2434:2480,localhost:2435:2481,localhost:2436:2482"); + + assertThat(raft.getUnambiguousPeerHttpAddress(RaftPeerId.valueOf("localhost_2435"))).isEqualTo("localhost:2481"); + + assertThat(log.countContaining(AMBIGUOUS_HTTP)).isZero(); + assertThat(log.countContaining(AMBIGUOUS_HTTPS)).isZero(); + } + + /** + * The HTTPS endpoint has its own latch. A cluster that declares distinct {@code http} ports and shares an + * {@code https} one passes the HTTP check with every HTTPS endpoint collapsed, and that must still be + * reported - the HTTP verdict cannot stand in for it, and neither can the HTTP latch. + */ + @Test + void theHttpsWarningIsNotMutedByTheHttpOne() { + // host:raftPort:httpPort:priority:httpsPort - distinct HTTP ports, one shared HTTPS port. + final RaftHAServer raft = newDetachedServer( + "localhost:2434:2480:0:2490,localhost:2435:2481:0:2491,localhost:2436:2482:0:2491"); + + assertThat(raft.getUnambiguousPeerHttpAddress(RaftPeerId.valueOf("localhost_2435"))).isEqualTo("localhost:2481"); + assertThat(raft.getUnambiguousPeerHttpsAddress(RaftPeerId.valueOf("localhost_2435"))).isNull(); + + assertThat(log.countContaining(AMBIGUOUS_HTTP)) + .as("the HTTP endpoints are fine, so nothing is said about them") + .isZero(); + assertThat(log.countFormattedContaining(AMBIGUOUS_HTTPS, "localhost_2435", "localhost_2436", "localhost:2491")) + .isEqualTo(1); + } + + /** + * A {@link RaftHAServer} built from {@code serverList} with Ratis never started. The node names itself with the + * {@code prefix_N} convention, so it is the FIRST entry of the list. + */ + private static RaftHAServer newDetachedServer(final String serverList) { + final ContextConfiguration config = new ContextConfiguration(); + config.setValue(GlobalConfiguration.HA_SERVER_LIST, serverList); + + final ArcadeDBServer mockServer = mock(ArcadeDBServer.class); + when(mockServer.getServerName()).thenReturn("ArcadeDB_0"); + + return new RaftHAServer(mockServer, config); + } +} diff --git a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/RaftLeaderDown2NodesIT.java b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/RaftLeaderDown2NodesIT.java index 01ed26d95d..9edadd13c9 100644 --- a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/RaftLeaderDown2NodesIT.java +++ b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/RaftLeaderDown2NodesIT.java @@ -150,18 +150,7 @@ protected void checkDatabasesAreIdentical() { @Override protected int[] getServerToCheck() { - final int count = getServerCount(); - int alive = 0; - for (int i = 0; i < count; i++) { - if (getServer(i) != null && getServer(i).isStarted()) - alive++; - } - final int[] result = new int[alive]; - int idx = 0; - for (int i = 0; i < count; i++) { - if (getServer(i) != null && getServer(i).isStarted()) - result[idx++] = i; - } - return result; + // Only the servers still running: this test stops one on purpose. + return startedServers(); } } diff --git a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/RaftLeaderFailoverIT.java b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/RaftLeaderFailoverIT.java index 7ad7f619c4..9cc5250172 100644 --- a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/RaftLeaderFailoverIT.java +++ b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/RaftLeaderFailoverIT.java @@ -122,20 +122,8 @@ protected void checkDatabasesAreIdentical() { @Override protected int[] getServerToCheck() { - // Only check servers that are still running - final int count = getServerCount(); - int alive = 0; - for (int i = 0; i < count; i++) { - if (getServer(i) != null && getServer(i).isStarted()) - alive++; - } - final int[] result = new int[alive]; - int idx = 0; - for (int i = 0; i < count; i++) { - if (getServer(i) != null && getServer(i).isStarted()) - result[idx++] = i; - } - return result; + // Only the servers still running: this test stops one on purpose. + return startedServers(); } private int waitForNewLeader(final int excludeIndex) { diff --git a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/RaftQuorumLostIT.java b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/RaftQuorumLostIT.java index 2f113b7ef4..ec2acd49ed 100644 --- a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/RaftQuorumLostIT.java +++ b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/RaftQuorumLostIT.java @@ -134,18 +134,7 @@ protected void checkDatabasesAreIdentical() { @Override protected int[] getServerToCheck() { - final int count = getServerCount(); - int alive = 0; - for (int i = 0; i < count; i++) { - if (getServer(i) != null && getServer(i).isStarted()) - alive++; - } - final int[] result = new int[alive]; - int idx = 0; - for (int i = 0; i < count; i++) { - if (getServer(i) != null && getServer(i).isStarted()) - result[idx++] = i; - } - return result; + // Only the servers still running: this test stops one on purpose. + return startedServers(); } } diff --git a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/RaftTimeSeriesOversizedSealedIT.java b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/RaftTimeSeriesOversizedSealedIT.java index 7a8f2037e2..5fea845556 100644 --- a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/RaftTimeSeriesOversizedSealedIT.java +++ b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/RaftTimeSeriesOversizedSealedIT.java @@ -65,8 +65,6 @@ void oversizedSealedStoreSkipsCompactionAndKeepsDataInMutable() throws Exception final Object previousCap = GlobalConfiguration.HA_TS_MAX_SEALED_INLINE_SIZE.getValue(); GlobalConfiguration.HA_TS_MAX_SEALED_INLINE_SIZE.setValue(10L); try { - Awaitility.await().atMost(30, TimeUnit.SECONDS).pollInterval(500, TimeUnit.MILLISECONDS) - .until(() -> findLeaderIndex() >= 0); final int leaderIndex = findLeaderIndex(); assertThat(leaderIndex).as("a leader must be elected").isGreaterThanOrEqualTo(0); diff --git a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/RaftTimeSeriesReplication3NodesIT.java b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/RaftTimeSeriesReplication3NodesIT.java index f6d9e22c67..34b071af38 100644 --- a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/RaftTimeSeriesReplication3NodesIT.java +++ b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/RaftTimeSeriesReplication3NodesIT.java @@ -70,8 +70,6 @@ protected void checkDatabasesAreIdentical() { @Test void timeSeriesDataReplicatesToFollowers() throws Exception { - Awaitility.await().atMost(30, TimeUnit.SECONDS).pollInterval(500, TimeUnit.MILLISECONDS) - .until(() -> findLeaderIndex() >= 0); final int leaderIndex = findLeaderIndex(); assertThat(leaderIndex).as("a leader must be elected").isGreaterThanOrEqualTo(0); @@ -109,8 +107,6 @@ void timeSeriesDataReplicatesToFollowers() throws Exception { @Test @Tag("slow") void compactionReplicatesSealedBlocksAndClearsMutable() throws Exception { - Awaitility.await().atMost(30, TimeUnit.SECONDS).pollInterval(500, TimeUnit.MILLISECONDS) - .until(() -> findLeaderIndex() >= 0); final int leaderIndex = findLeaderIndex(); assertThat(leaderIndex).as("a leader must be elected").isGreaterThanOrEqualTo(0); @@ -162,8 +158,8 @@ void compactionReplicatesSealedBlocksAndClearsMutable() throws Exception { @Test @Tag("slow") void compactionSurvivesLeadershipChange() throws Exception { - awaitLeaderElected(); final int firstLeader = findLeaderIndex(); + assertThat(firstLeader).as("a leader must be elected").isGreaterThanOrEqualTo(0); executeCommand(firstLeader, "sql", "CREATE TIMESERIES TYPE weather TIMESTAMP ts TAGS (location STRING) FIELDS (temperature DOUBLE) SHARDS 1"); @@ -174,10 +170,8 @@ void compactionSurvivesLeadershipChange() throws Exception { timeSeriesEngine(findLeaderIndex()).compactAll(); awaitAllServersReportSamples(30); - // Step the current leader down and wait for a leader to be (re)elected. + // Step the current leader down; findLeaderIndex() waits for the re-election on its own. getRaftPlugin(firstLeader).getRaftHAServer().transferLeadership(10_000); - Awaitility.await().atMost(30, TimeUnit.SECONDS).pollInterval(500, TimeUnit.MILLISECONDS) - .until(() -> findLeaderIndex() >= 0); // Write + compact under the new leadership. insertSamples(30, 50); @@ -196,8 +190,8 @@ void compactionSurvivesLeadershipChange() throws Exception { @Test @Tag("slow") void laggingFollowerCatchesUpWithSealedDataAfterRestart() throws Exception { - awaitLeaderElected(); final int leader = findLeaderIndex(); + assertThat(leader).as("a leader must be elected").isGreaterThanOrEqualTo(0); executeCommand(leader, "sql", "CREATE TIMESERIES TYPE weather TIMESTAMP ts TAGS (location STRING) FIELDS (temperature DOUBLE) SHARDS 1"); @@ -229,12 +223,6 @@ void laggingFollowerCatchesUpWithSealedDataAfterRestart() throws Exception { }); } - private void awaitLeaderElected() { - Awaitility.await().atMost(30, TimeUnit.SECONDS).pollInterval(500, TimeUnit.MILLISECONDS) - .until(() -> findLeaderIndex() >= 0); - assertThat(findLeaderIndex()).as("a leader must be elected").isGreaterThanOrEqualTo(0); - } - private void insertSamples(final int fromInclusive, final int toExclusive) throws Exception { for (int i = fromInclusive; i < toExclusive; i++) executeCommand(findLeaderIndex(), "sql", diff --git a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/SuperNodeAppendHAConsistencyIT.java b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/SuperNodeAppendHAConsistencyIT.java index 9c1bbfb104..ffedd83a9a 100644 --- a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/SuperNodeAppendHAConsistencyIT.java +++ b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/SuperNodeAppendHAConsistencyIT.java @@ -25,7 +25,6 @@ import com.arcadedb.graph.Vertex; import com.arcadedb.server.ArcadeDBServer; -import org.awaitility.Awaitility; import org.junit.jupiter.api.Test; import java.util.concurrent.CountDownLatch; @@ -61,8 +60,6 @@ void concurrentSuperNodeAppendsReplicateWithoutLoss() throws InterruptedExceptio final int savedRetryDelay = GlobalConfiguration.TX_RETRY_DELAY.getValueAsInteger(); GlobalConfiguration.TX_RETRY_DELAY.setValue(1); try { - Awaitility.await().atMost(30, TimeUnit.SECONDS).pollInterval(500, TimeUnit.MILLISECONDS) - .until(() -> findLeaderIndex() >= 0); final int leaderIndex = findLeaderIndex(); assertThat(leaderIndex).as("a leader must be elected").isGreaterThanOrEqualTo(0); final ArcadeDBServer leaderServer = getServer(leaderIndex); diff --git a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/SuperNodeConcurrentAppendHABenchmark.java b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/SuperNodeConcurrentAppendHABenchmark.java index b82a45652f..b145fc7109 100644 --- a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/SuperNodeConcurrentAppendHABenchmark.java +++ b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/SuperNodeConcurrentAppendHABenchmark.java @@ -30,7 +30,6 @@ import com.arcadedb.log.LogManager; import com.arcadedb.server.ArcadeDBServer; -import org.awaitility.Awaitility; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; @@ -102,8 +101,6 @@ void concurrentAppendToSuperNodeUnderHA() throws InterruptedException { final int savedRetryDelay = GlobalConfiguration.TX_RETRY_DELAY.getValueAsInteger(); GlobalConfiguration.TX_RETRY_DELAY.setValue(1); try { - Awaitility.await().atMost(30, TimeUnit.SECONDS).pollInterval(500, TimeUnit.MILLISECONDS) - .until(() -> findLeaderIndex() >= 0); final int leaderIndex = findLeaderIndex(); assertThat(leaderIndex).as("a leader must be elected").isGreaterThanOrEqualTo(0); final ArcadeDBServer leaderServer = getServer(leaderIndex); diff --git a/server/src/main/java/com/arcadedb/server/http/handler/openapi/PluginApiSpec.java b/server/src/main/java/com/arcadedb/server/http/handler/openapi/PluginApiSpec.java index 8888890a1b..061e8fb8c8 100644 --- a/server/src/main/java/com/arcadedb/server/http/handler/openapi/PluginApiSpec.java +++ b/server/src/main/java/com/arcadedb/server/http/handler/openapi/PluginApiSpec.java @@ -371,6 +371,13 @@ private Schema createClusterStatusSchema() { final Schema peer = SpecBuilders.object("One peer's replication health"); peer.addProperty("id", SpecBuilders.string("Peer identifier")); peer.addProperty("address", SpecBuilders.string("Peer address")); + peer.addProperty("httpAddress", SpecBuilders.string( + "Peer HTTP endpoint as resolved by this node. Absent when it cannot be resolved.")); + peer.addProperty("httpAddressAmbiguous", SpecBuilders.bool( + "True when the HTTP endpoint above does not identify this peer alone: two or more peers resolve to it, " + + "which is what happens when 'http' ports are not declared in arcadedb.ha.serverList and the nodes " + + "differ by port rather than by host. Peer-to-peer operations (snapshot resync, cluster verify) " + + "refuse to dial such a peer. Absent when the address is unambiguous.")); peer.addProperty("role", SpecBuilders.string("LEADER or FOLLOWER")); peer.addProperty("matchIndex", SpecBuilders.integer( "Highest log entry known replicated. Absent for the leader's own entry and until a health sample exists.")); diff --git a/server/src/test/java/com/arcadedb/server/http/handler/openapi/PluginApiSpecTest.java b/server/src/test/java/com/arcadedb/server/http/handler/openapi/PluginApiSpecTest.java index d6cb32c356..52f7062bfb 100644 --- a/server/src/test/java/com/arcadedb/server/http/handler/openapi/PluginApiSpecTest.java +++ b/server/src/test/java/com/arcadedb/server/http/handler/openapi/PluginApiSpecTest.java @@ -120,13 +120,14 @@ void clusterStatusSchemaCarriesTheLeadershipAndPeerFields() { "leaderId", "leaderHttpAddress", "electionCount", "lastElectionTime", "uptime", "peers", "databases", "databasePresence", "alerts"); - // Pinned to the exact set (not .contains(...)): GetClusterHandler writes exactly these 12 fields + // Pinned to the exact set (not .contains(...)): GetClusterHandler writes exactly these 14 fields // per peer, no more, no fewer. final Schema peersProperty = schema.getProperties().get("peers"); final Schema peerItemSchema = peersProperty.getItems(); assertThat(peerItemSchema.getProperties().keySet()).containsExactlyInAnyOrder( - "id", "address", "role", "matchIndex", "nextIndex", "replicationLag", "lastContactMs", - "replicaStatus", "laggingForMs", "lagging", "replicationRttMs", "replicationRttP99Ms"); + "id", "address", "httpAddress", "httpAddressAmbiguous", "role", "matchIndex", "nextIndex", + "replicationLag", "lastContactMs", "replicaStatus", "laggingForMs", "lagging", "replicationRttMs", + "replicationRttP99Ms"); } @Test @@ -143,8 +144,12 @@ void clusterStatusPeerFieldsDocumentAbsenceForLeaderAndBeforeHealthSample() { final Schema peerItemSchema = peersProperty.getItems(); final Map peerProperties = peerItemSchema.getProperties(); + // httpAddress/httpAddressAmbiguous (issue #6267) are conditional too: the address is written only when it + // resolves, and the ambiguity flag only when it is true - a correctly declared cluster carries neither the + // flag nor a reason to look for it. for (final String conditionalField : List.of("matchIndex", "nextIndex", "replicationLag", - "lastContactMs", "replicaStatus", "laggingForMs", "lagging", "replicationRttMs", "replicationRttP99Ms")) { + "lastContactMs", "replicaStatus", "laggingForMs", "lagging", "replicationRttMs", "replicationRttP99Ms", + "httpAddress", "httpAddressAmbiguous")) { final Schema fieldSchema = peerProperties.get(conditionalField); assertThat(fieldSchema.getDescription()) .as("'%s' is written only for a non-leader peer with a health sample; its description must say so", diff --git a/studio/src/main/resources/static/js/studio-cluster.js b/studio/src/main/resources/static/js/studio-cluster.js index 67799d0e79..957d009cad 100644 --- a/studio/src/main/resources/static/js/studio-cluster.js +++ b/studio/src/main/resources/static/js/studio-cluster.js @@ -537,6 +537,19 @@ function renderNodeCards(data) { } } + // The HTTP endpoint this node resolved for the peer does not identify it alone - two or more peers resolve + // to it - so a snapshot resync and a cluster verify refuse to dial it. Showing the address with no caveat + // would let a misconfigured cluster look correctly addressed until one of those operations refuses + // (issue #6267). + var addressWarning = ""; + if (peer.httpAddressAmbiguous) { + addressWarning = '
' + + 'HTTP endpoint ' + + escapeHtml(peer.httpAddress || "") + + " does not identify this peer alone: declare its 'http' port in arcadedb.ha.serverList" + + '
'; + } + var card = '
' + '
' + '
' @@ -547,6 +560,7 @@ function renderNodeCards(data) { + '
' + '' + escapeHtml(peer.address || "") + '
' + + addressWarning + lagLine + '
'; From f4306f801af0a63799764b2c55bae853967556cd Mon Sep 17 00:00:00 2001 From: Luca Garulli Date: Sun, 16 Aug 2026 21:37:15 -0400 Subject: [PATCH 2/5] fix(#6267): the cluster status resolves peer endpoints once for the group, not once per peer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code review follow-up on the two actionable points. 1. GetClusterHandler asked getPeerHttpAddress and getUnambiguousPeerHttpAddress per peer, and answering the second means resolving every peer to see who else claims the address - so a /api/v1/cluster request resolved the group twice per peer, O(peers²) for a question that is O(peers). Cluster sizes make that harmless today, which is why it is worth removing before the shape is copied somewhere it is not. RaftHAServer.getPeerHttpEndpoints() answers it for the whole group in one pass, returning each peer's address with its verdict; the handler does a lookup. It shares claimsByAddress/identifiesOnePeer with the per-peer accessors, so it cannot answer differently, and a new test asserts exactly that for every peer of an ambiguous and of a correctly declared cluster. An ambiguity seen here trips the same one-time warning. 2. warnAmbiguousPeerAddress's javadoc now says what the latch does NOT do: it is per JVM and never rearms, so a membership change that makes a different pair of peers collide is not logged again. The line names the peers that tripped it first; httpAddressAmbiguous on GET /api/v1/cluster is recomputed per request and is the current answer. The third point (the latch convention itself) was raised as consistent-not-a-problem and is unchanged, as is the timeout measurement. Run: ha-raft unit lane (919), Issue6267AmbiguousAddressVisibilityIT. --- .../server/ha/raft/GetClusterHandler.java | 26 ++++---- .../arcadedb/server/ha/raft/RaftHAServer.java | 60 ++++++++++++++++++- ...ue6267AmbiguousPeerAddressWarningTest.java | 27 +++++++++ 3 files changed, 101 insertions(+), 12 deletions(-) diff --git a/ha-raft/src/main/java/com/arcadedb/server/ha/raft/GetClusterHandler.java b/ha-raft/src/main/java/com/arcadedb/server/ha/raft/GetClusterHandler.java index a8db14e8f0..f64e6128b4 100644 --- a/ha-raft/src/main/java/com/arcadedb/server/ha/raft/GetClusterHandler.java +++ b/ha-raft/src/main/java/com/arcadedb/server/ha/raft/GetClusterHandler.java @@ -117,23 +117,27 @@ public ExecutionResponse execute(final HttpServerExchange exchange, final Server // idle cluster. Previously no latency figure was exposed in this JSON at all. final Map replicationLatencies = raftHAServer.getReplicationLatencies(); + // Each peer's HTTP endpoint, plus whether that endpoint identifies that peer and no other. With no 'http' + // port declared in arcadedb.ha.serverList a peer's endpoint is derived as its Raft host plus THIS node's + // port, so on a cluster whose nodes differ by port every peer collapses onto one address. Reporting the + // address alone would show an operator a plausible endpoint per peer with nothing to say that it names + // none of them, and they would find out only when a resync or a verify refuses to dial (issue #6267). + // Resolved for the whole group in one pass: the question is about the group, since an address identifies a + // peer only if no other peer resolves to it, so asking per peer would resolve the group once per peer. + final Map httpEndpoints = raftHAServer.getPeerHttpEndpoints(); + final JSONArray peers = new JSONArray(); for (final RaftPeer peer : raftHAServer.getRaftGroup().getPeers()) { final JSONObject peerJson = new JSONObject(); final String peerId = peer.getId().toString(); peerJson.put("id", peerId); peerJson.put("address", peer.getAddress()); - // The peer's HTTP endpoint as resolved, plus whether that endpoint identifies this peer and no other. - // With no 'http' port declared in arcadedb.ha.serverList a peer's endpoint is derived as its Raft host - // plus THIS node's port, so on a cluster whose nodes differ by port every peer collapses onto one - // address. Reporting the address alone would show an operator a plausible endpoint per peer with nothing - // to say that it names none of them, and they would find out only when a resync or a verify refuses to - // dial. The flag is present only when set, so a correctly declared cluster carries no extra field - // (issue #6267). - final String httpAddress = raftHAServer.getPeerHttpAddress(peer.getId()); - if (httpAddress != null) { - peerJson.put("httpAddress", httpAddress); - if (raftHAServer.getUnambiguousPeerHttpAddress(peer.getId()) == null) + // Both fields are written only when they have something to say: a peer whose endpoint cannot be resolved + // carries neither, and a correctly declared cluster carries no flag. + final RaftHAServer.PeerHttpEndpoint httpEndpoint = httpEndpoints.get(peer.getId()); + if (httpEndpoint != null) { + peerJson.put("httpAddress", httpEndpoint.address()); + if (httpEndpoint.ambiguous()) peerJson.put("httpAddressAmbiguous", true); } diff --git a/ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftHAServer.java b/ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftHAServer.java index 9437624cc9..2762f9d2be 100644 --- a/ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftHAServer.java +++ b/ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftHAServer.java @@ -1927,6 +1927,59 @@ public String getUnambiguousPeerHttpsAddress(final RaftPeerId peerId) { return unambiguousPeerAddress(peerId, true); } + /** A peer's HTTP endpoint as this node resolves it, and whether that endpoint identifies that peer alone. */ + public record PeerHttpEndpoint(String address, boolean ambiguous) { + } + + /** + * Every group peer's HTTP endpoint, each carrying the verdict {@link #getUnambiguousPeerHttpAddress} would + * give it, in one pass over the group rather than one pass per peer. + *

+ * The per-peer accessors answer about one peer, and answering that question means resolving all of + * them to see who else claims the address - so a caller that wants the answer for the whole group, as the + * cluster-status endpoint does, would resolve the group once per peer and once more for the plain address: + * O(peers²) for a question that is O(peers). Cluster sizes make that harmless today, which is exactly why it + * is worth removing before the shape is copied somewhere it is not. + *

+ * A peer whose address cannot be resolved at all is absent from the map rather than present with a + * {@code null} address: it is the same "nothing to report" an unknown peer produces from the accessors. + * The claim counting and the declared-beats-derived tie-break are the shared {@code claimsByAddress} / + * {@code identifiesOnePeer} pair, so this cannot answer differently from the accessors, and an ambiguity + * seen here trips the same one-time warning. + */ + public Map getPeerHttpEndpoints() { + final Collection peers = raftGroup.getPeers(); + final String[] addresses = new String[peers.size()]; + final boolean[] fromConfig = new boolean[addresses.length]; + final RaftPeerId[] owners = new RaftPeerId[addresses.length]; + int resolved = 0; + + for (final RaftPeer peer : peers) { + final String address = resolveHttpAddress(peer); + if (address == null) + continue; + addresses[resolved] = address; + fromConfig[resolved] = httpAddresses.containsKey(peer.getId()); + owners[resolved] = peer.getId(); + ++resolved; + } + + final Map claims = claimsByAddress(addresses, fromConfig, resolved); + final Map endpoints = new LinkedHashMap<>(resolved * 2); + String ambiguous = null; + for (int i = 0; i < resolved; i++) { + final boolean shared = !identifiesOnePeer(claims.get(addresses[i]), fromConfig[i]); + if (shared) + ambiguous = addresses[i]; + endpoints.put(owners[i], new PeerHttpEndpoint(addresses[i], shared)); + } + + if (ambiguous != null) + warnAmbiguousPeerAddress(false, ambiguous, addresses, owners, resolved); + + return endpoints; + } + /** * Shared body of the two accessors above: an address is handed out only when the peer asked about is the only * one that resolves to it, or the only one that declared it. Parameterized by protocol rather than duplicated, @@ -1983,7 +2036,12 @@ private String unambiguousPeerAddress(final RaftPeerId peerId, final boolean htt * routing tables of issue #6183, not about the peer-to-peer endpoints a resync and a cluster verify dial. *

* Once per protocol rather than once per attempt: the resync and verify paths ask on every attempt, and a - * misconfiguration that does not change between attempts should not be re-reported on each one. + * misconfiguration that does not change between attempts should not be re-reported on each one. The latch is + * per JVM and never rearms, the same convention {@link #deriveHttpAddressWithWarning} and + * {@link #warnAmbiguousRouting} follow, so a membership change that makes a different pair of peers + * collide is not logged again - the line names the peers that tripped it first, and a reader chasing a + * cluster they know is misconfigured should read {@code httpAddressAmbiguous} from {@code GET + * /api/v1/cluster}, which is recomputed per request and always current, rather than the log. */ private void warnAmbiguousPeerAddress(final boolean https, final String address, final String[] addresses, final RaftPeerId[] owners, final int count) { diff --git a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/Issue6267AmbiguousPeerAddressWarningTest.java b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/Issue6267AmbiguousPeerAddressWarningTest.java index 633c0b58ea..8122848305 100644 --- a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/Issue6267AmbiguousPeerAddressWarningTest.java +++ b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/Issue6267AmbiguousPeerAddressWarningTest.java @@ -27,6 +27,8 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import java.util.Map; + import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -115,6 +117,31 @@ void theHttpsWarningIsNotMutedByTheHttpOne() { .isEqualTo(1); } + /** + * The group-wide resolver the cluster-status endpoint uses answers exactly what the per-peer accessors do, + * for every peer, on both an ambiguous and a correctly declared cluster. It exists only to avoid resolving + * the group once per peer, so the one thing it must never do is answer differently. + */ + @Test + void theGroupWideEndpointsAgreeWithThePerPeerAccessors() { + for (final String serverList : new String[] { + "localhost:2434:2480,localhost:2435:2490,localhost:2436:2490", // two peers share an address + "localhost:2434:2480,localhost:2435:2481,localhost:2436:2482" }) { + final RaftHAServer raft = newDetachedServer(serverList); + + final Map endpoints = raft.getPeerHttpEndpoints(); + + assertThat(endpoints).as("every peer of '%s' resolves to something", serverList).hasSize(3); + for (final Map.Entry entry : endpoints.entrySet()) { + final RaftPeerId peerId = entry.getKey(); + assertThat(entry.getValue().address()).isEqualTo(raft.getPeerHttpAddress(peerId)); + assertThat(entry.getValue().ambiguous()) + .as("peer %s of '%s'", peerId, serverList) + .isEqualTo(raft.getUnambiguousPeerHttpAddress(peerId) == null); + } + } + } + /** * A {@link RaftHAServer} built from {@code serverList} with Ratis never started. The node names itself with the * {@code prefix_N} convention, so it is the FIRST entry of the list. From a9f3e7236716a30d81d517777b9ec5f150b290a6 Mon Sep 17 00:00:00 2001 From: Luca Garulli Date: Sun, 16 Aug 2026 21:52:41 -0400 Subject: [PATCH 3/5] fix(#6267): the ambiguity warning names every collision it found, in a stable order Round 2 of the code review. Two points taken, one declined. 1. The one-time warning reported a single colliding address - whichever the pass saw last - so a cluster with two independent colliding pairs got one of them named and the other silently absorbed. Since the latch never rearms, that pair would then be withheld with the log permanently quiet, and an operator who declared the two ports the line named would still have a broken cluster. It now names every collision the pass found, grouped by address, and takes the latch only once there is something to say. The groups and the peers within them are sorted, so the same misconfiguration produces the same line whichever node logs it - Ratis hands the group over in its own order, which is what made the first version of the new test order-dependent. 2. Dropped the JSONArray/JSONObject/ArcadeDBServer imports that exportClusterStatus() left behind in RaftClusterStatusExporter. Declined: routing buildPresenceMatrix's dial through the precomputed getPeerHttpEndpoints map instead of PeerDialAddress.resolve(). It would save one O(peers) resolution per peer on a path that then makes a network round trip per peer with a 5 s timeout - and it would do it by hand-assembling the guard (self-dial check, the HTTPS endpoint, the refusal text) that #6202/#6221 consolidated into one helper precisely because call sites had drifted apart. That trade is the wrong way round; the endpoint map is for reporting, PeerDialAddress is for dialling. Tests: oneWarningNamesEveryCollisionItFound covers two independent colliding pairs in one line. Run: ha-raft unit lane (920), Issue6267AmbiguousAddressVisibilityIT, Issue6221VerifyFanOutGuardIT. --- .../ha/raft/RaftClusterStatusExporter.java | 3 - .../arcadedb/server/ha/raft/RaftHAServer.java | 67 ++++++++++++------- ...ue6267AmbiguousPeerAddressWarningTest.java | 21 ++++++ 3 files changed, 62 insertions(+), 29 deletions(-) diff --git a/ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftClusterStatusExporter.java b/ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftClusterStatusExporter.java index 14c49c8a74..9e10fda949 100644 --- a/ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftClusterStatusExporter.java +++ b/ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftClusterStatusExporter.java @@ -19,9 +19,6 @@ package com.arcadedb.server.ha.raft; import com.arcadedb.log.LogManager; -import com.arcadedb.server.ArcadeDBServer; -import com.arcadedb.serializer.json.JSONArray; -import com.arcadedb.serializer.json.JSONObject; import org.apache.ratis.protocol.RaftPeer; import org.apache.ratis.protocol.RaftPeerId; diff --git a/ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftHAServer.java b/ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftHAServer.java index 2762f9d2be..16a68e5f1d 100644 --- a/ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftHAServer.java +++ b/ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftHAServer.java @@ -78,6 +78,7 @@ import java.util.Locale; import java.util.Map; import java.util.SortedMap; +import java.util.TreeMap; import java.util.UUID; import java.util.concurrent.ArrayBlockingQueue; import java.util.concurrent.ConcurrentHashMap; @@ -1966,16 +1967,13 @@ public Map getPeerHttpEndpoints() { final Map claims = claimsByAddress(addresses, fromConfig, resolved); final Map endpoints = new LinkedHashMap<>(resolved * 2); - String ambiguous = null; - for (int i = 0; i < resolved; i++) { - final boolean shared = !identifiesOnePeer(claims.get(addresses[i]), fromConfig[i]); - if (shared) - ambiguous = addresses[i]; - endpoints.put(owners[i], new PeerHttpEndpoint(addresses[i], shared)); - } + for (int i = 0; i < resolved; i++) + endpoints.put(owners[i], new PeerHttpEndpoint(addresses[i], + !identifiesOnePeer(claims.get(addresses[i]), fromConfig[i]))); - if (ambiguous != null) - warnAmbiguousPeerAddress(false, ambiguous, addresses, owners, resolved); + // No-op unless something is actually ambiguous, and then it names every collision this pass found - which + // is more than a per-peer caller can see, since that one asks about one address at a time. + warnAmbiguousPeerAddress(false, addresses, fromConfig, owners, resolved, claims); return endpoints; } @@ -2019,7 +2017,7 @@ private String unambiguousPeerAddress(final RaftPeerId peerId, final boolean htt if (identifiesOnePeer(claims.get(address), fromConfig[0])) return address; - warnAmbiguousPeerAddress(https, address, addresses, owners, resolved); + warnAmbiguousPeerAddress(https, addresses, fromConfig, owners, resolved, claims); return null; } @@ -2042,28 +2040,45 @@ private String unambiguousPeerAddress(final RaftPeerId peerId, final boolean htt * collide is not logged again - the line names the peers that tripped it first, and a reader chasing a * cluster they know is misconfigured should read {@code httpAddressAmbiguous} from {@code GET * /api/v1/cluster}, which is recomputed per request and always current, rather than the log. - */ - private void warnAmbiguousPeerAddress(final boolean https, final String address, final String[] addresses, - final RaftPeerId[] owners, final int count) { - if (!(https ? httpsAmbiguityWarned : httpAmbiguityWarned).compareAndSet(false, true)) + *

+ * Since the one line is all an operator gets, it names every collision this pass found, not just the + * one the caller asked about: a cluster can have two independent colliding pairs, and reporting one of them + * would send the operator to declare two ports and leave the other pair withheld with the log now silent. The + * latch is taken only once there is something to say, so a pass that finds nothing does not spend it. + */ + private void warnAmbiguousPeerAddress(final boolean https, final String[] addresses, final boolean[] fromConfig, + final RaftPeerId[] owners, final int count, final Map claims) { + // address -> the peers whose endpoint it fails to identify. A peer that DECLARED an address others merely + // derive to keeps it (declared beats derived), so it is not among them and must not be named as withheld. + // Sorted, by address and then by peer, because the group arrives in whatever order Ratis holds it: the + // operator gets the same line for the same misconfiguration whichever node logs it. + final Map> withheld = new TreeMap<>(); + for (int i = 0; i < count; i++) { + if (identifiesOnePeer(claims.get(addresses[i]), fromConfig[i])) + continue; + withheld.computeIfAbsent(addresses[i], a -> new ArrayList<>()).add(owners[i].toString()); + } + + if (withheld.isEmpty() || !(https ? httpsAmbiguityWarned : httpAmbiguityWarned).compareAndSet(false, true)) return; - final StringBuilder shared = new StringBuilder(); - for (int i = 0; i < count; i++) - if (address.equals(addresses[i])) { - if (!shared.isEmpty()) - shared.append(", "); - shared.append(owners[i]); - } + final StringBuilder collisions = new StringBuilder(); + for (final Map.Entry> collision : withheld.entrySet()) { + Collections.sort(collision.getValue()); + if (!collisions.isEmpty()) + collisions.append("; "); + collisions.append(String.join(", ", collision.getValue())).append(" -> ").append(collision.getKey()); + } final String protocol = https ? "HTTPS" : "HTTP"; final String field = https ? "https" : "http"; LogManager.instance().log(this, Level.WARNING, - "HA %s peer endpoints are ambiguous: peers %s all resolve to %s, which two listening sockets cannot both own. " - + "Their %s endpoint is withheld rather than guessed, so a snapshot resync and a cluster verify refuse to " - + "dial them and report them unverified instead of answering for the wrong node. Declare each node's %s port " - + "explicitly with the 'host:{raft:..,%s:..}' object syntax in %s.", - protocol, shared, address, protocol, protocol, field, GlobalConfiguration.HA_SERVER_LIST.getKey()); + "HA %s peer endpoints are ambiguous: %s. Two listening sockets cannot both own one address, so it identifies " + + "at most one of the peers that resolve to it and nothing can say which. Their %s endpoint is withheld " + + "rather than guessed, so a snapshot resync and a cluster verify refuse to dial them and report them " + + "unverified instead of answering for the wrong node. Declare each node's %s port explicitly with the " + + "'host:{raft:..,%s:..}' object syntax in %s.", + protocol, collisions, protocol, protocol, field, GlobalConfiguration.HA_SERVER_LIST.getKey()); } /** Tells the operator, once per protocol, that peers shared an address and what to write to fix it. */ diff --git a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/Issue6267AmbiguousPeerAddressWarningTest.java b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/Issue6267AmbiguousPeerAddressWarningTest.java index 8122848305..449b4db040 100644 --- a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/Issue6267AmbiguousPeerAddressWarningTest.java +++ b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/Issue6267AmbiguousPeerAddressWarningTest.java @@ -85,6 +85,27 @@ void aWithheldHttpAddressIsReportedOnceNamingThePeersThatShareIt() { .isEqualTo(1); } + /** + * A cluster with two independent collisions gets both named in the one line it is allowed. Reporting + * only the collision the caller happened to ask about would send an operator to declare two ports and leave + * the other pair withheld with the log now permanently silent, since the latch does not rearm. + */ + @Test + void oneWarningNamesEveryCollisionItFound() { + final RaftHAServer raft = newDetachedServer( + "localhost:2434:2480,localhost:2435:2490,localhost:2436:2490,localhost:2437:2491,localhost:2438:2491"); + + final Map endpoints = raft.getPeerHttpEndpoints(); + + assertThat(endpoints.values().stream().filter(RaftHAServer.PeerHttpEndpoint::ambiguous)) + .as("both pairs are withheld; this node's own address is its alone") + .hasSize(4); + assertThat(log.countFormattedContaining(AMBIGUOUS_HTTP, + "localhost_2435, localhost_2436 -> localhost:2490", "localhost_2437, localhost_2438 -> localhost:2491")) + .as("one line, both collisions") + .isEqualTo(1); + } + /** The address every peer owns is handed out, and nothing is logged: a correct cluster stays quiet. */ @Test void anUnambiguousClusterIsNotWarnedAbout() { From 1a8c61722dd7a1cea159df3079c9bbb1c26217bb Mon Sep 17 00:00:00 2001 From: Luca Garulli Date: Sun, 16 Aug 2026 21:58:49 -0400 Subject: [PATCH 4/5] fix(#6267): drop the TimeUnit import the removed await left behind in SuperNodeConcurrentAppendHABenchmark Round 3 of the code review, its only point: the class's sole use of TimeUnit was the Awaitility.await(...).atMost(30, TimeUnit.SECONDS) wrapper this PR removed. --- .../server/ha/raft/SuperNodeConcurrentAppendHABenchmark.java | 1 - 1 file changed, 1 deletion(-) diff --git a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/SuperNodeConcurrentAppendHABenchmark.java b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/SuperNodeConcurrentAppendHABenchmark.java index b145fc7109..f9354dac70 100644 --- a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/SuperNodeConcurrentAppendHABenchmark.java +++ b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/SuperNodeConcurrentAppendHABenchmark.java @@ -34,7 +34,6 @@ import org.junit.jupiter.api.Test; import java.util.concurrent.CountDownLatch; -import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; import java.util.logging.Level; From dae4a9a3b0b62c4956730b3a253d79245d7d4c2e Mon Sep 17 00:00:00 2001 From: Luca Garulli Date: Sun, 16 Aug 2026 22:09:32 -0400 Subject: [PATCH 5/5] fix(#6267): say what the ambiguity latch is scoped to, and why the eviction test is not racy Round 4 of the code review; it found no functional issue, both notes are wording. - The javadoc called the warning latch "per JVM". It is a field on this RaftHAServer, like every other warn-once latch in the class - true per process where there is one server, and not true in a test that builds several, which is exactly what the new unit tests rely on. Says that now. - The review flagged that removedPeerIsNotHeldToTheClusterConsistencyCheck would be flaky if removePeer() did not block until the reconfiguration commits. It does: RaftClusterManager.removePeer drives Ratis's blocking admin().setConfiguration(...) and returns only on a successful reply. Written down on evict(), where the next reader will ask the same question - the assertions that follow it read committed membership, and a write issued after it cannot still reach the evicted peer. Run: DynamicMembershipTest (7), Issue6267AmbiguousPeerAddressWarningTest (5). --- .../com/arcadedb/server/ha/raft/RaftHAServer.java | 12 +++++++----- .../server/ha/raft/DynamicMembershipTest.java | 6 ++++++ 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftHAServer.java b/ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftHAServer.java index 16a68e5f1d..9bc3898901 100644 --- a/ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftHAServer.java +++ b/ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftHAServer.java @@ -154,9 +154,10 @@ public class RaftHAServer implements HealthMonitor.HealthTarget { private final AtomicBoolean httpFallbackWarned = new AtomicBoolean(false); // Logged at most once: notes that peer HTTPS endpoints are derived from this node's local HTTPS port. private final AtomicBoolean httpsFallbackWarned = new AtomicBoolean(false); - // Logged at most once per protocol: warns that a peer-to-peer endpoint was WITHHELD because two peers - // resolved to it. Distinct from the two latches above, which fire whenever an address is derived at all - - // which a healthy homogeneous StatefulSet also does (issue #6267). + // Logged at most once per protocol, per node (these are this server's latches, like every other one here - + // a test that builds several RaftHAServer instances gets a fresh pair each time): warns that a peer-to-peer + // endpoint was WITHHELD because two peers resolved to it. Distinct from the two latches above, which fire + // whenever an address is derived at all - which a healthy homogeneous StatefulSet also does (issue #6267). private final AtomicBoolean httpAmbiguityWarned = new AtomicBoolean(false); private final AtomicBoolean httpsAmbiguityWarned = new AtomicBoolean(false); // Client-reachable Bolt endpoints (optional object-form 'bolt' field in HA_SERVER_LIST). Advertised @@ -2034,8 +2035,9 @@ private String unambiguousPeerAddress(final RaftPeerId peerId, final boolean htt * routing tables of issue #6183, not about the peer-to-peer endpoints a resync and a cluster verify dial. *

* Once per protocol rather than once per attempt: the resync and verify paths ask on every attempt, and a - * misconfiguration that does not change between attempts should not be re-reported on each one. The latch is - * per JVM and never rearms, the same convention {@link #deriveHttpAddressWithWarning} and + * misconfiguration that does not change between attempts should not be re-reported on each one. The latch + * lives on this node - one per {@code RaftHAServer}, which in a server process means once for its lifetime - + * and never rearms, the same convention {@link #deriveHttpAddressWithWarning} and * {@link #warnAmbiguousRouting} follow, so a membership change that makes a different pair of peers * collide is not logged again - the line names the peers that tripped it first, and a reader chasing a * cluster they know is misconfigured should read {@code httpAddressAmbiguous} from {@code GET diff --git a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/DynamicMembershipTest.java b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/DynamicMembershipTest.java index 67db261edc..0a47d520bf 100644 --- a/ha-raft/src/test/java/com/arcadedb/server/ha/raft/DynamicMembershipTest.java +++ b/ha-raft/src/test/java/com/arcadedb/server/ha/raft/DynamicMembershipTest.java @@ -151,6 +151,12 @@ void removePeerRefusedWhenItWouldBreakQuorum() { * Removes a peer from the group and records that it is no longer a replica. Recorded only once the removal * has returned: a refused removal leaves the peer a member, and excluding it from the teardown check would * hide exactly the divergence the refusal exists to prevent. + *

+ * Returning is also enough to make what follows deterministic rather than a race: {@code removePeer} drives + * Ratis's blocking {@code admin().setConfiguration(...)} and returns only on a successful reply, so the new + * configuration is committed by then. A write issued after this call cannot still reach the evicted peer, + * and the {@code getLivePeers()} assertions the methods above make immediately afterwards read committed + * membership rather than a request in flight. */ private void evict(final RaftHAServer raftServer, final int serverIndex, final boolean force) { if (force)