Skip to content

fix(ios): race every transport at once and stop replacing healthy connections - #948

Merged
arul28 merged 3 commits into
mainfrom
ade/pr2-transport-selection-v2-ae7b0a8f
Jul 29, 2026
Merged

fix(ios): race every transport at once and stop replacing healthy connections#948
arul28 merged 3 commits into
mainfrom
ade/pr2-transport-selection-v2-ae7b0a8f

Conversation

@arul28

@arul28 arul28 commented Jul 29, 2026

Copy link
Copy Markdown
Owner

iOS connect behaviour was broken in four measured ways. On cellular, relay was only dialed after the direct race burned its full 10s budget, so cold connects stalled 10-20s. The app could not remember that relay had worked (relay URLs were excluded from last-good) and could not remember failures at all, so dead endpoints were re-raced forever. The relay handshake serialized a Clerk token fetch behind the socket open and redialed every cold Durable Object. And while connected over relay on cellular, roaming launched a full connection race every ~30-70s and replaced the healthy connection each time — a captured idle phone made ~20 relay dials in 3.5 minutes, each felt as a 1-2s stream freeze.

This is iOS-only: apps/ios/** plus internal docs. No sync protocol, hello payload, or host-side change.

What changed

One race. Direct and relay candidates now share a single happy-eyeballs plan instead of two sequential races. A relay candidate that does not lead the ranking joins ~300ms behind the leading direct candidate; one that leads (because it is the proven route for this network) is dialed at t=0. The catch-driven "direct routes exhausted; beginning authenticated relay fallback" phase is gone.

Memory of what works. HostConnectionProfile gains networkRouteMemory: an MRU map (cap 8) from a coarse network fingerprint — wired, wifi:<the phone's own IPv4 /24>, cell — to the endpoint that last authenticated there. It is hoisted to the front of the plan and outranks the global lastSuccessfulAddress, which may belong to a different network. An SSID would need an entitlement the app does not hold; the phone's own /24 discriminates home/office/hotspot just as well for this purpose.

Memory of what fails. HostConnectionEndpointState gains lastFailedAt, consecutiveFailures, negotiatedReadyV2. Two consecutive failures inside 120s schedule an endpoint last — never removed, so a route that recovers is still tried on every connect. Success clears the streak. Cancellation, the single-flight refusal, and account/device errors are explicitly not route failures.

Path awareness. With no Wi-Fi or wired interface on the path, RFC1918 / mDNS / IPv6 link-local and unique-local candidates move to the back of the plan — each one raced eagerly costs a 5s socket-open timeout. Tailnet CGNAT and loopback are unaffected, since Tailscale works over cellular.

Relay handshake. The Clerk token is fetched concurrently with the WebSocket open via async let rather than after it. accepted (350ms window) now extends the deadline for ready, instead of the socket being abandoned and redialed without ?ready=2 on principle. That legacy redial now happens only for an endpoint that has never negotiated ready-v2. A token-based single-flight registry prevents two concurrent /connect dials for one machine key, which would otherwise consume the Durable Object's 16-tunnel cap with orphans swept only every 5-10 minutes.

Roaming. Triggers only on a real interface-set change (failover) or a 5-minute upgrade probe toward a strictly better class (lan < tailnet < relay). Being "on cellular with a saved tailnet route" is no longer a standing trigger. Roams are make-before-break, and a race that wins on the endpoint already in use keeps the live socket rather than swapping it.

Self-audit: constants

Constant Before After
SyncConnectionRaceTiming.relayReadyNegotiationNanoseconds 350ms, single window covering accepted and ready renamedrelayAcceptedNegotiationNanoseconds, 350ms, now covers accepted only
SyncConnectionRaceTiming.relayReadyAfterAcceptedNanoseconds — (post-accepted wait was unbounded, capped only by the 10s race budget) new, 7s. Deliberately under overallBudgetNanoseconds so a relay that accepts and never bridges fails inside the race and is recorded as a failing endpoint
SyncConnectionRaceTiming.relayJoinDelayNanoseconds new, 300ms
SyncEndpointFailureMemory.consecutiveFailureThreshold new, 2
SyncEndpointFailureMemory.recentFailureWindowSeconds new, 120s
syncNetworkRouteMemoryCapacity new, 8
SyncRoamTiming.pathChangeCooldownSeconds — (no cooldown existed; 250ms debounce only) new, 3s
SyncRoamTiming.minimumConnectionAgeSeconds new, 10s
SyncRoamTiming.upgradeProbeIntervalSeconds new, 300s
candidateStaggerNanoseconds (250ms), overallBudgetNanoseconds (10s), maximumCandidateCount (3) unchanged unchanged

Deleted: syncShouldRoamToTailnet(...) — the standing-fact predicate that made every path update a roam trigger. Replaced by syncRoamTrigger(SyncRoamInputs). SyncRankedEndpointAttempt.kind changed from Int to SyncConnectionRouteKind (now Comparable, lower is better).

New persisted fields are all optional, so profiles written before this change decode unchanged — covered by testAProfileSavedBeforeFailureMemoryStillDecodes.

Self-audit: connect-path narratives

Cold cellular connect (relay is the only viable route)

  • Before: the direct plan races saved LAN + tailnet candidates. On cellular no direct candidate can succeed, so the race burns its full 10s budget (or 5s per socket-open timeout across 3 concurrent slots) before the catch starts a second race for relay. The relay socket then opens, waits for accepted, and — on a cold Durable Object — usually fails the 350ms ready window, is abandoned, and is redialed without ?ready=2. Only then is the Clerk token fetched, serialized ahead of the hello. Measured: 10-20s.
  • After: ranking puts relay first once it is the remembered winner for cell (and on the very first connect it is at most 300ms behind the leading direct candidate). Saved LAN addresses are demoted to the back of the plan because the path has no Wi-Fi interface. The Clerk token is fetched concurrently with the socket open. accepted arrives in the 350ms window and extends the ready deadline to 7s, so the cold DO completes its wake + host-pipe dial + local-port dial on the first socket. One dial, no fallback phase.

Wi-Fi returns during a relay session

  • Before: nothing moved the connection off relay, ever. There was no roam-toward-LAN direction at all — the only roam target was tailnet, and it fired on the standing cellular condition rather than on the network actually changing.
  • After: the interface set changes (cellular → Wi-Fi), which is a genuine .pathChange trigger, so a full replacement race runs immediately and the LAN candidate wins. If instead the phone was already on Wi-Fi and the machine simply became visible on Bonjour, the discovery callback evaluates an .upgradeProbe: with a connection ≥10s old and ≥300s since the last roam, a quiet race restricted to strictly-better-class candidates runs. Either way the live relay socket keeps serving traffic until the LAN candidate has authenticated, and if the race fails the relay connection is untouched.

Tailscale switched off mid-session

  • Before: ~5s to recover — the path change fired attemptAuthenticatedPathReplacement, which ran a full connectUsingProfile. This is the behaviour that had to be preserved.
  • After: still an immediate full race. The interface set changed, so syncRoamTrigger returns .pathChange, which ignores the connection-age floor and uses the short 3s cooldown rather than the 300s upgrade interval. Failover speed is unchanged; what was removed is the other path, where an unchanged interface set also triggered a race.

Also fixed

A latent auth-gate leak: the slot-0 hoist synthesized an attempt from lastSuccessfulAddress without checking it against the vetted candidate set, so a relay URL could enter the race while signed out (the old code's transport filter happened to mask this in the direct plan but not the relay plan). Hoisting now only reorders candidates that address and relay-authorization policy already accepted.

Testing

SyncTransportSelectionTests (22) + SyncRecoveryPolicyTests (58) cover the ranking with failure memory, the single-race plan and relay offset, fingerprint memory hoisting, the token-based single-flight guard, the accepted-extends-deadline state machine, roam triggers/cooldowns, and profile decode back-compat.

Full iOS suite: 1049 tests. The only failures are the two pre-existing testDeepLinkRouter* cases, which reproduce identically on a clean main checkout and are unrelated to this branch.

🤖 Generated with Claude Code

arul28 and others added 3 commits July 29, 2026 14:06
…nections

iOS connect behaviour had four measured faults: relay was only dialled after
the direct race burned its whole 10s budget (a 10-20s stall on cellular, where
no direct candidate can ever win); the app could not remember that relay had
worked, nor that anything had failed, so dead endpoints were re-raced forever;
the relay handshake serialised a Clerk token fetch behind the socket open and
redialled every cold Durable Object; and roaming ran a full connection race
every path-monitor update, replacing a working socket each time.

- One race. Direct and relay candidates share a single plan; relay holds back
  300ms behind the leading direct candidate, or starts at t=0 when it leads
  the ranking. The direct-then-relay fallback phase is gone.
- Memory. Endpoint state gains a failure streak and per-network route memory
  (`wifi:<own /24>`, `cell`, `wired`), so what worked on this network is dialled
  first. Recently-failing endpoints are scheduled last, never removed.
- Path awareness. RFC1918/mDNS candidates move to the back of the plan when the
  path has no Wi-Fi or wired interface; tailnet and loopback are unaffected.
- Relay handshake. The Clerk token is fetched concurrently with the socket open.
  `accepted` now extends the deadline for `ready` from 350ms to 3.5s, and the
  legacy redial only happens for an endpoint that has never spoken ready-v2.
  A single-flight guard prevents two `/connect` dials for one machine.
- Roaming. Triggers on a real interface-set change, or on a 5-minute probe when
  a strictly better transport class is reachable — never on the standing fact of
  being on cellular. Fast failover on genuine path change is preserved, and a
  race that wins on the endpoint already in use keeps the live socket.

Also fixes a latent auth-gate leak: the slot-0 hoist synthesised an attempt from
`lastSuccessfulAddress`, letting a relay URL into the race while signed out.
Hoisting now only reorders candidates that policy already vetted.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review fixes on the transport-selection change.

Correctness:
- The per-network route memory could never win slot 0: the hoist list put the
  global last-good first, so coming home to a known Wi-Fi still led with the
  relay URL learned on cellular. The feature was inert in the exact case it
  exists for.
- A follow-up path update reporting no interface change cancelled the roam that
  the interface change had just scheduled, silently disabling failover. Roams
  now own a task separate from the scheduled reconnect.
- A single-flight dial key held by a superseded attempt could refuse a
  user-initiated Reconnect outright, with nothing left to retry. The registry is
  now token-based so an attempt can release every key it superseded while a late
  unwind cannot free a key it no longer owns.
- The post-`accepted` `ready` wait was bounded only by the race budget before
  this branch; 3.5s would have been a reduction that fails slow cold Durable
  Objects. It is now 7s — long enough for a cold DO, still inside the race so a
  half-open relay is recorded as a failing endpoint rather than leading forever.
- Failover no longer waits out the upgrade-probe cooldown (3s vs 300s): the
  route it is failing away from may already be gone.
- Connection age is stamped in applyHelloPayload, so pairing- and
  adoption-established connections are eligible for transport upgrades too.
- Account and device-state errors (relay authorization, missing key proof) no
  longer demote a healthy route; only route failures count.
- Project switch retires pending reconnect/roam work before superseding the
  attempt generation, so no second tunnel opens on the same Durable Object.
- IPv6 unique-local and link-local candidates are deprioritized on cellular too.

Shape:
- Replacement is an optional struct, not an enum whose case identity was unused.
- Route kinds are Comparable (lower is better) instead of comparing rawValues.
- Race failures are tagged with their connect generation instead of sharing one
  buffer across overlapping attempts.
- Unknown networks are `nil`, not a "none" sentinel string.
- Both roam entry points share one scheduler; discovery publishing no longer
  hides a connection race behind a name that says it publishes hosts.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…gration

- Move the roam-decision tests next to the policy they exercise: syncRoamTrigger
  and syncNetworkPathInterfacesChanged live in SyncRecoveryPolicy.swift, and
  syncNetworkPathRecoveryAction was already tested there.
- Make testRelayCandidateRuntimeIgnoresReadyBeforeAccepted discriminating. It
  asserted nothing, so a runtime that wrongly honored the leading `ready` frame
  would have passed it too; awaitRelayCandidateReady now returns its negotiation
  and the test asserts `accepted` was actually observed.
- Add the missing persistence contract test: a HostConnectionProfile written
  before failure/route memory existed must still decode. Every paired machine on
  an upgrading device is in that shape, so a decode failure would silently
  unpair them.
- Document the new connect path in docs/features/sync-and-multi-device/ —
  relay is no longer a phase that runs after direct routes exhaust.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@vercel

vercel Bot commented Jul 29, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
ade Ignored Ignored Jul 29, 2026 7:02pm

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@arul28, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 12 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 22f8b8d4-0da4-4116-8f4d-517b0bc3ee11

📥 Commits

Reviewing files that changed from the base of the PR and between cd1bb24 and b8cac17.

⛔ Files ignored due to path filters (4)
  • apps/ios/ADE.xcodeproj/project.pbxproj is excluded by !**/*.xcodeproj/project.pbxproj
  • docs/ARCHITECTURE.md is excluded by !docs/**
  • docs/features/sync-and-multi-device/README.md is excluded by !docs/**
  • docs/features/sync-and-multi-device/ios-companion.md is excluded by !docs/**
📒 Files selected for processing (7)
  • apps/ios/ADE/Models/RemoteModels.swift
  • apps/ios/ADE/Services/SyncConnectionRace.swift
  • apps/ios/ADE/Services/SyncRecoveryPolicy.swift
  • apps/ios/ADE/Services/SyncService.swift
  • apps/ios/ADETests/ADETests.swift
  • apps/ios/ADETests/SyncRecoveryPolicyTests.swift
  • apps/ios/ADETests/SyncTransportSelectionTests.swift

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

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

@arul28 arul28 changed the title pr2-transport-selection-v2 -> Primary fix(ios): race every transport at once and stop replacing healthy connections Jul 29, 2026
@arul28
arul28 merged commit 308f75c into main Jul 29, 2026
3 checks passed
@arul28
arul28 deleted the ade/pr2-transport-selection-v2-ae7b0a8f branch July 29, 2026 19:09
arul28 added a commit that referenced this pull request Jul 29, 2026
…ilure

Account adoption walked [lan, tailnet, relay] strictly in order, paying a
socket open plus a 3s identity challenge per route. On cellular, where no
direct route can ever succeed, that is 10-20s of dead time before relay
is even dialed. It now races every route at once through the same
machinery the paired reconnect path uses (#948): one plan, relay joining
behind the leading direct candidate rather than after it, one 10s
budget. Each candidate owns its socket and mailbox, so only the winner
touches app state; the challenge crypto is unchanged.

Any adoption failure also latched `autoReconnectPausedByUser`, which is
persisted and blocks reconnecting forever -- so one bad connect left the
app opening on a dead "Disconnected" screen for good. It is now written
only by an explicit user action, which stamps a source alongside it; a
paused flag with no source is provably fallout from an older build and
is cleared once.

Also:
- A host naming an adoption cipher this build does not implement now
  fails that route instead of aborting the attempt with an identity
  verification error. The cipher is still never used.
- Persist a PIN pairing secret before the hello, so a lost hello_ok
  cannot strand the phone on a secret the Mac already retired.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
arul28 added a commit that referenced this pull request Jul 29, 2026
…ilure

Account adoption walked [lan, tailnet, relay] strictly in order, paying a
socket open plus a 3s identity challenge per route. On cellular, where no
direct route can ever succeed, that is 10-20s of dead time before relay
is even dialed. It now races every route at once through the same
machinery the paired reconnect path uses (#948): one plan, relay joining
behind the leading direct candidate rather than after it, one 10s
budget. Each candidate owns its socket and mailbox, so only the winner
touches app state; the challenge crypto is unchanged.

Any adoption failure also latched `autoReconnectPausedByUser`, which is
persisted and blocks reconnecting forever -- so one bad connect left the
app opening on a dead "Disconnected" screen for good. It is now written
only by an explicit user action, which stamps a source alongside it; a
paused flag with no source is provably fallout from an older build and
is cleared once.

Also:
- A host naming an adoption cipher this build does not implement now
  fails that route instead of aborting the attempt with an identity
  verification error. The cipher is still never used.
- Persist a PIN pairing secret before the hello, so a lost hello_ok
  cannot strand the phone on a secret the Mac already retired.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
arul28 added a commit that referenced this pull request Jul 29, 2026
* fix(sync): stage PIN re-pairs so a dropped retry can't brick a pairing

A re-pair overwrote the device's working secret the moment the host
answered, two round trips before the device could persist the reply. A
drop in that gap left the host holding credentials the phone never saw,
and the only way back was another PIN typed at the Mac -- so on a flaky
network, retrying was what broke you.

A re-pair now stages: the committed secret stays live and the
replacement waits in a pending slot for 10 minutes, promoted by the
first hello that authenticates with it. Proof of possession IS the
commit, so no new wire message is needed and clients that predate this
get the protection unchanged. `pairing_result` carries an advisory
`rotation` field for diagnosis only. Only PIN pairing stages; account
adoption returns its secret inside hello_ok with no acknowledged
follow-up, and the local OS/SSH path hands it back in-process.

Also:
- Bucket failed PIN attempts by device as well as address, and raise the
  global breaker to 25. Five fumbled PINs on one phone used to block
  pairing and account adoption for every device in the house.
- Give each of the collapsed hello rejections its own actionable
  message; "Sync authentication failed." covered fourteen distinct
  causes with fourteen different fixes.
- Fold two copies of the pair-failure limiter into one shared tracker.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(ios): race adoption routes and stop latching auto-reconnect on failure

Account adoption walked [lan, tailnet, relay] strictly in order, paying a
socket open plus a 3s identity challenge per route. On cellular, where no
direct route can ever succeed, that is 10-20s of dead time before relay
is even dialed. It now races every route at once through the same
machinery the paired reconnect path uses (#948): one plan, relay joining
behind the leading direct candidate rather than after it, one 10s
budget. Each candidate owns its socket and mailbox, so only the winner
touches app state; the challenge crypto is unchanged.

Any adoption failure also latched `autoReconnectPausedByUser`, which is
persisted and blocks reconnecting forever -- so one bad connect left the
app opening on a dead "Disconnected" screen for good. It is now written
only by an explicit user action, which stamps a source alongside it; a
paused flag with no source is provably fallout from an older build and
is cleared once.

Also:
- A host naming an adoption cipher this build does not implement now
  fails that route instead of aborting the attempt with an identity
  verification error. The cipher is still never used.
- Persist a PIN pairing secret before the hello, so a lost hello_ok
  cannot strand the phone on a secret the Mac already retired.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(sync): acknowledge staged pairing rotations

Negotiate explicit pairing commits for clients that persist replacement credentials before hello. Keep legacy hello-as-commit behavior for older clients and bind acknowledgements to the exact staged secret so stale sockets cannot promote a newer rotation.

Extend host coverage for flaky re-pairs, commit expiry and races, per-device limiting, and actionable authentication errors. Add pure iOS coverage for persistence ordering and commit negotiation without running an iOS build.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(sync): loosen PIN pairing limits for onboarding

Ten tries and a two-minute cooldown per device instead of five tries and
ten minutes. Fumbling a 6-digit code while setting up a phone is the
common case, not an attack; a two-minute wait still caps a LAN brute
force at roughly seven codes a minute against a million-code space.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(sync): let a rename land before the rotation commits

Two suites disagreed about a staged re-pair. The desktop copy asserted a
renamed device shows its new name straight away; the new ade-cli coverage
asserted the whole staged record stays hidden. Splitting them by what is
actually a credential settles it: the secret, its DPoP binding, runtime-host
grant, and account ownership wait for the acknowledgement, while peerName,
platform, and device type apply immediately. A user who renames a phone
should not wait on a handshake they cannot see.

Also key the two PIN-limit loops off PAIR_FAILURE_THRESHOLD instead of
hardcoding the old value, so retuning the threshold cannot silently break
them again.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(sync): apply a staged re-pair's privilege reductions immediately

Staging protected more than the secret. A PIN re-pair is supposed to
declassify an account-owned pairing, but staging that left the committed
record account-owned, so the next account switch revoked the pairing the
user had just re-established at the Mac. A withdrawn runtime-host grant
had the same shape: authority the re-pair removed stayed readable until an
acknowledgement that may never arrive.

Split by direction instead of by field: elevations wait for proof,
reductions land at once. Costs at most one re-grant; the alternative is a
privilege leak and a destroyed pairing.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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.

1 participant