Repository navigation
Recover the Mac iroh host runtime from terminal failure without relaunch - #8930
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
A non-transient broker rejection (401/403/404/409, invalid response) tears CmxIrohHostRuntime down into a terminal .failed phase. That fail-closed teardown is deliberate, but nothing ever rebuilt the runtime: MobileHostIrohRuntime.retryIfNeeded() only re-synced LAN publication while it held a runtime reference, and no timer retried a failed activation. A Mac whose registration was rejected once stayed unregistered until sign-out/sign-in, a Settings-triggered restart, or an app relaunch (the 17-hour App Store review 409 wedge). Recovery is now owned by the macOS composition root, level-triggered through the existing reconcile path: - Every failed activation and every runtime self-teardown into .failed (reported through the existing handleDeactivation callback, filtered by lifecycle revision so deliberate stops are ignored) arms one pending rebuild with bounded exponential backoff (30s doubling to a 1h cap, jittered, via CmxIrohRetrySchedule and an injected clock). - retryIfNeeded() now rebuilds a .failed runtime immediately on any external wake signal (network path change, app-level retry) and resets the backoff ladder, instead of only re-syncing LAN state. - Each reconcile cancels the pending attempt and re-derives recovery from its own outcome: success resets the ladder, failure re-arms it, sign-out/deactivation ends it. The new package test pins the contract this depends on: a rejected registration refresh fails closed (endpoint torn down, deactivation notified) and the same runtime accepts start() again once the broker allows registration. The two-commit red/green structure does not apply because the wedge lives in app-target singleton wiring that has no practical automated harness; the package test guards the enabling semantics instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthrough
ChangesRuntime failure recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HostRuntimeTest
participant CmxIrohHostRuntime
participant Broker
participant MobileHostIrohRuntime
HostRuntimeTest->>CmxIrohHostRuntime: start()
CmxIrohHostRuntime->>Broker: refresh registration
Broker-->>CmxIrohHostRuntime: 409 binding replacement rejection
CmxIrohHostRuntime-->>MobileHostIrohRuntime: deactivate failed runtime
MobileHostIrohRuntime->>MobileHostIrohRuntime: schedule failure recovery
MobileHostIrohRuntime->>CmxIrohHostRuntime: restart active runtime
CmxIrohHostRuntime->>Broker: register again
Broker-->>CmxIrohHostRuntime: successful registration
CmxIrohHostRuntime-->>HostRuntimeTest: active lifecycle phase
Possibly related PRs
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 |
Contributor
Greptile SummaryThis PR adds automatic recovery for terminally failed macOS Iroh host runtimes.
Confidence Score: 3/5This PR should not merge until stale deactivation cleanup is prevented from disrupting a newly rebuilt Iroh host runtime. Immediate external-wake recovery can overlap the failed runtime’s asynchronous callback, whose unguarded global cleanup may close the replacement runtime’s connections and clear its published binding. Files Needing Attention: Sources/Mobile/MobileHostIrohRuntime+Lifecycle.swift and Sources/Mobile/MobileHostIrohRuntime+Activation.swift Important Files Changed
Sequence DiagramsequenceDiagram
participant Old as Failed runtime
participant Callback as Deactivation callback
participant Wake as External retry signal
participant Root as MobileHostIrohRuntime
participant New as Replacement runtime
Old->>Callback: Begin deactivation
Callback->>Callback: await LAN publisher stop
Wake->>Root: retryIfNeeded()
Root->>New: Reconcile and activate
New-->>Root: Active
Callback->>Root: Close all connections and clear binding
Note over Root,New: Stale cleanup disrupts replacement
Callback->>Root: Revision check rejects only recovery scheduling
Reviews (1): Last reviewed commit: "Recover the Mac iroh host runtime from t..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+278
to
+282
| self.cancelFailureRecovery(resetBackoff: true) | ||
| self.scheduleReconcile( | ||
| eraseAccountState: false, | ||
| restartActiveRuntime: true | ||
| ) |
Contributor
There was a problem hiding this comment.
When an external retry starts replacement activation while the failed runtime’s deactivation callback is suspended, the old callback resumes and unconditionally stops LAN publication, closes all Iroh connections, and clears the service binding before checking its lifecycle revision, causing the rebuilt host to become unreachable again without arming recovery for the new runtime.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
Knowledge Base Used: Shared Packages (Packages/Shared/)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@Sources/Mobile/MobileHostIrohRuntime.swift`:
- Around line 120-123: Make failureRecoveryClock and failureRecoverySchedule in
MobileHostIrohRuntime test-injectable by changing them from immutable concrete
defaults to internal mutable properties, or by accepting them through an
initializer while preserving the existing defaults. Ensure tests can substitute
a virtual clock and deterministic retry schedule via `@testable` import without
changing production behavior.
In `@Sources/Mobile/MobileHostIrohRuntime`+Lifecycle.swift:
- Around line 261-289: Update retryIfNeeded() to delegate failed-runtime
handling to recoverFailedRuntimeIfNeeded() instead of performing the inline
cancelFailureRecovery and scheduleReconcile restart flow. Preserve the existing
active-runtime and lifecycleRevision guards, and let the shared helper enforce
its transitionTask == nil guard and backoff reset behavior.
🪄 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: fbfdb702-bd89-47da-962e-ebfd6246a5fb
📒 Files selected for processing (4)
Packages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohHostRuntimeFailedRestartTests.swiftSources/Mobile/MobileHostIrohRuntime+Activation.swiftSources/Mobile/MobileHostIrohRuntime+Lifecycle.swiftSources/Mobile/MobileHostIrohRuntime.swift
Comment on lines
+120
to
+123
| var failureRecoveryTask: Task<Void, Never>? | ||
| var failureRecoveryFailureCount = 0 | ||
| let failureRecoveryClock: any CmxIrohRelayClock = CmxIrohSystemRelayClock() | ||
| let failureRecoverySchedule = CmxIrohRetrySchedule() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Backoff clock/schedule aren't test-injectable.
failureRecoveryClock and failureRecoverySchedule are let-bound to concrete defaults with no initializer seam, so the jittered-backoff timing logic in this composition-root file (unlike the CmxIrohHostRuntime layer, which has dedicated tests per the PR) can't be exercised deterministically from a test target via @testable import. Consider widening these to internal var (or accepting them via init) so a test can substitute a virtual clock/deterministic schedule, consistent with how other clock-dependent code in this codebase is tested.
🤖 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 `@Sources/Mobile/MobileHostIrohRuntime.swift` around lines 120 - 123, Make
failureRecoveryClock and failureRecoverySchedule in MobileHostIrohRuntime
test-injectable by changing them from immutable concrete defaults to internal
mutable properties, or by accepting them through an initializer while preserving
the existing defaults. Ensure tests can substitute a virtual clock and
deterministic retry schedule via `@testable` import without changing production
behavior.
Comment on lines
+261
to
+289
| guard let activeRuntime = runtime else { | ||
| scheduleReconcile(eraseAccountState: false) | ||
| return | ||
| } | ||
| let revision = lifecycleRevision | ||
| Task { @MainActor [weak self] in | ||
| guard let self, | ||
| self.desiredActive, | ||
| self.runtime === activeRuntime, | ||
| revision == self.lifecycleRevision else { return } | ||
| if await activeRuntime.snapshot().state == .failed { | ||
| guard self.desiredActive, | ||
| !self.signOutIntentActive, | ||
| self.runtime === activeRuntime, | ||
| revision == self.lifecycleRevision else { return } | ||
| await self.synchronizeLANPublicationWithSettings() | ||
| // A fresh external signal rebuilds a failed runtime now and | ||
| // restarts the recovery backoff ladder. | ||
| self.cancelFailureRecovery(resetBackoff: true) | ||
| self.scheduleReconcile( | ||
| eraseAccountState: false, | ||
| restartActiveRuntime: true | ||
| ) | ||
| return | ||
| } | ||
| guard self.runtime === activeRuntime, | ||
| revision == self.lifecycleRevision else { return } | ||
| await self.synchronizeLANPublicationWithSettings() | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find file and inspect relevant sections"
fd -a 'MobileHostIrohRuntime\+Lifecycle\.swift$' . || true
file="$(fd 'MobileHostIrohRuntime\+Lifecycle\.swift$' . | head -n 1)"
if [ -z "${file:-}" ]; then
echo "file not found"
exit 0
fi
echo "FILE=$file"
wc -l "$file"
echo "--- lines 220-295 ---"
sed -n '220,295p' "$file" | nl -ba -v220
echo "--- lines 310-365 ---"
sed -n '310,365p' "$file" | nl -ba -v310
echo "--- retryIfNeeded/recoverFailed callers/usages ---"
rg -n "retryIfNeeded|recoverFailedRuntimeIfNeeded|scheduleReconcile\\(.*restartActiveRuntime|cancelFailureRecovery\\(resetBackoff: true\\)" "$file" . --glob '!**/.git/**' --glob '!**/DerivedData/**' --glob '!**/build/**' | head -n 200Repository: manaflow-ai/cmux
Length of output: 448
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect relevant declarations and surrounding state"
file="$(fd 'MobileHostIrohRuntime\+Lifecycle\.swift$' . | head -n 1)"
echo "--- lines 1-220 ---"
sed -n '1,220p' "$file" | nl -ba -v1
echo "--- all occurrences of methods/properties relevant to race evidence ---"
rg -n "retryIfNeeded|recoverFailedRuntimeIfNeeded|scheduleReconcile|cancelFailureRecovery|transitionTask|lifecycleRevision|desiredActive|signOutIntentActive|desiredActive|runtime\s*=" "$file" -C 2Repository: manaflow-ai/cmux
Length of output: 266
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="./Sources/Mobile/MobileHostIrohRuntime+Lifecycle.swift"
echo "FILE=$file"
wc -l "$file"
echo "--- lines 220-295 ---"
sed -n '220,295p' "$file" | awk '{printf "%5s\t%s\n", NR+219, $0}'
echo "--- lines 310-365 ---"
sed -n '310,365p' "$file" | awk '{printf "%5s\t%s\n", NR+309, $0}'
echo "--- lines 1-180 ---"
sed -n '1,180p' "$file" | awk '{printf "%5s\t%s\n", NR, $0}'
echo "--- all occurrences relevant to lifecycle/transition/backoff/schedule ---"
rg -n "retryIfNeeded|recoverFailedRuntimeIfNeeded|scheduleReconcile|cancelFailureRecovery|transitionTask|lifecycleRevision|desiredActive|signOutIntentActive|runtime\s*=|desiredActive\s*=" "$file" -C 2Repository: manaflow-ai/cmux
Length of output: 21770
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find caller contexts"
rg -n "retryIfNeeded\\(|recoverFailedRuntimeIfNeeded\\(" . --glob '!**/.git/**' --glob '!**/DerivedData/**' --glob '!**/build/**' --glob '!**/.build/**' | head -n 200Repository: manaflow-ai/cmux
Length of output: 770
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- MobileHostService.swift around retryIfNeeded call ---"
sed -n '1660,1725p' ./Sources/Mobile/MobileHostService.swift | awk '{printf "%5s\t%s\n", NR+1659, $0}'
echo "--- MobileHostIrohRuntime+Lifecycle.swift scheduleFailureRecovery and cancel/backoff area ---"
sed -n '290,378p' ./Sources/Mobile/MobileHostIrohRuntime+Lifecycle.swift | awk '{printf "%5s\t%s\n", NR+289, $0}'
echo "--- MobileHostIrohRuntime+Lifecycle.swift network observation / callback references around setDesiredActive and shared observation fields ---"
rg -n "setDesiredActive|retryIfNeeded|update|Network|network|path|paths|Observing|Observation|Observable|`@Observable`|IrohNetwork|TransportVerification|CmxIrohTransportVerificationMode" ./Sources/Mobile ./cmuxTests --glob '!**/.git/**' --glob '!**/DerivedData/**' | head -n 250
echo "--- Swift availability of async Task cancellation behavior probe (no repo code) ---"
cat > /tmp/task_cancellation_probe.swift <<'SWIFT'
`@available`(macOS 14.0, macCatalyst 17.0, iOS 17.0, visionOS 1.0, watchOS 10.0, tvOS 17.0, *)
func probe() async {
let startedLock = Lock()
startedLock.lock()
var started = false
let task = Task {
startedLock.lock()
started = true
startedLock.unlock()
try await Task.sleep(nanoseconds: 2_000_000_000)
}
startedLock.wait()
task.cancel()
try? await task.value
print(started)
}
SWIFT
swiftc -typecheck /tmp/task_cancellation_probe.swift 2>&1 | sed -n '1,80p' || trueRepository: manaflow-ai/cmux
Length of output: 39971
Use the shared failed-runtime recovery path instead of duplicating it.
retryIfNeeded() is still invoked from network path changes, so its .failed runtime branch can run while recovery backoff is pending. The inline scheduleReconcile(eraseAccountState: false, restartActiveRuntime: true) also skips the post-snapshot() transitionTask == nil guard that recoverFailedRuntimeIfNeeded() keeps, creating a second restart entrypoint that can race. Delegate to recoverFailedRuntimeIfNeeded() from here; that helper already restarts the backoff ladder when a failed runtime is rebuilt.
🤖 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 `@Sources/Mobile/MobileHostIrohRuntime`+Lifecycle.swift around lines 261 - 289,
Update retryIfNeeded() to delegate failed-runtime handling to
recoverFailedRuntimeIfNeeded() instead of performing the inline
cancelFailureRecovery and scheduleReconcile restart flow. Preserve the existing
active-runtime and lifecycleRevision guards, and let the shared helper enforce
its transitionTask == nil guard and backoff reset behavior.
Source: Coding guidelines
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.
Problem
CmxIrohHostRuntimefails closed on any non-transient trust-broker rejection (401/403/404/409, malformed responses): it tears the endpoint down into a terminal.failedlifecycle phase. That posture is correct, but recovery was owned by nobody.MobileHostIrohRuntime.retryIfNeeded()returned early whenever it still held a runtime reference (only re-syncing LAN publication), and a failed cold-start activation had no retry timer at all. Result: one rejected registration left the Mac unregistered until sign-out/sign-in, a Settings-triggered iroh restart, or an app relaunch. This is the client half of the App Store review incident, where a stale binding produced a 409 and the review Mac sat unreachable for 17 hours.#8883 and #8888 remove the main 409 trigger (binding slot re-key). This PR removes the wedge class itself, for every non-transient rejection, including future ones.
Mechanism
Recovery is owned by the macOS composition root and is level-triggered through the existing
reconcilepath (no new mutation path):reconcilecatch) and every runtime self-teardown into.failed(observed via the existinghandleDeactivationcallback, filtered by lifecycle revision so deliberate stops are ignored) arms one pending rebuild with bounded exponential backoff: 30s doubling to a 1h cap, jittered, using the sameCmxIrohRetryScheduleand injected-clock idiom as the runtime's internal retries.retryIfNeeded()now checks the held runtime's snapshot: a.failedruntime is rebuilt immediately on any external wake signal (network path change viaMobileHostService, app-level retry), resetting the backoff ladder.reconcilecancels the pending attempt on entry and re-derives recovery from its own outcome: success resets the ladder, failure re-arms it, sign-out/deactivation ends it. At most one attempt is ever pending, and stale wake-ups are no-ops.Principled or hacky: principled. The inner runtime's fail-closed teardown is untouched (a rejected binding never keeps accepting connections),
.failedwas already designed restartable (LifecyclePhase.allowsStart), and the rebuild goes through the same reconcile path used by launch, sign-in, and Settings. Residual risk: a persistently rejecting broker now sees one registration attempt per backoff interval (capped at hourly) instead of silence; the challenge/register round trip is cheap and rate-limit floors (Retry-After) are honored by the schedule.Tests
CmxIrohHostRuntimeFailedRestartTestspins the package contract the watchdog depends on: a 409-rejected registration refresh fails closed (endpoint closed once, deactivation notified once, snapshot/phase.failed) and the same runtime acceptsstart()again once the broker allows registration, ending active. If.failedever stops allowingstart(), this fails instead of silently reverting to the relaunch-only wedge.Two-commit red/green does not apply: the wedge lives in app-target singleton wiring (
MobileHostIrohRuntime+MobileHostService+ auth) with no practical automated harness, so the package test guards the enabling semantics and the app wiring is covered by build plus the dogfood path below. All 36CmxIrohHostRuntime*package tests pass. No user-facing strings were added (log lines only), so no localization changes are needed.Verification
swift test --filter CmxIrohHostRuntimeinPackages/Shared/CmuxIrohTransport: 36/36 pass.irecovfrom this head.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Automatically recover the Mac iroh host runtime after terminal failures so a non-transient broker rejection no longer leaves the host unregistered until relaunch. Recovery is level-triggered via the existing reconcile path with bounded backoff.
.faileddeactivation.CmxIrohRetrySchedule; only one attempt pending.retryIfNeeded()immediately rebuilds a.failedruntime on external wake signals and resets backoff; eachreconcilecancels pending recovery and re-derives from outcome.handleDeactivationnow arms recovery (filtered bylifecycleRevision); addedCmxIrohHostRuntimeFailedRestartTeststo verify 409 rejection fails closed and laterstart()succeeds.Written for commit cb412f4. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests