Skip to content

test(net): verify browser IETF pause and resume - #7709

Merged
kixelated merged 8 commits into
mainfrom
quest/m0/interop-green/js-pause-resume
Nov 3, 2026
Merged

kixelated merged 8 commits into
mainfrom
quest/m0/interop-green/js-pause-resume

Conversation

@kixelated

@kixelated kixelated commented Nov 3, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Browser JS-to-JS moq-transport catalog or resumed media requests ended with PUBLISH_DONE INTERNAL_ERROR on drafts 20 and 22. The cell now passes. This PR identifies the landed fix, strengthens its unit coverage, and finishes quest/m0/interop-green/js-pause-resume.md.

Approach

  • The historical bisect identifies moq-dev/moq#7085, commit e3a1cdf232, as the first passing change. A resumed Rust timed-track copy requested INCLUDE_PROPERTIES=0 but still required timestamps. JS correctly omitted them, causing Rust to fail the track with MalformedTrack. The fix asks for properties when the resumed copy is timed.
  • Keep the existing Rust scripted-peer regression a_resumed_subscribe_opts_out_only_when_untimed, which checks timed and untimed copies on drafts 18, 20, and 22. Extend the existing JS publisher properties/timestamp test from draft 22 to both 20 and 22, verifying the corresponding wire behavior with opt-in and opt-out peers.
  • Delete the finished quest and its link. Size the parent README now that this was its last child. The parent's separate requirement to confirm complete scheduled Interop and Video runs remains.

Evidence

  • Ran the quest's browser command, just test interop --versions ietf --publishers js --subscribers js, in isolated historical source: 38566f5a84, 026bc66fa5, fcef1ea64b, and the fixing commit's parent d44ee77b18 fail drafts 20/22 with PUBLISH_DONE INTERNAL_ERROR; draft 18 passes. 0c8b0924b5 and e3a1cdf232 pass all three. git bisect over 38566f5a84..c95a9b1201, restricted to the quest's source paths, selects e3a1cdf232 as first fixed. Listing refusal alone is insufficient; the DYNAMIC_GROUPS and RequestStream fixes landed after this passing revision.
  • Two complete current-source browser runs at 204c97f4fb pass drafts 18/20/22, including catalog, decoded media/tone, and advancing audio/video after pause.
  • November 1's scheduled report records all three cells passing. November 2's run was cancelled later during media tests, but its completed moq-transport step explicitly records all three passing. Its report was not built or published; no full scheduled-run success is claimed.
  • The archived October 31 run failed draft 22 earlier on the catalog. The historical local runs reproduced the resume symptom on both target drafts, not that earlier catalog failure separately.

Impact

  • Public API: none.
  • Wire: none. Only tests and quest bookkeeping change.

Alternatives

No new production fix is needed. Reuse the existing scripted-peer regression and extend the JS wire test to the missing draft, keeping the browser matrix as integration coverage.

Choices made without asking

  • Dropped a broader listing/cancellation test once the bisect ruled out listing refusal as sufficient.
  • Left the concurrent Rust multi-group resume and audio capture work untouched.
  • Used existing scheduled logs and repeated local runs without dispatching or claiming another scheduled proof.

Validation

  • JS publisher unit tests: 115 pass.
  • Rust timed-property and DYNAMIC_GROUPS resume regressions: 2 pass.
  • quest check --base origin/main: 602 documents pass.
  • just ci: passed on 7d8841657cc9fdb65cc66e521ef15c46df33cf9d, including the JS build/unit suite and repository checks.

Follow-ups

No new quests. The parent scheduled workflow verification, existing browser media flakes, and concurrent Rust resume repair retain their scope.

Review

  • Claude Code via quest review, claude-opus-5-5, medium effort, round 2.
  • Reviewed head: 7d8841657cc9fdb65cc66e521ef15c46df33cf9d against origin/main at 801fd9a2f2f5c88371fa833d805620d97d372465.
  • Test-name finding fixed with moqt-20/moqt-22 labels.
  • Coverage finding refuted with the historical bisect and the fixing PR's existing Rust scripted-peer regression cited above. The removed listing test covered a different cause.
  • Round 2: no findings.

(Written by GPT-6.1 Sol)

kixelated and others added 7 commits November 3, 2026 00:59
Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
@kixelated kixelated changed the title The browser js -> js moq-transport cell stays fixed test(net): verify browser IETF pause and resume Nov 3, 2026
Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
@kixelated
kixelated marked this pull request as ready for review November 3, 2026 09:31

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Automated review by review (OpenAI)

Reviewed commit: 7d88416

Extending the existing wire regression to draft 20 and retaining the separate scheduled-run follow-up is a sensible, small change. No actionable bugs found in this static review. I did not run the browser matrix or unit tests.

@coderabbitai

coderabbitai Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Walkthrough

The publisher test now checks track-property opt-out behavior for IETF drafts 20 and 22, using the selected version to create the fixture and decode the served group. The Interop quest README title now begins with [XS]. Its Required section and linked task were removed, and the pause/resume quest document was deleted.

Merge Risk: Minimal · up to 7d884

The change broadens draft-version regression coverage and updates completed-task documentation without changing runtime behavior. No actionable merge-blocking risk is identified.

Pre-merge checks: Passed 5
Check name Status Explanation
Title check Passed The title "test(net): verify browser IETF pause and resume" matches the main change in the changeset. The PR extends the JS publisher properties/timestamp test to drafts 20 and 22, removes the complet…
Description check Passed The description explains the problem (JS-to-JS moq-transport requests ending with PUBLISH_DONE INTERNAL_ERROR on drafts 20 and 22), the bisected fix (#7085 and commit e3a1cdf), the test …
Docstring Coverage Passed No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title "test(net): verify browser IETF pause and resume" matches the main change in the changeset. The PR extends the JS publisher properties/timestamp test to drafts 20 and 22, removes the completed quest document and its README link, and updates the parent README. The title names the testing focus and the feature area, so it is specific enough for a teammate scanning history. It does not mention the underlying test expansion, but it does not need to cover every detail.

Full details: Description check

Explanation

The description explains the problem (JS-to-JS moq-transport requests ending with PUBLISH_DONE INTERNAL_ERROR on drafts 20 and 22), the bisected fix (#7085 and commit e3a1cdf), the test changes, the quest document completion, validation runs, and the impact. It is directly related to the changeset.

Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files. (1 skipped: 1 unsupported.)

Finishing Touches
Simplify code
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Some tools did not complete. Review the errors below.

Biome (2.5.15)
js/net/src/ietf/publisher.test.ts

Biome could not lint this file: configuration resulted in errors. Check the repository's Biome configuration and plugins.

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

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
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: cdcc2bd8-c576-450c-bcab-c74267342e6e
Commits

Reviewing files that changed from the base of the PR and between 204c97f and 7d88416.

Files selected for processing: 3
  • js/net/src/ietf/publisher.test.ts
  • quest/m0/interop-green/README.md
  • quest/m0/interop-green/js-pause-resume.md
Files with no reviewable changes: 1
  • quest/m0/interop-green/js-pause-resume.md

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.

@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

Test-only extension plus quest bookkeeping; no production or wire behavior changes.

  • Draft-20 coverage for the opt-out timestamp case — js/net/src/ietf/publisher.test.ts:2774 turns the draft-22-only case into a test.each over drafts 20 and 22, dropping the hardcoded V22/fixture() in favor of the tuple's version for the fixture, Reader, and GroupMessage.decode. ALPN labels (moqt-20/moqt-22) title the cases; the opt-in/opt-out inner loop is unchanged.
  • Quest completion — deletes quest/m0/interop-green/js-pause-resume.md, removes its ## Required bullet, and sizes the parent README [XS].

Verified locally: bun test src/ietf/publisher.test.ts (115 pass), bunx tsc --noEmit, and biome check all pass, and the parameterized cases emit moqt-20:/moqt-22: names. The regenerated titles match the round-1 fix. The Rust regression the body relies on (a_resumed_subscribe_opts_out_only_when_untimed, rs/moq-net/src/ietf/subscriber.rs:8689) exists and covers drafts 18/20/22 × timed/untimed, and no reference to the deleted quest file remains anywhere in the repo.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@kixelated
kixelated added this pull request to the merge queue Nov 3, 2026
Merged via the queue into main with commit b764ac4 Nov 3, 2026
8 checks passed
@kixelated
kixelated deleted the quest/m0/interop-green/js-pause-resume branch November 3, 2026 15:33
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