Skip to content

feat(net)!: an errored track end keeps its history - #7349

Merged
moq-bot[bot] merged 10 commits into
mainfrom
quest/m2/errored-end
Oct 31, 2026
Merged

moq-bot[bot] merged 10 commits into
mainfrom
quest/m2/errored-end

Conversation

@kixelated

@kixelated kixelated commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

A track can now end with an error and still serve the history it holds. Readers get every group below the end, then the error, instead of either the error alone (an abort) or a clean finish. archive::Consumer uses this to replay a stored {"error":code} end the way its source ended. This finishes quest/m2/errored-end.md.

Changes

  • moq-net model: track::Producer::finish_at(end, error: Option<Error>). Setting error stores it beside the end position. Unlike abort, the cache, fills, and listing behave exactly as after a clean finish. A reader that reaches the end gets the error instead of Ok(None). That covers recv_group, Ordered::next_group, recv_datagram, routed (resume) readers, and Producer::closed. A reader whose range stops short of the end still ends cleanly. A repeated or refined end must carry the same error code.
  • moq-lite-07 wire: SUBSCRIBE_END gains an Error Code field: 0 for a clean end, otherwise the code + 1. On lite-07 the publisher sends the error with the end and then FINs the subscription. On older versions the publisher resets with the error once the end is reached, which is the only way to carry it there.
  • @moq/net (JS): mirrors all of the above: finishAt(end, error?), the lite-07 SubscribeEnd codec, the publisher and subscriber, resume, and the blocking and nonblocking reads.
  • moq-archive: End::Error(code) becomes finish_at(after the newest stored sequence, Some(error)), which replaces the clean finish and its warn log.
  • Merge-time fixes (from review feedback):
    • Rust resume keeps the first error a copy's end carried beside the declared end, so a route switch to a copy that declares the same end cleanly keeps it. A datagram-only routed reader now ends with the error too.
    • A reader ends with the error only when its resolved ranges reach the last group, so a relative or time end that stops short ends cleanly.
    • Before lite-07, the publisher (Rust and JS) withholds the early SUBSCRIBE_END of an errored end unless the subscription stops short of it. A clean boundary sent early could let the subscriber settle before the reset that carries the error arrives.
    • moq-archive's producer records the error a read ended with, so recording an errored end stores End::Error, not a clean finish.
  • Draft and docs: drafts/draft-lcurley-moq-lite.md (SUBSCRIBE_END field and lite-07 changelog) and doc/concept/moq-lite.md (one sentence).
  • The quest file is deleted, along with its link in quest/m2/README.md.

Impact

  • Breaking (Rust): track::Producer::finish_at takes a second argument. To migrate, pass None to keep today's clean end. Every in-repo call site is updated, including moq-ffi, moq-c, moq-json, and the interop client. The FFI and C APIs are unchanged.
  • JS: Track.Producer.finishAt(end, error?) is additive.
  • Behavior: a reader at an errored end gets Err(error) / a rejection. Producer::closed() reports that error.
  • Wire: lite-07 (moq-lite-07-wip, unpublished) SUBSCRIBE_END adds Error Code (i). Published versions are unchanged.

Choices made without asking

  • API shape: finish_at takes the error as a parameter, as the quest Plan suggested. Alternatives I considered:

    • (a) finish_at(end: impl Into<track::End>) with a track::End { position, error } type. No call-site churn, but it adds a new exported type.
    • (b) A separate abort_at(end, err). Additive, but "abort" means dropping history everywhere else in this crate.

    I recommend the parameter: it is explicit, and the churn is mechanical.

  • Wire encoding: the error code is encoded as code + 1 (varint_opt), matching how lite-07 already encodes optional fields (TRACK_INFO Publisher Max Age, FETCH Subscriber Max Age), rather than as a trailing optional field.

  • Pre-07 fallback: the publisher resets the subscription with the error at the end, so the subscriber sees the error but not a retained history. Lite-06 has nowhere to put the error.

  • IETF: no wire change. A moq-transport publisher reaching an errored end fails the subscription as it does on an abort. Per AGENTS.md, IETF maps onto the model and gets the least code.

  • Archive outcome: the producer stores the first error a read ended with. This also stores an abort that won over an unsettled declared end as End::Error, not End::Finish, because that is what its reader saw.

  • Mixed-version copies: a routed reader keeps the first error any copy's end carried and never refuses over a mismatch, as JS already did. A lite-06 copy declares the same end without an error.

Overlap

  • moq-dev/moq#7081 (merged; a local close aborts like a remote one): this builds on it. An errored end is a declared end, so a session close after it has settled keeps the end and its error, through abort's existing downgrade.
  • moq-dev/moq#7295 (open; a lite track end aborts at session death): it touches the same SUBSCRIBE_END handling in rs/moq-net/src/lite/subscriber.rs. Whichever lands second will need a small textual merge where finish_at is called with the received end. Its semantics compose: an unsettled errored end aborts at session death, and a settled one keeps its error.

Tests

  • model::track::test::an_errored_end_keeps_its_history: arrival, ordered, and late readers; a capped range ends cleanly; a fetch still works; the repeat rule.

  • integration::errored_end::an_errored_end_keeps_its_history_across_a_hop: a first and a later subscriber over lite-06 and lite-07. On lite-07 the subscription is not reset; on lite-06 it is.

  • Wire round-trips in Rust and JS. The JS publisher puts the code on SUBSCRIBE_END.

  • JS: the model test including tryRecvGroup/tryNextGroup, and a resume test where the error outlives the copy that declared it.

  • moq-archive: the consumer replays every stored group, then the error, and a fetch still reads them.

  • Local: just check passes, and just test interop --all passes.

  • Merge-time: model::resume::test::an_errored_end_survives_a_route_switch, model::resume::test::a_routed_datagram_reader_ends_with_the_end_error, a relative-end case in an_errored_end_keeps_its_history, lite::publisher::test::recv_next_withholds_an_errored_boundary_without_the_end_error, the JS lite-06 tests "an errored end is not declared early" and "a subscription capped short of an errored end gets its boundary", and moq-archive's an_errored_end_records_its_error.

Review

Reviewer: Codex gpt-6.1-sol, medium effort, via quest review. Reviewed head 62110e4120d8a596cac1edb77fbd82fe3700c57d. Landing head 4df76c157e3fbc048c3eb0b0aa722fcf438ec55b: it only adds a rustfmt pass on two new tests and restores main's #[cfg(any(feature = "async", test))] on Subscription::end, which ended no longer reads. Review skipped as trivial.

Earlier session, rounds 1 to 2 (head 85368e3e): two JS findings fixed, then no findings.

Online review on 85368e3e (Codex connector and an automated review): two Rust resume findings, both fixed in e040177e:

  • A datagram-only routed reader lost the end error. It now checks the serving copy's datagram channel.
  • A route switch lost the error with the logical end. The reader now keeps it beside declared.

Merge-time session:

  • Round 1 at e040177e, three findings, all fixed in 017c8136:
    • Before lite-07, an early clean SUBSCRIBE_END could let the subscriber settle before the reset. The publisher now withholds it.
    • A relative or time range end that stops short got the error. It now checks the resolved ranges.
    • Re-recording an errored replay stored a clean finish. The archive now keeps the read error.
  • Round 2 at 017c8136, one finding, fixed in 62110e41: a lite-06 subscription capped short of an errored end never got its boundary. It is now declared to a range that stops short.
  • Round 3 at 62110e41: no findings.
  • 4df76c15: rustfmt and the restored cfg gate, skipped as trivial. just check passes on it.

Suggested follow-ups

  • Carry an errored end over moq-transport (for example the PUBLISH_DONE status), if a consumer needs one.
  • In JS, a reader capped below groups the cache still holds never ends after a clean close: it waits for the cap to rise. This is pre-existing and was seen while writing the test.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

kixelated and others added 5 commits October 30, 2026 04:29
Quest-Claim: 0b3e6cb0abb27a44
`track::Producer::finish_at` (JS `finishAt`) takes an optional error: the track ends there
with that error, yet keeps its cache, fills, and listing as after a clean finish. Readers get
every group below the end, then the error instead of the end of stream. Lite-07's
SUBSCRIBE_END carries the error code so a relay forwards it; older versions reset at the end.
`archive::Consumer` replays a stored `{"error":code}` end this way instead of a clean finish.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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 An errored track end keeps its history feat(net)!: an errored track end keeps its history Oct 30, 2026
@kixelated
kixelated marked this pull request as ready for review October 30, 2026 14:02
@coderabbitai

coderabbitai Bot commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

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 45 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: a1a6b55a-ff77-4618-8ace-799a4bdb0ba5
Commits

Reviewing files that changed from the base of the PR and between 9387174 and 4df76c1.

Files selected for processing: 43
  • doc/concept/moq-lite.md
  • drafts/draft-lcurley-moq-lite.md
  • js/net/api/index.api.md
  • js/net/src/internal.ts
  • js/net/src/lite/publisher.test.ts
  • js/net/src/lite/publisher.ts
  • js/net/src/lite/subscribe.test.ts
  • js/net/src/lite/subscribe.ts
  • js/net/src/lite/subscriber.ts
  • js/net/src/lite/version.ts
  • js/net/src/resume.test.ts
  • js/net/src/resume.ts
  • js/net/src/track.test.ts
  • js/net/src/track.ts
  • quest/m2/README.md
  • quest/m2/errored-end.md
  • rs/moq-archive/src/consumer/mod.rs
  • rs/moq-archive/src/consumer/tests.rs
  • rs/moq-archive/src/consumer/track.rs
  • rs/moq-archive/src/producer/ordered.rs
  • rs/moq-archive/src/producer/tests.rs
  • rs/moq-c/src/publish.rs
  • rs/moq-ffi/src/producer.rs
  • rs/moq-json/src/snapshot/mod.rs
  • rs/moq-net/benches/tail.rs
  • rs/moq-net/src/coding/mod.rs
  • rs/moq-net/src/ietf/publisher.rs
  • rs/moq-net/src/ietf/subscriber.rs
  • rs/moq-net/src/lite/publisher.rs
  • rs/moq-net/src/lite/subscribe.rs
  • rs/moq-net/src/lite/subscriber.rs
  • rs/moq-net/src/lite/version.rs
  • rs/moq-net/src/model/broadcast.rs
  • rs/moq-net/src/model/resume.rs
  • rs/moq-net/src/model/subscription.rs
  • rs/moq-net/src/model/track.rs
  • rs/moq-net/src/model/track/fill.rs
  • rs/moq-net/tests/integration/errored_end.rs
  • rs/moq-net/tests/integration/finite_relay.rs
  • rs/moq-net/tests/integration/main.rs
  • rs/moq-net/tests/integration/settled_end_survives_abort.rs
  • rs/moq-net/tests/integration/track_tail.rs
  • test/interop/clients/rust/tail.rs
  • 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

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-30T14:04:52.340918Z 85368e3 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 85368e3e20

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread rs/moq-net/src/model/resume.rs Resolved

@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 commit: 85368e3.

Two P2 issues remain in Rust resume handling:

  • Preserve the error with the logical end across route switches — rs/moq-net/src/model/resume.rs:803–804 reads only the newest copy's error, while learn_end (1127–1139) retains the earlier copy's boundary. If copy A declares an end with App(7), then B becomes serving before declaring its end, poll_finished still returns A's boundary but declared_error returns None. A lite-07 relay can consequently forward that errored logical end as clean; B subsequently finishing cleanly also loses A's terminal error. Persist and validate the error alongside Reader::declared, as the JS implementation now does, and add an A→B route-switch regression.
  • Datagram-only routed readers lose the terminal error — independently confirmed the existing finding at resume.rs:790–798: the helper requires the group channel's done, whereas datagram reads populate datagrams_done. A datagram-only reader gets clean EOF instead of the declared error. Determine completion for the channel that drained and cover it in a routed datagram test.

Direction: retaining history separately from abort, with a version-gated end error, is appropriate. The JS nonblocking-read and retained-error fixes are present; the Rust logical-end state needs the same consistency.

Verification limits: GitHub-only source/test inspection with an independent review. No Rust/JS tests, interop runs or builds executed; reported passes were not independently verified.

kixelated and others added 4 commits October 30, 2026 22:51
…agram readers

The resume reader now keeps the first error a copy's end carried beside the
declared end, so a route switch to a copy that declares the same end cleanly
keeps it. A reader that ran out of datagrams checks the serving copy's
datagram channel, not its group channel, before ending with that error.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ends faithful

- A reader ends with an errored end's error only when its resolved ranges reach
  the last group, so a relative or time end that stops short ends cleanly.
- Before lite-07, a publisher withholds the early SUBSCRIBE_END of an errored
  end: a clean boundary could let the subscriber settle before the reset that
  carries the error arrives.
- moq-archive records the error a read ended with, so recording an errored end
  stores End::Error rather than a clean finish.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A subscription capped short of an errored end is owed no error, so its
SUBSCRIBE_END still goes out before lite-07 rather than waiting forever on
groups parked above the cap.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`ended` no longer reads it, so restore main's cfg gate.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated enabled auto-merge (squash) October 31, 2026 08:32

@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 commit: 4df76c1.
Compared the four follow-up commits since 85368e3 (ancestor of this head), focusing on the changed behavior and surrounding code.

Both prior Rust resume findings are addressed: the logical end now retains its error across route switches, and datagram-only readers check their own drained channel. The latter also resolves the independently reported finding; focused regression tests are included for both.

No new actionable bugs found in the reviewed update. Direction is sound: resolved ranges determine whether the error is owed, pre-07 peers avoid an early clean end that races the reset, and archive recording preserves the observed terminal error. These targeted changes keep the distinction between an errored finish and an abort without a separate termination mechanism.

Verification limits: GitHub-only source and test inspection; no builds, Rust/JS tests, or interop runs executed. Test results are not independently verified.

@moq-bot
moq-bot Bot merged commit bd7a669 into main Oct 31, 2026
1 check passed
@moq-bot
moq-bot Bot deleted the quest/m2/errored-end branch October 31, 2026 16:38
kixelated added a commit that referenced this pull request Oct 31, 2026
Resolve the read path against main's errored track end (#7349) and reader mirror: a plain read that runs out maps to ended(), and resume keeps Mirror beside the peek/take pair.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant