Skip to content

fix(remotion): correct four defects in render-scoped asset staging - #478

Open
amitm7 wants to merge 1 commit into
calesthio:mainfrom
amitm7:fix/remotion-staging-delta
Open

fix(remotion): correct four defects in render-scoped asset staging#478
amitm7 wants to merge 1 commit into
calesthio:mainfrom
amitm7:fix/remotion-staging-delta

Conversation

@amitm7

@amitm7 amitm7 commented Aug 7, 2026

Copy link
Copy Markdown

Summary

Fixes four defects in the Remotion render-scoped asset staging that landed in 9482edd (#466). Each fix ships with a regression test that fails without it.

Why this PR exists (and how it relates to #328).

I opened #328 on 2026-07-07 to fix Remotion local-asset staging, and iterated through three review rounds — Windows file:// normalization, staging containment, and staging/cleanup lifecycle. On 2026-08-03, #466 independently implemented staging on main via _stage_remotion_media() + --public-dir, drawing on the local-media handling in #461/#459.

#466 swept "the latest 80 pull requests" (#377#464); #328 predates that window and is not in its superseded list, so the two implementations were written without knowledge of each other.

Rather than re-litigate architecture, this PR keeps main's implementation as-is and contributes only what it is missing: the four defects that #328's review rounds surfaced, which the parallel implementation reproduced. That turns five conflicting commits into a three-file delta on top of 4eab34c.

I'd suggest closing #328 in favour of this PR — its remaining unique change (slug-traversal rejection) does not apply here, because main's design never derives a path segment from artifact metadata, so that attack surface does not exist.

Related issue

Refs #328 (original PR, superseded by this one)
Refs #466 (introduced the code being fixed)

Changes

1. Naive Windows file:// URIs are silently dropped.
A bare f"file://{path}" on Windows produces file://C:\Users\.... With no slash after the scheme, urlsplit puts the entire drive path in netloc and leaves path empty. The current code reads that as a UNC authority (//C:\Users\...), which never resolves — so the asset is skipped rather than staged, and the render silently loses media. Verified against the current parser:

file://C:\Users\me\voice.mp3  ->  //C:\Users\me\voice.mp3   (never resolves)

Parsing is extracted to _file_uri_to_raw_path() so every URI shape is unit-testable off Windows. A genuine UNC host (file://server/share/...) is still treated as a network path.

2. --public-dir makes pre-staged public/ assets 404.
--public-dir replaces Remotion's default public dir for the render. Assets that earlier pipeline stages staged into remotion-composer/public/anime_scene.images[], screenshot_scene.backgroundImage, demo-props fixtures, all documented in SCENE_TYPES.md as public/-relative staticFile() paths — stop resolving as soon as a render points at its own dir. _mirror_public_dir() links the real tree in read-only, without clobbering staged media. Only applied to the dir we create and delete; a caller-supplied public_dir is left untouched.

3. Staging dir was not unique per render.
Naming it only after the output stem means every render of that output shares one directory: concurrent renders overwrite each other's media, the first to finish deletes the other's inputs, and cleanup can erase a pre-existing directory that happens to match the name. A random suffix makes the removed dir always one this invocation created.

4. Staging ran outside the try/finally.
A failure after the directory was created but before the render (props write, command setup) left staged user media on disk. The guard now spans the whole staging/setup/render lifetime.

Testing

tests/tools/test_remotion_media_staging.py and tests/tools/test_remotion_staging_lifecycle.py — 16 tests.

Each was confirmed to fail without the fix: 14 fail against pristine 4eab34c, all 16 pass with it.

No regressions in tests/tools/:

result
baseline 4eab34c 30 failed, 244 passed
this branch 30 failed, 260 passed

Same 30 pre-existing failures (missing API keys / optional deps in a clean venv); the +16 is exactly this PR's additions.

Not covered: no end-to-end Remotion CLI render was run. The tests exercise the Python staging path with the CLI stubbed, so they do not prove staticFile() resolves a mirrored asset at actual render time. Worth a maintainer spot-check on a machine with remotion-composer/node_modules installed.

Checklist

  • The change is focused on a single logical concern.
  • I ran the relevant tests locally where applicable.
  • I updated docs/README if behavior or usage changed. — no behavior/usage change; internal staging only.
  • No unrelated files (build artifacts, local config) are included in the diff.

_stage_remotion_media / --public-dir staging (9482edd) has four defects.
Each is fixed here with a regression test that fails without the change.

1. Naive Windows file:// URIs never resolve. A bare f"file://{path}" on
   Windows yields file://C:\Users\... — no slash after the scheme, so
   urlsplit puts the whole drive path in netloc with an empty path. That
   was read as a UNC authority (//C:\Users\...), which never resolves, so
   the asset was silently skipped rather than staged. Parsing is extracted
   to _file_uri_to_raw_path() so every URI shape is unit-testable off
   Windows; a genuine UNC host is still treated as a network path.

2. --public-dir REPLACES Remotion's default public dir, so assets earlier
   pipeline stages staged into remotion-composer/public/ (anime_scene
   .images[], screenshot_scene.backgroundImage, demo-props fixtures — all
   documented public/-relative staticFile() paths) 404 as soon as a render
   points at its own dir. _mirror_public_dir() links the real tree in,
   read-only, without clobbering staged media.

3. The staging dir was named only for the output stem, so it was shared by
   every render of that output: concurrent renders overwrote each other's
   media and the first to finish deleted the other's inputs, and cleanup
   could erase a pre-existing directory of that name. Added a random
   suffix so the dir removed is always one this invocation created.

4. Staging ran before the try/finally, so a failure after the directory
   was created but before the render (props write, setup) left staged user
   media on disk. The guard now spans the whole staging/setup/render
   lifetime.

Co-Authored-By: Claude Sonnet 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