Repository navigation
@remotion/media: Resume browser exports after offline fetch failures - #12021
alec-watts wants to merge 15 commits into
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Follow-up from downstream review in https://github.com/kino-ai/kino/pull/3936:
The downstream PR is now merge-ready with unit CI, Vercel, and Bugbot passing. That is not a substitute for an end-to-end browser disconnect/reconnect render in this upstream checkout, which remains unverified. |
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Fixed and pushed the CI failures caused by this change:
Local verification: core/media/web-renderer lint and formatting passed; core built and all 717 tests passed; the full SSR suite passed; built CJS and ESM imports passed on actual Node 16.20.2 without global fetch; recovery/reconnect/abort and cleared-timeout probes passed. Full local monorepo integration was blocked by missing language tooling (python, PHP, Go and Ruby bundle setup), but monorepo CI now passes. The new CI run has passed Linting + Formatting, SSR tests, and Monorepo tests. A remaining Windows Node 16 job failed on the unrelated bundler test "emits a zero-delay Rspack React Refresh runtime" hitting its 5000 ms timeout; no fetch import error remains in that job. Other integration jobs are still running. |
JonnyBurger
left a comment
There was a problem hiding this comment.
The UrlSource.fetchFn integration is a sensible direction, but I am requesting changes for the timeout scope wiring, disposal cleanup, and package ownership described below.
I reproduced both behavioral issues with targeted runtime probes against this head, using the pinned Mediabunny 1.56.1. A complete browser disconnect/reconnect export has not been verified.
…eview # Conflicts: # packages/core/src/internals.ts
There was a problem hiding this comment.
Important
AbortSignal.any in the new recovery wrapper raises the browser/Node floor for @remotion/media above what WebCodecs requires, and canRenderMediaOnWeb() will not warn about it. Details in the inline comments.
Reviewed changes
Reviewed the full diff of PR #12021 (13 files) at fefbd19d, which adds a scope-local delayRender timeout-suspension mechanism to core and wires @remotion/media browser fetch recovery to it. I also verified that the three concerns from the prior maintainer review are now addressed: recovery targets the real DelayRenderContextType scope, the policy lives in @remotion/media (core exposes only a generic suspension mechanism), and an input-owned abort controller releases reconnect waits before disposal.
- Core nested timeout suspension (
packages/core/src/delay-render.ts): a per-DelayRenderScopeWeakMapaddssuspendDelayRenderTimeout/getDelayRenderSuspendedTime, with nested suspends, clear-before-resume protection, and scope-local elapsed accounting. - Internal API exposure (
packages/core/src/internals.ts): exposes the two generic suspension operations. - Offline fetch recovery (
packages/media/src/offline-media-fetch.ts, new): wrapsfetchfor Mediabunny'sUrlSource, waiting for theonlineevent when a fetch rejects withTypeErrorwhilenavigator.onLine === false, backed by an input-owned abort controller. - Render-local tracking (
packages/media/src/caches.ts,get-sink.ts,get-frames-since-keyframe.ts,get-shared-input.ts): the media cache suspends tracked handles while any sink waits offline, and aborts recovery before disposing its input. - Handle registration (
video-for-rendering.tsx,audio-for-rendering.tsx): registers the in-flight delay handle against the actual render scope and releases it on completion and cleanup. - Readiness deadline (
packages/web-renderer/src/wait-for-ready.ts): subtracts scope-local suspended time from thetimeout + 3000msdeadline. - Tests: core suspension regressions, an input-disposal abort test, and an end-to-end web-renderer outage/cancellation test.
deepseek-v4.1-flash | 𝕏
There was a problem hiding this comment.
ℹ️ The incremental delta is clean — no new issues. The two suggestions from the first review are still open.
Reviewed changes
Reviewed the delta pushed after the previous review (9195cd71, 1 file). It is test-only and introduces no new findings.
- Offline-recovery test timing (
packages/web-renderer/src/test/offline-recovery.test.tsx): the<Video>and<Audio>under test now passdelayRenderTimeoutInMilliseconds={5000}, leaving 3 seconds for extraction after reconnecting on a shared CI runner; the 6.5-second outage still exceeds both that budget and the 5.5-second readiness deadline.
The two suggestions from the previous review (the AbortSignal.any environment floor and the unconditional onWaitChanged true/false pairing) remain open and are unaffected by this commit, so I am intentionally not approving.
deepseek-v4.1-flash | 𝕏
# Conflicts: # packages/core/src/delay-render.ts

Browser exports can exhaust their media and readiness deadlines during an outage, preventing the same export from continuing after reconnection. This change waits for reconnection when a media fetch rejects with TypeError and the browser explicitly reports offline.
Maintainer requests addressed
Merged upstream main at f29e9f3, preserving the current video effectsOutputSize API. The old caption-inspector selector repair is already upstream and is no longer part of this diff.
The follow-up review fixes remove the need for AbortSignal.any and throwIfAborted. Request aborts still reach response-body reads; input-lifetime forwarding is removed after fetch settles, so completed requests do not accumulate lifetime listeners. Offline wait notifications are paired inside try/finally. A regression disabling the newer AbortSignal methods passes with the fix and fails against the previous implementation.
Verification on the refreshed branch
The browser tests inject offline fetch failure and navigator state; the separate production harness disables actual Chrome networking.
Limits
navigator.onLine is a connectivity hint: failures while it stays true retain Mediabunny's retry policy. Recovery covers fetch rejection before a response, not failure reading an already-open response body. Offline exports may wait until reconnection or cancellation. Unrelated render scopes and authored handles retain their deadlines. Server-render timeout policy is unchanged.
Why each file changes
Current main integration
Merged upstream
bbe11edd85in8e50119b54. 4 shared-input browser tests and the offline export recovery browser test passed. Affected-package builds/typechecks passed; Upstream CI passed on8e50119b54; GitHub reports no merge conflicts.Main refresh — 2026-10-23
Merged upstream
2bd172f29ainto this branch ase6b17e3004. The merge required no manual conflict resolutions. Diff cleanup and whitespace checks passed; retained file changes are scoped to this proposal. Fresh upstream CI passed on this head, including Required CI. Earlier local validation above applies to the commits identified there.Local verification on this refreshed head: core TypeScript declaration build passed; all 726 core tests passed.