Skip to content

feat(moq-net): a withdrawn claim ends across mixed lite-06 and lite-07 relays - #8358

Queued
kixelated wants to merge 7 commits into
mainfrom
quest/m1/mixed-version-ring
Queued

kixelated wants to merge 7 commits into
mainfrom
quest/m1/mixed-version-ring

Conversation

@kixelated

@kixelated kixelated commented Nov 9, 2026 •

Copy link
Copy Markdown
Collaborator

Completes quest/m1/mixed-version-ring.md and deletes it.

Problem

After its publisher leaves, a broadcast never ends in a relay mesh that mixes moq-lite-06 and moq-lite-07 links. This includes a rolling upgrade, where one lite-06 relay links to three or more lite-07 relays. Subscribers see a Restart on every lap.

A route learned over lite-06 becomes the translating relay's own lite-07 claim, and its lite-06 hop chain was dropped there. When a lite-07 route was translated back out to lite-06, it carried only its origin node's hop. Once a claim crossed two lite-06/lite-07 boundaries, no relay ever saw its own hop again. In A-07-B-06-C-07-D-06-A, B and D kept re-originating each other's copy.

Approach

As the quest settled: a crate-private chain, carried through lite-07 by a temporary compatibility extension.

  • route::node::Claim gains chain: Hops. A claim the origin learned over an older wire keeps the hop chain it arrived with (OwnClaims::apply). A native claim's chain is empty. The learned flag stays separate, because moq-transport still needs it after lite-08 deletes the chain.
  • Wire, lite-07-wip only: there is a new empty SETUP parameter, Lite-06 Chain (0x9), and every lite-07 session sends it. ANNOUNCE_START, ANNOUNCE_UPDATE and ANNOUNCE_RESTART end with a trailing Lite-06 Chain. It is sent only to a peer that sent the parameter, and only when the chain is non-empty. A received empty chain is a PROTOCOL_VIOLATION. The lite-07 publisher (routed::Answer) waits for the peer's SETUP before its first claim. It sends an ANNOUNCE_UPDATE when only the chain changes.
  • Translating back to lite-06: a served lite-07 claim's route carries its chain followed by its origin node's hop, where before it carried that hop alone.
  • Both receive sides drop a chain that names them. This is one Subscriber::names_self check, against either this node's lite-06 hop or its route identity's hop. Lite-06 applies it in chain(). Lite-07 applies it when it serves a claim, and also drops a claim whose translated chain would exceed max_hops. That makes max_hops bound the whole path across both versions, and the old route-hop check becomes one case of names_self.
  • The draft defines the parameter, the field on all three messages, and the receive rule, with a changelog line. All of these are marked as temporary and removed in the next version.
  • The relay cluster doc describes the new translation.

Tests

New in rs/moq-net/tests/integration/mesh_withdraw.rs, next to mixed_version_ring_withdraw_retracts. Each test announces at one relay, checks that every relay sees it start, withdraws it, and checks that every relay sees it end:

  • alternating_version_ring_withdraw_retracts: A-07-B-06-C-07-D-06-A.
  • rolling_upgrade_withdraw_retracts: the publisher is on the lite-06 relay, which links to each relay of a three-relay lite-07 full mesh.
  • rolling_upgrade_from_lite07_withdraw_retracts: the same topology with the publisher on a lite-07 relay.

Route identities are random, and selection hashes them with the path. So the helper publishes eight paths, each with its own tie-breaks, which makes the lite-07-publisher case a reliable gate (see Checks).

Impact

  • Public API: none. The chain, the SETUP field and the codec field are all crate-private.
  • Wire (moq-lite-07-wip, unpublished): adds the SETUP Lite-06 Chain parameter (0x9) and a trailing optional Lite-06 Chain on ANNOUNCE_START, ANNOUNCE_UPDATE and ANNOUNCE_RESTART. The field goes only to a peer that offers the parameter, so a lite-07-wip peer from the previous release never receives it and still decodes everything sent to it. Between such a peer and this one, the chain is lost, which is the current behavior.
  • Wire (moq-lite-06 and earlier, moq-transport): unchanged. A lite-06 peer now receives longer hop lists for claims that crossed lite-06 earlier. This is the same encoding, still bounded by max_hops.
  • Behavior: a claim learned over moq-transport also keeps its hop chain through lite-07. The chain is generic, and relay links over moq-transport stay out of scope.

Alternatives

The quest's decision rejected a cost or hop ceiling with no wire change, never forwarding a lite-06-learned claim to another lite-07 relay, and documenting mixed rings as unsupported. The choices below were made here:

  • An always-present field (count 0 when empty) instead of a trailing one omitted when empty. A trailing field lets the decoder parse without knowing the negotiation, and saves a byte on every native claim.
  • Folding learned into !chain.is_empty(): rejected, because learned must outlive the chain for moq-transport routes.

Choices made without asking

  • Negotiated by a SETUP parameter rather than changed unconditionally. Release already ships lite-07-wip (opt-in), and the field would break that release's decoder, which rejects trailing bytes.
  • The lite-07-publisher variant is kept, and made deterministic by publishing eight paths, rather than dropped.
  • The quest's JS note is rewritten. routes/js.md said the JS port decodes the chain. JS never offers the parameter, so it is never sent one, and its bullet now says to leave the parameter unsent.

Follow-ups

None. Merging main brought in moq-dev/moq#8293. Its route-hop reflection check in chain() is now one case of names_self. Its with_draining replaces the drain cost in subscriber/routed.rs.

Ranking

A lite-07 claim's route now carries its lite-06 chain in its hops. So route_order ranks an entry with a node as one identified hop, which is how every lite-07 claim ranked before. Without that, an unknown hop in the chain would lose to a costlier native claim at any cost, and a longer chain would lose a cost tie before the rendezvous hash. The test is a_lite07_claims_carried_chain_does_not_rank_it.

Caused by

  • moq-dev/moq#8023 - added the lite-06 translation with a check that only catches a reflection across one lite-06 segment, dropping the chain at the boundary.

Checks

  • quest gate just check and quest gate just test interop --all pass on 5ee5a84 after merging main. quest gate just check passes again on 966a128, which merges main at 208c23d and gives the fuzz Claim that main added an empty chain, which CI's moq-net/fuzz build needed. In one earlier run, the 9 finish cpp -> * cases all failed together at a load average above 30. They passed when rerun on their own, and again in the full run. Its skips are the existing JS lite-07 quarantine that routes/js.md tracks.
  • Reproduced first. With the chain switched off in routed::Answer, alternating_version_ring_withdraw_retracts and rolling_upgrade_withdraw_retracts hang until timeout, and rolling_upgrade_from_lite07_withdraw_retracts hangs under all 12 of MOQ_SIM_SEED 1 to 12. With the fix, all three pass under all 12 seeds.
  • The lite-07-side drop is exercised. Without it, the ring and the lite-07-publisher variant trip origin.rs's "announce called with a looping hop chain" debug assertion.

Models

Size: L
Implementer: Claude Code, claude-opus-5-5, high effort
Implementer: Claude Code, claude-opus-5-5, low effort (iterate, S)

(Written by Claude Opus 5.5 with high)

Review

Rewritten by quest review after each round.

  • Reviewer: Codex, gpt-6.1-sol, high effort
  • Session: 01a1c1c5-e158-7ae2-9e63-4beaa13e43a8, round 3
  • Head: 966a128
  • Base: origin/main at d9800c1
  • Score: 10/10, The only new branch change initializes native fuzz claims with an empty compatibility chain as intended; no new findings arose from code review, and builds and tests were not run.
  • Findings: none

kixelated and others added 2 commits November 9, 2026 01:38
… claim ends in a mixed ring

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated kixelated changed the title A withdrawn claim ends across mixed lite-06 and lite-07 relays feat(moq-net): a withdrawn claim ends across mixed lite-06 and lite-07 relays Nov 9, 2026
@kixelated
kixelated marked this pull request as ready for review November 9, 2026 10:33

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed head: 3cbf67c

No actionable findings. Preserving the learned chain across lite-07, negotiating its trailing wire fields, and rejecting reflected or over-limit translated claims addresses the lost-path-information cause of mixed-version withdrawal loops. I traced initial announcements, chain-only updates, restarts, and withdrawal handling through the surrounding routing code and reviewed the new mesh regressions. No code changes suggested from this review.

Verification limits: GitHub-only static review; I did not run builds, tests, or quest tooling, or independently reproduce the reported seed runs. The current head's Check run is still in progress. PR state, full head SHA, and existing reviews were rechecked immediately before submission; no earlier GitHub reviews were present.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Chain on the claim — route::node::Claim gains chain: Hops; OwnClaims::apply seeds it from a route learned over an older wire, and routed::serve extends it with the origin node's hop when translating for lite-06.
  • Negotiated wire extension — SETUP parameter 0x9 is offered by every lite-07 session; ANNOUNCE_START/UPDATE/RESTART carry a trailing Lite-06 Chain only to a peer that offered it, and only when non-empty, with an explicitly empty chain rejected.
  • Publisher side — routed::Answer waits for the peer's SETUP before its first claim, gates the chain on the offer, and emits an ANNOUNCE_UPDATE when only the chain changes.
  • Reflection and bounds — Subscriber::names_self unifies the lite-06 hop and route-identity checks; lite-07 now drops a served claim whose translated chain names it or exceeds max_hops.
  • Docs, draft, quests — the draft defines the parameter, the three trailing fields, the receive rule, and a changelog line; the relay cluster doc describes the translation; quest/m1/mixed-version-ring.md is deleted with its references removed or updated.
  • Tests — three mixed-version withdrawal tests cover the alternating ring, a rolling upgrade publishing at the lite-06 relay, and one publishing at a lite-07 relay.

I traced the chain end to end: a lite-06-learned route keeps its hop list through the route node, across lite-07 announce streams, and back out to lite-06 with the origin hop appended, so the originating relay now sees its route-identity hop and drops the claim. Chain changes propagate because Served equality includes the chain, and the trailing field never reaches a peer that did not offer 0x9, keeping previously released lite-07-wip peers and the JS port (which leaves the parameter unsent) decodable.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏

@coderabbitai

coderabbitai Bot commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 16 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 53f8ea50-3345-492e-9332-2e6f88790564
Commits

Reviewing files that changed from the base of the PR and between 5ee5a84 and 966a128.

Files selected for processing: 19

  • doc/bin/relay/cluster.md
  • drafts/draft-lcurley-moq-lite.md
  • quest/m1/README.md
  • quest/m1/mixed-version-ring.md
  • quest/m2/cluster-routing/routes/js.md
  • quest/m2/lite07-finalize.md
  • rs/moq-net/src/client.rs
  • rs/moq-net/src/fuzz.rs
  • rs/moq-net/src/lite/announce.rs
  • rs/moq-net/src/lite/publisher.rs
  • rs/moq-net/src/lite/publisher/routed.rs
  • rs/moq-net/src/lite/session.rs
  • rs/moq-net/src/lite/setup.rs
  • rs/moq-net/src/lite/subscriber.rs
  • rs/moq-net/src/lite/subscriber/routed.rs
  • rs/moq-net/src/model/origin.rs
  • rs/moq-net/src/route/node.rs
  • rs/moq-net/src/server.rs
  • rs/moq-net/tests/integration/mesh_withdraw.rs

No actionable comments were generated in the recent review.

Walkthrough

The change defines the Lite-06 Chain SETUP parameter and optional chain fields on Lite-07 announcements. Rust negotiates chain support, sends chains to peers that offer it, and retains received chains in routed claims. Relay code checks chains when serving claims. Integration tests cover withdrawals in mixed-version rings and rolling-upgrade topologies. Protocol and planning documents are also updated.

Severity of issue fixed: Medium

Merge Risk: Minimal · previous review 5ee5a

No actionable merge-blocking risk is established; merge after normal checks.

Pre-merge checks: Passed 4 · Inconclusive 1

Failed Failed checks: 1 inconclusive

Check name Status Explanation Resolution
Docstring Coverage Inconclusive Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed

Check name Status Explanation
Title check Passed The title clearly describes the main change: withdrawn claims now end across mixed lite-06 and lite-07 relays.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description explains the mixed-version relay withdrawal problem and the changes that address it, including chain propagation, loop detection, and integration tests.
Full details: Docstring Coverage

Explanation

Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 24 functions across 12 files. (2 skipped: 1 unsupported, 1 too large.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches

Simplify code

  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

Recent review info

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 4ae9274c-5601-46de-9e59-4ab013b6c838
Commits

Reviewing files that changed from the base of the PR and between 3cbf67c and 5ee5a84.

Files selected for processing: 9

  • drafts/draft-lcurley-moq-lite.md
  • quest/m1/mixed-version-ring.md
  • rs/moq-net/src/fuzz.rs
  • rs/moq-net/src/lite/publisher/routed.rs
  • rs/moq-net/src/lite/subscriber.rs
  • rs/moq-net/src/lite/subscriber/routed.rs
  • rs/moq-net/src/model/origin.rs
  • rs/moq-net/src/route/node.rs
  • rs/moq-net/tests/integration/mesh_withdraw.rs
Files with no reviewable changes: 1

  • quest/m1/mixed-version-ring.md
Files skipped from review as they are similar to previous changes: 1

  • drafts/draft-lcurley-moq-lite.md

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

…n-ring

# Conflicts:
#	quest/m1/mixed-version-ring.md
#	rs/moq-net/src/lite/publisher/routed.rs
#	rs/moq-net/src/lite/subscriber.rs
#	rs/moq-net/src/lite/subscriber/routed.rs

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed head: 5ee5a84

No actionable findings in the changes since my review of 3cbf67c, comparing each revision against its merge base to exclude imported main changes. The ranking correction in rs/moq-net/src/model/origin.rs:797–818 restores each lite-07 claim's one-identified-hop ordering without discarding the compatibility chain needed for reflection checks and translation. I checked the selection callers, regression test, and integration with the merged identity/draining changes. The direction is sound; no further code changes suggested.

Verification limits: GitHub-only static review; no builds, tests, interop runs, or quest tooling executed. Check is still in progress for this head. Open/non-draft state, full head SHA, and existing reviews were rechecked immediately before submission; no review of this head was already published.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Merge integration — merged main and refit the chain work onto #8293's draining and route-ranking refactor; routed::serve now uses with_draining instead of a local Cost::DRAIN, and the route-hop reflection check is folded into names_self.
  • Fuzz constructor — fuzz::RouteNode::claim now builds its claim with an empty chain, keeping the fuzz/bench path compiling against the new Claim field.
  • Route ranking — route_order treats any entry with node: Some(_) as one identified hop, so the lite-06 chain a lite-07 claim carries no longer makes it anonymous or hop-length-ranked; a regression test covers the carried-chain case.

with_node is only set by the lite-07 receive path, which produced a single identified hop before this PR, so the ranking change restores the pre-chain behavior for every other route. The test gates the bug: without the change the carried claim ranks anonymous and loses before cost.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed head: 966a128

No actionable findings. Comparing with the previously reviewed 5ee5a84 and accounting for the main merge isolates the added empty chain in rs/moq-net/src/fuzz.rs:641. RouteRelay::claim creates native lite-07 claims for the benchmark fixture, so an empty compatibility chain is correct, matching the existing RouteNode fixture. The integration change is sound; no further code changes suggested.

Verification limits: GitHub-only static review; no builds, tests, interop runs, or quest tooling executed. Check remains in progress for this head. Open/non-draft state, full head SHA, and reviews were rechecked immediately before submission; no review of this head was already published.

@kixelated
kixelated added this pull request to the merge queue Nov 10, 2026
Any commits made after this event will not be merged.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant