Repository navigation
Register-when-ready Iroh hosts, bounded dial phases, cold-start release gate #9752
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6c2984a
feb58d3
a2ca769
1592f48
1cd310b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,12 @@ public actor CmxIrohClientSession { | |
| public typealias PrivateFallbackContextProvider = @Sendable () async throws -> CmxIrohClientContext | ||
|
|
||
| private let endpoint: any CmxIrohEndpoint | ||
| /// Bound on each dial phase. A peer that has not answered by then is | ||
| /// almost always a stale-hint or half-ready target (issue 9724 recorded a | ||
| /// 16s hang against a just-relaunched Mac); failing fast hands control to | ||
| /// the retry layer, which redials with fresher hints. Cancellation crosses | ||
| /// the FFI boundary, so the raced attempt aborts promptly. | ||
| private let dialPhaseTimeout: Duration | ||
| private let targetIdentity: CmxIrohPeerIdentity | ||
| private let dialPlan: CmxIrohDialPlan | ||
| private let credential: CmxIrohAdmissionCredential | ||
|
|
@@ -44,6 +50,7 @@ public actor CmxIrohClientSession { | |
| privateFallbackAuthorization: CmxIrohPrivateFallbackAuthorization? = nil, | ||
| privateFallbackValidator: (any CmxIrohPrivateFallbackValidating)? = nil, | ||
| privateFallbackContextProvider: PrivateFallbackContextProvider? = nil, | ||
| dialPhaseTimeout: Duration = .seconds(5), | ||
| protocolConfiguration: CmxIrohProtocolConfiguration = .cmuxMobileV1 | ||
| ) throws { | ||
| self.endpoint = endpoint | ||
|
|
@@ -53,6 +60,7 @@ public actor CmxIrohClientSession { | |
| self.privateFallbackAuthorization = privateFallbackAuthorization | ||
| self.privateFallbackValidator = privateFallbackValidator | ||
| self.privateFallbackContextProvider = privateFallbackContextProvider | ||
| self.dialPhaseTimeout = dialPhaseTimeout | ||
| self.protocolConfiguration = protocolConfiguration | ||
| headerCodec = try CmxIrohStreamHeaderCodec(configuration: protocolConfiguration) | ||
| } | ||
|
|
@@ -278,17 +286,42 @@ public actor CmxIrohClientSession { | |
| controlReceiveBuffer.removeAll(keepingCapacity: false) | ||
| } | ||
|
|
||
| /// Dials one address within `dialPhaseTimeout`, cancelling the attempt at | ||
| /// the bound so it cannot outlive the budget on a hung or stale path. | ||
| private func connectBounded( | ||
| to address: CmxIrohEndpointAddress | ||
| ) async throws -> any CmxIrohConnection { | ||
| let endpoint = endpoint | ||
| let alpn = protocolConfiguration.alpn | ||
| let bound = dialPhaseTimeout | ||
| return try await withThrowingTaskGroup( | ||
| of: (any CmxIrohConnection)?.self | ||
| ) { group in | ||
| group.addTask { | ||
| try await endpoint.connect(to: address, alpn: alpn) | ||
| } | ||
| group.addTask { | ||
| try await ContinuousClock().sleep(for: bound) | ||
| return nil | ||
| } | ||
| defer { group.cancelAll() } | ||
| guard let first = try await group.next(), let connection = first else { | ||
| throw CmxIrohClientSessionError.dialTimedOut | ||
| } | ||
| return connection | ||
| } | ||
| } | ||
|
|
||
| private func establishConnection() async throws -> CmxIrohConnectedControl { | ||
| var establishedConnection: (any CmxIrohConnection)? | ||
| var publicConnectionError: (any Error)? | ||
| if !dialPlan.publicPaths.isEmpty { | ||
| do { | ||
| establishedConnection = try await endpoint.connect( | ||
| establishedConnection = try await connectBounded( | ||
| to: CmxIrohEndpointAddress( | ||
| identity: targetIdentity, | ||
| pathHints: dialPlan.publicPaths | ||
| ), | ||
| alpn: protocolConfiguration.alpn | ||
| ) | ||
|
Comment on lines
+320
to
+324
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Add coverage for timeout-to-private fallback. A public Add a regression test with Based on the PR objective, a timed-out public dial must proceed to private fallback. 🤖 Prompt for AI Agents |
||
| ) | ||
| } catch { | ||
| try Task.checkCancellation() | ||
|
|
@@ -326,12 +359,11 @@ public actor CmxIrohClientSession { | |
| authorization | ||
| ) | ||
| try Task.checkCancellation() | ||
| establishedConnection = try await endpoint.connect( | ||
| establishedConnection = try await connectBounded( | ||
| to: CmxIrohEndpointAddress( | ||
| identity: targetIdentity, | ||
| pathHints: fallbackPaths | ||
| ), | ||
| alpn: protocolConfiguration.alpn | ||
| ) | ||
| ) | ||
| } | ||
| guard let establishedConnection else { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -15,6 +15,27 @@ struct CmxIrohClientSessionTests { | |||||||||||||||||||
| credential = try .pairGrant("e30.e30.AA") | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| @Test("a hung dial fails at the phase bound instead of hanging") | ||||||||||||||||||||
| func hungDialFailsAtThePhaseBound() async throws { | ||||||||||||||||||||
| let endpoint = TestDialingIrohEndpoint( | ||||||||||||||||||||
| localIdentity: localIdentity, | ||||||||||||||||||||
| dialResults: [.hang] | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| let session = try CmxIrohClientSession( | ||||||||||||||||||||
| endpoint: endpoint, | ||||||||||||||||||||
| targetIdentity: remoteIdentity, | ||||||||||||||||||||
| dialPlan: try testIrohDialPlan(publicPaths: [try publicRelayHint()]), | ||||||||||||||||||||
| credential: credential, | ||||||||||||||||||||
| dialPhaseTimeout: .milliseconds(40) | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| let clock = ContinuousClock() | ||||||||||||||||||||
| let started = clock.now | ||||||||||||||||||||
| await #expect(throws: CmxIrohClientSessionError.dialTimedOut) { | ||||||||||||||||||||
| try await session.connect() | ||||||||||||||||||||
| } | ||||||||||||||||||||
| #expect(clock.now - started < .seconds(2)) | ||||||||||||||||||||
|
Comment on lines
+31
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Remove the wall-clock assertion.
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
Suggested change
🤖 Prompt for AI AgentsSource: Coding guidelines |
||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| @Test | ||||||||||||||||||||
| func publicDialAdmitsControlAndPreservesFollowingRPCBytes() async throws { | ||||||||||||||||||||
| let events = TestIrohEventRecorder() | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -107,16 +107,20 @@ extension CmxIrohHostRuntimeTests { | |
| ), | ||
| configuration: fixture.configuration, | ||
| pendingRevocations: fixture.pendingRevocations(), | ||
| // 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 | ||
|
Comment on lines
+110
to
+120
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift Remove the wall-clock dependency from this test.
As per coding guidelines, tests must await completion signals or use injected virtual clocks instead of fixed-duration waits. 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| #expect(await bindings.count() == 1) | ||
|
|
||
| await gate.resume() | ||
| try await start.value | ||
| await runtime.stop() | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,7 @@ | ||
| enum TestIrohDialResult { | ||
| case connection(TestIrohConnection) | ||
| case failure(TestIrohTransportError) | ||
| /// Suspends until the dial attempt is cancelled, like a peer that never | ||
| /// answers on a stale path hint. | ||
| case hang | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Complete the
cold-startworkflow wiring.The new matrix value skips both automatic-mode setup conditions. It also has no
TAGcase. Withset -u, the gate exits when it expands$TAG.Include
cold-startin the conditions at Lines 83 and 89. Add a uniquecold-start) TAG=... ;;case before invoking the script.Based on the PR objective,
cold-startmust execute the automatic transport gate.🤖 Prompt for AI Agents