[kixelated]

Problem

js/net/src/lite/copy_fill.test.ts › copy fill: a yielded group the fill's drop names ends sometimes saw 2 open streams at expect(open.length).toBe(1). That broke main's merge gate (moq-dev/moq#8597, moq-dev/moq#8632). The test was right. The bug is in the code: a received JS copy downloaded the same group twice.

In the test, a live reader starts the copy's feed at group 1, so its START is 1. A history reader then asks for [0, 1). That lowers the demand's floor to 0, and the SUBSCRIBE_UPDATE asks the live subscription for group 0. #feedStart() already says a START only holds while the demand keeps the floor it answered. But Producer.#landing (behind owedRuns/mayLand) still clamped what the feed owes by the stale served.start. So the history reader decided the feed did not owe group 0, and asked a lite-07 fill subscription for it too. The publisher was right to serve group 0 on both subscriptions. Whether the live one's stream opened before the test read the list was a race.

Trace at the moment the reader asked the fill:

ASKMISS 0n 1n {"served":{"start":"1n"},"req":[{"start":{"absolute":"0n"},"end":{"absolute":"1n"}},{"start":{"relative":"0n"}}],"feed":"0n"}

Approach

• #landing now clamps by #feedStart(), the same rule #owes and #fillFloor use. A START bounds the feed only while the demand keeps its floor. The only case that changes is a moved floor; a pending or matching START behaves as before.
• New test fill: a group a lowered floor asks the feed for is not asked of a fill in fill.test.ts. It sets up exactly the state from the trace above, with no session and no timing. It fails every run without the fix.
• In the flaky test, the live reader now reads group 1 through to its FIN before going on. The publisher unlists a stream before it FINs, so the list is settled when the test reads it. These are the exact lines moq-dev/moq#8610 adds, so the two PRs merge cleanly (git merge-tree against fix(moq-net): bound paused groups and fix the yield and resume review findings #8610's head is clean).

Rust needs no change. Its lite-07 copy asks for ordinary fills as ranges on the same SUBSCRIBE (RangeFills in rs/moq-net/src/lite/subscriber/fills.rs), so a misjudged fill can't open a second stream. lite/publisher.rs is correct to serve a group on every subscription whose ranges include it.

Impact

• No public API or wire change.
• Behavior: a JS copy no longer opens a lite-07 fill for groups its lowered demand floor already asks the live feed for. Duplicate downloads of that history go away.

Validation

• Reproduced on unchanged main: 2 failures in 150 runs of the test alone, and 17 in 200 with tracing added, which widened the window.
• After the fix: 0 failures in 300 runs of the test alone, and 0 in 300 runs of the whole copy_fill.test.ts, run 6 at a time to add contention.
• Tracing showed group 0 served on sub 0 and sub 1 before the fix, and on the widened live subscription only after it.
• The new unit test fails without the fix and passes with it.
• bun test src in js/net: 3238 pass, 0 fail. just ci: pass.

Alternatives

• Fix only the test, by waiting for the second stream or resetting every open stream as fix(moq-net): bound paused groups and fix the yield and resume review findings #8610 does. Rejected: the second stream doesn't always open, so there is no event to wait for, and the duplicate download is a real cost.
• Move JS fills onto the copy's own SUBSCRIBE as fill ranges, like Rust's RangeFills. That removes this whole class of duplicates, but it is a large redesign and out of scope for a gate fix (see Follow-ups).

Follow-ups

• Overlap with moq-dev/moq#8610: it loosens this test to toBeGreaterThan(0), preempts every open stream, and says the group "may be served on both the fill and the copy's expanded feed". After this PR, exactly one stream serves it (the live subscription's), so fix(moq-net): bound paused groups and fix the yield and resume review findings #8610 can go back to toBe(1) and drop that comment.
• This test's comments ("ends with the fill's drop naming it", "the publisher drops it on the fill's subscription") now describe the drop on the widened live subscription. I left them alone so the file merges cleanly with fix(moq-net): bound paused groups and fix the yield and resume review findings #8610. Reword them in whichever of the two lands second.
• copy_fill.test.ts's header says JS fills "by run on a SUBSCRIBE of its own, as its Recovery does". In Rust, only stale-drop recovery gets its own SUBSCRIBE; ordinary fills ride the copy's subscription. Aligning JS with that would rule out duplicate streams by construction.
• Likely gap, not yet checked: a live reader with no ranges combined with a history range gave a demand of [{}] in a scratch test, which seems to lose the history floor.

Caused by

• moq-dev/moq#6141 - added the served.start clamp to #mayLand (now #landing) without the stale-START rule #owes already applied. moq-dev/moq#7793 later routed fill judgment through it.

Models

Size: S
Implementer: Claude, claude-opus-5-5, unknown effort

🤖 Generated with Claude Code

(Written by Claude Opus 5.5)

Review

Rewritten by quest review after each round.

• Head: 91ee8ca

• Base: origin/main at 6aa4f9c

• Reviewer: Codex, gpt-6.1-sol, medium effort

• Session: 01a1d520-425f-7542-9545-eae3afaf9028, round 1

• Score: 9/10, The focused fix consistently uses the current feed floor to prevent duplicate fill requests after demand widens, with sound regression coverage, though tests were not run as instructed.

• Rounds: 1; findings 0: fixed 0, refuted 0, open 0

• Findings: none

[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 16 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: 4acd712d-e65f-400c-bd15-96f4ac29fce7

Commits

Reviewing files that changed from the base of the PR and between 6aa4f9c and 91ee8ca.

Files selected for processing: 3

• js/net/src/fill.test.ts
• js/net/src/lite/copy_fill.test.ts
• js/net/src/track.ts

• 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

[pullfrog]

✅ No new issues found.

Reviewed changes

I reviewed the one-commit fix to how a received JS copy decides which groups the live feed still owes, plus its two test changes.

• #landing clamps by #feedStart(): Producer.#landing (behind mayLand, owedRuns and owedGroup) now uses #feedStart() instead of the raw served.start. This matches the rule #owes and #fillFloor already follow: a START bounds the feed only while the demand keeps the floor it answered. So a lowered floor no longer sends a duplicate lite-07 fill for groups the SUBSCRIBE_UPDATE already asked the feed for.
• Deterministic regression test: fill.test.ts rebuilds the exact state from the trace through hooks, with no session and no timing involved.
• Flaky test settles its stream list: in copy_fill.test.ts, the live reader now drains group 1 to its FIN before the history reader starts. These are the same lines #8610 adds.

One side effect isn't mentioned in the description. When the START resolved empty (served.start === null), #landing now clamps to the requested floor, where it used to clamp to 0n. That is the same thing #owes and #fillFloor already do with a null START, so I think it's correct. Just note that "a pending or matching START behaves as before" doesn't fully describe the null case.

  ^{｜ View workflow run ｜ Using claude-opus-5-5 ｜ 𝕏}