Harden route-content equivalence against reorders, missing baselines, and install races - #9402
Merged
Merged
Conversation
Pins six behaviors from the cubic review of #9342: reorder-only capability, relay fleet, and grant verification key revisions keep live sessions; a snapshot installed for a revision recorded without content fails closed; an older route revision install cannot roll back a newer one; a redundant-dial close raced by invalidation redials instead of returning the closed winner. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Canonicalizes route content so order carries no meaning where the admission policy reads sets: binding capabilities, the relay fleet, and grant verification keys (by kid) are sorted when the content is built, so reorder-only revision bumps keep live sessions. didInstallRouteRevision now drops installs older than the recorded revision, so an older completion of an overlapping reconciliation cannot roll back a newer installed revision. The same-revision branch compares the stored baseline and fails closed through the standard superseded-peer invalidation when the baseline is missing or differs, instead of silently adopting the content. The peer session no longer returns a stale winner capture after the redundant-dial close: settleRedundantDial re-reads the active slot and its liveness after the close suspension and redials when the winner was invalidated, replaced, or remotely closed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR canonicalizes route content, rejects stale revisions, avoids invalidation for equivalent content, and hardens redundant-dial settlement. New tests cover revision handling, route equivalence, invalidation races, and replacement dialing. ChangesConnectivity consistency and session safety
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Caller
participant CmxConnectivityPeerSession
participant RedundantConnection
participant InstalledConnection
participant Dialer
Caller->>CmxConnectivityPeerSession: connectedSession()
CmxConnectivityPeerSession->>RedundantConnection: close losing session
CmxConnectivityPeerSession->>InstalledConnection: verify same connection and open state
alt installed connection remains valid
CmxConnectivityPeerSession-->>Caller: return installed connection
else installed connection is invalid
CmxConnectivityPeerSession->>Dialer: retry dialing
Dialer-->>Caller: return replacement session
end
🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 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.
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/CmxConnectivityEngine.swift (1)
254-271: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftRe-validate the route-revision guard after peer invalidation.
didInstallRouteRevisionreadsrouteRevisiononly before asynchronous invalidation, but the same-path updates happen afterward. An older overlapping installation can resume after a newer one has installed and rewindrouteRevisionandrouteContent. Re-check the guard afterinvalidatePeersSuperseded(by:)in both branches that writerouteContent.🤖 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/CmxConnectivityEngine.swift` around lines 254 - 271, Update didInstallRouteRevision so each path that writes routeContent re-checks the current routeRevision after awaiting invalidatePeersSuperseded(by: content). If a newer revision has been installed during the await, return without overwriting routeRevision or routeContent; apply this re-validation in both the unchanged-revision baseline branch and the revision-update branch.
🤖 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/CmxConnectivityEngine.swift`:
- Around line 254-271: Update didInstallRouteRevision so each path that writes
routeContent re-checks the current routeRevision after awaiting
invalidatePeersSuperseded(by: content). If a newer revision has been installed
during the await, return without overwriting routeRevision or routeContent;
apply this re-validation in both the unchanged-revision baseline branch and the
revision-update branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7e800cc6-b7c8-4aa2-a820-40a03a374ba2
📒 Files selected for processing (5)
Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxConnectivityEngine.swiftPackages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxConnectivityPeerSession.swiftPackages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxConnectivityRouteContent.swiftPackages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxConnectivityEngineTests.swiftPackages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxConnectivityPeerSessionTests.swift
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.
Follow-ups to the six P2 findings from the cubic review of #9342. Commit 1 adds the failing tests, commit 2 the fixes, so CI proves each regression is pinned.
CmxConnectivityRouteContent.BindingMaterial). Capabilities are now sorted when the material is built, matching the admission policy's set semantics (the broker rejects duplicate capabilities at decode, so a sorted array equals set compare).AccountMaterialsorts the fleet when built; the discovery decoder already rejects duplicate relay URLs.AccountMaterialcanonicalizes the key set by (kid, alg, spki) so a reorder-only rotation snapshot compares equal.CmxConnectivityEngine.didInstallRouteRevision's same-revision branch silently adopted content even when the recorded revision had no stored baseline (a sync can store a revision from an unchanged response without a snapshot). It now compares the stored baseline and routes a missing or differing baseline throughinvalidatePeersSuperseded, which fails closed on a nil baseline.CmxIrohHostRuntime.reconcileConnectivityRevisionforwards fetched revisions without ordering). The engine now dropsdidInstallRouteRevisioncalls older than the recorded revision, so older completions cannot win regardless of the caller.CmxConnectivityPeerSession.connectedSessionreturned the previously capturedinstalled.sessionafter awaiting the redundant dial's close; an invalidation, remote close, or replacement during that suspension handed callers a closed session. Both redundant-dial paths now go throughsettleRedundantDial, which re-reads the active slot and its liveness after the close and retries the dial when the winner changed.Tests (all in
Packages/Shared/CmuxIrohTransport, suite green: 540 tests):reorderedCapabilitiesOnRevisionBumpKeepsTheLivePeerSessionreorderedRelayFleetOnRevisionBumpKeepsTheLivePeerSessionreorderedGrantVerificationKeysOnRevisionBumpKeepsTheLivePeerSessionsnapshotInstallForARevisionRecordedWithoutContentFailsClosedsameRevisionReinstallWithUnchangedContentKeepsTheLivePeerSessionolderRouteRevisionInstallCannotRollBackANewerInstallinvalidationDuringRedundantDialCloseTriggersAFreshDialinvalidationDuringPostProbeRedundantDialCloseTriggersAFreshDial🤖 Generated with Claude Code
Summary by cubic
Hardens route-content comparison and install ordering so reorder-only updates don’t drop sessions and overlapping reconciliations can’t roll back newer installs. Also fixes a redundant-dial race that could return a closed session, with tests added to pin behavior.
CmxConnectivityRouteContent: sort binding capabilities, relay fleet, and grant verification keys (by kid, alg, spki). Reorder-only revisions compare equal and keep sessions.CmxConnectivityEngine.didInstallRouteRevision: drop installs older than the recorded revision; for same revision, validate the stored baseline and invalidate on missing/different baseline; unchanged content keeps sessions.CmxConnectivityPeerSession: both paths usesettleRedundantDialto close the loser, re-check the installed winner and its liveness, and redial if it changed or closed, preventing stale closed sessions from being returned.Written for commit d011a69. Summary will update on new commits.
Summary by CodeRabbit