Keep the admitted session when a superseded control owner's dial lands

[azooz2003-bit]

Summary

Every iOS connect to a Mac paid for two QUIC handshakes, and a reconnect after the Mac stalled took about 17 s longer than it needed to. The phone journal shows the pattern on every fresh launch: the first session becomes ready and is retired with explicit-redial in the same millisecond, next to client-runtime control-lane-busy, and a second dial follows.

The cause is in IrxControlByteTransport. When a newer RPC client generation replaces one whose dial is still in flight, the old transport is closed; when its dial lands it finds itself closed and calls closeEstablishedPair, which retires the freshly admitted connection. The replacement owner's claim is refused while that happens (control-lane-busy), so it dials again.

That owner never read or wrote the control lane, so no EOF reached the Mac and the session is intact. It now only releases its claim (onClose with retiresConnection: false) and leaves the session to the owner that replaced it. Owners that did use the lane still retire the session on close, which is the rule #12324 introduced because the Mac treats control-stream EOF as session termination. Revocation and scope changes already close the engine directly. #11891 described the intended behavior the same way: a closing RPC client "releases only its own Iroh claims instead of forcing a QUIC session replacement… including close-during-connect".

Also fixes a flaky test from #14295: legacy hello without the barrier capability admits immediately dropped the server connection right after writing the admit, and the QUIC close could discard the admit before the client read it.

Testing

• IrxControlSupersededOwnerTests (new, live QUIC loopback) is committed first (a520084) and fails there: the superseded owner retires the connection and the replacement owner cannot use it. After the fix (0ece0aa) the claim is released without retiring, the connection stays open, and a replacement transport sends bytes that the server reads.
• closing while establishment is in flight releases the owner once in IrxLiveQUICTests is updated to the new semantics (released once, not retired, connection open).
• swift test in Packages/Shared/CmuxIrxTransport: 216 tests; the NAT barrier suite passed 6 of 6 repeated runs after the flake fix. One of five full runs hit IrxLivenessTests/nativePeerDeathIsDeferredInBackgroundAndRecoveredOnForeground (elapsed 3.0 s against a 2 s bound) on a machine with a load average above 500; that test does not touch this path.
• scripts/check-test-determinism.py: 0 findings.
• Found during an overnight relay soak of Give only the focused terminal its own event stream #15135 on tag atstr (journal evidence above). Not yet rebuilt into that soak; the phone-side connect is the next thing to check live.

Changelog

Fixed: The iOS app no longer connects twice when it opens a connection to a Mac, which also shortens reconnects after the Mac was briefly unreachable

🤖 Generated with Claude Code

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

---

Summary by cubic

Fixes every iOS connect to a Mac paying for two QUIC handshakes. When a newer RPC client generation replaces one whose dial is still in flight, the closed transport now releases its claim (retiresConnection: false) instead of retiring the freshly admitted session, so the replacement owner no longer hits control-lane-busy and dials again. Owners that actually used the control lane still retire the session on close; revocation and scope changes still close the engine directly.

• Adds a live-loopback test proving a superseded owner leaves a usable session for its replacement.
• Fixes a flaky IrxNatBarrierTests case that dropped the server connection before the client read the admit.

^{Written for commit 36bfbe2. Summary will update on new commits.}

Summary by CodeRabbit

• Bug Fixes
  • Closing a transport while connection setup is in progress now releases its ownership without shutting down the underlying connection. A replacement transport can reuse the session and continue sending data.
  • The pending connection attempt still reports that it was closed, while the established connection remains available for reuse.

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: df564f39-76cb-4eea-bfb3-1611f725b06d

📥 Commits

Reviewing files that changed from the base of the PR and between b76db12 and 36bfbe2.

📒 Files selected for processing (4)

• Packages/Shared/CmuxIrxTransport/Sources/CmuxIrxTransport/IrxControlByteTransport.swift
• Packages/Shared/CmuxIrxTransport/Tests/CmuxIrxTransportTests/IrxControlSupersededOwnerTests.swift
• Packages/Shared/CmuxIrxTransport/Tests/CmuxIrxTransportTests/IrxLiveQUICTests.swift
• Packages/Shared/CmuxIrxTransport/Tests/CmuxIrxTransportTests/IrxNatBarrierTests.swift

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

---

📝 Walkthrough

Walkthrough

When a control transport closes during establishment, it records the established connection and releases its owner claim without retiring the connection. The connect still throws IrxConnectionError.closed(nil). Tests cover replacement transport use and retain the server connection during legacy admission.

Changes

Control transport owner release

Layer / File(s)	Summary
Release owner without retiring connection Packages/Shared/CmuxIrxTransport/Sources/CmuxIrxTransport/IrxControlByteTransport.swift, Packages/Shared/CmuxIrxTransport/Tests/CmuxIrxTransportTests/IrxLiveQUICTests.swift	The closed-during-establishment path releases the owner claim without retiring or closing the admitted connection. The existing test now checks this behavior.
Verify replacement transport use Packages/Shared/CmuxIrxTransport/Tests/CmuxIrxTransportTests/IrxControlSupersededOwnerTests.swift	A gated dial test checks that the pending connect throws, the close callback runs once without retiring the connection, and a replacement transport can send data on the same session.

Legacy admission test connection lifetime

Layer / File(s)	Summary
Retain server connection through admission Packages/Shared/CmuxIrxTransport/Tests/CmuxIrxTransportTests/IrxNatBarrierTests.swift	The server task returns the admission result with its connection. The test verifies the admitted peer and closes the retained connection.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: austinywang

Merge Risk: ⚪ Minimal · up to 36bfb

The replacement owner’s session is protected from the superseded owner’s release. The change is mergeable after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 36bfb

The replacement client can reuse the admitted session without bypassing admission. Owner-specific cleanup and revocation controls remain in place. The behavior when a client closes during connection setup without a replacement is less directly covered.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The changed outcome is bounded to an already-admitted peer session rather than a new admission or broader service authority; a replacement must still obtain the session's control-lane claim.

Trust Boundaries and Controls

• observed — iOS revocation stops peer engines directly. Its stale close callback releases only its captured owner ID; on Mac, release checks the stored owner before stopping or removing a session.

Resilience and Maintainability Implications

• observed — The Mac close callback stops its owner-matched engine even when the transport supplies a non-retiring release; the iOS path instead retains engine ownership unless a separate retirement or lifecycle stop occurs.

🚥 Pre-merge checks | ✅ 24 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (24 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main behavior change: preserving the admitted session when a superseded control owner’s dial completes.
Description check	✅ Passed	The description is mostly complete. It provides a detailed problem statement, resulting behavior, test coverage, known limitation, and changelog entry. It omits the template’s Demo Video and Checklist…
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 Cloud Persistent Session And Early Input	✅ Passed	The pull request changes only IrxControlByteTransport lifecycle handling and related QUIC tests. It does not add Cloud terminal creation, cmux-tui, Ghostty, renderer, PTY, shell-readiness, input-a…
Cmux Swift Actor Isolation	✅ Passed	The production diff only changes behavior inside the existing public actor IrxControlByteTransport: it replaces the in-flight-close cleanup call with an existing onClose callback and adds comments…
Cmux Swift Blocking Runtime	✅ Passed	The only production Swift change awaits the existing asynchronous onClose callback and removes the close/retirement call. It adds no semaphore, blocking wait, sleep, delayed dispatch, polling loop, …
Cmux Browser Automation Off-Main	✅ Passed	PASS: The PR changes only CmuxIrxTransport transport logic and QUIC tests. The authoritative diff contains no browser.* commands, WebKit/AppKit access, socket-worker routing, or processV2Command chang…
Cmux Expensive Synchronous Load	✅ Passed	The only production change is in IrxControlByteTransport.establishedPair(). It calls onClose with retiresConnection: false after an in-flight dial completes. The diff adds no agent-history loade…
Cmux Cache Substitution Correctness	✅ Passed	The PR does not perform a cache substitution. The only production change replaces closeEstablishedPair with onClose(..., false) after an in-flight transport establishment, while retaining the admi…
Cmux No Hacky Sleeps	✅ Passed	PASS — The pull request changes only Swift source and Swift tests. The rule explicitly scopes this check to TypeScript, JavaScript, shell, and non-Swift build/runtime scripts. Swift timing primitives …
Cmux Algorithmic Complexity	✅ Passed	PASS. The only production change is in IrxControlByteTransport.swift:440-449: it assigns lastConnection and invokes onClose once with false instead of calling closeEstablishedPair. It adds n…
Cmux Swift Concurrency	✅ Passed	The diff does not introduce a forbidden legacy async pattern. The production change only awaits the existing onClose async callback and does not add Dispatch, Combine, or completion-handler APIs. Th…
Cmux Swift @Concurrent	✅ Passed	The PR does not violate the Swift concurrency annotation rule. The only production change is inside the existing actor-isolated private func establishedPair() async in IrxControlByteTransport; its…
Cmux Swift Package Boundaries	✅ Passed	The only production Swift change is in Packages/Shared/CmuxIrxTransport/Sources/CmuxIrxTransport/IrxControlByteTransport.swift, inside the existing CmuxIrxTransport SwiftPM target. Its package man…
Cmux Swiftpm Lockfiles	✅ Passed	The pull request changes only Swift source and test files. It does not change a Package.swift dependency, Package.resolved, .gitignore, workflow, or Xcode project/package reference. Therefore, the Swi…
Cmux Swift Logging	✅ Passed	The PR adds no production logging. The only runtime change replaces connection cleanup with onClose?(established.0, closeCode, false) in IrxControlByteTransport.swift; it adds no print, `debugPr…
Cmux User-Facing Error Privacy	✅ Passed	The production diff changes an internal onClose callback from retiring the connection to releasing the claim. It adds only developer comments and preserves the generic `IrxConnectionError.closed(nil…
Cmux Full Internationalization	✅ Passed	PASS. The authoritative diff changes one production Swift transport file and three test files. The production change adds only developer comments and changes control flow to call `onClose?(..., false)…
Cmux Swiftui State Layout	✅ Passed	PASS. The PR changes only IrxControlByteTransport and QUIC transport tests. The diff adds no SwiftUI views, ObservableObject/@Published, geometry readers, lazy/list row stores, or render-time st…
Cmux Architecture Rethink	✅ Passed	The production diff is a small local ownership fix in IrxControlByteTransport.establishedPair(). It releases the superseded owner claim with onClose(..., false) and leaves the admitted session for…
Cmux Swift Auxiliary Window Close Shortcuts	✅ Passed	PASS: The PR changes only IrxControlByteTransport and transport tests under Packages/Shared/CmuxIrxTransport. The diff adds or changes no NSWindow, NSPanel, NSWindowController, SwiftUI `Wind…
Cmux Source Artifacts	✅ Passed	All four changed paths are intentional Swift source or test files under the existing package layout. The diff adds no logs, screenshots, recordings, temporary or cache directories, dependency checkout…
Cmux No Test Or Debug Seam In Production Source	✅ Passed	PASS. The only changed production file is Packages/Shared/CmuxIrxTransport/Sources/CmuxIrxTransport/IrxControlByteTransport.swift. Its diff changes establishment cleanup to call the existing `onClos…

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Dogfood build of 36bfbe28ca7251e66b64a7bea2a93865614a9729

cmux DEV pr-15197-36bfbe28.app

The link opens this exact commit in the cmux dev menu bar app. The build starts on each push and the page waits until it is ready; a newer push replaces it. It signs in against production, so Cloud or backend changes still need a tagged build with a development backend.

[teamleaderleo]

Review (subagent, correctness-first; I re-verified the load-bearing claims against the diff)

Holding this one. The fix is right for the ordering it covers, but the mirror-image ordering still runs the destructive teardown this PR exists to remove.

Finding 1 (blocking): only one side of the race is fixed.
The new guard in establishedPair() fires when isClosed is already true as task.value resolves (IrxControlByteTransport.swift:439-450). The other interleaving: establish() finishes first, pair = established commits (no await between the guard and the assignment, so that part is atomic), and a superseding close() arrives just after. That call goes through the untouched close() (:208-219) into closeEstablishedPair() (:457-478), where

let retiresConnection = !controlTerminationObserved && !connectionWasAlreadyClosed

is true precisely because this owner never read or wrote the lane. So it retires the shared admitted connection and calls lane.writer.finish() / lane.reader.stop() - the exact teardown and forced re-dial the PR is meant to eliminate. This is reachable in practice: CmxIrohDeferredByteTransport.swift:53-61 has D.connect() fail its post-await guard !closed (line 58) and then call await connected.close() (line 59).

The production diff is a single hunk at establishedPair(); close() and closeEstablishedPair() are not touched at all. A fix likely needs the same "this owner did no I/O, hand the claim back" decision inside closeEstablishedPair, not only in the dial path.

Finding 2: the tests only cover the fixed ordering.
Both IrxControlSupersededOwnerTests.swift and the modified IrxLiveQUICTests.swift:385-455 sequence establishmentStarted.wait() -> transport.close() -> release the dial. Worth adding the reverse: let the dial complete and install pair, then close.

Checked and clean:

• No counter/epoch/timestamp ordering hazard. Ownership is decided by MobileIrxControlLaneClaims.claim/release, mutated only inside the MobileIrxRuntimeComposition actor, so claims serialize and cannot tie.
• dialGeneration/epoch/activityGeneration are per-process in-memory UInt64 and never compared across restarts.
• No fd/socket/task leak on the fixed path: connectInFlight is cleared via defer.
• The IrxNatBarrierTests.swift hunks are unrelated test hygiene, fine to keep.

Fixed: nothing. The gap is a behavior choice about where the "did this owner touch the lane" decision belongs, which is yours to make rather than something I should push onto your branch.

Left: findings 1 and 2, for you. Ping me when it is updated and I will re-review and merge. Nice catch on the supersede path in the first place :)

[teamleaderleo]

Reviewed the focused transport regression and its test coverage. Required checks are green; approving.

[github-actions]

Merge receipt for 36bfbe28ca: every check was green at merge (30 verified; 17 skipped by policy). Full suite runs on main after merge.