Reconnect stale iOS shell clients after liveness failure - #7065
Closed
azooz2003-bit wants to merge 3 commits into
Closed
azooz2003-bit wants to merge 3 commits into
azooz2003-bit wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughExtends MobileShellComposite's recovery flow with a new ChangesLiveness-probe reconnect flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant LivenessWatchdog
participant MobileShellComposite
participant PairedMacStore
participant RemoteClient
LivenessWatchdog->>MobileShellComposite: recoverMobileConnection(trigger: .livenessProbe)
MobileShellComposite->>MobileShellComposite: recoveryPlan(for: trigger)
alt still connected and plan = resyncConnectedClient
MobileShellComposite->>RemoteClient: resync and replay
else plan = reconnectFromStoredRoutes
MobileShellComposite->>RemoteClient: disconnect (preservingOtherMacWorkspaceState: true)
MobileShellComposite->>PairedMacStore: fetch stored paired-Mac routes
PairedMacStore-->>MobileShellComposite: routes
MobileShellComposite->>RemoteClient: reconnect using fresh client
end
Suggested reviewers: 🚥 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 |
Contributor
Greptile SummaryThis PR fixes a stale iOS shell connection bug where a failed render-grid liveness probe was reusing the same wedged RPC client (via
Confidence Score: 5/5Safe to merge. The liveness probe now correctly rebuilds the full transport stack instead of reusing a wedged client, and both new behaviors are regression-tested end-to-end. The behavioral change is narrowly scoped: liveness-probe and unavailable-status network-change triggers now route through a full disconnect+reconnect Task that explicitly tears down the old transport before dialing fresh routes. The synchronous guard ordering prevents concurrent triggers from interfering. The two new tests exercise the exact failure modes described in the PR, and the presencePush trigger is already guarded at its only call site to fire only when the connection is already down. No files require special attention. MobileShellComposite.swift is the only production change and is self-consistent. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[recoverMobileConnection trigger] --> B{recoveryInFlight?}
B -- yes --> Z[return drop trigger]
B -- no --> C[recoveryPlan for trigger]
C --> D{trigger}
D -- livenessProbe/presencePush --> E[plan: reconnectStoredMac]
D -- manual --> F{macConnectionStatus == .connected?}
F -- yes --> G[plan: resyncConnectedClient]
F -- no --> E
D -- networkChange --> H{macConnectionStatus == .unavailable?}
H -- yes --> E
H -- no --> G
G --> I{connectionState == .connected AND remoteClient != nil?}
I -- yes --> J[resyncTerminalOutput fast path]
E --> K{pairedMacStore == nil?}
K -- yes --> J
K -- no --> L[recoveryInFlight = true]
I -- no --> L
L --> M[Task body on MainActor]
M --> N{connectionState == .connected AND remoteClient != nil?}
N -- yes --> O[disconnectLiveConnection]
N -- no --> P{connectionState != .connected?}
O --> P
P -- no --> Q[return defer resets flags]
P -- yes --> R[await reconnectActiveMacIfAvailable]
R -- success --> S[connectionState = .connected]
R -- fail --> T[connectionRecoveryFailed = true]
S --> U[defer: recoveryInFlight = false]
T --> U
Q --> U
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[recoverMobileConnection trigger] --> B{recoveryInFlight?}
B -- yes --> Z[return drop trigger]
B -- no --> C[recoveryPlan for trigger]
C --> D{trigger}
D -- livenessProbe/presencePush --> E[plan: reconnectStoredMac]
D -- manual --> F{macConnectionStatus == .connected?}
F -- yes --> G[plan: resyncConnectedClient]
F -- no --> E
D -- networkChange --> H{macConnectionStatus == .unavailable?}
H -- yes --> E
H -- no --> G
G --> I{connectionState == .connected AND remoteClient != nil?}
I -- yes --> J[resyncTerminalOutput fast path]
E --> K{pairedMacStore == nil?}
K -- yes --> J
K -- no --> L[recoveryInFlight = true]
I -- no --> L
L --> M[Task body on MainActor]
M --> N{connectionState == .connected AND remoteClient != nil?}
N -- yes --> O[disconnectLiveConnection]
N -- no --> P{connectionState != .connected?}
O --> P
P -- no --> Q[return defer resets flags]
P -- yes --> R[await reconnectActiveMacIfAvailable]
R -- success --> S[connectionState = .connected]
R -- fail --> T[connectionRecoveryFailed = true]
S --> U[defer: recoveryInFlight = false]
T --> U
Q --> U
Reviews (2): Last reviewed commit: "Tighten stale iOS recovery planning" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
1329
to
1336
| /// User-initiated reconnect from the Retry control. | ||
| public func retryMobileConnection() { | ||
| connectionRecoveryFailed = false | ||
| recoverMobileConnection(trigger: .manual) | ||
| recoverMobileConnection( | ||
| trigger: .manual, | ||
| forceReconnectConnectedClient: macConnectionStatus != .connected | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
retryMobileConnection uses macConnectionStatus as the authority for "needs full reconnect"
macConnectionStatus != .connected includes .reconnecting — the status written by markMacConnectionReconnecting() inside the resync fast-path. If a prior network-change trigger is currently mid-resync (it set macConnectionStatus = .reconnecting but did not set recoveryInFlight), a user hitting Retry will see forceReconnectConnectedClient: true, skip the resync guard, reach the guard !recoveryInFlight gate, and start a full disconnect+reconnect that supersedes the ongoing resync. Whether that is intentional or surprising depends on whether a resync under .reconnecting should be interruptible; but using macConnectionStatus as the authority here while connectionState and recoveryInFlight are the other two sources makes the intended invariant hard to see.
Rule Used: Flag correctness-critical detection/identity deriv... (source)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTests.swift`:
- Around line 235-241: The current assertion in
MobileShellRenderGridLivenessTests around pollUntil/reconnected is too weak
because it can pass on the stale client resubscribe path. Tighten the test by
asserting a reconnect-only side effect in the same repro flow, using a unique
signal like mobile.attach_ticket.create (or another request that only occurs
after rebuilding the saved-Mac RPC client) instead of just workspace.list and
macConnectionStatus. Keep the check in the existing liveness test so it fails
against the pre-fix behavior and proves recovery came from a fresh reconnect.
In
`@Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swift`:
- Around line 152-153: The reconnect-ticket test helper is still using real
wall-clock time instead of the injected test clock, which can make reconnect
behavior flaky. Update `manualAttachTicketResultFrame` and the
`mobile.attach_ticket.create` path in `MobileShellRenderGridLivenessTestSupport`
to accept and use the suite’s fake clock, deriving the replacement ticket’s
`expiresAt` from `clock.now` (or the injected clock source) rather than
`Date()`. Also apply the same clock threading to the related helper code
referenced by the same test support flow so all expiry and reconnect checks stay
on the virtual clock.
🪄 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
Run ID: 993c12e1-f9db-4617-a98c-f86b9919a0f9
📒 Files selected for processing (3)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTests.swift
Comment on lines
+235
to
+241
| let reconnected = try await pollUntil(attempts: 800) { | ||
| await router.count(of: "workspace.list") >= 2 && store.macConnectionStatus == .connected | ||
| } | ||
| #expect( | ||
| reconnected, | ||
| "a failed liveness probe must reconnect from the saved Mac record instead of reusing the stale RPC client" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert a reconnect-only signal here, not just “eventual recovery.”
This predicate can still pass on the old same-client resubscribe path. In this harness, only subscribe request #2 is held; later requests still succeed on the original client, so workspace.list >= 2 && macConnectionStatus == .connected does not prove that recovery rebuilt the saved-Mac RPC client. Please assert a reconnect-only side effect such as mobile.attach_ticket.create so the test actually goes red against the pre-fix behavior. As per coding guidelines, "When a user says tests missed a bug, add or adjust behavior-level coverage around the exact repro path before claiming the fix is complete."
Suggested assertion tightening
- let reconnected = try await pollUntil(attempts: 800) {
- await router.count(of: "workspace.list") >= 2 && store.macConnectionStatus == .connected
- }
+ let reconnected = try await pollUntil(attempts: 800) {
+ let attachTicketRequests = await router.count(of: "mobile.attach_ticket.create")
+ let workspaceLists = await router.count(of: "workspace.list")
+ return attachTicketRequests >= 1 &&
+ workspaceLists >= 2 &&
+ store.macConnectionStatus == .connected
+ }📝 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.
Suggested change
| let reconnected = try await pollUntil(attempts: 800) { | |
| await router.count(of: "workspace.list") >= 2 && store.macConnectionStatus == .connected | |
| } | |
| #expect( | |
| reconnected, | |
| "a failed liveness probe must reconnect from the saved Mac record instead of reusing the stale RPC client" | |
| ) | |
| let reconnected = try await pollUntil(attempts: 800) { | |
| let attachTicketRequests = await router.count(of: "mobile.attach_ticket.create") | |
| let workspaceLists = await router.count(of: "workspace.list") | |
| return attachTicketRequests >= 1 && | |
| workspaceLists >= 2 && | |
| store.macConnectionStatus == .connected | |
| } | |
| `#expect`( | |
| reconnected, | |
| "a failed liveness probe must reconnect from the saved Mac record instead of reusing the stale RPC client" | |
| ) |
🤖 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/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTests.swift`
around lines 235 - 241, The current assertion in
MobileShellRenderGridLivenessTests around pollUntil/reconnected is too weak
because it can pass on the stale client resubscribe path. Tighten the test by
asserting a reconnect-only side effect in the same repro flow, using a unique
signal like mobile.attach_ticket.create (or another request that only occurs
after rebuilding the saved-Mac RPC client) instead of just workspace.list and
macConnectionStatus. Keep the check in the existing liveness test so it fails
against the pre-fix behavior and proves recovery came from a fresh reconnect.
Source: Coding guidelines
azooz2003-bit
force-pushed
the
feat-ios-connection-stale
branch
from
July 7, 2026 18:55
ed75854 to
13adb19
Compare
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
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
3 participants
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.
Summary
Verification
swift test --package-path Packages/iOS/CmuxMobileShell --filter MobileShellRenderGridLivenessTestsswift test --package-path Packages/iOS/CmuxMobileShell./scripts/reload-cloud.sh --tag ioscxn(cloud unavailable locally, local fallback succeeded)./ios/scripts/reload.sh --tag ioscxn(simulator succeeded)Dogfood Notes
Test with the
ioscxntag. Previous bad behavior: after leaving the iOS app open for a while, sessions could stop reconnecting until the app was force-closed. Expected behavior: liveness failure or Retry rebuilds the saved-Mac client and sessions recover without restarting the iOS app.Device reload note:
./ios/scripts/reload-cloud.sh --tag ioscxn --device-id 4A52829D-6427-599F-A166-4058881D2DF4 --wait 1200could not run because local ASC credentials are missing. The allowedios/scripts/reload.sh --tag ioscxn --device-only --device-id 4A52829D-6427-599F-A166-4058881D2DF4fallback reached signing and failed because no local development team is configured.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Reconnect stale iOS shell clients when the render‑grid liveness probe fails and after network changes if the connection is marked unavailable. Recovery now rebuilds the transport and RPC client instead of reusing a wedged one, so sessions recover without restarting the app.
Bug Fixes
macConnectionStatusisunavailable; manual reconnects only when already unavailable or disconnected.Tests
failedLivenessProbeReconnectsSavedMacWithFreshClientandnetworkChangeAfterUnavailableStatusReconnectsSavedMacWithFreshClient.Written for commit 13adb19. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Note
Medium Risk
Touches core iOS connection recovery and disconnect/reconnect paths; behavior change is intentional but could affect edge cases around network vs manual retry vs still-healthy transports.
Overview
Fixes iOS sessions that stayed broken until force-quit when the render-grid stream died but the persistent RPC client was wedged (e.g. after suspension).
Recovery planning in
MobileShellCompositeadds alivenessProbetrigger and aRecoveryPlanthat chooses resync vs full reconnect from the saved Mac usingmacConnectionStatusand trigger type. Failed liveness probes and presence-driven recovery now rebuild transport viareconnectActiveMacIfAvailableinstead of onlyresyncTerminalOutput. On forced reconnect, a still-“connected” client is disconnected first (disconnectLiveConnection) while preserving other Mac workspace state. Manual Retry reconnects when the Mac is already marked unavailable; network changes after unavailable also take the reconnect path.Adds regression tests and shared test helpers (attach ticket framing, manual reachability, reconnectable connected store) plus liveness router support for
mobile.attach_ticket.create.Reviewed by Cursor Bugbot for commit 13adb19. Bugbot is set up for automated code reviews on this repo. Configure here.