[user-github-me]

Subprocess workers launched through runCancellableProcess now decode stdout and stderr incrementally, preserving Unicode filenames and diagnostics when a UTF-8 character spans chunks. The existing output limit counts raw bytes across both streams, and unfinished trailing bytes are flushed on close. Browser-download progress receives the same decoded text as captured stdout.

The CLI guide names browser installation and the media probes used by snapshot and remove-background as affected commands. Process-group cancellation, termination-reason preservation and download-stall handling retain current-main behavior. The overflow fixture stays alive until termination, covering the earlier Windows failure.

Validation: all 99 tests across seven process, browser and worker suites passed for the code revision, with three callback assertions that fail on the naive merge. Full build, CLI rebuild/typecheck, lint, formatting, reachability and normal hooks passed. The prose-only scope clarification passed Mint validation/link checks, normal hooks and both comment checks. CI was green before that documentation-only update; new checks will run on this head.

Duplicate #5296 is closed. This PR retains the earlier review history and is the active review and merge target.

[jrusso1020]

The decoder change is right, but the required Tests on windows-latest check is red at this head. One of the tests this PR adds fails on Windows, and it failed again on rerun:

FAIL src/utils/cancellableProcess.test.ts > cancellable process output > still rejects output that exceeds the combined raw byte limit
AssertionError: expected Error: taskkill exited with status 128 to match object { code: 'ENOBUFS', killed: true }

Why. In splitOutputSource, the second write overflows the 3-byte limit and the child exits right after it. terminate() starts terminateProcessTree. On Windows that spawns taskkill, which exits 128 (process not found) because the child has already gone. processTree.ts turns any non-zero taskkill status into a rejection. The close handler awaits terminationTask, catches that rejection and passes it to finishReject, so the taskkill error replaces the ENOBUFS CancellableProcessError. On POSIX, signalling a dead group is swallowed, which is why Linux and macOS pass.

Fix, either of:

• Test-only: keep the child alive after the overflow write, for example by ending splitOutputSource's timeout callback with setTimeout(() => {}, 10_000) in the overflow case. The kill then hits a live process on every platform.
• Behavioral (arguably the real bug): treat taskkill "process not found" (status 128) as success in terminateProcessTree. Alternatively, keep the original terminationReason when the tree teardown fails after the child has already closed. Otherwise any Windows caller whose child exits during an ENOBUFS, timeout or abort gets a confusing taskkill exited with status 128 instead of the real reason.

I'll approve once the Windows lane is green at the new head.

— Rames

[jrusso1020]

Thanks for this, @user-github-me. This PR now has merge conflicts with main. Could you rebase it onto the latest main? Once it's conflict-free we'll review it and get it into the merge queue.

[mintlify]

Preview approval required

This pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit 3096c61.

Approve preview

[miguel-heygen]

Thanks for keeping this going through the rebases. The earlier Windows failure is fixed: the overflow fixture now keeps the child process alive, so the kill always hits a live process, and the Windows engine/CLI job passes at this head.

I went through the change again on top of current main. One decoder per stream is the right fix, and counting raw bytes for the output limit is better than before, because a character split across chunks used to be counted as two replacement characters. The only consumer of onStdout is the browser install's line reader, which handles an empty piece fine (when only part of a character has arrived) and now gets correct text for progress and log lines. The decoders are flushed before the termination wait and after the settled check, so nothing is added after the promise is done. The tests go through runCancellableProcess itself, which is what every caller uses.

Verdict: APPROVE
Reasoning: Correct incremental decoding with raw-byte limits, the Windows test failure from the earlier review is fixed and passing in CI, and the branch merges cleanly with main.

[miguel-heygen]

On a second look at this head I'm changing my earlier approval, because of the docs sentence below. The code change is right. The change in runCancellableProcess is right: each stream gets its own StringDecoder, the output limit is counted on raw bytes across both streams before decoding, and the decoders are flushed on close so a trailing partial character becomes a replacement character instead of disappearing. I also checked the two onStdout callers (installInChild.ts line reader and the whisper transcriber). The callback can now receive an empty string when a chunk holds only part of a character, and both handle that fine. The tests call runCancellableProcess directly, which is the function the CLI's setup workers go through, and CI is green on this head.

One thing to fix before merge: the new sentence in docs/developers/cli.mdx (lines 51-53) says diagnostics from child tools keep Unicode intact across chunks, but several other places still decode each chunk on its own:

• packages/cli/src/capture/captureCompositionFrame.ts:868-870 (ffmpeg stderr)
• packages/cli/src/background-removal/pipeline.ts:425-427 (ffmpeg stderr)
• packages/cli/src/commands/render.ts:868-871 (docker stderr tail)
• packages/cli/src/utils/optionalPackages.ts:293-294 (runNpm error output)

For example, an ffmpeg error during frame capture that names 项目/音声.wav can still show a replacement character if the name crosses a chunk boundary. Either narrow the docs sentence to the processes this PR covers, or give those sites the same decoder treatment (collecting Buffers and decoding once at the end, as stoppableExecFile.ts already does, also works). Either is fine with me.

Small optional tidy-up: skipping the onStdout call when decoded is empty would keep the callback from firing with nothing.

#5296 is open at this same commit, so please keep one of the two.

Verdict: REQUEST_CHANGES
Reasoning: The decoding fix is correct and tested through the real function, but the added docs promise the same behavior for child tools that still decode per chunk.

[miguel-heygen]

Thanks for the quick follow-up. The docs sentence in docs/developers/cli.mdx now only promises Unicode-safe output for workers that go through runCancellableProcess, which matches what the code does, so my earlier concern is resolved. The code is unchanged from the head I reviewed before, where the full CI run was green, and #5296 is closed, so there is one PR to land.

One optional note: runCancellableProcess is an internal function name on a page readers use to pick a command, so they cannot tell which commands it covers. Dropping that paragraph, or describing it in terms of what users run (browser setup, snapshots, background removal), would read better. Not blocking.

Verdict: APPROVE
Reasoning: The only change since my last review narrows the docs sentence to what the code guarantees, which resolves the one blocking point; the code was already correct and green.

[miguel-heygen]

Thanks, this reads well now. The docs sentence in docs/developers/cli.mdx names what users run, and each part matches the code: browser installation goes through the setup worker, unzip and preflight paths, and the snapshot and remove-background media probes all use runCancellableProcess, so they get the new decoding. It also stays clear of the paths that still decode per chunk, like the main ffmpeg stderr in background removal. The code is unchanged from the version I reviewed before.

Verdict: APPROVE
Reasoning: The only change is a docs sentence that now matches the code paths that use the new decoding; the code is unchanged and was already green.

[jrusso1020]

Re-reviewed exact 3096c61. My earlier blocking Windows overflow-fixture concern is resolved.

The overflow-only child now stays alive after writing, so termination targets a live process instead of racing its exit (packages/cli/src/utils/cancellableProcess.test.ts:11–21,55–60). The current-head Windows run 40222793326 succeeded, including its engine-cli job; later skipped reruns are not additional execution evidence. I did not run Windows locally.

The code correctly keeps one StringDecoder per stream, counts combined raw bytes and flushes trailing bytes before settling (cancellableProcess.ts:82–84,119–143). Captured stdout and onStdout reconstruct the same Unicode text. Browser-install line buffering and setup-result parsing tolerate empty decoded pieces and retain their newline/JSON protocols. Abort/timeout/process-tree cleanup remains intact.

The later documentation concern is resolved too: docs/developers/cli.mdx:51–53 names browser installation and snapshot/remove-background media probes, whose callers use this helper. It does not promise Unicode-safe decoding for every other FFmpeg/Docker stderr path. Runtime/test blobs are unchanged since 18da975; subsequent commits narrow this prose.

Fresh local validation: 13 tests across cancellableProcess/processTree pass. Naive per-chunk decoding causes 5 output tests to fail, with 3 passing and 3 unrelated tests filtered out; original source restored and verified. A fresh adversarial pass plus my replay passes 7 independent real-child control groups: one-byte Unicode on both streams/callback, Unicode JSON-result parsing, old-decoder negative control, exact raw-byte budget, overflow, timeout tail flush and original abort-reason identity. Fresh review-gap pass completed.

Audited: all three changed files, full process lifecycle, termination and consumer boundaries. Unverified: local Windows execution and the author's wider 99-test/full-build claims were not rerun here.

Need: yes · Size: focused decoder repair · Reuse: Node StringDecoder and existing process-tree teardown · Simpler: separate stream decoders with one byte counter · Structure: fine.

— hf-review

Verdict: APPROVE
Reasoning: The prior Windows test race and overbroad docs claim are repaired; exact-head CI and discriminating process controls support the fix.