fix(#5002): HA-aware Bolt ROUTE response with multi-server routing table - #5085
Conversation
…ct form Add an optional object-form 'bolt:' field to HA_SERVER_LIST, parsed into a boltAddresses map, and resolve each peer's client-reachable Bolt address in RaftHAServer (declared value, else derive peerHost:localBoltPort with a one-time WARNING). Expose getLeaderBoltAddress()/getReplicaBoltAddresses().
…ship handleRoute now advertises the leader as WRITE+ROUTE and each follower as READ+ROUTE from HAServerPlugin, falling back to the single-node self table when HA is absent or leaderless. Adds a single-node ROUTE regression test.
Verifies the routing table classifies the true leader as writer and followers as readers, that a neo4j:// driver routes reads and writes end to end, and that writer classification tracks a leader change.
|
Tick the box to add this pull request to the merge queue (same as
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 12 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements HA-aware Bolt ROUTE responses to reflect real cluster topology, allowing neo4j:// drivers to discover and route against ArcadeDB clusters. It introduces an optional bolt port field in the HA_SERVER_LIST configuration, resolves these addresses in RaftHAServer, and dynamically builds the routing table in BoltNetworkExecutor.handleRoute. The feedback highlights two key improvements: adding a null check in RaftHAServer.deriveBoltAddress to prevent a potential NullPointerException when a peer's address is unresolved, and catching exceptions within the retry loop of awaitRoutingTable in the new integration test to avoid flakiness during cluster re-elections.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| static String deriveBoltAddress(final String raftAddress, final int boltPort) { | ||
| if (boltPort <= 0) | ||
| return null; |
There was a problem hiding this comment.
The deriveBoltAddress method calls extractHost(raftAddress) directly without checking if raftAddress is null. If peerRaftAddress(peerId) returns null (e.g., if the peer is not found or not fully initialized), this will lead to a NullPointerException. Adding a null check for raftAddress at the beginning of the method ensures safety, matching the pattern used in deriveHttpAddressWithWarning.
| static String deriveBoltAddress(final String raftAddress, final int boltPort) { | |
| if (boltPort <= 0) | |
| return null; | |
| static String deriveBoltAddress(final String raftAddress, final int boltPort) { | |
| if (raftAddress == null || boltPort <= 0) | |
| return null; |
There was a problem hiding this comment.
Added the explicit raftAddress == null guard in 5614c67. Note there was no actual NPE: extractHost(null) already returns null, so deriveBoltAddress returned null safely. The explicit early return makes the contract clearer and matches the guard style, so it is worth keeping regardless.
| private Map<String, Object> awaitRoutingTable(final int boltPort, final String expectedWriter) throws Exception { | ||
| Map<String, Object> rt = null; | ||
| for (int attempt = 0; attempt < 40; attempt++) { | ||
| rt = fetchRoutingTable(boltPort); | ||
| if (List.of(expectedWriter).equals(addressesForRole(rt, "WRITE"))) | ||
| return rt; | ||
| Thread.sleep(250); | ||
| } | ||
| return rt; | ||
| } |
There was a problem hiding this comment.
In integration tests involving cluster re-elections and socket connections, the target port might temporarily be unreachable or throw connection exceptions during startup or failover. Catching and ignoring exceptions inside the retry loop of awaitRoutingTable prevents premature test failure and makes the test suite more robust against CI/CD environment flakiness.
private Map<String, Object> awaitRoutingTable(final int boltPort, final String expectedWriter) throws Exception {
Map<String, Object> rt = null;
for (int attempt = 0; attempt < 40; attempt++) {
try {
rt = fetchRoutingTable(boltPort);
if (List.of(expectedWriter).equals(addressesForRole(rt, "WRITE")))
return rt;
} catch (final Exception e) {
// ignore and retry
}
Thread.sleep(250);
}
return rt;
}There was a problem hiding this comment.
Applied in 5614c67: awaitRoutingTable now retries through connection exceptions thrown while contacting a node mid-failover. This matches the EOF/connection-reset observed when a node is hit during re-election.
|
Code Review: HA-aware Bolt ROUTE response (PR 5085) Nice piece of work overall. The change is well-scoped, cleanly mirrors the established Potential issues
Minor / style
Things done well
Nothing here is blocking; items 1 and 3 are the two I would most want a second look at before merge. |
…lback - Collapse getLeaderBoltAddress/getReplicaBoltAddresses into one getBoltRoutingTable() snapshot computed from a single getLeaderId() read, eliminating the writer/reader TOCTOU window and the CSV round-trip (Claude #1). - handleRoute: when HA is active but the leader is unknown, advertise this node as READ+ROUTE only (never WRITE), so a driver never writes to a follower during a leader-unknown window; true single-node still advertises all roles (Claude #3). - deriveBoltAddress: explicit null guard for raftAddress (Gemini). - Bolt5002RoutingTableIT.awaitRoutingTable: retry through transient connection errors during failover (Gemini).
|
Addressed the review in 5614c67. #1 TOCTOU between the two HA calls - Fixed. Replaced #3 fallback advertises a follower as WRITE - Fixed. #2 "live" vs configured membership - Correct, it is configured membership. Kept parity with Minor - CSV round-trip - Resolved as a side effect of #1: the new Minor - hardcoded ports in the IT - Acknowledged; kept consistent with the sibling HA ITs ( All tests green (resolver unit, single-node ROUTE regression, 3-node |
Code Review: HA-aware Bolt ROUTE response (#5085)Reviewed the full diff. This is a well-structured, well-tested change. The design is clean: it reuses the established One thing worth calling out as an improvement over the committed plan: the final code uses a single Findings1. (Security, low/medium) This gap is pre-existing, but this PR materially amplifies its impact: previously ROUTE leaked only the address the client already connected to; now it enumerates every peer's Bolt endpoint (derived from internal Raft hostnames), which is useful recon for an unauthenticated caller. Consider gating ROUTE behind an authenticated state (e.g. 2. (Minor) 3. (Behavioral note, not a bug) Stale-leader advertisement on followers. 4. (Housekeeping) The committed plan doc is already stale. Things done well
Overall this looks solid and mergeable; item 1 is the only one I'd want addressed or explicitly confirmed before shipping. |
- handleRoute now requires an authenticated (READY) session: ROUTE enumerates every peer's Bolt endpoint, so an unauthenticated caller (Bolt 5.1+ deferred-auth window, pre-LOGON) is refused. Adds routeBeforeLogonIsRejected regression (Claude #1). - BoltRoutingTable readers returned as List.copyOf to honor the immutability contract (Claude #2). - Reconcile design spec + plan with the final getBoltRoutingTable design, auth gate, and three-way fallback (Claude #4). Claude #3 (brief stale-leader window on a follower right after failover) is an accepted, self-healing behavior (driver recovers via routing TTL); no code change.
|
Addressed in 2214dfc. #1 (security) ROUTE reachable before authentication - Fixed. #2 (minor) #3 (behavioral note) stale-leader window on a follower - Acknowledged as accepted behavior. Right after a failover #4 (housekeeping) stale plan doc - Reconciled. The repo convention commits All tests green: parsing unit tests, single-node ROUTE regression, the new auth-gate test, and the 3-node |
|
Code Review: HA-aware Bolt ROUTE response Reviewed the net diff against Strengths
Suggestions / minor issues
Things I checked that are fine
No blocking concerns from my side - the items above are polish. Generated with Claude Code |
…g table - Move readRoutingTable/addressesForRole into BoltRouteTestSupport, shared by Bolt5002RoutingTableIT and BoltProtocolIT, so the SUCCESS/rt wire shape is decoded in one place (Claude review, DRY). - awaitRoutingTable now asserts the routing table is non-null after its retry budget, so a genuine failure reports a legible message instead of an NPE (Claude review).
|
Addressed the polish items in 9e87086. #1 #2 duplicated test helpers - Fixed. Extracted #3 ROUTE now requires READY is a behavior change - Acknowledged. It is spec-compliant (ROUTE is a READY-state message) and only affects non-conformant clients that send ROUTE before authenticating; official drivers always authenticate first. Worth a release-note line, which I'll leave to the maintainer's changelog rather than committing here. #4 dead/partitioned followers advertised until they leave the group - Confirmed acceptable. All route/auth/IT tests green. |
|
Code review: HA-aware Bolt ROUTE (PR 5085) Reviewed the full diff (source, tests, conformance, docs). This is a clean, well-scoped change that correctly closes the single-node-only ROUTE gap. The Strengths
Discussion points (non-blocking)
Nits
Nice work - the classification-tracks-leader-change IT in particular gives real confidence this behaves under failover. Automated review by Claude. |
The interface Javadoc listed null for HA-inactive and no-known-leader, but RaftHAServer also returns null when the leader has no resolvable Bolt address. Callers branch on null, so the contract now states all three cases (Claude review).
|
Thanks for the thorough passes. Addressed in 4ff8da2. #2 interface vs impl null contract - Fixed. #1 dead readers in a partially-down cluster - Acknowledged. This is documented on the interface method and in the #3 ROUTE rejected in TX_READY/STREAMING - Confirmed intended. Spec-compliant (ROUTE is a READY-state message) and real drivers use a dedicated routing connection; the tightening only affects a hand-rolled client multiplexing ROUTE onto a busy connection. #4 docs footprint - Confirmed intended. Per repo convention Nits - No outstanding items on my side. |
…losed CONN-004 was the last #4890-tracked expected-fail scenario. Flipping it to passing left the connection area with no tracked gap, failing the spec structural-integrity check. All #4890 gaps are now closed, so the required-gap set is empty (mirrors how errors/type-roundtrip/protocol were handled).
Review: HA-aware Bolt ROUTE response (#5002)Solid, well-scoped PR. The design is clean: the Correctness / robustness
Test coverage
Minor / style
Nice touches
Overall LGTM - the flagged items are minor robustness/coverage notes, not blockers. |
REQUIRED_GAP_AREAS is now empty (all #4890 gaps closed), so the test exercises the missing-gap mechanism against a temporarily patched required area instead of relying on the live config having 'connection'.
Code Review: HA-aware Bolt ROUTE (#5085)Thorough, well-scoped change. The Strengths
Minor points (non-blocking)
Performance / correctness
VerificationStatic review only - I did not run the full Overall: clean, well-tested, and consistent with repo conventions. LGTM modulo the minor doc/comment nits above. 🤖 Generated with Claude Code |
awaitRoutingTable catches Exception, but the SUCCESS assertion threw an AssertionError (an Error), bypassing the retry during a failover window. Throw an IOException on a non-SUCCESS ROUTE so the retry loop keeps polling as intended (Claude review).
|
Addressed the retry-robustness note in 5423acb. Test #1 ( Leaderless-branch coverage - Acknowledged. The HA-active-but-leaderless ( Auth-gate error classification - Keeping Single-router during election / configured-membership readers - Both transient/self-healing and documented on the interface + Route/auth/IT tests all green locally. |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 12 |
🟢 Coverage 61.33% diff coverage · -7.21% coverage variation
Metric Results Coverage variation ✅ -7.21% coverage variation Diff coverage ✅ 61.33% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (35e9c2e) 135513 101089 74.60% Head commit (5423acb) 167375 (+31862) 112786 (+11697) 67.39% (-7.21%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#5085) 75 46 61.33% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
Code Review: HA-aware Bolt ROUTE response Reviewed the full diff against the CLAUDE.md conventions. This is a high-quality, well-scoped change: the design (single Strengths
Suggestions (non-blocking)
Performance and style No concerns. The ROUTE path allocates a few small lists per call (fine for a low-frequency control message), Overall: solid, tested, and safe to merge after considering the coverage suggestions above. Reviewed with Claude Code. |
Review: HA-aware Bolt ROUTE response (5085)Thorough, well-executed PR. The design is clean, the layering is respected (bolt reaches HA only through Strengths
Minor observations (non-blocking)
Verdict |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5085 +/- ##
=========================================
Coverage 65.62% 65.62%
- Complexity 842 846 +4
=========================================
Files 1690 1690
Lines 135513 135572 +59
Branches 28987 29002 +15
=========================================
+ Hits 88924 88973 +49
- Misses 34461 34466 +5
- Partials 12128 12133 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Closes #5002. Part of #4890 (Group C protocol/type-fidelity gaps), epic #4882 (Bolt Driver Compatibility Certification). Certifies conformance scenario
CONN-004.Problem
BoltNetworkExecutor.handleRoutealways returned this node's own address asWRITE,READ, andROUTE, regardless of cluster topology. Against a real HA cluster aneo4j://driver could not discover the leader/followers or route reads vs writes; routing/discovery was unproven.Approach
The
boltmodule reaches the cluster only through theHAServerPlugininterface (ha-raft is test-scope there). The HA layer knew each peer's host plus Raft/HTTP/HTTPS ports, but no Bolt port - and theneo4j://driver actually connects to whatever addresses ROUTE returns, so they must be genuinely reachable on the Bolt port.HA_SERVER_LISTobject form with an optionalbolt:<port>field (host:{raft:2434,http:2480,bolt:7687}), parsed into aboltAddressesmap. When a peer omits it, derivepeerHost:localBoltPort(homogeneous-cluster assumption) with a one-time WARNING - exactly mirroring the establishedresolveHttpAddresspattern.HAServerPlugingains a singlegetBoltRoutingTable()returning an immutableBoltRoutingTable(writer, readers)snapshot computed from onegetLeaderId()read (so writer and readers cannot disagree about the leader), implemented byRaftHAPlugin->RaftHAServer. Readers reflect the configured cluster membership (parity withgetReplicaAddresses()).handleRouterequires an authenticated (READY) session - ROUTE enumerates every peer's Bolt endpoint, so it is not served to an unauthenticated caller. When authenticated, it advertises the leader asWRITE+ROUTEand each follower asREAD+ROUTE. The table is rebuilt on every ROUTE call, so reader/writer classification tracks leader changes automatically. Fallbacks: HA-active-but-leaderless advertisesREAD+ROUTEonly (never write to a possible follower); true single-node advertises all three roles - both using the connection's actual bound Bolt port.Tests
Bolt5002RoutingTableIT(3-nodeBaseRaftHATest,@Tag("slow")): asserts the routing table classifies the true leader as writer and followers as readers, that aneo4j://driver routes reads and writes end to end, and that writer classification tracks a leader change (stops the leader, waits for re-election, re-checks the table). Polls the queried follower to tolerate the brief leader-unknown window after connect.RaftHAServerAddressParsingTest:bolt:parsing (present/absent, named, positional-form has no Bolt field) and thederiveBoltAddresshelper.BoltProtocolIT#routeTableSingleNodeReturnsSelfForAllRoles: single-node (non-HA) ROUTE is unchanged.BoltProtocolIT#routeBeforeLogonIsRejected: ROUTE on a Bolt 5.x deferred-auth session before LOGON is refused.Conformance
Flips
CONN-004inbolt/conformance/spec.yamlfromexpected-failtopassing; the residual note documents that heterogeneous Bolt ports require the object-formbolt:field.Acceptance criteria (#5002)
CONN-004passes against a multi-node cluster; aneo4j://driver discovers writer + reader(s) and routes accordingly.Design and plan committed under
docs/superpowers/specsanddocs/superpowers/plans.🤖 Generated with Claude Code