Skip to content

MIR-1619: Choose cluster routes when connecting - #1424

Open
evanphx wants to merge 5 commits into
mainfrom
evan/mir-1619-decide-how-to-route-to-a-cluster-per-connection-not-at
Open

evanphx wants to merge 5 commits into
mainfrom
evan/mir-1619-decide-how-to-route-to-a-cluster-per-connection-not-at

Conversation

@evanphx

@evanphx evanphx commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Why

A laptop can change networks after adding a cluster. Persisting the route selected by cluster add leaves it trying an unreachable direct address or unnecessarily relaying through cloud. Probing on every RPC client avoids that stickiness but adds one round trip per client, or two for fingerprint-only entries.

Approach and tradeoffs

Remember the last route that successfully established an RPC client and use it without probing. A failed connection invalidates that route and triggers verified direct discovery followed by cloud fallback. Successful cloud routes expire after 30 seconds without extending on reuse, so returning to the direct network is noticed. Failed routes are not cached. When neither route works, normal CLI output preserves both failures and the existing direct-connect diagnosis.

Resolve credentials once before routing starts and reuse that bearer token across connection attempts. An expired or rate-limited login therefore stops before discovery or cache invalidation, retaining login or retry guidance rather than becoming a network error.

Retain advertised addresses and cloud CA fingerprints when adding remotely. Fingerprint-only entries recover the pinned CA through a failed normal TLS handshake, then retry with that root trusted; verification is never disabled. The recovered CA is cached with the successful route. Certificate recovery accepts the dial hostname or the established api.miren fallback, not an arbitrary same-CA certificate name. DNS resolution honors cancellation and probes explicitly close their sockets and transports.

Compatibility and review concerns

  • Explicit --via-cloud still forces the relay. Older via_cloud: true entries also remain forced: those configs cannot distinguish explicit intent from automatic fallback and discarded direct metadata. The docs give re-add instructions, including aliases and organization selection; this PR does not automatically migrate them.
  • Only connection setup can rediscover a route and fall back. Application operations are never automatically replayed.
  • The first connection after leaving a network can still pay for the failed remembered direct dial and rediscovery before reaching cloud. Lifecycle polling does not pin its starting route, and restart completion latency has not been measured.
  • The existing cluster list --format json ambiguity for automatic entries remains outside these routing and diagnostic fixes.

Verification

  • iso run env GOTOOLCHAIN=go1.26.10 bash hack/test.sh ./pkg/rpc ./clientconfig ./cli/commands -p 1 -failfast: 1,650 tests passed.
  • Focused route reuse, credential failure, and combined-diagnostic tests passed three repetitions under -race. Coverage includes one sign-in attempt after refresh 401/429, rendered login/rate-limit guidance, and preserving cached direct trust after an authentication failure.
  • Real-QUIC tests cover stored CA/server-name verification, legacy YAML, DNS cancellation, and credential nondisclosure to rejected peers. The routing-overhead and combined-failure regressions fail against the previous routing implementation; the credential regression reproduced repeated sign-ins before the fix.
  • Warm setup over a simulated 126 ms RTT (median of five runs) saved about 131 ms for one saved-CA client, 261 ms for fingerprint-only, and 388 ms for three saved-CA clients. These are connection-setup measurements, not real-cluster CLI benchmarks.
  • make lint: docs lint passed and both Go modules reported zero issues. Remote CI for the latest commit is pending; the previous run also hit a Docker Hub 502 while pulling nginx in a service-controller test.

Issue: https://linear.app/miren/issue/MIR-1619
Amp thread: https://ampcode.com/threads/T-01a11ecf-0fe7-72ca-8329-1dc52b5685ff

miren-code-agent[bot]

This comment was marked as outdated.

Comment thread clientconfig/routing.go Fixed
Retain advertised addresses and CA pins when adding remote clusters, probe verified direct routes on connection, and briefly cache failed direct attempts before using cloud. Preserve explicit and legacy forced-cloud entries.
Recover pinned roots from failed verified handshakes and retry with normal TLS validation. Add routing diagnostics and distinguish add-time cloud fallback from forced routing in CLI output.
miren-code-agent[bot]

This comment was marked as outdated.

@evanphx
evanphx force-pushed the evan/mir-1619-decide-how-to-route-to-a-cluster-per-connection-not-at branch from 146a65f to b58ecbb Compare October 29, 2026 23:39
miren-code-agent[bot]

This comment was marked as outdated.

@evanphx
evanphx marked this pull request as ready for review October 30, 2026 22:12
@evanphx
evanphx requested a review from a team as a code owner October 30, 2026 22:12
miren-code-agent[bot]

This comment was marked as outdated.

@phinze phinze self-assigned this Nov 3, 2026

@phinze phinze left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yesssssssss a miren client roaming plan w/ no extra fees!

no fees, but maybe some costs on steady state overhead that we can shave down. we spent some time staring at the "probe on every RPC client" model and wondering if the extra call added meaningful waiting. results below with a suggestion on a mitigation that could be done now or in a follow up.


🤖 Setup: this branch (84ddff1) vs. its base (2fbd2cd), read-only commands against real clusters, median of 10 runs.

Routing behaves as described

  • ✅ Reachable cluster with CA saved: goes direct.
  • ✅ Fingerprint-only entry (CA removed, real SHA-1 pinned): recovers the CA from a real cluster's chain and goes direct.
  • ✅ Wrong pin: refused with cluster CA fingerprint mismatch, falls back to the relay, and the command succeeds.
  • ⚠️ Cluster unreachable both ways: fails faster (2.5s, then 0.5s while the hint lasts, vs. 5s), but with a worse error. See the inline comment.

Cost: one RTT per RPC client

Scenario Base Branch Δ
app list, LAN cluster 87ms 91ms noise
app list, ~126ms RTT, CA saved (1 client) 753ms 875ms +123ms (1 RTT)
app list, ~126ms RTT, fingerprint-only 759ms (CA saved; base has no fp-only path) 1019ms +260ms (2 RTT)
app status, ~126ms RTT (3 clients) 2214ms 2577ms +363ms (3 RTT)

Fingerprint-only entries pay double because the recovered CA isn't saved, so every probe does the fail-then-redial handshake. A static scan of the CLI found that most commands open one RPC client. The everyday multi-client ones are deployment create (5: telemetry, build, deployment, and two for the health wait), app status, env set/delete, and app rollback (3 each). At this distance, a deploy should pick up about 0.6s, or about 1.3s fingerprint-only. That's estimated from the call sites, not measured, since a deploy writes to the cluster.

Suggested mitigation: flip the cache

Remember the last route that worked and use it without probing. When connecting over it fails, forget it, probe, and retry once. A failure in rpc.State.Connect happens during the dial, resolveCapability, or sendIdentity, all before any application call, so the retry can't replay an operation. A remembered cloud route would need a TTL so a return to the direct network gets noticed. Steady state then costs nothing extra on every command.

Comment thread clientconfig/routing.go Outdated
Comment on lines +48 to +51
if info, err := os.Stat(cachePath); err == nil && time.Since(info.ModTime()) < cloudRouteCacheTTL {
reason, _ := os.ReadFile(cachePath)
log.G(ctx).WithField("cluster", c.XID).WithField("direct_errors", string(reason)).Debug("Using cached direct-route failure; trying cloud")
return c.viaCloudOptions(ctx, config)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤖 Not in this diff, but cluster restart and cluster upgrade interact with this path. followRemoteOperation (server_lifecycle_remote.go) polls through lifecycleClient, which opens a new client on every poll while the server is deliberately down. With this branch, each poll probes first. Once one probe fails mid-restart, this hint sends the next 30s of polls to the relay, whose link to the cluster is bouncing too. It should still converge, but the polls end up chasing the relay instead of waiting on the direct address that's about to come back. That comes from reading the code. We didn't run a restart.

If the routing ever grows a "remember the last good route" mode, that poll loop probably wants an option to pin the route it started on and treat a failure as "still restarting" rather than a reason to re-route.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

That matches how I read routedOptions. Only failures are cached, and the hint is written whenever viaCloudOptions returns options, so one failed probe while the server is down sends the following polls to the relay for 30s, whatever state the relay's link is in. This is the same per-connection probing I'd already flagged for a human to judge. Your restart case is a sharper example of it than anything I gave. I agree it doesn't need to change in this PR if the loop still converges. Pinning the starting route in followRemoteOperation seems like the right fix if a 'last good route' mode ever arrives. A test running cluster restart against a discovered cluster would show whether the relay detour actually delays when the restart is reported as finished.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Updated context from the new code. I got this from reading it, not from running a restart. Now that successful routes are remembered, the first poll after the server goes down waits the full 5s handshake timeout on the remembered direct route. It then re-probes the same address for up to 3s before trying the relay. If the relay answers, that route is cached for 30s and later polls go through cloud, as you described. If the relay fails, nothing is cached and every poll repeats the 3s probe followed by the relay. The author has said lifecycle pinning is out of scope for this PR, so I'm adding this only as context for whoever picks it up.

Comment thread clientconfig/routing.go Outdated
return nil, err
}
log.G(ctx).WithField("cluster", c.XID).WithField("direct_errors", strings.Join(failures, "; ")).Debug("No verified direct route; trying cloud")
opts, err := c.viaCloudOptions(ctx, config)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤖 When both routes fail, the user now sees the relay's raw error. For a cluster that's down from here, the base build prints the usual diagnosis:

couldn't reach cluster "wilderness" at 163.192.194.181:8443
  Nothing answered after 5s. ... Try miren doctor

This branch prints:

error performing message request: failed to WebSocket dial: expected handshake response status code 101 but got 404

The direct failures only show up with -vv. Failing faster is nice: 2.5s, then 0.5s while the hint lasts. But could the direct-probe failures travel with the relay options (or the hint), so that when connecting through the relay fails, the error reads something like "couldn't reach wilderness directly (7 addresses timed out) or through Miren Cloud (no link to the cluster)" and still reaches the existing diagnosis in rpc_errors.go?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I raised this gap on the earlier routing thread and then accepted it, because the reasons were logged under -vv and cached with the hint. Your before/after output shows I underweighted it. The cost isn't only that the direct address goes missing from the message: the whole diagnosis disappears. wrapRPCError (cli/commands/rpc_errors.go:23-35) only recognises *rpc.ResolveError. The relay's WebSocket dial error isn't one, so it passes through unchanged. In routedOptions (clientconfig/routing.go:73-100), failures goes only to the debug log and the cache file, never into the error that's returned. For a discovered cluster that's simply down, the most common failure there is, this is now a step backwards from the base build. I'm moving my verdict to not ready until it's fixed.

Your suggestion is the right shape. Carry the direct failures with the relay options, or read them back from the cache file on a hint hit, so the relay's error can be wrapped in a typed error holding both. Then teach rpc_errors.go to render the combined message. When every direct probe timed out, it could still reach the existing unreachable diagnosis for the direct address. A test that fails both routes and asserts on the rendered diagnostic, not only the debug log, would keep this from slipping back.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🤖 We are no longer treating a direct miss as evidence that cloud is viable. The route cache now requires successful RPC client setup, so a dead relay cannot turn one direct outage into 30 seconds of relay-only failures.

For the both-routes-down case, the direct failures and relay failure travel together in a typed error. Default-verbosity output names the cluster and both routes, retains the direct address and timeout diagnosis, and includes miren doctor guidance. The underlying direct ResolveError remains available through error unwrapping. This deliberately replaces the earlier decision to leave the useful explanation behind -vv; the combined failure must be actionable without asking the user to rerun it.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I checked this at cf26a97. routedOptions saves a cloud route only after connect succeeds, so a failed relay leaves no hint behind. When both routes fail it returns ClusterRouteError{Direct, Cloud}. wrapRPCError diagnoses the first direct ResolveError (an unreachable address still gets "Nothing answered after…"), adds both routes as facts, and keeps miren doctor. TestRPCClientDiagnosesBothRoutesFailing covers two consecutive connections and checks that the ResolveError still unwraps. That fixes the regression I'd flagged here. This is phinze's thread, so I'll leave resolving it to them. There's a related problem inline at routing.go:100: credential errors now go down this same "couldn't reach either route" path.

Avoid repeated direct probes by remembering successful routes and recovered trust. Rediscover only during connection setup, expire successful cloud routes, and retain both failures in the normal CLI diagnostic.

evanphx commented Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

On the approval review timing concern, I compared the previous routing implementation with the successful-route cache using real QUIC connections through a UDP delay proxy (63 ms each direction, approximately 126 ms RTT). These are medians of five warm setup runs, excluding the first run:

Connection setup Before After Saved
Saved CA, one client 521 ms 390 ms 131 ms
Fingerprint-only, one client 650 ms 389 ms 261 ms
Saved CA, three clients 1,557 ms 1,169 ms 388 ms

These are isolated connection-setup measurements, not reruns of the real-cluster CLI commands in your review, so the absolute totals are not directly comparable. The savings match the extra round trips you identified.

The distinction for fingerprint-only entries is cold versus warm: the initial connection still pays for verified CA recovery, but subsequent clients reuse the recovered CA and need only their own RPC handshake. Saved-CA entries can avoid even the first preliminary probe. A remembered route is invalidated on a connection-stage failure; capability lookup rejections do not trigger rediscovery, and application calls are never replayed. Cloud success expires after 30 seconds without sliding on reuse, and a failed remembered cloud route triggers direct discovery immediately.

This does not add lifecycle-poll route pinning, and I have not measured restart completion latency; the setup results should not be read as that validation.

— e + 🤖

miren-code-agent[bot]

This comment was marked as outdated.

Keep authentication failures out of route rediscovery and preserve login and rate-limit diagnostics. Reuse the resolved bearer token across connection attempts, retain refresh retry timing, and correct the connection-setup retry documentation.

@miren-code-agent miren-code-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🍪 biscuit: ✅ ready to merge — auto-review, non-blocking

63c6033 fixes the credential regression that was blocking this, and I think the PR is ready for human review.

Credential failures now stay out of routing. routedOptions resolves the bearer token once, before it builds the routing closure (routing.go:79-82). attempt reuses that token for direct and cloud alike. So an expired or rate-limited login fails while the State is being built, and that has three effects:

  • The remembered route isn't deleted.
  • Nothing is probed, and sign-in isn't retried.
  • The error goes back through unusableClusterError.

Two tests pin this down:

  • TestRPCClientRoutingCredentialErrors covers a refresh that returns 429 and one that returns 401. It asserts exactly one sign-in, the "too many sign-ins / Retry in" or miren login guidance, no miren doctor, and that CloudRateLimitError / ErrLoginRequired still unwrap.
  • TestSuccessfulClusterRoutes checks that a credential failure leaves the cached direct route and the recovered CA in place.

The token.go change that turns a refresh 429 into CloudRateLimitError (:106-107) is what makes the rate-limit message reachable. It applies to every caller that refreshes a token, not only routed clusters, and I think that's correct. The token is still resolved once per State, which is how directRPCOptions already worked, so long-lived States aren't any worse off.

Docs and description match the code. "Only connection setup is retried" and the new paragraph on credentials both describe what routedOptions does. The PR description describes remembering the last route that worked, a cloud TTL that doesn't extend on reuse, and resolving credentials first.

Open items a reviewer should know about:

  • The cluster list --format json ambiguity (cluster.go:104) is still open. The description calls it out of scope, so I'm treating it as your call.
  • The first command after leaving a network takes about 8s. The description acknowledges this.
  • Lifecycle polling during cluster restart doesn't pin its route (phinze's thread). The description acknowledges this too.

I'm not recommending approval. This is a large change to connection setup and to how TLS trust is established for remotely added clusters. phinze is still reviewing it, and the route caching and CA recovery in routing.go should get a human sign-off.


🍪 full review note · reviewed at 63c6033

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants