[kixelated]

Finishes quest/m2/probe/viewer-followups.md, which this PR deletes along with its link in the probe epic.

Problem

moq-dev/moq#7090 shipped viewer PROBE targets but left three gaps:

• The Reload/pool viewers wiring that carries targets across reconnects had only type checks and the subscriber's wire tests.
• Its fix for targets piling up behind a blocked PROBE writer was untested, since the test setups couldn't stall a stream.
• The Python API reference didn't list the bandwidth types.

Approach

• Stalled PROBE writer, Rust (a_stalled_probe_writer_sends_only_the_latest_target, rs/moq-net/src/lite/subscriber.rs). Uses the in-memory session with stream_buffer: 0, so the stream holds one unread message, and a peer SETUP advertising Increase. The stream type fills the stream and the 1 Mbps target waits in the writer. The test moves the viewer to 2, 3 and 4 Mbps, then the peer reads. It asserts exactly [type, 1 Mbps, 4 Mbps] arrive, as separate writes, and nothing after.
• Stalled PROBE writer, JS (a stalled PROBE writer sends only the latest target once it drains, js/net/src/lite/subscriber.test.ts). The existing probeTargetHarness's stallTarget write can now be released by hand (release()), as well as by the reset the existing test uses. The new test makes the same moves and asserts the peer reads 1 Mbps, then 4 Mbps. Before the FIN it drains pending yields with a manual clock (clock.advance(Nano.zero)): reading a target back only proves its bytes were written, and a writer still yielding the shared write budget would see the FIN reset the stream instead of closing it. The existing target test gets the same barrier.
• Pooled reconnect, JS (pooled viewers send their summed target to an Increase peer across a reconnect, js/net/src/connection/pool.test.ts).
  • increasePeer plays a raw lite-06 peer on the mock transport: it sends a SETUP advertising Increase and records every PROBE target it reads. No JS session advertises Increase until padding, so the test needs a fake peer.
  • Two pooled handles on one URL each watch a viewer (3 and 2 Mbps), and the peer sees 5 Mbps. The server then drops the session.
  • After the reconnect, the new peer reads exactly [5 Mbps]. Closing one viewer then sends 2 Mbps on the new session, so it follows the live handles rather than a snapshot.
• Python docs. py/moq-rs/docs/index.md gains a Bandwidth section, after Subscribing, listing Allocator, Reservation, RateControl, RateTarget, BandwidthConsumer, Viewers and Viewer. Autosummary renders each one's one-line docstring, like the other sections, with a two-line intro like Media's and JSON's.

Impact

None. Tests and docs only; no public API or wire change.

Choices made without asking

• Rust test is a unit test, beside an_ended_probe_clears_the_estimate. It drives RecvBandwidth directly against the memory transport, the "in-memory session" the quest names. The integration harness (MockSession) can't stall a stream.
• One test per language, not per version. The writer logic is version-independent past the Increase gate, so the tests use lite-06, the version the existing target tests use.

Checks

• quest gate nix develop --command just check origin/main on 0ef97f98f2: every selected module passed (py with the Sphinx -W docs build, rs with 10,972 tests, js, biome, gh, quest, markdown and the rest).
• Mutation checks. Each test fails when the behavior it covers is broken, and passes again once the source is restored:
  • Rust: buffering every target without waiting for the writer to drain fails with all four targets in one write (\x05\x80\x0fB@\0\x05\x80\x1e\x84\x80\0... against \x05\x80\x0fB@\0).
  • JS stall: sending every target without the one-write guard fails (expected 4000000, received 2000000).
  • JS pool: dropping viewers from the reload's second dial fails the post-reconnect assertion.
  • JS FIN barrier: forcing Budget.poll to yield on every write, both target tests time out without clock.advance(Nano.zero) before the FIN and pass with it.

Follow-ups

None.

Review

Codex, gpt-6.1-sol, medium effort, quest review --base origin/main, session 01a1b1a0-a237-76e0-bc62-ef60528fe1a1:

• Round 1, head 0ef97f98f222c32ed51df496d7cd5befa4605b5c, base 43fcb958d5: no findings.

Online review threads on dd706f03bd:

• [P2] settle the last target write before FIN: fixed with a manual-clock drain before each FIN. Forcing a budget yield isn't needed, since the drain covers both schedules. Verified by the forced-yield mutation above.
• Pullfrog document RateControl/RateTarget: fixed.

🤖 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 39 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: aaa114bb-586b-4169-a3f8-cf8f0889c193

Commits

Reviewing files that changed from the base of the PR and between 344c2be and 0ef97f9.

Files selected for processing: 6

• js/net/src/connection/pool.test.ts
• js/net/src/lite/subscriber.test.ts
• py/moq-rs/docs/index.md
• quest/m2/probe/README.md
• quest/m2/probe/viewer-followups.md
• rs/moq-net/src/lite/subscriber.rs

• 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]

ℹ️ Minor suggestions only.

Reviewed changes

Reviewed the whole diff: three new test scenarios, a shared JS harness change, and the docs/quest edits. Ran bun test src/connection/pool.test.ts src/lite/subscriber.test.ts in js/net — 170 pass, 0 fail, including all three touched tests. (The Rust suite was not run here: cargo test -p moq-net blocked on a stale build-directory lock in this environment, so I read the test and the memory-transport buffering semantics by hand instead.)

• Stalled writer, Rust — the new unit test relies on stream_buffer: 0 meaning one queued message (matching zero_buffer_holds_one_message), stalls the first target behind the stream type, moves the viewer through 2/3/4 Mbps, and asserts three exact chunks with nothing pending afterwards. This does pin the coalescing behaviour rather than just the pending state.
• Stalled writer, JS — stallTarget now stalls only the first target write and can be released by hand, so the new test covers #sendTarget's single-write guard; the existing reset test still exercises the abort path.
• Pooled reconnect, JS — increasePeer plays a raw lite-06 peer and the test verifies a shared Viewers sum before the drop, the exact target after reconnect, and that closing a viewer follows the live handles.
• Docs and quest — Bandwidth autosummary section added after Subscribing; the finished quest file and its epic link are removed with no remaining references.

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

[kixelated]

Automated review by review (OpenAI)

Reviewed head dd706f03bdee26f8cdffddf0c1ab17c0152f483c against base 0672ad56ce29ae168c7122fe6ece20de08cdc12a.

One P2 finding in the new JS stalled-writer test: its final FIN assumes that receiving target bytes means the sender has finished its write, but the shared write budget can still be yielding. That valid schedule aborts the harness writable and leaves the final EOF assertion waiting indefinitely. The inline comment traces the schedule and suggests synchronizing completion.

Also checked the Rust memory-transport backpressure and exact-chunk assertions, JS intermediate-update/coalescing coverage, existing failure/reset harness paths, shared pooled Viewers ownership across reconnects, the Python exports/docs, and quest retirement. No additional actionable findings in those changes. I read the existing review and have not repeated its documentation suggestion.

Limits: GitHub-only static review. No local commands, test execution, mutation checks, Sphinx build, or quest tooling were run; the scheduling failure above was derived from the source, not reproduced locally. The PR's reported test results are not independent validation by this review.

[pullfrog]

✅ No new issues found.

Reviewed changes

Reviewed the delta since the prior pullfrog review (dd706f0 → 0ef97f9): the test-only FIN settle barrier and the completed Bandwidth docs section. bun test src/lite/subscriber.test.ts (136 pass) and bun test src/connection/pool.test.ts (34 pass) in js/net, including both target tests with the new clock barrier.

• FIN settle barrier — both JS PROBE target tests install a Manual clock and await clock.advance(Nano.zero) before the peer FIN. This drains any pending shared writeBudget yield, so #sendTarget has finished its write before the scope cleanup runs; otherwise the cleanup resets the stream and sent.done() never resolves. I traced the mechanism (Budget.poll → yieldNow → Manual.advance draining until #tasks === 0) and it covers both the yielding and non-yielding schedules.
• Bandwidth docs — RateControl and RateTarget added to the autosummary and the section intro extended to describe them, completing the prior review's suggestion. RateTarget's FFI re-export is documented as a mock, consistent with Grant and the other re-exported types.

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

[kixelated]

Automated review by review (OpenAI)

Follow-up review of head 0ef97f98f222c32ed51df496d7cd5befa4605b5c against base 344c2be000b2e73bbd693ff243ca1d62fd5ff183, comparing the PR changes with previously reviewed head dd706f03bdee26f8cdffddf0c1ab17c0152f483c and separating the merged main changes.

The previous P2 is fixed. Both target tests now install a manual clock and drain pending work with clock.advance(Nano.zero) before delivering FIN. I traced Writer.write, the shared budget, the manual clock's host-task draining, and #sendTarget cleanup: the barrier settles the final write/yield before FIN can trigger the in-flight-write abort path. The stalled-writer test retains its exact 1 Mbps then 4 Mbps assertions and final EOF check.

Also rechecked the current six-file PR diff, the existing stalled/reset harness paths, pooled reconnect assertions, and the added RateControl/RateTarget documentation. No new actionable findings.

Direction: The barrier fixes the test synchronization without changing production behavior or weakening the coalescing/EOF assertions.

Limits: GitHub-only static review. I did not run Bun/Rust tests, the forced-yield mutation, Sphinx, or quest/gate commands; the author's reported results were not independently reproduced. The exact-head Check workflow was still in progress when inspected. This COMMENT is not merge approval.