Stagger Iroh relay credential refreshes - #9581
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds deterministic, endpoint-specific relay refresh schedules. Host and client runtimes pass schedule-based deadline callbacks to the managed relay credential coordinator. Tests verify role separation, deadline bounds, and slot distribution. ChangesRelay refresh scheduling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant CmxIrohHostRuntime
participant CmxIrohClientRuntime
participant CmxIrohRelayRefreshSchedule
participant ManagedRelayCredentialCoordinator
CmxIrohHostRuntime->>CmxIrohRelayRefreshSchedule: Create host schedule for endpoint identity
CmxIrohClientRuntime->>CmxIrohRelayRefreshSchedule: Create client schedule for endpoint identity
CmxIrohHostRuntime->>ManagedRelayCredentialCoordinator: Provide deadline callback
CmxIrohClientRuntime->>ManagedRelayCredentialCoordinator: Provide deadline callback
ManagedRelayCredentialCoordinator->>CmxIrohRelayRefreshSchedule: Calculate refresh deadline
CmxIrohRelayRefreshSchedule-->>ManagedRelayCredentialCoordinator: Return scheduled deadline
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (24 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@Packages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohRelayCredentialCoordinatorTests.swift`:
- Around line 47-63: Extend refreshSlotsSpreadEndpointsWithinEachRole to create
a client identity cohort using CmxIrohRelayRefreshSchedule with role .client,
then compute its refresh slots like the host cohort. Assert the client slots
contain more than one distinct value and every slot falls within 30...44, while
preserving the existing host assertions.
🪄 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: de629dd6-5b47-4bf4-aba9-9c157f9377a9
📒 Files selected for processing (4)
Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohClientRuntime+RelayPolicy.swiftPackages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohHostRuntime+RelayPolicy.swiftPackages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohRelayRefreshSchedule.swiftPackages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohRelayCredentialCoordinatorTests.swift
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 (2)
Packages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohRelayCredentialCoordinatorTests.swift (2)
26-37: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert slot stability across refresh cycles.
The loop calls each schedule with the same endpoint identity eight times, but it checks only window membership. A non-deterministic scheduler could select a different second on every call and still pass. Record the first host and client seconds, then assert that later cycles return the same values.
Suggested assertions
+ var hostSeconds: [Int] = [] + var clientSeconds: [Int] = [] + for cycle in 1 ... 8 { ... let clientSecond = Int(clientDeadline.timeIntervalSince1970) % 60 + hostSeconds.append(hostSecond) + clientSeconds.append(clientSecond) ... } + `#expect`(Set(hostSeconds).count == 1) + `#expect`(Set(clientSeconds).count == 1)🤖 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/CmxIrohRelayCredentialCoordinatorTests.swift` around lines 26 - 37, Update the refresh-cycle loop in CmxIrohRelayCredentialCoordinatorTests to capture the first hostSecond and clientSecond values, then assert on subsequent cycles that each schedule returns the same second for the unchanged endpoint identity. Preserve the existing deadline and window-membership checks.
41-42: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the lower deadline bound.
CmxIrohRelayRefreshSchedule.deadlineclamps the result tonowinPackages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohRelayRefreshSchedule.swift, Lines 34-46. This test checks onlydeadline <= refreshAfter. A deadline from a previous minute can satisfy the slot-window assertions while still being earlier thannow. Add lower-bound assertions.Suggested assertions
`#expect`(hostDeadline <= refreshAfter) `#expect`(clientDeadline <= refreshAfter) + `#expect`(hostDeadline >= now) + `#expect`(clientDeadline >= now)🤖 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/CmxIrohRelayCredentialCoordinatorTests.swift` around lines 41 - 42, Update CmxIrohRelayCredentialCoordinatorTests to assert both hostDeadline and clientDeadline are greater than or equal to the current-time lower bound (now), in addition to the existing refreshAfter upper-bound checks. Preserve the existing slot-window assertions and use the same now value captured for the schedule calculation.
🤖 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
`@Packages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohRelayCredentialCoordinatorTests.swift`:
- Around line 26-37: Update the refresh-cycle loop in
CmxIrohRelayCredentialCoordinatorTests to capture the first hostSecond and
clientSecond values, then assert on subsequent cycles that each schedule returns
the same second for the unchanged endpoint identity. Preserve the existing
deadline and window-membership checks.
- Around line 41-42: Update CmxIrohRelayCredentialCoordinatorTests to assert
both hostDeadline and clientDeadline are greater than or equal to the
current-time lower bound (now), in addition to the existing refreshAfter
upper-bound checks. Preserve the existing slot-window assertions and use the
same now value captured for the schedule calculation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 63cca8be-b936-422e-8b97-ff5cdc21e4bf
📒 Files selected for processing (1)
Packages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohRelayCredentialCoordinatorTests.swift
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. |
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
1 participant
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.
Summary
Verification
Principled fix: refresh ownership is encoded as a deterministic scheduling invariant instead of a timing retry.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Staggered Iroh relay credential refreshes by role and endpoint so host and client don’t rotate at the same time. Slots are stable per endpoint to reduce reconnect churn.
CmxIrohRelayRefreshSchedule) that assigns per-endpoint refresh slots using FNV hashing: host 0–14s, client 30–44s within each minute.jittercallback to compute deadlines clamped betweennowandrefreshAfter.Written for commit a30cdb1. Summary will update on new commits.
Summary by CodeRabbit
Improvements
Tests