Don't warn on Alive::drop if aborted - #2580
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rs/moq-net/src/model/group.rs (1)
818-866: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse one shared warning-capture helper.
Both test modules add the same tracing
Subscriberimplementation. Keep one test-only helper and pass the expected warning message.
rs/moq-net/src/model/group.rs#L818-L866: replace the local helper with the shared helper.rs/moq-net/src/model/track.rs#L2756-L2805: replace the local helper with the shared helper.As per coding guidelines, "Refactor awkward internal shapes while making changes ... avoid duplicated one-off helpers."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-net/src/model/group.rs` around lines 818 - 866, Introduce one shared test-only tracing warning-capture helper that accepts the expected warning message, then replace the duplicated local Subscriber-based helpers in rs/moq-net/src/model/group.rs:818-866 and rs/moq-net/src/model/track.rs:2756-2805 with calls to it. Preserve each test’s existing warning-counting behavior while removing both local helper implementations.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@rs/moq-net/src/model/group.rs`:
- Around line 818-866: Introduce one shared test-only tracing warning-capture
helper that accepts the expected warning message, then replace the duplicated
local Subscriber-based helpers in rs/moq-net/src/model/group.rs:818-866 and
rs/moq-net/src/model/track.rs:2756-2805 with calls to it. Preserve each test’s
existing warning-counting behavior while removing both local helper
implementations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6fd41575-0295-4a51-941b-c27ea178de65
📒 Files selected for processing (2)
rs/moq-net/src/model/group.rsrs/moq-net/src/model/track.rs
|
@codex review |
![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: 8fac137f45
ℹ️ 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".
| /// Count `track::Producer` unfinished-drop WARN events while running `f`. | ||
| /// Uses only the existing `tracing` dependency (no tracing-subscriber). | ||
| fn count_drop_warnings(f: impl FnOnce()) -> usize { |
There was a problem hiding this comment.
Move this warning-counting subscriber into a shared test helper and pass the expected message as an argument. This implementation duplicates nearly the entire helper added in group.rs, so future tracing or matching fixes must be kept synchronized across two copies, contrary to the repository's requirement to generalize helpers instead of copying them.
AGENTS.md reference: AGENTS.md:L104-L110
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I did this in a separate commit in case this is a "isolation is more important than deduplication" codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rs/moq-net/src/model/test_tracing.rs`:
- Around line 41-44: Update Count::enabled to accept only metadata whose level
is exactly Level::WARN, rather than using an ordering comparison that also
enables ERROR events. Leave the rest of the Subscriber implementation unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d8d661c6-ff20-4528-9481-20c002abdc84
📒 Files selected for processing (4)
rs/moq-net/src/model/group.rsrs/moq-net/src/model/mod.rsrs/moq-net/src/model/test_tracing.rsrs/moq-net/src/model/track.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rs/moq-net/src/model/track.rs
CI failed on import ordering in group.rs and test_tracing.rs. Also match Level::WARN exactly so ERROR events are not counted. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-Authored-By: Codex <codex@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ec14cf3a9
ℹ️ 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".
| Run the matching recipe by hand when you touch this code, and if you can't (no such host), say plainly in the PR that it's uncompiled rather than implying CI covered it. | ||
|
|
||
| - **`just rs loom` is a manual gate. Run it by hand whenever you touch kio's refcount/waiter plumbing (`lock.rs`, `producer.rs`, `consumer.rs`, `weak.rs`, `waiter.rs`) or moq-net's model layer (`model/`), and mention the result in the PR.** Nothing else will run it: `--cfg loom` swaps kio's Mutex/atomics for loom's instrumented ones, which rebuilds the whole dependency tree and can't share artifacts with a normal `cargo test`, so it's deliberately outside `check`/`ci`. Budget about a minute of model checking on top of that build. The search is exhaustive on purpose, so don't reach for `preemption_bound` to speed it up; the recipe already buys the speed back with `--release`, which matters here because a model check reruns the body once per interleaving. | ||
| - **`just rs loom` model-checks concurrent handoffs in kio and moq-net.** It stays outside `check`/`ci`: `--cfg loom` swaps kio's Mutex/atomics for loom's instrumented ones, which rebuilds the whole dependency tree and can't share artifacts with a normal `cargo test`. Use it when developing or diagnosing concurrent handoffs. Budget about a minute of model checking on top of that build. The search is exhaustive on purpose, so don't reach for `preemption_bound` to speed it up; the recipe already buys the speed back with `--release`, which matters here because a model check reruns the body once per interleaving. |
There was a problem hiding this comment.
Restore the requirement to run just rs loom for changes to kio refcount/waiter plumbing or moq-net's model layer. This replacement makes the model checker optional even though the rs/justfile recipe remains outside both check and ci, so future concurrent-handoff changes can follow the documented workflow without running the repository's only exhaustive race check. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
2 participants
Alive::drop warns if a producer didn't finish with either finish() or abort(). The warning says "track::Producer drooped without finish() or abort()"... but it doesn't catch the case when abort() is set, since abort() doesn't set
final_sequence.The code is mostly self-explanatory but comments were included anyways. I could remove them if asked.