Skip to content

@remotion/renderer: Don't crash the process when the compositor quits while receiving commands - #12592

Closed
parbhatkapila4 wants to merge 2 commits into
remotion-dev:mainfrom
parbhatkapila4:fix/compositor-epipe
Closed

parbhatkapila4 wants to merge 2 commits into
remotion-dev:mainfrom
parbhatkapila4:fix/compositor-epipe

Conversation

@parbhatkapila4

Copy link
Copy Markdown
Contributor

If the compositor quits while commands are still being written to it, for example, after a panic or when the OS kills it for using too much memory, the write to its stdin fails with EPIPE. startCompositor() has no error listener on child.stdin, so Node throws an unhandled 'error' event, and the whole process exits, instead of the render failing with the compositor's error. ffmpeg doesn't have this problem because execa adds that listener.

Now stdin errors are logged with Log.verbose() instead. The command whose write failed is also no longer rejected with EPIPE: broken pipe, write. The close handler rejects it with the reason the compositor quit, like the other pending commands.

Added a test next to "Handle panics" that keeps sending Echo commands while the compositor panics. On main it fails with an unhandled EPIPE, and some commands get rejected with EPIPE: broken pipe, write, on both Linux and Windows. With the fix, every rejected command gets the compositor's error. I also tried it in Node, killing the compositor with SIGKILL while commands were being sent: on main, the Node process crashes with write EPIPE, and with the fix, every command is rejected with the compositor's SIGKILL error.

@vercel

vercel Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
bugs Ready Ready Preview Oct 24, 2026 9:10am UTC
remotion Ready Ready Preview Oct 24, 2026 9:10am UTC

Request Review

@pullfrog pullfrog 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.

ℹ️ No critical issues — one test-strength suggestion inline and one optional consistency note.

Reviewed changes

Reviewed the stdin error handling change in startCompositor() and the accompanying regression test.

  • stdin error listener: child.stdin.on('error', ...) now logs the failure with Log.verbose() instead of letting Node emit an unhandled 'error' event and kill the process (packages/renderer/src/compositor/compositor.ts:223).
  • executeCommand write: the write callback that rejected a command with the raw EPIPE is removed; a failed write now falls through to the close handler, which rejects every pending waiter with the compositor's exit reason (packages/renderer/src/compositor/compositor.ts:405).
  • Regression test: sends Echo commands concurrently with a DeliberatePanic and asserts every rejection starts with Compositor (packages/renderer/src/test/rust-errors.test.ts:73).

The reasoning holds: EPIPE on the compositor's stdin only occurs once the read end is closed, i.e. the process is exiting, so the close handler is the right place to surface the real error, and the listener keeps the event loop from throwing.

ℹ️ finishCommands() still surfaces a raw EPIPE

executeCommand() no longer rejects with EPIPE, but the sibling write in finishCommands() (packages/renderer/src/compositor/compositor.ts:328) still passes a write callback that rejects with the raw pipe error. If the compositor dies while the EOF sentinel is being written, the caller sees EPIPE: broken pipe, write rather than the compositor's exit reason — the outcome this PR removes from the command path. This no longer crashes (the new listener covers it) and runningStatus short-circuits most cases, so it is optional, but aligning it would make the error surface consistent.

Technical details
# Align finishCommands() EPIPE handling with executeCommand()

## Affected sites
- packages/renderer/src/compositor/compositor.ts:328 — `child.stdin.write('EOF\n', (e) => { if (e) { rej(e); ... } })` rejects with the raw write error.

## Required outcome
- A failed `EOF` write during shutdown should reject with the compositor's exit reason (as the `close` handler already does for pending waiters) instead of `EPIPE`, or the difference should be a deliberate, documented choice.

## Suggested approach (optional)
- Drop the callback here too and rely on the `close`/`runningStatus` paths, matching the new `executeCommand()` behavior.

Pullfrog  | Fix it ➔ | View workflow run | Using deepseek-v4.1-flash | 𝕏

@pullfrog

pullfrog Bot commented Oct 24, 2026

Copy link
Copy Markdown
Contributor

Follow-up on the review above — here is the inline suggestion it referenced, on packages/renderer/src/test/rust-errors.test.ts:105-109:

The loop only validates the rejections that exist; it never asserts that any command was actually rejected. If the compositor processes all Echo commands before the panic propagates, errors is all null and the guard passes without ever exercising the broken-pipe path. Consider asserting at least one rejection so the test cannot pass vacuously:

	const settledErrors = await Promise.all(errors);
	expect(settledErrors.some((err) => err !== null)).toBe(true);
	for (const err of settledErrors) {
		if (err) {
			expect(err.message).toStartWith('Compositor');
		}
	}

Pullfrog  | View workflow run | via Pullfrog | Using deepseek-v4.1-flash | 𝕏

@parbhatkapila4

Copy link
Copy Markdown
Contributor Author

Added the assertion from the inline suggestion.

I left finishCommands() as it is on purpose. It's also called on its own (for example, in get-video-metadata.ts), so it should still reject if it can't write EOF, and it can't crash now that stdin has an error listener. Rejecting with the exit reason would mean calling waitForDone() from inside it, and waitForDone() only keeps one resolve/reject, so that could leave another caller waiting forever.

@samohovets

Copy link
Copy Markdown
Member

Thanks for the PR! Before we review the fix, can you tell us more about the problem itself?

  • Did you run into this in a real render? If so, please open an issue with your Remotion version, OS, how you render (CLI, renderMedia(), Lambda, etc.), and the full error or stack trace you saw.
  • What made the compositor quit in your case (a panic, OOM kill, something else)? If it panicked, we'd rather fix the panic.
  • A minimal repro that doesn't use DeliberatePanic or a manual SIGKILL would help a lot.

We prefer to merge fixes that are tied to a problem someone actually hit, so we can confirm that the fix solves it.

@parbhatkapila4

Copy link
Copy Markdown
Contributor Author

Honest answer: no, I didn't hit it in a real render. I found it while reading compositor.ts. stdin has no 'error' listener, so if the compositor dies mid-write (an OOM kill, say), Node crashes on an unhandled EPIPE instead of surfacing the real exit reason.

No repro or user report, so totally fine to close if you only take reported issues. Happy to pick up something from the tracker instead.

@samohovets

Copy link
Copy Markdown
Member

Thanks for understanding, please file an issue first next time!

@samohovets samohovets closed this Oct 26, 2026

This branch was successfully deployed

2 active deployments
Preview – remotion — 13d6c724 Deployed Oct 24, 2026 by vercel[bot]
Preview – bugs — 13d6c724 Deployed Oct 24, 2026 by vercel[bot]
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.

2 participants