diff --git a/quest/m1/README.md b/quest/m1/README.md index fbbe8cef3d..3e40530157 100644 --- a/quest/m1/README.md +++ b/quest/m1/README.md @@ -164,6 +164,7 @@ above them. - [Draft 14-16 updates](/quest/m1/ietf-legacy-updates.md) - Rust and JS apply and answer moq-transport 14-16 request updates without ending or leaking the request - [Tail arrivals](/quest/m1/tail-arrivals.md) - a track judges its pending tail from recorded arrivals, not a cache scan, and errors a reader whose tail ends short - [Resume reorder](/quest/m1/resume-reorder.md) - a group whose stream header arrives after its successor's still resumes on failover; only a dropped or long-gone group fails +- [Refuse rewinds](/quest/m1/refuse-rewinds.md) - a timed track refuses a group whose first frame is earlier than the previous group's, in Rust and js/net, on every hop - [Feed fetch index](/quest/m1/feed-fetch-index.md) - held feed fetches wake per group and are capped per peer session, on main then release - [JS pending tail](/quest/m1/js-pending-tail.md) - a JS reader holds for a track's pending tail like Rust - [Group demand after accept](/quest/m1/group-demand-accept.md) - an accepted group request's demand ends cleanly instead of failing with NotFound diff --git a/quest/m1/refuse-rewinds.md b/quest/m1/refuse-rewinds.md new file mode 100644 index 0000000000..aa9110f902 --- /dev/null +++ b/quest/m1/refuse-rewinds.md @@ -0,0 +1,140 @@ +# [S] A track refuses a timestamp that rewinds + +## Goal + +A timed track never holds a group whose first frame is earlier than the +previous group's, in Rust and js/net. Writing one fails loudly instead of +being cached and served. Untimed tracks are exempt. + +## Plan + +Decided by the maintainer on 2026-10-13 +([#5446](https://github.com/moq-dev/moq/pull/5446)): never support timestamp +rewinds. Today the model accepts them, and its expiry logic works around +them. + +Where a group's start is stamped: + +- Rust: `GroupState::stamp` (`rs/moq-net/src/model/group.rs`) sets a + group's start from its first frame. Every write path calls it: + `group::Producer::write_frame`, `write_frames`, and `open_frame` behind + `create_frame` and `create_frame_owned`, which lite and IETF ingress use. + The track keeps groups by sequence in its `lookup` map + (`rs/moq-net/src/model/track.rs`). +- JS: `state.timestamp ??= frame.timestamp` in `js/net/src/group.ts` stamps + the start; the track keeps groups by sequence in its sorted `timeline` + (`js/net/src/track.ts`). + +The rule: when a group's first frame lands, its start must be at or after +the nearest cached, stamped, non-aborted group below it, and at or before +the nearest one above it. Both sides are checked because groups can be +created and received out of sequence order. An equal start is allowed, as +moq-mux's `container::Producer` already allows a keyframe at the previous +group's start. The write fails without writing: every Rust write path +commits the frame (or installs the partial one) before `stamp` today, so +the check runs before any cache, frame, or index change, and the start is +stamped only once it passes. A session receiving such a group aborts the +track with that error and keeps the session. The ingress handlers must +route it there: today Rust's IETF subscriber (`rs/moq-net/src/ietf/subscriber.rs`) +sends `TimestampMismatch` down the generic branch that aborts only the +group, and js/net's (`js/net/src/ietf/subscriber.ts`) closes the track only +for `ProtocolViolation` or `MalformedTrack`. Check the lite subscribers the +same way, and test that the track, not just the group, closes. + +Cases the rule must cover (from review): + +- Only an original start counts. A group received from a later frame + (`start_at(hdr.frame_start)` in `rs/moq-net/src/lite/subscriber.rs`, or a + partial fetch) stamps its first *received* frame, which can sit above a + valid successor's start, since moq-mux lets a keyframe overlap the previous + group's last frame. A group whose first held frame is not index 0 has no + known start: it is neither checked nor used as a neighbor's bound. +- The neighbor check and the first stamp are one atomic step under the + shared track state, taken in the existing track-before-group lock order. + Groups are written on independent streams, so two unstamped neighbors + checked under only their own locks could each accept a rewind. +- A JS `GroupProducer` can hold frames before `track.writeGroup` attaches + it, so its start is stamped with no timeline to check. Check an already + started group when it is attached, before it is cached or fanned out. +- Eviction must not drop the bound. An unstamped group whose stamped + predecessor expires (Rust's `evict_expired_scan`, JS's `#prune`) would + otherwise have no lower neighbor and accept a rewind. The track keeps the + sequence and original start of the highest-sequence group it evicted. A + group above it is checked against both its cached neighbors and that + watermark, never one in place of the other, since eviction is not a + sequence prefix (each recently written group is retained on its own). One + entry, so no history grows with the cache. +- A late or refetched group at or below that watermark (JS `writeGroup` + accepts long-evicted sequences; Rust takes older groups after `finish`) + is checked against its surviving cached neighbors plus the watermark as an + upper bound, but its lower bound may be gone. Open, for the implementer to + confirm with the maintainer. Recommended: accept it unverified, keep it out + of every other group's bounds and out of timestamp-driven expiry, so + backfill below the watermark keeps working (fetched backfill is already + served by sequence only). Alternative: refuse a late group whose lower + bound was evicted, which keeps the Goal absolute but refuses legitimate + backfill of evicted history. + +The check costs one ordered neighbor lookup per group, not per frame. The +existing `group_write_frames` benchmark writes into an otherwise empty +track, so add a row that stamps groups swept over the retained group count, +where a scan of `lookup` would show as a slope. + +Recommended: every track producer refuses, local or received. The check +sits where the start is stamped, which publishers, relays, and Rust and JS +subscribers all share, so one rule covers every hop. A buggy or foreign +publisher (including a moq-transport peer) fails at the first hop instead +of being forwarded, and every hop's expiry and delivery logic (`reach`, the +drift budget, #5446's delivered floor) can rely on starts never falling. +Alternative: refuse only local writes (`append_group`, `create_group`, and +the JS equivalents) and let received groups through. It is cheaper on the +forwarding path, but relays forward a peer's rewind and every hop must still +tolerate one. + +Error: recommended, reuse `TimestampMismatch` (stream code 0x39 on the wire) +and broaden its doc, in Rust, js/net, and the code's description in +`drafts/draft-lcurley-moq-lite.md`, to "a frame's timestamp doesn't fit its +track". No new code, no new public variant. Alternative: a distinct +`TimestampRewind` error with its own stream code, a wire addition that +needs a draft entry. + +Reconcile the docs: + +- `TrackState::reach` in track.rs and `#reach` in js/net's track.ts say + "timestamps need not rise with sequence (a rewind reorders them)". Keep the + immediate-successor rule and drop the rewind rationale: the bound is the + immediate servable successor's start, unbounded while that successor is + unstamped. Tightening `reach` to the first stamped group above, which the + refusal makes safe, is out of scope; suggest a follow-up if a stall shows + it matters. +- Delete tests whose premise is a rewind, such as + `reach_follows_the_immediate_successor_not_a_later_rewind`. +- The container layers already refuse rewinds (moq-mux's + `container::Producer` returns `TimestampRewind`; the Rust and js/hang + container consumers abort one). Keep all of them: they floor every frame + against the previous group's start, while the model checks only a group's + first frame, and a foreign wire publisher bypasses the producer's floor. +- If #5446 lands first, update its note that late-group delivery after a + rewind is unspecified. + +Regression tests in both languages: a timed track refuses a group whose +first frame rewinds, whether appended, created out of order, received +through the streamed-frame path, written after its predecessor was evicted +(including a delayed stamp above the watermark while an older group +survives a non-prefix eviction), or (JS) written before `writeGroup`; an +equal start is accepted; a resumed partial group of a valid overlapping +pair is accepted; a late group below the watermark is refused against a +surviving cached neighbor, follows the chosen rule otherwise, and never +causes a later refetch to be refused; an untimed track accepts groups in +any order. + +Public API: write paths can now fail with `TimestampMismatch` on a rewind. +Wire: none if `TimestampMismatch` is reused; the draft's description of +code 0x39 broadens. + +## Related + +- [#5446](https://github.com/moq-dev/moq/pull/5446) - where the maintainer decided rewinds are never supported +- [JS track handover](/quest/m1/js-group-handover.md) - mirrors Rust's resume contract, which relies on starts never falling +- [Same-epoch importers](/quest/m1/hop-aligned-import.md) - importers already refuse a rewind at the container layer +- [Epoch anchor](/quest/m1/epoch-anchor.md) - a rewind stays fatal to an importer, which restarts under a successor epoch diff --git a/quest/m1/test-flakes-2/README.md b/quest/m1/test-flakes-2/README.md index 74eea9ef3a..6b6e95d71c 100644 --- a/quest/m1/test-flakes-2/README.md +++ b/quest/m1/test-flakes-2/README.md @@ -34,7 +34,7 @@ Public API: none. Wire: none. - [Interop close code](/quest/m1/test-flakes-2/interop-close-code.md) - the browser close-code test reliably sees `unauthorized` for a refused session - [Handshake ACK](/quest/m1/test-flakes-2/handshake-ack.md) - the impaired cluster drill's connect survives a lost handshake ACK -- [Auth refresh expiry](/quest/m1/test-flakes-2/auth-refresh-expiry.md) - the auth refresh outage test measures the grant's life on one clock +- [Auth refresh mocks time](/quest/m1/test-flakes-2/auth-refresh-mock-time.md) - moq-relay's auth refresh and fixed-lease tests measure a grant's life on one mocked clock ## Related diff --git a/quest/m1/test-flakes-2/auth-refresh-expiry.md b/quest/m1/test-flakes-2/auth-refresh-expiry.md deleted file mode 100644 index 4f73db4361..0000000000 --- a/quest/m1/test-flakes-2/auth-refresh-expiry.md +++ /dev/null @@ -1,27 +0,0 @@ -# [XS] Auth refresh measures a grant's life on one clock - -## Goal - -moq-relay's `an_outage_refuses_transiently_and_never_extends_the_expiry` -(`rs/moq-relay/src/auth/refresh.rs`) passes under a loaded `just check`, -fixed at its cause. - -## Plan - -Seen 2026-10-13 on #5446's first `just check`: the session lived 28.862 s -against the test's 29 s floor, then the test passed alone and on rerun. - -The test's `grant()` stamps the expiry with `SystemTime::now() + 30s`, but -`Grant::deadline()` (`rs/moq-auth/src/grant.rs`) converts the remaining wall -time onto Tokio's paused clock only when the lease snapshots it. Wall time -keeps running under load while the paused clock does not, so a 1.14 s -scheduling delay before the snapshot leaves 28.86 s on the Tokio clock. -Moving `start` earlier does not fix that; `sessions()` uses an in-memory -duplex, so no real socket is involved. - -Measure against one clock: assert the session ends at the deadline the lease -captured, or build the grant's expiry from the moment that deadline is -taken, keeping the bounds. Reproduce first by delaying wall time between -`grant()` and admission. - -Public API: none. Wire: none. diff --git a/quest/m1/test-flakes-2/auth-refresh-mock-time.md b/quest/m1/test-flakes-2/auth-refresh-mock-time.md new file mode 100644 index 0000000000..604ab03181 --- /dev/null +++ b/quest/m1/test-flakes-2/auth-refresh-mock-time.md @@ -0,0 +1,62 @@ +# [XS] moq-relay auth refresh tests mock time + +## Goal + +The moq-relay auth tests that measure a grant's lifetime pass under any +load, because they read one mocked clock instead of the host's wall clock: + +- `an_outage_refuses_transiently_and_never_extends_the_expiry` and + `renewal_outlives_the_initial_credential_and_withdrawal_ends_only_its_grant` + (`rs/moq-relay/src/auth/refresh.rs`) +- `a_fixed_lease_still_expires` and + `an_outage_keeps_the_session_until_expires` + (`rs/moq-relay/tests/auth_lifetime.rs`) + +## Plan + +All three failed under load on 2026-10-13 (seen on #5148, #5462, and +#5446) and passed alone. One run reported the session outliving its grant +at 28.5 s against the outage test's 29 s floor. AGENTS.md requires unit +tests to mock time. + +Cause: a grant's `expires` is `SystemTime::now() + N`, but +`Grant::deadline()` (`rs/moq-auth/src/grant.rs`) converts the remaining wall +time onto Tokio's clock only when the lease snapshots it. The refresh tests +already run on paused Tokio time, which stops while wall time keeps running, +so a scheduling delay before the snapshot shortens the grant on the Tokio +clock. `a_fixed_lease_still_expires` runs on real sockets and real time with +a 1 s grant, so a slow connect and round trip races the expiry. + +Decided (maintainer, 2026-10-13): mock the clock so the tests are +deterministic, rather than widen a bound. + +- Read the grant's wall time through the controlled-clock crate (moq-clock + in [#5465](https://github.com/moq-dev/moq/pull/5465), planned as + moq-time) in both `Grant::deadline()` and `Grant::validate()` + (`rs/moq-auth/src/grant.rs`), which each compare `expires` with + `SystemTime::now()` today, and the tests stamp `expires` from its simulated + clock, which moves wall and monotonic time together. Nothing reads the host + clock. +- Paused Tokio time alone is not enough (review on #5474): it does not pause + `SystemTime`, so under load a short grant can still expire before admission, + and bracketing the bounds the way `an_outage_keeps_the_session_until_expires` + does cannot stop that. That test also unwraps a wall expiry that may + already be past, so it moves onto the mocked clock here too. Hence the + crate is Required, rather than a paused-Tokio fallback. +- `a_fixed_lease_still_expires` moves off real sockets onto + `memory_sessions()` and `moq_relay::supervise` under the same clock. If no + other real-socket test proves the tcp and ws paths hand their lease to + `supervise`, keep one real-socket case that asserts only the close, under a + watchdog. + +Reproduce first by delaying wall time between `grant()` and admission. + +Public API: none. Wire: none. + +## Required + +- [The controlled-clock crate](/quest/m1/time/crate.md) - the simulated clock that moves wall and monotonic time together + +## Related + +- [Controlled time](/quest/m1/time/README.md) - the systemic fix: every crate on one controlled clock, and relay tests on the sim through [the relay seam](/quest/m1/time/relay-seam.md)