Repository navigation
quest: plan the passive-reader spin, retry intervals, and a real-clock test guard - #8043
Conversation
…t wait on it Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou'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 17 minutes. View limit details
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. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head: 631b5fe282582c7608239e452b03ae0d55117508.
No actionable findings in this planning-only change. I read all 18 changed documents and checked the key premises against the referenced source: the lite Live/Shared passive-reader loop, the real-clock copy_fill wait, JS timer cancellation and lint coverage, and the overload/refusal forwarding paths.
Direction: fixing the passive-reader state transition and enforcing deterministic tests at the shared clock layer addresses the causes rather than extending waits. The Rust guard remains a feasibility spike, as planned.
Limits: static GitHub review only. I did not execute tests, quest check, or the markdown checks, reproduce the reported flake, or independently recount the full audit. This is a review of the plans, not validation of their future implementations.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The lite spin plan parks on a None priority without waking when the last passive reader leaves, so the serve would stop burning a core but never cancel the upstream subscription. The rest are inventory corrections, inline.
Reviewed changes
- Lite passive-reader spin — a new m0 quest plans to stop
DatagramState::Live/Sharedbusy-looping when only passive readers remain, by tracking the last priority asOption<i8>. - lite-05 copy-fill flake — a new test-flakes-2 child plans to replace the 200 ms
idle()wait with the file'sManualclock or an awaited event. - Retry intervals — two m1 quests plan to send #7993's may-pass interval on an over-limit PUBLISH refusal and to forward an upstream refusal's interval verbatim.
- Clock guard epic — a new m0 epic plans a JS test preload that fails a test whose host-clock timer fires, a Rust spike of the same guard with a grep fallback, and restoring the mock-time rule to
REVIEW.md. - Mock-clock burn-down — a new m1 epic with seven area children covering about 300 tests, plus the m0/m1 README entries for all of the above.
I re-ran both claimed checks at 631b5fe: quest check reports "605 documents ok" and sh/markdown.sh check is clean. The lite Live/Shared spin, the dropped upstream retry_interval, the EXCESSIVE_LOAD refusal, and the #7761 move of the mock-time rule all match the source.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
| - Track the last priority as `Option<i8>`, as the IETF subscriber does, so a | ||
| `None` priority parks on the waiter instead of re-polling. The fix lands | ||
| for both `Live` and `Shared`. |
There was a problem hiding this comment.
This mechanism drops the close_unused() path. With only passive readers left the aggregate priority is None, so poll_priority(waiter, None) stays Pending when the last reader leaves and nothing re-checks whether the subscription is unused, which strands the upstream subscription rather than cancelling it — the goal's "or the readers go" half.
Technical details
# Lite `None` priority strands the subscription
## Affected sites
- `quest/m0/lite-passive-spin.md:29` — the plan tracks `Option<i8>` but names no wait for the consumer set emptying.
- `rs/moq-net/src/lite/subscriber.rs:2905` and `:2944` — today the loop only reaches `close_unused()` because `poll_priority(..)` returns `Ready(None)` on every pass.
- `rs/moq-net/src/model/datagram.rs:347-353` — `poll_priority` parks whenever `state.priority() == last`, and `None == None` never becomes false when the last passive consumer leaves.
- `rs/moq-net/src/model/datagram.rs:357-364` — `poll_unused` is ready once the priorities slab empties; nothing in `lite/subscriber.rs` polls it after setup (`:2980` returns `Pending` for `Live`/`Shared`).
- Reference: `rs/moq-net/src/ietf/subscriber/datagrams.rs:57` parks `Requested` on `request.poll_unused(waiter)` and reads `priority()` beside it.
## Required outcome
- With only passive readers, the serve parks without re-polling.
- When the last reader leaves, the track ends with `Error::Cancel`, as `close_unused()` does today.
- A demanding reader that joins still wakes the serve and its priority reaches the wire.
## Suggested approach
- Park on `producer.poll_unused(waiter)` while the tracked priority is `None`, and on `poll_priority(waiter, last)` while it is `Some`, so both wake sources survive the change. Update the plan bullet to name the `poll_unused` wait.| `States::recv_terminal`, which the scan undercounted. | ||
| - moq-cli, 9: `tests/integration/export.rs` 3, `auth_serve.rs` 2, | ||
| `listen.rs`, `src/archive.rs` 2, and `src/main.rs`. | ||
| - kio, 3: `src/clock/sim.rs` 2 and `src/channel.rs`. |
There was a problem hiding this comment.
These three kio entries already meet the epic's goal: src/clock/sim.rs's tests run under sim::run and assert simulated instants, and src/channel.rs's until is a 20 s hang guard on real threads carrying an #[expect(clippy::disallowed_methods, reason = "real threads hang on real time, which no kio clock moves")]. There is nothing to convert, so drop the kio line. The same classifier pulls in other out-of-scope files below.
Technical details
# Inventories include tests the epic's scope excludes
## Affected sites
- `quest/m1/mock-clock-tests/rs-bindings.md:19` — kio: `src/clock/sim.rs` 2 are simulator tests (`rs/kio/src/clock/sim.rs:831`, `:848` run inside `run(async { .. })`), and `src/channel.rs` is the real-thread `until` hang guard (`rs/kio/src/channel.rs:512-516`).
- `quest/m1/mock-clock-tests/rs-transport.md:18-19` — moq-quic: `src/connection/mtud.rs` 4 and `src/tests/characterize/endpoint.rs` 2 are plain `#[test]`s passing explicit `Instant` arguments (e.g. `mtud.rs:544`, `:578`); they take `Instant::now()` as an arbitrary base and never wait.
- `quest/m1/mock-clock-tests/rs-media.md:16` — `src/capture/wgc.rs` and `src/frame/cuda_test.rs` drive a real device or encoder, which the same file's closing paragraph excludes.
## Required outcome
- Each area list names only real-clock negative proofs, settle sleeps, and elapsed-time assertions.
- Tests already on a simulated clock, explicit instants, real threads, or real devices leave the lists (or are marked as left alone).
## Suggested approach
- Re-derive the lists with the classifier restricted to the three agreed categories, then drop the entries above.|
|
||
| As of 2026-11-06, 28 tests: `tests/integration/auth_lifetime.rs` 9, | ||
| `session_revalidate.rs` 4, `public_rules.rs` 3, `goaway_cluster.rs` 2, | ||
| `runtime_uring.rs` 2, `tenant_matrix.rs` 2, `web.rs` 2, and one each in |
There was a problem hiding this comment.
rs/moq-relay/tests/integration/ has no web.rs; the count looks like it came from src/web.rs, whose #[tokio::test]s build TLS configs with no timers (rs/moq-relay/src/web.rs:1700, :1789, the latter a real-socket test the epic exempts as real I/O). Fix the path and re-derive this entry's count.
| each hop; leaving the lossy hop. | ||
| - ✅ m1: additive, no semver break. | ||
|
|
||
| Starts once #7993 lands. Cover it with a two-hop test in Rust, and in JS if |
There was a problem hiding this comment.
#7993 merged into main as 0fe4df8d3 and is already in this PR's base, so this gate is satisfied and now reads as a blocker. quest/m1/retry-interval-overload.md:23 carries the same sentence; drop both, or point at the merged code.
Plans the follow-ups from today's
/quest-completerun, and a guard plus burn-down for tests that wait on the real clock, which the maintainer asked to have audited.Quests
quest/m0/lite-passive-spin.md[S]: a lite datagram subscription left with only passive readers stops busy-looping inrs/moq-net/src/lite/subscriber.rs(from fix(net): a lite-05/06 SUBSCRIBE sends datagrams written since its TRACK_INFO #8000 / feat(net): restore IETF datagram tracks #7959).quest/m0/test-flakes-2/copy-fill-idle.md[XS]: the lite-05 copy-fill test proves no FETCH went out on theManualclock, not with a 200 ms real wait (from feat(net)!: lite-07 SUBSCRIBE_SKIP and budget-qualified stale drops #7670).quest/m1/retry-interval-overload.md[XS]: the over-limit PUBLISH refusal sends feat(net): invite a retry on a REQUEST_ERROR that may pass #7993's may-pass retry interval, not 0.quest/m1/retry-interval-forward.md[S]: a relay forwards an upstream refusal's retry interval verbatim.quest/m0/clock-guard/[M] epic:js.md:@moq/clockfails a test whose real-clock timer fires, with an opt-out.rust.md: a spike of the same guard inkio, with a grep check as the fallback.REVIEW.md.quest/m1/mock-clock-tests/[XL] epic: about 300 tests stop proving nothing happened by waiting, sleeping to settle, or asserting on elapsed time. There's one child per area (js-net, js-rest, rs-relay, rs-transport, rs-media, rs-bindings, go-py).Audit summary
The lints from #7329 ban how a timer is spelled, not the clock it runs on.
kio::clock::sleepand@moq/clock'ssleepare the approved replacements, and they still wait in real time unless a test installs a mocked clock. Nothing makes a mocked clock the default: 610 of 4,369 JS tests and 3,288 of 4,756 Rust async tests opt in. Biome skipstest/**and theBunglobal. The written rule left rootAGENTS.mdin #7761, so online reviewers stopped seeing it. When reviewers did flag a wait (#4039, #3641, #1717, #7845), it usually survived anyway: the fix landed in one language but not its siblings, or swapped one real wait for another (#7845'sidle()).The scan found:
Decisions (maintainer, 2026-11-06)
Lite subscriber spin: goal and milestone?
Copy-fill flake?
Over-limit PUBLISH retry interval?
Forwarding an upstream refusal's interval?
from_codelosslessJS guard?
Bunin biomejust checkRust guard?
Burn-down scope?
Milestone?
Impact
Planning only. Public API: none. Wire: none.
Review
Codex gpt-6.1-sol (medium), round 1 on head 631b5fe against origin/main 4722e76: no findings. Checks:
quest check --base origin/mainandsh/markdown.sh checkpass.(Written by Claude Opus 5.5 with high effort)
🤖 Generated with Claude Code