Skip to content

fix(cli): stopping remove-background on a Mac no longer aborts the process - #6848

Open
miguel-heygen wants to merge 1 commit into
mainfrom
dsec/rb-stop-race
Open

miguel-heygen wants to merge 1 commit into
mainfrom
dsec/rb-stop-race

Conversation

@miguel-heygen

Copy link
Copy Markdown
Collaborator

Stopping hyperframes remove-background on a Mac (Ctrl-C, a timeout, a kill, a quit of the app that started it) no longer aborts the process with libc++abi: terminating due to uncaught exception of type std::__1::system_error: mutex lock failed: Invalid argument. The run stops with a message, removes its partial output, and exits.

Two things were wrong together, and each alone still aborted:

  • The shutdown released the CoreML session while a frame was inside it. registerBrowserSessionCleanup ran session.release() on the signal, with no wait for the frame in flight. The session now remembers that frame: a stop waits for it, releases once, and refuses the next frame ("Background removal was stopped."), so the run unwinds through its own finally (the .tmp partial is removed). close() after a stop does not release twice.
  • The CLI then ended the process with process.exit(). With CoreML's runtime loaded, exit() runs its static destructors before Node has torn the addon down, and one of them locks a destroyed mutex. A run that used CoreML now ends the way a finished run does, by letting the event loop drain (process.exitCode set, endProcess in commandResult.ts, with cli.ts still the only place that touches process.exit). A forced exit follows after 8 s if something still holds the loop.

A run that finishes on its own was never affected (it exits through the drain already).

What I measured

fails without the fix: packages/cli/src/background-removal/inference.stop.test.ts "releases the session only after that frame has ended" fails on main's inference.ts (the session is released while the frame is inside it); packages/cli/src/utils/commandResult.test.ts "after CoreML ran, leaves the exit to Node's own teardown and forces it only after a grace" covers the exit half. Both pass with the change, with inference.coremlTemp and the rest of commandResult; check:cli-process-ownership, oxfmt --check and oxlint are clean.

On an Apple Silicon Mac with the CLI of the current release (remove-background clip.mp4 -o out.webm --quality best on a 60 s 1080p 25 fps clip, CoreML), SIGTERM 20 s in:

build runs result
released CLI 3 of 3 exit 134, the mutex lock failed abort, a .tmp partial left
the same CLI with only the session change 3 of 3 still exit 134
the same CLI with only the exit change 3 of 3 still exit 134 (the session was released under the frame)
both changes, applied to the released CLI's compiled files 3 of 3 no abort, "Background removal was stopped.", no .tmp left, exit 1

A full run (250 s, 6,250 frames, CoreML, 11.5 fps) with no signal finished with exit 0 on the released CLI, so the abort needs a stop.

What I did NOT exercise

  • The built CLI from this branch: the Mac runs used the same two changes applied by hand to the released CLI's compiled files; the source here is tested by its unit tests on Linux, where CoreML does not exist.
  • SIGINT and SIGHUP (same handler as SIGTERM, only SIGTERM was sent), Windows and Linux (no CoreML there, so nothing changes for them).
  • Why the exit code is 1 and not 143 after a stop: the command's own failure sets it after the exit request; unchanged here.
  • Other forced process.exit paths (a closed pipe, a failure after the output is written) when CoreML ran: they still call exit() directly.

@github-actions

github-actions Bot commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 271 (base branch 271), smooth 116 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

Unstable (1)

  • seqplainkeys-none-pct-r0-nested-z100: tracking 0.05, pressJump 0, drop 0, reload 0, render 0.02, renderKey -, undo true, teleport true / tracking 0.05, pressJump 0, drop 0, reload 0, render 0.02, renderKey -, undo true, teleport true / tracking 0.05, pressJump 0, drop 0, reload 0, render 0.02, renderKey -, undo true, teleport true

@heygengenesis heygengenesis Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Verdict: APPROVE · 0 blocker · 1 important · 0 nits · head a0b24221

Checks

Ran locally: both tests in inference.stop.test.ts fail against main's inference.ts (released during the frame, then a 20 s hang) and pass at head, along with commandResult.test.ts (7/7). pipeline.ts awaits session.process one frame at a time, so a single inFlight covers every frame. close() after a stop releases once. CI is green at head, including Windows render verification. The only other in-process caller is preview's studio server (studioServer.ts:739); see the inline comment.

export function endProcess(exitCode: number, host: ProcessEnd): void {
if (!drainsToExit) return host.exit(exitCode);
host.setExitCode(exitCode);
host.later(() => host.exit(exitCode), DRAIN_GRACE_MS).unref();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

important (non-blocking): drainToExit() is process-wide, and preview runs this pipeline in-process (studioServer.ts:739). After any CoreML job in Studio, the preview's 3 s hard deadline (preview.ts:1741, requestCliExit(0) for a hung shutdownStudio(), e.g. dead Chrome) only sets exitCode and arms this 8 s timer. A hung Studio quit then takes about 11 s instead of 3 s. If you want to keep the 3 s deadline, give endProcess a shorter grace or skip the drain when no session is live (clear the flag in close()).

This branch has not been deployed

No deployments
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