Stop redundant Iroh registration publication - #9350
Merged
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughThe runtimes now track registration publication state, use authoritative discovery for eligible refreshes, and register only when required. Lifecycle handling preserves or clears this state with bindings. Tests cover discovery, failures, direct-port changes, queued refreshes, and lifecycle races. ChangesRegistration refresh flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Runtime as CmxIroh runtime
participant Refresh as Refresh scheduler
participant Policy as resolvePolicy
participant Broker as Iroh broker
Runtime->>Refresh: Request refresh
Refresh->>Policy: Resolve policy with discovery requirement
Policy->>Broker: Discover authoritative policy
Broker-->>Policy: Discovery result
alt Publication state is current and binding matches
Policy-->>Refresh: Return policy without registration
else Publication is required or binding is missing
Policy->>Broker: Submit signed registration
Broker-->>Policy: Registration response
Policy-->>Refresh: Return policy and publication state
end
Refresh-->>Runtime: Complete refresh
Possibly related PRs
🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
azooz2003-bit
force-pushed
the
feat-iroh-registration-floor
branch
from
August 3, 2026 21:40
7a36665 to
adb62e2
Compare
azooz2003-bit
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohClientRuntime+Policy.swift (1)
98-119: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMove payload signing after the read-only eligibility check.
signer.prepare(payload:)runs unconditionally at line 102, before the code checksallowReadOnlyRegistrationRefreshandshouldUseReadOnlyRegistrationRefresh. When the read-only path applies,preparedis computed and then discarded. This signing work runs on every foreground activation and every network-change refresh, not only when a signed mutation is actually needed.Move the
CmxIrohRegistrationSignercreation andprepare(payload:)call after the read-only check, so signing only happens on the branch that uses it.♻️ Proposed reordering
- let signer = try CmxIrohRegistrationSigner( - identity: configuration.identity, - endpointID: expectedEndpointID.endpointID - ) - let prepared = try signer.prepare(payload: payload) - let refreshState = Self.registrationRefreshState( + let refreshState = Self.registrationRefreshState( payload: payload, now: now() ) if allowReadOnlyRegistrationRefresh, shouldUseReadOnlyRegistrationRefresh(refreshState, at: now()) { do { return try await readOnlyResolvedPolicy( expectation: expectation, offlineExpectation: offlineExpectation ) } catch CmxIrohClientRuntimeError.localBindingMissingFromDiscovery { // The server no longer has this binding. Fall through to a // signed mutation so the client self-heals instead of staying // read-only forever. } } + let signer = try CmxIrohRegistrationSigner( + identity: configuration.identity, + endpointID: expectedEndpointID.endpointID + ) + let prepared = try signer.prepare(payload: payload) let registration: CmxIrohRegistrationResponse?🤖 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/CmxIrohClientRuntime`+Policy.swift around lines 98 - 119, Move the CmxIrohRegistrationSigner creation and signer.prepare(payload:) call below the read-only eligibility block in the registration flow. Keep the readOnlyResolvedPolicy return and localBindingMissingFromDiscovery fallback unchanged, and ensure the signer and prepared values are created only on the subsequent signed-mutation path where they are used.
🤖 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/Sources/CmuxIrohTransport/CmxIrohClientRuntime`+Policy.swift:
- Around line 98-119: Move the CmxIrohRegistrationSigner creation and
signer.prepare(payload:) call below the read-only eligibility block in the
registration flow. Keep the readOnlyResolvedPolicy return and
localBindingMissingFromDiscovery fallback unchanged, and ensure the signer and
prepared values are created only on the subsequent signed-mutation path where
they are used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ac89a7c8-a56e-4db6-bfcf-dcbe20331818
📒 Files selected for processing (7)
Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohClientRuntime+Lifecycle.swiftPackages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohClientRuntime+Policy.swiftPackages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohClientRuntime+PolicyRefresh.swiftPackages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohClientRuntime.swiftPackages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohClientRuntimeLifecycleRaceTests.swiftPackages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohClientRuntimeTests.swiftPackages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/TestIrohClientBroker.swift
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
azooz2003-bit
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohHostRuntime+SignOut.swift (1)
38-48: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClear
lastRegistrationRefreshStatetogether withlocalBindingon successful sign-out.At line 39,
performSignOutclearslocalBinding = nilbut does not clearlastRegistrationRefreshState. The client's equivalent method,CmxIrohClientRuntime+Lifecycle.swiftperformSignOut, clears both fields together at the same point. LeavinglastRegistrationRefreshStateset after the binding it was derived from no longer exists violates the requirement to clear or preserve cached publication state consistently with the binding during sign-out.
resolveInitialPolicycurrently forces a full signed registration on everystart(), so this stale value is overwritten before it is read. Fix this now so a future change to the startup path (for example, extending the read-only optimization to initial resolution) does not silently reintroduce stale-state behavior.Based on learnings, applying `.github/review-bot-rules/reliability-single-source-of-truth.md`: "Keep cached publication state strictly derived from the authoritative registration/payload data; clear or preserve it consistently with the binding during sign-out, teardown, start, and foreground transitions."🔧 Proposed fix
localBinding = nil + lastRegistrationRefreshState = nil lifecyclePhase = .inactive🤖 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/CmxIrohHostRuntime`+SignOut.swift around lines 38 - 48, Update performSignOut to clear lastRegistrationRefreshState immediately alongside localBinding, matching the client runtime’s sign-out behavior. Preserve the existing lifecycle snapshot and operation-reset logic.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.
Inline comments:
In
`@Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohClientRuntime`+Policy.swift:
- Around line 78-102: Move the CmxIrohRegistrationSigner initialization and
signer.prepare(payload:) calls in the registration flow to after the
allowReadOnlyRegistrationRefresh/shouldUseReadOnlyRegistrationRefresh branch.
Keep payload and registrationRefreshState creation before the branch, and
preserve the existing read-only return and fallback behavior so signing occurs
only when the signed mutation path is needed.
In
`@Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohClientRuntime`+PolicyRefresh.swift:
- Around line 84-96: Ensure endpoint-instance replacement forces a signed
registration even when route keys and direct ports are unchanged. Update
CmxIrohRegistrationPublicationState and the non-discovery refresh flow around
registrationRefreshState, requiresPublication, and
shouldUseReadOnlyRegistrationRefresh to track runtimeGeneration (or equivalent
endpoint-instance generation) and prevent read-only refreshed policy until that
generation is published.
In
`@Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohHostRuntime`+PolicyRefresh.swift:
- Around line 386-409: Update refreshRegistration so non-forced refreshes do not
return solely because state.requiresPublication(after:now:) is false. Mirror the
client read-only shortcut by running authoritative discovery without requiring
publication, then force a signed registration refresh when the local host
binding is absent from the discovered result; preserve the existing
publication-required behavior for forced refreshes.
---
Outside diff comments:
In
`@Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohHostRuntime`+SignOut.swift:
- Around line 38-48: Update performSignOut to clear lastRegistrationRefreshState
immediately alongside localBinding, matching the client runtime’s sign-out
behavior. Preserve the existing lifecycle snapshot and operation-reset logic.
🪄 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: 98dc1f66-1f47-4cca-b140-c29a103dfb4a
📒 Files selected for processing (16)
Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohClientRuntime+Lifecycle.swiftPackages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohClientRuntime+Policy.swiftPackages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohClientRuntime+PolicyRefresh.swiftPackages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohClientRuntime.swiftPackages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohHostRuntime+PolicyRefresh.swiftPackages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohHostRuntime+PublicAPI.swiftPackages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohHostRuntime+SignOut.swiftPackages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohHostRuntime.swiftPackages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohRegistrationPublicationState.swiftPackages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohClientRuntimeLifecycleRaceTests.swiftPackages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohClientRuntimeTests.swiftPackages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohHostRuntimeLifecycleRaceTests.swiftPackages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohHostRuntimeLifecycleTests.swiftPackages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohHostRuntimePolicyTests.swiftPackages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/TestIrohClientBroker.swiftPackages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/TestIrohEndpoint.swift
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)
Packages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohClientRuntimeLifecycleRaceTests.swift (1)
264-267: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winVerify the changed direct port in both publication payloads.
Both tests prove only that a second broker registration occurred. They do not prove that the changed direct port was serialized, so stale-port publication can pass.
Packages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohClientRuntimeLifecycleRaceTests.swift#L264-L267: inspect the second prepared registration and assert direct port50909.Packages/Shared/CmuxIrohTransport/Tests/CmuxIrohHostRuntimeLifecycleTests.swift#L603-L608: assertregistrationDirectPortsequals IPv4 port50909with no IPv6 port.Based on the existing prepared-registration assertion pattern in
Packages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohHostRuntimeLifecycleTests.swiftLines 480-486.🤖 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/CmxIrohClientRuntimeLifecycleRaceTests.swift` around lines 264 - 267, Verify the changed direct port in both registration payloads: in Packages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohClientRuntimeLifecycleRaceTests.swift:264-267, inspect the second prepared registration and assert direct port 50909; in Packages/Shared/CmuxIrohTransport/Tests/CmuxIrohHostRuntimeLifecycleTests.swift:603-608, assert registrationDirectPorts contains IPv4 port 50909 and no IPv6 port, following the existing prepared-registration assertion pattern.
🤖 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/CmxIrohClientRuntimeLifecycleRaceTests.swift`:
- Around line 264-267: Verify the changed direct port in both registration
payloads: in
Packages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohClientRuntimeLifecycleRaceTests.swift:264-267,
inspect the second prepared registration and assert direct port 50909; in
Packages/Shared/CmuxIrohTransport/Tests/CmuxIrohHostRuntimeLifecycleTests.swift:603-608,
assert registrationDirectPorts contains IPv4 port 50909 and no IPv6 port,
following the existing prepared-registration assertion pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5ae3a905-015b-4f42-847a-14b6eea01870
📒 Files selected for processing (2)
Packages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohClientRuntimeLifecycleRaceTests.swiftPackages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohHostRuntimeLifecycleTests.swift
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/CmxIrohRegistrationPublicationStateTests.swift`:
- Around line 90-116: Add an IPv6-only direct-port publication test alongside
changedDirectPortsRequirePublication, using state to create two registrations
with the same hints and IPv4 value but different ipv6 values, then assert the
changed state requires publication after the original.
🪄 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: ece39c90-6b30-47ae-9bbc-cae9720a5c26
📒 Files selected for processing (1)
Packages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohRegistrationPublicationStateTests.swift
A live discovery that observes an in-flight unchanged-fingerprint refresh and its coalesced successors can return .refreshed without any authoritative broker read. TestIrohEndpoint gains an armable address() gate so each raced refresh is deterministically held in flight. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Observing a coalesced successor no longer forfeits the live discovery request's right to schedule one discovery-forced refresh, and a no-op .refreshed outcome without a generation advance no longer satisfies the request or masks an earlier real failure. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… IPv6 ports Payload signing now happens only after the read-only eligibility gate declines, so the read-only fast path no longer performs a discarded signature. The publication-state test also pins IPv6-only direct-port changes as requiring publication. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 03bae91. Configure here.
Collaborator
Author
|
Review round addressed on top of a fresh
CmuxIrohTransport: 590/590 tests pass locally. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Mirrors the client sign-out path so a stale fingerprint cannot suppress the next session's non-forced publications. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Sep 2, 2026
Merged
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.
Root cause: Mac and iOS treated every endpoint event as an instruction to republish registration. Sequential events with unchanged reachability repeatedly called challenge, register, discovery, and endpoint attestation.
This change gives both runtimes an actor-owned fingerprint of the last successfully published reachability state. Raw endpoint events publish only when path hints or direct ports changed, or renewal is due. Explicit live discovery and scheduled renewal remain authoritative broker operations. LAN refresh remains immediate.
Regression history:
Validation:
This is a principled idempotency boundary, with no request budget, feature gate, or connectivity delay.
Note
Medium Risk
Changes when signed registration and discovery run on the connectivity/broker path; incorrect fingerprint or coalescing logic could delay reachability updates or leave stale broker state, though missing-binding fallback and forced refresh paths mitigate that.
Overview
Mac and iOS Iroh runtimes no longer treat every endpoint network event as a full broker republish. They track
lastRegistrationRefreshStatevia newCmxIrohRegistrationPublicationState, which fingerprints path hints and direct ports and only requires publication when reachability changes, hints are near expiry, or the ~50‑minute renewal window elapses.Event-driven refreshes compare the live payload to that fingerprint and skip signed register when nothing material changed; LAN refresh stays immediate. Explicit live discovery and scheduled renewal still force authoritative broker work (
requiresDiscovery/forcePublication). On iOS, policy resolution can take a read-only discovery path when the fingerprint matches, with fallback to signed registration if the binding is missing from discovery.Lifecycle cleanup clears publication state on sign-out and teardown (including when preserving binding during sign-out network deactivation) so a stale fingerprint cannot suppress the next session’s publications. Live discovery on the client no longer treats coalesced unchanged-fingerprint no-ops as success without a broker read.
Tests and test brokers were extended for unchanged network storms, port changes, discovery hooks, and sign-out state clearing.
Reviewed by Cursor Bugbot for commit 3cf51a1. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit