Skip to content

fix(ffmpeg): unload the instance when the core traps or aborts - #88

Merged
Project516 merged 3 commits into
masterfrom
fix/unload-after-trap
Oct 6, 2026
Merged

Project516 merged 3 commits into
masterfrom
fix/unload-after-trap

Conversation

@Project516

@Project516 Project516 commented Oct 6, 2026 •

Copy link
Copy Markdown
Owner

What and why

When the core traps or aborts (a wasm RuntimeError, an Emscripten abort, unreachable), the failing call rejects with the error, every pending call rejects with a "crashed" error, and the FFmpeg instance is unloaded (loaded is false). Later calls reject with the not-loaded error and load() starts a fresh worker. The core's memory can be corrupt after a trap, so it is not reused.

Detection is on the main thread: the worker already reports errors as toString() strings, and every trap and abort string starts with RuntimeError. Both workers (browser and Node) need no change.

Upstream: ffmpegwasm#979 (only the unload-after-trap part; the Vue proxy and transfer: false parts are already here).

Tests

  • Browser suite (st and mt): a stand-in worker (via classWorkerURL) answers exec with RuntimeError: unreachable. Asserts the call rejects, loaded is false, a later call rejects as not loaded, and load() then works with the real core. Also asserts an ordinary error leaves the instance loaded.
  • Node suite: only the ordinary-error case. No command traps the current core, and the Node adapter ignores classWorkerURL, so there is no deterministic way to inject a trap there.

Not run locally

Browser and Node suites need built cores and a browser; they run in CI. pnpm lint and a type check ran locally.

Noticed, not fixed

  • A native abort() inside exec()/ffprobe() (for example a failed av_assert) is swallowed by bind.js, because the normal exit path also ends in abort(). It returns -1 and the instance stays loaded, so it does not take this unload path.

Review

review-bot's verification step failed on this head, and its note about loaded is answered in a comment. A Sonnet reviewer confirmed the error string handling in both workers, the reject order, and that the public terminate() is unchanged. It found one medium issue: bind.js swallows an Emscripten abort() inside exec() and ffprobe(), so only wasm traps unload the core. The CHANGELOG and docs now say so. Making aborts unload too would change how exec() reports a crash, so it is left for a separate decision.

@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 29 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 4267286d-be56-45dc-8f9e-9ba82034a919
📥 Commits

Reviewing files that changed from the base of the PR and between 73567d3 and 5ff6b43.

📒 Files selected for processing (6)
  • CHANGELOG.md
  • apps/website/docs/getting-started/usage.md
  • packages/ffmpeg/src/classes.ts
  • packages/ffmpeg/src/errors.ts
  • tests/ffmpeg-node.test.mjs
  • tests/ffmpeg.test.js
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

Comment @coderabbitai help to get the list of available commands.

@project516-review-bot project516-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The change modifies the FFmpeg class to unload the instance when a core trap or abort is detected (by checking for a string starting with 'RuntimeError' in an error message from the worker). It adds a new ERROR_CRASHED error, refactors the terminate method to use a private #terminate method, and updates documentation and tests to reflect the new behavior.

Not approved: the check on these comments could not run, so they are unverified.

Other notes

  • packages/ffmpeg/src/classes.ts: Add this.#loaded = false; at the start of this method to mark the instance as unloaded when terminating due to a crash or explicit terminate. Without this, the loaded property remains true after a core trap, which is incorrect.

review-bot, model nvidia/nemotron-3-super-120b-a12b:free, verdict comment

@Project516

Copy link
Copy Markdown
Owner Author

On review-bot's note: #terminate already sets this.loaded = false when it tears down the worker (packages/ffmpeg/src/classes.ts:358). The ERROR handler at line 109 calls it after rejecting the failing call, so loaded is false after a trap. The browser test asserts it. No change.

@project516-review-bot project516-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The change modifies the FFmpeg class to unload the instance when the core traps or aborts (detected by a RuntimeError string from the worker). It adds a new ERROR_CRASHED error, updates the error handler to call a private terminate method on RuntimeError, and updates documentation and tests to reflect the new behavior. The public terminate method is refactored to use the private terminate method.


review-bot, model nvidia/nemotron-3-super-120b-a12b:free, verdict approve

@Project516

Copy link
Copy Markdown
Owner Author

The browser tests job fails on the new "core trap" test in both st and mt: Error: Cannot find module 'blob:http://localhost:3000/...' thrown from load() in dist/umd/ffmpeg.js. The test pages load the UMD build, and webpack compiles new Worker(new URL(classWorkerURL, import.meta.url)) in classes.ts into a module lookup, so any runtime classWorkerURL fails in the UMD build. That is a library bug, not just a test bug: classWorkerURL only works from the ESM build. Next step: keep the URL out of webpack's reach in the UMD build (for example new URL(/* webpackIgnore: true */ classWorkerURL, import.meta.url) if our webpack version supports it, or pass the string straight to new Worker when it is already absolute), then rerun this test. Not fixed yet.

@project516-review-bot project516-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The PR adds handling for wasm traps (RuntimeError) in the worker message handler to unload the FFmpeg instance. It also updates the changelog, documentation, and adds tests to verify the behavior.


review-bot, model nvidia/nemotron-3-super-120b-a12b:free, verdict approve

@Project516
Project516 merged commit 6a8124e into master Oct 6, 2026
20 checks passed
@Project516
Project516 deleted the fix/unload-after-trap branch October 6, 2026 16:22
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