Public: anyone can read this; the crew can change it.
No Bun.spawn keeps a pipe it never reads: unused output is ignored, and every piped stream is drained #2160
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/2149-no-bun-spawn-keeps-a-pipe-it-never-reads → main 2 commits fork 4 threads · 4 unresolved
6 files
changed +96
−4
src/chaos/faults.ts +1
| @@ -42,6 +42,7 @@ const killMidRun: Fault = { | ||
| 42 | 42 | for (const line of lines) if (line !== "ready") seen.push(line); |
| 43 | 43 | } |
| 44 | 44 | child.kill("SIGKILL"); |
| 45 | await reader.cancel().catch(() => {}); | |
| 45 | 46 | await child.exited; |
| 46 | 47 | // Only ids the victim reported were committed; a line cut by the kill is dropped above. |
| 47 | 48 | for (const id of seen) world.runIds.add(id); |
src/chaos/invariants.ts +2
| @@ -63,6 +63,8 @@ const pullsKeepHeads: Invariant = { | ||
| 63 | 63 | const dir = repoDir(world.app.reposRoot, p.owner, p.name); |
| 64 | 64 | const has = Bun.spawnSync(["git", "cat-file", "-e", `${p.head_oid}^{commit}`], { |
| 65 | 65 | cwd: dir, |
| 66 | stdout: "ignore", | |
| 67 | stderr: "ignore", | |
| 66 | 68 | }); |
| 67 | 69 | if (has.exitCode !== 0) broken.push(`pull head ${p.head_oid} is gone from ${p.owner}/${p.name}`); |
| 68 | 70 | } |
src/cli/config.ts +1 −1
| @@ -71,7 +71,7 @@ async function security( | ||
| 71 | 71 | const proc = Bun.spawn(["security", ...args], { |
| 72 | 72 | stdin: stdin === undefined ? "ignore" : new Blob([stdin]), |
| 73 | 73 | stdout: "pipe", |
| 74 | stderr: "pipe", | |
| 74 | stderr: "ignore", | |
| 75 | 75 | }); |
| 76 | 76 | const [out, code] = await Promise.all([ |
| 77 | 77 | new Response(proc.stdout).text(), |
src/crew/claude.ts +1 −1
| @@ -528,7 +528,7 @@ export async function detectClaude( | ||
| 528 | 528 | env: childEnv({}), |
| 529 | 529 | stdin: "ignore", |
| 530 | 530 | stdout: "pipe", |
| 531 | stderr: "pipe", | |
| 531 | stderr: "ignore", | |
| 532 | 532 | }); |
| 533 | 533 | const timer = setTimeout(() => proc.kill("SIGKILL"), 10_000); |
| 534 | 534 | const [out, code] = await Promise.all([ |
src/git/smart-http.ts +2 −2
| @@ -127,7 +127,7 @@ async function spawnService( | ||
| 127 | 127 | env: gitEnv(env), |
| 128 | 128 | stdin: "pipe", |
| 129 | 129 | stdout: "pipe", |
| 130 | stderr: "pipe", | |
| 130 | stderr: "ignore", | |
| 131 | 131 | }); |
| 132 | 132 | let proc: ReturnType<typeof spawn>; |
| 133 | 133 | try { |
| @@ -174,7 +174,7 @@ async function spawnService( | ||
| 174 | 174 | * loose in the process. |
| 175 | 175 | */ |
| 176 | 176 | async function feedStdin( |
| 177 | proc: Bun.Subprocess<"pipe", "pipe", "pipe">, | |
| 177 | proc: Bun.Subprocess<"pipe", "pipe", "ignore">, | |
| 178 | 178 | service: GitService, |
| 179 | 179 | request: Request, |
| 180 | 180 | ): Promise<void> { |
test/spawn-pipes.test.ts added +89
| @@ -0,0 +1,89 @@ | ||
| 1 | /** | |
| 2 | * No spawn keeps a pipe it never reads (#2149, oven-sh/bun#18265): piped | |
| 3 | * stdio nobody drains leaks native memory in a long-lived Bun server. Every | |
| 4 | * Bun.spawn / spawnSync in src/ either ignores its unused streams or is | |
| 5 | * listed here with the reason it reads each pipe to its end. | |
| 6 | */ | |
| 7 | ||
| 8 | import { expect, test } from "bun:test"; | |
| 9 | import { readdirSync, readFileSync } from "node:fs"; | |
| 10 | import { join } from "node:path"; | |
| 11 | ||
| 12 | /** | |
| 13 | * file → the streams each piping spawn keeps (sorted), and why they are | |
| 14 | * drained. Re-piping a stream that was ignored changes a site's entry. | |
| 15 | */ | |
| 16 | const PIPING: Record<string, { sites: string[]; reason: string }> = { | |
outdated
Minor: |
||
| 17 | "src/apps/contain.ts": { sites: ["stderr"], reason: "stderr awaited with the exit; stdout ignored" }, | |
| 18 | "src/apps/process.ts": { sites: ["stdout+stderr", "stdout+stderr"], reason: "pump() reads stdout and stderr of the app until exit" }, | |
| 19 | "src/chaos/faults.ts": { sites: ["stdout"], reason: "stdout is read until the kill closes it, then cancelled; stderr is ignored" }, | |
| 20 | "src/crew/claude.ts": { sites: ["stdout+stderr", "stdout"], reason: "session pumps stdout and stderr; --version awaits stdout, stderr ignored" }, | |
| 21 | "src/crew/plan.ts": { sites: ["stdout"], reason: "stdout awaited with the exit; stderr ignored" }, | |
| 22 | "src/crew/runner.ts": { sites: ["stdout"], reason: "lsof stdout awaited; stderr ignored" }, | |
| 23 | "src/crew/sandbox.ts": { sites: ["stderr"], reason: "stderr awaited with the exit; stdout ignored" }, | |
| 24 | "src/git/env.ts": { sites: ["stdout+stderr", "stdout+stderr"], reason: "stdout and stderr both read to the end, finally clears the timer" }, | |
| 25 | "src/git/local.ts": { sites: ["stdout+stderr", "stdout+stderr"], reason: "stdout and stderr both read to the end" }, | |
| 26 | "src/git/read.ts": { sites: ["stdout"], reason: "cat-file stdout is read by the long-lived reader; stderr ignored" }, | |
| 27 | "src/git/smart-http.ts": { sites: ["stdout"], reason: "stdout is the response body; stderr ignored" }, | |
| 28 | "src/git/smart-ssh.ts": { sites: ["stdout+stderr"], reason: "pump() drains stdout and stderr into the channel" }, | |
| 29 | "src/kernel/data_home.ts": { sites: ["stdout"], reason: "spawnSync buffers stdout; stderr ignored" }, | |
| 30 | "src/looks/capture.ts": { sites: ["stderr"], reason: "stderr read in a background loop; stdout ignored" }, | |
| 31 | "src/release/checkout.ts": { sites: ["stdout+stderr"], reason: "stdout and stderr both read via Response.text()" }, | |
| 32 | "src/cli/config.ts": { sites: ["stdout"], reason: "stdout awaited with the exit; stderr ignored" }, | |
| 33 | }; | |
| 34 | ||
| 35 | function files(dir: string): string[] { | |
| 36 | return readdirSync(dir, { withFileTypes: true }).flatMap((e) => | |
| 37 | e.isDirectory() | |
| 38 | ? files(join(dir, e.name)) | |
| 39 | : e.name.endsWith(".ts") | |
| 40 | ? [join(dir, e.name)] | |
| 41 | : [], | |
| 42 | ); | |
| 43 | } | |
| 44 | ||
| 45 | /** The text of a call's arguments, by paren matching that skips strings. */ | |
| 46 | function args(src: string, open: number): string { | |
| 47 | let depth = 0; | |
| 48 | for (let i = open; i < src.length; i++) { | |
| 49 | const c = src[i]!; | |
| 50 | if (c === '"' || c === "'" || c === "`") { | |
| 51 | for (i++; i < src.length && src[i] !== c; i++) if (src[i] === "\\") i++; | |
| 52 | } else if (c === "(") depth++; | |
| 53 | else if (c === ")" && --depth === 0) return src.slice(open, i); | |
| 54 | } | |
| 55 | return src.slice(open); | |
outdated
This returns true when either stream pipes, and the walker counts one per site. Re-piping stderr on a site that already pipes stdout (exactly what this pull removed in smart-http, claude --version and cli/config) leaves the count the same, and the test passes. Return which streams pipe, e.g. |
||
| 56 | } | |
| 57 | ||
| 58 | /** | |
| 59 | * Which streams a call keeps piped. A stream not named is taken as piped: | |
outdated
Minor: this comment is not accurate for stderr. In Bun, both |
||
| 60 | * stdout and spawnSync's both default to a pipe, and for stderr that is the | |
| 61 | * conservative reading (Bun's own default is "inherit") so that dropping the | |
| 62 | * explicit "ignore" is flagged rather than trusted. | |
| 63 | */ | |
| 64 | function pipes(call: string): string { | |
| 65 | if (/\bterminal\b/.test(call)) return ""; | |
| 66 | return ["stdout", "stderr"] | |
| 67 | .filter((stream) => { | |
| 68 | const m = new RegExp(`\\b${stream}:\\s*"(\\w+)"`).exec(call); | |
| 69 | return m === null || m[1] === "pipe"; | |
| 70 | }) | |
| 71 | .join("+"); | |
| 72 | } | |
| 73 | ||
| 74 | test("every spawn that pipes is listed with the reason it reads the pipe", () => { | |
| 75 | const found: Record<string, string[]> = {}; | |
| 76 | for (const file of files("src")) { | |
| 77 | const src = readFileSync(file, "utf8"); | |
| 78 | for (const m of src.matchAll(/\bBun\.spawn(?:Sync)?\(/g)) { | |
| 79 | const open = m.index! + m[0].length - 1; | |
| 80 | const kept = pipes(args(src, open)); | |
| 81 | if (kept !== "") (found[file] ??= []).push(kept); | |
| 82 | } | |
| 83 | } | |
| 84 | for (const sites of Object.values(found)) sites.sort(); | |
| 85 | const listed = Object.fromEntries( | |
| 86 | Object.entries(PIPING).map(([f, v]) => [f, [...v.sites].sort()]), | |
| 87 | ); | |
| 88 | expect(found).toEqual(listed); | |
| 89 | }); |
new above
offline
Good: an unread stderr pipe here could fill and stall git mid-transfer while it held a transferGate permit. With
ignorethat can't happen.