Repository navigation
Harden reconnect: host cooldown parity, persistent diagnostics, per-device rate limits - #8576
Conversation
Parity hardening on top of the client reconnect fix: - The Mac host runtime now records the same account-scoped broker cooldown when activation legs fail with a rate-limit directive, gates re-activation on it (zero broker calls while floored, retryScheduled diagnostic), and clears it on successful activation, so a Mac can no longer storm the broker or keep a shared rate-limit window exhausted. - Host activation reuses the bootstrap-minted relay credential (refreshWithCredential) instead of minting twice. - A 429 without a Retry-After header now arms a short default floor via the shared CmxIrohBrokerCooldown.directiveSeconds helper (client and host), so a missing header can never reopen the retry storm. - The offline-policy and binding-expectation errors carry diagnostic failure kinds instead of decoding as unknown (the b=255 field signature). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
Greptile SummaryThis PR hardens reconnect handling and adds durable mobile diagnostics. The main changes are:
Confidence Score: 4/5These two state-consistency issues should be fixed before merging.
MobileIrohRuntimeComposition.swift and MobileIrohSettingsModel.swift
|
| Filename | Overview |
|---|---|
| ios/cmuxPackage/Sources/cmuxFeature/MobileIrohRuntimeComposition.swift | Adds lifecycle archive persistence and clearing, but pending detached writes can restore cleared data or overwrite newer snapshots. |
| Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileIrohSettingsModel.swift | Adds verbose-log controls, but startup UI state can disagree with the file sink after an open failure. |
| Packages/iOS/CmuxMobileDiagnostics/Sources/CmuxMobileDiagnostics/MobileDebugLog.swift | Persists runtime toggle changes after sink acceptance, while startup failures are not reflected back into Settings state. |
| Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/DiagnosticReportArchive.swift | Adds bounded atomic archive storage whose callers must still coordinate concurrent save and clear operations. |
Reviews (6): Last reviewed commit: "Merge remote-tracking branch 'origin/fea..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private let diagnosticArchive = DiagnosticReportArchive.defaultArchive() | ||
| private var previousLaunchDiagnosticReport: DiagnosticReport?? |
There was a problem hiding this comment.
The new archive and its cache are process-scoped, but sign-out only clears the live diagnostic ring. After account A backgrounds and signs out, account B can export A's archived timeline because neither wipeLocalState() nor sign-out preparation clears these values. Clear both values during account erasure or scope the archive by account.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
| public func setFileLogging(enabled: Bool) async -> Bool { | ||
| UserDefaults.standard.set(enabled, forKey: Self.verboseLogDefaultsKey) | ||
| return await sink.setFileLogging(enabled: enabled) | ||
| } |
There was a problem hiding this comment.
This stores true before the sink confirms that it opened the log file, and the Settings caller discards the returned failure. If directory creation, rotation, or opening fails, the toggle remains enabled across the current session and relaunch even though no log is being recorded.
| if previousLaunchDiagnosticReport == nil { | ||
| previousLaunchDiagnosticReport = .some(diagnosticArchive?.load()) | ||
| } | ||
| let runtime = runtime | ||
| let diagnosticLog = diagnosticLog | ||
| let diagnosticArchive = diagnosticArchive | ||
| sceneTransitionTask = Task { | ||
| // Archive the diagnostic ring while backgrounded so a later | ||
| // relaunch keeps the events around a drop exportable. | ||
| if let diagnosticLog, let diagnosticArchive { | ||
| diagnosticArchive.save(await diagnosticLog.snapshot()) |
There was a problem hiding this comment.
didEnterBackground() synchronously loads the archive, and its unstructured Task inherits this type's MainActor isolation before performing the synchronous atomic save. A slow filesystem or near-limit archive can stall the UI and consume the app's background suspension window; move archive reads and writes behind an explicit non-main async boundary.
Rule Used: Flag incorrect or missing use of Swift @Concurrent... (source)
…t-in Two export lanes so connection drops stay diagnosable in release builds: - The privacy-safe diagnostic ring is archived when the scene backgrounds (DiagnosticReportArchive, bounded JSON in Application Support) and the Share Safe Report export now prepends the previous launch's block, so a drop followed by a relaunch no longer erases the evidence. - A localized Verbose Connection Log toggle in the Iroh settings enables the durable debug-log file in release builds (DEBUG keeps logging always-on), persisted across launches, with a Share Verbose Log row for the file. The sink gained a runtime setFileLogging toggle; terminal contents and credentials are never written to this lane. Localization audit: three new keys added to CmuxMobileShellUI Localizable.xcstrings with en and ja translations; no other user-facing strings changed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The relay token limiter keyed by account only, so one storming device exhausted the budget for every phone, simulator, and tagged build on the account. The check now keys by account plus the validated endpoint id and runs after body validation, so malformed requests never consume a device's budget and a misbehaving device only starves itself. The devices/iroh routes already partition per account, operation, and registration identity. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
a31e36b to
9def536
Compare
…events Three export-vocabulary additions so a single Share Safe Report can name a drop's cause remotely: - appLifecycleChanged (52): scene phase transitions recorded at the existing composition hooks, so a session that closes seconds after backgrounding reads as a suspension casualty rather than a network failure. Verified live on the simulator (inactive/active/inactive/background sequence in the archived report). - reachabilityChanged (53): the shell's network path observation records online state on every transition, correlating drops with WiFi/cellular moves. - recoveryStarted now carries the recovery trigger in its b slot (stable append-only codes), so every reconnect cycle names why it began: network change, manual retry, presence push, foreground, liveness, stream end, subscription failure, write timeout, or backoff expiry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Account erasure and sign-out now clear the archived diagnostic report and its in-memory cache, so a second account can never export the previous account's connection timeline. - The verbose-log opt-in persists only when the sink actually opened the file, and the Settings toggle reverts when enabling fails, so the UI can no longer claim to be recording without a log. - Archive reads and writes moved behind a detached utility task; scene backgrounding no longer spends main-actor time or the suspension window on filesystem work. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| Task.detached(priority: .utility) { [weak self] in | ||
| let previous = needsPreviousLoad ? diagnosticArchive.load() : nil | ||
| if needsPreviousLoad { | ||
| await self?.cachePreviousLaunchReport(previous) | ||
| } | ||
| diagnosticArchive.save(await diagnosticLog.snapshot()) |
There was a problem hiding this comment.
Each inactive or background transition starts an independent detached task that writes to the same archive. A task already waiting for diagnosticLog.snapshot() can finish after sign-out clears the archive and recreate it with the previous account's events. The next account can then export that report. The tasks can also finish out of order, letting the older inactive snapshot replace the newer background snapshot. Serialize these operations and invalidate pending writes when diagnostics or account state are cleared.
| private(set) var verboseLogEnabled = UserDefaults.standard.bool( | ||
| forKey: MobileDebugLog.verboseLogDefaultsKey | ||
| ) |
There was a problem hiding this comment.
This initializes the toggle from the saved preference, but the sink can fail to open its file during startup and silently disable file logging. After such a failure, Settings still shows logging as enabled while file writes are discarded, and an old file can keep the Share row visible. Reconcile this value with the sink's actual startup state before presenting it as enabled.
…iroh-hardening # Conflicts: # ios/cmuxPackage/Sources/cmuxFeature/MobileIrohRuntimeComposition.swift
| let needsPreviousLoad = previousLaunchDiagnosticReport == nil | ||
| Task.detached(priority: .utility) { [weak self] in | ||
| let previous = needsPreviousLoad ? diagnosticArchive.load() : nil | ||
| if needsPreviousLoad { | ||
| await self?.cachePreviousLaunchReport(previous) | ||
| } | ||
| diagnosticArchive.save(await diagnosticLog.snapshot()) |
There was a problem hiding this comment.
| private(set) var verboseLogEnabled = UserDefaults.standard.bool( | ||
| forKey: MobileDebugLog.verboseLogDefaultsKey | ||
| ) |
There was a problem hiding this comment.
1 participant
Stacked on #8557. Three hardening legs so the reconnect lockout class cannot recur, plus the diagnostics needed to root-cause the underlying session drops.
fix(iroh): extend broker cooldown to the Mac host and bare 429s): the host runtime gets the same account-scoped Retry-After cooldown, single-mint activation, and precise diagnostic kinds for the previously-unclassified activation failures. A 429 without a Retry-After header now arms a short default floor on both client and host, so a missing header can never reopen the retry storm.feat(ios): persist diagnostics across launches and add verbose log opt-in): both field reports were young processes — the in-memory ring died with each relaunch, erasing the drop moment. The ring is now archived on scene background and the Share Safe Report export prepends the previous launch's block. A localized (en/ja) Verbose Connection Log toggle additionally enables the durable debug-log file in release builds, with a Share Verbose Log row; terminal contents and credentials are never written to it.fix(web): partition relay token rate limit per device): the relay token limiter keyed by account only, so one storming device starved every build on the account. It now keys per account+endpoint and runs after validation; the devices/iroh routes already partition per account/operation/identity.Verification: CmuxIrohTransport 440 tests green locally (new directive-seconds tests); CMUXMobileCore 237 green locally (new archive tests); CmuxMobileDiagnostics 14 green; web relay-token route tests 7/7 green with the per-device contract asserted, tsc clean; tagged macOS build (irhrd) green; full iOS app build green and installed on an isolated simulator. The new Settings rows are compile- and build-verified; on-device toggle interaction is pending the dogfood pass.
Localization audit: three new keys added to CmuxMobileShellUI Localizable.xcstrings with en and ja translations; no other user-facing strings changed.
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Prevents reconnect lockouts and account-wide retry storms while making drops easier to diagnose. Adds host cooldown parity, durable cross-launch diagnostics with lifecycle/reachability attribution, and per-device relay token rate limits.
New Features
CMUXMobileCoreDiagnosticReportArchive; exports prepend the previous launch and archive on scene inactive/background (work runs off the main actor).CmuxMobileDiagnostics/CmuxMobileShellUI; persists only if the sink opens the file, supports runtime enable/disable, and adds “Share Verbose Log” (DEBUG always logs; Release only when enabled).Bug Fixes
CmuxIrohTransport/mobile host: respect account-scoped Retry-After, default 60s for bare 429 viaCmxIrohBrokerCooldown.directiveSeconds, gate re-activation withretryScheduled, clear on success; map offline-policy/binding errors to precise diagnostic kinds; reuse bootstrap relay credential.CmxIrohTrustBrokerClient; allow fresh authenticated discovery when registration is rate-limited; treat Retry-After up to 24h.web/services/relay/http.tsto key asaccount:device.Written for commit 7541d43. Summary will update on new commits.