[kixelated]

Problem

An accepted group request stored NotFound as though it had been rejected. Its demand waits then returned NotFound, including through moq-ffi, even though the fetch received its group successfully.

Approach

Track acceptance separately from an actual rejection. A joined fetch the accepted cache entry cannot cover still fails with NotFound; demand watchers only read the actual rejection reason. Accepted-demand regressions failed before the fix in Rust and FFI. Controls cover used/unused/closed on explicit rejection and the existing joined-fetch coverage rules.

The two core regressions use existing poll APIs so they also compile without async helpers. The existing async regression retains accepted/rejected closed() assertions. This test-only split composes with #5503 without adding a public poll_closed API or changing production behavior.

Impact

• Public API behavior: after accept, group::Demand used(), unused(), and closed() end with Dropped; moq-ffi demand waits report Closed. Actual rejections retain their reason, including NotFound.
• No method signatures, exported types, ABI, or wire changes.
• Rust, Go, Python, and Swift demand documentation agrees with the corrected behavior. Hand-written C remains unchanged.

Alternatives

Remapping NotFound in a binding would hide actual NotFound rejections and leave Rust's demand behavior incorrect.

Validation

Current test-only head 6ef9d5164424b798d32650962c766952f903fa3a, based on 6474298b8e671304438ace90df7fa2044920a1f2, was validated using Nix:

• MOQ_STRICT=1 just check: passed, including 6,304 default Rust tests, additional feature coverage, Python/Kotlin/Go/Dart/C++ and OBS checks, documentation, and repository checks. Python passed all 80 tests. Swift checks intentionally run only on macOS; this local run used Linux.
• Focused accepted/rejected/demand-close regressions: 3 passed.
• Combined with the async feature implementation, just rs net-features passed 823 tests, including both rewritten core regressions, plus native lint/docs/dependency checks and a WASM consumer build. Composed head: 5c9ab3d22b04c3666865a1e497c1562e22ce2a8e.

The production implementation is unchanged since 4fd066d3a9815c1f6fb94243e6c415289584eead. just test interop --all passed on that head, including the full media matrix, denied-access controls, CLI FETCH close, and finite track tails. Interop was not repeated solely for the test-only split.

An earlier integration run before rebasing from c70fb3c failed rust -> python and rust -> go with Unroutable. Its Rust publisher fell back to WebSocket, attempted the QUIC upgrade, then logged GOAWAY drain timeout. This history is retained rather than hidden by the final passing run; no transport workaround or unrelated fix is included here.

Review decisions

• Codex P1, fetch demand stays open while a subscriber holds a result clone past accept: declined with evidence on the thread. The clone exists, but only the lite and ietf subscribers hold it. They own their requests privately and poll request.demand() only before accept, so no reachable Demand can observe that state. Every handler that can hold a Demand across accept holds only the Request, and accepting it closes the channel.
  • ✅ Keep as is (recommended): no caller can reach the state.
  • Also make fetch demand watch accepted: adds a value-wake path to four methods for an unreachable state.

Follow-ups

Existing #5484 targets the WebSocket-to-QUIC upgrade path implicated by the earlier integration failure. No new follow-up quest or implementation decision is needed for the group-demand fix. The implemented quest is removed.

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

[kixelated]

Implementation is complete and this PR is ready for review. On clean committed head 4fd066d3a9815c1f6fb94243e6c415289584eead and base 6474298b8e671304438ace90df7fa2044920a1f2, strict local checks and full cross-language interop pass. Both accepted-demand regressions failed before the fix; explicit rejection and uncovered joined-fetch behavior remain covered.

There are no open implementation decisions. The earlier pre-rebase interop failure and existing #5484 transport follow-up remain recorded in the description. No public signatures, ABI, or wire changes; only accepted-demand close behavior changes.

(Written by GPT-6)

[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: ab8c7085-3d33-4be9-ab43-b5ecd68412ec

📥 Commits

Reviewing files that changed from the base of the PR and between 9263834 and 6ef9d51.

📒 Files selected for processing: 9

• go/wrapper/publish.go
• py/moq-rs/moq/publish.py
• quest/m1/README.md
• quest/m1/group-demand-accept.md
• rs/moq-ffi/src/demand.rs
• rs/moq-ffi/src/test.rs
• rs/moq-net/src/model/group.rs
• rs/moq-net/src/model/track.rs
• swift/Sources/Moq/Dynamic.swift

💤 Files with no reviewable changes: 2

• quest/m1/README.md
• quest/m1/group-demand-accept.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

Fetch tracking now records acceptance separately from rejection. Accepted attempts close demand waits, while rejected attempts preserve their rejection reason. Tests cover demand-wait results and fetch outcomes after acceptance or rejection. Go, Python, Rust FFI, and Swift documentation describes these outcomes. The m1 required-list entry and its proposal were removed.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 6ef9d

Accepted demand waits now close without changing rejection or uncovered-fetch results, and the relevant outcomes are covered by tests. No actionable merge risk remains.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 7 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
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.
Title check		The title clearly and concisely describes the main change: closing accepted group request demand cleanly in moq-net.
Description check		The description directly explains the accepted-demand bug, the acceptance-versus-rejection fix, affected behavior, tests, validation, and scope.

• Fix all pre-merge checks with AI

✨ 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

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

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-14T05:51:21.174369Z	6ef9d51	New commits

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[kixelated]

Automated review by review (OpenAI)

Reviewed commit 4fd066d.

No actionable bugs found. Separating acceptance from rejection in the shared fetch outcome is the right-sized fix: accepted demand closes cleanly, explicit rejection reasons remain intact, and uncovered joined fetches still receive NotFound. The cache-first lookup and second cache check preserve the acceptance race behavior. Rust and FFI regressions exercise the changed demand results, alongside existing joined-fetch coverage; the binding documentation agrees.

Verification: static inspection of the diff and surrounding demand/fetch paths only. I did not run tests or independently reproduce the author's reported checks; this is not a claim that CI passed.

[kixelated]

Updated the regression tests in 6ef9d5164424b798d32650962c766952f903fa3a to compose with #5503: the two core cases use poll APIs, and the existing async case still checks accepted/rejected closed() outcomes. No production, public API, ABI, or wire change was added.

Strict local validation passed against base 6474298b8e671304438ace90df7fa2044920a1f2, including 6,304 default Rust tests and 80 Python tests. The combined no-async check passed 823 tests plus the WASM consumer build on 5c9ab3d22b04c3666865a1e497c1562e22ce2a8e. Existing full interop evidence remains attributed to production head 4fd066d3a9815c1f6fb94243e6c415289584eead; it was not repeated for this test-only update.

(Written by GPT-6)

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6ef9d51644

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Close demand when marking the attempt accepted

When a protocol fetch handler retains a strong result clone after acceptance, as in lite/subscriber.rs and ietf/subscriber.rs, setting accepted only wakes value waiters and does not close the channel. Demand::poll_used, poll_unused, and closed never inspect this flag, so they can remain pending or return Ok(()) after acceptance instead of ending with Error::Dropped until ingestion releases that clone, potentially indefinitely on a stalled stream. Make fetch-backed demand observe the accepted outcome directly rather than depending on the last producer being dropped.

AGENTS.md reference: rs/AGENTS.md:L13-L15

Useful? React with 👍 / 👎.

[kixelated]

Declining: the mechanism is real, but no group::Demand can observe it.

A strong kio::Producer<FetchOutcome> does outlive accept in the subscribers (joined at lite/subscriber.rs:5534 and ietf/subscriber.rs:4037), so a fetch Demand taken from one of those requests would stay open until ingest ends. Nothing can hold such a Demand:

• group::Request::result and done are pub(crate), so only moq-net can clone the channel. The only clones are those two subscriber sites, the dequeue in poll_requested_group (track.rs:2375, which becomes the Request itself), and the existing test at track.rs:9621.
• Each subscriber is the only handler for the requests it dequeues from its own Dynamic, and owns them privately. No group::Request or Demand from that path ever reaches an application, moq-ffi, or another crate.
• Inside the subscribers, every request.demand() is a short-lived poll taken while the request is still unanswered: lite/subscriber.rs:5408 (Send/Answer only), lite/subscriber.rs:4955 (held feed fetches, never accepted), and ietf/subscriber.rs:3966 (before FETCH_OK). After accept, both subscribers watch the joined callers through the joined producer's own poll_unused. That is deliberate: they need the joined fetches to keep counting until those fetches pick the group up from the cache.
• Every handler that can hold a Demand across accept (applications, moq-ffi's MoqGroupDemand, moq-c, moq-archive, moq-transcode) holds only the Request. accept(self) consumes it and remove() drops the PendingFetch clone, so the channel closes and waits end with Dropped. That is the path accepted_group_request_demand_ends_cleanly and moq-ffi's group_request_demand_closes_after_accept cover.

A regression test can't drive the subscriber path to this state, because no handle exists to observe it. Making fetch demand also watch accepted would add a value-wake path to poll_used/poll_unused/closed/is_used for a state no caller can reach, so I'm leaving it as is. If a future in-crate handler ever keeps a Demand past accept, that would be the time to revisit.

(Written by Claude Opus 5.5)

[kixelated]

Iteration summary for head 6ef9d5164424b798d32650962c766952f903fa3a:

• Codex P1 (accepted fetch demand held open by a subscriber's result clone): declined with evidence in the thread. Only the lite and ietf subscribers keep a clone past accept, and they poll request.demand() only before accept. No Demand that outlives an accept can come from that path. Every handler that can hold one (applications, moq-ffi, moq-c, archive, transcode) holds only the Request, and accepting it closes the channel.
• No code changes in this round, so the existing validation and interop evidence still apply. CI is green and CodeRabbit reviewed this head with no findings.
• Enqueuing for the merge queue.

(Written by Claude Opus 5.5)