Repository navigation
docs(go): define deterministic reservation release with Update(0) - #6424
Conversation
Quest-Claim: e44ca1a110b698a3
kixelated
changed the title
Co-Authored-By: GPT-6 <noreply@openai.com>
|
Completed the release-audit quest with the existing Final (Written by GPT-6) |
|
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 29 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 |
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. |
# Conflicts: # quest/m1/release-audit/README.md
![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: 7e67abdca3
ℹ️ 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".
| replacement := bandwidth.Reserve(demand, 1) | ||
| // Even a retained predecessor must relinquish the allocation explicitly. | ||
| // Priority makes this independent of the live estimate's exact value. | ||
| grant(sibling, 0) |
There was a problem hiding this comment.
On a fresh connection this assertion races the asynchronous estimate forwarding: Session::new creates the send-bandwidth producer without seeding it (rs/moq-net/src/session.rs:345-347), and the connection publishes Connected before run_session forwards the first sample (rs/moq-tokio/src/connection.rs:875-882). Consequently Grant() can still return nil here, causing the helper to fail before the release behavior is tested; the context timeout cannot help because these reads are nonblocking. Synchronize on estimate availability or use a deterministic injected estimate so this remains a high-signal merge-gate test.
AGENTS.md reference: AGENTS.md:L28-L28
Useful? React with 👍 / 👎.
Problem
Go's reservation docs say to drop the handle to return bandwidth, but collection does not promptly finalize it. Replacing a reservation on a still-subscribed track can keep its previous share and probe ceiling until collection.
Approach
Document the existing
Update(0)contract: it immediately relinquishes the share and automatic probe ceiling while keeping the track and reservation usable. Prefer updating one reservation to replacing it. Public Go coverage retains the old handle across replacement without forced GC, checks sibling allocation, repeated release, and reuse; deterministic native coverage checks the matching probe target. Complete the release-audit quest.Impact
Alternatives
An explicit disposal API is unnecessary for the intended allocation-release contract. Reusing
Update(0)avoids expanding the public surface.Validation
just ci: passed on the final tree, including 8,253 native tests, optional-feature suites, documentation, quests, and race-enabled Go wrapper tests.just go checkand focused native allocation/probe-target regression: passed.quest check --base origin/mainandgit diff --check: passed.Follow-ups
None. This is a deterministic release contract, not evidence of a permanent native leak or a measured performance regression.
(Written by GPT-6)