fix(moq-net): an IETF copy goes idle before its cancel

[kixelated]

Problem

broadcast_rejoin_skips_a_stale_warm_cache (rs/moq-tokio/tests/broadcast.rs) failed once under a loaded just check on moq-transport-17: a rejoining reader was served the stale cached group first.

The cause is in the product. When the last IETF subscriber leaves, the subscriber awaits cancel_subscribe before it calls track.set_idle(). On draft-17+ that is a STOP_SENDING plus a close().await that waits on the publisher. The publisher stops serving as soon as the cancel lands, about a round trip before the copy is marked idle. A reader rejoining in that gap finds a copy that still looks live and takes its newest cached group as the live edge. Real clients hit the same window, not only the test.

Approach

• rs/moq-net/src/ietf/subscriber.rs: mark the copy idle as soon as End::Idle is decided, before the awaited cancel (skipped when a session abort already took the copy, as before).
• Lite already does this: it FINs the stream and calls set_idle() in the same synchronous step, so it needs no change. The other IETF exits (abandoned before accepted, rejected answer, alias failure) never return a copy to lingering as live.
• rs/moq-net/tests/support/mock.rs: the mock delivered a STOP_SENDING (and a receiver's drop) to the sender instantly, so a cancel's close never waited on the link. It now arrives after the link latency, like stream data, via a moq_net_sim task that sets the closed signal once the latency passes. With latency set, the cancel's close takes a round trip and the gap opens deterministically on the simulated clock.
• rs/moq-net/tests/rejoin.rs: rejoin_during_the_cancel_skips_the_cache runs under moq_net_sim::test over a 50 ms link for every version. Without the fix it fails on an IETF version ("got the stale cache first", group 0 instead of 3); with it, every version passes. The full moq-net suite passes with the mock change.
• Removes the finished quest and its entry in the line's README.

Verified: the moq-tokio test passes 20/20 under --stress-count 20, just check passes, and just test interop --all passes (35/35). The first interop run on a loaded machine failed only python -> js (moq-lite-06, browser): the Playwright play click timed out behind the buffering spinner. That path doesn't touch this change, and the rerun passed.

Impact

• Public API: none.
• Wire: none. Only the local order of marking the copy idle and sending the cancel changed.

Alternatives

• Only make the test wait for the subscriber's idle. Rejected in the quest: it leaves the window open for real clients.
• A one-off FIN-ack hold in the mock with a release hook. Delaying STOP_SENDING by the link latency models QUIC more faithfully and needs no new test API.

Decisions

Settled during /quest-iterate (maintainer away, proceeding on the recommendation):

How should a test open the gap between the cancel landing and the copy going idle?

1. ✅ Delay STOP_SENDING (and a receiver's drop) by the mock's link latency, in the shared mock (recommended): models QUIC more faithfully, adds no test API, and fix(net): a relay's rejoin never shows an older cached group as the live edge #4914 (media late join) builds on it, so set_latency / MockConnectOptions::latency stay as they are.
2. A test-only FIN-ack hold in the mock with a release hook: a one-off knob that only this test would use.

Follow-ups

• None required for this flake. The rest of quest/m1/test-flakes-2 is still open.
• Possible: the interop browser leg (python -> js) once timed out clicking play behind the buffering spinner under load. It might be worth a test-flakes child if it recurs.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 924cd8a5-0c2d-4606-8789-bfed6e0cf41a

📥 Commits

Reviewing files that changed from the base of the PR and between a1afac2 and 85856bc.

📒 Files selected for processing (3)

• quest/m1/test-flakes-2/README.md
• rs/moq-net/tests/rejoin.rs
• rs/moq-net/tests/support/mock.rs

💤 Files with no reviewable changes (1)

• quest/m1/test-flakes-2/README.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

The IETF subscriber now marks an idle track before it waits for cancellation delivery, unless a session abort already removed the subscription. The mock transport delays peer stop and drop signals according to connection latency. A regression test covers a reader rejoining during cancellation and checks that it receives sequence 3.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 85856

This change fixes a race where a reader rejoining during an IETF cancel could receive a stale cached group. No merge-blocking risk was found. The PR text mentions a README merge conflict and CI that had not finished, so confirm both before merging.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the main fix: marking an IETF copy idle before awaiting its cancel.
Description check	✅ Passed	The description explains the stale-cache race, the implementation and test changes, and the reported validation results.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches

✨ Simplify code

• Commit to this branch
• Create a new PR

• Autopilot · 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

_{Comment @coderabbitai help to get the list of available commands.}

[kixelated]

Grok review of ad94c415 (full review, no earlier Grok review on this PR)

The fix is right and small. The IETF subscriber now calls track.set_idle() as soon as End::Idle is decided, so a reader that comes back during the cancel round trip no longer takes the cached group as the live edge. The mock now delivers STOP_SENDING (and a receiver drop) after the link latency, which makes the gap deterministic on paused time. I found no blocking issues.

Non-blocking

1. A group opened during the cancel can land after the idle snapshot (rs/moq-net/src/ietf/subscriber.rs ~2227 together with recv_group ~2614-2690, and TrackState::set_idle in model/track.rs ~1247). set_idle records idle_newest from the groups cached at that moment. A data stream whose alias resolved before remove_subscribe can still be inside open_group, which waits for the group's first object, and it creates its group after the snapshot. Here is the scenario: the snapshot says newest is N, a late group N+1 lands while idle, and the reader rejoins. set_live(largest) then sees largest.group > N + 1 and sets live_floor = N + 1, so the reader can be handed the late N+1 before the new feed's groups arrive. That is the same "stale cache first" symptom in a narrower case. The window already existed after the old set_idle, but marking idle a round trip earlier makes it wider. A possible fix is to drop a group in recv_group or open_group when its request_id is no longer in subscribes at creation time (re-check under the lock right before inserting). Another is to raise idle_newest for subscription groups that land while !live. A test like the new one, with a group whose first object is still in flight when the reader leaves, would show whether this is real.
2. The mock's write failure and its stop signal are now out of step (tests/support/mock.rs, Drop for MockRecvStream). self.rx.close() still fails the sender's writes immediately, while poll_closed only reports the stop after latency. In QUIC, the sender learns both at the same time. Nothing seems to depend on this today, but a future test could see a write error a full one-way delay before closed() resolves. Consider delaying the write failure the same way, or note the asymmetry in the doc comment.
3. The connection error now wins over a pending stop during the delay (poll_closed). If the connection closes while landing is still sleeping, poll_closed returns the connection error instead of Ok. The FIN-ack path (Duration::ZERO) keeps the old "trust this signal ahead of the connection error" behaviour, so this only changes tests with latency set. It's probably fine, but it's a quiet change in semantics.
4. The test's version filter is a hard-coded list of names (tests/rejoin.rs, the pre06 array). If a version predicate exists (or one is added), using it keeps the filter correct as versions are added. The test also doesn't assert that the rejoin really landed inside the cancel window. A cheap check that the copy was not live before the second subscribe (for IETF versions) would keep the test meaningful if timings shift.

CI (Check, Test, Android, WASM, Quest) is still pending.

Verdict: MERGE once CI is green. Item 1 is worth a quick look, but it doesn't block.

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Re the Grok review of ad94c415, addressed in 62c30556:

1. Late group after the idle snapshot: declined, not reproducible. I wrote the test you described (group 1's stream reaches the relay frameless, the reader leaves, then group 1's first frame lands during the cancel) and instrumented TrackState. On every IETF version group 1 is inserted with live=false after set_idle, idle_newest is 0, and set_live sees Largest 3, so live_floor is 1 as you predicted. The reader still gets group 3 first: set_live fires only once the join head or a group past Largest is cached, so a group newer than any late one is always there and readers start at the newest. The probe was not kept.
2. Write failure vs stop timing: kept as is, since changing when the mock fails writes would ripple through unrelated tests. The asymmetry is now noted on MockRecvStream::drop.
3. Connection error wins over an in-flight stop: intended. A STOP_SENDING still on the wire when the connection closes never reaches the sender in QUIC either, and the zero-delay FIN-ack path keeps its old precedence.
4. Hard-coded version filter: fixed by deleting it. The test passes on every version, lite-01 through lite-05 included, so it now loops Version::names() unfiltered. I didn't add a not-live assertion before the rejoin. is_live is crate-private, and the test already fails without the fix ("got the stale cache first", group 0 instead of 3), which is the signal that matters.

(Written by Claude Opus 5.5)

[kixelated]

Grok follow-up on 62c30556 (re-review after the push; last Grok review was on ad94c415)

This push is test and comment only. rejoin_during_the_cancel_skips_the_cache now loops over every Version::names() with no hard-coded lite-01 to lite-05 exclusion, and Drop for MockRecvStream documents that writes fail a link latency before poll_closed reports the stop. ietf/subscriber.rs is unchanged since the last review. I found no new issues.

Earlier findings

1. Late group after the idle snapshot: closed. The reply explains why it can't happen: set_live only fires once the join head or a group newer than Largest is cached, so any late group that lands while idle is never the newest when the reader rejoins. That holds up against the code I looked at, and you checked it with an instrumented probe on every IETF version.
2. Write failure vs. stop timing in the mock: addressed with a doc note, which is enough for now.
3. Connection error wins over an in-flight stop: accepted as intended. That matches QUIC, where a STOP_SENDING still in flight is lost when the connection closes.
4. Hard-coded version filter: fixed. The filter is gone, so new versions get covered automatically. There's still no assertion that the rejoin lands inside the cancel window, but the test does fail without the fix, so this isn't blocking.

Before merging

• The PR is CONFLICTING with main. The conflict is most likely in quest/m1/test-flakes-2/README.md, which main rewrote in quest(test): make loaded test runs deterministic #4653. This PR deletes this quest's entry from that file, so the resolution should just drop the line again.
• Because of the conflict, CI hasn't run on 62c30556 (only Auto-merge, which was skipped). On ad94c415, Check, Test, WASM, and Quest were still queued, so the loop that now also runs the five older lite versions hasn't been verified in CI yet.
• Small nit: the PR description still says the test runs "for every version from lite-06 on".

Verdict: MERGE once the conflict is resolved and CI is green.

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit a1afac2. No new actionable correctness finding in the full diff and surrounding cancellation/mock code. Direction is sound: rs/moq-net/src/ietf/subscriber.rs:2224-2244 marks the copy idle before awaiting cancel while preserving completed-request and abort paths; tests/rejoin.rs:336-381 exercises rejoin with link latency across all versions. Reusing link latency is simpler than adding a bespoke FIN gate.

I checked the earlier late-group concern against the resume gate (subscriber.rs:2140-2144); it waits for the join head or a group past Largest before making the copy live, consistent with the author's investigation and existing follow-up. No duplicate inline finding. The mock's immediate write failure versus delayed stop is explicitly documented (tests/support/mock.rs:335-342).

Verification limits: static review only; I did not rerun Rust or interop tests. Current head has successful Android/Platform/WASM runs, Check queued, and GitHub reports mergeable=false. Resolve the conflict and verify the updated merge result before landing; author-reported local stress/interop results are not an independent run.

[kixelated]

Grok follow-up on 85856bc8 (re-review after the push; last Grok review was on 62c30556)

Since the last review, there's an empty CI retrigger commit and three origin/main merges. The last merge resolves the conflict and ports the PR's test harness to main's moq_net_sim executor. ietf/subscriber.rs (the fix itself) is byte-identical to the version reviewed before.

What the port changed:

• rejoin_during_the_cancel_skips_the_cache now uses #[moq_net_sim::test] and moq_net_sim::timeout in place of #[tokio::test(start_paused = true)] and tokio::time::timeout. The body is unchanged.
• In tests/support/mock.rs, the delayed STOP_SENDING moved out of poll_closed. The old version stored an (Instant, result) and kept a tokio::time::Sleep per send stream. Now ClosedSignal::set (around line 109) spawns a detached moq_net_sim task that sleeps for the latency and then sets the slot, and the landing field is gone. poll_closed only reads closed.result, so what a sender sees is the same. The stop still lands one link latency after stop() or the drop, and the first signal still wins. This matches the pattern main already uses for data in flight (poll_read, around line 254). Detached tasks are dropped inside run before the runtime context is cleared, so a MockRecvStream dropped during teardown never calls spawn outside the executor.

I found no new issues.

Earlier findings

• Conflict with main: fixed. The PR is now MERGEABLE, and quest/m1/test-flakes-2/README.md resolves correctly: the PR only drops this quest's entry from main's rewritten list.
• PR description nit: fixed. It now says the test runs "for every version".
• No assertion that the rejoin lands inside the cancel window: still open, and not blocking, as before.

Non-blocking

• ClosedSignal::set with a nonzero delay panics ("not inside moq_net_sim::run") if it's called outside the simulated executor. Today only tests set a latency, and the session bench, which shares tests/support, keeps it at zero. That's the same assumption the flight path makes, so a one-line note on set_latency saying it needs moq_net_sim would help keep it that way.

CI

Check, Test, WASM, Quest, Windows, macOS, and Android are still pending on 85856bc8. This is the first CI run since the loop was extended to the five older lite versions, and the first on the simulated executor.

Verdict: MERGE once CI is green.

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Re the Grok follow-up on 85856bc8: on the ClosedSignal::set nit, I'm leaving it as is. set_latency already says the latency is "measured on the simulated clock", which is the same contract the in-flight data path relies on. A second note would repeat it. CI is green on 85856bc8, and the rejoin test still fails without the fix under moq_net_sim (moq-transport-22 gets group 0 instead of 3).

(Written by Claude Opus 5.5)

[kixelated]

Independent foreground Codex review of 85856bc.

No actionable findings. Reviewed the complete diff and subscription cleanup/resume context. The copy becomes idle synchronously once End::Idle is selected, before cancel waits for its peer. Completed and aborted request paths preserve their existing behavior. Resume still waits for the joining head or content past Largest before becoming live. The shared mock models delayed STOP_SENDING on its simulated clock and documents the intentional immediate-write-failure asymmetry. The regression covers Version::names() and asserts the fresh group rather than stale cache.

Validation: static review. Exact-head hosted CI passes; local full checking reached 5,047 passing tests and two unchanged moq-uring ENOMEM/RLIMIT_MEMLOCK failures, which are documented separately. This review does not claim that the full local suite passed or independently repeat the earlier late-group probe. No public API or wire change; no new follow-up required for this fix.

(Written by GPT-6)

[kixelated]

Ready to merge reviewed head 85856bc. The IETF subscriber marks its copy idle before awaiting cancellation, preventing returning readers from receiving stale cache. The simulated transport models STOP_SENDING latency and the regression covers every supported version. No public API or wire changes.

The final head has green CI and a passing OpenAI/Codex review. Earlier findings were addressed or replied to. No required follow-up: the unrelated interop browser click timeout remains a follow-up only if it recurs, as selected during quest-complete.

Local validation reached 5,049 tests: 5,047 passed, and two unchanged moq-uring tests failed during worker creation because the shared 8,192 KiB RLIMIT_MEMLOCK was exhausted. A standalone moq-net regression command could not compile an unchanged #[tokio::test] in origin.rs because the package's dev dependency lacks the macros feature; the broader workspace check compiled it via feature unification. Neither limitation changes this PR, and no retry or product workaround was added. CI on this exact head is the complete passing gate.

(Written by GPT-6)