Skip to content

iOS sign-in: bound every phase, cancellable OAuth waits, Cancel escape hatch - #5728

Merged
lawrencecchen merged 4 commits into
mainfrom
feat-ios-signin-hang
Jun 10, 2026
Merged

lawrencecchen merged 4 commits into
mainfrom
feat-ios-signin-hang

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Jun 9, 2026 •

Copy link
Copy Markdown
Contributor

Why

An external user reported sign-in loading forever on iOS: settings-style sheet, infinite spinner, no error, no timeout, no way to escape. Two structural holes made that possible:

  1. The interactive OAuth waits in the vendored Stack SDK (ASWebAuthenticationSession and ASAuthorizationController) park a checked continuation on a system callback that is not guaranteed to fire. The wait was not cancellation-responsive, so nothing could unwind it.
  2. No sign-in phase had a deadline. Any wedged call held isLoading = true indefinitely with every control disabled.

Hang-point inventory

Every phase that holds the sign-in loading state, now bounded:

Phase Call Bound
oauth client.signInWithOAuth (SIWA native / web OAuth sheet) 300s interactive
send_code sendMagicLinkEmail 60s
verify_code signInWithMagicLink 60s
password_sign_in signInWithCredential 60s
fetch_user post-sign-in currentUser fetch 60s
validate_session launch/foreground currentUser validation 60s
list_teams listTeams after sign-in 60s
post_sign_in onSignedIn hook (push token re-upload), runs while isLoading is still true 60s, failure tolerated

The interactive bound is generous on purpose (the user may be doing 2FA inside the sheet); it exists to terminate the callback-never-fires hang, and the new Cancel button covers bailing out earlier.

Mechanism

  • AuthTimeouts + withAuthPhaseTimeout: each phase races a deadline on an injected Clock (defaults ContinuousClock; virtual ManualTestClock in tests). Structured task group; the loser is cancelled and joined, so nothing outlives the call. A deadline ends the phase as the new localized, retryable AuthError.timedOut (en + ja in both the macOS and iOS catalogs). .timedOut maps to preserveCachedSession during validation, never signs out.
  • AuthFlowCancellationGate (vendored Stack SDK): makes both interactive waits cancellation-responsive. Cancellation claims the continuation exactly once (CancellationError), tears down the system sheet, and a late system callback is dropped instead of double-resuming. A post-present re-check (synchronous on the main actor) closes the race where cancellation lands between the pre-start check and presenting the sheet.
  • Structured token exchange: the web OAuth token exchange no longer runs in a detached Task; the continuation resumes with the callback URL and the exchange runs in the caller's task, so cancel/timeout cancels it too. Every sign-in token write goes through one publishSessionTokens chokepoint that refuses to persist a session on a cancelled task, so a cancelled or timed-out flow can never silently sign the user in behind the UI's back.
  • Cancellation is silent, not an error: CancellationError (and any error caught on a cancelled task, since URLSession surfaces cancellation as URLError(.cancelled) or a wrapped Stack transport error) maps to AuthError.cancelled, which SignInView swallows (analytics ios_sign_in_cancelled only) instead of flashing "Something went wrong".
  • Cancel affordance: SignInView keeps the in-flight sign-in Task and shows a localized Cancel button whenever a flow is in flight, on both the provider screen and the code-entry screen.
  • Diagnostics without secrets: phase timeouts and OAuth failures log one redacted line via AuthDebugLog (phase + duration / provider + error shape; tokens, JWTs, and emails are redacted before any sink).

Relationship to #5600

Same swallowed-failure class, complementary scopes: that PR makes OAuth failures observable (AuthErrorReporting seam, Sentry capture, PostHog failure_reason enrichment) without changing thrown types; this PR makes non-failures (hangs) terminate and cancellable. No duplication: this branch adds no error-reporting seam, that PR adds no deadlines. Both touch AuthCoordinator/SignInView, so whichever lands second takes a small rebase; the catch-site shape (AuthError(displaySafe:) ?? error) is preserved by both.

Most plausible root cause for the reporter

A Sign in with Apple flow whose ASAuthorizationController delegate callback never fired. Evidence: the wait parks a CheckedContinuation resumed only by the delegate (StackClientApp.signInWithAppleNative); nothing else can resume it, there was no deadline anywhere in the coordinator, and signInWithOAuth holds isLoading = true across the whole await, which disables every button in SignInView (isAuthInProgress), exactly matching "infinite spinner, no error, no escape". The PR #5600 investigation already established SIWA is structurally broken on per-tag dev bundles (INVALID_APPLE_CREDENTIALS comes back as an error, which renders), but a stuck Apple ID system sheet (unverified iCloud session, parental approval, network loss mid-sheet) returns nothing at all, which is the only state that produces this exact symptom. The new phase-tagged timeout logs plus the 5600 capture will name the exact phase on the next report.

Tests

Two-commit red/green:

  • Red (2d7fb36): cancelling a stuck OAuth sign-in surfaces .serverError(0, "auth_failed") (the generic "Something went wrong") instead of .cancelled, and no timeout machinery exists. Verified failing on the parent commit.
  • Green (this head): 60/60 pass in CmuxAuthRuntime, including new virtual-clock tests: stuck OAuth callback times out as .timedOut with loading stopped; wedged sendCode times out; all phases win their deadlines with the clock frozen at zero (proves deadline-child cleanup); hung post-sign-in hook is bounded and tolerated; cancelled-task error shapes map to .cancelled while live-task URLError(.cancelled) stays .networkError.

Verification

  • swift test --package-path Packages/CmuxAuthRuntime: 60/60 pass.
  • iOS app build (arm64 simulator, /tmp/cmux-signin-hang DerivedData, build only): BUILD SUCCEEDED.
  • Autoreview (branch vs origin/main): clean. Two residual Aziz policy notes are deliberate: ManualTestClock's nested Instant/Sleeper are intrinsic to a Clock conformance, and AuthFlowCancellationGate uses NSLock because withTaskCancellationHandler's onCancel is synchronous and may fire on any thread, so the claim cannot await actor isolation (documented in the source).

🤖 Generated with Claude Code


Note

High Risk
Touches authentication flows, vendored Stack SDK OAuth/token persistence, and session validation behavior; incorrect timeout or cancellation handling could sign users out incorrectly or leave stale UI state.

Overview
Fixes infinite iOS sign-in spinners by bounding every loading phase and making OAuth waits cancellable end-to-end.

AuthCoordinator now injects AuthTimeouts (300s interactive OAuth, 60s network) and an optional Clock, and wraps OAuth, magic-link, password, user fetch, session validation, team list, and the post-sign-in hook in withAuthPhaseTimeout, surfacing AuthError.timedOut (localized en/ja) instead of hanging forever. Session validation treats timeouts as transient (preserve cached session). AuthError(displaySafe:) maps CancellationError / cancelled-task errors to .cancelled so deliberate bail-out does not flash a generic server error.

SignInView tracks the in-flight Task, adds a Cancel button during auth, and swallows .cancelled for analytics; timeouts get a failure_reason of "timeout".

The vendored Stack SDK adds AuthFlowCancellationGate so ASWebAuthenticationSession and Sign in with Apple respond to task cancellation, dismiss system UI, and avoid double-resuming; web OAuth runs token exchange in the caller task, and publishSessionTokens refuses writes after cancellation.

New virtual-clock tests cover stuck OAuth, wedged send-code, post-sign-in hook bounds, and cancellation mapping.

Reviewed by Cursor Bugbot for commit 2f8bbf5. Bugbot is set up for automated code reviews on this repo. Configure here.


View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.


Summary by cubic

Fixes iOS sign-in hangs by adding per‑phase timeouts and making OAuth waits cancellable. Adds a Cancel button so users can exit stuck flows; timeouts return a localized, retryable error and never sign users out.

  • New Features

    • Per‑phase timeouts via AuthTimeouts/withAuthPhaseTimeout (300s interactive OAuth; 60s network).
    • New AuthError.timedOut with en/ja strings; session validation treats it as transient.
    • Cancel button in SignInView; cancellation maps to .cancelled and stays silent in UI.
  • Bug Fixes

    • OAuth waits in vendored StackAuth (ASWebAuthenticationSession, ASAuthorizationController) are cancellation‑responsive and dismiss on cancel; token exchange runs in the caller task, and publishSessionTokens refuses writes on a cancelled task to prevent silent sign‑in.
    • AuthCoordinator bounds oauth, code send/verify, password, user fetch, session validation, team list, and the post‑sign‑in hook.
    • Display‑safe mapping treats cancellation (including URLSession cancellations on a cancelled task) as .cancelled; live‑task URLError(.cancelled) stays .networkError.

Written for commit 2f8bbf5. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • New Features

    • Per-phase timeouts for sign-in and session operations to avoid indefinite waits
    • Visible cancel button to abort ongoing sign-in flows
  • Bug Fixes

    • Timeouts and cancellations now surface as clear, localized errors and stop loading states
    • Improved robustness of OAuth and interactive sign-ins so hangs no longer block progress
  • Localization

    • Added user-facing timeout and cancel strings (English + Japanese)

lawrencecchen and others added 2 commits June 9, 2026 15:47
…generic error

Red half of the sign-in-hang regression pair: a sign-in flow parked on a
system auth callback that never fires currently cannot be backed out of.
Cancellation surfaces as the generic "Something went wrong" server error
instead of AuthError.cancelled, so a cancel affordance would spuriously
show an error, and there is no timeout machinery at all.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… a Cancel escape hatch

An external user reported sign-in loading forever: settings-style sheet,
infinite spinner, no error, no timeout, no way out. The sign-in flow had
two structural holes:

1. The interactive OAuth waits in the vendored Stack SDK
   (ASWebAuthenticationSession and ASAuthorizationController) parked a
   checked continuation on a system callback that is not guaranteed to
   fire, and the wait was not cancellation-responsive: cancelling the
   awaiting task did nothing, so the continuation (and the spinner it
   gated) could be suspended forever.
2. No sign-in phase had a deadline. Any wedged call (OAuth callback,
   magic-link send/verify, credential sign-in, post-sign-in user fetch,
   session validation, team list, the post-sign-in hook) held
   isLoading=true indefinitely with every control disabled.

Mechanism:

- AuthTimeouts + withAuthPhaseTimeout race each phase against a deadline
  on an injected Clock (virtual-time testable; structured task group, the
  loser is cancelled and joined). Interactive flows get a generous 300s
  bound (the user may legitimately be doing 2FA in the sheet); plain
  backend calls get 60s, above the SDK's 30s per-request timeout.
- A deadline ends the phase as the new localized, retryable
  AuthError.timedOut (en + ja, in both the macOS and iOS catalogs); it
  maps to preserveCachedSession during validation, never signs out.
- AuthFlowCancellationGate in the vendored Stack SDK makes both
  interactive waits cancellation-responsive: task cancellation claims the
  continuation exactly once (CancellationError), tears down the system
  sheet, and a late system callback is dropped instead of double-resuming.
- CancellationError now maps to AuthError.cancelled (silent in the UI),
  not the generic "Something went wrong" server error.
- SignInView keeps the sign-in Task and shows a Cancel button while a
  flow is in flight, so a stuck sheet can always be backed out of; the
  post-sign-in hook is bounded too so it cannot hold the spinner after
  the session is published.
- Timeouts and OAuth failures log a redacted line via AuthDebugLog
  (phase + duration only, no tokens/emails).

Green half of the regression pair; the previous commit's cancellation
test now passes, and new virtual-clock tests cover the timeout, win, and
hung-hook paths without real waiting.

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

vercel Bot commented Jun 9, 2026 •

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
cmux Ready Ready Preview, Comment Jun 10, 2026 12:47pm
cmux-staging Building Building Preview, Comment Jun 10, 2026 12:47pm

@coderabbitai

coderabbitai Bot commented Jun 9, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR adds auth phase timeouts, cancellation-safe sign-in handling, cancelable UI sign-in tasks, timeout and cancel localizations, vendor OAuth/Apple flow cancellation guards, and tests covering bounded deadlines, cancellation mapping, and loading-state behavior.

Changes

Auth timeout and cancellation-safe sign-in

Layer / File(s) Summary
Timeout infrastructure types
Packages/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthTimeouts.swift, .../AuthPhase.swift
AuthTimeouts defines interactive and network deadlines with defaults, and AuthPhase labels auth operation phases used by timeout handling.
Timeout enforcement mechanism and error cases
Packages/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthPhaseTimeout.swift, .../AuthError.swift, .../AuthError+DisplaySafe.swift
Adds the phase timeout runner, the .timedOut auth error and localized description, and cancellation-first display-safe mapping to .cancelled.
AuthCoordinator timeout integration
Packages/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator.swift
The coordinator initializer now accepts timeouts and clock, adds runPhase, and bounds session validation, sign-in steps, user fetch, post-sign-in work, and team refresh by phase.
SignInView cancellation UI support
Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/SignInView.swift, ios/cmux/Resources/Localizable.xcstrings, Resources/Localizable.xcstrings
The sign-in view stores the active task, shows a cancel button while auth is running, handles cancelled email flows without showing errors, and adds localized timeout and cancel strings.
Vendor SDK cancellation-safe refactoring
vendor/stack-auth-swift-sdk-prerelease/Sources/StackAuth/StackClientApp.swift
OAuth and Apple sign-in continuations are gated for single resume and cancellation teardown, and token publication is centralized behind cancellation checks.
Test infrastructure and suites
Packages/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/ManualTestClock.swift, .../TestPhaseSignal.swift, .../HangingMagicLinkAuthClient.swift, .../HangingOAuthAuthClient.swift, .../AuthCoordinatorCancellationTests.swift, .../AuthCoordinatorTimeoutTests.swift, .../AuthErrorDisplaySafeTests.swift
Adds virtual-time and hanging-client test helpers, then verifies coordinator timeouts, cancellation mapping, cancelled OAuth behavior, loading-state cleanup, and timeout retryability behavior.

Sequence Diagrams

sequenceDiagram
  participant AuthCoordinator
  participant withAuthPhaseTimeout
  participant AuthClient
  participant Clock
  AuthCoordinator->>withAuthPhaseTimeout: runPhase(phase, duration, clock, operation)
  par operation
    withAuthPhaseTimeout->>AuthClient: start auth operation
    AuthClient-->>withAuthPhaseTimeout: value or error
  and deadline
    withAuthPhaseTimeout->>Clock: sleep until deadline
    Clock-->>withAuthPhaseTimeout: deadline reached
  end
  alt operation finishes first
    withAuthPhaseTimeout-->>AuthCoordinator: value
  else deadline wins
    withAuthPhaseTimeout-->>AuthCoordinator: AuthError.timedOut
  end
Loading
sequenceDiagram
  participant SignInView
  participant StackClientApp
  participant AuthFlowCancellationGate
  participant AuthSession
  participant TokenStore
  SignInView->>StackClientApp: start OAuth or Apple sign-in task
  StackClientApp->>AuthFlowCancellationGate: create gated continuation
  AuthFlowCancellationGate->>AuthSession: present auth session
  alt callback arrives
    AuthSession-->>AuthFlowCancellationGate: callback result
    AuthFlowCancellationGate-->>StackClientApp: resume once
    StackClientApp->>TokenStore: publishSessionTokens
  else task cancelled
    SignInView->>StackClientApp: cancel task
    StackClientApp->>AuthFlowCancellationGate: run teardown
    AuthFlowCancellationGate->>AuthSession: dismiss or cancel UI
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • manaflow-ai/cmux#5567: Also updates SignInView.swift around sign-in cancellation analytics and cancellation handling paths.

Poem

A bunny tapped a sign-in gate,
Then taught each hop to watch the clock.
If flows get stuck, they terminate,
If cancelled, doors unclick, unlock.
Soft strings now say, “Try once again.” 🐇

🚥 Pre-merge checks | ✅ 20 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 31.82% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (20 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and accurately describes the main changes: bounding every auth phase with deadlines, making OAuth waits cancellable, and adding a Cancel button escape hatch.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cmux Swift Actor Isolation ✅ Passed All new types have proper isolation: Sendable value types, @MainActor AuthCoordinator, Sendable protocols. No mutable shared Sendable refs or background context access issues found.
Cmux Swift Blocking Runtime ✅ Passed Production code uses injected clock.sleep (not Task.sleep) for deadline timeouts; pre-existing Task.sleep in signOut unchanged; test-only files contain deterministic blocking scaffolding only.
Cmux Expensive Synchronous Load ✅ Passed PR adds timeout infrastructure and cancellation handling, not expensive synchronous loaders. CMUXAuthIdentityStore.load() is lightweight JSON decode, unlike RestorableAgentSessionIndex.
Cmux Cache Substitution Correctness ✅ Passed Fresh reads always attempted before cached user; cache only used on transient failures with surviving tokens; cold/stale cache properly handled per documented ground-truth validation logic.
Cmux No Hacky Sleeps ✅ Passed PR contains only Swift code and localization resources; no TypeScript, JavaScript, or shell script changes. Swift is explicitly out of scope per runtime-no-hacky-sleeps.md.
Cmux Algorithmic Complexity ✅ Passed No algorithmic complexity violations: withAuthPhaseTimeout uses O(1) fixed task group; resolveTeamID scans bounded team lists (~10); test ManualTestClock handles 1-5 sleepers in test context.
Cmux Swift Concurrency ✅ Passed New code uses proper structured concurrency with withThrowingTaskGroup and managed Tasks. No Dispatch queues, Combine, completion-handlers, or fire-and-forget Tasks introduced.
Cmux Swift @Concurrent ✅ Passed All async functions properly isolated: AuthCoordinator @MainActor, network ops bounded via structured concurrency, no nonisolated async without @concurrent, no invalid @concurrent usage.
Cmux Swift File And Package Boundaries ✅ Passed New files under 400 lines with single responsibilities; AuthCoordinator at 757 lines (under 800 threshold) with +74 lines (under 250 limit); no mixed concerns; all code properly packaged.
Cmux Swift Logging ✅ Passed All logging uses AuthDebugLog with regex-based redaction of tokens/JWTs/emails; no print/NSLog/dump found; new logs only expose non-sensitive data.
Cmux User-Facing Error Privacy ✅ Passed User errors show only generic messages. Debug logging uses AuthDebugLog which redacts tokens, emails, and credentials. No vendor names, provider details, or secrets in user-visible text.
Cmux Full Internationalization ✅ Passed All user-facing strings use String(localized:) or equivalent APIs with complete en/ja translations in xcstrings catalogs; debug logs properly marked as developer-only.
Cmux Swiftui State Layout ✅ Passed SignInView adds @State Task for cancellation—value type (not ObservableObject), view-local, mutated in event handlers only, not passed to children.
Cmux Architecture Rethink ✅ Passed Uses structured concurrency; no timing repairs, polling, or state duplication. Clear AuthCoordinator ownership of isLoading/isAuthenticated. Required platform bridge documented. Complies.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PR does not add NSWindow, NSPanel, NSWindowController, SwiftUI Window, or WindowGroup. SignInView adds a cancel button within existing View structure, which is allowed.
Cmux Source Artifacts ✅ Passed All 17 changed files are hand-written source, tests, or localization catalogs—no local artifacts, build output, caches, or generated files detected.
Description check ✅ Passed PR description comprehensively addresses all required template sections: detailed 'Why' with structural holes explained, 'Testing' with red/green commits and test counts verified, 'Demo Video' not applicable to this backend/infrastructure change, 'Review Trigger' commands provided, 'Checklist' all items marked complete.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat-ios-signin-hang

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 and usage tips.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3d779d1292

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +610 to +614
try Task.checkCancellation()
if let tokenStoreOverride {
await client.setTokens(accessToken: accessToken, refreshToken: refreshToken, tokenStoreOverride: tokenStoreOverride)
} else {
await client.setTokens(accessToken: accessToken, refreshToken: refreshToken)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Close the cancellation window before writing tokens

When a user cancels or the phase timeout fires just after Task.checkCancellation() passes, the subsequent async hop into client.setTokens still runs and the token stores do not check cancellation before writing. In that timing window, the UI reports a cancelled/timed-out sign-in but live OAuth tokens can still be persisted, so the next restore can silently sign the user in behind the cancelled flow; the check needs to be atomic with the write or followed by cancellation-aware rollback that cannot clear a newer flow's tokens.

Useful? React with 👍 / 👎.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/CmuxMobileShellUI/Sources/CmuxMobileShellUI/SignInView.swift`:
- Line 261: Add the missing localization key referenced by SignInView.swift:
insert "mobile.signIn.cancel" into Resources/Localizable.xcstrings with English
and Japanese translations (matching the existing key in
ios/cmux/Resources/Localizable.xcstrings) so both locale catalogs are complete;
update the en and ja strings entries in Resources/Localizable.xcstrings to
include the appropriate translations for "Cancel" and the Japanese equivalent.
🪄 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: c23e1566-56d6-4ec7-b44f-e1f2edb49257

📥 Commits

Reviewing files that changed from the base of the PR and between eaf6ee3 and 3d779d1.

📒 Files selected for processing (17)
  • Packages/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator.swift
  • Packages/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthError+DisplaySafe.swift
  • Packages/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthError.swift
  • Packages/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthPhase.swift
  • Packages/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthPhaseTimeout.swift
  • Packages/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthTimeouts.swift
  • Packages/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/AuthCoordinatorCancellationTests.swift
  • Packages/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/AuthCoordinatorTimeoutTests.swift
  • Packages/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/AuthErrorDisplaySafeTests.swift
  • Packages/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/HangingMagicLinkAuthClient.swift
  • Packages/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/HangingOAuthAuthClient.swift
  • Packages/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/ManualTestClock.swift
  • Packages/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/TestPhaseSignal.swift
  • Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/SignInView.swift
  • Resources/Localizable.xcstrings
  • ios/cmux/Resources/Localizable.xcstrings
  • vendor/stack-auth-swift-sdk-prerelease/Sources/StackAuth/StackClientApp.swift

Button {
signInTask?.cancel()
} label: {
Text(L10n.string("mobile.signIn.cancel", defaultValue: "Cancel"))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify mobile.signIn.cancel key exists with translations in both xcstrings files

echo "=== Checking Resources/Localizable.xcstrings ==="
fd -t f 'Localizable.xcstrings' Resources -x sh -c 'echo "File: {}"; cat "{}" | python3 -c "
import sys, json
data = json.load(sys.stdin)
key = \"mobile.signIn.cancel\"
strings = data.get(\"strings\", {})
if key not in strings:
    print(f\"  ERROR: Key {key} not found\")
    sys.exit(1)
entry = strings[key]
localizations = entry.get(\"localizations\", {})
print(f\"  Key found with localizations: {list(localizations.keys())}\")
for loc, val in localizations.items():
    sv = val.get(\"stringUnit\", {}).get(\"value\", \"(empty)\")
    print(f\"    {loc}: {sv[:50]}...\")
"'

echo ""
echo "=== Checking ios/cmux/Resources/Localizable.xcstrings ==="
fd -t f 'Localizable.xcstrings' ios/cmux/Resources -x sh -c 'echo "File: {}"; cat "{}" | python3 -c "
import sys, json
data = json.load(sys.stdin)
key = \"mobile.signIn.cancel\"
strings = data.get(\"strings\", {})
if key not in strings:
    print(f\"  ERROR: Key {key} not found\")
    sys.exit(1)
entry = strings[key]
localizations = entry.get(\"localizations\", {})
print(f\"  Key found with localizations: {list(localizations.keys())}\")
for loc, val in localizations.items():
    sv = val.get(\"stringUnit\", {}).get(\"value\", \"(empty)\")
    print(f\"    {loc}: {sv[:50]}...\")
"'

Repository: manaflow-ai/cmux

Length of output: 499


Add mobile.signIn.cancel to Resources/Localizable.xcstrings.

SignInView.swift references mobile.signIn.cancel; the key exists in ios/cmux/Resources/Localizable.xcstrings but is missing from Resources/Localizable.xcstrings. Add the entry (with en/ja translations) to keep all supported locale catalogs complete.

🤖 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/CmuxMobileShellUI/Sources/CmuxMobileShellUI/SignInView.swift` at
line 261, Add the missing localization key referenced by SignInView.swift:
insert "mobile.signIn.cancel" into Resources/Localizable.xcstrings with English
and Japanese translations (matching the existing key in
ios/cmux/Resources/Localizable.xcstrings) so both locale catalogs are complete;
update the en and ja strings entries in Resources/Localizable.xcstrings to
include the appropriate translations for "Cancel" and the Japanese equivalent.

Source: Coding guidelines

@greptile-apps

greptile-apps Bot commented Jun 9, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes iOS sign-in hanging forever by bounding every loading phase with a deadline and making the interactive OAuth waits (ASWebAuthenticationSession, ASAuthorizationController) cancellation-responsive, with a Cancel button as an escape hatch.

  • AuthCoordinator now races all eight sign-in/session phases (OAuth, send/verify code, password, fetch user, validate session, list teams, post-sign-in hook) through withAuthPhaseTimeout on an injected clock; missed deadlines surface as the new localized, retryable AuthError.timedOut; validation timeouts preserve the cached session.
  • Vendored Stack SDK adds AuthFlowCancellationGate so both interactive flows respond to task cancellation and dismiss their system UI; web OAuth token exchange now runs in the caller's task; publishSessionTokens refuses writes on a cancelled task to prevent silent session persistence.
  • SignInView tracks the in-flight Task, shows a localized Cancel button whenever isAuthInProgress, and swallows AuthError.cancelled silently (analytics ios_sign_in_cancelled only); AuthError(displaySafe:) maps CancellationError and all errors on a cancelled task to .cancelled.

Confidence Score: 4/5

Safe to merge after adding the missing locale translations to the macOS string catalog — everything else is well-structured and covered by tests.

The macOS Resources/Localizable.xcstrings was touched to add auth.error.timed_out, but only en and ja were provided. The catalog already carries 20 locales; the 18 missing entries mean macOS users in Arabic, German, French, Chinese, and 14 other locales will silently fall back to the English default string whenever a sign-in phase times out. Everything else — the structured task-group timeout, cancellation gate, token-write guard, and SwiftUI cancel affordance — is correct and well-tested.

Resources/Localizable.xcstrings — new auth.error.timed_out key needs translations for all 18 non-en/ja locales already present in the macOS catalog.

Important Files Changed

Filename Overview
Packages/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator.swift Wraps every loading-phase call (OAuth, send/verify code, password, fetch user, validate session, list teams, post-sign-in hook) in runPhase with injected timeouts; adds AuthDebugLog; no logic or isolation issues found.
Packages/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthPhaseTimeout.swift New withAuthPhaseTimeout using a structured throwing task group; correct defer { group.cancelAll() } on all exit paths; private sentinel AuthPhaseDeadlineExceeded cleanly distinguishes deadline vs other errors.
Packages/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthError+DisplaySafe.swift Adds CancellationError/Task.isCancelled arm that maps cancellation (including URLSession-wrapped forms) to .cancelled; contract locked by two new tests.
vendor/stack-auth-swift-sdk-prerelease/Sources/StackAuth/StackClientApp.swift Adds AuthFlowCancellationGate (NSLock-guarded, justified by synchronous onCancel), cancellation-responsive gates on both ASWebAuthenticationSession and ASAuthorizationController, structured token exchange, and publishSessionTokens chokepoint; NSLock justification should be surfaced inline per blocking-runtime rule.
Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/SignInView.swift Tracks in-flight task via @State var signInTask, shows localized Cancel button via cancelSignInButton computed view, swallows AuthError.cancelled on all code paths; clean SwiftUI state management.
Resources/Localizable.xcstrings Adds auth.error.timed_out with only en/ja; macOS catalog already carries 20 locales — 18 are missing from the new key.
ios/cmux/Resources/Localizable.xcstrings Adds auth.error.timed_out and mobile.signIn.cancel for both en and ja; iOS catalog only supports those two locales so coverage is complete.
Packages/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/ManualTestClock.swift Virtual clock for deadline tests; NSLock justified for synchronous onCancel; correct pre-park/post-park cancellation race handling and waitUntilSleepers rendezvous.
Packages/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/AuthCoordinatorTimeoutTests.swift Covers stuck OAuth timeout, wedged sendCode timeout, deadline-child cleanup (clock frozen at zero), bounded post-sign-in hook, and timedOut-as-preserveCachedSession invariant.

Sequence Diagram

sequenceDiagram
    participant SV as SignInView
    participant AC as AuthCoordinator
    participant PT as withAuthPhaseTimeout
    participant SC as StackClientApp
    participant Gate as AuthFlowCancellationGate
    participant Sys as System UI (ASAuth*)

    SV->>AC: signInWithApple()
    AC->>PT: runPhase(.oauth, 300s)
    PT->>SC: signInWithOAuth()
    SC->>Gate: activate(continuation, onCancel: teardown)
    SC->>Sys: start sheet
    Note over Sys: User may interact

    alt OAuth completes
        Sys-->>Gate: resume(returning: callbackURL)
        Gate-->>SC: callbackURL
        SC->>SC: callOAuthCallback()
        SC-->>PT: success
        PT-->>AC: returns
        AC->>AC: applySignedInUser()
        AC->>SV: "isAuthenticated = true"
    else Deadline fires
        PT->>PT: deadline child wakes
        PT-->>AC: throws AuthError.timedOut
        AC-->>SV: "isLoading = false, show timedOut error"
    else User taps Cancel
        SV->>SV: signInTask.cancel()
        Gate->>Gate: cancel() CancellationError
        Gate->>Sys: teardown: session.cancel()
        SC-->>PT: CancellationError
        AC->>AC: AuthError(displaySafe) to .cancelled
        AC-->>SV: .cancelled swallowed silently
    end
Loading

Reviews (3): Last reviewed commit: "Merge branch 'main' into feat-ios-signin..." | Re-trigger Greptile

Comment on lines +345 to 353
if gate.isFinished {
// Cancelled between the pre-start check and start(): the
// teardown already ran against an empty holder, so the
// just-presented sheet is dismissed here instead. This
// re-check runs synchronously on the main actor, so it
// cannot interleave with the teardown's main-actor task.
sessionHolder.session?.cancel()
sessionHolder.session = nil
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The comment claims this re-check "runs synchronously on the main actor," but StackClientApp is a plain actor — not @MainActor. The post-start check therefore runs on the StackClientApp actor's executor, while the teardown closure runs a Task { @MainActor in … }. These are two different executors, so they can interleave: the teardown task may run between the session.start() call and this guard, or concurrently with it. In practice the NSLock gate ensures the continuation is resumed exactly once, and double-cancelling ASWebAuthenticationSession is safe (idempotent), so there is no functional bug — but the comment describes a guarantee the code does not actually provide. A future reader relying on this comment to decide whether additional synchronization is needed would be misled.

Suggested change
if gate.isFinished {
// Cancelled between the pre-start check and start(): the
// teardown already ran against an empty holder, so the
// just-presented sheet is dismissed here instead. This
// re-check runs synchronously on the main actor, so it
// cannot interleave with the teardown's main-actor task.
sessionHolder.session?.cancel()
sessionHolder.session = nil
}
if gate.isFinished {
// Cancelled between the pre-start check and start(): the
// teardown already ran against an empty holder, so the
// just-presented sheet is dismissed here instead.
// Note: this runs on the StackClientApp actor, not the
// main actor. The teardown Task { @MainActor in … } may
// run concurrently, but double-cancelling the session is
// safe (idempotent) and the gate ensures the continuation
// is resumed exactly once.
sessionHolder.session?.cancel()
sessionHolder.session = nil
}

Comment on lines +444 to +445
try await client.currentUser(throwOnMissing: true)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Raw error logged at .public privacy via AuthDebugLog

log.log("auth.oauth provider=\(provider) failed: \(error)") routes through AuthDebugLog, which passes the redacted string to logger.log(…, "\(redactedMessage, privacy: .public)"). The redaction patterns strip tokens, JWTs, and email addresses, but not the message field of StackAuthError or OAuthError — those can include server-controlled strings or the verbose Apple SIWA diagnostic messages hardcoded in AppleSignInDelegate (up to ~300 characters of Xcode/developer.apple.com instructions). Using .public means this content is visible to any log collector without the usual privacy: .private scrubbing. The existing error logs in validateCachedSession and tryAutoLogin use authLog.error(…, privacy: .private) for the same reason. Either route this through authLog with privacy: .private, or extend AuthDebugLog.redacted to strip the message field from Stack SDK error representations before the .public sink sees them.

Comment on lines +40 to +45
// Cancel the loser on every exit path; the group then joins it.
defer { group.cancelAll() }
do {
guard let first = try await group.next() else {
throw AuthError.timedOut
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 nil branch of group.next() is unreachable

With exactly two child tasks added (the operation and the deadline), the first call to group.next() can never return nil — nil signals an empty group, but both children are still pending at that point. The else branch mapping to AuthError.timedOut is dead code. While it doesn't affect correctness (the nil case would map to the same error as a real timeout), it could mislead a future reader into thinking group.next() has some documented empty-return path here.

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!

Comment on lines 18 to +30
return
}
if error is CancellationError || Task.isCancelled {
// A cancelled flow (the user backed out, or the sheet's task was
// torn down) is not a failure to render. Mapping it to the generic
// server error would flash "Something went wrong" after every
// deliberate cancel. The `Task.isCancelled` arm matters because
// URLSession-backed phases surface task cancellation as
// `URLError(.cancelled)` (or a Stack transport error wrapping it),
// not as `CancellationError`: this mapping always runs in the
// cancelled flow's own catch, so any error caught on a cancelled
// task is the cancellation, not an independent failure.
self = .cancelled

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Task.isCancelled check swallows all errors on a cancelled task, including real auth failures

The mapping if error is CancellationError || Task.isCancelled { self = .cancelled } is intentional (the PR description explains the URLSession surface), and the two new tests (anyErrorCaughtOnACancelledTaskMapsToCancelled, urlCancellationOnALiveTaskStaysNetworkError) lock the contract well. One narrow edge case worth noting in the comment: if a real auth error (e.g., serverError(401, "auth_failed")) is thrown at the exact moment the task is externally cancelled — cancelled wins and the error is silently dropped as .cancelled. In practice the window is tiny and the user chose to cancel, so the silent outcome is acceptable UX, but this could become surprising if AuthCoordinator is ever called from a context where task cancellation originates from something other than a user action (e.g., app-lifecycle teardown). A brief note in the comment would help the next reader not widen this mapping without understanding the trade-off.

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!

@lawrencecchen
lawrencecchen merged commit 0baf6ec into main Jun 10, 2026
32 of 36 checks passed

This branch was successfully deployed

1 active deployment
Preview – cmux — 2f8bbf5c Deployed Jun 10, 2026 by vercel[bot]
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