Repository navigation
fix(net): an END_OF_TRACK landing after a viewer's release keeps the session - #8754
Conversation
…session Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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 3 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. |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the JS fix that stops a late wire end from closing the session, plus its regression test and the quest notes.
- Late wire end ignored:
#endPendingreturns instead of throwing when the track already closed and has no declared end. That covers END_OF_TRACK, PUBLISH_DONE, FETCH_OK and the liteendTrackcaller. Skipping#owe()changes nothing, because it already returns early when no end is declared. The publicfinishAtstill throws, and the conflicting-end checks are unchanged. - Regression test: a group stream sends an object and END_OF_TRACK, the track is released before the FIN arrives, and the test checks that the transport stays open.
subscribeTracknow also returns the transport. - Quest docs: updates the interop-green waiting note, adds a nightly follow-up note, and adds the new
firefox-resume-audioquest.
claude-opus-5-5 | 𝕏
Drop #8769's group-stream guard and this branch's duplicate test: the track's wire end now ignores a late end for every path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the merge of main, which brought in #8769's narrower guard and resolves the overlap with this PR.
- Dropped #8769's subscriber guard: the
trackReleased && !trackFinishedearly return afterawait last()is removed, so a late END_OF_TRACK reaches#endPending, which now ignores an end on a closed track. On that path,producer?.finish()is idempotent (group.Producer.#endreturns once closed) andsubscription.end ??=only touches a released subscription, so neither has any effect. - Dropped the duplicate test: #8769's test on
mainnow exercises the#endPendingpath instead of the removed guard. - Updated quest notes: the interop-green note now credits #8769 and #8754, the nightly README link now points to #8676, and the
firefox-resume-audioquest records that it stays in m0.
claude-opus-5-5 | 𝕏

Problem
Scheduled Interop 43606836954, the first since moq-dev/moq#8635, still failed
Media output and lifecycle, now on chromium'sreplacementcase:live incumbent fallback playsonmoq-transport-22, withaudioStalled:true. (#8635's pause/resume case passed.)The viewer's log shows why: when the replacement withdrew, the viewer released its tracks, and the relay's END_OF_TRACK for one of them arrived on a group stream whose FIN the handler was still waiting on. The JS IETF subscriber then declared the end on an already closed track, which throws, and turned that into a ProtocolViolation that closed the whole session (
connection closed, reconnecting: remote error: 3 (invalid END_OF_TRACK: track is closed)). Reconnecting and resubscribing overran the 1039 ms fallback bound.Approach
A wire end (END_OF_TRACK, PUBLISH_DONE, FETCH_OK) that lands after the track closed has nothing left to end, so
#endPendingignores it instead of throwing. That is a race the peer cannot see, not its violation. Rust already never closes the session here: its group stream stops on its own (stop_on_error), andend_track's doc says a late marker changes nothing. The publicfinishAtstill throws on a closed track.moq-dev/moq#8769 landed on main meanwhile with a narrower guard for the group-stream END_OF_TRACK alone. The merge drops that guard and this branch's duplicate test:
#endPendingcovers it, and #8769's test now exercises this fix (it fails with#endPendingthrowing again).Decisions
Impact
Validation
subscriber.test.tscase: a group stream delivers an object and END_OF_TRACK, the viewer releases the track before the FIN, and the session must stay open. It fails on main (session closed) and passes with the fix.bun test src/injs/net: 3306 passed, 0 failed.just test media --media-cases replacement(chromium, real relay and browsers), countinginvalid END_OF_TRACKin the kept run logs: on main 1 of 2 runs failedlive incumbent fallback playswith one session close, matching CI. With the fix 7 of 7 passed with zero session closes (moq-transport-22fallback 208 to 510 ms against the 1039 ms bound).just check: passed.bun test src/injs/net3314 passed, 0 failed;just checkpassed;just test interop --allall passed.just test media --media-cases replacement(chromium): 2 of 3 passed, zeroinvalid END_OF_TRACKcloses in all three. The failing run stalledmoq-lite-06audio afterskipping missing groups, the separate hole interop-green lists as still open, not this IETF path.Caused by
await last(), which also returns on the track's release) before declaring the end, so a release during that wait reachesendTrackon a closed track, which moq-dev/moq#4086 turns into a session-closing ProtocolViolation.Alternatives
last()resolves on the release. Rejected: PUBLISH_DONE and FETCH_OK hit the same declaration and can race a release the same way, so the wire end itself is the one place to settle it.Follow-ups
after resumewindow still fails after fix(watch): snap resumed audio to the live clock #8635. Audio's first groups reach the player about 0.4 s after video's. This did not reproduce on an idle host. No open decisions; the Plan recommends reproducing under load and fixing the hop that holds audio.Models
Size: S
Implementer: Claude Code, claude-opus-5-5, unknown effort
Implementer: Claude Code, claude-opus-5-5, unknown effort (iterate, S)
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code
Review
Rewritten by
quest reviewafter each round.Head: 2827c49
Base: origin/main at 77b8ec7
Risk: medium,
the base has no RISK.md to name what is always high or customer-facing, so every change is mediumClassified: medium by default
Reviewer: Codex, gpt-6.1-sol, medium effort
Session:
01a1de3a-5ee5-7bb3-8a14-e7cf1b449458, round 1Score: 9/10,
The focused change tolerates late wire-end markers on closed tracks while preserving validation of previously declared ends; source review identified no actionable regression, and tests were not run as instructed.Rounds: 1; first round 0; findings 0: fixed 0, refuted 0, open 0
Findings: none