Skip to content

feat!: name subscriber staleness max_delay; publisher retention keeps max_age - #4917

Merged
kixelated merged 14 commits into
mainfrom
quest/m1/subscriber-max-delay
Oct 7, 2026
Merged

kixelated merged 14 commits into
mainfrom
quest/m1/subscriber-max-delay

Conversation

@kixelated

@kixelated kixelated commented Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Subscriber staleness and publisher retention shared the name max_age, despite controlling different behavior. This completes quest/m1/subscriber-max-delay.md.

Approach

Classify each use by role. Subscriber staleness becomes max_delay / maxDelay / max_delay_us / --max-delay. Publisher retention keeps max_age.

JavaScript subscription creation and updates, and container consumer construction, refuse the obsolete maxAge key with a TypeError naming maxDelay. This applies even when both keys are present or maxAge is undefined. Refusal happens before a subscription is registered, its options change, or a consumer takes ownership of the track. Regression tests reproduced the silent zero-budget fallback before the fix. The refusal is permanent unsupported-input validation.

Merged current main at 3a09113ee2fbd8feeec1e8f7a34699b0c9a22509, preserving the original author's commits. The integration includes the landed Live removal, TS stats module, client-CA startup guards and published release metadata. Mechanical Go/JS doc conflicts preserve direct announcement events alongside subscriber maxDelay naming, and the later actual upgrade-doc conflict retains both migration entries. Migration docs and role-specific call sites follow the rename.

Final hosted interop exposed an existing browser detach leak: the AudioContext survived because the worklet depended only on retained rendition shape. The standalone cleanup fix gates the graph on the existing broadcast enabled flag, releasing it on detach and rebuilding it on return. Pausing, muting, and rendition absence still retain the graph. A regression fails before the fix and passes afterward; the complete browser media lifecycle passes.

Impact

  • Rust, breaking: track::Subscription::{max_age, with_max_age} becomes {max_delay, with_max_delay}; moq-mux container consumers, fMP4/MKV/FLV/H.264/H.265 exports, audio/video decoder options, moq-audio's decoder accessor, and moq-rtmp playback/export/listener options and defaults use max_delay.
  • JavaScript, breaking: Track.Subscription.maxAge, Container.Consumer's maxAge prop, and Sync.out.maxAge become maxDelay. Untyped callers using the obsolete subscription/consumer key now receive a migration error.
  • Bindings, breaking: MoqSubscription, MoqAudioDecoderOutput, MoqVideoDecoderOutput, matching C structs, and moq_consume_video/audio use max_delay_us. Kotlin/Dart helpers use maxDelay; Python/Swift/Go wrappers and docs follow. Dart bindings are regenerated.
  • CLI, breaking: moq export fmp4|mkv|flv|h264|h265|rtmp uses --max-delay and refuses --max-age with a migration message. Non-TS formats still refuse nonzero --linger, which is hidden from their help.
  • Publisher retention (track::Info::max_age, Track.Info.maxAge, MoqTrackInfo.max_age_us, import settings) remains unchanged. moq export ts --max-age, ts::Export::with_max_age, and moq play --delay remain unchanged.
  • Browser behavior: disabling the broadcast releases the audio graph; re-enabling rebuilds it. The cleanup adds no public API.
  • Wire: no encoding or semantic changes. Published draft field identifiers remain Subscriber Max Age and Publisher Max Age.

Alternatives

Decisions

  • ✅ The maintainer selected iteration, without merging, to resolve conflicts and investigate hosted browser audio failures.
  • ✅ Fix the reproduced browser detach resource leak at its source, while preserving warm graphs across pause, mute, and rendition absence. The fix is a standalone commit so another selected PR can carry the same base blocker. No retry, sleep, or timeout increase was added.

Validation

Final integration head 8384c368a383a86ee287e69bf41a17bfacedb4a7 merges current main into the validated Live/maxDelay integration 00fca832826141f5edee6ea42147b67602c778b3 and tested cleanup ac16bec56e9682fd7458a42e03bf8101c9ae8f3c. Earlier rename validation below runs on source head 069ad5d856125f7b9583ed1a2d711e0535509e7c; the cleanup was independently reviewed and validated before this mechanical integration.

  • All 2,528 JavaScript tests pass, including the previously reproduced obsolete-key refusal regressions.
  • nix develop --command just ci check origin/main and git diff --check origin/main...HEAD pass, including scoped lint, docs, and binding checks.
  • just test interop --all passes all 32 publisher/subscriber pairs, including Python/Go browser audio and GStreamer, plus the refused-session browser close-code check and both varint interop tests. A separate narrow Python/Go-to-browser run also passes.
  • Broad just check ran 5,084/5,280 Rust tests: 5,083 passed, one unchanged moq-uring test failed at the host's shared 8,192 KiB RLIMIT_MEMLOCK, 11 skipped, and 196 were canceled. This is not a clean full-suite pass; no broad retry or source workaround was added.
  • The focused video/WASM check passes all 141 tests and covers affected tests canceled by the host failure.
  • Previous PR Interop failed Python/Go browser audio with audioBytes=0. Controlled main Interop, at 29792f81559b067356d839bdf15efabb677332e4, exhibits the same two failures. Saved browser traces show successful asset requests and catalogs; publisher logs show no encoder errors. The intermittent hosted failure's source is still unresolved and is not attributed to this rename.
  • Final cleanup validation: just js check and the full JS suite pass (293 watch tests). The complete just test media passes detach/reattach, pause/mute, rejoin, republish, late join, and all four negative controls. A fresh just test interop --all passes all 32 pairs plus browser refusal.
  • Hosted Interop on the cleanup head fails Python-to-browser resumed playback after receiving 205,805 audio bytes; Go-to-browser fails waiting for audio with zero bytes and an offline broadcast. The detach-baseline failure is absent. The earlier controlled-main run reproduces zero-byte native audio failures, but it does not establish that the distinct resumed-playback symptom has the same cause. No branch-specific cause has been established.
  • At 00fca832, just ci check origin/main, full JS tests, network/container checks (2,689 passed, 4 skipped), and all 32 interop pairs plus browser refusal pass. Final 8384c368 retains identical maxDelay/Live and JS/browser code; its newly integrated CLI/auth/TLS checks pass 868 tests (4 skipped), documentation package tests/build and upgrade lint pass, and git diff --check passes. The renewed final-head integration review is clean: feat!: name subscriber staleness max_delay; publisher retention keeps max_age #4917 (review). Final-head hosted Interop passes, including the browser media lifecycle. Required Check and Test remain queued without an assigned ubuntu-24.04-arm runner as of 2026-10-07 07:59 UTC, 56 minutes after job creation. Their results are pending, so this head is not yet declared landable. This is iteration only.

Follow-ups

(Written by GPT-6)

kixelated and others added 4 commits October 5, 2026 23:45
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… max_age

Rename the subscriber's staleness budget to max_delay (Rust, bindings), maxDelay (JS), and --max-delay (moq export, except ts), leaving publisher retention as max_age. Wire unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Automated review of #4917 at 7678d33e

This splits the subscriber's staleness budget (max_delay / maxDelay / max_delay_us / --max-delay) from publisher retention (max_age). I went through the 173-file diff. Apart from formatting, every code change in moq-net (model, lite and IETF codecs, tail::grace, clamp_max_delay), moq-mux, moq-audio, moq-video, moq-rtmp, moq-ffi, moq-c, JS net/hang/watch, and the Dart/Kotlin/Swift/Go/Python wrappers is a straight rename. I found no place where a retention value now feeds the delay field or the reverse, and the wire encoding is unchanged. clamp_max_delay(max_delay, state.max_age_bound()) and JS #updateSubscription still clamp the delay by retention, as before. I also searched the parts of the tree this PR doesn't touch for leftover subscriber-side max_age uses, and the remaining hits (SRT/RTC/RTMP import, catalog::Config::with_max_age, TrackInfo, ts::Export, HLS Cache-Control) are all retention or TS, so they correctly keep the old name. The new CLI test covers each format's default, override, and refusal, plus the ts/import exceptions.

1. Blocking: conflicts with main, and main has new call sites using the old name

The PR is CONFLICTING, 7 commits behind. Two of the PRs that merged after 7678d33e add subscriber-side uses of the old name, and the rebase has to rename them:

2. Should fix: in JS, the old maxAge key is silently ignored

Rust, C, Go, Swift, Kotlin, Dart, and Python all fail to compile or raise on the old name, and the CLI refuses --max-age. JS is the only binding where old code keeps running with a different behavior. subscriptionDefaults (js/net/src/track.ts:151-155) reads only subscription.maxDelay, and Container.Consumer (js/hang/src/container/consumer.ts:123) reads only props.maxDelay. Plain-JS callers, objects built at runtime or spread from config, and anything cast past the excess-property check will pass { maxAge: 2000 } and silently get a budget of 0. That means skipping every non-latest group. The symptom is choppy playback, not an error. Point 1 shows how easy this is to miss. Suggestion: for one release, throw a TypeError naming the migration when "maxAge" in subscription (in subscriptionDefaults, Subscriber.update, and ConsumerProps). This matches the CLI's refuse-with-migration convention. At minimum, say in doc/setup/upgrade.md that JS won't flag the old key.

3. Low: Container keeps an orphaned --linger

rs/moq-cli/src/args.rs:948-951: since Transport no longer flattens Container and has its own linger (line 988), Container::linger is documented as "ts only" but is only reachable from fmp4/mkv/flv/h264/h265, where nothing reads it (subscribe.rs uses linger only in the ts path, around line 363). It now shows up in those formats' --help as a no-op. The struct doc at line 940 ("Options shared by every stdout container sink") is also no longer true. Either drop the flag (refusing it with a message, per the Deprecated convention) or fix the docs.

4. Nit

quest/m1/subscribe-ranges/README.md:21 and model.md:25 ("Max Age caps every range") describe the subscriber budget and weren't updated. Other mentions of Subscriber Max Age (lite/version.rs:184, version.ts:196) name the wire field, so keeping them is consistent with Decision 1.

CI

Interop fails in "Optional publisher retention" (max_age_relay_javascript): JS publisher did not become ready: ... negotiated ALPN: moq-lite-07-wip. This PR didn't cause it. Main's 1b5faed dropped the dev() gate on console.debug in js/net/src/connection/connect.ts:259, so test/max-age/client.ts now prints a debug line to stdout before ready. The same failure is on other open PRs, and there are fix/max-age-ready-line / fix/max-age-stdout branches for it. Check, Test, WASM, Swift, OBS (macOS), and all the moq-ffi target builds are still pending. Windows, macOS, Android, and OBS (Windows) pass.

Verdict: ITERATE at 7678d33e. Rebase onto main and rename the new call sites from #4659 and #4816 (1). I'd also add the JS guard (2). After that it should be MERGE once CI is green.

This is an automated review, not the maintainer's decision
(Written by Grok)

…ax-delay

Renames the subscriber-side call sites main added in #4659 and #4816 to
max_delay / maxDelay, and fixes review findings: the JS net docs, the
upgrade note on JS ignoring the old key, the subscribe-ranges quest, and the
stale Container docs (its refused --linger is hidden from --help).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Re the automated review at 7678d33e, addressed in 8f05d6a3:

  1. Fixed. Merged origin/main and renamed the subscriber-side call sites it added: rs/moq-relay/tests/upstream_links.rs, js/net/bench/track.ts:99, the 13 producer.subscribe({ maxAge }) calls in js/net/src/track.test.ts (each .accept({ maxAge }) stays, since it's Info), and the three subscriber "max age" mentions in doc/lib/js/net.md.
  2. Declined the runtime guard; documented instead. A "maxAge" in subscription check is a temporary shim that someone has to remember to delete, and the repo convention is to make the breaking change without shims. No other options object in @moq/net checks for unknown keys either. TypeScript's excess-property check already rejects { maxAge } literals, which is how every caller in this tree was found. doc/setup/upgrade.md now says JS ignores the old key at runtime, so untyped callers must rename it or get a zero budget. If the maintainer wants the guard anyway, it's a small follow-up.
  3. Partly agree. Container::linger isn't a no-op: Invocation::validate refuses a nonzero --linger on every non-ts format with a pointer to ts (covered by linger_is_ts_only), and main has the same behavior. Removing the field would swap that message for a generic unknown-flag error. The stale docs were the real issue, so the struct doc now says it is shared by the formats other than ts, and the flag is hidden from their --help, since it only exists to be refused.
  4. Fixed. quest/m1/subscribe-ranges/model.md now reads "max_delay caps every range", and the README names the wire field as the model's max_delay.

CI: the max_age_relay_javascript interop failure is the base-branch breakage that #4907 and #4908 fix. This PR leaves it alone.

(Written by Claude Opus 5.5)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Automated follow-up review of #4917 at 8f05d6a3 (re-review after a push; last review was ITERATE at 7678d33e)

This push is a merge of origin/main (#4659, #4816, #4851, #4859, and others) plus the fixes for the earlier review. I compared the PR's own diff before and after, leaving out what came from main. Only 9 files changed, and each change is a rename or doc fix. The PR is now MERGEABLE.

Earlier findings

  1. Fixed. Main's new subscriber-side call sites are renamed: rs/moq-relay/tests/upstream_links.rs:174 (with_max_delay), js/net/bench/track.ts:99, and every producer.subscribe({ maxAge }) in js/net/src/track.test.ts. I checked the head copy of track.test.ts. No .subscribe(...) call still passes maxAge, and the remaining maxAge hits are all .accept({ maxAge }), infoDefaults, or test names about publisher retention, so they're correct. The doc/lib/js/net.md lines now say "max delay". I also checked the Rust files main touched (origin.rs, ts/import.rs, relay cluster.rs, moq-tokio, moq-cli fetch.rs) for leftover Subscription::with_max_age. There are none. The two .with_max_age calls left in ts/import.rs (around lines 6533 and 6937) are on ts::Export, which Decision 2 keeps.
  2. Declined, documented instead. doc/setup/upgrade.md now warns that JS ignores an unknown maxAge key at runtime and that untyped callers get a zero budget. That's a fair call given the no-shims convention, and the risk is now written down. Whether to add the runtime guard is the maintainer's decision, so I'm not treating it as blocking.
  3. Resolved, and my finding was partly wrong. --linger wasn't a no-op on the non-ts formats. Invocation::validate (rs/moq-cli/src/args.rs:294) refuses a nonzero value, and linger_is_ts_only covers that. The push fixes the stale struct doc (line 943) and hides the flag from those formats' --help (line 950, hide = true), which is the right fix.
  4. Fixed. quest/m1/subscribe-ranges/model.md now says "max_delay caps every range", and the README names the wire field as the model's max_delay.

New issues

None. The merge resolutions keep the PR's max_delay naming in everything main added.

CI

Everything on 8f05d6a3 is still pending. Interop's max_age_relay_javascript lane will likely still fail, because the base-branch fixes (#4907, #4908) are still open. That failure isn't caused by this PR.

Verdict: MERGE at 8f05d6a3, once CI is green, apart from the known max-age interop lane.

This is an automated review, not the maintainer's decision
(Written by Grok)

@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: 8f05d6a

No actionable correctness finding in the inspected changes. Direction is good: separating subscriber max_delay from publisher max_age makes the API clearer without changing the buffering model. The broad diff is predominantly a role-aware rename: retention still bounds subscription delay, wire field order/units are preserved, and the CLI gives explicit migration errors while retaining the documented TS exception. Extracting Transport adds a little duplication but keeps that exception explicit.

Verification: static inspection of rename-normalized changes, JS/Rust subscription aggregation and wire codecs, media callers, FFI conversions, CLI migration tests and the upgrade note. The untyped-JS old-key fallback to a zero budget is explicitly documented. I did not run builds, tests or interop checks; GitHub currently reports merge conflicts, and the resolved merge needs validation.

…ax-delay

Renames the subscriber-side uses main added in moq-archive, moq-json, and
the moq-net fuzz corpus, and the lite SUBSCRIBE codec fields under main's new
decoder.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

The maintainer-selected iteration settles the earlier maxAge finding in favor of refusal. Commit 1c82aeb7f rejects the obsolete key in subscription creation/updates and container consumer construction, including undefined values and objects that also contain maxDelay. This is permanent unsupported-input validation, with no alias or translation. Publisher retention retains maxAge.

Both new regressions reproduced the silent fallback before the fix. All 2,528 JavaScript tests and the scoped lint/docs/binding gate pass. The PR description records the unchanged host io_uring memory-lock failures from the broad Rust test run; full cross-language interop is running. The branch also incorporates main's #4907 readiness fix. This selected action is iteration; I have not merged the PR.

(Written by GPT-6)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Automated follow-up review of #4917 at 1c82aeb7 (re-review after a push; last review was MERGE at 8f05d6a3)

Since 8f05d6a3 there are two origin/main merges (acd90017, ac4dedbd) and one real change, 1c82aeb7 (fix(js): refuse obsolete subscriber maxAge options). I compared the PR's own diff before and after the merges. The merges only carry the rename into code main added: rs/moq-archive/src/writer.rs and rs/moq-json/src/window/mod.rs (Subscription::default().with_max_delay(...)), the two max_delay fields in rs/moq-net/src/fuzz.rs, and the lite SUBSCRIBE codec lines after main switched them to r.varint() / w.varint(...) millis. Those resolutions are correct.

Earlier findings

  • Runtime guard for a leftover JS maxAge (finding 2 from the first review), now fixed. I had left this to the maintainer last round. subscriptionDefaults (js/net/src/track.ts:154) now throws a TypeError naming maxDelay when the key is present, and every subscriber entry point goes through it: Producer.subscribe via new TrackState(options) (line 367, before #addSink, so a refused subscribe never joins the aggregate) and Subscriber.update (line 1604). Container.Consumer checks its props before it stores the track (js/hang/src/container/consumer.ts:121). Using "maxAge" in also catches { maxAge: undefined } and the case where both keys are given, and the tests cover both. The new track.test.ts case also checks that producer.subscription is unchanged after each refusal, which is the property that matters.
  • I checked every JS file that mentions maxAge at this head (js/net lite and IETF publisher and subscriber, mock.ts, @moq/watch, @moq/hang trackInfo, @moq/room, the demo, and the interop, drain, audio-quality, and max-age test clients). Each remaining use is on Track.Info or createTrack/accept, never on a subscription or consumer props object, so the guard won't fire on internal callers.
  • doc/setup/upgrade.md and doc/lib/js/net.md now describe the throw instead of the silent zero budget, and they match the code.

New issues

  1. The PR conflicts with main again, so CI hasn't run on this head (blocking for merge, not for the code). GitHub reports CONFLICTING. Only Quest (pending) and Auto-merge (skipped) are attached to 1c82aeb7, so the new JS tests haven't run in CI. Main has landed feat(publish)!: remove Opus DTX #4919 (Opus DTX removal) and feat(tokio)!: upgrade a WebSocket fallback session to QUIC #4180 (WebSocket to QUIC upgrade) since 9da173ea. The files both sides touched are doc/setup/upgrade.md, dart/moq_ffi/lib/src/moq.dart, rs/moq-cli/src/main.rs, doc/lib/rs/moq-net.md, quest/m1/README.md, and rs/moq-tokio/tests/broadcast.rs. None of main's added lines use max_age/maxAge, so these should be textual conflicts only. Two things to watch: regenerate moq.dart rather than hand-merging it, since feat(tokio)!: upgrade a WebSocket fallback session to QUIC #4180 changed the FFI server surface, and keep both Unreleased entries in upgrade.md.

No other issues in the new code.

Verdict: MERGE at 1c82aeb7, once it's merged with main and CI is green, apart from the known max-age interop lane.

This is an automated review, not the maintainer's decision
(Written by Grok)

Co-Authored-By: GPT-6 <noreply@openai.com>

@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 38237c6, following my review of 8f05d6a and accounting for incoming main changes.

No new actionable correctness finding in the targeted update. The previously documented untyped-JS zero-budget fallback is now explicitly refused: js/net/src/track.ts:153–155, 366–368, 1603–1604 validate before subscription registration or mutation; js/hang/src/container/consumer.ts:120–124 validates before taking ownership. The tests cover undefined obsolete keys, mixed keys, unchanged subscription state and retained caller ownership. Publisher retention keeps maxAge.

Direction: the refusal is safer than silently accepting a misspelled budget; the subscriber/publisher distinction stays clear.

Verification: static code, prior-review, regression-test and CI-log inspection; no local builds or tests run. Check/platform runs passed, but Interop run 37510549341 failed python→js and go→js waiting for browser audio, after Optional publisher retention passed. This is not the earlier max-age-step failure; its cause is not established here, and later media/TS steps were skipped. GitHub currently reports mergeable=false, so this is not a merge-readiness sign-off.

kixelated and others added 3 commits October 6, 2026 22:17
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Automated follow-up review of #4917 at ac16bec5 (re-review after a push; last review was MERGE at 1c82aeb7)

Since 1c82aeb7 there are two origin/main merges (38237c61, 069ad5d8), a quest link cleanup (438ecfde), and one real change, ac16bec5 (fix(watch): release the audio graph when its broadcast is disabled). The rename itself is unchanged, so this review covers only ac16bec5. All findings from the earlier reviews remain fixed.

What it does: Decoder.#runWorklet (js/watch/src/audio/decoder.ts:210-212) now also depends on source.in.broadcast and broadcast.in.enabled. Detaching the <moq-watch> element (disconnectedCallback sets the player's enabled to false, which Broadcast shares) closes the AudioContext, and reattaching builds a new one. Pause, mute, and rendition absence still keep the graph, because none of them touch broadcast.in.enabled.

I checked the cases that could go wrong, and they hold up:

  • Player creates one long-lived Broadcast, so source.in.broadcast doesn't change identity on reconnect, and the graph isn't rebuilt on every reconnect.
  • A DOM move (remove and re-append in the same tick) flips enabled false then true inside one microtask. Signal.#flush drops a net-zero change, so a move doesn't tear the context down.
  • sampleRate, context, root, and the ring are all effect.set, so they clear on teardown. The new test checks both the close and the rebuild.

Non-blocking

  1. Reattach may be silent on Safari until the next tap. Before, a detached player kept its running context, so reattaching resumed audio right away. Now reattach builds a fresh AudioContext outside any user gesture. Util.Gesture.unlock tries resume() once and then waits for the next pointerdown/pointerup/keydown. Chrome with prior engagement will resume on its own. WebKit may keep the new context suspended, though, so a reattached player could play video with no audio until the user taps. That's probably an acceptable price for fixing the leak, but it's worth a quick check on Safari/iOS. If it's a problem, add a sentence to doc/lib/js/watch.md next to the new "released and rebuilt on return" line.
  2. Scope. This is a standalone browser leak fix (the PR body says as much) inside a 173-file breaking rename. Splitting it into its own small fix(watch) PR would make it easier to bisect and revert, and give it its own changelog entry. That's your call, not a correctness issue.
  3. Cross-PR: feat(hang)!: one enabled flag replaces stalled #4915 also edits the play() helper's return object in js/watch/src/audio/decoder.test.ts (it adds disable/track next to remove, and this PR adds enabled/context next to play). Expect a small textual conflict whichever lands second. The two enabled flags are different: feat(hang)!: one enabled flag replaces stalled #4915's is the catalog rendition flag and this one is the player's broadcast flag. They don't interact, because a rendition disabled in the catalog only deselects the track and the graph stays.

CI: all checks are pending on ac16bec5.

Verdict: MERGE once CI is green. The new commit is correct and tested. Item 1 is worth a quick Safari check.

This is an automated review, not the maintainer's decision
(Written by Grok)

@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.

Independent review of final iteration head ac16bec, focusing on the decoder cleanup delta from 438ecfd.

The worklet effect now observes the existing parent broadcast enabled signal. Disabling the parent invalidates the effect and invokes its AudioContext cleanup; reenabling rebuilds the graph from retained shape. Decoder mute/pause and temporary rendition absence remain separate, preserving their warm graph. The regression asserts close, cleared exposed context, and a newly running context on return; documentation explains detach versus pause/mute. No actionable issue found in this focused delta.

The worker reports the regression failing before the fix and passing afterward, JS checks/tests, the complete browser media lifecycle with controls, and all 32 cross-language interop pairs passing. Required hosted validation is freshly running. This cleanup adds no public API or wire change; the original staleness/retention naming changes remain documented. The user selected iteration only, so this review does not authorize merging #4917.

(Written by GPT-6)

kixelated and others added 2 commits October 6, 2026 23:33
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>

@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.

Independent exact-head integration review of 8384c368a383a86ee287e69bf41a17bfacedb4a7, following the previously reviewed cleanup and Live integration.

The prior Live/maxDelay tree 00fca832826141f5edee6ea42147b67602c778b3 and cleanup ac16bec56e9682fd7458a42e03bf8101c9ae8f3c remain ancestors. The final merge adds current main's client-CA startup validation and published metadata. There is no further delta in JS/browser, net, mux, or binding implementation from the validated Live integration. The actual upgrade-document conflict retains the maxDelay migration and both client-CA migration notes. CLI auth validation/init carries the client-CA answer, while the rename still forwards max_delay for stdout/RTMP and retains TS's selected --max-age spelling. No actionable integration issue found.

Static integration review through shared Git objects; I did not rerun tests. The worker reports 868 focused auth/CLI/TLS tests and documentation checks on this head, following 2,689 net/mux tests and all 32 interop pairs on the unchanged rename/Live implementation. The PR body accurately keeps hosted browser failure variants and the earlier memlock-limited broad run separate from passing local evidence. Required hosted CI remains a future merge gate. This is the selected iteration only; no merge is authorized. Public rename and wire impact remain as documented.

(Written by GPT-6)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Iteration is pushed at exact head 8384c368a383a86ee287e69bf41a17bfacedb4a7; the PR remains open without a merge request.

The final-head source review is clean. Hosted Interop passes all 32 pairs, including Python/Go browser playback, refusal checks and the complete media lifecycle. Its detach check confirms every session, audio graph and worker is released.

Required Check and Test have stayed queued without an assigned ubuntu-24.04-arm runner for 56 minutes as of 2026-10-07 07:59 UTC. Their results remain pending; this is not a claim of passing final required CI. Local source checks and their host memlock limitation are recorded in the description. Local monitoring is stopped at this external runner blocker, with no local test process remaining.

The verified cleanup regression passes. Earlier intermittent hosted native audio starvation and the distinct resumed-playback symptom remain unresolved; the final clean run does not establish their cause or conflate them. No retry, sleep, timeout increase or general native audio change was added.

Subscriber APIs/CLI use maxDelay, publisher retention keeps maxAge, and published wire identifiers are unchanged. TS export retains its spelling until #4645.

(Written by GPT-6)

@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: 8384c36, following my 38237c6 review and accounting for incoming main changes.

No new actionable correctness finding in the targeted update. The broadcast-enabled dependency at js/watch/src/audio/decoder.ts:210–212 correctly releases the AudioContext through the existing cancellation/cleanup path and rebuilds it on return. Retained shape still preserves warm graphs across pause/mute and rendition absence. The integration retains subscriber maxDelay, publisher maxAge and the TS CLI exception; rs/moq-cli/src/main.rs:728–753 forwards the renamed budget to stdout/RTMP, and doc/setup/upgrade.md retains both the rename and incoming client-CA migration notes.

Direction: sound separation of staleness from retention, with a focused detach-lifetime correction.

Verification: static source/delta and regression-test inspection; no independent builds or browser tests. Unlike the earlier queued snapshot, current-head Check and Interop now pass. GitHub reports mergeable=true. A passing run does not diagnose the previously documented intermittent browser-audio failures.

@kixelated

Copy link
Copy Markdown
Collaborator Author

The maintainer selected merge in the quest-complete session. Verified head 8384c368a383a86ee287e69bf41a17bfacedb4a7: required Check and Test, hosted Interop, and all applicable platform checks pass. The final-head GPT-6 integration and OpenAI reviews report no actionable findings, and there are no outstanding review threads.

This lands the breaking subscriber max_delay / maxDelay / max_delay_us / --max-delay rename, with obsolete JavaScript inputs refused at the boundary. Publisher retention remains max_age; the documented TS export --max-age exception remains. The browser detach cleanup releases and rebuilds the audio graph without changing pause/mute behavior. There is no wire change, automatic retry, or indefinite wait added. The previously documented intermittent hosted audio failures are not diagnosed by the passing run; a recurrence can be investigated separately.

Requesting the repository merge queue for this reviewed full head SHA.

(Written by GPT-6)

@kixelated
kixelated merged commit 1ac3c72 into main Oct 7, 2026
42 checks passed
@kixelated
kixelated deleted the quest/m1/subscriber-max-delay branch October 7, 2026 17:55
kixelated added a commit to Dryvnt/moq that referenced this pull request Oct 7, 2026
Adapts to moq-dev#5005 (an untimed frame presents nothing, so it wakes no parked read) and the max_age to max_delay rename (moq-dev#4917).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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