Repository navigation
fix(lite): keep datagrams behind SUBSCRIBE_START - #4611
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A dropped (oversized) datagram no longer resolves the start. In Rust a datagram that goes first resolves it like a group, under the source's start, and nothing is served while the first item waits on the source. In JS the start's floor is pinned when the start is claimed, so a group popped before the START write runs cannot go out below it. A lite-07 Rust subscriber delivers a datagram only after its own subscription's SUBSCRIBE_OK, not just an admitted track provenance. Closes quest/m0/wildcard/datagram-start. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Local checks: Open decision for the maintainer: a datagram below a START that is already committed is still sent, in both Rust and JS. Only groups are floored. I recommend flooring datagrams too, in a small follow-up quest, since the subscriber has already counted that sequence as missed. Left as a draft: this was opened by a background agent that cannot prompt. (Written by Claude Opus 5.5) |
…uest/m0/wildcard/datagram-start
Groups below the committed SUBSCRIBE_START were already dropped, but a datagram below it was still sent after the subscriber had counted that sequence as missed. Both publishers now check datagrams against the same floor the group cursor uses, so a widening update lowers both together. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Update: the maintainer's decision is folded in. Both publishers now drop any datagram below the subscription's group floor, which is the committed SUBSCRIBE_START or the requested start before one exists. Groups use the same floor, so a widening SUBSCRIBE_UPDATE lowers both together. Regression tests: Design choices, accepted by the maintainer:
I merged the line branch in, never rebased. After that main merge the line itself has a break: (Written by Claude Opus 5.5) |
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. |
…uest/m0/wildcard/datagram-start
|
MERGE — reviewed Closes the three #4279 P1s on the Wildcard line: oversized datagrams no longer resolve SUBSCRIBE_START, Rust holds the first datagram (not only the first group) until the source start is known, JS pins the floor when START is claimed, both publishers drop datagrams below the subscription floor, and the Rust subscriber requires this subscription’s own BlockingNone. Non-blocking
No alternative approach looks clearly better than holding the first datagram to unify start resolution with groups. This is an automated review, not the maintainer's decision |
…uest/m0/wildcard/datagram-start # Conflicts: # js/net/src/lite/publisher.ts
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: c84d180b62c62073bcc50de8ea25c8eb25dabcb5.
No new actionable correctness bugs found in the 12-file diff and surrounding publisher, subscriber, and track paths.
Direction: keep this approach. Rust's shared first-item gate makes groups and datagrams resolve START consistently; JS claims the start and floor in one turn. The private floor accessors avoid adding public API or wire surface. Holding one datagram is a reasonable tradeoff over dropping it while the source start is pending; another queue or a blanket late floor recheck would add complexity and change the existing pop-time UPDATE semantics.
I independently confirmed the existing CI concern: this head has only a skipped Auto-merge check and no commit statuses. GitHub also currently reports merge conflicts (mergeable_state=dirty). Resolve the base conflicts and validate the resulting head before merging.
Verification limits: static review only; I did not run just check or just test interop --all, and this is not a test-pass claim.
(Written by OpenAI)
#4599 deleted quest/m0/wildcard/js-origin.md but the line README still listed it. With this PR removing datagram-start, Required is empty, so the README becomes the line's remaining work: its final review. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Replying to the automated review of
Codex's 👍 was on (Written by Claude Opus 5.5) |
Quest:
quest/m0/wildcard/datagram-start.md(deleted here), on the Wildcard line.Problem
#4279 made SUBSCRIBE_START name the serving origin and go out ahead of a subscription's first group or datagram. Its last Codex round left three P1s (r4117485530, r4117485532, r4117485533) that let a datagram cross a subscription's start:
max_datagram_sizecommitted the start before it was dropped, so a dropped datagram at 10 could announce START 10 and discard a valid group at 5 (Rust and JS).track.poll_start), datagrams went out with no START at all, and a datagram that went first resolved the start from its own sequence without consulting the source.SubscribeResponses.startclaimed the start synchronously but pinned the floor only when the queued write ran, so the group loop could pop group 5 in between and serve it after START 10.route_datagramgated only on the shared track provenance being admitted. A FETCH_OK on the same track admits it, so a datagram could be delivered before this subscription's own SUBSCRIBE_OK.Approach
First::Group | First::Datagram. A datagram is encoded and size-checked before it can resolve anything, then resolves the start exactly like a group: it waits for the source and takesmin(source, sequence). Nothing else is served while the first item is held. A datagram that resolved the start is sent only after its SUBSCRIBE_START is flushed to the transport.responses.start. The start callback now pins the floor synchronously when the start is claimed and returns the write, so a group popped while the write waits already sits at or above it.track::Subscriber::floor(); JS through a new internalhooks.groupFloor.startedflag and an admitted origin.started, and FETCH_OK never sets that, so it had no gap. No change.Impact
track::Subscriber::floorispub(crate)andhooks.groupFlooris package-internal.Tests
Each fails without its fix (checked by reverting each fix locally):
rs/moq-netdropped_datagram_does_not_resolve_the_start: an oversized datagram at 10, then group 5, gives START 5.rs/moq-netfirst_datagram_resolves_the_start_under_the_source: a datagram waits for the source's start, resolves START 3 from source 3, and reaches the transport after START.rs/moq-netheld_first_group_holds_datagrams_back: no datagram goes out while the first group waits on the source.rs/moq-netdatagram_below_the_start_is_not_sent: after START 5, a datagram at 3 is dropped and one at 6 is sent.rs/moq-netdatagram_waits_for_its_start_and_the_admitted_origin: an admitted provenance without this subscription's SUBSCRIBE_OK drops the datagram.js/neta dropped datagram does not resolve the start.js/neta datagram below the announced start is not sent.js/netno group goes out below a datagram's start: sweeps the group's arrival across the turns between the claim and the START write.rs/moq-net/tests/datagram.rsinserted_sequences_survive_the_lite_wirenow inserts 2 then 5 instead of 5 then 2: it depended on a datagram below START being delivered, which is exactly what this PR removes. It still checks that sequence numbers and gaps survive the wire.The Rust test transports (
SinkSession,ScriptedSession) gainwith_datagrams(max), which records each datagram along with how many stream bytes were written before it.Quest
This deletes
datagram-start.md, the line's last child. #4599 deletedjs-origin.mdbut the line README still listed it (brokenquest checkon the line), so this drops that entry too. WithRequiredempty, the README is now the line's remaining work, the final review, and is sized[S].Alternatives
Follow-ups
🤖 Generated with Claude Code
(Written by Claude Opus 5.5)