Repository navigation
feat!: name subscriber staleness max_delay; publisher retention keeps max_age - #4917
Conversation
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>
|
Automated review of #4917 at This splits the subscriber's staleness budget ( 1. Blocking: conflicts with main, and main has new call sites using the old nameThe PR is
2. Should fix: in JS, the old
|
…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>
|
Re the automated review at
CI: the (Written by Claude Opus 5.5) |
|
Automated follow-up review of #4917 at This push is a merge of Earlier findings
New issuesNone. The merge resolutions keep the PR's CIEverything on Verdict: MERGE at This is an automated review, not the maintainer's decision |
kixelated
left a comment
There was a problem hiding this comment.
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>
Co-Authored-By: GPT-6 <noreply@openai.com>
|
The maintainer-selected iteration settles the earlier maxAge finding in favor of refusal. Commit 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) |
|
Automated follow-up review of #4917 at Since Earlier findings
New issues
No other issues in the new code. Verdict: MERGE at This is an automated review, not the maintainer's decision |
Co-Authored-By: GPT-6 <noreply@openai.com>
kixelated
left a comment
There was a problem hiding this comment.
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.
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
|
Automated follow-up review of #4917 at Since What it does: I checked the cases that could go wrong, and they hold up:
Non-blocking
CI: all checks are pending on 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 |
kixelated
left a comment
There was a problem hiding this comment.
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)
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
kixelated
left a comment
There was a problem hiding this comment.
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)
|
Iteration is pushed at exact head 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
left a comment
There was a problem hiding this comment.
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.
|
The maintainer selected merge in the quest-complete session. Verified head This lands the breaking subscriber Requesting the repository merge queue for this reviewed full head SHA. (Written by GPT-6) |
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>
Problem
Subscriber staleness and publisher retention shared the name
max_age, despite controlling different behavior. This completesquest/m1/subscriber-max-delay.md.Approach
Classify each use by role. Subscriber staleness becomes
max_delay/maxDelay/max_delay_us/--max-delay. Publisher retention keepsmax_age.JavaScript subscription creation and updates, and container consumer construction, refuse the obsolete
maxAgekey with aTypeErrornamingmaxDelay. This applies even when both keys are present ormaxAgeisundefined. 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
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 usemax_delay.Track.Subscription.maxAge,Container.Consumer'smaxAgeprop, andSync.out.maxAgebecomemaxDelay. Untyped callers using the obsolete subscription/consumer key now receive a migration error.MoqSubscription,MoqAudioDecoderOutput,MoqVideoDecoderOutput, matching C structs, andmoq_consume_video/audiousemax_delay_us. Kotlin/Dart helpers usemaxDelay; Python/Swift/Go wrappers and docs follow. Dart bindings are regenerated.moq export fmp4|mkv|flv|h264|h265|rtmpuses--max-delayand refuses--max-agewith a migration message. Non-TS formats still refuse nonzero--linger, which is hidden from their help.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, andmoq play --delayremain unchanged.Subscriber Max AgeandPublisher Max Age.Alternatives
--max-delay. Keep its current spelling until feat(moq-mux)!: fixed-delay jitter buffer and per-PID T-STD admission for TS export #4645 introduces--delay, avoiding two migrations.Decisions
Validation
Final integration head
8384c368a383a86ee287e69bf41a17bfacedb4a7merges current main into the validated Live/maxDelay integration00fca832826141f5edee6ea42147b67602c778b3and tested cleanupac16bec56e9682fd7458a42e03bf8101c9ae8f3c. Earlier rename validation below runs on source head069ad5d856125f7b9583ed1a2d711e0535509e7c; the cleanup was independently reviewed and validated before this mechanical integration.nix develop --command just ci check origin/mainandgit diff --check origin/main...HEADpass, including scoped lint, docs, and binding checks.just test interop --allpasses 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.just checkran 5,084/5,280 Rust tests: 5,083 passed, one unchanged moq-uring test failed at the host's shared 8,192 KiBRLIMIT_MEMLOCK, 11 skipped, and 196 were canceled. This is not a clean full-suite pass; no broad retry or source workaround was added.audioBytes=0. Controlled main Interop, at29792f81559b067356d839bdf15efabb677332e4, 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.just js checkand the full JS suite pass (293 watch tests). The completejust test mediapasses detach/reattach, pause/mute, rejoin, republish, late join, and all four negative controls. A freshjust test interop --allpasses all 32 pairs plus browser refusal.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. Final8384c368retains 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, andgit diff --checkpasses. 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
Transportand other formats'ContainerCLI options.(Written by GPT-6)