Preserve live peer sessions across equivalent route revision bumps - #9342
Merged
Merged
Conversation
Two regressions captured from foreground telemetry on build 20260801001626: 1. equivalentRouteRevisionBumpKeepsTheLivePeerSession: a broker connectivity sync that bumps the account route revision without changing the peer's material route content (only last_seen_at moved) tears down the live admitted session with runtimeReconfigured. 2. concurrentRedialCannotDisplaceAnInstalledLiveSession: two concurrent connectedSession callers can both pass the installed-slot check across the dead-on-arrival probe suspension, so the second install displaces the first admitted session without closing it and records a second established lifecycle event. Both tests fail on current code; the fix lands in the next commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The connectivity engine tore down every peer session whenever the account route revision changed, even when the peer's route content was identical. Broker registration heartbeats bump the revision while only moving last_seen_at and path-hint freshness, so a foreground iOS client lost its live control session every 10-90 seconds to a runtimeReconfigured close followed by a full rediscover-dial-pair cycle. The engine now derives CmxConnectivityRouteContent from each installed snapshot: per-peer admission material (binding id, app instance, tag, platform, identity generation, pairing flag, capabilities) plus account-wide trust material (relay fleet, LAN rendezvous, grant verification keys). On a revision change it invalidates only peers whose material content differs. A changed endpoint identity keys the peer out of the new content, a removed binding leaves it unrouted, and any account-material change tears down all peers, so every security-relevant change still invalidates. A missing baseline or a revision bump without a replacement snapshot fails closed and keeps the old invalidate-all behavior. Also close the double-establish race in CmxConnectivityPeerSession: the dead-on-arrival probe suspends the actor between clearing the pending dial and installing it, so a concurrent caller could install its own dial in that window and the late installer silently displaced the live session while double-recording an established lifecycle. The installer now rechecks the installed slot after the probe and adopts the winner, closing its own redundant session. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
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. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxConnectivityRouteContent.swift">
<violation number="1" location="Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxConnectivityRouteContent.swift:33">
P2: A capability-order-only revision will invalidate the live peer session even though the advertised capability set is unchanged. Store a canonical ordering here so route-content equality matches the set semantics used by the admission policy.</violation>
<violation number="2" location="Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxConnectivityRouteContent.swift:42">
P2: A revision that only reorders the same relay fleet will be treated as an account-material change, causing unnecessary teardown of all live sessions. Canonicalize the fleet order (or compare it as a set) when constructing route content so equivalent snapshots remain equivalent.</violation>
<violation number="3" location="Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxConnectivityRouteContent.swift:44">
P2: A revision that only reorders equivalent grant verification keys will be treated as changed account trust material and tear down all live sessions. Canonicalize the key array by `kid` (or compare key sets order-independently) when building route content.</violation>
</file>
<file name="Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohHostRuntime+PublicAPI.swift">
<violation number="1" location="Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohHostRuntime+PublicAPI.swift:153">
P2: Overlapping host reconciliations can install an older discovery snapshot after a newer one because this call forwards the fetched revision without a monotonic check. That can roll back the engine's installed route revision and tear down or retain sessions based on stale route content; an engine-level monotonic install/coalescing guard would keep older completions from winning.</violation>
</file>
<file name="Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxConnectivityPeerSession.swift">
<violation number="1" location="Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxConnectivityPeerSession.swift:207">
P2: A concurrent invalidation, remote close, or replacement dial can run while the redundant session is being closed, but this returns the previously captured `installed.session` without revalidating the slot. Re-read the active ID (and liveness) after the await and retry the dial when the winner changed, otherwise callers can receive a closed session.</violation>
</file>
<file name="Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxConnectivityEngine.swift">
<violation number="1" location="Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxConnectivityEngine.swift:253">
P2: A live peer can survive the first snapshot installed after the engine recorded a revision without route content. `performRouteSync` can leave `routeContent` nil for an unchanged response, but this same-revision branch skips the documented fail-closed invalidation; compare the prior content and invalidate, or invalidate all when the baseline is missing, before storing it.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| platform = binding.platform | ||
| identityGeneration = binding.identityGeneration | ||
| pairingEnabled = binding.pairingEnabled | ||
| capabilities = binding.capabilities |
There was a problem hiding this comment.
P2: A capability-order-only revision will invalidate the live peer session even though the advertised capability set is unchanged. Store a canonical ordering here so route-content equality matches the set semantics used by the admission policy.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxConnectivityRouteContent.swift, line 33:
<comment>A capability-order-only revision will invalidate the live peer session even though the advertised capability set is unchanged. Store a canonical ordering here so route-content equality matches the set semantics used by the admission policy.</comment>
<file context>
@@ -0,0 +1,65 @@
+ platform = binding.platform
+ identityGeneration = binding.identityGeneration
+ pairingEnabled = binding.pairingEnabled
+ capabilities = binding.capabilities
+ }
+ }
</file context>
| account = AccountMaterial( | ||
| relayFleet: snapshot.relayFleet, | ||
| lanRendezvous: snapshot.lanRendezvous, | ||
| grantVerificationKeys: snapshot.grantVerificationKeys |
There was a problem hiding this comment.
P2: A revision that only reorders equivalent grant verification keys will be treated as changed account trust material and tear down all live sessions. Canonicalize the key array by kid (or compare key sets order-independently) when building route content.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxConnectivityRouteContent.swift, line 44:
<comment>A revision that only reorders equivalent grant verification keys will be treated as changed account trust material and tear down all live sessions. Canonicalize the key array by `kid` (or compare key sets order-independently) when building route content.</comment>
<file context>
@@ -0,0 +1,65 @@
+ account = AccountMaterial(
+ relayFleet: snapshot.relayFleet,
+ lanRendezvous: snapshot.lanRendezvous,
+ grantVerificationKeys: snapshot.grantVerificationKeys
+ )
+ var routes: [CmxConnectivityPeerID: [BindingMaterial]] = [:]
</file context>
|
|
||
| init(snapshot: CmxIrohDiscoveryResponse) { | ||
| account = AccountMaterial( | ||
| relayFleet: snapshot.relayFleet, |
There was a problem hiding this comment.
P2: A revision that only reorders the same relay fleet will be treated as an account-material change, causing unnecessary teardown of all live sessions. Canonicalize the fleet order (or compare it as a set) when constructing route content so equivalent snapshots remain equivalent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxConnectivityRouteContent.swift, line 42:
<comment>A revision that only reorders the same relay fleet will be treated as an account-material change, causing unnecessary teardown of all live sessions. Canonicalize the fleet order (or compare it as a set) when constructing route content so equivalent snapshots remain equivalent.</comment>
<file context>
@@ -0,0 +1,65 @@
+
+ init(snapshot: CmxIrohDiscoveryResponse) {
+ account = AccountMaterial(
+ relayFleet: snapshot.relayFleet,
+ lanRendezvous: snapshot.lanRendezvous,
+ grantVerificationKeys: snapshot.grantVerificationKeys
</file context>
Suggested change
| relayFleet: snapshot.relayFleet, | |
| relayFleet: snapshot.relayFleet.sorted(), |
| try requireCurrent(revision) | ||
| await connectivityEngine.didInstallRouteRevision(discoveredRevision) | ||
| await connectivityEngine.didInstallRouteRevision( | ||
| discoveredRevision, |
There was a problem hiding this comment.
P2: Overlapping host reconciliations can install an older discovery snapshot after a newer one because this call forwards the fetched revision without a monotonic check. That can roll back the engine's installed route revision and tear down or retain sessions based on stale route content; an engine-level monotonic install/coalescing guard would keep older completions from winning.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohHostRuntime+PublicAPI.swift, line 153:
<comment>Overlapping host reconciliations can install an older discovery snapshot after a newer one because this call forwards the fetched revision without a monotonic check. That can roll back the engine's installed route revision and tear down or retain sessions based on stale route content; an engine-level monotonic install/coalescing guard would keep older completions from winning.</comment>
<file context>
@@ -149,7 +149,10 @@ extension CmxIrohHostRuntime {
try requireCurrent(revision)
- await connectivityEngine.didInstallRouteRevision(discoveredRevision)
+ await connectivityEngine.didInstallRouteRevision(
+ discoveredRevision,
+ routes: discovery
+ )
</file context>
| // caller that dialed in that window may have installed first; | ||
| // installing over it would leak its session and double-record | ||
| // an established lifecycle for the same peer. | ||
| if let installed = activeConnection { |
There was a problem hiding this comment.
P2: A concurrent invalidation, remote close, or replacement dial can run while the redundant session is being closed, but this returns the previously captured installed.session without revalidating the slot. Re-read the active ID (and liveness) after the await and retry the dial when the winner changed, otherwise callers can receive a closed session.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxConnectivityPeerSession.swift, line 207:
<comment>A concurrent invalidation, remote close, or replacement dial can run while the redundant session is being closed, but this returns the previously captured `installed.session` without revalidating the slot. Re-read the active ID (and liveness) after the await and retry the dial when the winner changed, otherwise callers can receive a closed session.</comment>
<file context>
@@ -200,6 +200,16 @@ actor CmxConnectivityPeerSession {
+ // caller that dialed in that window may have installed first;
+ // installing over it would leak its session and double-record
+ // an established lifecycle for the same peer.
+ if let installed = activeConnection {
+ if installed.id != pending.id {
+ await connected.close()
</file context>
Comment on lines
+253
to
+256
| guard routeRevision != revision else { | ||
| routeContent = content | ||
| return | ||
| } |
There was a problem hiding this comment.
P2: A live peer can survive the first snapshot installed after the engine recorded a revision without route content. performRouteSync can leave routeContent nil for an unchanged response, but this same-revision branch skips the documented fail-closed invalidation; compare the prior content and invalidate, or invalidate all when the baseline is missing, before storing it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxConnectivityEngine.swift, line 253:
<comment>A live peer can survive the first snapshot installed after the engine recorded a revision without route content. `performRouteSync` can leave `routeContent` nil for an unchanged response, but this same-revision branch skips the documented fail-closed invalidation; compare the prior content and invalidate, or invalidate all when the baseline is missing, before storing it.</comment>
<file context>
@@ -240,10 +241,22 @@ public actor CmxConnectivityEngine {
+ routes: CmxIrohDiscoveryResponse
+ ) async {
+ let content = CmxConnectivityRouteContent(snapshot: routes)
+ guard routeRevision != revision else {
+ routeContent = content
+ return
</file context>
Suggested change
| guard routeRevision != revision else { | |
| routeContent = content | |
| return | |
| } | |
| guard routeRevision != revision else { | |
| if let previous = routeContent { | |
| if previous != content { | |
| await invalidatePeersSuperseded(by: content) | |
| } | |
| } else { | |
| await invalidateAllPeers(failure: .superseded) | |
| } | |
| routeContent = content | |
| return | |
| } |
azooz2003-bit
added a commit
that referenced
this pull request
Aug 3, 2026
… and install races (#9402) * Add failing tests for route-content equivalence hardening Pins six behaviors from the cubic review of #9342: reorder-only capability, relay fleet, and grant verification key revisions keep live sessions; a snapshot installed for a revision recorded without content fails closed; an older route revision install cannot roll back a newer one; a redundant-dial close raced by invalidation redials instead of returning the closed winner. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Harden route-content equivalence against reorders and races Canonicalizes route content so order carries no meaning where the admission policy reads sets: binding capabilities, the relay fleet, and grant verification keys (by kid) are sorted when the content is built, so reorder-only revision bumps keep live sessions. didInstallRouteRevision now drops installs older than the recorded revision, so an older completion of an overlapping reconciliation cannot roll back a newer installed revision. The same-revision branch compares the stored baseline and fails closed through the standard superseded-peer invalidation when the baseline is missing or differs, instead of silently adopting the content. The peer session no longer returns a stale winner capture after the redundant-dial close: settleRedundantDial re-reads the active slot and its liveness after the close suspension and redials when the winner was invalidated, replaced, or remotely closed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
cmuxdiag from build 20260801001626 (post connectivity v2) shows the live foreground-control session being closed every 10-90 seconds while the app is in the foreground:
transportSessionLifecycleruntimeReconfigured(raw 9) followed bysessionClosedwith failuresuperseded(raw 19), then the render event stream ends and a full discover, dial, pair cycle reconnects in about 250ms-1s. One ~9 minute capture contains 24+ of these teardowns, and each one is a visible terminal interruption. The same capture also shows pairs of fgControlestablishedevents ~50µs apart (a double dial) with the loser discarded.Root cause
CmxConnectivityEngineinvalidated every peer session whenever the account route revision changed (didInstallRouteRevisionand the tail ofperformRouteSync). The broker bumps the account revision on every registration heartbeat, but a heartbeat only moveslast_seen_atand path-hint freshness; the peer's actual route is unchanged. So each periodic registration refresh or connectivity sync tore down a healthy admitted session for nothing.The double establish is a separate race in
CmxConnectivityPeerSession.connectedSession: the dead-on-arrival probe (await connected.isClosed()) suspends the actor between clearing the pending dial and installing it. A concurrent caller could start and complete its own dial in that window; the late installer then overwrote the installed slot without closing the displaced session and recorded a secondestablishedlifecycle event.Fix
The engine now derives
CmxConnectivityRouteContentfrom every installed discovery snapshot: per-peer admission material (binding id, app instance, tag, platform, identity generation, pairing flag, capabilities) plus account-wide trust material (relay fleet, LAN rendezvous, grant verification keys). On a revision change it invalidates only peers whose material content differs, and records the new revision without teardown for peers with equivalent routes. Volatile freshness fields (last_seen_at, path hints, direct ports, display name) are excluded because they only shape the next dial.Security invariants are unchanged: a changed endpoint identity keys the peer out of the new content, a removed binding leaves it unrouted, and any account-material change (relay fleet, grant keys, LAN rendezvous) still tears down all peers. A missing baseline or a revision bump without a replacement snapshot fails closed with the old invalidate-all behavior.
The peer session installer now rechecks the installed slot after the dead-on-arrival probe and adopts the already-installed winner, closing its own redundant session instead of displacing a live one.
Tests
Two-commit red/green: 29d362c adds the failing regression tests only, the second commit adds the fix.
CmxConnectivityEngineTests/equivalentRouteRevisionBumpKeepsTheLivePeerSession(red on main)CmxConnectivityPeerSessionTests/concurrentRedialCannotDisplaceAnInstalledLiveSession(red on main)changedIdentityGenerationOnRevisionBumpStillInvalidatesTheSession,removedPeerBindingOnRevisionBumpStillInvalidatesTheSession,changedRelayFleetOnRevisionBumpStillInvalidatesTheSession,revisionBumpWithoutReplacementContentFailsClosed,installedRouteRevisionUsesRouteContentEquivalenceswift test --package-path Packages/Shared/CmuxIrohTransport: 531 tests in 62 suites, all green.Still needs dogfood
runtimeReconfiguredteardown loop is gone from cmuxdiag during a multi-minute foreground session.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Preserves live peer sessions across route revision bumps that don’t change material route content, and fixes a concurrent redial race that could displace a live session. This stops foreground reconnect loops and prevents duplicate “established” events.
Written for commit baa0c49. Summary will update on new commits.