Repository navigation
Bound iOS pairing attempts - #6495
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces hard deadlines for both QR/manual pairing and app-launch session restoration. A thread-safe ChangesPairing-attempt deadline enforcement
Session-restore deadline enforcement
Sequence Diagram(s)sequenceDiagram
actor User
participant MobileShellComposite
participant withRequestTimeout
participant RPCRequestTimeoutState
participant WorkspaceListRPC
rect rgba(100, 149, 237, 0.5)
note over MobileShellComposite: Pairing attempt starts
User->>MobileShellComposite: connectManualHost / QR scan
MobileShellComposite->>MobileShellComposite: record pairingAttemptStartedAt
end
loop Each candidate route
MobileShellComposite->>MobileShellComposite: boundedPairingRequestTimeoutNanoseconds()
alt budget exhausted
MobileShellComposite-->>User: throw .requestTimedOut
else budget remaining
MobileShellComposite->>withRequestTimeout: call with bounded timeout
withRequestTimeout->>RPCRequestTimeoutState: install continuation
withRequestTimeout->>WorkspaceListRPC: start operation task
withRequestTimeout->>withRequestTimeout: start sleep(boundedNanoseconds)
alt RPC responds in time
WorkspaceListRPC-->>RPCRequestTimeoutState: resume(returning:)
RPCRequestTimeoutState-->>MobileShellComposite: workspace list
else deadline fires
withRequestTimeout-->>RPCRequestTimeoutState: resume(throwing: .requestTimedOut)
RPCRequestTimeoutState-->>MobileShellComposite: .requestTimedOut
end
end
end
rect rgba(220, 80, 80, 0.5)
note over MobileShellComposite: All routes failed
MobileShellComposite->>MobileShellComposite: DiagnosticEvent(.pairFail)
MobileShellComposite-->>User: throw lastError / .connectionClosed
end
sequenceDiagram
participant LaunchController
participant AuthCoordinator
participant withAuthPhaseTimeout
participant AuthPhaseTimeoutState
participant sessionProbeTask
participant deadlineTask
rect rgba(100, 149, 237, 0.5)
note over AuthCoordinator: Session restore phase starts
LaunchController->>AuthCoordinator: bootstrap()
AuthCoordinator->>AuthCoordinator: validateSession with sessionRestore deadline
end
AuthCoordinator->>withAuthPhaseTimeout: runPhase(.validateSession, timeout: 8s)
withAuthPhaseTimeout->>AuthPhaseTimeoutState: install continuation
withAuthPhaseTimeout->>sessionProbeTask: start accessToken/refreshToken probes
withAuthPhaseTimeout->>deadlineTask: start sleep(8 seconds)
alt probes complete in time
sessionProbeTask->>AuthPhaseTimeoutState: resume(returning: tokenState)
AuthPhaseTimeoutState-->>AuthCoordinator: tokens validated
AuthPhaseTimeoutState->>deadlineTask: cancel
else 8-second deadline fires
deadlineTask->>AuthPhaseTimeoutState: resume(throwing: .timedOut)
AuthPhaseTimeoutState-->>AuthCoordinator: .timedOut
AuthPhaseTimeoutState->>sessionProbeTask: cancel
AuthCoordinator->>AuthCoordinator: preserve cached session
end
rect rgba(80, 220, 80, 0.5)
note over AuthCoordinator: Session ready or preserved
AuthCoordinator-->>LaunchController: bootstrap complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (5 errors, 2 warnings)
✅ Passed checks (16 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![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
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.swift (1)
3328-3350:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftStart the deadline at the user pairing attempt, not inside
connect(ticket:).Line 3328 resets the budget after pre-connect work, but manual pairing starts earlier and can await
manualHostTicket(...)/mobile.attach_ticket.createbefore reaching this loop. A slow manual attach-ticket RPC can therefore exceed the intended 8-second attempt window and still get a fresh per-route budget here. Carry thebeginPairingAttemptstart instant through manual ticket creation and this route loop, and bound that pre-connect RPC too.🤖 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 3328 - 3350, The deadline for the pairing attempt should start before manual ticket creation, not at line 3328 inside the route loop. Move the initialization of pairingAttemptStartedAt to an earlier point in the pairing flow (before manualHostTicket or mobile.attach_ticket.create RPC calls), then pass this earlier timestamp through to the route loop where supportedRoutes is iterated and boundedPairingRequestTimeoutNanoseconds is called. This ensures the entire manual pairing attempt, including the attach_ticket RPC and all route attempts, is bounded by the 8-second window starting from when pairing actually begins, not reset at this intermediate point.
🤖 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 3397-3413: In the boundedPairingRequestTimeoutNanoseconds
function, replace the direct call to Date() with runtime.now() when calculating
elapsedSeconds. This ensures consistency with other usages throughout the file
(lines 115, 122, 193, 3622, 3649) and maintains proper dependency injection for
testability. Update the line that calculates elapsedSeconds to use
runtime.now().timeIntervalSince(attemptStartedAt) instead of
Date().timeIntervalSince(attemptStartedAt).
In
`@Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobilePairingAttemptDeadlineTests.swift`:
- Around line 11-22: The test
qrPairingURLTimesOutWithoutWaitingForStuckTransport uses real wall-clock time
assertions (Date().timeIntervalSince(startedAt) < 0.05 on line 21) which makes
the test flaky under CI load. Replace the wall-clock timing expectation with a
virtual or fake clock mechanism that advances deterministically. Update the fake
transport implementation (referenced around lines 79-82) to use the same virtual
clock instead of real-time delays. Assert on causality transitions (such as
state changes or timeout behavior) rather than elapsed duration thresholds to
ensure the test is deterministic and not dependent on actual machine
performance.
---
Outside diff comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 3328-3350: The deadline for the pairing attempt should start
before manual ticket creation, not at line 3328 inside the route loop. Move the
initialization of pairingAttemptStartedAt to an earlier point in the pairing
flow (before manualHostTicket or mobile.attach_ticket.create RPC calls), then
pass this earlier timestamp through to the route loop where supportedRoutes is
iterated and boundedPairingRequestTimeoutNanoseconds is called. This ensures the
entire manual pairing attempt, including the attach_ticket RPC and all route
attempts, is bounded by the 8-second window starting from when pairing actually
begins, not reset at this intermediate point.
🪄 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: bc575ef3-a47d-438b-b6d7-5a4626e3872c
📒 Files selected for processing (7)
Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileSyncRuntime.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobilePairingAttemptDeadlineTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobilePairingScannerSheet.swiftios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRuntime.swift
Contributor
Greptile SummaryThis PR introduces a single 8-second end-to-end budget for iOS pairing attempts (QR, pasted QR, manual host), replaces nested per-subphase timeouts with a shared
Confidence Score: 3/5Two issues in the RPC layer need attention before merge: a named test seam in production Sources, and a retry gate that incorrectly fires on user cancellation as well as genuine timeouts. The
Important Files Changed
Reviews (20): Last reviewed commit: "Deflake auth runtime package CI" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+3397
to
+3413
| private static func boundedPairingRequestTimeoutNanoseconds( | ||
| runtime: any MobileSyncRuntime, | ||
| attemptStartedAt: Date | ||
| ) -> UInt64 { | ||
| let requestTimeout = runtime.pairingRequestTimeoutNanoseconds | ||
| let attemptTimeout = runtime.pairingAttemptTimeoutNanoseconds | ||
| guard attemptTimeout > 0 else { | ||
| return requestTimeout | ||
| } | ||
|
|
||
| let elapsedSeconds = max(0, Date().timeIntervalSince(attemptStartedAt)) | ||
| let elapsedNanoseconds = UInt64((elapsedSeconds * 1_000_000_000).rounded(.up)) | ||
| guard elapsedNanoseconds < attemptTimeout else { | ||
| return 0 | ||
| } | ||
| return min(requestTimeout, attemptTimeout - elapsedNanoseconds) | ||
| } |
Contributor
There was a problem hiding this comment.
Date() bypasses the injected runtime.now clock
boundedPairingRequestTimeoutNanoseconds calls Date() directly to measure elapsed time, and the call site captures pairingAttemptStartedAt the same way. MobileSyncRuntime already carries a now: @Sendable () -> Date injectable clock precisely so time-dependent paths can be exercised deterministically.
Because both reads use the system clock, the new deadline math cannot be controlled in tests — the test in MobilePairingAttemptDeadlineTests works around this by setting pairingAttemptTimeoutNanoseconds to a 1 ms budget instead of driving injected time. Passing runtime.now into boundedPairingRequestTimeoutNanoseconds (and using it for pairingAttemptStartedAt) would keep the approach consistent with the rest of the codebase.
This comment has been minimized.
This comment has been minimized.
Comment on lines
+6
to
+47
| private final class RPCRequestTimeoutState<T: Sendable>: @unchecked Sendable { | ||
| private let lock = NSLock() | ||
| private var continuation: CheckedContinuation<T, any Error>? | ||
| private var cancelHandler: (@Sendable () -> Void)? | ||
|
|
||
| func install(_ continuation: CheckedContinuation<T, any Error>) { | ||
| lock.lock() | ||
| self.continuation = continuation | ||
| lock.unlock() | ||
| } | ||
|
|
||
| func setCancelHandler(_ handler: @escaping @Sendable () -> Void) { | ||
| lock.lock() | ||
| cancelHandler = handler | ||
| lock.unlock() | ||
| } | ||
|
|
||
| func resume(returning value: T) { | ||
| finish { $0.resume(returning: value) } | ||
| } | ||
|
|
||
| func resume(throwing error: any Error) { | ||
| finish { $0.resume(throwing: error) } | ||
| } | ||
|
|
||
| func cancel() { | ||
| finish { $0.resume(throwing: CancellationError()) } | ||
| } | ||
|
|
||
| private func finish(_ resume: (CheckedContinuation<T, any Error>) -> Void) { | ||
| lock.lock() | ||
| let continuation = continuation | ||
| let cancelHandler = cancelHandler | ||
| self.continuation = nil | ||
| self.cancelHandler = nil | ||
| lock.unlock() | ||
|
|
||
| guard let continuation else { return } | ||
| cancelHandler?() | ||
| resume(continuation) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
RPCRequestTimeoutState is missing the finished flag, risking a leaked CheckedContinuation
AuthPhaseTimeoutState (the direct model for this class) guards both install and setCancelHandler against a prior cancel() call via a finished: Bool flag. RPCRequestTimeoutState does not: its install unconditionally stores the continuation, and its setCancelHandler never calls the handler eagerly.
Concrete failure: withTaskCancellationHandler guarantees that if the task is already cancelled when the function is called, onCancel fires before the body runs. If that happens, state.cancel() reads a nil continuation and returns without resuming anything; then install stores the continuation that nothing will ever resume. The result is a permanently hung CheckedContinuation — a logged runtime warning in release and a crash on dealloc in debug builds — leaving the pairing spinner active indefinitely.
Second gap: if cancel() fires after install but before setCancelHandler, the continuation is correctly resumed with CancellationError, but operationTask and timeoutTask are never cancelled because setCancelHandler does not call the handler eagerly when already finished. The AuthPhaseTimeoutState addresses this with if finished { handler() } in setCancelHandler. The same two-line pattern needs to be added here.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator`+SessionPriming.swift:
- Around line 90-94: The session restoration process is giving each runPhase
call a fresh full timeout instead of sharing a single budget across all phases.
Capture the restore start time once at the beginning of the checkExistingSession
method, then modify all runPhase calls (including those in the validateSession
phase around lines 90-94 and the validateCachedSession phase around lines
185-201 and 232-234) to pass the remaining time calculated from the initial
start time instead of using the fixed timeouts.sessionRestore timeout. This
ensures that slow token probes or validation checks consume from a single shared
budget rather than each phase getting its own full allocation.
- Around line 229-239: The catch block for the session refresh-token validation
does not verify that sessionGeneration remains unchanged before calling
preserveCachedSessionAfterValidationFailure(), which can resurrect stale cached
auth state if the generation is bumped during the await on line 232. Add a
sessionGeneration guard check in the catch block before invoking
preserveCachedSessionAfterValidationFailure(), mirroring the validation guard
used in the successful code path at line 240 to ensure the session has not been
invalidated by a sign-out or newer sign-in during the restore timeout period.
In
`@Packages/Shared/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/AuthCoordinatorTimeoutTests.swift`:
- Around line 90-99: The test uses a fixed Task.sleep with milliseconds before
asserting on the completion signal, which relies on timing and can fail under
load. Instead of the Task.sleep call on line 97, await the completion signal
that was already created. Replace the sleep with code that waits for the
completion signal to be marked as started (by awaiting a method on the
completion object or similar mechanism) before the assertion on didStart,
ensuring the test waits on a real async signal rather than wall-clock time.
- Around line 169-171: The while true loop with try? await Task.sleep(for:
.seconds(3600)) creates a post-cancellation busy loop because Task.sleep throws
on cancellation, the try? silently catches it, and the loop continues spinning
forever. Replace this cancellation-aware suspension with a
non-cancellation-aware suspension mechanism that will keep the task genuinely
suspended even after cancellation occurs, such as using DispatchSemaphore with a
blocking wait or Thread.sleep, which do not respond to task cancellation.
🪄 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: e05aa21a-105e-48a6-a2b2-728d36196dce
📒 Files selected for processing (5)
Packages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator+SessionPriming.swiftPackages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthPhaseTimeout.swiftPackages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthTimeouts.swiftPackages/Shared/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/AuthCoordinatorTimeoutTests.swiftPackages/Shared/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/TestPhaseSignal.swift
Comment on lines
+90
to
+94
| hasStoredTokens = try await runPhase(.validateSession, timeout: timeouts.sessionRestore) { | ||
| let hasAccessToken = await client.accessToken() != nil | ||
| let hasRefreshToken = await client.refreshToken() != nil | ||
| return hasAccessToken || hasRefreshToken | ||
| } |
There was a problem hiding this comment.
Carry one remaining restore budget instead of restarting sessionRestore for each phase.
Each runPhase(... timeout: timeouts.sessionRestore) gets a fresh deadline, so a slow token probe can consume nearly the whole launch budget and validateCachedSession can then spend another full budget on currentUser/refresh-token checks. That weakens the PR’s hard launch-restore deadline and can keep “Restoring session” visible longer than configured. Capture the restore start once in checkExistingSession and pass remaining time into the nested probes/validation. Based on PR context: “Adds a hard deadline for session restoration at app launch.”
Also applies to: 185-201, 232-234
🤖 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/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator`+SessionPriming.swift
around lines 90 - 94, The session restoration process is giving each runPhase
call a fresh full timeout instead of sharing a single budget across all phases.
Capture the restore start time once at the beginning of the checkExistingSession
method, then modify all runPhase calls (including those in the validateSession
phase around lines 90-94 and the validateCachedSession phase around lines
185-201 and 232-234) to pass the remaining time calculated from the initial
start time instead of using the fixed timeouts.sessionRestore timeout. This
ensures that slow token probes or validation checks consume from a single shared
budget rather than each phase getting its own full allocation.
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.swift (1)
1196-1206:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-check attempt currency before minting the manual attach ticket.
failPairingIfOfflineawaits and can return.proceedwithout checkingattemptIDwhen the device is online. If the user cancels or starts another pairing during that await, this path still callsmanualHostTicketand reads the now-globalpairingAttemptStartedAt, so a superseded manual attempt can issuemobile.attach_ticket.createunder the wrong budget.Proposed fix
activeRoute = directRoute let attemptID = beginPairingAttempt(method: "manual") + let attemptStartedAt = pairingAttemptStartedAt // Fast offline preflight: fail immediately instead of stacking // per-route timeouts into the opaque ~60s blob. let manualRoutes = directRoute.map { [$0] } ?? [] guard await failPairingIfOffline(attemptID: attemptID, phase: "preflight", routes: manualRoutes) == .proceed else { return } + guard isCurrentPairingAttempt(attemptID) else { return } do { let ticket = try await manualHostTicket( name: trimmedName, host: normalizedHost, port: port, - attemptStartedAt: pairingAttemptStartedAt + attemptStartedAt: attemptStartedAt )🤖 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 1196 - 1206, After the guard statement where failPairingIfOffline returns proceed, add an additional check to verify that the attemptID is still current before proceeding to call manualHostTicket. The issue is that during the await on failPairingIfOffline, the user could cancel or start another pairing attempt, which would make the current attemptID stale. Without re-checking the attempt currency, the code will call manualHostTicket with an outdated attemptID and use the now-global pairingAttemptStartedAt value, causing a superseded manual attempt to potentially issue the attach ticket under the wrong budget. Add a validation after the guard statement to ensure attemptID hasn't been superseded before proceeding with the ticket creation.
🤖 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.swift`:
- Around line 1196-1206: After the guard statement where failPairingIfOffline
returns proceed, add an additional check to verify that the attemptID is still
current before proceeding to call manualHostTicket. The issue is that during the
await on failPairingIfOffline, the user could cancel or start another pairing
attempt, which would make the current attemptID stale. Without re-checking the
attempt currency, the code will call manualHostTicket with an outdated attemptID
and use the now-global pairingAttemptStartedAt value, causing a superseded
manual attempt to potentially issue the attach ticket under the wrong budget.
Add a validation after the guard statement to ensure attemptID hasn't been
superseded before proceeding with the ticket creation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c4729552-9d78-4628-992c-84f599f3fc9c
📒 Files selected for processing (5)
Packages/Shared/CMUXAuthCore/Sources/CMUXAuthCore/Models/CMUXAuthState.swiftPackages/Shared/CMUXAuthCore/Tests/CMUXAuthCoreTests/CMUXAuthStateTests.swiftPackages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthPhaseTimeout.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swift (1)
225-231:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStart the request deadline before auth augmentation.
Line 225 awaits
requestDataWithAuthbefore the timeout race is installed, but that path can awaitruntime.stackAccessTokenProvider()at Lines 290 and 322. A hung token provider can still block QR/manual pairing beyond the remaining budget beforewithRequestTimeoutstarts, so the “hard” request deadline is not actually end-to-end.🐛 Proposed fix
- let authenticated = try await requestDataWithAuth(augmented) return try await Self.withRequestTimeout( timeoutNanoseconds: timeoutNanoseconds ?? runtime.rpcRequestTimeoutNanoseconds ) { + let authenticated = try await self.requestDataWithAuth(augmented) + try Task.checkCancellation() try await self.session.send(payload: authenticated, requestID: id) } cancelOperation: { await self.session.tearDown(error: .connectionClosed) }🤖 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/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swift` around lines 225 - 231, The request timeout deadline is being installed after the requestDataWithAuth call completes, but this authentication call can potentially await runtime.stackAccessTokenProvider() which may hang, causing the operation to exceed the intended timeout. Move the withRequestTimeout wrapper to encompass the entire operation including the requestDataWithAuth call at line 225, so that both the authentication augmentation and the session.send operation are protected by the timeout deadline from the start.
🤖 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/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCClientTests.swift`:
- Around line 170-172: Replace the fixed 20ms sleep in the test (the Task.sleep
call) with a deterministic wait mechanism. In the SlowConnectTimeoutTransport
test double class, add a new method called waitUntilConnectAttemptFinished()
that returns when the connect() method has fully unwound after close() resumes
it. Implement this method by using a continuation that resumes from within a
defer block in the connect() method to signal completion. Then, in the test at
line 170-172, replace the Task.sleep call with an await to this new
waitUntilConnectAttemptFinished() method before asserting on
sentRequests().isEmpty.
---
Outside diff comments:
In `@Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swift`:
- Around line 225-231: The request timeout deadline is being installed after the
requestDataWithAuth call completes, but this authentication call can potentially
await runtime.stackAccessTokenProvider() which may hang, causing the operation
to exceed the intended timeout. Move the withRequestTimeout wrapper to encompass
the entire operation including the requestDataWithAuth call at line 225, so that
both the authentication augmentation and the session.send operation are
protected by the timeout deadline from the start.
🪄 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: 1347297b-d072-4ddb-9c66-3598b87ddd5d
📒 Files selected for processing (4)
Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCSession.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCClientTests.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/TransportTestDoubles.swift
lawrencecchen
force-pushed
the
feat-ios-pairing-timeout
branch
from
June 20, 2026 11:26
746ce57 to
a40c7d1
Compare
# Conflicts: # Packages/Shared/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/HostBrowserSignInFlowTests.swift # Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swift # Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCSession.swift
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ad0fa68. Configure here.
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
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.
Summary
Tests
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
High Risk
Large changes to authentication lifecycle, token races, and sign-out—security-critical paths—with substantial new concurrency/timeout behavior on mobile RPC pairing.
Overview
iOS pairing & RPC — User-initiated pairing is capped by a single 8s attempt budget (default on
MobileSyncRuntime); per-RPC timeouts clip to whatever time remains.MobileCoreRPCClientuses oneRPCRequestDeadlinefor Stack token fetch/force-refresh, connect, and send, withRPCStackTokenGatededuping and briefly blocking retries after timeouts.MobileRPCConnectAttemptRegistryand abandoned-connect cleanup close slow or superseded connects without blocking later attempts; host-status probes can attach a cached token viastackAccessTokenForStatusProvider.Auth — Launch
CMUXAuthState.primedtreats a cached user with known tokens as signed in immediately (validation continues in the background) and adds a shortersessionRestoretimeout (8s) so restore fails open instead of sticking on “Restoring session”.AuthCoordinatorgains coordinator-owned phases for credential exchange, validation, token-touching calls, and post-sign-in hooks, plus sign-in preflight that cancels/waits on stale token work; sign-out coordinates credential capture, cancels in-flight work, and clears late token writes.storedAccessToken()is added onTokenProvidingfor non-mutating reads.CLI & CI — New
cmux sessions/session-debugreads~/.cmuxterm/*-hook-sessions.jsonwith filters and optional Codex index/transcript hints (no live socket). CI runsCmuxAuthRuntimeSwift Testing suites individually via--filter.Reviewed by Cursor Bugbot for commit 810f2f7. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Bounds iOS pairing to a single 8s attempt and tightens auth/RPC deadlines so timeouts fail fast without piling up connects or token fetches. Launch authenticates cached users immediately and adds a
cmux sessions listCLI for session-restore diagnostics with localized help.New Features
MobileRPCConnectAttemptRegistrywith one bounded retry and a brief hard-gate/reset if late close hangs; stored‑Mac reconnects reuse an in‑flight attempt.sessionRestore; a cached user with stored tokens is treated as signed in while validation runs; token read/refresh, credential exchange, validation, and post‑sign‑in hooks are bounded and tracked; sign‑in preflight cancels/awaits stale token work; dev auto‑login is skipped when tokens exist; adds non‑mutatingstoredAccessToken().RPCStackTokenGatededupes normal and force‑refresh token fetches and briefly suppresses retries after timeouts; slow or preinstalled connects are closed before sending; request‑queue tombstones are fixed and connect retry is allowed after a timeout; host‑status probes use a cached token viastackAccessTokenForStatusProvider.cmux sessions list(aliassession-debug) lists/filters saved agent hook sessions from~/.cmuxtermwith optional Codex transcript/index hints; localized usage/help.Bug Fixes
requestTimedOutand then tear down connects without blocking other waiters; timed‑out Stack token acquisition maps to a retryablerequestTimedOutinCMUXMobileRuntime.SystemDeliveredNotificationClearerno‑ops outside the app bundle; stabilize tests and deflakeCmuxAuthRuntimeCI by running Swift Testing suites individually.Written for commit 810f2f7. Summary will update on new commits.
Summary by CodeRabbit