Skip to content

fix(command): no timer leak on spawn error + UTF-8 chunk reassembly#50

Open
Asgarrrr wants to merge 1 commit into
mainfrom
fix/command-observer-correctness
Open

fix(command): no timer leak on spawn error + UTF-8 chunk reassembly#50
Asgarrrr wants to merge 1 commit into
mainfrom
fix/command-observer-correctness

Conversation

@Asgarrrr

@Asgarrrr Asgarrrr commented May 8, 2026

Copy link
Copy Markdown
Owner

Summary

Two correctness fixes in CommandObserver.observe (src/command/command-observer.ts). Both are tiny — total +19/-6 in production code, +12 lines of test.

What changed

# Bug Fix
1 await closed; clearTimeout(timer) left the timer armed if closed rejected (bad cwd, EMFILE, fork failure) — default 300s timer kept the event loop alive after observe() rejected, and could fire a SIGKILL on a stale pid later. Switched to await closed.finally(() => clearTimeout(timer)).
2 Each chunk in the data handlers was decoded with chunk.toString("utf8") independently, so a multibyte UTF-8 codepoint split across two emits became two replacement-char fragments — non-ASCII output corruption (emoji, CJK, accented French/EU text) under stream pressure. Added proc.stdout?.setEncoding("utf8") / proc.stderr?.setEncoding("utf8") so chunks flow through Node's StringDecoder, which buffers partial codepoints across emits.

Why now

The H1 audit (orphan-process timeout) hardened the kill path; this PR closes two adjacent correctness holes on the same code path:

  1. The H1 fix introduced the timer + group-kill logic. The cleanup branch was incomplete — a proc.error event still leaks the timer because the rejection skips the post-await clearTimeout. Promise.finally makes it symmetric.
  2. The original buffer-concat path predates UTF-8 awareness. With long noisy commands (tsc, bun test) emitting >64KB-ish output, chunks fragment at arbitrary byte boundaries. ASCII tests didn't surface the issue because every codepoint is one byte.

Neither bug had a reported user incident, but both are deterministic correctness gaps under load and on non-ASCII output.

Test plan

  • bun test tests/command-observer.test.ts — 14 pass / 0 fail (13 existing + 1 new bad-cwd test)
  • bun test — 317 pass / 0 fail / 23 files
  • All H1 group-kill tests pass unchanged (the timer-leak fix is orthogonal to the kill path)
  • Targeted regression: observer.observe("echo hi", "<missing-dir>") rejects rather than hangs (new test)

Risk / rollback

  • Risk: setEncoding switches the stream from object-mode-Buffer to object-mode-string. Any future code reading proc.stdout directly would receive strings instead of Buffers. Today there is no such consumer; the only reader is the data handler in this file.
  • Rollback: single commit, clean git revert.

Out of scope (deferred)

  • Unbounded buffer growthstdoutBuf / stderrBuf / combinedBuf grow without limit; a multi-hundred-MB output command can OOM the host. Real concern, but the fix (streaming-to-disk + capped in-memory tail for the returned ObserveResult) is a bigger semantic change than warranted here.
  • Three-tier kill catches all errors, not just ESRCH — defense-in-depth; the empty catch masks permission errors. Low impact, separate PR if at all.

Two correctness fixes in `CommandObserver.observe`:

- Switch the timeout cleanup from a post-await `clearTimeout(timer)` to
  `await closed.finally(() => clearTimeout(timer))`. Without it, a
  `proc.error` event (bad cwd, EMFILE, fork failure) rejected the
  `closed` promise without ever clearing the timer — leaving the
  default 300s timeout armed on the event loop after observe()
  rejected, and potentially firing a SIGKILL on a stale pid later.
- Add `setEncoding("utf8")` on stdout/stderr streams. Decoding each
  Buffer chunk independently with `chunk.toString("utf8")` corrupts
  multibyte codepoints split across two `data` events into pairs of
  replacement-character fragments. `setEncoding` routes chunks through
  Node's StringDecoder, which buffers partial codepoints across emits.

Adds one test asserting observe() rejects on an invalid cwd, documenting
the error-path contract that the timer-leak fix relies on.
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