fix(subprocess): harden process spawning on Windows (MSYS paths + process-tree kill) - #93
Open
Zlatanwic wants to merge 3 commits into
Open
fix(subprocess): harden process spawning on Windows (MSYS paths + process-tree kill)#93Zlatanwic wants to merge 3 commits into
Zlatanwic wants to merge 3 commits into
Conversation
Adapters' tierGlobal resolvers feed `which <name>` output straight back as cmd[0]. Under Git Bash / MSYS that returns MSYS drive paths (`/d/...`) which Bun.spawn cannot resolve, producing ENOENT uv_spawn. Convert `/x/...` to `X:/...` (trying .exe for bun-compiled binaries like node_modules/.bin/pi) on win32; no-op elsewhere and for bare PATH names.
runSubprocess's timeout only called proc.kill() (SIGTERM to the direct child). Adapter wrappers (pi.exe, opencode) spawn grandchildren that survive and keep the stdout pipe open, so proc.exited / Response.text() never resolve and the timeout never actually fires — a stuck agent hangs the whole bench run (observed: pi on tb-adaptive-rejection-sampler ran 11min past a 600s timeout, bun at 4.65GB, zero tasks completed). Two fixes: - killProcessTree: taskkill /T /F on Windows (SIGKILL the group elsewhere) to take down the wrapper AND its descendants. - Replace Response(stream).text() with cancellable reader.read() loops; on timeout we cancel the readers so runSubprocess returns promptly with partial output instead of blocking on a pipe held open by a killed process's orphaned grandchild. Verified: sleep 30 with 2s timeout now returns in ~2.6s (was 30s).
This was referenced Jul 7, 2026
There was a problem hiding this comment.
Pull request overview
This PR hardens the runSubprocess implementation to be more robust on Windows (Git Bash / MSYS) by normalizing MSYS-style absolute paths before spawning, and by improving timeout teardown to avoid hangs caused by still-open stdout/stderr pipes.
Changes:
- Normalize MSYS
/d/...drive paths (and optionally add.exe) forcmd[0]before callingBun.spawnon Windows. - On timeout, attempt to kill the whole process tree (Windows
taskkill /T /F, POSIX process-group SIGKILL) and cancel stdout/stderr readers to return promptly with partial output. - Replace
Response(stream).text()with manual stream readers for stdout/stderr to support cancellation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two robustness fixes to
src/core/subprocess.tsfor spawning external processes, both surfaced while running the benchmark harness on Windows (Git Bash / MSYS).1. Resolve MSYS drive paths for
Bun.spawnon WindowsUnder Git Bash, absolute paths arrive MSYS-style (
/d/SkVM/...).Bun.spawnon Windows expects native paths (D:/SkVM/...), socmd[0]could fail to resolve. This normalizes the spawn executable path before handing it to Bun.2. Kill the whole process tree on timeout + cancel pipe readers
On timeout we previously killed only the direct child, leaving grandchildren (e.g. a shell that forked a long-running tool) orphaned. This now:
Why it's standalone
These are pure
subprocess.tsinternals with no signature changes torunSubprocess. No other module needs to change.Test plan
bunx tsc --noEmitpasses/d/...paths spawn correctlyScope
Additive/hardening only. No breaking changes.