Repository navigation
Keep Iroh reconnects alive through control-plane outages - #8781
azooz2003-bit wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughChangesThe PR adds verified cached-policy fallback for eligible broker refresh failures and reworks stored-Mac reconnect deadline handling. It exposes the shared timeout utility, centralizes reconnect timeout settlement, controls retry state and secondary dials, and adds regression coverage. Iroh policy cache fallback
Stored-Mac reconnect orchestration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CmxIrohHostRuntime
participant TrustBroker
participant VerifiedPolicyCache
CmxIrohHostRuntime->>TrustBroker: preflight discovery or register
TrustBroker-->>CmxIrohHostRuntime: eligible refresh failure
CmxIrohHostRuntime->>VerifiedPolicyCache: request cached verified policy
VerifiedPolicyCache-->>CmxIrohHostRuntime: cached binding
CmxIrohHostRuntime-->>CmxIrohHostRuntime: activate with cached policy
sequenceDiagram
participant MobileShellComposite
participant RPCTaskTimeout
participant StoredMac
participant ReconnectState
MobileShellComposite->>RPCTaskTimeout: enforce reconnect deadline
RPCTaskTimeout->>StoredMac: await restore and dial
RPCTaskTimeout-->>MobileShellComposite: outcome or deadline timeout
MobileShellComposite->>ReconnectState: settle reconnect and record backoff
Possibly related PRs
🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR hardens Iroh reconnect reliability across two dimensions: the host-side policy layer now falls back to its last verified cached relay policy when broker preflight fails with transient errors (rate limits, cooldowns, 503s) instead of propagating the error and stalling the runtime; and the mobile stored-Mac reconnect deadline is moved from a per-trigger call site in
Confidence Score: 5/5Safe to merge — no correctness regressions found across the broker fallback, deadline centralization, or race implementation changes. The broker preflight catch routes transient errors through the established cachedPolicy path guarded by preservesVerifiedPolicyDuringRefresh and is covered by two new lifecycle tests. Moving the deadline into reconnectActiveMacOutcome is a net simplification with every caller sharing the same ceiling and abandoned-dial accounting. The NSLock-based RaceContinuationOnce is replaced by the cleaner RPCTaskTimeout actor settlement. No new global state, blocking primitives, or test seams in production source. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller as recoverMobileConnection / retryActiveMacReconnect
participant Entry as reconnectActiveMacOutcome
participant Race as raceAgainstDeadline (RPCTaskTimeout)
participant Impl as performReconnectActiveMacAttempt
participant Broker as CmxIrohTrustBroker
Caller->>Entry: reconnectActiveMacOutcome(...)
Entry->>Entry: claim generation, start restoringDeadline
Entry->>Race: raceAgainstDeadline(30 s)
Race->>Impl: performReconnectActiveMacAttempt(generation)
Impl->>Broker: preflight / register / discover
alt Broker OK
Broker-->>Impl: policy
Impl-->>Race: .connected / .failed
Race-->>Entry: outcome (deadline still open)
Entry-->>Caller: StoredMacReconnectOutcome
else Preflight transient error (rate-limit, cooldown, 503)
Broker-->>Impl: error
Note over Impl,Broker: preservesVerifiedPolicyDuringRefresh to cachedPolicy
Impl-->>Race: .connected (cached binding)
Race-->>Entry: outcome
Entry-->>Caller: StoredMacReconnectOutcome
else Hard deadline fires
Race-->>Entry: nil (abandoned task tracked)
Entry->>Entry: finishStoredMacReconnectAttempt + recordTransientBackoff
Entry-->>Caller: .failed(.timedOut)
end
Reviews (3): Last reviewed commit: "fix(ios): signal reconnect deadlines wit..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| z.string().min(64).max(16_384), | ||
| ), | ||
| CMUX_RELAY_TOKEN_RATE_LIMIT_ID: requireVercelRelayValue(), | ||
| CMUX_RELAY_TOKEN_RATE_LIMIT_ID: z.string().min(1).optional(), |
There was a problem hiding this comment.
Making CMUX_RELAY_TOKEN_RATE_LIMIT_ID globally optional changes the failure mode from a startup crash (hard to miss) to a running deployment that silently issues relay tokens without any rate limit. The runtime already handles a missing rule ID gracefully (Effect.void), so this is an intentional trade-off — but it removes the "fail fast" safety net that would alert an operator who accidentally deletes or forgets the variable. A misconfigured production deployment will serve unlimited relay tokens with no visible signal until traffic patterns or billing anomalies surface the gap. Consider keeping the env-level requirement and instead catching the not-found sentinel before the process starts, or at minimum emitting a structured startup warning when CMUX_RELAY_TOKEN_RATE_LIMIT_ID is absent on a live Vercel deployment.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
bcd3aae to
eff3aa4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 1890-1894: Update the abandoned reconnect ceiling check in the
timeout handling flow to use a strict less-than comparison, accounting for the
increment performed by registerAbandonedReconnectDial before this check.
Preserve the existing accountID fallback and
recordTransientAutomaticReconnectBackoff behavior while ensuring no retry is
scheduled when abandonedReconnectDialCount equals
maximumAbandonedReconnectDials.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: af2be0d4-36c3-4e71-8399-6703a0848191
📒 Files selected for processing (9)
Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohHostRuntime+PolicyRefresh.swiftPackages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohHostRuntime.swiftPackages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohHostRuntimeLifecycleTests.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ConnectionRecovery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/ReconnectAttemptDeadlineTests.swiftweb/app/env.tsweb/services/relay/http.tsweb/tests/client-config-env.test.ts
💤 Files with no reviewable changes (1)
- Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohHostRuntime.swift
| if abandonedReconnectDialCount <= Self.maximumAbandonedReconnectDials, | ||
| let accountID = stackUserID ?? identityProvider?.currentUserID { | ||
| recordTransientAutomaticReconnectBackoff(accountID: accountID) | ||
| } | ||
| return .failed(.timedOut) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Off-by-one in the abandoned-dial ceiling check.
maximumAbandonedReconnectDials is documented as the ceiling on concurrently outstanding abandoned dials before automatic retries pause, but registerAbandonedReconnectDial already incremented the count for the just-timed-out dial before this check runs. Using <= schedules another automatic retry when the count already equals the ceiling (3), letting a 4th abandoned dial accumulate before retries actually pause — one more than the documented bound of 3.
🐛 Proposed fix
- if abandonedReconnectDialCount <= Self.maximumAbandonedReconnectDials,
+ if abandonedReconnectDialCount < Self.maximumAbandonedReconnectDials,
let accountID = stackUserID ?? identityProvider?.currentUserID {
recordTransientAutomaticReconnectBackoff(accountID: accountID)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if abandonedReconnectDialCount <= Self.maximumAbandonedReconnectDials, | |
| let accountID = stackUserID ?? identityProvider?.currentUserID { | |
| recordTransientAutomaticReconnectBackoff(accountID: accountID) | |
| } | |
| return .failed(.timedOut) | |
| if abandonedReconnectDialCount < Self.maximumAbandonedReconnectDials, | |
| let accountID = stackUserID ?? identityProvider?.currentUserID { | |
| recordTransientAutomaticReconnectBackoff(accountID: accountID) | |
| } | |
| return .failed(.timedOut) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`
around lines 1890 - 1894, Update the abandoned reconnect ceiling check in the
timeout handling flow to use a strict less-than comparison, accounting for the
increment performed by registerAbandonedReconnectDial before this check.
Preserve the existing accountID fallback and
recordTransientAutomaticReconnectBackoff behavior while ensuring no retry is
scheduled when abandonedReconnectDialCount equals
maximumAbandonedReconnectDials.
eff3aa4 to
29021b6
Compare
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. |
62faea5 to
4d7cb68
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ConnectionRecovery.swift (1)
862-863: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale doc comment: "MainActor-confined" once-guard no longer matches the delegated implementation.
The doc comment above
raceAgainstDeadlinestill says the once-guard is "MainActor-confined," but the reimplementation now delegates settlement toRPCTaskTimeout, whose own doc states the guard works "through an actor" (backed byRPCTaskTimeoutRace, not MainActor). Worth updating the comment so future readers of this concurrency-sensitive path aren't misled about which primitive actually enforces the once-guard.📝 Suggested comment update
- /// operation runs in its own task that the deadline path abandons after - /// a best-effort cancel; the once-guard is MainActor-confined so exactly - /// one side resumes. An abandoned dial retains its captures until it + /// operation runs in its own task that the deadline path abandons after + /// a best-effort cancel; `RPCTaskTimeout`'s actor-backed once-guard + /// ensures exactly one side resumes. An abandoned dial retains its captures until itAlso applies to: 905-932
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+ConnectionRecovery.swift around lines 862 - 863, Update the documentation above raceAgainstDeadline to remove the outdated “MainActor-confined” description and accurately state that settlement is guarded through the actor-backed RPCTaskTimeout/RPCTaskTimeoutRace implementation. Apply the same correction to the corresponding documentation around the additionally referenced section.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+ConnectionRecovery.swift:
- Around line 862-863: Update the documentation above raceAgainstDeadline to
remove the outdated “MainActor-confined” description and accurately state that
settlement is guarded through the actor-backed RPCTaskTimeout/RPCTaskTimeoutRace
implementation. Apply the same correction to the corresponding documentation
around the additionally referenced section.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d582aab2-dd8b-4864-b5c4-1d9fc333a7bc
📒 Files selected for processing (2)
Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/RPCTaskTimeout.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ConnectionRecovery.swift
lawrencecchen
added
the
stale-revisit
3 participants
Fixes #8531
Related lifecycle context: #8573
Summary
Testing
sync.subscribe_ok./,/handler/sign-in, and/handler/after-sign-inreturned 200 from the local API.Demo Video
Codex-irrel-49440simulator and taggedirrelMac app.Review Trigger
Automatic repository reviews run on push. No manual Codex review was requested.
Checklist