Repository navigation
fix(net): a fill's OLD under the max age is a stale verdict, not a loss for good - #8551
Merged
Merged
Conversation
Quest-Claim: 192781fc126caec2
…ss for good `Fill::reject` recorded an OLD answer under the publisher's max age as a final drop, which survived new terms. It is now held as a stale verdict under that age, forgotten by new terms, a widening, or the drop set's expiry window. JS range fills also stop recording a subscribe stream's OLD drop as final, matching Rust's `Refusal::answer`. Finishes quest/m1/group-error-cache.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
…g now Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
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 29 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. |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Rust: an OLD answer under the max age becomes a stale verdict.
Inner::rejectrecordsHole::stale(binding_max_age)for the owed groups instead ofHole::drop(Old);old_binds_everyonebecamebinding_max_age, which returnsNonewhen the max age is unknown so that case refuses for now and records nothing.Inner::drop_groupsnow takes the owedRangeSetand aHole, keeping the NotFound and OLD paths on one recording site. - Rust: the readers waiting now are refused as well.
Inner::drop_groupsstill refuses the owed runs throughrefuse, so a bounded reader that the stale hole does not settle gets OLD immediately rather than waiting on a fill that will not return. - JS parity for fills.
FillTrack.drop/fillOps.dropcarry the hole;track.tspasses it through, and becauseProducer.#dropno longer resolves fills on a stale hole,fillOps.droprefuses the pending askers itself before resolving.rejectGroupstakesJudged { budget, everyone }, withundefinednow meaning the max age is unknown rather than "binds everyone". - JS: range fills stop recording an OLD subscribe-stream drop as final.
#fillResponsesnow refuses anything butNotFound, matching RustRefusal::answer. - Tests and quest. Mirrored Rust and JS tests cover the waiting readers (including a bounded one), a later no-wider reader settled without a fill, and
supersede_stalereopening the verdict; the finished quest file is deleted, consistent with the repo's completion convention.
I traced the two implementations against each other: clamp_max_delay(asked, bound) < bound is exactly asked < bound, a stale hole under max_age_bound() settles every reader because each budget clamps to that bound, a Rust refusal stamped with asked: None is the JS no-budget refusal, and Hole::stale reuses the Drop variant so drop-set sizing, billing, expiry, and reaches are unchanged. Locally cargo test -p moq-net --lib --features async passes (3216 tests) and bun test js/net/src passes (3201 tests), with the new JS test stable over 50 reruns.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Nov 12, 2026
Contributor
|
quest land watch: the merge queue removed this pull request at 85397cc because its The land fixer works on it in the background, and the watcher re-enqueues it at the back of the queue once it is fixed or judged flaky, or hands it back here with what it needs.
…_pad
PASS [ 0.047s] ( 1983/12962) moq-gst source::imp::tests::aac_caps_follow_the_description_framing
PASS [ 0.242s] ( 1984/12962) moq-gst source::imp::session_tests::an_unserved_rendition_does_not_claim_the_first_pad_name
PASS [ 0.062s] ( 1985/12962) moq-gst source::imp::tests::an_unrepresentable_timestamp_is_refused
PASS [ 0.123s] ( 1986/12962) moq-gst source::imp::session_tests::stop_inside_another_executor
PASS [ 0.156s] ( 1987/12962) moq-gst source::imp::session_tests::stop_from_a_runtime_worker_does_not_deadlock
PASS [ 0.263s] ( 1988/12962) moq-gst source::imp::session_tests::an_ended_broadcast_resumes_on_the_same_pad
PASS [ 0.080s] ( 1989/12962) moq-gst source::imp::tests::opus_caps_carry_the_stream_layout
PASS [ 0.047s] ( 1990/12962) moq-gst source::imp::tests::opus_warmup_is_clipped_after_the_pre_skip
PASS [ 0.074s] ( 1991/12962) moq-gst source::imp::tests::opus_pre_skip_is_clipped_across_packets
PASS [ 0.156s] ( 1992/12962) moq-gst source::imp::session_tests::stop_from_its_own_reader_does_not_deadlock
PASS [ 0.164s] ( 1993/12962) moq-gst source::imp::session_tests::stop_releases_a_blocked_push
PASS [ 0.214s] ( 1994/12962) moq-gst source::imp::session_tests::paused_to_ready_returns_after_the_connection_closes
PASS [ 0.069s] ( 1995/12962) moq-gst source::imp::tests::the_first_frame_fixes_the_segment_start_and_base
PASS [ 0.183s] ( 1996/12962) moq-gst source::imp::session_tests::stop_returns_after_the_connection_closes
PASS [ 0.077s] ( 1997/12962) moq-gst source::imp::tests::video_caps_preserve_nal_framing
PASS [ 0.083s] ( 1998/12962) moq-gst source::imp::tests::surround_opus_caps_carry_the_family_1_mapping
PASS [ 0.256s] ( 1999/12962) moq-gst source::imp::session_tests::restarts_under_reconnects_interleave_on_one_pad
PASS [ 0.430s] ( 2000/12962) moq-gst source::imp::session_tests::an_update_keeps_the_run
PASS [ 0.913s] ( 2001/12962) moq-gst source::imp::session_tests::a_delisted_sibling_rendition_follows_no_later_instance
FAIL [ 1.069s] ( 2002/12962) moq-gst source::imp::session_tests::a_moqsink_restart_keeps_pads_when_caps_arrive_at_different_times
stdout ───
running 1 test
test source::imp::session_tests::a_moqsink_restart_keeps_pads_when_caps_arrive_at_different_times ... FAILED
failures:
failures:
source::imp::session_tests::a_moqsink_restart_keeps_pads_when_caps_arrive_at_different_times
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 181 filtered out; finished in 1.01s
stderr ───
thread 'source::imp::session_tests::a_moqsink_restart_keeps_pads_when_caps_arrive_at_different_times' (420089) panicked at rs/moq-gst/src/source/imp.rs:6267:17:
assertion failed: playing.iter().all(|pad| held.contains(pad) && pad.starts_with("video_"))
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
(gstmoq-8590436b237682a5:420076): GStreamer-CRITICAL **: 07:57:50.030:
Trying to dispose element moqsink0, but it is in PAUSED instead of the NULL state.
You need to explicitly set elements to the NULL state before
dropping the final reference, to allow them to clean up.
This problem may also be caused by a refcounting bug in the
application or some element.
Cancelling due to test failure: 23 tests still running
PASS [ 0.862s] ( 2003/12962) moq-gst source::imp::session_tests::a_retired_rendition_drains_to_eos
PASS [ 0.815s] ( 2004/12962) moq-gst source::imp::session_tests::audio_and_video_share_a_run_timeline_across_scales_and_restarts
PASS [ 0.823s] ( 2005/12962) moq-gst::element a_pad_released_from_pad_added_is_not_returned
PASS [ 1.235s] ( 2006/12962) moq-gst source::imp::session_tests::surround_opus_plays_every_channel_through_a_relay
PASS [ 2.162s] ( 2007/12962) moq-gst source::imp::session_tests::a_protocol_violation_posts_a_bus_error_after_playback
PASS [ 2.542s] ( 2008/12962) moq-gst source::imp::session_tests::a_refused_redial_posts_a_bus_error_after_playback
PASS [ 3.400s] ( 2009/12962) moq-gst source::imp::session_tests::a_closed_publisher_session_resumes_on_the_next_start
PASS [ 3.669s] ( 2010/12962) moq-gst source::imp::session_tests::a_restart_renders_through_a_synced_sink
PASS [ 4.608s] ( 2011/12962) moq-gst::element a_pad_without_caps_stays_pending
PASS [ 4.774s] ( 2012/12962) moq-gst::element a_failed_ready_to_paused_rolls_back_the_publication
PASS [ 4.887s] ( 2013/12962) moq-gst::element a_failed_first_connect_errors_without_retrying
PASS [ 4.879s] ( 2014/12962) moq-gst::element a_flush_after_eos_makes_the_pad_flow_again
PASS [ 4.732s] ( 2015/12962) moq-gst::element a_pad_the_element_already_lists_holds_the_aggregation
PASS [ 4.856s] ( 2016/12962) moq-gst::element a_new_stream_after_eos_makes_the_pad_flow_again
PASS [ 4.902s] ( 2017/12962) moq-gst::element a_colliding_name_reports_on_the_second_pad
PASS [ 4.910s] ( 2018/12962) moq-gst::element a_buffer_after_the_end_reports_eos_instead_of_ok
PASS [ 4.907s] ( 2019/12962) moq-gst::element a_failed_paused_to_ready_keeps_the_publication
PASS [ 4.865s] ( 2020/12962) moq-gst::element a_pad_requested_after_the_end_is_refused
PASS [ 4.915s] ( 2021/12962) moq-gst::element a_pad_requested_from_the_eos_notify_is_refused
PASS [ 5.315s] ( 2022/12962) moq-gst source::imp::session_tests::relay_loss_reconnects_after_refused_redials
PASS [ 5.957s] ( 2023/12962) moq-gst::element a_pad_negotiated_from_pad_added_still_publishes
PASS [ 5.976s] ( 2024/12962) moq-gst::element a_lingering_sink_holds_the_eos_until_the_pipeline_stops
PASS [ 5.965s] ( 2025/12962) moq-gst::element a_pad_kept_across_runs_publishes_again
────────────
Summary [ 25.847s] 2025/12962 tests run: 2024 passed, 1 failed, 151 skipped
FAIL [ 1.069s] ( 2002/12962) moq-gst source::imp::session_tests::a_moqsink_restart_keeps_pads_when_caps_arrive_at_different_times
stdout ───
running 1 test
test source::imp::session_tests::a_moqsink_restart_keeps_pads_when_caps_arrive_at_different_times ... FAILED
failures:
failures:
source::imp::session_tests::a_moqsink_restart_keeps_pads_when_caps_arrive_at_different_times
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 181 filtered out; finished in 1.01s
stderr ───
thread 'source::imp::session_tests::a_moqsink_restart_keeps_pads_when_caps_arrive_at_different_times' (420089) panicked at rs/moq-gst/src/source/imp.rs:6267:17:
assertion failed: playing.iter().all(|pad| held.contains(pad) && pad.starts_with("video_"))
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
(gstmoq-8590436b237682a5:420076): GStreamer-CRITICAL **: 07:57:50.030:
Trying to dispose element moqsink0, but it is in PAUSED instead of the NULL state.
You need to explicitly set elements to the NULL state before
dropping the final reference, to allow them to clean up.
This problem may also be caused by a refcounting bug in the
application or some element.
warning: 10937/12962 tests were not run due to test failure (run with --no-fail-fast to run all tests, or run with --max-fail)
error: test run failed
mbx[cache]: object cache: 400 hits, 445 misses, 84 bypassed; 0 B downloaded, 0 B uploaded, 9.4 GiB stored locally
mbx[cache]: 8m 18s session; 5m 28s estimated compiler time avoided (summed across compilations)
mbx[cache]: bypass reasons: 84 unsupported-crate-type
mbx[cache]: Cargo artifact reuse and CI cache archive transfers are not included above
mbx[savings]: 16d 3h of compilation avoided across 19248 builds. imagine the fan noise. now stop.
error: recipe `test` failed on line 377 with exit code 100
error: recipe `test-changed` failed on line 97 with exit code 100
##[endgroup]
##[error]ci: test rs failed
##[error]Process completed with exit code 100.
##[group]Run actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9
with:
name: cargo-timings
path: target/cargo-timings/cargo-timing-*.html
if-no-files-found: ignore
retention-days: 14
compression-level: 6
overwrite: false
include-hidden-files: false
archive: true
env:
BASE: 8c8f0effaa161df7fa67e34dcdabbcc8b2536132
CHECK_FAIL_FAST: 1
NEXTEST_TEST_THREADS: 24
MBX_CACHE_LINKS: 1
MBX_REMOTE_URL:
##[endgroup]
With the provided path, there will be 1 file uploaded
Artifact name is valid!
Root directory input is valid!
Uploading artifact: cargo-timings.zip
Beginning upload of artifact content to blob storage
Uploaded bytes 93994
Finished uploading artifact content to blob storage!
SHA256 digest of uploaded artifact is 5a384dcce618ff86974e24a8ab6e75bd856ee81d04cefc6e0d39408cba56357e
Finalizing artifact upload
Artifact cargo-timings successfully finalized. Artifact ID 13961366309
Artifact cargo-timings has been successfully uploaded! Final size is 93994 bytes. Artifact ID is 13961366309
Artifact download URL: https://github.com/moq-dev/moq/actions/runs/43250788147/artifacts/13961366309
Post job cleanup.
##[start-action display=Run jdx/mr-boxington-action@88acf2bb50552c7c508cbac13e5f8c576b95b1de;id=__89743681-dff3-4a9b-82b7-7075ec7e3109.mbx]
##[end-action id=__89743681-dff3-4a9b-82b7-7075ec7e3109.mbx;outcome=skipped;conclusion=skipped;duration_ms=0]
Post job cleanup.
[command]/nix/store/lg46w7hrjx7kylsh6645c9l975i8h43b-git-2.55.0/bin/git version
git version 2.55.0
Temporarily overriding HOME='/var/cache/github-runner-work/_temp/c8ea7573-d22f-4a21-b2b9-ae6a14a076d9' before making global git config changes
Adding repository directory to the temporary git global config as a safe directory
[command]/nix/store/lg46w7hrjx7kylsh6645c9l975i8h43b-git-2.55.0/bin/git config --global --add safe.directory /var/cache/github-runner-work/moq/moq
Removing SSH command configuration
[command]/nix/store/lg46w7hrjx7kylsh6645c9l975i8h43b-git-2.55.0/bin/git config --local --name-only --get-regexp core\.sshCommand
[command]/nix/store/lg46w7hrjx7kylsh6645c9l975i8h43b-git-2.55.0/bin/git submodule foreach --recursive sh -c "git config --local --name-only --get-regexp 'core\.sshCommand' && git config --local --unset-all 'core.sshCommand' || :"
Removing HTTP extra header
[command]/nix/store/lg46w7hrjx7kylsh6645c9l975i8h43b-git-2.55.0/bin/git config --local --name-only --get-regexp http\.https\:\/\/github\.com\/\.extraheader
[command]/nix/store/lg46w7hrjx7kylsh6645c9l975i8h43b-git-2.55.0/bin/git submodule foreach --recursive sh -c "git config --local --name-only --get-regexp 'http\.https\:\/\/github\.com\/\.extraheader' && git config --local --unset-all 'http.https://github.com/.extraheader' || :"
Removing includeIf entries pointing to credentials config files
[command]/nix/store/lg46w7hrjx7kylsh6645c9l975i8h43b-git-2.55.0/bin/git config --local --name-only --get-regexp ^includeIf\.gitdir:
[command]/nix/store/lg46w7hrjx7kylsh6645c9l975i8h43b-git-2.55.0/bin/git submodule foreach --recursive git config --local --show-origin --name-only --get-regexp remote.origin.url
A job completed hook has been configured by the self-hosted runner administrator
##[group]Run '/nix/store/gx2v6q84b0w43bafm0whkdx93qyxbpps-ci-cache-gc.sh/bin/ci-cache-gc.sh'
shell: /nix/store/1mv3qz005gkbalxfghk4y3s8ayv4d2dl-bash-interactive-5.3p15/bin/bash --noprofile --norc -e -o pipefail {0}
##[endgroup]
evicted 1053 objects and 121 action results (8.3 GiB logical); 49.5 GiB logical remain
dropped 2 stale checkout records
dropped 2 session event streams
removed 0 learned incremental units and 2 checkout directories (3.0 KiB logical); 120 units (5.0 GiB logical) remain
removed 2 target directories (13.7 MiB logical, 2 abandoned and 0 live); 24.7 GiB logical remain
Cleaning up orphan processes
(written by quest land watch) |
Contributor
|
codex, gpt-6.1-sol, medium effort land fixer note:
(written by codex, gpt-6.1-sol, medium effort) |
Contributor
|
quest land watch: re-enqueued this pull request at 85397cc, at the back of the merge queue, unchanged, since the land fixer judged the failure flaky: (written by quest land watch) |
This was referenced Nov 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Finishes
quest/m1/group-error-cache.md.Summary
The inventory below shows that the drop set already has the two time bounds this quest asks for. Each run is dated against the pool's idle window (
drops_born, from #8476), and a stale run is held under the budget it was judged by, which a widening or new terms reverse. Only one path still recordedOldas a loss for good:Fill::rejectunder the publisher's max age (RustInner::reject, JSrejectGroups) used to recordHole::drop(Old), which survives new terms and binds every reader for as long as it is held. It now recordsHole::stale(max_age). Readers no wider than the max age are still settled without asking again. New terms (supersede_stale), a widening, or the idle window forget it, and the next reader then asks. If the max age isn't known here, the refusal goes only to the readers waiting now and nothing is recorded, as anOldunder the fill's own budget already is.Olddrop as final throughrejectGroups(..., undefined). Rust (Refusal::answer) refuses it for now. JS now refuses it too.The other main path the quest names, a live SUBSCRIBE_DROP caching
Old, was already fixed on lite-07: anOlddrop there isHole::stale(budget), reopened by the next update. Before lite-07 a drop carries no code and decodes asOldwhatever the cause, so it can't be read as a stale verdict. It stays a final drop under the idle window.Inventory
lite::subscriberSUBSCRIBE_DROP / JS#runResponsesdropOld(no code)Old: stale under the subscription budget; else finallite::subscriberSUBSCRIBE_SKIPHole::SkipFill::drop_groups, fill dropped unanswered (Inner::drop/ JS#settle)NotFoundFill::refuse_groups/Fill::refuse/ JSrefuseGroupsFill::reject_groups/GroupFill::reject/ JSrejectGroupsOldunder max ageOldunder the fill's budget, or max age unknownNotFoundfor one frame range included)Refusal::answer/ JS#fillResponsesNotFoundfrom a lost source: final; anything elseOld, now refused like Rustietf/subscriber.rs), JSietf/range.tsNotFound(NonExistent) recorded; other errors refused or rejectedreject_groupsTrackState::reclaim_aborted,tear_downChoices made without asking
Oldwould need a bound shorter than the window, and none earns a new constant: once a group is past its max age, asking again only getsOld. So the only kind-specific rule left isOld's budget, andHole::stalealready says that once. A pool with no idle window (Pool::unbounded, the inert default) reclaims nothing by time, and drops follow cached groups there.Olddrops on lite-07 (such asEvictedorCancel) stay recorded under the window rather than not recorded. They settle live readers and are forwarded by relays right away. Not recording them would stall each hop's readers for the arrival grace before a fill.Decisions
Old? ✅ (a) yes:Oldhas one bound, its budget, reversed by a widening or new terms, plus the idle window for memory (rejected: (b) also hold stale runs for a time bound). Decided by the coordinator on the maintainer's behalf while away; already implemented on this head.Public API
None.
rejectGroupsandJudgedare@internalin JS, andFillTrack.drop(internal) now takes a hole. Wire: none.Caused by
Oldunder the publisher's max age as a final drop, which this replaces with a stale verdict.Tests
a_fills_old_answer_under_the_max_age_is_held_until_forgottenand JSfill: an OLD answer under the max age is held as stale until forgotten: the readers waiting now (a bounded one included) are refused with OLD, the verdict is held stale under the max age, a later reader is settled without a fill, and oncesupersede_staleforgets it a reader asks again. Expiry by the idle window is covered by the existingthe_sweep_forgets_runs_past_the_expiry_window.just check: everything passes exceptmoq-relayruntime_uring::uring_startup_reports_the_memlock_limit. That test expects startup to fail under a low memlock limit, but this host's limit (8192 KB) lets startup succeed, and the crate is untouched here.Follow-ups
None.
Models
Size: M
Implementer: Claude Code, claude-opus-5-5, medium effort
Implementer: Claude Code, claude-opus-5-5, low effort (iterate, XS)
🤖 Generated with Claude Code
(Written by Claude Opus 5.5 with medium)
Review
Rewritten by
quest reviewafter each round.01a1cf31-d85e-7bf2-969d-58467da17457, round 2The incremental change fixes the bounded-reader OLD refusal regression, and code review found no new issues.js/net/src/fill.ts:487:Deliver the OLD refusal before resolving the fill;js/net/src/fill.ts now calls fillOps.refuse without a budget before resolving stale fills, delivering OLD directly to current bounded readers; both language tests add bounded-reader coverage.