Skip to content

fix(net): an END_OF_TRACK landing after a viewer's release keeps the session - #8754

Merged
moq-bot[bot] merged 3 commits into
mainfrom
fix/ietf-end-of-track-after-release
Nov 15, 2026
Merged

moq-bot[bot] merged 3 commits into
mainfrom
fix/ietf-end-of-track-after-release

Conversation

@kixelated

@kixelated kixelated commented Nov 14, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Scheduled Interop 43606836954, the first since moq-dev/moq#8635, still failed Media output and lifecycle, now on chromium's replacement case: live incumbent fallback plays on moq-transport-22, with audioStalled: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 #endPending ignores 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), and end_track's doc says a late marker changes nothing. The public finishAt still 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: #endPending covers it, and #8769's test now exercises this fix (it fails with #endPending throwing again).

Decisions

  • Land: ✅ merge (maintainer, 2026-11-15, /quest-complete).
  • Firefox resume audio follow-up: ✅ keep in m0 (rejected: m1, drop). Recorded in its Plan.

Impact

  • No public API or wire change. A JS moq-transport subscriber no longer drops its session when a publisher's END_OF_TRACK crosses a local release.

Validation

  • New subscriber.test.ts case: 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/ in js/net: 3306 passed, 0 failed.
  • just test media --media-cases replacement (chromium, real relay and browsers), counting invalid END_OF_TRACK in the kept run logs: on main 1 of 2 runs failed live incumbent fallback plays with one session close, matching CI. With the fix 7 of 7 passed with zero session closes (moq-transport-22 fallback 208 to 510 ms against the 1039 ms bound).
  • just check: passed.
  • After merging main (2827c49): bun test src/ in js/net 3314 passed, 0 failed; just check passed; just test interop --all all passed. just test media --media-cases replacement (chromium): 2 of 3 passed, zero invalid END_OF_TRACK closes in all three. The failing run stalled moq-lite-06 audio after skipping missing groups, the separate hole interop-green lists as still open, not this IETF path.

Caused by

  • moq-dev/moq#7808 - made the END_OF_TRACK path wait for the stream's FIN (await last(), which also returns on the track's release) before declaring the end, so a release during that wait reaches endTrack on a closed track, which moq-dev/moq#4086 turns into a session-closing ProtocolViolation.

Alternatives

  • Return from the group handler when 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

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 review after 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 medium

  • Classified: medium by default

  • Reviewer: Codex, gpt-6.1-sol, medium effort

  • Session: 01a1de3a-5ee5-7bb3-8a14-e7cf1b449458, round 1

  • Score: 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

kixelated and others added 2 commits November 14, 2026 09:24
…session

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated marked this pull request as ready for review November 14, 2026 17:25
@coderabbitai

coderabbitai Bot commented Nov 14, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You'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.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: db0b1eae-fa2f-4b52-9671-5052ae7ee7fe
Commits

Reviewing files that changed from the base of the PR and between 98ed0f8 and 2827c49.

Files selected for processing: 5

  • js/net/src/ietf/subscriber.ts
  • js/net/src/track.ts
  • quest/m0/interop-green/README.md
  • quest/m0/nightly/README.md
  • quest/m0/nightly/firefox-resume-audio.md
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

pullfrog[bot]
pullfrog Bot previously approved these changes Nov 14, 2026

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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: #endPending returns instead of throwing when the track already closed and has no declared end. That covers END_OF_TRACK, PUBLISH_DONE, FETCH_OK and the lite endTrack caller. Skipping #owe() changes nothing, because it already returns early when no end is declared. The public finishAt still 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. subscribeTrack now also returns the transport.
  • Quest docs: updates the interop-green waiting note, adds a nightly follow-up note, and adds the new firefox-resume-audio quest.

Pullfrog  | View workflow run | Using 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>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 && !trackFinished early return after await 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.#end returns once closed) and subscription.end ??= only touches a released subscription, so neither has any effect.
  • Dropped the duplicate test: #8769's test on main now exercises the #endPending path 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-audio quest records that it stays in m0.

Pullfrog  | View workflow run | Using claude-opus-5-5 | 𝕏

@moq-bot
moq-bot Bot added this pull request to the merge queue Nov 15, 2026
Merged via the queue into main with commit dfdd301 Nov 15, 2026
6 checks passed
@moq-bot
moq-bot Bot deleted the fix/ietf-end-of-track-after-release branch November 15, 2026 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant