Repository navigation
Register-when-ready Iroh hosts, bounded dial phases, cold-start release gate - #9752
Closed
azooz2003-bit wants to merge 5 commits into
Closed
azooz2003-bit wants to merge 5 commits into
azooz2003-bit wants to merge 5 commits into
Conversation
A just-launched Mac registers with the broker before its endpoint has attached to the home relay, so the advertised binding carries no relay hint and phones race a half-ready endpoint (16s hung dials observed in dogfood, #9724). The strict relay-readiness re-register block in CmxIrohHostRuntime.start() only runs for relay-only mode; automatic mode publishes the bootstrap snapshot immediately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Generalize the relay-only readiness barrier opportunistically: automatic mode now starts relay activation eagerly and gives the relay a bounded window (automaticRelayReadinessTimeout, default 5s) to become usable, so the binding published at activation carries post-attach dialable hints instead of the half-ready bootstrap snapshot phones were racing (#9724). Liveness is preserved everywhere: only the cancellation-safe waitForUsableHomeRelay is awaited against the budget, relay activation stays on the async sidecar with unchanged retry ownership, and any timeout, broker error, or hung credential install publishes the bootstrap policy exactly as before, leaving repair to the standard refresh-on-online path. Direct-only startup still skips readiness entirely. The credential-installation race test now injects a tight budget and asserts publication completes despite a hung install. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Dials had no clock anywhere: CmxIrohClientSession.establishConnection awaited endpoint.connect unbounded on both the public and the private fallback phase, so a stale-hint or half-ready target hung the attempt until an outer RPC deadline (16.2s observed in the issue 9724 dogfood trace). Each phase now races the fork's cancellable ConnectAttempt against one bound; expiry cancels the FFI dial promptly and surfaces CmxIrohClientSessionError.dialTimedOut (diagnostic kind timedOut), and a timed-out public phase still falls through to the private fallback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
New --mode cold-start row: the orchestrator already relaunches the tagged Mac unconditionally, so the scenario pins the launch-to-usable deadline (CMUX_ATTACH_READY_TIMEOUT_SECONDS=20) instead of inheriting the helper's ambient default, making the just-launched-Mac dial race of #9724 a permanent tripwire: a half-ready registration or unbounded dial blows the deadline and fails the gate. The cold_start_dial report scenario reuses the standard usable-session proofs; the timing assertion lives at the script layer, which measures true cross-process wall clock. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe change adds bounded Iroh dialing, automatic relay-readiness waiting, and a cold-start release-gate mode. It also adds timeout diagnostics, startup and dial tests, workflow selection, and script configuration. ChangesIroh transport readiness
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ReleaseGateScript
participant MobileIrohReleaseGateRunner
participant CmxIrohHostRuntime
participant Broker
participant CmxIrohClientSession
ReleaseGateScript->>MobileIrohReleaseGateRunner: Start cold_start_dial with automatic transport
CmxIrohHostRuntime->>Broker: Register and activate relay
CmxIrohHostRuntime->>Broker: Publish relay-ready route hints
MobileIrohReleaseGateRunner->>CmxIrohClientSession: Dial the newly launched Mac
CmxIrohClientSession->>CmxIrohClientSession: Apply dialPhaseTimeout
CmxIrohClientSession-->>MobileIrohReleaseGateRunner: Return usable session or dialTimedOut
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 2 warnings)
✅ Passed checks (21 passed)
✨ Finishing Touches 💡 1📝 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: 5
🤖 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 @.github/workflows/iroh-release-gate.yml:
- Line 63: Update the workflow conditions governing automatic-mode setup to
include the cold-start matrix value, specifically the branches near lines 83 and
89. Extend the TAG selection case statement with a unique cold-start) assignment
before invoking the gate script, ensuring cold-start follows the automatic
transport gate path and initializes TAG under set -u.
In
`@ios/cmuxPackage/Sources/CmuxIrohReleaseGateSupport/MobileIrohReleaseGateRunner.swift`:
- Around line 52-54: Update the scenario guard in MobileIrohReleaseGateRunner so
coldStartDial is accepted only when mode is automatic, while preserving standard
and relayOnly behavior. Add rejection tests covering coldStartDial with
directOnly and relayOnly modes.
In
`@Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohClientSession.swift`:
- Around line 320-324: Add a regression test for the CmxIrohClientSession
connection flow using the dial sequence [.hang, .connection(...)]. Assert the
public attempt produces a timeout, the validated private fallback is selected
and dialed, and admission completes successfully; keep the existing
terminal-timeout and failure-based fallback coverage unchanged.
In
`@Packages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohClientSessionTests.swift`:
- Around line 31-36: Remove the ContinuousClock setup, started timestamp, and
elapsed-time assertion from CmxIrohClientSessionTests. Keep the await
session.connect() check asserting the typed
CmxIrohClientSessionError.dialTimedOut result; add cancellation completion
signaling only if separate cancellation coverage is required.
In
`@Packages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohHostRuntimeLifecycleRaceTests.swift`:
- Around line 110-120: Remove the fixed .milliseconds(20) timeout from the
runtime setup in CmxIrohHostRuntimeLifecycleRaceTests and inject a controllable
relay-readiness clock or completion signal instead. After
gate.waitUntilSuspended(), explicitly advance or complete that readiness
mechanism before awaiting start.value, while preserving the existing transport
and binding handlers.
🪄 Autofix
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: bf5a7474-b94a-4a24-8e62-3fc2801b7990
📒 Files selected for processing (14)
.github/workflows/iroh-release-gate.ymlPackages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohClientSession.swiftPackages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohClientSessionError.swiftPackages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohDiagnosticFailure.swiftPackages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohHostRuntime.swiftPackages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohClientSessionTests.swiftPackages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohHostRuntimeLifecycleRaceTests.swiftPackages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohHostRuntimeTests.swiftPackages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/TestDialingIrohEndpoint.swiftPackages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/TestIrohDialResult.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShellReleaseGateSupport/MobileIrohReleaseGateScenario.swiftios/cmuxPackage/Sources/CmuxIrohReleaseGateSupport/MobileIrohReleaseGateRunner.swiftios/cmuxPackage/Tests/cmuxFeatureTests/MobileIrohReleaseGateRunnerTests.swiftscripts/run-iroh-release-gate.sh
| fail-fast: false | ||
| matrix: | ||
| mode: ${{ fromJSON(inputs.mode == 'all' && '["automatic","relay-only","relay-expiry","direct-only","private-path"]' || format('["{0}"]', inputs.mode)) }} | ||
| mode: ${{ fromJSON(inputs.mode == 'all' && '["automatic","relay-only","relay-expiry","direct-only","private-path","cold-start"]' || format('["{0}"]', inputs.mode)) }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Complete the cold-start workflow wiring.
The new matrix value skips both automatic-mode setup conditions. It also has no TAG case. With set -u, the gate exits when it expands $TAG.
Include cold-start in the conditions at Lines 83 and 89. Add a unique cold-start) TAG=... ;; case before invoking the script.
Based on the PR objective, cold-start must execute the automatic transport gate.
🤖 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 @.github/workflows/iroh-release-gate.yml at line 63, Update the workflow
conditions governing automatic-mode setup to include the cold-start matrix
value, specifically the branches near lines 83 and 89. Extend the TAG selection
case statement with a unique cold-start) assignment before invoking the gate
script, ensuring cold-start follows the automatic transport gate path and
initializes TAG under set -u.
Comment on lines
+52
to
+54
| guard scenario == .standard | ||
| || scenario == .coldStartDial | ||
| || mode == .relayOnly else { return nil } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restrict coldStartDial to automatic transport.
This condition accepts cold_start_dial with directOnly or relayOnly. Those runs can pass without testing automatic relay startup and fallback behavior. Require mode == .automatic when scenario == .coldStartDial. Add rejection tests for the other modes.
🤖 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
`@ios/cmuxPackage/Sources/CmuxIrohReleaseGateSupport/MobileIrohReleaseGateRunner.swift`
around lines 52 - 54, Update the scenario guard in MobileIrohReleaseGateRunner
so coldStartDial is accepted only when mode is automatic, while preserving
standard and relayOnly behavior. Add rejection tests covering coldStartDial with
directOnly and relayOnly modes.
Comment on lines
+320
to
+324
| establishedConnection = try await connectBounded( | ||
| to: CmxIrohEndpointAddress( | ||
| identity: targetIdentity, | ||
| pathHints: dialPlan.publicPaths | ||
| ), | ||
| alpn: protocolConfiguration.alpn | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add coverage for timeout-to-private fallback.
A public .dialTimedOut error must continue to the validated private fallback. The new test covers a terminal timeout without fallback. The existing fallback test uses .failure, not .hang.
Add a regression test with [.hang, .connection(...)]. Assert that the public path times out, the private path is validated and dialed, and admission completes.
Based on the PR objective, a timed-out public dial must proceed to private fallback.
🤖 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/Sources/CmuxIrohTransport/CmxIrohClientSession.swift`
around lines 320 - 324, Add a regression test for the CmxIrohClientSession
connection flow using the dial sequence [.hang, .connection(...)]. Assert the
public attempt produces a timeout, the validated private fallback is selected
and dialed, and admission completes successfully; keep the existing
terminal-timeout and failure-based fallback coverage unchanged.
Comment on lines
+31
to
+36
| let clock = ContinuousClock() | ||
| let started = clock.now | ||
| await #expect(throws: CmxIrohClientSessionError.dialTimedOut) { | ||
| try await session.connect() | ||
| } | ||
| #expect(clock.now - started < .seconds(2)) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the wall-clock assertion.
ContinuousClock and the two-second ceiling make this test depend on runner scheduling. Keep the typed .dialTimedOut assertion. If cancellation needs separate coverage, expose a fixture completion signal and await it.
Proposed fix
- let clock = ContinuousClock()
- let started = clock.now
await `#expect`(throws: CmxIrohClientSessionError.dialTimedOut) {
try await session.connect()
}
- `#expect`(clock.now - started < .seconds(2))As per coding guidelines, tests must not use wall-clock assertions or hard absolute latency ceilings.
📝 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
| let clock = ContinuousClock() | |
| let started = clock.now | |
| await #expect(throws: CmxIrohClientSessionError.dialTimedOut) { | |
| try await session.connect() | |
| } | |
| #expect(clock.now - started < .seconds(2)) | |
| await `#expect`(throws: CmxIrohClientSessionError.dialTimedOut) { | |
| try await session.connect() | |
| } |
🤖 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/CmxIrohClientSessionTests.swift`
around lines 31 - 36, Remove the ContinuousClock setup, started timestamp, and
elapsed-time assertion from CmxIrohClientSessionTests. Keep the await
session.connect() check asserting the typed
CmxIrohClientSessionError.dialTimedOut result; add cancellation completion
signaling only if separate cancellation coverage is required.
Source: Coding guidelines
Comment on lines
+110
to
+120
| // The in-start readiness pass may wait this long for the relay, | ||
| // but a hung credential installation must never block activation | ||
| // or binding publication beyond it. | ||
| automaticRelayReadinessTimeout: .milliseconds(20), | ||
| handleTransport: { session, _ in await session.close() }, | ||
| handleBinding: { _, _, _ in await bindings.record() } | ||
| ) | ||
| let start = Task { try await runtime.start() } | ||
| await gate.waitUntilSuspended() | ||
|
|
||
| try await start.value |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Remove the wall-clock dependency from this test.
automaticRelayReadinessTimeout: .milliseconds(20) makes start.value depend on scheduler timing. CI load can make this test flaky. Inject a controllable relay-readiness clock or readiness signal, then advance or complete it after gate.waitUntilSuspended().
As per coding guidelines, tests must await completion signals or use injected virtual clocks instead of fixed-duration waits.
🤖 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/CmxIrohHostRuntimeLifecycleRaceTests.swift`
around lines 110 - 120, Remove the fixed .milliseconds(20) timeout from the
runtime setup in CmxIrohHostRuntimeLifecycleRaceTests and inject a controllable
relay-readiness clock or completion signal instead. After
gate.waitUntilSuspended(), explicitly advance or complete that readiness
mechanism before awaiting start.value, while preserving the existing transport
and binding handlers.
Source: Coding guidelines
The cold-start scenario proves the same usable-session legs as standard; its launch-to-usable deadline is enforced at the orchestrator script layer, so the probe switch reuses the standard branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Fixes #9724 in three layers.
Summary
automaticRelayReadinessTimeout, default 5s) to become usable, so the binding published at activation carries post-attach dialable hints instead of the half-ready bootstrap snapshot phones were racing. Only the cancellation-safewaitForUsableHomeRelayis awaited; timeout, broker error, or a hung credential install publishes the bootstrap policy exactly as before and leaves repair to the standard refresh-on-online path. Direct-only startup still skips readiness; the strict relay-only barrier is unchanged.CmxIrohClientSession.establishConnectionawaitedendpoint.connectwith no clock on either the public or the private-fallback phase, which is where the dogfood trace burned a 16.2s hang against stale hints. Each phase now races the fork's cancellable ConnectAttempt againstdialPhaseTimeout(default 5s); expiry cancels the FFI dial promptly, surfacesdialTimedOut(diagnostic kindtimedOut), and a timed-out public phase still falls through to the private fallback.--mode cold-start(GATE_SCENARIO=cold_start_dial) pins the launch-to-usable deadline to 20s on the gate's phone launch against the freshly relaunched Mac, making this race a permanent tripwire. Also selectable in the iroh-release-gate workflow matrix.Evidence base: decoded cmuxdiag timeline from the PR 9430 dogfood round (discovery succeeded at t+7s; dial 1 connected then failed pairing after 7.6s; dial 2 hung 16.2s; dial 3 connected in 455ms; usable at ~t+43s), full decode in the issue.
Verification
Build and behavior verification (2026-08-07, tag irdy)
Pending verification
--mode cold-start) on this branch.Deferred (documented in the issue)
readyflag on iroh_endpoint_bindings needs a schema migration via the cloud-vm-ops flow; this PR is client-only.Notes
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Note
Cursor Bugbot is generating a summary for commit 1592f48. Configure here.
Summary by cubic
Prevents long post-restart dial hangs by publishing relay-ready hints during automatic host startup and by bounding each dial phase. Adds a cold-start release gate that relaunches the Mac and enforces a 20s launch-to-usable deadline via the orchestrator.
Bug Fixes
CmxIrohHostRuntimeeagerly activates relay and waits up to 5s for readiness before publishing; on timeout/error it publishes bootstrap and refreshes later. Direct-only behavior is unchanged.CmxIrohClientSessionbounds each dial phase withdialPhaseTimeout(default 5s); expiry cancels the connect, surfacesdialTimedOut, and public timeouts still fall back to private paths..timedOutfordialTimedOut.New Features
cold-start(cold_start_dial) with a 20s launch-to-usable deadline; added to the workflow matrix, runner admission, probe switch, andscripts/run-iroh-release-gate.sh. The probe reuses standard usability checks; timing is enforced by the script.Written for commit 1cd310b. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests