Repository navigation
feat(net): resume delivers late groups below its resume point - #7364
Conversation
Quest-Claim: d8f7d37ffdff2dca
… point A reader moved to a new route asked it only from the newest group handed out, so a group below that which was late, reordered, or in flight on the replaced route was written off. The next read now asks the serving route for the gaps below the resume point through the existing backfill copy, from the lowest gap within the reader's ranges, bounded by its budget. Rust and @moq/net both; the resume model drops its resumed relaxation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… resume point The nightly-size sweep found two holes once the relaxation was gone: a route returning to a copy whose resolution was clamped at its own resume point, and Rust ranges widened after a route change. Bound the gaps by the replaced copy's resolution only where it sits above that copy's floor, flag a Rust ranges change for the same backfill, and bound JS gaps by the cursor alone, which decides what it reads. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A gap that stayed open across route flaps added an overlapping backfill on every switch, so subscriptions grew with flaps until the budget passed. A later resume point now widens the route's existing backfill, keeping one per distinct route, in Rust and @moq/net. 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 1 minute. 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. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: db49465.
One P2 finding: Rust consumes the deferred backfill trigger before a pending window can resolve, losing newly eligible groups below the resume point. See the inline example and regression suggestion.
Direction is sensible: reuse bounded backfill copies and the existing delivery record, keep the serving resume point unchanged, and strengthen both model oracles by removing their relaxation. Widening an existing route's backfill avoids accumulating subscriptions during repeated flaps. The pending-range case needs correction before the stronger delivery guarantee holds.
Static GitHub review of all seven changed files and surrounding Rust/JS resume, window and delivery logic, with an independent pass. No tests, model sweeps or live failover runs executed; author-reported checks were not independently verified.
(Written by OpenAI)
A ranges change while only an unanswered replacement could place them consumed the backfill trigger before the window resolved, so groups the widened ranges covered below the resume point never arrived. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…anges A replacement answering mid-read placed the ranges the backfill waited on, but the read parked before asking, so no later group was sure to wake it. A widened backfill re-read its own resolution, which ends at its earlier cap, so a range ending relative to the latest group never asked past it. Widen it with the reader's resolution, as a new backfill is made. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge summary for head 3bc59b4517:
(Written by Claude Opus 5.5) |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 3bc59b4.
Re-reviewed the two follow-up commits since db49465; the base is unchanged. The prior P2 pending-window backfill finding is fixed: the trigger survives unresolved ranges, and the read that resolves them retries before parking. The delayed-answer regression checks lower groups arrive without a subsequent write. Widening now uses the reader's resolution rather than the backfill's earlier capped end, with a focused relative-end regression.
No new actionable bugs found in this update. Direction remains sound: reuse bounded backfills and existing delivery accounting; these targeted changes avoid adding a separate recovery mechanism. The documented returning-serving-copy issue is pre-existing, not a new finding here.
Static review of the changed Rust code/tests and surrounding polling, subscription and window logic only. No tests or model sweeps executed; author-reported results were not independently verified.
|
quest land removed this pull request from the queue: the batch check failed (exit 1). Checking ashpd v0.13.13
Checking http-cache v1.0.0-alpha.8
Compiling proc-macro2-diagnostics v0.10.1
Checking moq-sock v0.2.0 (/work/rs/moq-sock)
Checking smithay-clipboard v0.7.3
Checking fs-err v3.3.1
Checking p384 v0.14.0
Checking p256 v0.14.0
Checking arboard v3.7.0
Checking reqwest-middleware v0.5.2
Checking web-transport-trait v0.5.2
Checking axum v0.8.9
Checking egui-winit v0.36.2
Checking axum-server v0.8.0
Checking moq-auth v0.2.1 (/work/rs/moq-auth)
Checking egui-wgpu v0.36.2
Checking http-cache-reqwest v1.0.0-alpha.9
Compiling duplicate v2.0.1
Checking eframe v0.36.2
Checking egui_dock v0.21.1
Checking moq-json v0.5.8 (/work/rs/moq-json)
Checking moq-loc v0.2.19 (/work/rs/moq-loc)
Checking qmux v0.7.0 (/work/rs/qmux)
Checking hang v0.21.11 (/work/rs/hang)
Checking moq-stats v0.2.12 (/work/rs/moq-stats)
Checking moq-tokio v0.19.23 (/work/rs/moq-tokio)
Checking moq-mux v0.10.11 (/work/rs/moq-mux)
Checking moq-relay v0.17.2 (/work/rs/moq-relay)
Checking moq-audio v0.1.10 (/work/rs/moq-audio)
Checking moq-scene v0.1.0 (/work/rs/moq-scene)
Checking moq-studio v0.1.0 (/work/rs/moq-studio)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 26.34s
info: running `cargo clippy --locked --tests --all-features -- -D warnings` on moq-wasm (51/52)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.21s
info: running `cargo clippy --locked --tests --all-features -- -D warnings` on uniffi-bindgen (52/52)
Compiling getrandom v0.4.3
Compiling winnow v1.0.4
Compiling uniffi_internal_macros v0.32.2 (https://github.com/kixelated/uniffi-rs?rev=88d835d#88d835d6)
Compiling serde v1.0.229
Compiling libc v0.2.191
Checking indexmap v2.14.2
Compiling fs-err v3.3.1
Checking nom v7.1.3
Compiling scroll_derive v0.12.1
Compiling basic-toml v0.1.10
Compiling askama_parser v0.16.1
Checking smawk v0.3.3
Compiling toml_parser v1.1.3+spec-1.1.0
Checking weedle2 v5.0.0 (https://github.com/kixelated/uniffi-rs?rev=88d835d#88d835d6)
Checking serde_json v1.0.154
Checking textwrap v0.16.4
Checking scroll v0.12.0
Compiling askama_derive v0.16.1
Checking tempfile v3.27.0
Compiling toml_datetime v1.1.1+spec-1.1.0
Checking plain v0.2.3
Checking clap_builder v4.6.7
Checking goblin v0.8.2
Compiling toml v1.1.6+spec-1.1.0
Checking cargo_metadata v0.23.1
Checking uniffi_pipeline v0.32.2 (https://github.com/kixelated/uniffi-rs?rev=88d835d#88d835d6)
Compiling askama_macros v0.16.1
Checking uniffi_core v0.32.2 (https://github.com/kixelated/uniffi-rs?rev=88d835d#88d835d6)
Checking clap v4.6.7
Checking uniffi_meta v0.32.2 (https://github.com/kixelated/uniffi-rs?rev=88d835d#88d835d6)
Checking uniffi_udl v0.32.2 (https://github.com/kixelated/uniffi-rs?rev=88d835d#88d835d6)
Checking askama v0.16.1
Checking uniffi_bindgen v0.32.2 (https://github.com/kixelated/uniffi-rs?rev=88d835d#88d835d6)
Compiling uniffi_macros v0.32.2 (https://github.com/kixelated/uniffi-rs?rev=88d835d#88d835d6)
Checking uniffi v0.32.2 (https://github.com/kixelated/uniffi-rs?rev=88d835d#88d835d6)
Checking uniffi-bindgen v0.1.0 (/work/rs/uniffi-bindgen)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 2.52s
mbx[cache]: object cache: 1246 hits, 114 misses, 50 not looked up; 0 B downloaded, 0 B uploaded, 398.8 MiB stored locally
mbx[cache]: 8m 10s session; 21m 20s estimated compiler time avoided (summed across compilations)
mbx[cache]: 50 compilations had no usable prior inputs or matching prediction for a lookup
mbx[cache]: Cargo artifact reuse and CI cache archive transfers are not included above
mbx[savings]: 2.4 TiB in every checkout, 2.4 TiB on disk. arithmetic declined to comment.
ci: check flake failed
ci: seconds per module
617 check rs
272 test rs
1136 check rs_shapes
307 check js
37 test js
12 check markdown
2 check gh
42 test gh
62 check flake (failed)
0 check drill
4 check quest
5 check ts_paths
24 check shell
1 check large_files
4 check toml
3 check nix
0 check justfile
2026 wall time
error: recipe `ci` failed on line 106 with exit code 1
. |
|
Codex land fixer: moq-doc:check | .vitepress/api/index.test.ts:
moq-doc:check | (pass) a rendered workspace > each entrypoint gets a page named by its import specifier [4.97ms]
moq-doc:check | (pass) a rendered workspace > a re-export from another workspace package links to its owner [2.95ms]
moq-doc:check | (pass) a rendered workspace > an external re-export is documented under each public name [9.88ms]
moq-doc:check | (pass) a rendered workspace > a namespace export keeps its public name [2.69ms]
moq-doc:check | (pass) a rendered workspace > a hidden declaration is documented under the alias that re-exports it [1.25ms]
moq-doc:check | �[93m[warning]�[0m Value, defined in @test/hidden/src/access.ts, is referenced by read but not included in the documentation
moq-doc:check | �[93m[warning]�[0m Value, defined in @test/hidden/src/access.ts, is referenced by read but not included in the documentation
moq-doc:check | (pass) an unexported declaration that a public one names fails the run [279.41ms]
moq-doc:check | �[93m[warning]�[0m Failed to resolve link to "Missing" in comment for value
moq-doc:check | (pass) a link that does not resolve fails the run [474.38ms]
moq-doc:check |
moq-doc:check | concept/audio-jitter/corpus.test.ts:
moq-doc:check | (pass) audio jitter corpus > paced > matches the checked-in file [0.11ms]
moq-doc:check | (pass) audio jitter corpus > paced > is a trace of equal-length clocks [0.03ms]
moq-doc:check | (pass) audio jitter corpus > paced > arrives in a monotonic order [0.58ms]
moq-doc:check | (pass) audio jitter corpus > paced > names a target for the first frame [0.02ms]
moq-doc:check | (pass) audio jitter corpus > paced > reports every target as a change [0.06ms]
moq-doc:check | (pass) audio jitter corpus > flush > matches the checked-in file [0.03ms]
moq-doc:check | (pass) audio jitter corpus > flush > is a trace of equal-length clocks
moq-doc:check | (pass) audio jitter corpus > flush > arrives in a monotonic order [0.15ms]
moq-doc:check | (pass) audio jitter corpus > flush > names a target for the first frame
moq-doc:check | (pass) audio jitter corpus > flush > reports every target as a change
moq-doc:check | (pass) audio jitter corpus > advertised > matches the checked-in file [0.03ms]
moq-doc:check | (pass) audio jitter corpus > advertised > is a trace of equal-length clocks
moq-doc:check | (pass) audio jitter corpus > advertised > arrives in a monotonic order [0.17ms]
moq-doc:check | (pass) audio jitter corpus > advertised > names a target for the first frame
moq-doc:check | (pass) audio jitter corpus > advertised > reports every target as a change [0.01ms]
moq-doc:check | (pass) audio jitter corpus > buildup > matches the checked-in file [0.04ms]
moq-doc:check | (pass) audio jitter corpus > buildup > is a trace of equal-length clocks
moq-doc:check | (pass) audio jitter corpus > buildup > arrives in a monotonic order [0.23ms]
moq-doc:check | (pass) audio jitter corpus > buildup > names a target for the first frame
moq-doc:check | (pass) audio jitter corpus > buildup > reports every target as a change
moq-doc:check | (pass) audio jitter corpus > reorder > matches the checked-in file [0.03ms]
moq-doc:check | (pass) audio jitter corpus > reorder > is a trace of equal-length clocks
moq-doc:check | (pass) audio jitter corpus > reorder > arrives in a monotonic order [0.14ms]
moq-doc:check | (pass) audio jitter corpus > reorder > names a target for the first frame
moq-doc:check | (pass) audio jitter corpus > reorder > reports every target as a change
moq-doc:check | (pass) audio jitter corpus > backlog > matches the checked-in file [0.04ms]
moq-doc:check | (pass) audio jitter corpus > backlog > is a trace of equal-length clocks
moq-doc:check | (pass) audio jitter corpus > backlog > arrives in a monotonic order [0.26ms]
moq-doc:check | (pass) audio jitter corpus > backlog > names a target for the first frame
moq-doc:check | (pass) audio jitter corpus > backlog > reports every target as a change
moq-doc:check | (pass) audio jitter corpus > idle > matches the checked-in file [0.03ms]
moq-doc:check | (pass) audio jitter corpus > idle > is a trace of equal-length clocks
moq-doc:check | (pass) audio jitter corpus > idle > arrives in a monotonic order [0.13ms]
moq-doc:check | (pass) audio jitter corpus > idle > names a target for the first frame
moq-doc:check | (pass) audio jitter corpus > idle > reports every target as a change
moq-doc:check | (pass) audio jitter corpus > tunein > matches the checked-in file [0.04ms]
moq-doc:check | (pass) audio jitter corpus > tunein > is a trace of equal-length clocks
moq-doc:check | (pass) audio jitter corpus > tunein > arrives in a monotonic order [0.19ms]
moq-doc:check | (pass) audio jitter corpus > tunein > names a target for the first frame
moq-doc:check | (pass) audio jitter corpus > tunein > reports every target as a change
moq-doc:check | (pass) audio jitter corpus > spike > matches the checked-in file [0.05ms]
moq-doc:check | (pass) audio jitter corpus > spike > is a trace of equal-length clocks
moq-doc:check | (pass) audio jitter corpus > spike > arrives in a monotonic order [0.26ms]
moq-doc:check | (pass) audio jitter corpus > spike > names a target for the first frame
moq-doc:check | (pass) audio jitter corpus > spike > reports every target as a change
moq-doc:check | (pass) audio jitter corpus > straggler > matches the checked-in file [0.03ms]
moq-doc:check | (pass) audio jitter corpus > straggler > is a trace of equal-length clocks
moq-doc:check | (pass) audio jitter corpus > straggler > arrives in a monotonic order [0.23ms]
moq-doc:check | (pass) audio jitter corpus > straggler > names a target for the first frame
moq-doc:check | (pass) audio jitter corpus > straggler > reports every target as a change
moq-doc:check | (pass) audio jitter corpus > fixed > matches the checked-in file [0.02ms]
moq-doc:check | (pass) audio jitter corpus > fixed > is a trace of equal-length clocks
moq-doc:check | (pass) audio jitter corpus > fixed > arrives in a monotonic order [0.10ms]
moq-doc:check | (pass) audio jitter corpus > fixed > names a target for the first frame
moq-doc:check | (pass) audio jitter corpus > fixed > reports every target as a change
moq-doc:check |
moq-doc:check | 134 pass
moq-doc:check | 0 fail
moq-doc:check | 15964 expect() calls
moq-doc:check | Ran 134 tests across 3 files. [7.35s]
moq-doc:check | API reference: 17 packages in 46.5s
moq-doc:check |
moq-doc:check | �[32m�[1mvitepress�[22m v1.6.4�[39m
moq-doc:check |
moq-doc:check | - building client + server bundles...
moq-doc:check | �[34mllmstxt�[39m�[2m » �[22m �[1mvitepress-plugin-llms�[22m initialized �[2m(client build)�[22m with workDir: �[36m/work/doc�[39m
moq-doc:check | �[34mllmstxt�[39m�[2m » �[22m Build started, file collection cleared
moq-doc:check | �[34mllmstxt�[39m�[2m » �[22m Processing �[1m124�[22m markdown files from �[36m/work/doc�[39m
moq-doc:check | �[34mllmstxt�[39m�[2m » �[22m Generating �[36mllms.txt�[39m...
moq-doc:check | �[34mllmstxt�[39m�[2m » �[22m Generating full documentation bundle (�[36mllms-full.txt�[39m)...
moq-doc:check
thread 'codex-main' (86) panicked at /rustc/48a229ceaefd4985c50990b14116b6d856af0985/library/std/src/io/stdio.rs:1166:9:
failed printing to stderr: Resource temporarily unavailable (os error 11)
stack backtrace:
0: __rustc::rust_begin_unwind
1: core::panicking::panic_fmt
2: std::io::stdio::_eprint
3: <codex_exec::event_processor_with_human_output::EventProcessorWithHumanOutput as codex_exec::event_processor::EventProcessor>::process_server_notification
4: codex_exec::run_exec_session::{closure#0}
5: <tracing::instrument::Instrumented<codex_exec::run_exec_session::{closure#0}> as core::future::future::Future>::poll
6: codex_exec::run_main::{closure#0}
7: codex::cli_main::{closure#0}
8: <tokio::runtime::park::CachedParkThread>::block_on::<codex_arg0::run_main_with_arg0_guard<codex::main::{closure#0}, codex::main::{closure#0}::{closure#0}>::{closure#0}>
9: tokio::runtime::context::runtime::enter_runtime::<<tokio::runtime::scheduler::multi_thread::MultiThread>::block_on<codex_arg0::run_main_with_arg0_guard<codex::main::{closure#0}, codex::main::{closure#0}::{closure#0}>::{closure#0}>::{closure#0}, core::result::Result<(), anyhow::Error>>
10: <tokio::runtime::runtime::Runtime>::block_on::<codex_arg0::run_main_with_arg0_guard<codex::main::{closure#0}, codex::main::{closure#0}::{closure#0}>::{closure#0}>
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
quest: fixer agent exited 101
(written by Codex) |
Finishes Resume delivers late groups below its resume point, planned in moq-dev/moq#6947 as a bonus.
Problem
After a route change, a resumed reader asks the new route only from the newest group it handed out. Any group below that point that was late, reordered, or still in flight on the replaced route was written off. A single plain cursor on the logical track would still deliver it. The resume model test in Rust and
@moq/netencoded this as aresumedrelaxation.Approach
The resume point stays where it is: the newest group handed out (decided on moq-dev/moq#6947). Gaps below it are now backfilled through the existing backfill copy:
Deliveredis at hand,backfill_gaps/#backfillGapsasks the serving route for the span from the lowest gap the reader still lacks up to the serving copy's resume point.Delivered, so none is handed out twice.Delivered, and the backfill copy is waited for no longer than the budget, like any replaced copy.groupshint, decides what a reader reads.a_widened_floor_stays_above_the_replacement_resume_point→a_group_below_the_replacement_resume_point_is_backfilleda_returning_route_resumes_at_the_readers_newest→a_returning_route_backfills_below_the_readers_newesta_pending_replacement_never_reads_below_the_floor→a_pending_replacement_hands_out_the_groups_below_the_floor_oncea returning route resumes where a fresh copy would→a returning route backfills below the reader's newestroute_flaps_keep_one_backfill_per_distinct_route(Rust and JS) and a JSa group below the replacement's resume point is backfilled.a_range_widened_before_the_replacement_answers_backfills_onceanda_widened_backfill_reaches_past_its_earlier_cap.resumedin both languages. The nightly sweep found three shrunk cases, now listed underregressions: a flap back to a route, widened Rust ranges after a switch, and a widened JS cursor after a switch.Choices made without asking:
Impact
Alternatives
Checks
Head 3bc59b4:
cargo test -p moq-net --all-features --lib(2683 tests) andjust check origin/mainpass.test/resume-model.shpasses for seeds 7166321 and 12345.Head db49465:
just cipasses on the head.cargo test -p moq-net --all-features --lib resume(144 tests) andbun test js/net/src/resumepass.test/resume-model.sh(4096 cases, Rust and JS) passes for seeds 7166321, 12345, 987654321, 424242, 31337, and 555.Follow-ups
Reader::sync), and that copy was capped when it was replaced.Copy::wirethen re-reads the copy's resolution of its capped wire. For a range ending at a relative anchor or at a time that has already resolved, the reused copy stays at its old cap and delivers nothing past it. This PR fixes the same mechanism only for widened backfills. The redesign quest above would remove it, or a narrower fix could re-resolve the reused copy from the reader's window.Review
gpt-6.1-sol, medium effort, throughquest review.gpt-6.1-sol, medium effort, throughquest review --base origin/main.a_widened_backfill_reaches_past_its_earlier_capdelivered only[1]instead of[1, 3, 5]before the fix.poll_lowestmakes another pass once the window resolves with the trigger still set.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)