Skip to content

fix(net): copies of one group identity agree beyond their bytes - #8201

Closed
kixelated wants to merge 5 commits into
mainfrom
quest/m1/copy-identity
Closed

kixelated wants to merge 5 commits into
mainfrom
quest/m1/copy-identity

Conversation

@kixelated

@kixelated kixelated commented Nov 7, 2026 •

Copy link
Copy Markdown
Collaborator

Closes the quest quest/m1/copy-identity.md.

Problem

Two copies of the same broadcast+track+group (another route, a FETCH answer, a re-offer, a mid-frame recovery) could disagree on what a reader sees while both stayed accepted. Shared content rows (rs/moq-net/src/model/group/content.rs) and the mid-frame recovery (frame.rs) compared only size, inflated, and overlapping bytes. Nothing compared a frame's timestamp, its IETF object properties, or where the group ends, so a copy with one more frame could raise largest after finish_largest declared the group settled. A resumed group read could also end cleanly on a replacement that finished before frames the earlier copy had already delivered.

Approach

Rust (moq-net):

  • A shared row now records the frame's timestamp and a keyed digest (per-process RandomState) of its object properties. A copy that disagrees on either gets ProtocolViolation and fails, as a byte conflict already did.
  • Content records the group's end: fin (set by a copy's own finish) and reached (one past the furthest frame position any copy opened). A frame at or past fin fails its copy. So does a finish short of reached or at a different fin. Positions are the frame's key (Object ID, which a frame without one shares with its ordinal). A copy whose IDs left its ordinals (sparse) declares no end, and neither does a copy a transport bound cut short (range_end). An empty copy that starts past frame 0 only bounds the end from above: it says nothing lies past its start.
  • activate_content carries the end of a copy that finished before any other copy shared its content.
  • Timestamps are skipped where they may be send times (track::Info::unchecked, lite-05/06), via a new GroupState::unchecked (struct size unchanged at 384 bytes).
  • The mid-frame recovery also compares the timestamp (unless either copy is unchecked) and the object properties (only when both copies carry wire Object IDs, since a lite copy without records drops them).
  • A resumed group read fails with ProtocolViolation when the replacement finishes before frames an earlier copy held. It counts only frames a copy actually committed, so a reader whose range starts past the group's end still ends cleanly.

JS (js/net): JS has no shared content rows; copies meet only at recovery. group.ts now refuses a replacement that finishes before frames an earlier copy held, matching the Rust recovery check.

The end is kept as one fin value on Content, which leaves room for the planned quest/m1/explicit-state/track-end.md outcome enum to replace it.

Impact

  • Public API: none.
  • Wire: none.
  • Behavior: a copy that disagrees with another copy on a timestamp, object properties, or the group's end now fails with ProtocolViolation. Before, it was accepted and readers of each copy saw different content under one identity.
  • Five existing model::track test fixtures wrote contradictory copies (for example, a whole-group fill finishing at 1 frame while the live copy held frame 3). They now write consistent groups. None of them asserted the contradiction.

Choices made without asking

  • Digest instead of storing the properties. Per the quest plan, a row holds a 64-bit keyed SipHash of the properties, not the properties. A peer cannot pick a collision without the per-process key. Keeping an Arc<Fields> in the row would compare exactly, but it pins one copy's metadata unbilled after that copy goes away.
  • Same-protocol scope. Rows are keyed Frame(i) or Object(id), so a lite copy without records and an IETF copy never share a row. That limit already existed and this PR does not change it.
  • The IETF end-of-group object is not compared. One copy may end with an End of Group object and another with a stream FIN; both are valid for the same content.
  • The recovery end check runs only on the read path, as in Rust (poll_resumed on Ok(None) and JS readFrame). A consumer's closed/done still reports the clean end until a read is attempted.

Validation

  • just ci on 909653b: pass.
  • cargo nextest run -p moq-net --all-features --no-fail-fast: 3700 passed.
  • New Rust tests: copies_disagreeing_on_a_timestamp_fail, copies_disagreeing_on_object_properties_fail, a_copy_past_the_declared_end_fails, a_copy_finishing_short_fails (group.rs); a_half_read_frame_refuses_a_replacement_with_another_timestamp, a_resumed_group_refuses_a_replacement_ending_before_what_was_read (resume.rs). With every new check disabled at once, all six fail.
  • New JS tests in group.test.ts: a short replacement is refused (fails when the check is removed), and a reader of a copy that held nothing takes the replacement's end (fails when the check counts the reader's start instead of held frames).
  • Benchmarks (host load 20-28 on 24 cores, run under bench/lock.sh and quest gate):
    • session_delivery_group_cost / session_delivery_datagram with SESSION_ALLOCS=1, pinned to one CPU, two base/head pairs: user instructions per viewer-group or datagram are at or below base in 38 of 40 cells (-0.05% to -1.3%). The other two are +0.12% and +0.98%, inside the base-to-base spread of those cells.
    • Criterion group_write_frames, group_roundtrip, group_relay_objects, canonical_storage, paired in both orders: wall-clock deltas swing ±10-20% with load and flip sign between orderings. The write path is unchanged when no content is shared, and the instruction counts above show no added work.

Caused by

  • moq-dev/moq#6922 - shared content rows compared only size and inflated
  • moq-dev/moq#4741 - mid-frame and group recovery compared only the frame size, and not the group's end

Follow-ups

  • content_for_copy activates content on only the first existing copy of a sequence, so a third copy that already held frames is never compared. Recommend planning a quest if a route can hold two pre-existing copies of one group; otherwise drop it.

Models

Size: M
Implementer: Claude Code, claude-opus-5-5, high effort

(Written by Claude Opus 5.5 with high)

Review

Rewritten by quest review after each round.

  • Reviewer: Codex, gpt-6.1-sol, xhigh effort
  • Session: 01a1b8e6-49a9-7af3-b440-aa8db245748d, round 1
  • Head: 909653b, named in full once a round reports no findings
  • Base: origin/main at 544f144
  • Score: 6/10, The head needs rework to preserve copy end bounds and propagate recovered-end violations to terminal observers.
  • Findings, open until the next round settles them:
    • Medium, rs/moq-net/src/model/group/content.rs:214: Retain the upper bound from an empty late-starting copy
    • Medium, js/net/src/group.ts:1608: Validate recovered ends before settling the closed handle
    • Medium, rs/moq-net/src/model/group.rs:3763: Apply end-consistency validation to finished()

kixelated and others added 2 commits November 7, 2026 14:36
Quest-Claim: e5f93fd162d41627
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated and others added 3 commits November 7, 2026 16:56
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…om above

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated kixelated changed the title Copies of one group identity must agree beyond their bytes fix(net): copies of one group identity agree beyond their bytes Nov 8, 2026
@kixelated

Copy link
Copy Markdown
Collaborator Author

Closing without merging. Maintainer decision, 2026-11-07 (/quest-spawn): copies of one identity disagree only when the publisher broke the identity rule, which hurts only its own broadcast, so moq-net spends no cycles detecting it. moq-dev/moq#8242 adds that rule to AGENTS.md and deletes quest/m1/copy-identity.md. Not chosen: narrowing this PR to refusing commits past a group's settled end, or keeping it as planned.

(Written by Claude Opus 5.5)

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