Repository navigation
Honor broker Retry-After across iOS endpoint activations - #8557
Conversation
CmxIrohBrokerCooldown carries a broker Retry-After directive across endpoint activation attempts (the credential coordinator honors it only within one activation, and a torn-down runtime discarded it). CmxIrohBrokerCooldownError conforms to CmxRetryAfterProviding so the reconnect scheduler can adopt the server floor. CmxIrohRelayPolicyService.refreshWithCredential returns the broker-minted relay credential alongside the effective policy so activation can install it without a second mint. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesThe PR adds account-scoped broker cooldown handling, exposes relay credentials from policy refresh, integrates cooldown checks and credential selection into mobile runtime activation, adjusts empty-fleet policy behavior, and adds focused tests. Broker runtime flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MobileIrohRuntimeComposition
participant CmxIrohBrokerCooldown
participant CmxIrohRelayPolicyService
participant CmxIrohRelayPolicyServing
MobileIrohRuntimeComposition->>CmxIrohBrokerCooldown: Check account cooldown
MobileIrohRuntimeComposition->>CmxIrohRelayPolicyService: Refresh policy with credential
CmxIrohRelayPolicyService->>CmxIrohRelayPolicyServing: Issue relay bootstrap
CmxIrohRelayPolicyServing-->>CmxIrohRelayPolicyService: Return policy and credential
CmxIrohRelayPolicyService-->>MobileIrohRuntimeComposition: Return RefreshOutcome
MobileIrohRuntimeComposition->>CmxIrohBrokerCooldown: Clear successful account cooldown
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (22 passed)
✨ 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 improves iOS endpoint recovery when broker requests are rate-limited or relay policy is unavailable. The main changes are:
Confidence Score: 5/5No additional blocking issue qualifies for this follow-up review.
Important Files Changed
Reviews (9): Last reviewed commit: "Harden reconnect under broker cooldowns ..." | Re-trigger Greptile |
2d912bc to
983d084
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ios/cmuxPackage/Sources/cmuxFeature/MobileIrohRuntimeComposition.swift (1)
1178-1190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHandle broker cooldown as a non-failure path
CmxIrohBrokerCooldownErroralready maps to.policyUnavailable, so this catch turns expectedRetry-Afterskips into.endpointFailedtelemetry and an error-level log. Handle that error explicitly here, or downgrade it to a non-failure diagnostic/notice.🤖 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/cmuxFeature/MobileIrohRuntimeComposition.swift` around lines 1178 - 1190, Update the error handling around the activation catch in MobileIrohRuntimeComposition so CmxIrohBrokerCooldownError is handled separately from unexpected failures: avoid recording .endpointFailed telemetry and avoid error-level logging, using the existing non-failure diagnostic or notice behavior instead. Preserve the CancellationError path and current failure handling for all other errors.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@ios/cmuxPackage/Sources/cmuxFeature/MobileIrohRuntimeComposition.swift`:
- Around line 1178-1190: Update the error handling around the activation catch
in MobileIrohRuntimeComposition so CmxIrohBrokerCooldownError is handled
separately from unexpected failures: avoid recording .endpointFailed telemetry
and avoid error-level logging, using the existing non-failure diagnostic or
notice behavior instead. Preserve the CancellationError path and current failure
handling for all other errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2df014e5-8f99-4e69-8d7e-60ac5a6bf398
📒 Files selected for processing (3)
Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohClientRuntime+Policy.swiftios/cmuxPackage/Sources/cmuxFeature/MobileIrohRuntimeComposition.swiftios/cmuxPackage/Tests/cmuxFeatureTests/MobileIrohRuntimeCompositionCooldownTests.swift
983d084 to
ba6ef61
Compare
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ios/cmuxPackage/Sources/cmuxFeature/MobileIrohRuntimeComposition.swift (1)
1176-1191: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDon't re-record the synthetic cooldown error here (
MobileIrohRuntimeComposition.swift:1181).CmxIrohBrokerCooldownErroralready conforms toCmxRetryAfterProviding, so this catch feeds the active-cooldown signal back intorecordBrokerCooldown(for:). Becauserecord(...)keeps the later floor andremainingSecondsrounds up, repeated retries can pushretryAtforward on each attempt. Only genuine broker Retry-After errors should update the cooldown.🤖 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/cmuxFeature/MobileIrohRuntimeComposition.swift` around lines 1176 - 1191, The activation error handler around activate and recordBrokerCooldown must avoid re-recording synthetic CmxIrohBrokerCooldownError values. Update recordBrokerCooldown usage so only genuine broker Retry-After errors update the cooldown, while preserving diagnostic logging and failure handling for all other errors.Source: Path instructions
🤖 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.
Outside diff comments:
In `@ios/cmuxPackage/Sources/cmuxFeature/MobileIrohRuntimeComposition.swift`:
- Around line 1176-1191: The activation error handler around activate and
recordBrokerCooldown must avoid re-recording synthetic
CmxIrohBrokerCooldownError values. Update recordBrokerCooldown usage so only
genuine broker Retry-After errors update the cooldown, while preserving
diagnostic logging and failure handling for all other errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 474d059a-c651-4fb4-8b24-c919a7b6395e
📒 Files selected for processing (5)
Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohClientRuntime+Lifecycle.swiftPackages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohClientRuntime+Policy.swiftPackages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohClientRuntimeEmptyFleetTests.swiftios/cmuxPackage/Sources/cmuxFeature/MobileIrohRuntimeComposition.swiftios/cmuxPackage/Tests/cmuxFeatureTests/MobileIrohRuntimeCompositionCooldownTests.swift
Behavior-level regression tests: composition-level (counting broker fake, serialized suite, bounded settle) proving a rate-limited activation floors retries, surfaces Retry-After through dial errors, and re-reaches the broker only after the window; plus a transport-level test proving an empty managed relay fleet (relay policy unavailable) still activates the runtime for registration and direct paths. Red on the current tree: the Retry-After floor is discarded between activations, activation mints twice, and an empty fleet fails activation before any broker call (the unclassified endpointFailed b=255 signature from the field diagnostics). Co-Authored-By: Codex <noreply@openai.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reconnect lockout fix, three legs sharing one mechanism: 1. The composition records an account-scoped CmxIrohBrokerCooldown when any activation leg fails with a Retry-After directive, gates the next activation on it (zero broker calls while floored, retryScheduled diagnostic), and surfaces the remaining floor through the inactive-runtime dial errors as CmxIrohBrokerCooldownError. The shell's existing CmxRetryAfterProviding seam then adopts the server floor instead of its 2-64s transient backoff, so a rate-limited phone stops re-exhausting the server's fixed rate-limit window on every retry. 2. One activation now costs one relay-token mint: refreshWithCredential returns the bootstrap-minted credential and the runtime configuration prefers it (fleet-compatible) over the disk cache, so the credential coordinator installs it instead of minting again. 3. An unavailable relay policy no longer kills activation: with an empty managed fleet the offline-policy expectation is skipped and the discovery fleet cross-check is bypassed (nothing verified to compare, no relay gets configured), so registration and direct LAN paths proceed without relays instead of failing with invalidExpectation/relayFleetMismatch before or during the first broker round trip. Co-Authored-By: Codex <noreply@openai.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ba6ef61 to
e9471c2
Compare
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
- The registry context provider no longer rejects dial contexts when the verified managed fleet is empty (relay policy unavailable): nothing can be cross-checked and allowedRouteRelayURLs is empty so no relay hint survives, while direct dial plans stay valid. Without this, the empty-fleet activation reached .active but every dial threw relayFleetMismatch, making the direct-only fallback unusable. Regression test proven red pre-fix. - The cooldown gate's synthesized error is no longer recorded as a fresh server directive, so sub-second retry pressure cannot slide the deadline forward indefinitely. Regression test hammers the gate late in the window and proves expiry stays on schedule. - A rate-limited relay bootstrap's remaining Retry-After now seeds the relay policy refresh scheduler when activation succeeds through the cached path, so the scheduler's default 30s cadence cannot retry the rate-limited endpoint early; the activation gate itself stays cleared because the endpoint is healthy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ios/cmuxPackage/Tests/cmuxFeatureTests/MobileIrohRuntimeCompositionCooldownTests.swift (1)
16-29: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftReplace the fixed-sleep settle loop with a completion signal.
The 500-iteration sleep loop is not deadline-bounded and can be flaky under load. Have the fixture signal broker activity/runtime activation, or poll the predicate until an explicit deadline.
As per coding guidelines, tests must await real completion signals or use deadline-bounded polling of real predicates rather than 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 `@ios/cmuxPackage/Tests/cmuxFeatureTests/MobileIrohRuntimeCompositionCooldownTests.swift` around lines 16 - 29, Replace the fixed 500-iteration sleep loop in settleActivation with either a fixture-provided completion signal for broker activity/runtime activation or polling of the existing condition until an explicit deadline. Ensure the helper returns promptly when the predicate completes and fails deterministically when the deadline expires, without relying on a fixed-duration retry count.Source: Coding guidelines
🤖 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 `@ios/cmuxPackage/Sources/cmuxFeature/MobileIrohRuntimeComposition.swift`:
- Around line 2136-2151: Refactor the relay-policy retry flow initialized by the
surrounding scheduler method so deadline ownership and next-eligible signaling
live in a cancellable scheduler/actor rather than the MainActor Task. Keep the
MainActor task limited to receiving and applying results, and remove its
retry-backoff or delayed-coordination Task.sleep usage while preserving
initialRetryAfterSeconds behavior.
In
`@Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohRegistryContextProvider.swift`:
- Around line 145-149: Update the validation guard in
CmxIrohRegistryContextProvider so the bypass is allowed only when both
managedRelayURLs and allowedRouteRelayURLs are empty; otherwise require the
broker-reported fleet to match the authoritative managed relay set. Preserve
direct dial behavior for the fully empty case, fail closed for the inconsistent
empty-managed/non-empty-allowed state, and add a regression test covering that
combination.
---
Outside diff comments:
In
`@ios/cmuxPackage/Tests/cmuxFeatureTests/MobileIrohRuntimeCompositionCooldownTests.swift`:
- Around line 16-29: Replace the fixed 500-iteration sleep loop in
settleActivation with either a fixture-provided completion signal for broker
activity/runtime activation or polling of the existing condition until an
explicit deadline. Ensure the helper returns promptly when the predicate
completes and fails deterministically when the deadline expires, without relying
on a fixed-duration retry count.
🪄 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
Run ID: 1a3303af-8dc7-4c51-94cf-a936da094565
📒 Files selected for processing (4)
Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohRegistryContextProvider.swiftPackages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohRegistryContextProviderPolicyTests.swiftios/cmuxPackage/Sources/cmuxFeature/MobileIrohRuntimeComposition.swiftios/cmuxPackage/Tests/cmuxFeatureTests/MobileIrohRuntimeCompositionCooldownTests.swift
| revision: UInt64, | ||
| initialRetryAfterSeconds: Int? = nil | ||
| ) { | ||
| relayPolicyRefreshTask?.cancel() | ||
| guard let service, let trustRoot else { | ||
| relayPolicyRefreshTask = nil | ||
| return | ||
| } | ||
| // A Retry-After from the activation-time bootstrap failure seeds the | ||
| // first attempt so the scheduler's default cadence cannot retry the | ||
| // rate-limited endpoint early. | ||
| let initialRetryAt = initialRetryAfterSeconds.map { | ||
| now().addingTimeInterval(TimeInterval($0)) | ||
| } | ||
| relayPolicyRefreshTask = Task { @MainActor [weak self] in | ||
| var retryAt: Date? | ||
| var retryAt: Date? = initialRetryAt |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Move retry timing out of this MainActor task.
initialRetryAfterSeconds adds an activation-time retry path that subsequently waits via Task.sleep in this production scheduler. Move deadline ownership and next-eligible signaling to a cancellable scheduler/actor; keep MainActor work limited to applying results.
As per coding guidelines, production Swift must not materially expand Task.sleep for retry backoff or delayed coordination.
🤖 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/cmuxFeature/MobileIrohRuntimeComposition.swift`
around lines 2136 - 2151, Refactor the relay-policy retry flow initialized by
the surrounding scheduler method so deadline ownership and next-eligible
signaling live in a cancellable scheduler/actor rather than the MainActor Task.
Keep the MainActor task limited to receiving and applying results, and remove
its retry-backoff or delayed-coordination Task.sleep usage while preserving
initialRetryAfterSeconds behavior.
Source: Coding guidelines
| // Without a verified managed fleet there is nothing to cross-check and | ||
| // allowedRouteRelayURLs is empty, so no relay hint survives filtering; | ||
| // direct dial plans stay valid while relays remain unusable. | ||
| guard managedRelayURLs.isEmpty | ||
| || Set(discovery.relayFleet) == managedRelayURLs else { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not bypass validation unless the allowed relay set is also empty.
managedRelayURLs and allowedRouteRelayURLs are independent state: callers can provide or update them separately. If managedRelayURLs is empty while allowedRouteRelayURLs is non-empty, this guard accepts any broker-reported fleet, and the downstream dial-plan builder can still retain those allowed relay routes.
Fail closed on that inconsistent state or normalize both values at the source before skipping the cross-check. Add a regression test for this combination.
As per path instructions, correctness-critical relay eligibility must have one authoritative source of truth and must not rely on a permissive 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/CmxIrohRegistryContextProvider.swift`
around lines 145 - 149, Update the validation guard in
CmxIrohRegistryContextProvider so the bypass is allowed only when both
managedRelayURLs and allowedRouteRelayURLs are empty; otherwise require the
broker-reported fleet to match the authoritative managed relay set. Preserve
direct dial behavior for the fully empty case, fail closed for the inconsistent
empty-managed/non-empty-allowed state, and add a regression test covering that
combination.
Source: Path instructions
- The relay credential coordinator accepts a mintNotBefore floor, plumbed from the composition's cooldown through the runtime configuration, so the in-activation credential mint waits out a Retry-After from the same endpoint's bootstrap failure instead of spending a guaranteed rejection. Coordinator test proves the first mint sleeps to the floor. - The account cooldown survives a successful direct-only activation: live dials bypass the gate through the active runtime, while a torn-down runtime can no longer reach the rate-limited broker before Retry-After expires (the refresh-scheduler seed alone died with the runtime). - refreshWithCredential returns the resolution-validated bootstrap credential rather than the raw response, so a rejected credential can never displace a valid cached one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Third structured-review round: the Retry-After floor was copied into one-shot schedules instead of enforced by the broker-request owner. - The credential coordinator now retains the floor as actor state, extends it from every observed Retry-After, and enforces it at every mint path: the no-bootstrap first mint, bootstrap-failure retries, the post-install refresh schedule, and on-demand refreshIfNeeded (which reschedules to the floor instead of spending a guaranteed rejection). - Observed directives flow back through the new handleRelayRateLimit runtime callback into the composition's account-scoped cooldown, so a Retry-After received inside the coordinator survives runtime teardown. - The relay policy refresh scheduler's seeded wake is staggered past the coordinator's floor-expiry mint so the endpoint's two consumers never wake together. Coordinator tests prove the deferred first mint and the floored post-bootstrap schedule. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Two internal-build users hit a permanent iPhone-to-Mac reconnect lockout: the Computers sheet shows the Mac Online (presence is a separate service) but every reconnect cycle fails, even with the Mac reachable on the same LAN. Their cmuxdiag exports show every recovery cycle failing with
relayPolicyRefreshFailed/endpointFailedkindpolicyUnavailable(one log also shows the unclassifiedendpointFailed b=255), with retry cycles every 2-64 seconds.Two production bugs compose into the lockout:
Retry-After: 600, only the in-activation credential coordinator honors the floor; the activation error is swallowed, later dials throw a bareinactive, and the reconnect scheduler falls back to its 2-64s transient backoff. Every retry re-runs the full broker call set, so the server's fixed window never drains. b2899a11a2 fixed one duplicate call site (discovery reuse); this PR fixes the activation path.CmxIrohClientOfflinePolicyExpectationthrowsinvalidExpectationbefore any broker call (the unclassifiedendpointFailed b=255in the field diagnostics), and even past that,validateRelayFleetrejects the discovery response. A phone in this state cannot reconnect at all, not even LAN-direct.Changes:
CmxIrohBrokerCooldown(CmuxIrohTransport): account-scoped floor carrying a brokerRetry-Afterdirective across endpoint activations, withCmxIrohBrokerCooldownErrorconforming toCmxRetryAfterProviding.CmxIrohRelayPolicyService.refreshWithCredential: returns the broker-minted relay credential alongside the effective policy so activation installs it without a second mint.MobileIrohRuntimeComposition: gates activation on the cooldown (zero broker calls while floored,retryScheduleddiagnostic), records the floor from rate-limited refresh/registration failures while keeping the cached-policy restore path, prefers the fresh fleet-compatible credential for the runtime configuration, and surfaces the remaining floor through the inactive-runtime dial errors.MobileShellComposite.recordAutomaticReconnectBackoffalready adoptsCmxRetryAfterProvidingfloors, so the reconnect scheduler now waits out the server directive instead of retrying into it.CmxIrohClientRuntime: with an empty managed fleet, the offline-policy expectation is skipped and the discovery fleet cross-check is bypassed (nothing verified to compare, no relay gets configured), so registration and direct LAN paths proceed without relays.With the fix, a rate-limited phone makes at most one activation's worth of broker calls per
Retry-Afterwindow and recovers cleanly at the floor, and a phone without a usable relay policy still reconnects over direct paths.Tests, red first (Commits tab): a transport-level empty-fleet activation test (red/green verified locally both ways), and a composition-level cooldown suite with a counting broker fake (serialized, bounded settle; verified on an isolated runner: red on the tests-only commit, green on this head). The suite is serialized because three parallel composition instances contend on the main actor in-process; noted for a follow-up look at composition lifecycle liveness under test parallelism.
Out of scope, follow-ups: Mac-host runtime parity for the same cooldown; the server-side rate-limit keying change (infra config, tracked separately).
Localization audit: no user-facing strings changed (transport/composition/tests only).
🤖 Generated with Claude Code
Summary by CodeRabbit