Skip to content

Coalesce iOS presence recovery during startup reconnect - #10005

Closed
azooz2003-bit wants to merge 3 commits into
mainfrom
feat-ios-presence-startup-race
Closed

azooz2003-bit wants to merge 3 commits into
mainfrom
feat-ios-presence-startup-race

Conversation

@azooz2003-bit

@azooz2003-bit azooz2003-bit commented Aug 11, 2026 •

Copy link
Copy Markdown
Collaborator

Mark’s beta logs showed an authenticated presence snapshot starting a second Iroh recovery while launch reconnect was already dialing. The nested reconnect advanced the stored-Mac generation, canceled launch, and retried the same route.

This coalesces automatic recovery triggers into the active startup restore while keeping manual retry and connection-method changes as explicit replacements. It also logs the coalesced trigger for future diagnostics.

Regression history:


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Prevent nested reconnects during iOS startup by coalescing automatic recovery triggers into the active stored‑Mac restore. This avoids canceling the in‑flight dial, stops generation churn, and prevents duplicate Iroh dials.

  • Bug Fixes
    • Coalesce automatic triggers (e.g., presence push) during isReconnectingStoredMac; keep manual retry and connection‑method changes as explicit replacements.
    • Log coalesced triggers via MobileDebugLog.anchormux with the current stored‑Mac generation.
    • Add tests covering presence coalescing and explicit startup replacements (manual retry and connection‑method change).

Written for commit 9c2d258. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes
    • Improved automatic connection recovery to avoid duplicate reconnect attempts while one is already in progress.
    • Preserved the active startup connection attempt when presence updates occur during recovery.
    • Manual retries and connection-method changes now reliably replace the active attempt and start a fresh recovery operation.
    • Improved recovery handling so the latest connection attempt remains active after an earlier attempt fails.

@coderabbitai

coderabbitai Bot commented Aug 11, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

recoverMobileConnection now coalesces automatic recovery triggers during an active stored-Mac reconnect. Manual retries and connection-method changes can still replace the operation. Tests cover both paths.

Changes

Connection recovery

Layer / File(s) Summary
Recovery trigger arbitration
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ConnectionRecovery.swift
When a stored-Mac reconnect is active, automatic triggers are logged with the reconnect generation and ignored. Manual retries and connection-method changes continue through the replacement flow.
Recovery replacement regression coverage
Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/IrohConnectionRecoveryOwnerTests.swift
Tests verify that presence recovery preserves the startup Iroh dial, while explicit recovery creates a replacement owner, advances the generation, connects the replacement, and fails the superseded attempt.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
  participant RecoveryTrigger
  participant recoverMobileConnection
  participant StartupIrohDial
  participant ReplacementIrohDial
  RecoveryTrigger->>recoverMobileConnection: Send recovery trigger
  recoverMobileConnection->>StartupIrohDial: Preserve automatic recovery
  RecoveryTrigger->>recoverMobileConnection: Request explicit recovery
  recoverMobileConnection->>StartupIrohDial: Supersede active attempt
  recoverMobileConnection->>ReplacementIrohDial: Start replacement attempt
  ReplacementIrohDial-->>recoverMobileConnection: Connect successfully
Loading

Possibly related PRs

  • manaflow-ai/cmux#9979: Both PRs modify Iroh connection recovery behavior and its tests, but address different logic.
🚥 Pre-merge checks | ✅ 24 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description explains the change and cites verification runs, but it omits the required Testing, Demo Video, Review Trigger, and Checklist sections. Add the required template sections, include test and manual verification details, provide a demo link or explain why none applies, and complete the checklist.
✅ Passed checks (24 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 The only production addition is inside the existing @MainActor MobileShellComposite extension; it adds no models, protocols, Sendable references, or background UI access. New tests are allowed.
Cmux Swift Blocking Runtime ✅ Passed The production diff adds only trigger arbitration and logging; it adds no blocking or timing primitive. The only new pollUntil use is test-only scaffolding, which the rule allows.
Cmux Browser Automation Off-Main ✅ Passed The PR changes only iOS recovery code and tests; the rule-scoped browser automation files are unchanged, with no browser/WebKit terms in the cumulative diff.
Cmux Expensive Synchronous Load ✅ Passed The only production additions are recovery-trigger cases and a diagnostic log; no expensive agent-history loader, file read, directory scan, or JSON parsing was added or moved.
Cmux Cache Substitution Correctness ✅ Passed The full PR diff only adds recovery-trigger arbitration and tests; it does not replace an authoritative persistence, history, undo, or snapshot read with a cache.
Cmux No Hacky Sleeps ✅ Passed The diff changes only Swift production code and Swift tests; it adds no covered non-Swift sleep, timer, delayed dispatch, polling, or wall-clock wait.
Cmux Algorithmic Complexity ✅ Passed The production diff adds a fixed-enum switch and one diagnostic log in recoverMobileConnection; it adds no collection scan, sort, filter, join, or nested iteration. Added collection use is test-only.
Cmux Swift Concurrency ✅ Passed The production diff adds only synchronous trigger coalescing and logging. The sole new Task is a stored, awaited XCTest race helper, which the rule allows.
Cmux Swift @Concurrent ✅ Passed The PR adds only synchronous logic in an existing @MainActor extension and @MainActor regression tests; it adds no @concurrent/nonisolated async work or invalid annotation.
Cmux Swift Package Boundaries ✅ Passed The 20-line change is @MainActor MobileShellComposite lifecycle arbitration using stored-Mac state and its owner; it is app-specific composition, not reusable domain logic requiring a new package t...
Cmux Swiftpm Lockfiles ✅ Passed The patch changes only MobileShell Swift source and tests; it changes no Package.swift, Package.resolved, .gitignore, workflow, or Xcode project package references.
Cmux Swift Logging ✅ Passed The only added log uses existing MobileDebugLog.anchormux; its implementation is #if DEBUG and logs only trigger names and a reconnect generation.
Cmux User-Facing Error Privacy ✅ Passed The only added production text is a DEBUG-only MobileDebugLog diagnostic with a recovery trigger and generation; no user-facing error, alert, command output, or API body changed.
Cmux Full Internationalization ✅ Passed The production diff only narrows an existing recovery switch; tests add no UI text, and the existing MobileDebugLog.anchormux call is DEBUG-only. No localization or message catalogs changed.
Cmux Swiftui State Layout ✅ Passed The diff only changes connection-recovery logic and tests; it adds no SwiftUI views, ObservableObject/@published state, GeometryReader, lazy/list row store references, or render-time state mutation.
Cmux Architecture Rethink ✅ Passed The production diff adds no timing, blocking, observer, lock, side-channel, or duplicate wiring; it uses existing reconnect state and generation with explicit owner rules, and tests use allowed pol...
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed The PR changes only iOS connection-recovery logic and recovery tests; the diff adds no NSWindow, NSPanel, NSWindowController, SwiftUI Window, or WindowGroup code.
Cmux Source Artifacts ✅ Passed The PR changes only one hand-written Swift source file and one Swift test file; the diff adds no logs, caches, scratch directories, build output, screenshots, recordings, or binary artifacts.
Cmux No Test Or Debug Seam In Production Source ✅ Passed The only Sources diff changes trigger handling and adds a diagnostic log; it adds no test/debug member, guard, accessor, or widened visibility. New seams remain in Tests/ with @testable import.
Cmux No Ambient Global State ✅ Passed The production diff only adds trigger-coalescing logic inside MobileShellComposite.recoverMobileConnection; it adds no top-level function, mutable global, static namespace, or singleton.
Title check ✅ Passed The title clearly summarizes the primary change: coalescing iOS presence recovery during startup reconnect.
✨ 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-presence-startup-race

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.

@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: 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/Sources/CmuxMobileShell/MobileShellComposite`+ConnectionRecovery.swift:
- Around line 92-103: Update the trigger switch in the isReconnectingStoredMac
and inactive connectionRecoveryOwner branch to list every current automatic
RecoveryTrigger case explicitly instead of using default; keep .manual and
.connectionMethodChanged as the non-coalesced cases, and preserve the existing
logging and return behavior for the enumerated automatic cases.

In
`@Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/IrohConnectionRecoveryOwnerTests.swift`:
- Around line 117-140: Add held-startup-dial recovery tests for both `.manual`
and `.connectionMethodChanged`, alongside
`presenceDuringStartupReconnectDoesNotSupersedeTheActiveDial`. For each trigger,
assert the recovery owner replaces the active startup dial, the stored reconnect
generation changes, and the original dial is superseded; release the held
attempts and verify exactly one replacement attempt reaches `.connected`.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 18b343fd-07ca-4c59-9e34-742528e0b2f3

📥 Commits

Reviewing files that changed from the base of the PR and between 006d2c8 and c144998.

📒 Files selected for processing (2)
  • Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ConnectionRecovery.swift
  • Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/IrohConnectionRecoveryOwnerTests.swift

@cursor

cursor Bot commented Aug 11, 2026

Copy link
Copy Markdown

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@azooz2003-bit

Copy link
Copy Markdown
Collaborator Author

Verification on 9c2d258dc7:

The aggregate package command remains red from unrelated concurrent-suite failures in MobileMacConnectionPoolTests and terminal viewport/input tests. The three changed connection-recovery tests all passed before those failures.

@azooz2003-bit

Copy link
Copy Markdown
Collaborator Author

Hosted iPhone-simulator connection smoke also passes on the PR head: cmuxUITests/cmuxUITests/testManualHostConnectsAndNavigatesToWorkspace, 1 test with 0 failures.

https://github.com/manaflow-ai/cmux/actions/runs/31527235580/job/93899147719

@azooz2003-bit

Copy link
Copy Markdown
Collaborator Author

Superseded by #10090, which consolidates startup recovery, per-peer session ownership, diagnostics, stale discovery refresh, and the reliability workload.

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