Repository navigation
iOS: evict pooled iroh sessions that lose every path without a closure callback - #9190
Closed
azooz2003-bit wants to merge 1 commit into
Closed
azooz2003-bit wants to merge 1 commit into
azooz2003-bit wants to merge 1 commit into
Conversation
…lback The 2026-07-29 13:03 outage export shows a foreground session whose relay and privateNetwork paths both closed at 13:03:14 while the session object stayed in the client pool for the remaining 73s of the log: no sessionClosed, no close attribution, no eviction. The pool only noticed death via waitUntilClosed(), and the iroh boundary never fired it. The pool's selected-path observation now consumes the observed value it previously discarded. When a session's selected path becomes .unavailable it arms a bounded 15s eviction via the injected relay clock; any usable path disarms it. At the deadline the pool re-reads live path state (level-triggered, not event-trusting) and, if still unavailable, invalidates the session with a new append-only allPathsClosed lifecycle kind and a noRoute failure, closing the connection and releasing the control owner. Eviction timers are cancelled wherever the session leaves the pool (closure watcher, invalidation, runtime deactivation). Fixes #9178 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughChangesAll-paths-closed eviction
Estimated code review effort: 4 (Complex) | ~40 minutes Sequence Diagram(s)sequenceDiagram
participant SelectedPathObserver
participant CmxIrohClientSessionPool
participant Session
participant DiagnosticLog
SelectedPathObserver->>CmxIrohClientSessionPool: report unavailable path
CmxIrohClientSessionPool->>CmxIrohClientSessionPool: arm grace-window eviction
CmxIrohClientSessionPool->>Session: recheck path state
CmxIrohClientSessionPool->>Session: invalidate with allPathsClosed/noRoute
Session->>DiagnosticLog: record closure and lifecycle removal
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors)
✅ Passed checks (23 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohClientSessionPoolPathEvictionTests.swift`:
- Around line 128-133: Replace the fixed 50ms Task.sleep in the test assertion
around selectedObservedPath with deterministic synchronization using
HoldingGraceClock state, preferably verifying that release() did not re-arm an
eviction task. If state inspection is unavailable, use a bounded polling loop
that waits for the relevant condition without relying on a single wall-clock
delay, while preserving the observedCloseCallCount() == 0 assertion.
🪄 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 Plus
Run ID: a5f3d2d4-7250-49fa-aea5-60f3b22eceae
📒 Files selected for processing (4)
Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/DiagnosticTaxonomy.swiftPackages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohClientSessionPool.swiftPackages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohClientSessionPoolPathEvictionTests.swiftPackages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohClientSessionPoolTests.swift
Comment on lines
+128
to
+133
| // Give a mistaken eviction every chance to land before asserting. | ||
| await Task.yield() | ||
| try await Task.sleep(nanoseconds: 50_000_000) | ||
| #expect(await connection.observedCloseCallCount() == 0) | ||
| #expect(await pool.selectedObservedPath() == .relay(url: "https://relay.example")) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Fixed real-time sleep before a negative assertion.
The fixed Task.sleep(nanoseconds: 50_000_000) before asserting observedCloseCallCount() == 0 is a wall-clock-dependent, single fixed-duration wait immediately preceding a correctness assertion. If a mistaken eviction landed slightly later than 50ms (e.g. under CI load), this passes without ever exercising the failure path; conversely it could occasionally flake. Since HoldingGraceClock already tracks state, prefer a deterministic check (e.g. assert no eviction task was re-armed after release()) or a bounded poll loop instead of a single fixed wait.
♻️ Example bounded-poll alternative
- // Give a mistaken eviction every chance to land before asserting.
- await Task.yield()
- try await Task.sleep(nanoseconds: 50_000_000)
- `#expect`(await connection.observedCloseCallCount() == 0)
+ // Poll for a bounded window to give a mistaken eviction every chance
+ // to land, without depending on a single fixed-duration wait.
+ for _ in 0..<10 {
+ `#expect`(await connection.observedCloseCallCount() == 0)
+ try? await Task.sleep(nanoseconds: 5_000_000)
+ }
`#expect`(await pool.selectedObservedPath() == .relay(url: "https://relay.example"))As per coding guidelines, {cmuxTests,cmuxUITests,ios/cmuxUITests,Packages/**/Tests,tests,tests_v2,web/tests,webviews/test}/**: "Do not use fixed sleeps, measured wall-clock assertions, or hard absolute latency ceilings in correctness tests." Note this conflicts with the **/*Tests.swift carve-out ("Test-only synchronization or sleeps are allowed") that also matches this filename; flagging per the more specific, directly-applicable test-timing rule.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Give a mistaken eviction every chance to land before asserting. | |
| await Task.yield() | |
| try await Task.sleep(nanoseconds: 50_000_000) | |
| #expect(await connection.observedCloseCallCount() == 0) | |
| #expect(await pool.selectedObservedPath() == .relay(url: "https://relay.example")) | |
| } | |
| // Poll for a bounded window to give a mistaken eviction every chance | |
| // to land, without depending on a single fixed-duration wait. | |
| for _ in 0..<10 { | |
| `#expect`(await connection.observedCloseCallCount() == 0) | |
| try? await Task.sleep(nanoseconds: 5_000_000) | |
| } | |
| `#expect`(await pool.selectedObservedPath() == .relay(url: "https://relay.example")) | |
| } |
🤖 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/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohClientSessionPoolPathEvictionTests.swift`
around lines 128 - 133, Replace the fixed 50ms Task.sleep in the test assertion
around selectedObservedPath with deterministic synchronization using
HoldingGraceClock state, preferably verifying that release() did not re-arm an
eviction task. If state inspection is unavailable, use a bounded polling loop
that waits for the relevant condition without relying on a single wall-clock
delay, while preserving the observedCloseCallCount() == 0 assertion.
Source: Coding guidelines
This was referenced Jul 31, 2026
Collaborator
Author
|
Status after connectivity v2: session eviction on lost paths ships in v2 (unavailableSelectedPathEvictsTheSessionAndTheNextOperationRedials), and the 15s all-paths-closed grace eviction is being ported into CmxConnectivityPeerSession by #9241. Looks fully superseded once 9241 lands. |
lawrencecchen
added
the
stale-revisit
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In the 2026-07-29 13:03 outage export (#9178), the foreground control session lost both its iroh paths (
transportPathEvent closedfor relay and privateNetwork) and then sat in the client session pool as a corpse for the remaining 73 s of the log: nosessionClosed, no close attribution, no eviction.CmxIrohClientSessionPoolonly detected death through the connection'swaitUntilClosed()callback, and the iroh boundary never fired it.Mechanism. The pool's per-session selected-path observation task now consumes the
CmxIrohObservedConnectionPathvalue it previously discarded..unavailablearms a bounded 15 s eviction deadline on the pool's injectedCmxIrohRelayClock; any usable path (direct, privateNetwork, relay) disarms it. When the deadline fires, the pool re-reads the connection's live path state (level-triggered, so a recovery without an intervening change event still survives) and only then invalidates the session: connection closed, control owner released, closure recorded with a new append-onlyDiagnosticSessionLifecycleKind.allPathsClosedand anoRoutefailure kind, so the next export names this eviction exactly. Timers are cancelled at every other pool-departure path (closure watcher, lane-failure invalidation, runtime deactivation/reconfiguration).15 s sits above a normal path migration (the iroh detector fails over in ~1-3 RTT) and below the point where recovery visibly stalls; the constant is documented at the declaration.
Verification: 2 new behavior tests (
CmxIrohClientSessionPoolPathEvictionTests) driving the real pool with a test connection: eviction fires after grace with correct attribution, and a path recovery inside the grace window disarms it (held-clock determinism, no wall-clock sleeps in the decision path). Full CmuxIrohTransport suite 496/496; CMUXMobileCore 306/306. A red/green commit split was not practical because the eviction API is new; the tests pin the outage behavior directly.Fixes #9178
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Evicts iOS iroh sessions from the pool when all paths are lost and the closure callback never fires. Prevents zombie sessions and adds clear
noRouteattribution, addressing cmux #9178.CmxIrohClientSessionPoolnow consumes observed selected-path changes and arms a 15s eviction viaCmxIrohRelayClockon.unavailable; any usable path cancels it.DiagnosticSessionLifecycleKind.allPathsClosedwithnoRoute.Written for commit fe5cc02. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests