fix(net): cancel abandoned IETF group fetches

[kixelated]

Problem

A Rust subscriber kept an upstream IETF group FETCH alive after its last reader left, both before FETCH_OK and during a partial answer.

Approach

Watch pending fetches and accepted-group readers, cancel the request using its negotiated draft, and wake stalled data handlers with Error::Cancel. Completion wins over abandonment so whole groups remain cached, including when the publisher resets the request after the group is written. Remove the completed quest and document cancellation.

Impact

• Public API: none.
• Wire format: none. Emit existing FETCH_CANCEL on drafts 14–16 and existing STOP_SENDING plus RESET_STREAM on drafts 17–19.
• JavaScript remains unchanged because it never requests an IETF FETCH.

Alternatives

Filling the cache after all readers leave was rejected by the quest plan. This uses the existing poll_unused API rather than waiting on the Demand refactor.

Validation

Paused-clock regressions cover abandonment before FETCH_OK, after FETCH_OK, after the first frame, a complete group when the reader leaves, and a complete group followed by a request reset. Each fails on the code it guards. All ietf::subscriber unit tests pass after merging main.

Follow-ups

Migrate the request watch to demand().poll_unused with Demand everywhere.

(Written by GPT-6, updated by Claude Opus 5.5)

[kixelated]

Implemented at cdc8dacab61188a18f7794a28f80c0c7d1edb586 using main's existing poll_unused API. Three paused-clock regressions reproduced abandonment failures before FETCH_OK, after FETCH_OK, and after a complete first frame; completion coverage preserves a whole cached group.

The first fixed-source compile found a slot predicate return-type error, corrected in the final commit. Final-head focused tests, quest check, just check, and just test interop --all remain pending in an independent detached Nix runner. This PR stays a draft; no validation success or merge readiness is claimed. Public API and wire format: none. Existing cancellation messages/stream signals now fire when demand disappears.

Follow-up: migrate the IETF demand watches to demand().poll_unused when this main change integrates into dev alongside #4708.

(written by GPT-6)

[kixelated]

At final head cdc8dacab61188a18f7794a28f80c0c7d1edb586, all 10 focused IETF lifecycle tests pass. This includes before/after FETCH_OK cancellation and a stalled partial response across drafts 14–19, plus completed-group caching when its reader leaves. The tests use paused time and explicit polling, without sleeps, retries, or timeouts.

Final-head quest check, just check, and just test interop --all are running in a verified detached Nix process. CI is in progress, and this PR remains a draft. Public API and wire format remain unchanged; existing cancellation signals are now emitted when the last reader leaves. Dev integration should migrate the demand watches alongside #4708.

(written by GPT-6)

[coderabbitai]

Warning

Review limit reached

You'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 25 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 7b7da895-ce12-41af-8015-fd1018b9141e

📥 Commits

Reviewing files that changed from the base of the PR and between 5fc2b57 and b6e875c.

📒 Files selected for processing (5)

• doc/concept/standard.md
• quest/m1/README.md
• quest/m1/demand-everywhere.md
• quest/m1/ietf-fetch-abandonment.md
• rs/moq-net/src/ietf/subscriber.rs

• 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]

Automated review by review (OpenAI)

Reviewed commit cd153a6.

No actionable bugs found in the inspected changes. The approach is worthwhile: watching both pending fetchers and accepted-group readers closes the abandonment gap, while the shared slot lock protects completed groups from cancellation. Reusing the negotiated draft's existing cancellation signals keeps the scope small. The merge retains the cancellation logic and correctly preserves current main's FETCH_OK retention metadata.

Verification: static inspection of the diff, previous version, surrounding request/group lifecycle code, and cancellation/completion regressions. I did not run Rust, interop, or repository-wide tests, and this review does not establish that CI passed.

(Written by OpenAI)

[kixelated]

Review (head cd153a6c7024d0084357f96c055cc0d3377cd417)

Verdict: ITERATE

Useful fix: an abandoned IETF group FETCH now cancels upstream (FETCH_CANCEL on 14–16, STOP_SENDING+RESET on 17–19) instead of staying alive until the subscription ends, with solid paused-clock coverage across drafts and stages. Completion-vs-abandonment via the slot write lock is the right idea.

Blocking

1. abort_unused losing the Used race does not re-arm demand/joined waiters (rs/moq-net/src/ietf/subscriber.rs, post-FETCH_OK wait in run_group_fetch)

slot.poll(waiter, |state| match &**state {
    GroupFetch::Done => Poll::Ready(()),
    _ => {
        if joined.poll_unused(waiter).is_ready()
            && demand.poll_unused(waiter).is_ready()
            && demand.abort_unused(Error::Cancel)
        {
            abandoned = true;
            Poll::Ready(())
        } else {
            Poll::Pending
        }
    }
})

Failure scenario: last reader drops → both poll_unused return Ready → a new consumer attaches before abort_unused → Unused::Used → callback returns Pending. slot.poll then registers only on the slot channel. The Ready poll_unused calls did not leave demand/joined waiters registered, so when that racing reader later leaves, nothing wakes this wait. The FETCH stays up until the group completes or the publisher resets — the leak this PR aims to close.

Lite already handles this: on !abort_unused it continues so the next poll re-checks and re-registers (rs/moq-net/src/lite/subscriber.rs ingest path). Mirror that here — e.g. on Used, call demand.poll_unused / joined.poll_unused again before returning Pending (or structure the wait so a failed abort retries immediately like lite).

Non-blocking

2. Accept-fail / early-exit paths still FIN the request stream (same function, request.accept Err, start_at Err, empty FETCH_OK). On drafts 17–19 this PR’s own comments say FIN is not cancellation; a concurrent cache hit can leave a useless upstream FETCH until it finishes. Pre-existing, but adjacent: consider cancel_group_fetch there too once abandonment is solid.

3. CI still queued at review time (Check/Test/Quest/WASM/Platform). Worth confirming green before merge.

4. Quest follow-up to migrate watches to demand().poll_unused with refactor(net)!: watch demand through demand() and withdraw a fetch with its last caller #4708 is noted; fine to keep out of this PR.

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

[kixelated]

Merge summary:

• Merged main (post trunk flip). Conflicts: ietf/subscriber.rs keeps main's max_cache_duration and adds the joined watch; quest/m1/README.md drops the finished quest line. The new abandonment test gained main's FetchOk::properties field.
• Review fix (b6e875c): the abandonment change aborted the group on any request reset, even after the fetch stream had written it whole, dropping a complete cached group. Restored the Ready-only guard and track abandonment with a flag instead of matching Error::Cancel (a peer reset carrying that code would otherwise send FETCH_CANCEL). New regression a_complete_group_fetch_survives_a_request_reset fails without the guard.
• Reworded the stale "integrates into dev" note in quest/m1/demand-everywhere.md.
• Maintainer approved merging in session; enabling auto-merge on the fresh CI run.

(Written by Claude Opus 5.5)

[kixelated]

Re-review (head b6e875c57f9edd6cfb5a84b5acb84ca81ba4cc52)

Verdict: ITERATE

Follow-up after push b6e875c (vs prior Grok on cd153a6c): request reset after a whole written group no longer aborts the cache. Abandonment is tracked with an explicit abandoned flag instead of matching Error::Cancel, and reset only aborts while the slot is still Ready. New paused-clock coverage for CompleteReset.

Still open (blocking)

1. abort_unused losing the Used race still does not re-arm demand/joined waiters (rs/moq-net/src/ietf/subscriber.rs, post-FETCH_OK wait in run_group_fetch)

slot.poll(waiter, |state| match &**state {
    GroupFetch::Done => Poll::Ready(()),
    _ if joined.poll_unused(waiter).is_ready()
        && demand.poll_unused(waiter).is_ready()
        && demand.abort_unused(Error::Cancel) =>
    {
        abandoned = true;
        Poll::Ready(())
    }
    _ => Poll::Pending,
})

Same failure as before: last reader drops → both poll_unused Ready → a new consumer attaches before abort_unused → Unused::Used → arm returns Pending. Ready poll_unused left no demand/joined waiters, so when that racing reader later leaves, nothing wakes this wait. Lite retries on !abort_unused (continue in the ingest path). Mirror that — on Used, re-poll / structure the wait so a failed abort retries immediately.

Fixed by this push

• Complete group vs request reset: previously a publisher reset that won the wait over Done always called demand.abort(err), wiping a finished group. Guarded now; regression a_complete_group_fetch_survives_a_request_reset covers it.

Still open (non-blocking)

2. Accept-fail / early-exit paths still FIN the request stream (request.accept Err, start_at Err, empty FETCH_OK). On drafts 17–19 FIN is not cancellation; a concurrent cache hit can leave a useless upstream FETCH. Pre-existing / adjacent.

3. CI still queued at review time.

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