[kixelated]

Problem

Nightly's Tests (test resume-model) failed two ways:

• JS model (run 42050683248, seed 764723972): reader 0: missing groups 2,3,4,5,6,7,8,9,10. A reader resumed on a new route from group 1. The switch asked a backfill copy for group 0, which the new route never had. That route then finished the track. The backfill copy's own bound (until) is applied as its subscriber's local cap, and group 11 sits past that cap. JS deliberately keeps a capped reader open on a finished track so the cap can rise, so the backfill never drained, the logical track never ended, and the drained serving copy then skipped groups 2..10 when they landed late. Rust's arrival cursor ends on a complete track, so the Rust reader ended cleanly at the finish.
• Rust step (the two newest runs, 42119312487, 42102235726): moq-net no longer compiles its lib tests without async. moq-dev/moq#7607 made async default-off, and the Rust model (#[cfg(all(test, feature = "async"))]) was never selected without it anyway.

The earlier symptom (the track ended cleanly though no route declared an end, seeds 1378751129 and 2002958325) had a separate cause, already fixed by moq-dev/moq#7869. Both seeds pass on main today.

Approach

• js/net/src/resume.ts: a copy bounded where it was replaced counts as drained (done = null) once its track ended, no fill still owes it a group (new internal hooks.awaitsFill, mirroring Rust awaits_fill), and it holds nothing unread under its bound (hooks.spent). This mirrors the Rust arrival cursor. The reader's own cap still holds the read open, because a group under the copy's bound past that cap stays unread.
• The shrunk counterexample is now a regression case in both the JS and Rust models. A unit test covers a backfill that waits on a fill the finished route's handler still owes.
• test/resume-model.sh runs the Rust model with --features async.
• moq-net builds without warnings when async is off: EXPORTER_LABEL and the test helpers that only async tests use are gated to match their callers.

Impact

• Public API: none. Wire: none.

Choices made without asking

• I kept the non-async build cleanup in this PR. It is 15 lines, and it also removes the only error in Nightly's features / Features (rs features) job (unused import: EXPORTER_LABEL). It could move to its own PR under the nightly epic if you'd rather keep this one narrow.

Validation

• MOQ_RESUME_SEED=764723972 just test resume-model: Rust and JS pass (4096 cases).
• Replayed through the full script: nightly seeds 1539493080 and 183243466. Run on the JS model only: seeds 1378751129, 2002958325, 11, 22, 33, 44. All pass.
• The new JS regression case fails without the fix and passes with it.
• cargo clippy -p moq-net with no features, async, and fuzz (lib and tests): no warnings.
• just ci on 80a005a4da: every module passed. Run it unwrapped: wrapping just ci in quest gate deadlocks on test gh, because sh/local.sh takes its own gate slot.
• bun test js/net/src: 3020 pass.

Follow-ups

• Nothing builds moq-net without async on the PR gate, which is how this break reached main unnoticed. Consider having the gate's feature selection (or just rs features) compile moq-net's lib tests with default features when moq-net changes.

Review

Codex gpt-6.1-sol at medium effort, via quest review, head 80a005a4da:

• Round 1 (head 259a916c65): one finding, medium. A bounded backfill copy that is idle while a fill is pending would count as drained and withdraw the fill. Fixed in 80a005a4da with hooks.awaitsFill, plus a unit test that fails without the check.
• Round 2 (head 80a005a4da): no findings.

🤖 Generated with Claude Code

(Written by Claude Opus 5.5)

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 20 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: e77503d7-2cb8-4148-a531-d34c7ce17c30

Commits

Reviewing files that changed from the base of the PR and between 1a6ac47 and 80a005a.

Files selected for processing: 14

• js/net/src/internal.ts
• js/net/src/resume.model.test.ts
• js/net/src/resume.test.ts
• js/net/src/resume.ts
• js/net/src/track.ts
• quest/m0/nightly/README.md
• quest/m0/nightly/resume-model.md
• rs/moq-net/src/model/frame.rs
• rs/moq-net/src/model/group.rs
• rs/moq-net/src/model/resume_model.rs
• rs/moq-net/src/model/track.rs
• rs/moq-net/src/route/mod.rs
• rs/moq-net/src/route/proof.rs
• test/resume-model.sh

• 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

• X
• Mastodon
• Reddit
• LinkedIn

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 80a005a

No actionable findings in the 14-file diff. The bounded-copy drain check in js/net/src/resume.ts:790–797,890–895 requires a completed copy with no unread groups under its replacement bound and preserves an outstanding fill through hooks.awaitsFill. Tracing the subscriber paths confirms that unread groups held back by the reader's own cap still prevent this shortcut. The new pending-fill test and mirrored model regression cover the two key cases. The Rust model's async feature requirement matches test/resume-model.sh, and the additional cfg gates are on internal/test helpers rather than a public API removal.

Overall direction: the fix and targeted regression coverage are sound. Verification limits: GitHub-only static review, including the surrounding drain, fill, retention, feature-selection and test paths. I did not execute tests or independently reproduce the reported seeds. The current head's Check run is still in progress: https://github.com/moq-dev/moq/actions/runs/42201612419. This review does not establish a passing CI result.

[pullfrog]

✅ No new issues found.

Reviewed changes

• A bounded backfill copy drains at the track's end — Reader.tryGroup in js/net/src/resume.ts now sets copy.done = null when a read comes back idle or parked, copyEnd is defined, no fill still owes the copy a group, and nothing under the copy's bound is unread. Only copies bounded by until (backfills, and replaced copies bounded by #boundAtEnd) qualify, so the serving copy — and therefore the reader's own cap raising — is untouched.
• A pending fill keeps the copy open — the new hooks.awaitsFill exposes Subscriber.#fillWait, which #askFills sets for a run a fill still owes and each read clears before judging, so the check is read at the right moment. This is the round 1 finding from the PR's Codex review.
• Regression coverage in both models — the shrunk counterexample (groups 11 and 1 written, slot down, slot end) is added to js/net/src/resume.model.test.ts and rs/moq-net/src/model/resume_model.rs, plus a JS unit test for a backfill waiting on a fill the finished route still owes.
• The Rust resume model now selects itself — test/resume-model.sh passes --features async, without which the #[cfg(all(test, feature = "async"))] model was never compiled.
• moq-net builds cleanly without async — EXPORTER_LABEL, frame::Buffer's async-only methods, group::Consumer::poll_read_frames, track::Producer::evict, and the four async-only test helpers are gated to their callers. All are pub(crate) or test-only, and fuzz = ["async"], so no public surface moves.
• Quest bookkeeping — quest/m0/nightly/resume-model.md retired with its README entry.

I re-ran the important claims rather than only reading them: with MOQ_RESUME_SEED=764723972 the JS model reproduces the nightly failure exactly once the #drained call is removed, and the new unit test fails only when the awaitsFill condition is dropped, so each guard covers its own half. bun test js/net/src/resume.test.ts (137 pass), the JS model on seeds 11, 22, 33, 44, 1539493080 and 183243466, the Rust regressions test, the full Rust model on the failing seed (4096 cases), and cargo check -p moq-net --lib --tests both with and without async all pass here. I did not run just ci, so the check suite remains the authority on the rest.

One residual thought, not a finding: this path sets done = null without setting #settled, so a copy dropped here no longer counts in #end's cross-copy settled clause (it is dropped by #letGo before #end runs). That could in principle tip a later serving-copy error from clean end to failure, but I could not construct it and the model sweep does not hit it.

  ^{｜ View workflow run ｜ Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏}