fix(stream): detect a rolled log without waiting for the group to end - #3282
Conversation
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. |
A `stream` track is one group, and a second group means records are missing. Every implementation only checked for one at a group boundary, so a publisher that opened a second group and left the first open (the same violation, one step worse) parked the consumer forever on a track that already carried the failure. The consumers now consult the track whenever the held group's read would block: a second group there fails with `Rolled` rather than waiting for a group a broken publisher may never finish. Records already in hand are drained first, and the failure is sticky so a later read cannot report the rest of the log as a whole one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Tagging it inside the race chained a fresh reaction onto the one pending `recvGroup` for every frame the log delivers. Build the tagged promise with the read instead, so a long log costs one. The cached promise then carries the failure on its own: a second group leaves it resolved, so dropping the held group sends every later read back to the same throw, and the sticky flag goes away. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
b5d6a0b to
0cc66c6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0cc66c68b2
ℹ️ 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".
| const frame = group.readFrame(); | ||
| const winner = await Promise.race([frame.then((frame) => ({ frame }) as const), this.#recvGroup()]); |
There was a problem hiding this comment.
When the second group wins this race while the first group remains open, the losing readFrame() cannot be cancelled and stays subscribed to the first group's signals indefinitely. That pending async operation retains the Group.Consumer, whose expiry closure retains the track subscriber, so the exact broken-publisher scenario handled here can keep the subscription and its state alive even after the caller discards the stream consumer. Race group.readable() or another cancellable readiness primitive and only start readFrame() after readiness wins; the identical JSON implementation needs the same correction. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
Review findings on the JS consumers, both from the cached `recvGroup`. Two concurrent `next()` calls await the same promise, so the second took the first's group for a second one and failed a perfectly good log. Refuse the concurrent call the way the window consumer does; Rust gets this from `&mut self`. The read that loses the race stays registered on the held group's signals, which keeps the group consumer, and through its expiry closure this consumer's subscription, reachable after the caller drops it. Closing the group settles that read, so close both it and the second group the failure rejects. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The cleanup had no assertion behind it: dropping the `close()` calls left every test green. Both mirrors report unused once the read fails. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Closes #3260.
Summary
streamtrack is one group; a second group means records are missing, which all four implementations report asRolled/Stream.Rolled. But each only asked the track for another group once the held one ended (poll_recv_group/recvGroupgated ongroup.is_none()). A publisher that opens a second group and leaves the first open (itself the same violation) therefore parked the consumer indefinitely, on a track that already carried the condition the contract says to fail on.Pending; TS drains withtryReadFrame(guarded byskipped, which is the evicted prefixreadFramereports as lagged) before racing the blocking read.poll_nextafterRolledcould returnOk(None), which reads as a cleanly completed log.recvGrouppromise once per consumer, tagged for the race.recvGroupdelivers each group exactly once, so a second call would take a second group off the track and lose the first; and tagging it inside the race would chain a fresh reaction onto that one pending promise for every frame the log delivers. That cached promise is also what makes the failure sticky there: a second group leaves it resolved, so dropping the held group sends every later read back to the same throw, with no flag to carry. Rust polls rather than awaits, so it keeps an explicitrolledflag instead.next()is refused, the wayjs/json/src/window/consumer.tsalready does it. Sharing the onerecvGrouppromise means two concurrent calls resolve to the same group, and the second would take the first's group for a rolled log and fail a valid one. Rust gets this from&mut self.Public API changes
None. No
pub/ exported item added, renamed, or re-signed;Error::RolledandRolledalready existed. Behavior only, which is why this could targetmain, except the code being changed exists only ondev.Wire behavior changes
None on the wire. What changes is when a consumer gives up on a non-conforming publisher: previously never, while the first group stayed open; now as soon as the second group is observed and the first has nothing more in hand.
drafts/draft-lcurley-moq-hang.mdis updated to require that, and to say explicitly that already-received payloads may be yielded first.Test plan
a_second_group_is_reported_while_the_first_is_open/ "a second group is reported while the first is still open"): two groups opened, neither finished. Without the fix the Rust reads returnPendingforever and the TS reads never settle. Each also asserts the failure is sticky, and the TS ones assert both group mirrors are released (dropping theclose()calls turns them red).cargo test -p moq-binary -p moq-json(30 tests) andcargo clippy -p moq-binary -p moq-json --all-targets -- -D warnings: clean.bun test js/binary/src/stream js/json/src/stream(26 tests),bun biome check, andbun run --filter=@moq/binary --filter=@moq/json check(tsc): clean.(Written by claude-opus-5)