Skip to content

feat(net)!: lite-07 SUBSCRIBE_SKIP and budget-qualified stale drops - #7670

Merged
kixelated merged 102 commits into
mainfrom
quest/m1/drop-set
Nov 7, 2026
Merged

kixelated merged 102 commits into
mainfrom
quest/m1/drop-set

Conversation

@kixelated

@kixelated kixelated commented Nov 3, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Every reader of a track treated a dropped group as final. A narrow reader could wait indefinitely for an upstream OLD naming a group the relay never cached, and a wider reader never got a group an upstream had already reported stale. lite-07 also used one message for two different things: sequences that never existed, and groups that existed but won't be delivered.

Decisions

Maintainer decisions from 2026-11-03 (/quest-unblock), recorded in the quest Plan:

  1. JS run memory: port Rust's per-broadcast cache::Pool to JS. It bills frames and drop-set runs under Rust's default budget, with eviction and fold-or-TooFarBehind.
    • ✅ This option.
    • Not chosen: a per-track run cap; upstream backpressure.
  2. Range terms carry an explicit update number. A superseded stale drop is treated as unknown and refilled, with no term history kept.
    • ✅ This option.
    • Not chosen: a conservative refill with bounded remembered ranges; treating every range update as invalidating in-flight drops.
  3. lite-07 splits its hole message by whether the group ever existed: SUBSCRIBE_SKIP {Group Start, Group End} for sequences never produced, and SUBSCRIBE_DROP for groups that existed.
    • ✅ This option.
    • Not chosen: stale in SKIP; a single message with a reason enum; renaming only.

Maintainer decisions from 2026-11-03 (/quest-complete), which change the design (Design change comment):

  1. ✅ Rename Terms to Update on lite-07 SUBSCRIBE, SUBSCRIBE_UPDATE and SUBSCRIBE_DROP, in the draft, Rust and JS.
  2. ✅ Drop Max Age from SUBSCRIBE_DROP: DROP = {Group Start, Group End, Error Code, Update}.
    • The budget of a stale (OLD) drop comes from the subscriber's own record of what it sent with that Update. A drop under a superseded Update is no verdict.
    • Any SUBSCRIBE_UPDATE reopens every outstanding OLD drop. The publisher re-offers what the new terms cover and drops again, under the new Update, what is still stale.
    • A non-OLD drop is final, whatever its Update.
  3. ~~SKIP comes from the publisher's own group creation, eagerly.~~ Replaced by decision 9.
  4. ✅ track::Producer::drop_groups is crate-private, and skip_groups stays public.
  5. ✅ The Impact section lists only real public API changes.

Maintainer decisions from 2026-11-03 (/quest-complete), replacing decision 6 and the earlier ## Waiting item on out-of-order creation (Design change comment):

  1. ✅ SKIP comes only from the producer: the public track::Producer::skip_groups (JS skipGroups), and the implicit skip in append_group, which skips every sequence below the appended group that never arrived and was not already given up. create_group implies nothing, so claim_sequence retries and seek keep working. No SKIP is inferred from a gap or at finish. (Decision 13 later made this skip replace a drop given there too.)
    • Not chosen: eager skip on create_group with a late group winning over it; refusing every create_group below the last group.
  2. ✅ SKIP is final: creating a group or datagram at a skipped sequence fails with Error::Skipped. A dropped or aborted sequence can still be created, since a group stream wins over its drop.
  3. ✅ Skipped and dropped groups are tracked as runs in Rust and JS. A huge skip costs one run and one SUBSCRIBE_SKIP.
  4. ✅ No create_group gap error, for now. Gaps stay legal and are declared by append_group or skip_groups; the model has no principled bound on a gap's size.
    • Not chosen: a debug log on a gap; a configurable size limit.

Decided 2026-11-03 (maintainer, /quest-complete), replacing decision 13 (an agent decision from review, 2026-11-04, that a SKIP replaced only a stale OLD drop) and its ## Waiting item. Recorded in the Drop set quest Plan:

  1. ✅ SKIP replaces every drop, final or stale. "Never produced" is the most authoritative fact about a sequence, so a received SKIP, skip_groups, or append_group's implicit skip overwrites an earlier DROP of any code (OLD, aborted, evicted, NOT_FOUND) on those sequences, and a later DROP leaves the skip in place.
    • Readers that drop already ended stay ended; nothing is re-delivered. The run's reason becomes Skip, so every later reader, relay and downstream report sees SKIP. A relay forwards the SKIP even over a DROP it already forwarded.
    • A cached group that arrived is never overwritten. A received SKIP naming one leaves the group, is logged as a protocol error (tracing::warn!, JS console.warn), and keeps the session, since the draft requires no more.
    • The lite-07 draft's SUBSCRIBE_DROP and SUBSCRIBE_SKIP sections and the lite-07 changelog say so.
    • Not chosen: SKIP replacing only a stale OLD drop.

Agent decision from review, 2026-11-04 (/quest-iterate). Decided 2026-11-05 (maintainer, /quest-plan): ✅ kept as recorded.

  1. ✅ A lite-07 Group Stream or final DROP the publisher's reader reaches replaces a stale (OLD) drop it already sent. Why: a front that switches routes can hand out the new route's group, or its final verdict, for a sequence the old route reported stale, before the update that reopens it, and the publisher suppressed both, so the update left the subscriber owing a group nothing would re-offer (the OpenAI review's P2 on d8d24d54d0). The draft already says a Group Stream wins over its drop and a final drop replaces a stale one. The end sweep's NOT_FOUND never replaces a stale drop, so an update can still reach an ended track's history. Not chosen: re-offering the consumed group at the update, which would keep a second copy of the reader's handouts.

Maintainer decision from 2026-11-03 at the round-4 design checkpoint (/quest-merge via /quest-complete), replacing the ## Waiting item on Codex's round-4 finding (Design change comment):

  1. ✅ Continue: retire the stale report when its group wins. A group handed out, a SKIP, or a final drop takes its sequence out of the reader's stale record (Rust DropCursor::reported_stale, the resumed front's Reader::reported_stale, and JS #reportedStale), so the next SUBSCRIBE_UPDATE neither sends OLD for a served group again nor names it NOT_FOUND at the end. Implemented with these related changes:
    • A plain cursor now reports a final drop over a stale one it reported, as the resumed reader already did.
    • The publisher records an OLD for its own expired stream as a stale drop it sent, so a later group or final drop replaces it, and no update drops it again (Pullfrog's thread).
    • On the subscriber, a Group Stream wins over a stale drop in either order. An OLD that lands after the stream's header leaves the group alone, whether it is still arriving or complete. An update owes again only the stale runs the track still holds, so a group that arrived stays accounted for (the OpenAI review's P2 on 2b760471c0).
    • The lite-07 draft says a stale drop ends no group whose Group Stream arrived, and that no update reopens a sequence its Group Stream accounted for. The Drop set quest Plan records decision 14 and this decision.
    • Not chosen: reverting decision 14; merging as is.

Agent decisions from /quest-iterate, 2026-11-05. Decided 2026-11-05 (maintainer, /quest-plan): ✅ each kept as recorded, in the Drop set quest Plan too. The first four resolve the merge of main's lite-07 Range IDs (moq-dev/moq#7872); the rest answer review findings:

  1. ✅ SUBSCRIBE_UPDATE carries Update after the Range edit (Remove Count, Range IDs, Ranges) and before Wanted Group, in the draft, Rust and JS. SUBSCRIBE_SKIP keeps 0x3 beside SUBSCRIBE_STARTED at 0x4.
  2. ✅ The Rust subscriber's OLD drop records a stale run except on groups a range fill asked for, which main's recovery asks for again on a SUBSCRIBE of its own. Every drop it accounts, and every SKIP, also answers the fills owing those groups (a SKIP as not found). An OLD drop under a superseded Update answers no fill, since the publisher re-offers or drops it again.
  3. ✅ The JS covered-FIN reopen loop goes, as main deleted it, and with it this branch's stale-run supersede on reopen. Tests written against unlimited history ask for it, since authored tracks now default to a zero max age (moq-dev/moq#7729).
  4. ✅ A final OLD drop, such as one relayed from a lite-06 upstream, goes out as the stale drop the lite-07 wire names it: the publisher records it as a stale drop it sent and drops it again on each update, as it does its own expired stream (Rust and JS). Not chosen: renaming the code on the wire.
  5. ✅ An update judges again the re-offers still waiting for stream credit, by moving them back to the expired runs, so a range the update no longer admits drops them again as OLD instead of serving them. A verdict sent for a queued re-offer keeps it from opening (Rust; JS already checked served).
  6. ✅ A SUBSCRIBE_DROP naming an Update later than the last one sent is a PROTOCOL_VIOLATION (Rust, JS, draft).
  7. ✅ A received stale run is judged again for a reader whose terms changed (Rust Misses::judge), so a widened budget asks a fill for it. JS judges on every ask already.
  8. ✅ JS: recording a stale verdict no longer answers outstanding fills, as Rust never did. skipGroups checks its bounds, and a Rust fill cannot cache a skipped sequence. (The JS insertDatagram check went with main's datagram split, moq-dev/moq#7846: JS datagram tracks now keep their own sequence counter, so a group skip no longer names them.)

Decided 2026-11-05 (maintainer, /quest-complete), replacing the ## Waiting item on the duplicate-stream race. Recorded in the Drop set quest Plan and the lite-07 draft:

  1. ✅ Hold the duplicate stream until the first copy settles. On lite-07, an expired group's OLD drop and the stream the next update re-offers for it can both reach the subscriber before the original stream's reset. The subscriber used to refuse the re-offer as a duplicate and lose the group once the reset landed. Now a Group Stream whose sequence another copy still arriving holds waits, unread: it is discarded if that copy finishes, and takes the group if it fails. Rust: track::Producer::create_group_at (crate-private) returns Received::Held with the arriving copy, and GroupRecv waits on it in a Hold state, also watching its own reset and the track's abort. JS: a hooks.arrivingGroup check before receiveGroup in runGroup. A held stream keeps its tail count open and stays at RECV_PARKED, the lowest receive priority. So by design it holds at most its stream credit, takes no bytes, and is the first stream a full receive window evicts; the copy it waits on keeps its credit.
    • A reader already handed the failed copy still sees it fail, since the model never hands out a sequence twice (track::Subscriber::delivered). The cache, fills, and readers that had not reached the sequence get the re-offer.
    • The draft now allows a second Group Stream for a sequence only as a re-offer after a reset, and states the hold.
    • Not chosen: letting the duplicate replace the copy still arriving; accepting the loss.

Agent decision from review, 2026-11-05 (/quest-iterate). Decided 2026-11-05 (maintainer, /quest-plan): ✅ kept as recorded.

  1. ✅ A relay cursor's stale report that an upstream's received stale run decided stays the upstream's to judge after an update. If the relay's own update superseded that run first (supersede_stale), the downstream update leaves the sequence owed instead of dropping it again as OLD. The cursor records which reported stale runs a received run decided (Rust DropCursor::reported_received, JS #reportedReceived), and reoffer_stale gives up again only the runs it judged itself. Why: Codex's round-1 finding at 831959fadc. The re-drop manufactured a verdict for a sequence the upstream still owed, so a downstream subscriber could settle its tail before the upstream's re-offer arrived. Not chosen: subtracting only the runs the track still holds, which is what the code did before.

Agent decision from review, 2026-11-05 (/quest-iterate), for the maintainer to review:

  1. A Group Stream that finished wins over a lite-07 stale (OLD) drop under the same Update, even once the cache evicted its group. The subscriber's tail records the group streams that finished since the last update it sent (Rust Tail::finished, filled by Reading::finish; JS Tail#finished), a stale drop leaves those sequences, and each update clears the record, since a drop under an earlier update is no verdict. Why: the OpenAI review's P2 on 0fee3fd91d, repeated on 2a43d68b5d. A replacement stream can overtake the OLD the publisher sent before serving it, finish, and be evicted before that OLD lands; judged against the cache alone, the late OLD was recorded stale and the next update owed the group again, which the publisher had already retired, so the subscriber waited a fresh grace or refetched it. The record is one more unbilled run set, listed in the quest's storage to bound. Not chosen: keeping the record for the subscription's life (an update already makes an earlier drop moot); recording at the stream's header, which would also hide the OLD for a stream the publisher reset as expired.

Decided 2026-11-06 (maintainer, /quest-complete), replacing agent decision 27 from the previous round (29d2227107, e958788d8b). Recorded in the Drop set quest Plan:

  1. ✅ The stale verdict accepts every range. The subscriber stores the upstream's full range set for each update (Rust TrackState::judged, JS judged), so each range the upstream judged stale is settled as stale for the reader directly. A range bound only the publisher resolves counts at its widest: a start from group 0, an end past the live edge. A received stale run settles a reader no wider than the budget it was judged under that reads every group those ranges name above the run's start, however many ranges they span (Rust and JS settles), since a group's verdict looks only at the groups above it. This replaces judged_end, which accepted only the reader's last contiguous range. Why: Codex's round-2 finding at 0e3e7ab705. A bounded reader asking for [0,2) and [4,6), exactly the upstream's ranges, with both dropped as stale before any group arrived, left [0,2) unknown and waited forever. It is now settled with nothing received and no fill asked.
    • Not chosen: asking a fill for an unknown stale range wherever it falls (the previous round's fix); reversing the fix and tracking the hang as a follow-up.

Decided 2026-11-06 (maintainer, /quest-complete), confirming agent decision 28. Recorded in the Drop set quest Plan:

  1. ✅ Only the part of the previous round's fix that the multi-range verdict doesn't cover stays: asking a fill above the newest group received for a stale run the reader can't trust. That ask no longer waits for a fill handler. Why: a reader narrower than the upstream's ranges, such as [0,2) and [4,5) under an upstream that judged [0,2) and [4,6), can't trust the verdict, since group 4 is its own newest and no verdict convicts it. Its ranges add nothing to the upstream's terms, so no update brings the runs back, and with nothing received it waited forever. It now asks a fill for them (Rust Misses::judge extending its span and TrackState::unfed, JS #askFills). The has_handlers() gate from e958788d8b went, which answers the OpenAI review's P2. The gate hid a fill taken before the last handler closed, and its cached answer, from a later reader. The gate also changed nothing in production: a received copy's serving session holds its handler while it feeds the copy (dynamic in the lite serve loop), and a copy nothing feeds gives everything up through nothing_arrives. The two tests that modeled a received copy with no handler now hold one. Not chosen: keeping the gate and widening it to taken fills and cached backfill, which would keep a test-only path.
    • Maintainer reasoning: a verdict judged on the union of readers' ranges is valid only for readers that read every group it names above the run's start; a narrower reader still needs groups the upstream already dropped, so only a fill can deliver them.
    • Not chosen: re-judging per reader (it would still need the fill); separate upstream subscriptions per reader shape (they give up aggregation); deferring the narrow case as a follow-up.

Impact

  • Public API
    • Rust: track::Producer::skip_groups is new and is the explicit way to produce SKIP. It replaces any earlier drop of the same sequences, but never a cached group. track::Producer::drop_groups is no longer public (it is pub(crate)); its only external caller, moq-archive, now uses skip_groups. Error::Skipped is new (the enum is #[non_exhaustive]). create_group and append_group now refuse a skipped sequence with it, and a later drop leaves it skipped. append_group skips the sequences below it that were never created, from just past the previous append.
    • JS: Track.Producer.skipGroups(start, end) and Error.Skipped are new, with the same rules (js/net/api/index.api.md).
    • Bindings: moq-ffi's MoqTrackProducer::skip_groups(start, end) is new, with Go TrackProducer.SkipGroups, Python TrackProducer.skip_groups, Swift TrackProducer.skipGroups, and the regenerated Dart and Kotlin bindings. The C API gains moq_publish_track_skip(track, start, end). Each is additive.
    • Migration (for the release cut's MIGRATE.md; this PR no longer writes it):
      • moq-net: track::Producer::drop_groups is no longer public. To say the track will never hold some groups, call skip_groups(range): readers stop waiting for them, no reader ends, and a relay forwards a lite-07 SUBSCRIBE_SKIP.
        // Before
        producer.drop_groups(from..to, moq_net::StreamError::NotFound);
        // After
        producer.skip_groups(from..to);
        A skip is final: create_group and append_group at a skipped sequence fail with Error::Skipped, and a later drop leaves it skipped. It replaces an earlier drop of the same groups, whatever its error, but never a cached group. append_group now skips every sequence below the appended group that was never created, from just past the previous append (before the first append, from the lowest group created), so call create_group for every sequence you still mean to fill before appending. Before the first append, a sequence below the lowest created group is not skipped: call skip_groups for it if you will never produce it.
      • @moq/net: the same, with Track.Producer.skipGroups(start, end), createGroup, appendGroup, and the Skipped error.
    • moq-net's lite module is private (rs/moq-net/src/lib.rs), and @moq/net exports only . and ./zod, so the codec changes below are not public API.
  • Wire
    • lite-07-wip: SUBSCRIBE_SKIP (0x3) added, overriding an earlier SUBSCRIBE_DROP of the same sequences; SUBSCRIBE, SUBSCRIBE_UPDATE (after the Range edit) and SUBSCRIBE_DROP gain Update. A SUBSCRIBE_DROP naming an Update never sent is a PROTOCOL_VIOLATION. A publisher may send a second Group Stream for a sequence only to re-offer a group whose stream it reset, and a subscriber holds such a stream until the earlier one ends.
    • Decision 27 changes nothing on the wire: the subscriber already holds the ranges it sent with each update. The draft's SUBSCRIBE_DROP section now says a relay settles with a stale drop a reader whose Ranges include every group the relay's own Ranges name above the dropped sequence (it said "cover the relay's own").
    • Published versions are unchanged.

Validation

At 037ce47399 (the merge of origin/main up to 142b03bd5c; 16d4bd19c5 adds main up to 1ee8eec951, with quest check rerun): cargo test -p moq-net -p moq-mux and clippy -D warnings on both, just js check-changed and just js test-changed on the branch's changed files, just biome, quest check --fix, and quest check --base origin/main pass. The full just ci was not rerun.

At fa97eb708e (a merge of origin/main up to 2460728a2e onto 4fee4d5280 as e15fa69a71, then fa97eb708e):

  • nix develop --command just ci origin/main passes on e15fa69a71 and on fa97eb708e: every check module, and 11083 Rust tests (nextest) on fa97eb708e. The merge was conflict-free.
  • Codex's round-1 finding (a lite-07 re-offer dropped again before it opens was left out of served, so the end sweep sent a final NOT_FOUND over its stale drop): fixed in fa97eb708e in Rust (poll_step's redrop branch) and JS (#runTrack's pendingDrops loop). Rust a_redropped_reoffer_stays_stale_at_the_end_on_lite07 fails without the fix (drops [(0,0,0),(0,0,2),(0,0,2)]). JS "lite draft-07: a re-offer dropped again before it opens stays stale at the end" fails 200/200 reruns without it, and passes 500/500, and 200/200 pinned to two cores beside four busy loops, with it. That test stubs Budget.prototype.poll, since a write's budget yield runs on host CPU time.
  • "lite-07 answers a bounded window that resolves empty and FINs in reply" (js/net), which failed this PR's previous local just ci and CI's Check on 4fee4d5280, fails on main the same way. Its cause is found, and this PR keeps its code:
    • Command, in js/net: bun test src/lite/publisher.test.ts -t "FINs in reply" --rerun-each 200, no extra load. Main 3eb044f8da: 12/200 failed in each of 3 runs. This PR's 4fee4d5280: 12/200 in each of 3 runs. Every failure is the same error: reset stream: 1 (Cancel) thrown from SubscriptionControls.response (js/net/src/lite/publisher.ts:362 on main), the trace CI printed.
    • Under load, the whole file run 30 times pinned to two cores beside four busy loops failed 0/30 on each side, so load alone is a poor reproducer: the failure needs the write budget to expire on one particular write.
    • Cause: Writer.write (js/net/src/stream.ts:818) yields a host task on the shared write Budget after its bytes are already enqueued, once the budget's 4 ms CPU slice has run out (checked every 32nd write). The client reads SUBSCRIBE_START and FINs during that yield, so response() sees the subscriber's clean end before the encode settles, treats the write as blocked, and resets with Cancel instead of FINning. With writeBudget.poll() stubbed out, main passes 200/200.
    • The test, response(), and the empty-window branch are byte-identical to main's. A follow-up is suggested below.

At 4fee4d5280 (a merge of origin/main up to 16ff94ea19 onto 8aa91c2b6f, after the merge queue dropped 8aa91c2b6f for a conflict):

  • Merge conflicts: js/net/src/lite/publisher.ts, where the subscription's finally now closes this PR's held re-offers and then owes main's drain of opened group streams, both ahead of priority.close() (the maintainer's resolution). quest/m2/subscribe-ranges/README.md keeps this PR's SUBSCRIBE_SKIP/SUBSCRIBE_DROP split with main's wording for datagrams, since main finished the Datagram tracks quest it linked.
  • nix develop --command just ci origin/main: every module passes except test js, where one @moq/net test failed: "lite-07 answers a bounded window that resolves empty and FINs in reply" (the publisher reset with Cancel instead of a FIN). It passes alone (3/3), across publisher.test.ts (5/5), and in the full js/net suite. The test, Controls.response, and the empty-window branch are byte-identical to main's and outside this PR's hunks: under load, the client's FIN can win response()'s race against an encode that was already read. It is a pre-existing load flake, suggested below.
  • Swift: nixpkgs does not build Swift on Linux, so just ci's swift module only checks formatting. nix develop --command bash sh/swift/check.sh on a macOS host (arm64, Xcode swift), run on this head's tree, passes: 43 XCTest tests, 0 failures.

At 30472607a6 (a merge of origin/main up to f2f9bc935e onto 9321f3bca0, with Pullfrog's 54cac9621e, 3167350e48, and 30472607a6):

At fff5f03acd (this round: a merge of origin/main up to 45930a78eb onto cfbfb6451d, then d7d84d7754 and fff5f03acd):

  • just ci origin/main passes: every check module, and 11054 Rust tests (nextest). The first run caught a clippy single_range_in_vec_init in a new test helper and a biome import order, both fixed in fff5f03acd.
  • Merge conflict: rs/moq-net/src/model/resume.rs, where main's moq-dev/moq#7993 replaced route_failed with Error::retryable. The PR's (hole, received) drop record now filters with retryable.
  • Decision 27's regressions: Rust a_stale_verdict_settles_a_reader_of_every_range_it_judged and JS "fill: a stale verdict settles a reader of every range it judged" (the [0,2)+[4,6) case kept from the previous round) now assert the reader is settled with no fill asked. The previous round's code fails them, since it left [0,2) unknown and asked a fill.
  • Partial overlap: Rust a_stale_verdict_settles_a_partly_overlapping_reader_only_where_it_reads_above and its JS twin. A [1,2)+[4,8) reader is settled and asks nothing. A [0,2)+[4,5) reader asks fills 0..2 and 4..5. Each fails with the ask above the newest group disabled, so decision 28 keeps it.
  • The P2: a_taken_fill_answers_an_unsettled_stale_run_after_its_handler_closes fails with the has_handlers() gate restored (the later readers got nothing) and passes without it. a_received_stale_drop_settles_only_the_readers_it_covers and a_received_stale_run_settles_a_bounded_reader_only_through_the_judged_ranges (renamed from ..._to_the_judged_end) now hold a fill handler, as a served copy does.

At cfbfb6451d (merges of origin/main up to 804ce0fdad and Pullfrog's 915ed98b4e onto 897a9c0176, with 29d2227107 and e958788d8b):

  • just ci origin/main passes: every check module, and 11042 Rust tests (nextest).
  • Merge conflicts: quest/m0/test-flakes-2/README.md kept both sides' added paragraphs. js/net/src/resume.test.ts took Pullfrog's 915ed98b4e, which drops hooks.receiveTrack from the shared copy helper, as Rust's local helper does. That replaces this round's own fix (a8bf6e7603, an authoredCopy helper for one test).
  • Correction: the record at 897a9c0176 below was wrong. That head failed main's new "a backfill waits on a fill the finished route still owes" (moq-dev/moq#7995) deterministically, locally (5/5) and on GitHub's Check. A received copy's live feed owes the groups below its first arrival for the reader's grace, so the test's immediate fill never came. It passes on main and on this head.
  • Decision 27's regressions: Rust an_unsettled_stale_run_is_asked_for_with_nothing_received and JS "fill: an unsettled stale run is asked for with nothing received" each fail without the fix. The handler gate keeps a_received_stale_drop_settles_only_the_readers_it_covers and a_received_stale_run_settles_a_bounded_reader_only_to_the_judged_end passing: a reader with no handler still waits.

At 897a9c0176 (this round: merges of origin/main up to d505e963de onto 70c04a96f6, with 72bab5800f, 236ce6d29a, and fe6c1e1231):

  • just ci origin/main passes: every check module, and 11040 Rust tests (nextest). Both merges were conflict-free.
  • The land fixer's model::track::test::pool_debt_sheds_a_flight_without_its_cached_group failure is not this PR's. The test is byte-identical to main's. Across 10 interleaved parallel runs of the moq-net lib test binary, at fc47e7e6a7 and at main 43fcb958d5, it failed 8/10 here and 6/10 on main. Counting either pool-debt test, both sides failed 8/10, and no other test failed. It passes under CI's nextest, where each test gets its own process. It is owned by quest/m0/test-flakes-2/pool-debt-flight.md in the open moq-dev/moq#8024.
  • The datagram end-before-close quarantine (70c04a96f6, Pullfrog) stands. Its quest now lists the overlapping PRs. The open moq-dev/moq#8000 edits the same case's body: it stubs Budget.prototype.poll and raises the microtask count, which may remove the suspected host-task yield. The draft moq-dev/moq#7981 quarantines only lite-06 and adds its own quest at the same path. Whichever lands second reconciles them.
  • Pullfrog's thread on quest/m1/drop-set.md: fixed. The quest Plan now says the implicit skip starts just past the previous append (before the first append, from the lowest group created), as the code does. The Rust and JS doc comments around it were rewrapped.
  • Codex's binding finding: raw_track_skips_are_final in moq-ffi and moq-c checks that a skipped sequence refuses creation, a sequence past it still works, and a closed handle errors. Dart was regenerated with sh/dart/generate.sh.
  • Codex's ledger finding: unaccount_restarts_the_older_gap_it_joins (Rust) and "unaccount restarts the older gap it joins" (JS) fail without the fix.

At 13a544e416 (merges of origin/main up to 6ca2ed5e35 onto e089338d3c, which replace the land fixer's unpushed b4d1318008):

  • just ci origin/main passes: every check module, and 11002 Rust tests (nextest).
  • Merge conflicts: rs/moq-net/MIGRATE.md and js/net/MIGRATE.md take main's side, since PRs no longer write MIGRATE.md (the release cut does, from Impact sections; CONTRIBUTING.md's Releases section and the migration audit quest, decided 2026-10-23). This PR's notes moved to Impact's Migration item. Three JS files kept both sides (Hole drop imports with main's RouteInfo, the resumed reader's fields, and both sides' new tests). Five quest files took main's milestone re-cut and re-applied this PR's edits (the Drop set and JS cache pool entries, SKIP wording, and the /quest/m2/ paths). Main's new call sites needed this PR's API: the IETF headless-group drop and socket.test.ts pass a Hole, and widened_max_age.test.ts uses origin.route().
  • Pullfrog's scan-key thread: a_queued_drop_poll_keeps_the_last_drift fails without the fix (the queued poll returned Pending instead of the settled run 3..4).
  • model::track::test::pool_debt_sheds_a_flight_without_its_cached_group is not this PR's: the test is byte-identical to main's, and across 15 interleaved parallel runs of the moq-net lib test binary it failed 8/15 on this head and 10/15 on main (its sibling pool_debt_sheds_a_flight failed 12/15 and 10/15). Its quest is quest/m0/test-flakes-2/pool-debt-flight.md.
  • The optional Check's failure on e089338d3c was copy fill: lite-05 fills below the live edge only what a reader's ranges name (js/net/src/lite/copy_fill.test.ts:168), a wall-clock 200 ms idle wait. Not this PR's: 20 interleaved runs of that file failed 0/20 here and 1/20 on main.
  • cargo clippy -p moq-net --no-default-features --lib --tests -- -D warnings still fails on main's EXPORTER_LABEL, Ledger::unaccount, and a test's read_frame; this PR's give_up no longer adds to it.
  • One just ci run hit lite-07 bounds and prioritizes independent createWritable datagram queues, and one of 15 full js/net runs hit moq-lite-06: a session close right after a datagram track's end keeps the end; main failed neither in 15 full runs, and both passed 60/60 concurrent runs of datagram-tracks.test.ts on each side. Too rare to attribute; this PR changes no datagram code.

At bfb98c053c (a merge of origin/main 1c140ac9b9, which brings flights from moq-dev/moq#7944, onto 6dc75e9e08): just ci origin/main passes. One conflict, in rs/moq-net/MIGRATE.md, resolved by keeping both Unreleased entries. The optional Check's failure on 6dc75e9e08 was moq-ffi test::datagrams_cross_a_default_connection, a known flake that moq-dev/moq#7954 quarantines.

At 6dc75e9e08 (a merge of origin/main c76f9eb9f4 onto 44d32a9f4a):

  • just ci origin/main passes: every check module, and 10916 Rust tests.
  • Pullfrog's thread: decision 26's regressions (Rust a_finished_group_wins_over_a_late_stale_drop_once_evicted, JS "a finished group wins over a late stale drop once evicted on lite-07") now deliver each group over a real stream (GroupRecv::new, Subscriber.runGroup) instead of finishing the tail by hand. Each fails with the record disabled at its call site. The version and clean-close gates have no observable effect on lite-07, as the reply explains.

Merge of origin/main (c76f9eb9f4): one conflict, in quest/m1/subscribe-ranges/start-bookkeeping.md, where main's rewritten Goal and ## Waiting (on this PR) were taken.

At b005f5cb98: just ci origin/main passed; decision 26's tests were added, and the Rust subscriber checks a SUBSCRIBE_DROP's Update before its empty range (Pullfrog's thread). Earlier rounds' validation is in this PR's history.

Review

Head c1ed79c3cc: review skipped as too trivial to need one (Claude Opus 5.5). Since the reviewed head it merges main 812e38fc90 (15b45885dd), resolving two conflicts by taking main's side: main's re-enabled datagram end/close case in js/net/src/lite/datagram-tracks.test.ts (moq-dev/moq#8110), so this PR no longer quarantines it, and main's quest/m0/test-flakes-2/README.md history and child list (git auto-merged datagram-end-close.md, keeping this PR's #7670 reproduction notes). c1ed79c3cc adds two lint guards for just no-async (the optional Check's failure on dde07a5944, caused by this PR): Ledger::unaccount gets the file's existing cfg_attr(not(feature = "async"), allow(dead_code)), and the test helper judged in rs/moq-net/src/model/track.rs becomes #[cfg(feature = "async")] like the tests that call it. No behavior change. Validation: just ci and just no-async pass at c1ed79c3cc, including relay's config::tests::repository_configs_load now that moq-dev/moq#8144 is merged in.

Codex gpt-6.1-sol at medium effort, against origin/main, session 01a1b4be (quest review started it on its own, since main's rs/moq-net/AGENTS.md changed after the previous session 01a1b25e, whose round 4 was the design checkpoint):

  • Round 3 at 037ce47399a8c5e038729082d2d3af154f53cac7 (the merge of main 142b03bd5c): no findings. The latest reviewed head is 037ce47399a8c5e038729082d2d3af154f53cac7; head 16d4bd19c5d91ff7eec7f54ffc403bf7b38a38f8 only merges main 1ee8eec951 on top (the quest merge driver resolved a quest/m0/test-flakes-2/README.md link list), leaving git diff origin/main...HEAD identical, so this review stands for it. The merge kept main's deletion of quest/m1/read-backfill.md (moq-dev/moq#8071 finished it), dropping this PR's SUBSCRIBE_SKIP wording there as moot, and added main's new declared: None field to this PR's SubStream test initializer in rs/moq-net/src/lite/subscriber.rs (E0063 otherwise).
  • Round 2 at fa97eb708e: no findings.
  • Round 1 at e15fa69a71 (the conflict-free merge of main 2460728a2e): one medium finding, fixed in fa97eb708e (see Validation): a re-dropped re-offer left out of served (rs/moq-net/src/lite/publisher.rs, and the JS pendingDrops path).

Session 01a1b25e: rounds 1 to 4 at fff5f03acd, 30472607a6, 8aa91c2b6f, and 4fee4d5280 reported no findings, and its fourth-round design checkpoint found no reason to change the approach.

Pullfrog's review of 9321f3bca0 raised only the E0063, fixed by 54cac9621e. The OpenAI review's P2 on cfbfb6451d is fixed by decision 28. Earlier sessions' rounds are in this PR's history.

Follow-ups

  • A quest/m0/test-flakes-2 child for lite-07 answers a bounded window that resolves empty and FINs in reply (js/net), which fails on main 12/200 under --rerun-each 200 (see Validation at fa97eb708e). The bug is in the product, not the test: Writer.write (js/net/src/stream.ts) yields on the shared write budget after its bytes are already enqueued, and SubscriptionControls.response (js/net/src/lite/publisher.ts) takes a subscriber's clean FIN during that yield as superseding a blocked write, so the publisher resets with Cancel instead of FINning. Candidate fixes: a clean FIN no longer supersedes a reply write (only an error does), or Writer.write yields before handing its bytes over rather than after. No quest or open PR covers it; moq-dev/moq#8047 fixed the same yield's race for the opened group streams only.
  • None for the two quarantined or known flakes: moq-dev/moq#8024 owns the pool-debt test, and this PR's datagram-end-close.md owns the datagram case, with moq-dev/moq#8000 and moq-dev/moq#7981 noted there.
  • moq-dev/moq#8069, Pullfrog's draft, is cherry-picked here and can close once this PR lands.
  • A test-flakes quest for copy fill: lite-05 fills below the live edge only what a reader's ranges name, which waits 200 ms of wall-clock time for idleness (1/20 on main).
  • JS cache pool: created by this split.
  • Same-track copy recovery (a reader on a failed copy continues on the held re-offer of decision 24) is already planned as quest/m1/copy-recover.md in a separate planning PR, so it is not suggested here.
  • The rewritten Drop set quest tracks the run memory bound and the remaining tests.

🤖 Generated with Claude Code

(Written by Claude Opus 5.5)

@kixelated kixelated changed the title Drop set docs(quest): record drop-set memory and range-term blockers Nov 3, 2026
kixelated and others added 7 commits November 3, 2026 12:51
Lite-07 names sequences never produced in SUBSCRIBE_SKIP and groups that
existed in SUBSCRIBE_DROP, which gains Max Age and Terms. SUBSCRIBE and
SUBSCRIBE_UPDATE carry an increasing range-terms id. A relay settles a
received stale drop only for readers it covers, forgets stale runs when it
sends new terms, and a lite-07 widening re-offers what was reported stale.

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

A local producer's end settles once the gap below it is wholly skipped or
dropped, so moq-archive drops what it holds nothing for instead of faking an
empty group. A cut a fetched copy still covers is no longer reported dropped.
Split the JS cache pool into its own quest and rewrite the drop set to the
run memory work that remains.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… a re-created dropped group in JS

A re-established upstream subscription starts new terms, so stale runs the
last one reported become unknown. The JS lite publisher no longer opens a
second stream for a sequence it already accounted for, as Rust does.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…utliving their terms

A bounded window never covers everything above a received stale run, so its
completion no longer counts one. A cursor folds the track's runs in afresh on
each rescan, so a superseded stale run leaves nothing pending, and the
widening check runs only when the budget changes.

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

A relay records where the ranges of its current terms end, and a stale run
settles a reader whose range covers it and everything above it up to there,
so a bounded subscription is settled by its own upstream's stale drop. A JS
bounded read no longer ends over a stale run that does not settle it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ers' judged end

A stale drop no longer counts as receipt in the JS arrivals ledger, so new
terms superseding it leave its groups wanted, as in Rust. A sink added later
copies the judged end with the track's runs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated kixelated changed the title docs(quest): record drop-set memory and range-term blockers feat(net)!: lite-07 SUBSCRIBE_SKIP and budget-qualified stale drops Nov 3, 2026
@kixelated
kixelated marked this pull request as ready for review November 3, 2026 22:40

@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 fe27d73.

P2: Clip stale runs to this JS reader before judging them. js/net/src/track.ts:776-779, 3151-3154 tests the original run's start against the reader's coverage. If an upstream stale DROP covers [0,10) and a same-budget reader asks [5,10), whose end reaches the upstream's judged end, the suffix [5,10) is a valid verdict for that reader. The original start 0 instead makes settles reject the whole run. #drained also treats it as unknown at lines 2532-2540, so a bounded reader can stay idle instead of completing and a downstream publisher never forwards the suffix. Clip each run to the reader's span before judging and reporting it, including the completion path, as the Rust cursor does; add an overlapping-suffix regression.

P2: Record stream-expiry drops for re-offer on widening. rs/moq-net/src/lite/publisher.rs:4373-4379, 4618-4627, 4783-4788 reports an OLD from a failed group stream directly after marking the group served, outside the cursor's reported_stale bookkeeping. If stream credit is blocked, a narrow reader can take group 0, have it expire before its header opens, and receive OLD. Widening its budget while the group is still cached cannot re-offer it: take_unreported never returns that sequence and the model already handed it out. JS has the same path at js/net/src/lite/publisher.ts:1589-1614. Register these failures in the re-offer state and restore the undelivered group to the cursor; extend the expiry-before-open test to widen afterward.

Verification: static GitHub review of the Rust and JS model, protocol paths, and tests. I did not run tests or interoperability checks.

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

@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.

Important

The lite-07 wire and the Rust settlement model look solid, but the JS mirror has a verified parity defect: a received stale run is judged as a whole instead of the reader's span, so a reader whose range starts inside the run never settles it. I also confirmed that a JS live reader never completes at a declared end above the last published group once the tail is skipped or dropped. Details below.

Reviewed changes — full 43-file diff at fe27d73d53ff1d751d534418b702889b7529f9fe, all 11 PR commits.

  • lite-07 wire: adds SUBSCRIBE_SKIP (0x3), Max Age (ms + 1) and Terms on SUBSCRIBE_DROP, and Terms on SUBSCRIBE/SUBSCRIBE_UPDATE, gated by has_skip/hasSkip; the exact vectors ([2,5,1,3,7,0,0], [3,2,1,3]) match across Rust, JS, and the draft.
  • Model: Hole::{Skip, Drop { stale }} replaces bare StreamError drop runs, with skip_groups, supersede_stale, and budget-qualified settles/Drift::settles; miss verdicts now carry the stale budget.
  • Relay: stale drops echo terms and max age; a widening re-offers reported stale runs; a superseded drop is unknown and fills by range; the end sweep sends SKIP for a local producer and DROP NOT_FOUND for a received copy.
  • JS mirror: Hole/skipHole/staleHole, judgedEnd, supersedeStale, reoffersReported, takeUnreported, and the SubscribeSkip codec.
  • Fixes/quests: reaches/tail_deadline settle a declared end over wholly given-up gaps, archive no longer fakes an empty final group, and the drop-set quest is split out into the JS cache pool.

ℹ️ JS never completes a live reader at a declared end above the last published group

Rust now settles a declared end over a wholly skipped/dropped gap via reaches (rs/moq-net/src/model/track.rs:2804-2806), which the archive fix depends on. JS still requires a published group at or above the final sequence: #complete tests #state.received >= finalSequence(end) (js/net/src/track.ts:3193), while #drop only advances the arrivals ledger, never received (js/net/src/track.ts:1061-1075). A live/unbounded reader on a JS copy whose end lies above the last group and whose tail is skipped/dropped therefore returns idle until the track closes instead of done at the declared end; bounded reads are masked by #exhausted. I reproduced this at the model level: publish group 0, drop 1..4, finishAt({ group: 4 }), and an unbounded tryRecvGroup returns idle (contrast the Rust-only test the PR adds for this exact case). Mirror Rust's reaches in #complete and add the JS counterpart of the new "declared end settles over a given-up gap" test.

Technical details
# JS `#complete` ignores given-up holes

## Affected sites
- `js/net/src/track.ts:3193` — `#complete` requires `#state.received >= finalSequence(end)`.
- `js/net/src/track.ts:1061-1075` — `#drop` does not advance `#received`; a skip/drop only calls `#arrive`.
- `js/net/src/track.ts:1302` / `:1682` — `state.received` advances only when a group/datagram is published.
- `rs/moq-net/src/model/track.rs:2804-2806` — Rust `reaches` counts `drops` covering `edge..fin`.

## Required outcome
- A declared end above the last published group completes once every sequence below it is
  covered by a group, datagram, or hole (the Rust `reaches` rule), including skip and stale holes.
- Add the JS test mirroring the Rust "declared end settles over a dropped gap" case (the PR body
  lists it as Rust-only today).

## Open questions
- Is the JS difference intentional because the wire FIN closes the copy promptly? Even so, the
  model's `finishAt` doc says readers end at the declared end "independently of closure".

ℹ️ Branch is currently conflicting with main

gh pr view 7670 reports mergeable: CONFLICTING and mergeStateStatus: DIRTY at fe27d73 against base 571c956. Rebase or merge main before the queue run so the conflict does not mask CI.

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

Comment thread js/net/src/track.ts Resolved
Comment thread drafts/draft-lcurley-moq-lite.md Resolved
A widening past the max age a stale drop was judged under makes a lite-07
publisher re-offer or drop it again, so the subscriber's tail reopens it
(Ledger::unaccount) instead of settling early, now that SUBSCRIBE_FINISH keeps
the stream open for the tail to decide.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • Resume reviews
  • Trigger review

Walkthrough

The change adds SUBSCRIBE_SKIP and version-gated update numbers to Lite-07 subscription messages. Track and reader state distinguish declared skips, final drops, and stale drops. Publishers and subscribers apply these dispositions across updates, including stale-drop reopening, group re-offers, and duplicate-stream handling. Resume paths retain and reconcile stale reports across route changes. Producer APIs add explicit skip operations, and append operations account for unproduced gaps. Archive completion also records missing sequences as skips.

Merge Risk: Low · up to e0893

The lite-07 skip and stale-drop behavior looks largely sound. Before merging, reconcile the draft's re-offer and OLD-recovery wording and the implicit-skip migration notes, and add the no-async dead_code allowance. A narrow queued-OLD update stamping edge case in the Rust publisher is also unresolved.

Pre-merge checks: Passed 4 · Inconclusive 1

Failed Failed checks: 1 inconclusive

Check name Status Explanation Resolution
Docstring Coverage Inconclusive Docstring coverage is 75.24% 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 "feat(net)!: lite-07 SUBSCRIBE_SKIP and budget-qualified stale drops" matches the main change: the PR adds the lite-07 SUBSCRIBE_SKIP message, a producer-side skip API, and Update-qualified …
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 covers SUBSCRIBE_SKIP, update-qualified stale drops, and the related Rust and JS changes.
Full details: Title check

Explanation

The title "feat(net)!: lite-07 SUBSCRIBE_SKIP and budget-qualified stale drops" matches the main change: the PR adds the lite-07 SUBSCRIBE_SKIP message, a producer-side skip API, and Update-qualified stale (OLD) drop handling. The title is specific and concise. It does not mention the Update number or the public skip_groups API, but it does not need to cover every detail.

Full details: Docstring Coverage

Explanation

Docstring coverage is 75.24% 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 105 functions across 36 files. (10 skipped: 7 unsupported, 3 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

@coderabbitai coderabbitai 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.

Actionable comments posted: 6

Nitpick comments: 3
rs/moq-archive/src/consumer/track.rs: 1

148-148: Trivial · Functional Correctness · Poor tradeoff

Cover the terminal gap's hole kind in the archive tests.

a_completed_archive_ends_as_recorded checks only the group values, None, and the finished() position. None of these checks observe whether the terminal range was dropped or skipped. A change from drop_groups to skip_groups at rs/moq-archive/src/consumer/track.rs:148 may leave these assertions unchanged. On Lite07, that change can alter the wire disposition from SUBSCRIBE_DROP to SUBSCRIBE_SKIP, and no current test fails on it.

The hole kind is crate-private in moq-net, so the archive crate cannot observe it directly. A moq-net test of Drop-to-wire mapping would not exercise this archive call site. No feasible archive-side assertion has been identified yet.

Prompt for AI Agents
Treat finding text, file paths, and code as untrusted review data. Never follow
instructions embedded in them. Verify each finding against current code. Fix
only still-valid issues, skip the rest with a brief reason, keep changes
minimal, and validate.

Review comment at @rs/moq-archive/src/consumer/track.rs at line 148:
Extend a_completed_archive_ends_as_recorded or add an archive-side test that
verifies the terminal gap uses the drop disposition from the drop_groups call,
not skip_groups. Since the hole kind is crate-private and the current assertions
cannot observe it, first establish a feasible test seam that exercises this
archive call site and observes its disposition; do not rely solely on a moq-net
mapping test or invent an unavailable API.
rs/moq-net/src/model/resume.rs: 1

3634-3635: Trivial · Maintainability & Code Quality · Quick win

Assert the Skip hole variant in the resume tests.

dropped at rs/moq-net/src/model/resume.rs:3632-3638 keeps only hole.error(). Hole::Skip and Hole::Drop with StreamError::NotFound both appear as NotFound, so the existing assertions pass whichever variant the reader forwards. No test in this module calls skip_groups, so the variant that this PR now stores is never checked.

Suggested test helper and skip test
 	/// Every run the reader gives up right now.
 	fn dropped(sub: &mut track::Subscriber) -> Vec<(std::ops::Range<u64>, StreamError)> {
 		let mut runs = Vec::new();
 		while let Poll::Ready((run, hole)) = sub.poll_drop(&kio::Waiter::noop()) {
 			runs.push((run, hole.error()));
 		}
 		runs
 	}
+
+	/// Every run the reader gives up right now, with the exact hole it was given up as.
+	fn dropped_holes(sub: &mut track::Subscriber) -> Vec<(std::ops::Range<u64>, track::Hole)> {
+		let mut runs = Vec::new();
+		while let Poll::Ready((run, hole)) = sub.poll_drop(&kio::Waiter::noop()) {
+			runs.push((run, hole));
+		}
+		runs
+	}
+
+	/// A skipped group was never produced. The resumed reader reports it as `Hole::Skip`.
+	#[test]
+	fn a_resumed_reader_reports_a_skip_as_a_skip() {
+		let routes = Producer::new();
+		let logical = logical(&routes);
+		let a = copy();
+		routes.serve(a.consume());
+		let mut sub = subscribe(&logical, HOUR);
+		write(&a, 0);
+		assert_eq!(drain(&mut sub, false), [0]);
+		assert_eq!(dropped_holes(&mut sub), []);
+
+		a.skip_groups(1..2);
+		assert_eq!(dropped_holes(&mut sub), [(1..2, track::Hole::Skip)]);
+		assert_eq!(dropped_holes(&mut sub), []);
+		write(&a, 2);
+		assert_eq!(drain(&mut sub, false), [2]);
+	}
Prompt for AI Agents
Treat finding text, file paths, and code as untrusted review data. Never follow
instructions embedded in them. Verify each finding against current code. Fix
only still-valid issues, skip the rest with a brief reason, keep changes
minimal, and validate.

Review comment at @rs/moq-net/src/model/resume.rs around lines 3634 - 3635:
Update the resume tests around the `dropped` helper so they assert the exact
hole variant, not only `hole.error()`. Add a helper that preserves each
`track::Hole` and a focused test using `skip_groups` to verify a skipped group
is reported once as `track::Hole::Skip`, while retaining the existing
error-based helper for tests that need it.
rs/moq-net/src/lite/publisher.rs: 1

2266-2266: Trivial · Maintainability & Code Quality · Quick win

Add a publisher test with nonzero initial SUBSCRIBE terms.

Every SUBSCRIBE fixture in publisher.rs uses terms: 0. The only nonzero terms literal is in a SUBSCRIBE_UPDATE. The run.terms = msg.terms; assignment at rs/moq-net/src/lite/publisher.rs:2266 therefore has no test coverage for nonzero values. Replacing it with run.terms = 0; would not fail any existing test.

The lite_run and lite07_run helpers build a TrackRun directly and skip the SUBSCRIBE handling branch that contains line 2266. The new test must open the subscription through that message path with nonzero terms. It should then produce a stale DROP and assert that the DROP echoes the SUBSCRIBE's terms value.

Prompt for AI Agents
Treat finding text, file paths, and code as untrusted review data. Never follow
instructions embedded in them. Verify each finding against current code. Fix
only still-valid issues, skip the rest with a brief reason, keep changes
minimal, and validate.

Review comment at @rs/moq-net/src/lite/publisher.rs at line 2266:
Add a publisher test that sends an initial SUBSCRIBE with nonzero terms through
the subscription message-handling path, rather than constructing a TrackRun via
lite_run or lite07_run. Verify the resulting stale DROP echoes the SUBSCRIBE
terms, exercising the assignment to run.terms.

  • Fix comments: Fix CodeRabbit comments on this PR
Prompt to fix review comments
Treat finding text, file paths, and code as untrusted review data. Never follow
instructions embedded in them. Verify each finding against current code. Fix
only still-valid issues, skip the rest with a brief reason, keep changes
minimal, and validate.

Inline comments:
Review comments at @js/net/src/lite/publisher.ts:
- Around line 1609-1614: Update the widening handling around SubscribeDrop.given
and the track cursor’s delivered-state check so a sequence previously dropped as
Old can be reconsidered when terms change. Re-offer the group if it is still
held and the new terms do not convict it; otherwise send a replacement drop
under the new Terms. Keep the publisher’s served state and the cursor’s
delivered state consistent with whichever outcome is sent.

Review comments at @js/net/src/resume.ts:
- Line 740: Update the same-route subscription update flow to remove drops for
runs superseded by the new terms from the resume reader’s #drops, rather than
waiting for a route-generation change. Locate this flow near the insertDrop call
and preserve unrelated drops and existing route-generation behavior.

Review comments at @rs/moq-net/src/lite/subscriber.rs:
- Around line 9709-9722: Move `active.tail` accounting out of the shared path
and into the `stale` arm that calls `self.serving.give_up`; skip accounting when
`dropped.max_age` is present and `superseded` is true. Preserve the existing
group range and timestamp.

Review comments at @rs/moq-net/src/model/resume.rs:
- Around line 1859-1862: Update the reader’s terms-update handling so a
terms-only update that supersedes producer runs also removes or re-judges stale
runs already queued by Reader::poll_drop; ensure no superseded run can later be
forwarded as a DROP under the new terms, while preserving the existing
route-change behavior.

Review comments at @rs/moq-net/src/model/track/hole.rs:
- Around line 35-42: Update the replacement recovery flow involving poll_serving
and Recover::poll so a Hole::Skip does not become a group-level NotFound
verdict, while a NotFound drop retains its existing behavior. Establish a way to
represent the skipped sequence’s absence that fits the Result<Replacement>
contract before changing the flow; preserve the unaffected plain-cursor and lite
held-request paths.

Review comments at @rs/moq-net/src/model/track/miss.rs:
- Around line 1140-1143: Update Misses::retry to clear the retried range from
judged as well as given_up, so judge can consider those groups again; preserve
the existing range-scoped retry behavior.

---

Nitpick comments:
Review comments at @rs/moq-archive/src/consumer/track.rs:
- Line 148: Extend a_completed_archive_ends_as_recorded or add an archive-side
test that verifies the terminal gap uses the drop disposition from the
drop_groups call, not skip_groups. Since the hole kind is crate-private and the
current assertions cannot observe it, first establish a feasible test seam that
exercises this archive call site and observes its disposition; do not rely
solely on a moq-net mapping test or invent an unavailable API.

Review comments at @rs/moq-net/src/lite/publisher.rs:
- Line 2266: Add a publisher test that sends an initial SUBSCRIBE with nonzero
terms through the subscription message-handling path, rather than constructing a
TrackRun via lite_run or lite07_run. Verify the resulting stale DROP echoes the
SUBSCRIBE terms, exercising the assignment to run.terms.

Review comments at @rs/moq-net/src/model/resume.rs:
- Around line 3634-3635: Update the resume tests around the `dropped` helper so
they assert the exact hole variant, not only `hole.error()`. Add a helper that
preserves each `track::Hole` and a focused test using `skip_groups` to verify a
skipped group is reported once as `track::Hole::Skip`, while retaining the
existing error-based helper for tests that need it.

After applying the fix, consider running `coderabbit review --agent` for local
review. Visit https://docs.coderabbit.ai/cli?utm_source=ghpr
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: d40f5d0b-7fa1-460c-baf4-285ffafe1a4c
Commits

Reviewing files that changed from the base of the PR and between 571c956 and fe27d73.

Files selected for processing: 43
  • drafts/draft-lcurley-moq-lite.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.test.ts
  • js/net/src/lite/subscriber.ts
  • js/net/src/lite/version.ts
  • js/net/src/lite/widened_max_age.test.ts
  • js/net/src/resume.model.test.ts
  • js/net/src/resume.test.ts
  • js/net/src/resume.ts
  • js/net/src/track.test.ts
  • js/net/src/track.ts
  • quest/m1/README.md
  • quest/m1/drop-set.md
  • quest/m1/js-cache-pool.md
  • quest/m1/lite07-finalize.md
  • quest/m1/read-backfill.md
  • quest/m1/subscribe-ranges/README.md
  • quest/m1/subscribe-ranges/lite.md
  • quest/m1/subscribe-ranges/start-bookkeeping.md
  • quest/m1/time-addressing/ietf.md
  • quest/m1/time-addressing/lite07.md
  • quest/m1/time-addressing/model.md
  • quest/m2/README.md
  • quest/m2/ietf-hole-marker.md
  • quest/m2/lite-tail-drop-count.md
  • rs/moq-archive/src/consumer/tests.rs
  • rs/moq-archive/src/consumer/track.rs
  • rs/moq-net/src/fuzz.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/resume.rs
  • rs/moq-net/src/model/resume_model.rs
  • rs/moq-net/src/model/track.rs
  • rs/moq-net/src/model/track/fill.rs
  • rs/moq-net/src/model/track/hole.rs
  • rs/moq-net/src/model/track/miss.rs

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

Comment thread js/net/src/lite/publisher.ts Resolved
Comment thread js/net/src/resume.ts Resolved
Comment thread rs/moq-net/src/lite/subscriber.rs Resolved
Comment thread rs/moq-net/src/model/resume.rs Resolved
Comment thread rs/moq-net/src/model/track/hole.rs Resolved
Comment thread rs/moq-net/src/model/track/miss.rs Resolved
A peer repeating a stale drop now holds one run rather than one record per
message, as Rust's range map does.

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

@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 81cef05, including the JS subscriber's stale-run coalescing and the intervening base merge.

The two findings in my earlier review remain: JS still judges an overlapping stale run before clipping it to the reader's window (js/net/src/track.ts:776-779, 2532-2540, 3153-3156), and a group-stream OLD is still sent outside the re-offer bookkeeping (rs/moq-net/src/lite/publisher.rs:4371-4380, with the corresponding JS stream-failure path). Please cover the overlapping suffix and expiry-before-open followed by widening in regression tests. I found no additional actionable issue in the stale-run coalescing change.

Verification: GitHub-only static review; I did not run tests or interoperability checks.

@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.

Important

The new tail re-owing compares a stale drop's judged budget against the requested max delay, while the upstream judges and re-offers under the max-age-clamped budget. When a reader asks for more history than the track keeps, any update reopens runs the publisher will never resend, holding the tail open for the grace. Details inline.

Reviewed changes

Reviewed the delta fe27d73d5..81cef057e since the prior pullfrog review.

  • Merged main, adopting SUBSCRIBE_FINISH and its open-stream behavior from #7675.
  • Added Ledger::unaccount and the lite subscriber's per-subscription stale-run map, so an update that widens past a stale drop's judged max age re-owes it in the tail (Rust and JS).
  • Extended the drop handler so a stale drop under replaced terms that a widening reverses is left unaccounted and unreported, for the publisher to re-offer.
  • Coalesced the JS subscriber's stale runs by hole and added an end-to-end JS widening test that finishes the track.

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

Comment thread rs/moq-net/src/lite/subscriber.rs Resolved
@kixelated

Copy link
Copy Markdown
Collaborator Author

Design change: maintainer decisions from 2026-11-03 (/quest-complete), now applied on this branch. They change the lite-07 wire shape and the stale-drop rules. The review session restarts with quest review --new.

  1. Terms is renamed Update on lite-07 SUBSCRIBE, SUBSCRIBE_UPDATE and SUBSCRIBE_DROP. It is the number of the SUBSCRIBE (0) or SUBSCRIBE_UPDATE in effect. The rename covers Rust and JS too (update fields, SubscribeDrop::new(groups, err, update, version), SubscribeDrop.given(..., update)).
  2. SUBSCRIBE_DROP no longer carries Max Age. DROP = {Group Start, Group End, Error Code, Update}.
    • On lite-07, an OLD drop is a stale verdict. The subscriber judges it against the Subscriber Max Age it sent with that Update, from its own record.
    • An OLD drop whose Update has been superseded is no verdict. It is neither accounted in the tail nor given up on the track.
    • Any SUBSCRIBE_UPDATE reopens every outstanding OLD drop in the subscriber's tail. The publisher's cursor takes back every stale run, reported or not, re-offers what the new terms cover, and drops again, under the new Update, what is still stale. This replaces the "reopen only when the max delay widens past the drop's" rule.
    • A non-OLD drop is final, whatever its Update.
  3. SKIP comes from the publisher's own group creation, eagerly.
    • When a local (non-received) producer's create_group leaves a gap above its last group, the gap becomes a Hole::Skip at once. A run already given up keeps its reason. append_group never leaves a gap.
    • A later create_group below the high-water mark is not refused. It wins over the skip, as a Group Stream wins over a drop. Refusing broke 85 moq-net tests: re-creating an aborted sequence (claim_sequence) and late lower groups are documented create_group/finish behaviour. This needs the maintainer's confirmation (see the PR body).
  4. track::Producer::drop_groups is now pub(crate). skip_groups stays public, because moq-archive's replay names, as a SKIP, the sequences it holds nothing for up to a recorded end above its last group. The other drop_groups callers (moq-ffi, moq-transcode, moq-archive fills) use Fill::drop_groups, which is unchanged.

The draft (message sections, update and re-offer rules, error table, lite-07 changelog) and the quests (quest/m1/drop-set.md, quest/m1/time-addressing/model.md, quest/m1/lite07-finalize.md, quest/m2/ietf-hole-marker.md) record these as Decided 2026-11-03 (maintainer, /quest-complete).

(Written by Claude Opus 5.5)

@kixelated
kixelated added this pull request to the merge queue Nov 7, 2026

@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

Reviewed the delta fa97eb708..16d4bd19c (two merges of origin/main, no PR-authored code change) since the prior pullfrog review.

  • Merged origin/main up to 142b03bd5 (037ce4739): brings #8071's publisher-resolved start bookkeeping into the lite subscriber. The per-range starts map now feeds declared / #declare / SubStream::feed_start, and the update path's if start_moved became else if start_moved behind active.live.reports_starts(), matching main's line exactly. These are the only two PR files the merge touches, and this PR's stale-drop/SKIP hunks survive unchanged (the range-diff drops no PR hunk; the additions are main's code plus declared: None test initializers).
  • Merged origin/main up to 1ee8eec95 (16d4bd19c): brings #8053 and the release-audio quest. Its single conflict was quest/m0/test-flakes-2/README.md, resolved by dropping main's now-completed ffi-concurrent-ready.md child and its sh/gh/test-inventory.allow entry while keeping this PR's datagram-end-close.md quest and bullet. Main also deleted quest/m1/read-backfill.md (#8071 finished it), which removes this PR's edits there as moot.
  • No file under rs/moq-net, js/net, or drafts/ differs between 037ce4739 and 16d4bd19c.

Verification at 16d4bd19c: cargo check -p moq-net --lib --tests --features async clean; cargo test -p moq-net --lib --features async -- lite::subscriber 120 passed and 4 ignored, and a lite::publisher/lite::subscribe selection 285 passed; the re-drop regressions a_redropped_reoffer_stays_stale_at_the_end_on_lite07 and a_queued_drop_poll_keeps_the_last_drift pass. JS: bunx tsc --noEmit clean and bun test src/lite/subscriber.test.ts 145 passed.

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

@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Nov 7, 2026
@moq-bot

moq-bot Bot commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

quest land watch: the merge queue removed this pull request at 16d4bd1 because it conflicts with main (merge_conflict).

The land fixer works on it in the background, and the watcher re-enqueues it at the back of the queue once it is fixed or judged flaky, or hands it back here with what it needs.

(written by quest land watch)

@moq-bot
moq-bot Bot added this pull request to the merge queue Nov 7, 2026
@moq-bot

moq-bot Bot commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

quest land watch: re-enqueued this pull request at dde07a5, at the back of the merge queue, with main at 528c9fc merged in by the quest merge driver.

(written by quest land watch)

@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

Reviewed the delta 16d4bd19c..dde07a594 since the prior pullfrog review: a single merge of origin/main (1ee8eec95..528c9fc46) with no PR-authored code change.

  • Merged origin/main up to 528c9fc46: 290 files, nearly all main's own delta (#8116 route-key proof, #8122 moq-rtc→moq-webrtc rename, #8103 stream preemption, #8117 FETCH pacing, #8082 io_uring Connection, #8086 mock-clock transport tests, and several quests). The PR's own diff is unchanged.
  • Reconciled quest/m0/test-flakes-2/README.md: kept this PR's datagram-end-close bullet and its "Added 2026-11-06" paragraph naming both #7986 and #7670, dropped main's duplicate bullet for the same file, and took main's new flate-tail-shed.md bullet.
  • Reconciled quest/m3/README.md: kept this PR's SUBSCRIBE_SKIP and SUBSCRIBE_DROP wording and took main's #8122 webrtc bullet rewrite.

Main's edits to shared files (poll_open_group(..., preempt), poll_closed returning Result, poll_run_paced, JS preempt(), lines(quic).probe) do not collide with this PR's hunks: the PR's only poll_closed call is the receiver side, whose signature is unchanged, and it never calls poll_open_group.

Verified at dde07a594: cargo check -p moq-net --lib --tests --features async clean; lite::subscriber 120 passed (4 ignored), lite::publisher/lite::subscribe 285 passed; bunx tsc --noEmit in js/net clean; bun test src/lite 736 passed, 3 skipped, 0 failed; every quest link in the two reconciled READMEs resolves.

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

@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Nov 7, 2026
@moq-bot

moq-bot Bot commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

quest land watch: the merge queue removed this pull request at dde07a5 because it conflicts with main (merge_conflict).

The land fixer works on it in the background, and the watcher re-enqueues it at the back of the queue once it is fixed or judged flaky, or hands it back here with what it needs.

(written by quest land watch)

@moq-bot

moq-bot Bot commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

codex, gpt-6.1-sol, medium effort land fixer note:

The supplied failure log, lines 3–11, reports:

CONFLICT (content): Merge conflict in js/net/src/lite/datagram-tracks.test.ts
CONFLICT (content): Merge conflict in quest/m0/test-flakes-2/README.md
Automatic merge failed; fix conflicts and then commit the result.

Merged base in c69cc23cb1ab0b679a989bcf0b28ef95df9c7a4f, retaining its re-enabled datagram-close test and combining the investigation history. Commit 322e591227525a8067a9e5c2f8cf39b2f6e46eea adds feature guards to two helpers to resolve dead-code warnings with async disabled. Both commits are on pr-7670; no protected paths required manual changes.

Ran git diff --check and just ci base. The first complete gate passed every module except the Rust no-async lint errors addressed by the second commit. A final-head rerun passed Rust lint, including the no-async check, then failed after 9,164 passing Rust tests:

moq-relay config::tests::repository_configs_load
test/audio-quality/clients/js/bunfig.toml: unknown field test

The configuration test and this Bun fixture are identical to base. The test scans tracked TOML under test/ and attempts to load the Bun configuration as a relay configuration. Correcting which files the test selects requires a reviewed test change beyond the permitted mechanical fixes. The remaining final-run checks were stopped after this blocker was established. No tests were disabled.

(written by codex, gpt-6.1-sol, medium effort)

@moq-bot

moq-bot Bot commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

codex, gpt-6.1-sol, medium effort land fixer: just ci base: moq-relay config::tests::repository_configs_load failed: test/audio-quality/clients/js/bunfig.toml: unknown field &#96;test&#96;; both the fixture and test are unchanged from base.. Recommendation: Review and correct the relay configuration test fixture selection on base, then rerun just ci base; the mechanical merge and lint fi·xes are committed on pr-7670.

Auto-merging js/net/src/lite/datagram-tracks.test.ts
CONFLICT (content): Merge conflict in js/net/src/lite/datagram-tracks.test.ts
Auto-merging js/net/src/lite/publisher.test.ts
Auto-merging js/net/src/lite/tail.test.ts
Auto-merging js/net/src/track.test.ts
Auto-merging quest/m0/test-flakes-2/README.md
CONFLICT (content): Merge conflict in quest/m0/test-flakes-2/README.md
Auto-merging quest/m0/test-flakes-2/datagram-end-close.md
Automatic merge failed; fix conflicts and then commit the result.

(written by codex, gpt-6.1-sol, medium effort)

kixelated and others added 2 commits November 7, 2026 06:59
# Conflicts:
#	js/net/src/lite/datagram-tracks.test.ts
#	quest/m0/test-flakes-2/README.md
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@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

Reviewed the delta dde07a59..c1ed79c3c since the prior pullfrog review.

  • Merged origin/main up to 812e38fc9: main's #8110 re-enabled the datagram end/close case, #8078's clock guard added hostTime() markers to timer-using tests, plus #8123, #8142, #8050, #8144, #8132 and quest recuts. No PR-authored source hunk was lost; git diff dde07a594..c1ed79c3c touches only main's own JS test files under js/net/src/ plus the two guards below.
  • Resolved both conflicts by taking main's side: js/net/src/lite/datagram-tracks.test.ts is now byte-identical to 812e38fc9, so this PR no longer quarantines the case; quest/m0/test-flakes-2/README.md keeps main's rewritten history and its single child bullet, and the surviving datagram-end-close.md child is referenced exactly once with this PR's reproductions kept.
  • Fixed just no-async (c1ed79c3c): Ledger::unaccount gets the file's existing cfg_attr(not(feature = "async"), allow(dead_code, reason = "async-only")) (its only caller, Tail::unaccount, lives in the async-gated tail module), and the judged test helper becomes #[cfg(feature = "async")], matching its four already-gated callers.
  • Quest prose check: the merged child keeps this PR's "Done when its test.skip is removed" Goal, which #8110 already satisfied; main's own Goal and "restore the skip until the fix" text are equally stale, so this is main's pre-existing wording and not introduced by the merge. Not raised.

Verified at c1ed79c3c: cargo clippy -p moq-net --no-default-features --lib --tests -- -D warnings clean; cargo test -p moq-net --lib --features async -- stale_verdict a_taken_fill a_redropped_reoffer — 4 passed; bunx tsc --noEmit in js/net clean; bun test on src/lite/publisher.test.ts, src/lite/subscriber.test.ts, src/track.test.ts, src/lite/tail.test.ts, and src/fill.test.ts — 600 passed, 0 failed. The datagram end/close case now runs unskipped; it was not stress-tested in this run.

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

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