[kixelated]

Finishes quest/m1/cut-audit/reserved-route-return.md and deletes it.

Problem

A resumed reader's reservation lived in two places: the resume Reader.peeked and the copy cursor's Cursor.peeked. When the route returned to an earlier copy (A to B to A), Reader::sync reused A's copy and called set_range on it, which cleared the cursor's half while the reader's half survived. The next take re-queried the cursor and hit unreachable!("the peek reserved a group") in resume.rs. The same split existed one level up: an outer reader's route returning to a nested logical track cleared the nested reader's peeked through Reader::set_range, so the outer take panicked instead. The window is real in production, since Producer::serve syncs every reader from the serving thread, between another thread's peek and take.

Fix

The quest's preferred direction: a peek returns its reservation instead of storing it, and a take hands that exact group out without asking the cursor again.

• Cursor::poll_peek returns a Peeked (the group plus the step that moves past it: the arrival walk index, an owed or filled group, or the sequence cursor). Cursor::take commits it. A plain read is now that peek followed by that take, so the take: bool plumbing through poll_recv_with/poll_next_with is gone.
• track::Subscriber::poll_peek_group returns a track::Reserved (Plain(Peeked) or Resume(Box<resume::Candidate>)), and the new track::Subscriber::take hands it out, records it in delivered, and meters it.
• resume::Candidate now carries the copy's own Reserved beside its lease. Reader::take replaces poll_group/poll_group_once.
• Deleted: Cursor.peeked, Reader.peeked, resume::Subscriber::skip_ordered, resume::Subscriber::poll_group, every clearing site (set_range at both levels, skip_ordered, learn_end, past_end), and the unreachable!/assert_eq! pair. A take through a nested reader that is now past its declared end returns ProtocolViolation where the old code would also have panicked.

Tests

• New a_reserved_group_survives_its_route_returning in resume.rs: peek, evict the group, flip the route A to B to A, then take. It covers both orders; a direct and a nested reader; the nested reader's route returning and the outer reader's route returning; and, for arrival order, a late group that sits below the new floor. All 9 cases panicked before this change.
• Existing reservation tests now take the returned reservation instead of relying on a stored one. changed_bounds_invalidate_a_peeked_group is now changed_bounds_after_a_peek_decide_the_next_read, since a peek no longer stores anything to invalidate.
• cargo test -p moq-net --lib passes: 2626 passed, 12 ignored.
• just check passes. Its paired bench reported rust/kio/lock/4 at +40%. This PR does not touch kio, and the host's load average was 25 to 65 during the run.

Choices made without asking

• No cache refresh on a stale arrival skip. The arrival take path used to stamp cache_refresh before the drift check, so groups it then skipped as stale were refreshed too. The stamp now happens only when a group is taken, which matches the sequence path. This only affects how long the cache keeps a group the reader gave up on.
• No cursor sync() on commit. The old stored-reservation take re-synced the cursor because the take could come in a later poll. A take now follows its peek in the same read, and deliver reads the subscription and servable channels live.
• A top-level resumed read takes the reader lock twice, once for the peek and once for the take. A concurrent serve() can land between them, which is exactly the window the take now tolerates. The nested path always had that window.
• #[allow(clippy::large_enum_variant)] on Reserved. Plain holds a group::Consumer (about 280 bytes) by value, so the hot plain read does not allocate. It is a stack temporary that is never stored.

Impact

• Public API: none. Everything touched is pub(super) or private in rs/moq-net/src/model.
• Wire: none.
• js/net is unaffected: its resume reader peeks and takes synchronously, with no stored reservation.

Benchmarks

No new benchmark: the read path does the same work in a different order. A plain read peeks and then takes, with no extra cursor sync(), clone, or allocation, and it no longer clones the group into a stored reservation. A resumed read drops the Reader.peeked bookkeeping. I saved a base run of track_fanout_group, track_parked_read, and origin/{copy_walk,resume_overlap,passthrough}, but the host's load average stayed at 40 to 65 for over 30 minutes, so I abandoned the head-to-head comparison rather than report noise. just bench origin/main on an idle host would confirm it.

Merge with main

Merged origin/main at c04f7771dc (no rebase). Two conflicts, both in the read path:

• track::Subscriber::take/poll_group: main's errored track end (moq-dev/moq#7349) maps a read that runs out to ended(false). poll_group now does that on the peek's Ok(None), and keeps main's end_error/ended_with_error/ended/reaches. Main's Option<u64> poll_peek_group is dropped for this PR's Reserved one.
• resume::Subscriber: kept main's Mirror (it syncs the reader from another thread, one more caller of the window the take now tolerates) beside this PR's poll_peek_group/take, and dropped main's poll_group.

After the merge, just check origin/main passes: 9188 nextest tests, lint, and the other gates. Its paired bench again reported rust/kio/lock/4 at +40%; this PR does not touch kio. just rs check-test -p moq-net passes 3229 tests. Plain cargo test -p moq-net --lib now fails setup::tests::credentials_stay_out_of_traces, since two tests from main each install a global tracing subscriber. nextest runs each test in its own process, so it passes there; see Follow-ups.

Review

mechanical fix by codex / Codex: Merged base; preserved reservation reads and threaded delivered-group state into recovery probes. (head beb015d)

• Reviewer: Codex (quest review), gpt-6.1-sol, medium effort, round 1.
• Head: 98701c267c.
• Findings: none.

Follow-ups

• setup::tests::credentials_stay_out_of_traces (traced_test) and model::test_tracing both set a global tracing subscriber, so plain cargo test -p moq-net --lib fails. nextest is unaffected. This is pre-existing on main.

No other new follow-ups. Resume as producer replacement still deletes Candidate and the copy cursors altogether.

(Written by Claude Opus 5.5; taken over and merged with main by Claude Opus 5.5)

🤖 Generated with Claude Code

[kixelated]

Taking this draft over at the user's request. Its starter session had been quiet for about 3 days and the body had no ## Decisions needed section.

• Merged origin/main (no rebase). Two read-path conflicts are resolved as the body's "Merge with main" section describes. The one behavior to carry over was main's errored-end mapping on a read that runs out.
• just check origin/main and just rs check-test -p moq-net pass on 98701c267c.
• Local Codex review (gpt-6.1-sol, medium): 0 findings on that head.
• Marked ready for review.

(Written by Claude Opus 5.5)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 98701c267c31c7424c5c7c7b7cc1b4b8a3288fa7.

No actionable bugs found in the changed reservation, cursor-commit, and nested-reader paths. Carrying the exact group and cursor step through Reserved/Candidate fixes the split ownership at its source, rather than adding more invalidation exceptions. The copy lease preserves the commit target across route changes; direct and nested route-return tests cover both read orders and the late-arrival case. This is a proportionate internal refactor with no public API or wire change.

Verification: static inspection of the diff and relevant selection, retention, range-update, delivery, and metering code. I did not run tests, builds, or benchmarks; the PR's reported results were not independently reproduced.

(Written by OpenAI)

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: e276fa8e-ccf0-4d1b-80f4-d8416f64f8b8

Commits

Reviewing files that changed from the base of the PR and between c04f777 and 98701c2.

Files selected for processing: 5

• quest/m1/cut-audit/README.md
• quest/m1/cut-audit/reserved-route-return.md
• quest/m2/resume-redesign.md
• rs/moq-net/src/model/resume.rs
• rs/moq-net/src/model/track.rs

Files with no reviewable changes: 3

• quest/m1/cut-audit/README.md
• quest/m1/cut-audit/reserved-route-return.md
• quest/m2/resume-redesign.md

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

Walkthrough

Track cursors now return group reservations that subscribers and resumed readers consume in a separate take step. Resumed readers retain the selected candidate and its copy lease across route changes. Tests cover taking reservations after eviction, replacement, and route return. Related quest documentation and links were removed.

Severity of issue fixed: Medium

Merge Risk: Minimal · up to 98701

No actionable merge-blocking issue remains; proceed with normal checks.

Pre-merge checks: 5

Check name	Status	Explanation
Title check		The title clearly and concisely describes the main change: taking the exact reserved group without re-querying its cursor.
Description check		The description is directly related to the changeset. It explains the panic, the reservation redesign, affected paths, tests, validation results, and impact.
Docstring Coverage		Docstring coverage is 90.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentati…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 90.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 10 functions across 1 files. (1 skipped: 1 too large.)

Finishing Touches

Simplify code

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[pullfrog]

✅ No new issues found.

Reviewed changes

A peek now returns its reservation instead of storing it on two levels, and a take hands that exact group out; this deletes the two-level reservation whose split caused the unreachable! panic when a route returned A→B→A between peek and take.

• Cursor reservation is now a value: poll_peek returns Peeked { group, arrival: Option<Taken> } and take commits it, replacing the stored Cursor.peeked and the take: bool threading through poll_recv_with/poll_next_with.
• Subscriber-level peek/take: track::Subscriber::poll_peek_group returns track::Reserved (Plain(Peeked) | Resume(Box<Candidate>)); the new take hands it out, records it in delivered, and meters it. Reserved keeps the hot plain group on the stack.
• Resume path: resume::Candidate carries the copy's own track::Reserved beside its lease; Reader::take replaces poll_group/poll_group_once.
• Deletions: Cursor.peeked, Reader.peeked, resume::Subscriber::skip_ordered/poll_group, every clearing site, and the unreachable!/assert_eq! pair. A take through a nested reader past its declared end now returns ProtocolViolation instead of panicking.
• Tests: new a_reserved_group_survives_its_route_returning covering both read orders, direct and nested readers, and each route returning; existing reservation tests take the returned reservation.

I verified the peek+take split stays atomic per track::Subscriber (&mut self, no yield between), that Inner is fixed at construction so the Reserved/Inner mismatch arm is unreachable, and that Reserved::ordered() (from Peeked.arrival.is_none()) matches the requested order. cargo test -p moq-net --lib -- a_reserved_group_survives_its_route_returning and the reservation/peek/skip set (610 tests) both pass. The one intentional semantic drift — a held reservation is handed out even if a later route/bound change moved past it — is documented in the PR body and covered by the new test, so I am not flagging it.

  ^{｜ View workflow run ｜ Using deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏}

[kixelated]

Landing 98701c267c.

• Changes: a peek returns its reservation (Peeked / track::Reserved / resume::Candidate) and a take commits that exact group, so a route returning A to B to A between peek and take no longer hits unreachable!. The stored Cursor.peeked / Reader.peeked and their clearing sites are gone. Finishes and deletes quest/m1/cut-audit/reserved-route-return.md.
• Reviews on this head: local Codex round 1 (no findings), OpenAI review (no actionable bugs), Pullfrog (no new issues). No unresolved threads.
• CI: all 6 required checks pass. quest check against origin/main passes on the head.
• Decisions: none outstanding. The choices made without asking are listed in the PR body.
• Follow-up: two tests install a global tracing subscriber, so plain cargo test -p moq-net --lib fails (nextest passes). This already happens on main.

(Written by Claude Opus 5.5)

[pullfrog]

✅ No new issues found.

Reviewed changes

Since the prior pullfrog review (98701c26), the branch merged main (4a3f7c0d) and adapted the reservation refactor to it. I read the delta and the current read path; every adaptation preserves the pre-merge behavior.

• Merged base via beb015d72: resolved the read-path conflicts by keeping this PR's peek/take split over main's errored track end.
• Threaded the subscriber's delivered into the probe: Cursor::poll_peek / poll_peek_recv / poll_peek_next / poll_drop and probe now carry taken: &Delivered, and Subscriber::poll_peek_group passes &self.delivered. This is main's miss-judging, evaluated at the same point (the peeked group is not in delivered until take), so a peek does not change which fills are asked.
• Kept main's errored track end: Subscriber::poll_group maps a peek that runs out to ended(false) and keeps end_error / ended_with_error / ended / reaches, matching main.
• Kept main's Mirror in resume::Subscriber beside this PR's poll_peek_group / take; dropped main's poll_group.
• Quest bookkeeping: deleted the finished quest/m1/cut-audit/reserved-route-return.md and its links in cut-audit/README.md and m2/resume-redesign.md.

I confirmed the panic path stays closed: Reader::take still finds its copy by lease, and Copy::retains keeps any copy whose lease strong count exceeds one, so an in-flight Candidate keeps its copy in self.copies across a route flap. No inline findings — the merge adaptation is faithful and the prior review's "no findings" stands.

  ^{｜ View workflow run ｜ Using deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏}

[moq-bot]

quest land removed this pull request from the queue: Conflicts with the base: rs/moq-net/src/model/track.rs
.