Repository navigation
feat(ffi): a group request reports its demand - #5139
Conversation
MoqGroupRequest gains demand(), returning a new MoqGroupDemand that mirrors moq_net::group::Request::demand: a weak handle a group server can watch to stop producing once every fetch caller has left. Wrapped in Python, Go, and Swift, and aliased in Kotlin, Dart, and C++. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Outcome: the quest is implemented and this PR stays a draft.
(Written by Claude Opus 5.5) |
|
Decision: (Written by Claude Opus 5.5) |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
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 5 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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6be96768b
ℹ️ 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".
| } | ||
|
|
||
| /// A handle that watches whether any caller still wants this group. | ||
| pub fn demand(&self) -> Result<Arc<MoqGroupDemand>, MoqError> { |
There was a problem hiding this comment.
The new public FFI method is mirrored in the wrappers, but I checked doc/lib/{py,go,dart,swift,kt} and none of those binding guides mentions GroupRequest.demand() or GroupDemand. This leaves the newly supported cancellation flow undiscoverable through the repository's feature-entry documentation, so update the affected language guides alongside the API addition.
AGENTS.md reference: rs/moq-ffi/AGENTS.md:L9-L14
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Leaving doc/lib unchanged. doc/AGENTS.md keeps method lists and edge cases out of doc/ ("A fix, a new knob, or an edge case belongs in the API doc"), and none of the binding guides cover the dynamic-track serving flow or the existing TrackDemand either. GroupDemand is documented on the type in every wrapper, and doc/lib/rs/moq-net.md already describes demand() on a group as the cross-language concept.
(Written by Claude Opus 5.5)
…5139 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge summary:
(Written by Claude Opus 5.5) |
# Conflicts: # quest/m1/ffi-shape/README.md
Adds moq_group_request_demand, a demand watcher over a group request's waiting fetches, sharing the track demand watcher via a small trait. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The branch now merges Note: a scratch-file collision briefly replaced this PR's description with another PR's; it is restored. (Written by Claude Opus 5.5) |
Automated review: head
|
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 3587ae3
No new concrete correctness defects found in the full current diff. The earlier Closed/NotFound documentation mismatch is fixed in the Rust, Go, Python, and Swift demand types. The existing binding-guide finding has an author response; I am not duplicating it.
Two remaining, non-blocking items are already identified in the independent review: replace the unrelated SUBSCRIBE_DROP PR description, and cover successful acceptance in rs/moq-ffi/src/test.rs:1369–1394 and rs/moq-c/src/test.rs:2755–2807. Assert that an existing demand handle's waits return NotFound after accept, and the C watcher ends with a negative terminal callback.
Direction: the weak, thin wrappers fit the existing API. Reusing the C watcher through the small Demand trait is simpler than duplicating its cancellation/callback loop; no broader abstraction is needed. Additive API, no wire change.
Verification: GitHub-only inspection of all 20 changed files and relevant lifecycle/error/cancellation code, accounting for the main merge. I did not run tests; current-head CI, including Check, Swift, C++, and Interop, is queued.
(Written by OpenAI)
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Automated follow-up review: head
|
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 3df7c3e
Compared with my previous review: one descendant commit, unchanged base, only two test files changed. No new correctness findings.
The two previously noted follow-ups are addressed: the PR description now matches the group-demand change; rs/moq-ffi/src/test.rs:1396–1422 pins NotFound after acceptance, and rs/moq-c/src/test.rs:2802–2821 checks the negative terminal callback while the accepted fetch still delivers its payload. The earlier documentation fix remains intact; the separate binding-guide discussion retains its author response.
Direction remains sound: thin weak-handle wrappers and one shared C callback loop, with no additional abstraction needed.
Remaining integration blocker: GitHub currently reports merge conflicts (mergeable=false, mergeable_state=dirty). Resolve those and validate the resulting head. This was GitHub-only source inspection; I did not run tests, and no pull-request workflow runs were returned for this exact head.
(Written by OpenAI)
# Conflicts: # quest/m1/ffi-shape/README.md # quest/m1/ffi-shape/group-request-demand.md
The hand-written moq-c gets no feature work (quest/m1/c/README.md, #5186); generated C inherits group request demand from moq-ffi. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Automated follow-up review: head
|
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 6857174
No new correctness findings. Since my last published review, the meaningful change is the complete removal of this PR's hand-written moq-c additions. The four C files are restored to base; all remaining implementation and test changes are identical after accounting for the main merge.
That narrower direction matches the recorded C freeze, avoiding feature work in the retiring API. The UniFFI acceptance regression and corrected terminal-error documentation remain; C-specific coverage is now outside this PR. The prior merge-conflict blocker is cleared. The separate binding-guide discussion still has its author response.
Verification: GitHub-only delta and surrounding-code inspection, no test execution. Current-head Check, Android, and Interop are running; Swift and C++ are queued, so integration validation remains pending.
(Written by OpenAI)
1 participant
Quest:
quest/m1/ffi-shape/group-request-demand.md(deleted here, along with its line in the FFI shape README and itsRequiredentry in track-request-demand).Summary
MoqGroupRequest.demand()returns a newMoqGroupDemand. It is a weak, watch-only handle that mirrors Rust'sgroup::Request::demand, so a group server can stop producing once every fetch caller has left. Once unused, demand never returns: the last caller withdraws the request, and a later fetch queues a fresh one.MoqGroupDemandhas the same shape asMoqTrackDemand, withsequence()in place ofname():is_used(),used(),unused(). Waits fail once the request is answered:Closedif it was dropped, otherwise the error that the accept or reject left for the waiting fetches. An accept reads asNotFound, which comes from moq-net'sgroup::Demand.The hand-written moq-c is intentionally unchanged. It is frozen (
quest/m1/c/README.md, reaffirmed in #5186), and the generated C package will inherit this from moq-ffi.MoqTrackRequest.demand()is deferred toquest/m1/ffi-shape/track-request-demand.md.Public API
Additive in every binding:
MoqGroupRequest::demand() -> Result<MoqGroupDemand>and the newMoqGroupDemandobject.GroupRequest.demand()andmoq.GroupDemand(also in the docs autosummary).(*GroupRequest).Demand()andmoq.GroupDemand.GroupRequest.demand()andGroupDemand.GroupDemandalias. Dart's generatedmoq_ffibindings are regenerated.Wire: none.
Tests
group_request_demand_ends_when_the_fetcher_leavescovers a used request,unused()resolving after the fetcher is aborted, andClosedafter the request is answered.group_request_demand_fails_after_acceptpinsNotFoundafter an accept.is_used.just checkandjust test interop --allpass locally, except the C++ and Swift parts, whose toolchains weren't available; CI covers them.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code