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:
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:
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.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.
Posted to the PR's comment threads when you submit.
63c6033. routedOptions now resolves the bearer token before routing starts (routing.go:79-82), and attempt only wraps dial options around that token. So a sign-in failure comes back from RPCOptions/State before any route is touched, and it reaches unusableClusterError.TestRPCClientRoutingCredentialErrors covers both 429 and 401. It asserts a single refresh call, the rate-limit or miren login guidance with no miren doctor, and that the typed errors still unwrap. TestSuccessfulClusterRoutes shows the cached direct route and recovered CA survive an auth failure. That's the fix I asked for. Resolving.
63c6033. The PR description now lists it as out of scope for the routing and diagnostics work, so I'm treating it as your accepted tradeoff and not a blocker. It's still a real gap for scripts that read cluster list --format json, so I'll leave the thread open for you or a reviewer to close, or to track as a follow-up.Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1424,
"verdict": "ready",
"event": "comment",
"summary": "`63c6033` fixes the credential regression that was blocking this, and I think the PR is ready for human review.\n\n**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:\n- The remembered route isn't deleted.\n- Nothing is probed, and sign-in isn't retried.\n- The error goes back through `unusableClusterError`.\n\nTwo tests pin this down:\n- `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.\n- `TestSuccessfulClusterRoutes` checks that a credential failure leaves the cached direct route and the recovered CA in place.\n\nThe `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.\n\n**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.\n\n**Open items a reviewer should know about:**\n- 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.\n- The first command after leaving a network takes about 8s. The description acknowledges this.\n- Lifecycle polling during `cluster restart` doesn't pin its route (phinze's thread). The description acknowledges this too.\n\nI'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.",
"replies": [
{
"thread_id": "PRRT_kwDONZ__ss6zX8wD",
"body": "I checked this at `63c6033`. `routedOptions` now resolves the bearer token before routing starts (`routing.go:79-82`), and `attempt` only wraps dial options around that token. So a sign-in failure comes back from `RPCOptions`/`State` before any route is touched, and it reaches `unusableClusterError`.\n\n`TestRPCClientRoutingCredentialErrors` covers both 429 and 401. It asserts a single refresh call, the rate-limit or `miren login` guidance with no `miren doctor`, and that the typed errors still unwrap. `TestSuccessfulClusterRoutes` shows the cached direct route and recovered CA survive an auth failure. That's the fix I asked for. Resolving.",
"resolve": true,
"resolve_reason": "addressed"
},
{
"thread_id": "PRRT_kwDONZ__ss6yKQ5a",
"body": "This is unchanged at `63c6033`. The PR description now lists it as out of scope for the routing and diagnostics work, so I'm treating it as your accepted tradeoff and not a blocker. It's still a real gap for scripts that read `cluster list --format json`, so I'll leave the thread open for you or a reviewer to close, or to track as a follow-up."
}
],
"posted_to_pr": true,
"head_sha": "63c60335a75514e789a5c3555ae7d9223f144635"
}cf26a97 fixes the two diagnostics problems from phinze's thread. It also introduces a new regression in a common failure case, so this isn't ready yet.
What the commit fixes. A cloud route is cached only after connect succeeds, so a dead relay no longer sticks for 30s. When both routes fail, the error is a ClusterRouteError carrying both causes. wrapRPCError renders it with the direct ResolveError diagnosis, the relay error, and miren doctor. TestRPCClientDiagnosesBothRoutesFailing checks this across two consecutive connections. Remembering successful routes also settles the per-connection probe cost I'd handed to a human: saved-CA entries skip the probe, and fingerprint-only entries recover the CA once and reuse it. Your measurements and phinze's review support that, so I've dropped that handoff.
New regression: credential failures now look like network failures (clientconfig/routing.go:89-103, :170-182). Before, viaCloudOptions/directRPCOptions ran when the State was built. A failed TokenForIdentity (expired login, CloudRateLimitError) then went through unusableClusterError, which points you at miren login or shows the rate-limit "waiting will help" message. Those calls now run inside attempt. Their errors aren't ResolveErrors, so routeConnectionFailed treats them as connection failures. For an expired or rate-limited sign-in on any discovery-added cluster, that means:
miren doctor, and no login or retry guidanceDetails are in the inline comment.
Docs and description no longer match the code. In cloud-routed-clusters.md, the "Route selection precedes RPC" note still says a failed direct RPC connection is not retried through cloud. The paragraph above it, and routedOptions, now rediscover the route and fall back. The PR description still describes the older design: failed direct attempts cached for 30s, success re-probed on every connection, and no relay fallback after a direct RPC failure.
For a human to weigh: the first command after leaving the network now waits the full 5s remoteHandshakeTimeout on the remembered direct route. Rediscovery then re-probes the same address for up to 3s before it tries cloud, so that first command takes about 8s where it used to take about 3s. Skipping the address that just failed in discoverDirectRoute would recover part of that.
My cluster list --format json thread (cluster.go:104) still stands.
viaCloudOptions/directRPCOptions here is almost always a credential problem from TokenForIdentity, such as an expired login or a *CloudRateLimitError (ratelimit.go:44), rather than a route problem. It comes back as a plain error, and routeConnectionFailed (:170-182) treats anything that isn't a ResolveError as a failed connection. So the caller:ClusterRouteErrorwrapRPCError renders that as "couldn't reach … directly or through Miren Cloud" and suggests miren doctor. Before this commit, these errors came out of ClusterConfig.State and went to unusableClusterError in rpcClient, which shows the miren login hint and cloudRateLimitError. For routed clusters neither can be reached now. It also contradicts the docs' claim that a permissions rejection doesn't trigger rediscovery.
One way to fix it: wrap option-building errors in their own type, have routeConnectionFailed return false for that type so nothing is retried or invalidated, and have rpcClient/wrapRPCError send it to unusableClusterError. Another: resolve the bearer token once, before routing starts. A test with a token identity whose refresh returns 429 would lock this in. It should assert a single sign-in attempt and the rate-limit diagnostic.
Posted to the PR's comment threads when you submit.
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.Verdict: not_ready
The only new commit is 84ddff1, which merges main into the branch. The PR's own changes in routing.go, local.go, cluster_add.go, clientconfig.go and the tests are the same as at b58ecbb. Nobody has left new comments or replies, and every thread I opened is resolved. The merge brought in code from main, so I checked for new readers of ClusterConfig.ViaCloud and found only connectTarget in connect_notice.go. For an automatic entry it does no harm: with no hostname, the notice falls back to the cluster name. I still think this is ready for a human, but it shouldn't merge without one.
Still open: cluster list --format json (cluster.go:51-53, :93). The ambiguity I described last time is still there. My earlier note probably never became a thread because line 53 is an unchanged context line, not part of the diff. This time I've attached it to line 104, which this PR adds: the table branch that already labels automatic entries.
For the human reviewer: someone still needs to decide whether the per-connection probe cost on the common path is acceptable, as I described in my last review. Every RPC state built for a cluster added through discovery runs a verified QUIC handshake first. A fingerprint-only entry runs two, and a successful route isn't remembered (routing.go:28-29, local.go:111-119). The branch hasn't changed anything there.
via_cloud: false with an empty address, and that comment says ViaCloud is the reason an entry has no address. So a script reading cluster list --format json can't tell an automatic entry from a broken one. It's the same ambiguity you fixed in cluster add --format json with the via_cloud/force_cloud split.The fix could mirror that split. Either report the saved override as force_cloud, or keep via_cloud and document it as forced-only. Then add a field or route value for automatic entries, using the condition this branch already uses (Hostname == "" && XID != "" && Identity != ""), and update the comment to match. (I've attached this note here because line 53 isn't part of the diff.)
Verdict: ready
The only change since my last review is history. The branch is now two commits (b792f9a, b58ecbb), and the code matches the 146a65f tree I reviewed before. routing.go, the add-time via_cloud/force_cloud split, the per-address diagnostics and the tests all read the same, and nobody has left a new comment or reply. I still think it's ready for a human, but not ready to merge without one.
Still open: cluster list --format json (cluster.go:51-53, :94). via_cloud still comes straight from ccfg.ViaCloud, so an entry saved by an automatic fallback still reports via_cloud: false, address: "". My inline note on this last round never became a thread on the PR, so I've posted it again below.
For the human reviewer: the per-connection cost, more precisely. RPCOptionsWithName (local.go:111-119) sends every discovery-added cluster with an issuer through routedOptions. That includes entries written by login's auto-configure. The probe runs once per RPC state, not once per command, and the tokenMemo comment in local.go says deploy builds three states. So when direct works, a deploy runs three probe handshakes before its RPC connections. For a fingerprint-only entry that's six handshakes, because each probe first fails verification and then retries with the recovered CA. Nothing caches a successful probe, which is deliberate (the comment at routing.go:28-29). When direct is down, only the first state waits out the 3s probe; later ones use the 30s failure hint. Someone should decide whether that cost on the common path is acceptable. If it isn't, one option is to remember a successful route for the life of the process.
cluster add has ViaCloud: false and an empty Hostname. So cluster list --format json reports via_cloud: false, address: "", but this comment says ViaCloud is why an entry has no address. It's the same ambiguity you already fixed in cluster add --format json. The table cell at line 105 got the "direct when reachable, otherwise via cloud" label, but the JSON didn't. One fix is to mirror the add document: report the saved override as force_cloud (or keep via_cloud and document it as forced-only), and add a field or route value for automatic entries. Then update this comment to match.Verdict: ready
Both of my earlier concerns are fixed in 146a65f, and the TLS rework holds up. I think this is ready for a human, but I'm not recommending it merge without one. I found one smaller output gap the fix didn't reach.
Diagnostics (routing.go): You kept the 30s negative cache on purpose. Each failed address and its error is now logged at debug level, and the reasons are saved in the cache file so a cache hit logs them too. The real-QUIC test checks for the address and the fingerprint-mismatch text on both the miss and the hit. A relay failure on this path still doesn't mention the direct address unless you run with -vv; you've accepted that and the docs say so.
Output contract (cluster_add.go, cluster_add_json.go): via_cloud now reports the route used while adding and force_cloud reports the saved override. So an automatic fallback returns via_cloud: true, force_cloud: false with no address, which matches the documented contract again. The success message and the list table cell describe what the next connection will do, and tests cover both.
Still open, smaller: cluster list --format json (cluster.go:51-53, :87-94). It still sets via_cloud straight from ccfg.ViaCloud. An automatic-fallback entry therefore comes out as via_cloud: false, address: "", while the field comment says ViaCloud is the reason an entry has no address. A script reading the list JSON hits the same ambiguity you fixed in cluster add. Details are in the inline comment.
TLS without InsecureSkipVerify: A fingerprint-only entry dials with an empty RootCAs pool (routing.go:144-147). A certificate that chains to a system root therefore can't pass the first handshake, and the pin is always enforced. If the server doesn't send its CA, the last certificate in UnverifiedCertificates won't match the pin, so the probe fails closed. The expired-certificate case shows the retry still runs Go's normal checks.
For the human reviewer: Every discovery-added cluster with an issuer now gets a probe handshake on each command. That includes entries written by login's auto-configure (login.go:841), not only cluster add. These commands also skip the 1h address cache. A fingerprint-only entry also fails one handshake and retries on every connection, because the recovered CA lives only for that connection and is never saved (the test asserts this on purpose). That's three handshakes before the first RPC. The docs mention it, but someone should decide that cost on the common path is acceptable.
cluster add has ViaCloud: false and an empty Hostname. So cluster list --format json reports via_cloud: false, address: "", while this comment says ViaCloud is why an entry has no address. It's the same ambiguity you fixed in cluster add --format json. The table cell got a label, but the JSON didn't. One option is to mirror the add document: keep via_cloud for the forced route (or rename it to match force_cloud) and add a field or route value that says "direct when reachable, otherwise via cloud" for automatic entries. Then update this comment to match.Posted to the PR's comment threads when you submit.
TestAutomaticClusterRouting checks for the address and the cluster CA fingerprint mismatch text on both the miss and the hit. The docs also say the hint records a failed direct attempt, not a working relay, and point people to -vv. You've decided to keep the default-verbosity behaviour, and I said I'd accept that. Resolving.via_cloud reports the route used while adding and force_cloud reports clusterConfig.ViaCloud. The success message and the cluster list table describe automatic entries as "direct when reachable, otherwise via cloud". cluster_add_json.go and the generated cluster-add.md describe both fields the same way. TestClusterAddJSONAutomaticFallback and TestAddClusterFallbackPreservesAutomaticRouting cover the JSON output, the saved entry and the human-readable output. Resolving. There's a related gap in cluster list --format json, which I've raised separately at cluster.go:53.RootCAs pool (routing.go:144-147), so a certificate that chains to a system root still fails and still goes through the pin check. If the server doesn't include its CA in the chain, the last certificate in UnverifiedCertificates won't match the pin, and the probe fails closed and the command goes through cloud. InsecureSkipVerify is no longer in routing.go. There's no real-QUIC test with a publicly trusted certificate, but the empty pool rules that case out directly in the code. Resolving this thread and dismissing the alert are still up to a maintainer.Verdict: ready
I think this is ready for a human to look at, and it does what the issue asks: the route is now chosen on each connection, failures are cached only briefly, and routing stays forced only for explicit or legacy via_cloud entries. It isn't ready to merge without review, though. It changes the connect path for every cluster added through cloud discovery, not just the ones that used to fall back to cloud. I also found one diagnosability problem and one output-contract problem that I'd want fixed or deliberately accepted first.
Probe errors are thrown away, and a failed probe turns into a sticky cloud route (clientconfig/routing.go:69-87). res.err is never recorded. When every probe fails, the code builds the relay options and writes the 30s hint before anything has shown the relay works: viaCloudOptions only assembles options. Take a direct-only cluster with no working cloud link, such as an older runtime or one whose link is down. cluster add already guards against exactly this case. If that cluster has a brief direct outage, the user waits 3s and gets a relay error that never mentions the direct address. Then every command keeps failing through the relay for 30s after the cluster comes back. Before this PR they got a direct dial error naming the address. routing_test.go:139-142 asserts the stickiness on purpose, but nothing makes the direct failure visible. A fingerprint mismatch, for example after a CA rotation, also falls back to cloud silently, with no log.
Automatic fallback at add time now writes an entry that reports neither route (cluster_add.go:485, :595). The fallback path leaves address empty and now writes ViaCloud: false. So cluster add --format json returns via_cloud: false with no address. The documented contract in cluster_add_json.go says address is absent only for a cloud-routed cluster, so a script can't tell what it got. cluster list (cluster.go:101-103) also shows a blank ADDRESS cell for it. Its own comment says a blank cell reads as a broken entry. The success message at line 574 still says the cluster "needs no address here".
For the human reviewer: every discovery-added cluster whose identity has an issuer now goes through routedOptions (local.go:107-115). That costs one extra verified QUIC handshake per command when direct works. It also bypasses the old 1h cluster_address_* cache and the single-address shortcut, because directRPCOptions is called with an empty name and AllAddresses cleared. The description accepts this, but it lands on the common path, so it deserves a deliberate yes.
The probe's TLS handling looks sound to me. Rebuilding verification from the pinned root checks both the chain and the hostname. The api.miren fallback applies only when that SAN is present. The credential-nondisclosure tests exercise this against a real QUIC server, including a peer signed by the same CA but carrying an unrelated SAN.
viaCloudOptions builds options, but that function never contacts the relay. The probe errors from the loop above are dropped too. So for a cluster that can't be relayed, a short direct outage turns into 30s of relay failures, and none of them mention the direct address. I'd collect the probe errors and include them in any error that comes back from this path. I'd also write the hint only once the relay is known to work, or at least make sure the direct failure isn't hidden. If you keep the current behaviour, logging each probe's error at debug level would still explain why a command went through cloud (including a fingerprint mismatch).address stays empty and ViaCloud is now false. The JSON document therefore reports via_cloud: false with no address, but the contract in cluster_add_json.go says address is absent only for a cloud-routed cluster. cluster list (cluster.go:101-103) also shows a blank ADDRESS cell for this entry, because it only labels ViaCloud entries. The opts.viaCloud message at line 574 ("needs no address here") is wrong for this case as well, since the next connection tries direct first. One option is to report the route actually chosen at add time separately from whether it's forced, and to describe automatic entries as "direct when reachable, otherwise via cloud" in both places.Verdict: ready