Repository navigation
iOS Iroh: bounded dials, hint-refresh fallback, fail-open credential refresh #10739
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
6ed798b
01c9410
d5ad101
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 |
|---|---|---|
|
|
@@ -431,56 +431,71 @@ public actor CmxIrohClientSession { | |
|
|
||
| do { | ||
| try Task.checkCancellation() | ||
| guard await establishedConnection.remoteIdentity() == targetIdentity else { | ||
| throw CmxIrohClientSessionError.remoteIdentityMismatch | ||
| // A half-ready peer can accept the QUIC connection and then never | ||
| // serve the admission frames. Bound the whole barrier like a dial | ||
| // phase so a silent peer hands control back to recovery instead | ||
| // of holding the redial owner open-endedly (cmux#9724). | ||
| return try await boundedByDialPhase { [weak self] in | ||
| guard let self else { throw CancellationError() } | ||
| return try await self.performAdmissionBarrier( | ||
| on: establishedConnection | ||
| ) | ||
| } | ||
| } catch { | ||
| await establishedConnection.close(errorCode: 1, reason: "admission_failed") | ||
| throw error | ||
| } | ||
| } | ||
|
|
||
| private func performAdmissionBarrier( | ||
| on establishedConnection: any CmxIrohConnection | ||
| ) async throws -> CmxIrohConnectedControl { | ||
| guard await establishedConnection.remoteIdentity() == targetIdentity else { | ||
| throw CmxIrohClientSessionError.remoteIdentityMismatch | ||
| } | ||
| try await establishedConnection.setIncomingStreamLimits( | ||
| maximumBidirectionalStreamCount: 0, | ||
| maximumUnidirectionalStreamCount: 0 | ||
| ) | ||
| let stream = try await establishedConnection.openBidirectionalStream() | ||
| let header = try CmxIrohStreamHeader( | ||
| lane: .control, | ||
| credential: credential | ||
| ) | ||
| try await stream.sendStream.send(headerCodec.encode(header)) | ||
| let admission = try await readAdmissionFrame(from: stream.receiveStream) | ||
| switch admission.frame { | ||
| case .acceptedPendingNatTraversal, .acceptedRelayOnly: | ||
| if admission.frame == .acceptedPendingNatTraversal { | ||
| try Task.checkCancellation() | ||
| try await establishedConnection.authorizeNatTraversal() | ||
| } | ||
| try await establishedConnection.setIncomingStreamLimits( | ||
| maximumBidirectionalStreamCount: 0, | ||
| maximumUnidirectionalStreamCount: 0 | ||
| try Task.checkCancellation() | ||
| try await stream.sendStream.send( | ||
| admissionCodec.encodeFrame(.clientReady) | ||
| ) | ||
| let stream = try await establishedConnection.openBidirectionalStream() | ||
| let header = try CmxIrohStreamHeader( | ||
| lane: .control, | ||
| credential: credential | ||
| let confirmation = try await readAdmissionFrame( | ||
| from: stream.receiveStream, | ||
| initialBuffer: admission.trailingBytes | ||
| ) | ||
| try await stream.sendStream.send(headerCodec.encode(header)) | ||
| let admission = try await readAdmissionFrame(from: stream.receiveStream) | ||
| switch admission.frame { | ||
| case .acceptedPendingNatTraversal, .acceptedRelayOnly: | ||
| if admission.frame == .acceptedPendingNatTraversal { | ||
| try Task.checkCancellation() | ||
| try await establishedConnection.authorizeNatTraversal() | ||
| } | ||
| try Task.checkCancellation() | ||
| try await stream.sendStream.send( | ||
| admissionCodec.encodeFrame(.clientReady) | ||
| ) | ||
| let confirmation = try await readAdmissionFrame( | ||
| from: stream.receiveStream, | ||
| initialBuffer: admission.trailingBytes | ||
| ) | ||
| switch confirmation.frame { | ||
| case .serverReady: | ||
| break | ||
| case let .denied(code): | ||
| throw CmxIrohClientSessionError.admissionDenied(code: code) | ||
| case .acceptedPendingNatTraversal, .acceptedRelayOnly, .clientReady: | ||
| throw CmxIrohClientSessionError.invalidAdmissionFrame | ||
| } | ||
| try Task.checkCancellation() | ||
| return CmxIrohConnectedControl( | ||
| connection: establishedConnection, | ||
| stream: stream, | ||
| initialReceiveBuffer: confirmation.trailingBytes | ||
| ) | ||
| switch confirmation.frame { | ||
| case .serverReady: | ||
| break | ||
| case let .denied(code): | ||
| throw CmxIrohClientSessionError.admissionDenied(code: code) | ||
| case .clientReady, .serverReady: | ||
| case .acceptedPendingNatTraversal, .acceptedRelayOnly, .clientReady: | ||
| throw CmxIrohClientSessionError.invalidAdmissionFrame | ||
| } | ||
| } catch { | ||
| await establishedConnection.close(errorCode: 1, reason: "admission_failed") | ||
| throw error | ||
| try Task.checkCancellation() | ||
| return CmxIrohConnectedControl( | ||
| connection: establishedConnection, | ||
| stream: stream, | ||
| initialReceiveBuffer: confirmation.trailingBytes | ||
| ) | ||
| case let .denied(code): | ||
| throw CmxIrohClientSessionError.admissionDenied(code: code) | ||
| case .clientReady, .serverReady: | ||
| throw CmxIrohClientSessionError.invalidAdmissionFrame | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -498,22 +513,32 @@ public actor CmxIrohClientSession { | |
| ) async throws -> any CmxIrohConnection { | ||
| let endpoint = endpoint | ||
| let alpn = protocolConfiguration.alpn | ||
| return try await boundedByDialPhase { | ||
| try await endpoint.connect(to: address, alpn: alpn) | ||
| } | ||
| } | ||
|
|
||
| /// Races one dial-phase operation against the injected phase bound. The | ||
| /// operation must cancel cooperatively; on timeout the loser is cancelled | ||
| /// and the phase fails typed so the redial machinery supersedes it | ||
| /// instead of wedging behind it (cmux#8531). | ||
| private func boundedByDialPhase<Value: Sendable>( | ||
| _ operation: @escaping @Sendable () async throws -> Value | ||
| ) async throws -> Value { | ||
| let bound = dialPhaseTimeout | ||
| return try await withThrowingTaskGroup( | ||
| of: (any CmxIrohConnection)?.self | ||
| ) { group in | ||
| return try await withThrowingTaskGroup(of: Value?.self) { group in | ||
|
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. P1: When the native admission read does not cooperatively cancel, this timeout does not bound the dial: structured task-group teardown waits for Prompt for AI agents |
||
| group.addTask { | ||
| try await endpoint.connect(to: address, alpn: alpn) | ||
| try await operation() | ||
| } | ||
| group.addTask { | ||
| try await ContinuousClock().sleep(for: bound) | ||
| return nil | ||
| } | ||
| defer { group.cancelAll() } | ||
| guard let first = try await group.next(), let connection = first else { | ||
| guard let first = try await group.next(), let value = first else { | ||
| throw CmxIrohClientSessionError.dialTimedOut | ||
| } | ||
| return connection | ||
| return value | ||
|
Comment on lines
+525
to
+541
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 🔎 Supported by static analysis🏁 Script executed: sed -n '470,555p' Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohClientSession.swift
printf '\n--- test excerpts ---\n'
sed -n '80,165p' Packages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohClientSessionDialBoundTests.swift
printf '\n--- relevant symbols ---\n'
rg -n -C 3 'dialPhaseTimeout|boundedByDialPhase|ContinuousClock|Task\.sleep|CmxIrohClientSessionDialBoundTests' \
Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport \
Packages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTestsRepository: manaflow-ai/cmux Length of output: 50372 🏁 Script executed: printf '%s\n' '--- client session declarations and initializer ---'
sed -n '1,85p' Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohClientSession.swift
printf '%s\n' '--- dial-bound test setup ---'
sed -n '1,125p' Packages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohClientSessionDialBoundTests.swift
printf '%s\n' '--- clock abstractions in this package ---'
rg -n -g '*.swift' 'any Clock|Clock<|TestClock|SuspendingClock|ContinuousClock' \
Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport \
Packages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTestsRepository: manaflow-ai/cmux Length of output: 14467 🏁 Script executed: printf '%s\n' '--- admission path ---'
sed -n '400,465p' Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohClientSession.swift
printf '%s\n' '--- dial stub ---'
sed -n '1,70p' Packages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/TestDialingIrohEndpoint.swift
printf '%s\n' '--- focused diff ---'
git diff -- Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohClientSession.swift \
Packages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohClientSessionDialBoundTests.swiftRepository: manaflow-ai/cmux Length of output: 5281 Inject the dial-phase clock and remove fixed-time test gates.
📍 Affects 2 files
🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -193,6 +193,28 @@ public actor CmxIrohRegistryContextProvider: CmxIrohClientContextProvider { | |||||||
| ) | ||||||||
| usedFreshDiscovery = true | ||||||||
| } catch { | ||||||||
| try Task.checkCancellation() | ||||||||
| // The refresh failed. Dialing with the last verified snapshot | ||||||||
| // beats not dialing at all (cmux#9724): the staleness mark | ||||||||
| // survives, so the next attempt still refetches once the | ||||||||
| // broker recovers. Verification is not weakened; this | ||||||||
| // snapshot passed the same checks when it was fetched. | ||||||||
| if let lastGood = authoritativeDiscovery { | ||||||||
|
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. P2: The new lastGood fallback runs for every error thrown by sharedDiscover, while the two fallbacks just below it are gated on Prompt for AI agents
Suggested change
|
||||||||
| do { | ||||||||
| return try await resolveContext( | ||||||||
| for: request, | ||||||||
| targetIdentity: targetIdentity, | ||||||||
| routeHints: routeHints, | ||||||||
| discovery: lastGood, | ||||||||
|
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. P2: The lastGood fallback reads Prompt for AI agents |
||||||||
| at: clock | ||||||||
| ) | ||||||||
| } catch is CancellationError { | ||||||||
| throw CancellationError() | ||||||||
| } catch { | ||||||||
|
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. P1: When the authority sync returns a reset or inconsistent revision, Prompt for AI agents
Suggested change
|
||||||||
| // The last-good snapshot no longer authorizes this | ||||||||
| // peer; fall through to the offline cache. | ||||||||
| } | ||||||||
|
Comment on lines
+203
to
+216
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. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win Preserve retry directives from fallback resolution. Lines 203-216 suppress every non-cancellation error from Propagate retry-directive errors from 🤖 Prompt for AI Agents |
||||||||
| } | ||||||||
| guard Self.isConnectivity(error), | ||||||||
| let cached = try await cachedPolicy( | ||||||||
| for: request, | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -374,6 +374,14 @@ public actor CmxIrohRelayCredentialCoordinator { | |
| ), | ||
| initialFailureCount: 1 | ||
| ) | ||
| // Fail open while a last-good credential is installed on the | ||
| // endpoint: a failed mint must not fail the caller's foreground | ||
| // path into zero-route dial churn (cmux#10375). The bounded retry | ||
| // loop above keeps refreshing in the background, and the relay is | ||
| // the authority that rejects a credential that truly went bad. | ||
| if installedCredential != nil, !(error is CancellationError) { | ||
| return | ||
| } | ||
|
Comment on lines
+382
to
+384
Contributor
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.
When foreground activation finds an expired installed credential and minting its replacement fails permanently, this branch returns success solely because Knowledge Base Used: Remote connectivity |
||
| throw error | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -97,16 +97,47 @@ public actor CmxIrohRelayPolicyCache { | |||||||
| /// - Parameters: | ||||||||
| /// - trustRoot: App-pinned public verification keys. | ||||||||
| /// - now: Verification time. | ||||||||
| /// - expiredPolicyReuseGrace: Bounded fail-open window after the signed | ||||||||
| /// expiry in which the last-good policy still loads. The policy is | ||||||||
| /// re-verified at its final valid instant, so signature, rollback, | ||||||||
| /// and claim checks run unweakened; only the expiry gate is graced | ||||||||
| /// (cmux#10375). Zero preserves strict expiry. | ||||||||
| /// - Returns: The verified policy, or `nil` when no policy is cached. | ||||||||
| /// - Throws: ``CmxIrohRelayPolicyError`` or a secure-storage error. | ||||||||
| public func load( | ||||||||
| trustRoot: CmxIrohRelayPolicyTrustRoot, | ||||||||
| now: Date | ||||||||
| now: Date, | ||||||||
| expiredPolicyReuseGrace: TimeInterval = 0 | ||||||||
| ) async throws -> CmxIrohManagedRelayPolicy? { | ||||||||
| await acquire() | ||||||||
| defer { release() } | ||||||||
| guard let record = try await storedRecord() else { return nil } | ||||||||
| let policy = try verifier.verify(record.signedPolicy, trustRoot: trustRoot, now: now) | ||||||||
| let policy: CmxIrohManagedRelayPolicy | ||||||||
| do { | ||||||||
| policy = try verifier.verify( | ||||||||
| record.signedPolicy, | ||||||||
| trustRoot: trustRoot, | ||||||||
| now: now | ||||||||
| ) | ||||||||
| } catch CmxIrohRelayPolicyError.expired { | ||||||||
| // The recorded expiry is cross-checked against the signed claims | ||||||||
| // below; a record that overstates it re-fails as expired here or | ||||||||
| // as rollback below. | ||||||||
| guard expiredPolicyReuseGrace > 0, | ||||||||
|
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. P2: When Prompt for AI agents
Suggested change
|
||||||||
| let recordedExpiry = record.expiresAt, | ||||||||
| now.timeIntervalSince1970 | ||||||||
| <= TimeInterval(recordedExpiry) + expiredPolicyReuseGrace else { | ||||||||
| throw CmxIrohRelayPolicyError.expired | ||||||||
| } | ||||||||
| let lastValidInstant = Date( | ||||||||
| timeIntervalSince1970: TimeInterval(recordedExpiry) - 1 | ||||||||
| ) | ||||||||
| policy = try verifier.verify( | ||||||||
| record.signedPolicy, | ||||||||
| trustRoot: trustRoot, | ||||||||
| now: min(now, lastValidInstant) | ||||||||
| ) | ||||||||
| } | ||||||||
| guard policy.sequence == record.highestSequence, | ||||||||
| Self.metadataMatches(policy, record: record) else { | ||||||||
| throw CmxIrohRelayPolicyError.rollback | ||||||||
|
|
||||||||
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.
P2: The admission bound only fires if the underlying operation terminates when its task is cancelled.
boundedByDialPhaseuseswithThrowingTaskGroup, which on timeout callscancelAll()and then blocks at scope exit until every child task actually completes, so a non-cooperativeRecvStream.read(IrohLibCmxIrohLibReceiveStream.receive→driver.read(sizeLimit:)) would keep the whole phase hung — the same unbounded-admission hang this PR is meant to fix. The dial path already relied on this pattern, but extending it to the admission barrier is a new dependence onRecvStream.readhonoring Swift task cancellation. The new tests cannot catch this:TestHangingIrohReceiveStream/SlowIrohReceiveStreamhang onTask.sleep, which is trivially cancellable, unlike the real FFI stream. Consider verifying that IrohLib'sRecvStream.readaborts on task cancellation (or wrapping it in awithTaskCancellationHandler/try Task.checkCancellation()-aware guard) before relying on this bound in production.Prompt for AI agents