Repository navigation
fix(net): keep driving cache-miss fetches through subscription waits - #7903
Conversation
Quest-Claim: 48dfb52e60b522fa
Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Quest-Claim: ae079970e0949fb7
…/backport-fill-head-race
The PUBLISH_DONE grace and a resumed subscription's setup waits stopped polling the track's group fetches, so a miss the joined head released, or a reader cancelling its FETCH, stalled until the wait ended. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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 3 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
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 527e4c9
[P2] Drive FETCH cancellation during failed resumed setup too — rs/moq-net/src/ietf/subscriber.rs:3436–3441. A lingering copy can have an unanswered cache-miss FETCH when SUBSCRIBE resumes. If its grant is revoked before SUBSCRIBE_OK, this branch aborts the track, then awaits cancel_subscribe without polling group_fetches. That await can remain pending on the SUBSCRIBE FIN acknowledgement. Aborting the track only wakes the FETCH task: it must still poll fill.poll_aborted to unregister and send STOP/RESET. Consequently the FETCH remains uncancelled until that unrelated ACK or session teardown. Returning None eventually drops the set, but cannot do so while this await is pending. Apply the new drive pattern to these terminal cancellation waits too (also lines 3402 and 3551), and extend the resumed-setup test with revocation and an unacknowledged FIN.
The grace and normal resumed-setup changes address the intended stall, but the same cancellation problem remains on failed setup.
Verification limits: GitHub-only static tracing of subscription/FETCH ownership, TaskSet and transport close semantics, with comparison to the existing cancellation-before-ACK tests. I did not run the new tests or reproduce the failure.
(Written by review (OpenAI))
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Grace wait drives fetches.
End::Done's PUBLISH_DONE grace now pollsgroup_fetches, so a cache miss waiting on the joined group's head FETCHes as soon as the subscription's fill settles rather than after the grace (which can run well pastGRACE). - Resumed SUBSCRIBE setup drives fetches. The listed-track handoff, request-stream open, and answer waits now poll
group_fetches, and the abandonment path usesgroup_fetches.drive(...)aroundcancel_subscribe, so a leaving reader cancels its FETCH even when the publisher never answers the resumed SUBSCRIBE. - Two reproductions.
a_miss_fetches_during_the_publish_done_graceanda_resubscribe_keeps_driving_the_lingering_fetches, plus aJoinRun::subscribehelper that accepts a prebuiltScriptedSession. - Quest bookkeeping.
quest/m0/backport-fill-head-race.mdnow points at this PR as the main-side fix for thereleasebackport (#7680) and leaves only review/merge and the patch release as the remaining wait.
I confirmed both tests pass at head and are not tautological: removing the grace poll makes the grace test see 0 FETCHes during the grace, and removing the answer-wait poll leaves the dropped reader's FETCH uncancelled. The quest's claim about release also checks out: its run_subscribe has no linger/resume cycle, so the backport needs only the grace poll. The per-wait poll pattern matches the existing linger/learn_track_status code, and the body records why the single outer-drive alternative was rejected.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏

Problem
An IETF subscription's cache-miss fetches (
group_fetches) are only polled by a few ofsubscribe_once's waits. While any other wait runs, they stall:GRACE. Fetches already in flight also stop being read. The release backport moq-dev/moq#7680 adds this poll onrelease, and the main-first backport rule needs it onmainfirst.subscribe_once. Four waits there leave them unpolled until SUBSCRIBE_OK arrives, or forever if the publisher never answers: the listed-track handoff, the request-stream open, the answer wait, and the cancel on abandonment that returns to linger. So a reader that leaves cannot cancel its FETCH.Approach
Poll
group_fetchesin each of those waits. The cancel-then-linger path drives them withTaskSet::drive, as the existing cancel at the end of the function does by hand. Paths that returnNonedrop the set, along with the track it served, so they need no change.Each fix has a reproduction that failed on unfixed
main:a_miss_fetches_during_the_publish_done_grace: on unfixedmain, no FETCH goes out during the grace.a_resubscribe_keeps_driving_the_lingering_fetches: on unfixedmain, dropping the reader stops no stream while SUBSCRIBE is unanswered.The quest is rewritten to what remains, which is the maintainer's review and merge of moq-dev/moq#7680 and the
moq-netpatch release.Impact
Alternatives
releasehas no counterpart: it creates its fetch set after SUBSCRIBE_OK and never lingers, so moq-dev/moq#7680 needs nothing more.subscribe_once/lingercycle inrun_subscribe. This would make the set impossible to forget in any later wait. But it means pushing through aTaskshandle, and that breaksdrain's "finish when the children finish" semantics. It is a larger refactor of a 17k-line file that moq-dev/moq#7670 is also changing.lingerafterward, or refused when the subscription failed. That is a delay bounded by the grace, not a stall, andreleasebehaves the same way.Choices made without asking: I included the setup-wait fix in this PR, rather than as a follow-up, because it is the same stall in the same function.
Validation
mainand pass with the fix.just checkpassed: rs lint and clippy, and 10022 Rust tests.Review
quest review), gpt-6.1-sol, medium effort, round 1 againstorigin/main, reviewed 527e4c9c17: no findings.Follow-ups
None.
(Written by Claude Opus 5.5)