Repository navigation
fix(ios): anchor Iroh connection diagnostics - #8456
Merged
5 commits merged intoJul 19, 2026
Merged
5 commits merged into
5 commits merged into
Conversation
Bugbot is paused — on-demand spend limit reachedBugbot 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. |
📝 WalkthroughWalkthroughTransport requests now carry a session purpose, callers classify control, probe, and feature-lane connections, and the session pool preserves purpose during ownership transitions to prioritize foreground control paths. The iOS root scene now requires and consistently passes a diagnostic log. ChangesTransport session purpose
Diagnostic log wiring
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MobileCoreRPCClient
participant CmxByteTransportRequest
participant CmxIrohClientSessionPool
participant SelectedPathObserver
MobileCoreRPCClient->>CmxByteTransportRequest: create request with sessionPurpose
CmxByteTransportRequest->>CmxIrohClientSessionPool: acquire control session
CmxIrohClientSessionPool->>CmxIrohClientSessionPool: store ControlOwner id and purpose
CmxIrohClientSessionPool->>SelectedPathObserver: publish selected path
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 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 |
Contributor
Greptile SummaryThis PR fixes iOS Iroh diagnostics by introducing
Confidence Score: 5/5Safe to merge — the changes are well-scoped, thoroughly tested with two new regression tests, and the CmxIrohClientSessionPool actor isolation is maintained throughout. The foreground-preference logic in selectedObservedPath() and the publishSelectedPathChangeIfEstablished guard have been traced through all call sites (sessionDidClose, invalidateSession, releaseControlSession, reserveControlOwner) and produce no double-publish or stale-notification scenarios. The TestHangingDialEndpoint race fix is correct. No blocking primitives, no test seams in production source, and no ambient globals were introduced. No files require special attention — the session pool changes are fully covered by the new tests and the classification labels are consistently applied across all shell call sites. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[selectedObservedPath called] --> B{Find foregroundKey\ncontrolOwners.purpose == .foregroundControl\nAND sessions != nil}
B -- found --> E[Return foreground session path]
B -- not found --> C{Find controlKey\ncontrolOwners != nil\nAND sessions != nil}
C -- found --> F[Return control session path]
C -- not found --> D{sessionOrder.last\nAND sessions != nil}
D -- found --> G[Return any established session path]
D -- not found --> H[Return .unavailable]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[selectedObservedPath called] --> B{Find foregroundKey\ncontrolOwners.purpose == .foregroundControl\nAND sessions != nil}
B -- found --> E[Return foreground session path]
B -- not found --> C{Find controlKey\ncontrolOwners != nil\nAND sessions != nil}
C -- found --> F[Return control session path]
C -- not found --> D{sessionOrder.last\nAND sessions != nil}
D -- found --> G[Return any established session path]
D -- not found --> H[Return .unavailable]
Reviews (3): Last reviewed commit: "docs(ios): state production diagnostic o..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
349
to
384
| @@ -348,17 +367,19 @@ actor CmxIrohClientSessionPool { | |||
| return | |||
| } | |||
| if let existing = controlOwners[key] { | |||
| if existing == ownerID { | |||
| if existing.id == ownerID { | |||
| continuation.resume() | |||
| } else { | |||
| controlWaiters[key, default: []].append(ControlWaiter( | |||
| id: waiterID, | |||
| ownerID: ownerID, | |||
| purpose: purpose, | |||
| continuation: continuation | |||
| )) | |||
| } | |||
| } else { | |||
| controlOwners[key] = ownerID | |||
| controlOwners[key] = ControlOwner(id: ownerID, purpose: purpose) | |||
| publishSelectedPathChange() | |||
| continuation.resume() | |||
| } | |||
Contributor
There was a problem hiding this comment.
publishSelectedPathChange() fires before the session is established
publishSelectedPathChange() is now called inside reserveControlOwner at both the fast-path (controlOwners[key] = ...; publishSelectedPathChange(); return) and the slow-path continuation branch. At that point sessions[key] is still nil — the session hasn't been established yet. selectedObservedPath() guards foregroundKey and controlKey with sessions[key] != nil, so the notification causes consumers to wake and read the old fallback path (or .unavailable) rather than the new foreground session's path. The session establishment fires a second notification that brings the correct value, so state is eventually consistent. The concern is that consumers will briefly observe either stale data from an older background session or .unavailable between control-owner registration and session connect, which could cause a visible flicker in connection-status UI. Consider deferring these two publishSelectedPathChange() calls to the moment the session is added to sessions (which already fires its own notification via the session(for:) path).
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\n- share one production diagnostics ring between the Iroh runtime and mobile shell\n- label foreground, background, probe, and feature-lane Iroh sessions locally\n- report the active foreground control path instead of whichever pooled session changed last\n- keep exported reports bounded and redacted\n\n## Verification\n- CmxIrohTransport: 380 tests passed\n- CmuxMobileShell: 597 tests passed\n- CmuxMobileRPC: 101 tests passed\n- CMUXMobileCore full suite passed\n- regression test failed in the first commit and passed after the fix\n- tagged macOS build irdg connected an isolated iOS Simulator without QR\n- authenticated Iroh workspace and terminal rendering survived app terminate/relaunch\n- git diff --check\n\n## Risk\nSession purpose is process-local and never enters the protocol. Path reporting now prefers the visible foreground control session, with control and pooled fallbacks when no foreground owner exists.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes iOS Iroh diagnostics to always report the foreground control path and uses a single production diagnostic log owned by the iOS root scene, shared by the runtime and shell. Path-change events only fire for established sessions; background and feature lanes no longer override the visible path.
Bug Fixes
CmxTransportSessionPurposeand threaded it throughCmxByteTransportRequestandMobileCoreRPCClientto label sessions as foreground, background, probe, or feature-lane.CmxIrohClientSessionPoolto prefer the foreground control session forselectedObservedPath(); background/feature sessions cannot replace it, and selected-path changes publish only after a session is established..probe,.featureLane,.backgroundControl) to anchor diagnostics to the foreground.Migration
DiagnosticLogto theCMUXMobileRootSceneinitializer (required).Written for commit 2be2f03. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Diagnostics
Tests