Name lane-failure and send-queue-overflow iroh session close reasons - #8834
Merged
Merged
Conversation
These Debug tokens reach IrohError.message() from connection-level operations (accept_bi/open_bi) without the ConnectionLost(...) wrapper and currently classify as unknown (b=255 in the 2026-07-23 host ring). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bare iroh::endpoint::ConnectionError Debug tokens (TimedOut, LocallyClosed, Reset, ApplicationClosed(...), ConnectionClosed(...), TransportError(...), VersionMismatch, CidsExhausted) now map to honest DiagnosticFailureKinds instead of unknown, and the host's bounded event queue overflow close is labeled with the new appended sendQueueOverflow = 24 instead of protocolViolation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe change adds explicit queue-overflow diagnostics, broadens Iroh error classification for unwrapped connection errors, verifies stable taxonomy mappings, and displays localized queue-overflow labels in mobile and macOS settings. ChangesDiagnostic reporting updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MobileHostConnection
participant DiagnosticFailureKind
participant MobileIrohSettingsView
participant IrohNetworkingSection
MobileHostConnection->>DiagnosticFailureKind: record sendQueueOverflow
DiagnosticFailureKind->>MobileIrohSettingsView: provide lastFailureKind
DiagnosticFailureKind->>IrohNetworkingSection: provide lastFailureKind
MobileIrohSettingsView->>MobileIrohSettingsView: localize queue overflow label
IrohNetworkingSection->>IrohNetworkingSection: localize queue overflow label
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (22 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 two
Confidence Score: 4/5Safe to merge; the behavioral changes are well-scoped to diagnostic classification with thorough test coverage for all new token mappings. The classification ordering in CmxIrohDiagnosticFailure.swift carries one non-obvious maintenance trap: the new bare TransportError( arm at line 69 silently shadows the later TLS keyword fallback for any message that contains TransportError( without an explicit Code::crypto( or TLS error: marker. This is intentional per the PR's vocabulary analysis, but lacks an inline comment tying the ordering to that invariant. Everything else — enum addition, raw-value uniqueness test, UI switch coverage, localization in all three catalogs — is correct and complete. Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohDiagnosticFailure.swift — specifically the ordering relationship between the new bare-token block and the TLS/crypto keyword fallback below it. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[IrohError message] --> B{Wrapped ConnectionLost form?}
B -->|TimedOut| C[transportIdleTimedOut]
B -->|LocallyClosed| D[cancelled]
B -->|TransportError plus crypto code| E[secureChannelFailed]
B -->|Reset or TransportError or AppClosed| F[connectionClosed]
B -->|no match| G{DNS keywords?}
G -->|yes| H[dnsFailed]
G -->|no| I{NEW bare token block}
I -->|TimedOut| C
I -->|LocallyClosed| D
I -->|VersionMismatch| J[protocolViolation]
I -->|CidsExhausted| K[endpointUnavailable]
I -->|ApplicationClosed, ConnectionClosed, TransportError, Reset| F
I -->|no match| L{Generic fallbacks}
L -->|timed out| M[timedOut]
L -->|TLS or Handshake keywords| E
L -->|ConnectionLost or ClosedStream| F
L -->|no match| N[unknown]
subgraph MobileHostService sendEvent
O[queue overflow] -->|was protocolViolation| P[sendQueueOverflow 24]
Q[frame encode error] --> R[protocolViolation unchanged]
end
Reviews (1): Last reviewed commit: "Name lane-failure and send-queue-overflo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+67
to
+72
| if message.contains("ApplicationClosed(") | ||
| || message.contains("ConnectionClosed(") | ||
| || message.contains("TransportError(") | ||
| || message.contains("Reset") { | ||
| return .connectionClosed | ||
| } |
Contributor
There was a problem hiding this comment.
TransportError( now shadows TLS keyword fallback
The bare TransportError( arm at line 69 causes any message containing TransportError( without an explicit Code::crypto( or TLS error: marker to return .connectionClosed before reaching the TLS/crypto keyword block at lines 78-87 (Handshake, certificate, TLS, CryptoError, etc.). In the old code those fallbacks were reachable for unwrapped TransportError( payloads that embed TLS-adjacent language in their reason strings (e.g. TransportError(Error { code: Code(1), reason: "TLS handshake failed" })), which would have matched Handshake → .secureChannelFailed; now they classify as .connectionClosed. The PR description states iroh always emits Code::crypto( for QUIC crypto errors, which makes this intentional — but a brief comment here tying the ordering to that vocabulary guarantee would make the invariant explicit and prevent a future contributor from moving the TLS keyword block above this arm to "improve coverage" and inadvertently changing both paths.
3 tasks done
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.
Field evidence (2026-07-23, host ring captured on a MacBook during the WiFi path-flap reconnect loop): dozens of admitted iroh session deaths logged
transportSessionLifecycle a=7 (applicationLaneFailed)withsessionClosed b=255 (unknown), plusa=4 (controlWriteFailed)withb=17 (protocolViolation)2-5 seconds after admission. Neither label names the kill mechanism, so cmuxdiag rings stayed indecisive.#8716 pinned
IrohError.message()tokens for stream read/write errors, which arrive wrapped asConnectionLost(...). Connection-level operations (accept_bi,open_bi,accept_uni,open_uni) throw the bareiroh::endpoint::ConnectionErrorinstead: iroh-ffi 1.0.2-cmux.4 formats it withanyhow!("{:?}"), so the host lane-accept loop seesTimedOut,LocallyClosed,Reset,ApplicationClosed(..),ConnectionClosed(..),TransportError(..),VersionMismatch, orCidsExhaustedwith no wrapper (noqConnectionErrorat manaflow-ai/noq@2271bbc, via the manaflow-ai/iroh fork). None matched, so every lane-failure exit classifiedunknown. New bare-token mappings, mirroring the wrapped forms:TimedOutLocallyClosedResetApplicationClosed(/ConnectionClosed(/TransportError(TransportError(+Code::crypto(orTLS error:VersionMismatchCidsExhaustedThe
a=4/b=17deaths were not write errors at all: no error thrown on the control write path can classify to protocolViolation. They came fromMobileHostConnection.sendEvent's bounded event-queue overflow close, which hardcoded.protocolViolation. During a path flap the drain task blocks inside the QUIC write while terminal deltas keep queueing; 256 events fill within seconds and the host kills the session, which matches the observed 2-5s timing. That close now uses the new appendedDiagnosticFailureKind.sendQueueOverflow = 24(no existing case renumbered). Both Connection Report UIs (macOS Settings > Networking, iOS Iroh settings) render the new kind, localized en+ja across all three string catalogs. The two frame-encode closes keep protocolViolation with comments:MobileSyncFrameCodec.encodeFrameonly throwsframeTooLarge, a genuine local wire-limit violation.Not done: recording a raw or hashed unknown-error string into the event payload.
sessionClosedhas a/b/c taken (transport, failure, session ID),msis documented as a millisecond magnitude, and the diagnostics design deliberately excludes string payloads; hashes of full Debug messages also would not match precomputed candidates because they embed dynamic codes and reasons. With the noq error vocabulary now enumerated, residual unknowns should be structurally rare, and host os_log retains the raw string privately at each classify site.Commit 1 adds the failing token table only (8 of 9 new rows red); commit 2 adds the fix.
swift testpasses for CmuxIrohTransport (465 tests) and CMUXMobileCore (283 tests); CmuxSettingsUI builds. Localization audit: the only new user-facing string is the failure label, added with en+ja entries toResources/Localizable.xcstrings, the CmuxMobileShellUI package catalog, andios/cmux/Resources/Localizable.xcstrings.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Improves iroh session diagnostics by mapping previously “unknown” lane failures and introducing a clear reason for send-queue overflow. This makes rings and Settings UIs show accurate close causes, especially during path flaps.
Bug Fixes
ConnectionErrortokens toDiagnosticFailureKind:TimedOut,LocallyClosed,Reset,ApplicationClosed(...),ConnectionClosed(...),TransportError(...),VersionMismatch,CidsExhausted(no moreunknown).CmuxIrohTransportandCMUXMobileCorepin the new classifications.New Features
sendQueueOverflow = 24and use it for the host’s bounded control send-queue close (wasprotocolViolation).CmuxSettingsUI,CmuxMobileShellUI), localized in en and ja.Written for commit 532ca81. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes