Fix reading catalogs#1404
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughConsumer now tracks an internal finished flag and implements a state-aware poll loop: it repeatedly calls TrackConsumer::poll_next_group, marks finished when the track yields Ready(None), and either returns Ok(None) if no group exists or polls the acquired group's poll_read_frame. When a frame arrives it is decoded into a Catalog and returned; otherwise poll_next can remain Pending even after the track finished. A comprehensive test module validates pending behavior, ordering, superseding, and empty-track end semantics. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ 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-mux/src/catalog/consumer.rs (1)
50-103: ⚡ Quick winAdd test coverage for the new
Ok(None)end-of-stream and multi-group paths.The two added tests validate the basic happy path and the "track finished before frame written" race. Two gaps remain on paths this PR introduces or changes:
- The new
Poll::Ready(None) => Poll::Ready(Ok(None))branch inpoll_nextis not exercised. A small test that finishes both the group and the track, drains the catalog, and then asserts the subsequentpoll_nextreturnsOk(None)would lock in end-of-stream behavior.- With the group-tracking logic removed, there is no test asserting that when multiple catalog groups are written, the consumer surfaces the newest catalog rather than an older one. This is the semantic the previous implementation explicitly preserved, and tying down a test would prevent silent regressions if the underlying
moq_litebehavior shifts.🤖 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-mux/src/catalog/consumer.rs` around lines 50 - 103, Add two unit tests in the existing test module: (1) a test (e.g., waits_for_end_of_stream_returns_ok_none) that creates a track via Catalog::default_track().produce(), a Consumer::new(track.consume()), appends a group, writes a catalog frame, then finishes the group and finishes the track, drains the single catalog via consumer.poll_next(&waiter) and finally calls consumer.poll_next(&waiter) again to assert it returns Poll::Ready(Ok(None)) to cover the end-of-stream branch in Consumer::poll_next; (2) a test (e.g., surfaces_newest_catalog_from_multiple_groups) that appends two groups to the same track, writes an older catalog payload to the first group and a newer catalog payload to the second group, finishes both groups (and optionally the track), then asserts successive consumer.poll_next(&waiter) yields the newer catalog (the last-written group) to lock in the multi-group semantics now expected by Consumer::poll_next; reuse existing helpers like Catalog::default_track, append_group, write_frame, finish and conducer::Waiter::noop to set up and poll.
🤖 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-mux/src/catalog/consumer.rs`:
- Around line 50-103: Add two unit tests in the existing test module: (1) a test
(e.g., waits_for_end_of_stream_returns_ok_none) that creates a track via
Catalog::default_track().produce(), a Consumer::new(track.consume()), appends a
group, writes a catalog frame, then finishes the group and finishes the track,
drains the single catalog via consumer.poll_next(&waiter) and finally calls
consumer.poll_next(&waiter) again to assert it returns Poll::Ready(Ok(None)) to
cover the end-of-stream branch in Consumer::poll_next; (2) a test (e.g.,
surfaces_newest_catalog_from_multiple_groups) that appends two groups to the
same track, writes an older catalog payload to the first group and a newer
catalog payload to the second group, finishes both groups (and optionally the
track), then asserts successive consumer.poll_next(&waiter) yields the newer
catalog (the last-written group) to lock in the multi-group semantics now
expected by Consumer::poll_next; reuse existing helpers like
Catalog::default_track, append_group, write_frame, finish and
conducer::Waiter::noop to set up and poll.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3edb61a9-c2ed-4263-81e1-bed4fb3ecc13
📒 Files selected for processing (1)
rs/moq-mux/src/catalog/consumer.rs
Qizot
force-pushed
the
fix-catalog-consumer
branch
from
c138466 to
86ee91b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rs/moq-mux/src/catalog/consumer.rs (1)
82-234: 💤 Low valueTest coverage is solid; minor nit on module naming.
The six tests cover the meaningful state transitions (pending-before-payload, pending-after-track-finish, latest-wins, newer-pending-preferred-over-older-ready, retained-pending-superseded, empty-finished-track). Nicely scoped.
Optional nit:
mod test→mod testsis the more conventional Rust idiom for a test submodule, e.g., as recommended by Rust API guidelines and used throughout the standard library. Trivial rename if you care.♻️ Proposed rename
#[cfg(test)] -mod test { +mod tests { use std::task::Poll;One coverage gap you might consider (also optional): there is no test for
group.poll_read_framereturningPoll::Ready(None)while the track is still open (i.e., a group that finishes without ever delivering a frame). The branch on Lines 51-54 currently isn't exercised by any of the new tests. Easy to add if you want full branch coverage.🤖 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-mux/src/catalog/consumer.rs` around lines 82 - 234, Rename the test module from "mod test" to the conventional "mod tests" (update the #[cfg(test)] mod declaration) and keep all existing tests (e.g., waits_for_pending_catalog_group_payload, returns_latest_complete_catalog_group, retained_pending_group_is_superseded_by_newer_group) intact; optionally add one more unit test that constructs a track and group via Catalog::default_track().produce(), calls track.append_group() then immediately finishes the group without writing a frame (exercise group.poll_read_frame -> Poll::Ready(None) while track still open) and assert Consumer::poll_next returns the expected Ready(Ok(None)) or pending behavior to cover the unexercised branch.
🤖 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-mux/src/catalog/consumer.rs`:
- Around line 82-234: Rename the test module from "mod test" to the conventional
"mod tests" (update the #[cfg(test)] mod declaration) and keep all existing
tests (e.g., waits_for_pending_catalog_group_payload,
returns_latest_complete_catalog_group,
retained_pending_group_is_superseded_by_newer_group) intact; optionally add one
more unit test that constructs a track and group via
Catalog::default_track().produce(), calls track.append_group() then immediately
finishes the group without writing a frame (exercise group.poll_read_frame ->
Poll::Ready(None) while track still open) and assert Consumer::poll_next returns
the expected Ready(Ok(None)) or pending behavior to cover the unexercised
branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ee442a2-2c99-4d94-9b31-851ee47cdb1a
📒 Files selected for processing (1)
rs/moq-mux/src/catalog/consumer.rs
Track finished state inside poll_next via a local variable. The underlying TrackConsumer reports Ready(None) idempotently once finished, so re-deriving it each call is equivalent to caching it in a field. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 participants
It happened that the previous logic for reading the catalogs had a race condition where a pending frame in a group could case a group drop in
self.group.take().This PR changes the behaviour of when we drop old groups.