Repository navigation
fix(ffmpeg): unload the instance when the core traps or aborts - #88
Conversation
|
Warning Review limit reachedYou'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. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (6)
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. Comment |
There was a problem hiding this comment.
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: Addthis.#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
|
On review-bot's note: |
There was a problem hiding this comment.
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
|
The browser |
There was a problem hiding this comment.
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
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 theFFmpeginstance is unloaded (loadedisfalse). Later calls reject with the not-loaded error andload()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 withRuntimeError. Both workers (browser and Node) need no change.Upstream: ffmpegwasm#979 (only the unload-after-trap part; the Vue proxy and
transfer: falseparts are already here).Tests
classWorkerURL) answersexecwithRuntimeError: unreachable. Asserts the call rejects,loadedis false, a later call rejects as not loaded, andload()then works with the real core. Also asserts an ordinary error leaves the instance loaded.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 lintand a type check ran locally.Noticed, not fixed
abort()insideexec()/ffprobe()(for example a failedav_assert) is swallowed bybind.js, because the normal exit path also ends inabort(). It returns-1and 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.