Sign up
View stargazersSign up

fix(client-runtime): preserve healthy sessions when better routes appear#24849

closed2
main
GitHub fork: fix/24516-route-upgrade-flapping
opened4d agobyZZheruel
5 files changed
View on GitHubTwo-way sync enabledPR last imported 1d ago

Keep healthy environment sessions on their current route. Route ranking and learned routes still apply on the next necessary reconnect, across web, desktop, and mobile.

A LAN discovery/auth preflight can succeed while its WebSocket upgrade or initial config fails. Currently, that preflight closes the working Tailscale socket, producing a new fallback connection and resync each time the route cooldown expires. Remove speculative upgrade checks and their preference/cooldown state, preserving one session owner and the existing reconnect, probe, and cleanup paths.

Verified locally:

  • Both deterministic regressions fail on unchanged main (e57c093cdd). The fix retains the same session through healthy wakeups, network notifications, explicit retry, and twelve virtual minutes.
  • Isolated protocol QA and before/after evidence use the production supervisor, driver, RPC session, and durable subscription code with real loopback HTTP/WebSockets. Upgrade failure and config failure retain one stream without duplicate snapshots. Real socket loss selects working LAN; long resume reconnects; disconnect and disposal close stalled setup sockets. All scenarios finish with zero peers and at most one concurrent socket.
  • vp run check:changed and 111 focused tests pass.

Authorization is synthetic. Physical iPhone/Tailscale, packaged desktop, and DPoP expiry were not exercised. No live state or credentials were used.

Important

Healthy relay and direct sessions wait until a necessary reconnect to move to a faster route. This trades immediate route optimization for connection stability without parallel socket ownership.

Closes #24516.

Implemented and reviewed with GPT-6.1 Sol in the Codex harness, running in T3 Code.

Checks

6 successful · 7 skipped
All checks passed
3a110af

Added 2 commits

  • fix(client-runtime): preserve healthy sessions when better routes appear6c88073
  • test(client-runtime): focus route stability regressions3a110af
CcoderabbitaiBot4d ago
Synced from GitHub

Review in Change Stack →

No actionable comments were generated in the recent review.

External image Recent review info
External image Run configuration
External image Commits

Reviewing files that changed from the base of the PR and between e57c093cdd9032355c6251dd7192280b915b7a23 and 3a110af02bb584c83df26138e1ef79e56dd76c4b.

External image Files selected for processing: 5
  • docs/internals/connection-runtime.md
  • packages/client-runtime/src/connection/driver.ts
  • packages/client-runtime/src/connection/registry.test.ts
  • packages/client-runtime/src/connection/supervisor.test.ts
  • packages/client-runtime/src/connection/supervisor.ts
External image Files with no reviewable changes: 1
  • packages/client-runtime/src/connection/registry.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

Walkthrough

The connection supervisor no longer probes for better routes or switches a healthy session when another route becomes available. Necessary connection attempts use the current catalog entry, including routes learned during the session.

Changes: Connection route stability

Layer / File(s)Summary
Remove the driver preflight API
packages/client-runtime/src/connection/driver.ts, packages/client-runtime/src/connection/registry.test.ts		The driver service no longer exposes preflight. Its test stub now provides checkRoute without a preflight callback.
Retain sessions until a necessary reconnect
packages/client-runtime/src/connection/supervisor.ts, packages/client-runtime/src/connection/supervisor.test.ts, docs/internals/connection-runtime.md		The supervisor removes better-route checks, preferred-route retries, and automatic route switching. Tests verify that healthy sessions remain active and that newly reachable or learned routes are used on a necessary reconnect. The documentation describes the updated route behavior.

Priority: External image Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Possibly related issues

  • pingdotgg/t3code#16418 — The changes remove some wakeup-triggered checks while retaining explicit reconnect wakeups. This overlaps with the issue’s concern about reconnect handling after mobile app resume.

Suggested reviewers: t3dotgg

Merge Risk: External image Minimal · up to 3a110

Healthy sessions remain on their current route until a necessary reconnect, as intended. No actionable merge-blocking issue is established.

Pre-merge checks: Passed 4
Check nameStatusExplanation
Title checkPassedThe title clearly identifies the client-runtime fix and its primary behavior: preserving healthy sessions when better routes appear.
Description checkPassedThe description explains the problem, the implementation, scope, verification results, limitations, linked issue, and behavior tradeoff. It is complete enough for review, although it does not use the …
Linked Issues checkPassedIssue [#24516] requires that a healthy session does not flap when a better route only passes discovery. The supervisor removes speculative better-route checks, BetterRouteAvailable, route preference…
Out of Scope Changes checkPassedThe changed driver contract, supervisor logic, route tests, and connection-runtime documentation directly support [#24516]. The driver removes the preflight API that enabled speculative switching. The…
Full details: Description check

Explanation

The description explains the problem, the implementation, scope, verification results, limitations, linked issue, and behavior tradeoff. It is complete enough for review, although it does not use the template's section headings or provide explicit maintainer approval evidence.

Full details: Linked Issues check

Explanation

Issue [#24516] requires that a healthy session does not flap when a better route only passes discovery. The supervisor removes speculative better-route checks, BetterRouteAvailable, route preference state, and route cooldown state. It keeps the active session until an existing reconnect path is necessary. Each new attempt reads the current catalog, so ranked and learned routes still apply. The added tests cover LAN failure during opening and synchronization, wakeups, network changes, explicit retry, a 12-minute interval, cleanup, and the next necessary connection.

Full details: Out of Scope Changes check

Explanation

The changed driver contract, supervisor logic, route tests, and connection-runtime documentation directly support [#24516]. The driver removes the preflight API that enabled speculative switching. The tests verify session retention and route selection after reconnect. No unrelated product behavior is demonstrated.

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

Mmaria3d ago

Note

Written by gpt-6.1-sol on behalf of Maria

thanks @Zheruel. your fix correctly keeps the healthy socket owned by its current session, removes speculative upgrade state, and uses learned routes on the next necessary reconnect.

maintainer edits are disabled on your fork, so i rebuilt it from current main in #25526 instead of updating your branch. the replacement covers the same route policy, opening/config failure regressions, cleanup, and learned-route reuse. i compared your full diff against the replacement: no behavior, tests, or documentation were dropped or moved. your release credit is retained in the commit trailer.

both regressions failed on current main before the fix. blacksmith passed 111 focused tests and vp run check:changed, including lint, knip, and seven affected package typechecks. chrome reproduced the working fallback socket being replaced after a foreground wakeup on main; matching fixed-branch evidence is being completed on the replacement. physical tailscale/lan and native clients remain unverified. maintainer review of the route-policy tradeoff is still required. the original pr remains open.

Ttheo1d ago

Note

🤖 Claude Opus 5.5 responding on behalf of Theo

Superseded by #26757, which folds in the route rule (same diff as #25529). You are credited with a Co-authored-by trailer. Closing this so the two don't conflict.