Repository navigation
feat(net)!: an errored track end keeps its history - #7349
Conversation
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>
|
Warning Review limit reachedYou'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. View limit details
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. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 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".
kixelated
left a comment
There was a problem hiding this comment.
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–804reads only the newest copy's error, whilelearn_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_finishedstill returns A's boundary butdeclared_errorreturns 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 alongsideReader::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'sdone, whereas datagram reads populatedatagrams_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.
…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
left a comment
There was a problem hiding this comment.
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.
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>
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::Consumeruses this to replay a stored{"error":code}end the way its source ended. This finishesquest/m2/errored-end.md.Changes
track::Producer::finish_at(end, error: Option<Error>). Settingerrorstores it beside the end position. Unlikeabort, the cache, fills, and listing behave exactly as after a clean finish. A reader that reaches the end gets the error instead ofOk(None). That coversrecv_group,Ordered::next_group,recv_datagram, routed (resume) readers, andProducer::closed. A reader whose range stops short of the end still ends cleanly. A repeated or refined end must carry the same error code.SUBSCRIBE_ENDgains anError Codefield: 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.finishAt(end, error?), the lite-07SubscribeEndcodec, the publisher and subscriber, resume, and the blocking and nonblocking reads.End::Error(code)becomesfinish_at(after the newest stored sequence, Some(error)), which replaces the clean finish and its warn log.SUBSCRIBE_ENDof 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 storesEnd::Error, not a clean finish.drafts/draft-lcurley-moq-lite.md(SUBSCRIBE_END field and lite-07 changelog) anddoc/concept/moq-lite.md(one sentence).quest/m2/README.md.Impact
track::Producer::finish_attakes a second argument. To migrate, passNoneto keep today's clean end. Every in-repo call site is updated, includingmoq-ffi,moq-c,moq-json, and the interop client. The FFI and C APIs are unchanged.Track.Producer.finishAt(end, error?)is additive.Err(error)/ a rejection.Producer::closed()reports that error.moq-lite-07-wip, unpublished)SUBSCRIBE_ENDaddsError Code (i). Published versions are unchanged.Choices made without asking
API shape:
finish_attakes the error as a parameter, as the quest Plan suggested. Alternatives I considered:finish_at(end: impl Into<track::End>)with atrack::End { position, error }type. No call-site churn, but it adds a new exported type.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, notEnd::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
abort's existing downgrade.SUBSCRIBE_ENDhandling inrs/moq-net/src/lite/subscriber.rs. Whichever lands second will need a small textual merge wherefinish_atis 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 checkpasses, andjust test interop --allpasses.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 inan_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", andmoq-archive'san_errored_end_records_its_error.Review
Reviewer: Codex
gpt-6.1-sol, medium effort, viaquest review. Reviewed head62110e4120d8a596cac1edb77fbd82fe3700c57d. Landing head4df76c157e3fbc048c3eb0b0aa722fcf438ec55b: it only adds a rustfmt pass on two new tests and restores main's#[cfg(any(feature = "async", test))]onSubscription::end, whichendedno 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 ine040177e:declared.Merge-time session:
e040177e, three findings, all fixed in017c8136:SUBSCRIBE_ENDcould let the subscriber settle before the reset. The publisher now withholds it.017c8136, one finding, fixed in62110e41: a lite-06 subscription capped short of an errored end never got its boundary. It is now declared to a range that stops short.62110e41: no findings.4df76c15: rustfmt and the restored cfg gate, skipped as trivial.just checkpasses on it.Suggested follow-ups
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code