fix: skip an unusable client-route row, keep the refresh (DRIVER-201)

[nikagra]

A system.client_routes row the driver cannot use took the whole refresh down: ClientRouteRecord's constructor threw inside ClientRoutesTopologyMonitor.refresh's row loop, the exception reached the stage's exceptionally, and every route in that pass was discarded.

Fixes

• Catch per row rather than at the two known throw sites, so a malformed host_id or port cell costs one route, not the pass.
• Resolve the connection_addr override before touching the address column, so an empty, absent or undecodable column cannot defeat it, as ClientRouteProxy documents.
• Scope the query to configured connection_ids. Scylla broadcasts every changed key, so an event routinely names other tenants' proxies, and taking its IDs verbatim drew conclusions from their rows. Pre-existing since 2dea0bacb1.

Changes, and what they cost

• Evict only where the pass can prove a host absent. An unusable row is evidence the route exists — a deletion arrives as an absent row — so the cached record is kept. Kept indefinitely if the row never becomes readable: dropping it strands the node on an unreachable private address, while a stale route fails fast on connect. Rationale in keepableHostIds and withRetainedCachedRoutes.
• Because nothing evicts such a route, count the passes carrying each one over and report at ERROR past the third.
• The empty-result counter keys on zero rows, not zero rebuilt routes, so unusable rows are reported rather than counted as an empty result.

Verified: mvn clean test -pl core — 3989 tests, 0 failures; each new guard checked by mutating it and watching the matching test fail. Not covered: ClientRoutesIT (CI only), and whether the server ever writes a permanently unusable row.

Filed, not fixed here: #1063 (last-write-wins per host_id), #1064 (address never syntax-checked).

Part of the #890 split. Refs: #890

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: QUIET

Plan: Advanced

Run ID: 194f1670-0ba9-447f-b82c-4e3544dcc973

📥 Commits

Reviewing files that changed from the base of the PR and between da711bf and 455a0da.

📒 Files selected for processing (2)

• core/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesTopologyMonitor.java
• core/src/test/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesTopologyMonitorTest.java

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

ClientRoutesTopologyMonitor now processes malformed route rows independently, tracks readable host IDs, and applies connection-address overrides through shared helpers. Targeted and full refreshes retain cached routes when host absence is unproven. Carry-over counts track repeated unusable rows. Event queries ignore unconfigured connection IDs. Tests cover malformed values, overrides, duplicate hosts, selective eviction, cache retention, and carry-over tracking.

Sequence Diagram(s)

sequenceDiagram
  participant RouteUpdateEvent
  participant ClientRoutesTopologyMonitor
  participant AdminQuery
  participant RouteCache
  RouteUpdateEvent->>ClientRoutesTopologyMonitor: provide connection IDs
  ClientRoutesTopologyMonitor->>ClientRoutesTopologyMonitor: filter configured IDs
  ClientRoutesTopologyMonitor->>AdminQuery: query route rows
  AdminQuery->>ClientRoutesTopologyMonitor: return route rows
  ClientRoutesTopologyMonitor->>RouteCache: retain or evict routes

Loading

Suggested reviewers: dkropachev

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 455a0

This change reworks client-route refresh handling to tolerate malformed rows and defer eviction until host absence is proven, with accompanying tests. No unresolved, evidence-backed defect was established in this review pass, so the change appears mergeable as-is.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 36.73% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 49 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly identifies the main fix: skip an unusable client-route row without discarding the refresh.
Description check	✅ Passed	The description directly explains the refresh failure, the per-row handling, route retention, query scoping, empty-result behavior, and test verification.

• Fix all pre-merge checks with AI

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Copilot]

🟡 Changes recommended

Empty addresses are rejected before a valid configured address override can be applied.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Ensures malformed client-route rows do not abort an entire topology refresh.

Changes:

• Validates empty addresses and invalid ports.
• Isolates constructor failures per row.
• Adds regression tests preserving valid routes.

File summaries

File	Description
ClientRoutesTopologyMonitor.java	Skips unusable rows while continuing refreshes.
ClientRoutesTopologyMonitorTest.java	Tests mixed invalid and valid rows.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Mixed full refreshes and targeted null-address refreshes can still remove valid cached routes.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

core/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesTopologyMonitor.java:444

• A mixed full refresh still drops cached routes for every skipped row: as soon as one good row makes newRoutes non-empty, this replacement removes the old entry for each ID in skippedHostIds. That sends those nodes back to their private addresses, the same failure the targeted/all-unusable branches explicitly avoid. Preserve cached entries for skipped IDs while replacing valid and genuinely absent rows, and cover a pre-populated cache with one good and one unusable result.

                } else {
                  consecutiveEmptyResults.set(0);
                  resolvedRoutesCache.set(Collections.unmodifiableMap(newRoutes));
                  LOG.debug(
                      "[{}] Updated client routes: {} routes loaded", logPrefix, newRoutes.size());

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

The all-unusable path retains cached routes whose host IDs are absent from a full refresh.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Targeted refreshes can still evict routes for unreadable host IDs, and malformed addresses can prevent valid overrides.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

core/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesTopologyMonitor.java:470

• The table address is decoded before looking up the override. If getString("address") throws for a malformed cell, the row is skipped even when its connection_id has a valid configured override, contradicting the method's documented outright-replacement behavior. Resolve the override first and only read the table address as the fallback.

    String tableAddress = row.isNull("address") ? null : row.getString("address");
    String connId =
        row.contains("connection_id") && !row.isNull("connection_id")
            ? row.getString("connection_id")
            : null;
    String override = connId == null ? null : connectionAddrOverrides.get(connId);
    return override != null ? override : tableAddress;

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟢 Approval recommended

The implementation matches the stated behavior and thoroughly tests the identified failure modes.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[dkropachev]

[P2] Retain by the complete route identity here. system.client_routes is keyed by (connection_id, host_id), but this check matches only host_id. With cached (A, H) and a refresh containing only an unusable (B, H) row (for example, a missing port), the deleted A route is retained forever because every nonempty pass resets the empty-result counter. Track the cached route source, or avoid carry-over when multiple connection IDs make the match ambiguous.

[nikagra]

Took the second option: carry-over now applies only where host_id is the whole route identity — one configured connection_id. With several, a refresh keeps only what it rebuilt (full refresh replaces, targeted sweep evicts), so the deleted A route goes on the next pass. The sweep keyed on hostIdsInResult too, so both writers now read one derived set.

Tracking the source doesn't stop at the record: once the key is (connection_id, host_id), the sweep needs a scope guard it doesn't need today — cached (A,H), an event naming only B, query connection_id IN ('B'), and absence of (A,H) from a result that never asked about A would evict a live route. Eviction scope then has to mirror the WHERE clause. That is #1063, which already owes a tie-break rule for the same reason, so I left it there.

Cost, stated in the javadoc: with several connection IDs an unusable row costs the route again — no worse than scylla-4.x, and in that config the cached entry is already whichever row the server returned last.

b440055080; 3 new tests, all red on the old head.

[nikagra]

Reversing the answer I gave here. The carve-out is gone in 455a0daadf, and hostIdIdentifiesRoute with it: retention no longer varies with the connection-ID count.

Your objection was that the cached route is retained forever, since every non-empty pass resets the empty-result counter. That is right, but dropping the route was the wrong way to bound it. An unusable row is evidence the route exists — a deletion arrives as an absent row, and absence still evicts — so the ambiguity you named is about which route the cached entry is, not about whether one is there. Dropping it costs the node its only reachable address until the table changes; keeping a stale one costs a connect that fails fast. So the unboundedness is now answered by reporting rather than by eviction: recordCarryOvers (:806) counts the passes that carry each route over and logs at ERROR past the third.

I also owe a correction on "no worse than scylla-4.x". Against ce317fa90c:

• the targeted sweep evicted on !newRoutes.containsKey(hostId), so an event-named host whose row came back unusable was already evicted — unchanged by this PR, and the claim held there;
• the full refresh did resolvedRoutesCache.set(newRoutes) outright, so a skipped row's host was already dropped;
• but the empty-result guard keyed on newRoutes.isEmpty(), so an all-unusable pass bought three passes of grace. Keying it on rowCount removed that, and the carve-out then emptied the cache in one pass with several connection IDs — or in none at all pre-PR, since a throwing row aborted the whole pass. That part of the claim was wrong, and it is what 455a0daadf fixes.

The ambiguity itself stays with #1063, where the cache key is.

Tests: the three multi-endpoint tests inverted, plus five covering the carry-over counting and the guard. 3989 core green; each guard re-checked by mutating it and watching the matching test fail.

[Copilot]

🟢 Approval recommended

The implementation matches the stated behavior and includes comprehensive regression coverage.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[dkropachev]

[P2] Derive route identity from IDs used by this query. Scylla broadcasts all changed keys, and buildQuery uses event IDs verbatim. With configured A plus unconfigured B, an unusable (B,H) row can make this flag retain a deleted cached (A,H) route. Filter event IDs against configured IDs or base this decision on actual query scope.

[nikagra]

Took the first remedy. allowedConnectionIds (ClientRoutesTopologyMonitor:594) intersects the event's IDs with the configured ones, and an event naming none of them is dropped rather than queried, so a pass's scope is always inside the configured set.

Splitting the two halves by provenance, since I ran them together before and that blurred it:

• The query taking event IDs verbatim is pre-existing, since 2dea0bacb1, the original PrivateLink commit — git log -S"eventConnectionIds" has nothing between. With it, a usable (B,H) row for an unconfigured proxy installs a route through a proxy this client is not configured to use. That consequence is mine, not something you raised; I previously wrote it as though it were yours.
• The retention consequence is the one you named, and it was introduced by this PR: only hostIdIdentifiesRoute (b440055080) made an unusable (B,H) row able to keep a deleted cached (A,H).

gocql has never done otherwise: filterAllowedConnectionIDs (client_routes.go:455-466) intersects and continues when the result is empty, and 8995b14 "always block unknown endpoints" removed the option to skip it. So the Java side was a port gap rather than a choice.

Since then hostIdIdentifiesRoute has gone (455a0daadf) — retention no longer varies with the connection-ID count, so the flag had no readers left. The filter stands on its own regardless: a row is only evidence about the connection it belongs to, and querying another tenant's proxy is wrong whatever retention does with the result.

An event naming no connection at all still falls back to every configured ID — that one carries no scope, so it cannot rule this session out.

Tests: should_query_only_the_configured_connection_ids_an_event_names and should_ignore_an_event_that_names_no_configured_connection_id. Both red against b440055080, where the query is literally WHERE connection_id IN ('conn-1', 'conn-unconfigured') AND host_id IN (...).

[dkropachev]

[P2] Include freshly rebuilt hosts in this set. During a targeted refresh with usable H1 plus an unreadable-ID row, an empty old cache makes this return empty; H1 is merged, then immediately removed by the event-host sweep. Union cached keys with newRoutes.keySet() (or otherwise preserve proven-present hosts).

[nikagra]

Fixed in f5a8c1803f, one line, now at ClientRoutesTopologyMonitor:783:

keepable.addAll(newRoutes.keySet());

keepableHostIds used to return cachedRoutes.keySet() once a row had an unreadable host_id. Every host the pass rebuilt is now in the keep-set by construction. As of 455a0daadf that union is unconditional, below both branches, so a future branch cannot miss it.

Worth being explicit about where the defect came from: the shared keep-set is new in this PR (b440055080), so this is one the PR introduced rather than one it inherited. Before it, the sweep keyed on !newRoutes.containsKey(hostId), and a host the pass had just rebuilt was in newRoutes and so never swept.

The cause is that the set has two readers and I gave it one contract. withRetainedCachedRoutes starts from newRoutes and reads the set as what to add, so omitting a rebuilt host is harmless there. The targeted sweep reads it as the complete keep-list and removes every event host ID outside it, so omitting one deletes it. The union makes both readings the same set. The full-refresh writer is unaffected, as you say.

One correction to the framing: the trigger is not the empty cache but whether the rebuilt host was already cached. With cache {B}, result [good A, unreadable row] and an event naming both, A was merged and swept out too — so it bit every pass that first discovered a host.

Tests: should_keep_rebuilt_route_when_a_row_had_an_unreadable_host_id_and_cache_was_empty and should_keep_both_a_rebuilt_and_a_carried_over_route_when_a_row_was_unreadable, red against b440055080 with {} and {carried} respectively. Re-checked since by making the union a no-op: those two are exactly what fails.