Repository navigation
fix: launch child processes asynchronously in unit tests - #1432
Conversation
Bun 1.4.2's synchronous spawns can miss a child's exit and hang the calling test until its timeout, also under `--no-isolate`. Unit tests and the buildIfNeeded code they run in-process now use spawnAsync, which closes the child's stdin after any input like spawnSync does. Co-authored-by: WillBooster (Claude Code) <agent@willbooster.com>
Co-authored-by: WillBooster (Claude Code) <agent@willbooster.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses rare, intermittent test hangs observed in CI environments. By replacing synchronous process spawning with asynchronous alternatives across the test suite and specific production commands, the changes eliminate issues related to Bun's private loop corruption. The PR also enforces a new repository rule requiring asynchronous child process launching in unit tests to ensure long-term stability. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. A test that would hang in the night, Gave developers quite a big fright. With spawnAsync now, We make a deep bow, And tests run with all of their might. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request migrates synchronous child process executions to asynchronous ones using spawnAsync (or Bun.spawn/Bun.which) across various packages and test suites to prevent tests from hanging under Bun 1.4.2. It also updates the repository rules documentation to reflect this requirement. A review comment correctly points out that using an async function as a describe callback in applyDockerEnv.test.ts is an anti-pattern and should be made synchronous.
Co-authored-by: WillBooster (Claude Code) <agent@willbooster.com>
Co-authored-by: WillBooster (Claude Code) <agent@willbooster.com>
Close #1429
Why
After #1421 made
wb testrun unit tests with--parallel --no-isolate, a test still hung in a synchronous spawn: in run 36857401401 (attempt 1),startCommand.test.ts'swb start --dry-runtest ran into its 5 s timeout and Bun printedkilled 1 dangling process. In 21 other CI runs the same test took 0.38–2.7 s.The hang is rare. Since #1421, 1 of 22 Test workflow attempts hit it. 220 extra runs of the wb suite on
ubuntu-24.04did not hit it at all. Those runs had a watchdog that dumped/procstate for any test-worker child that stayed a zombie for 1.5 s, and it never fired. So no live process state was captured. The signature matches oven-sh/bun#34069, and the maintainers' fix PR describes a cause that--no-isolatedoes not remove. WhilespawnSyncwaits, the GC can finalize any object that owns a poll on the main loop (aFileSink, aSubprocess, a socket). That corrupts the poll count of the private loop the wait runs on, so a laterspawnSyncmisses its child's exit.--no-isolateonly removed the most frequent such object (each file's stdout/stderr writers). The wb test workers still made about 900 synchronous spawns per run (about 360 s spent waiting, 196 s of it indeployRailway.test.ts's 60-second CLI runs). They did this next to in-process async spawns, sockets, and servers.Async spawns never use that private loop, so this PR moves the unit tests off synchronous spawns instead of tuning timeouts or retries.
Requirements
spawnSyncbug surviving--no-isolateor a different cause, from the evidence, and fix it at that rootspawnSyncpath, e.g. by replacingspawnSyncin the affected code paths and tests with async spawns where feasiblewb testkeeps running unit test files in parallel worker processes (the contract indocs/expected-repository-rules.md)spawnAsyncof@willbooster/shared-lib-noderather than a new helper, because it already returnsspawnSync'sstatus/stdout/stderrshape and every package can import itspawnAsynccloses the child's stdin after the input (or at once without input), becausespawnSyncdoes so and tests rely on it (verifyOutput.test.tschecks stdin EOF). Before, nothing could ever write to that pipe or close it.wb's production code keepsspawnSyncexceptbuildIfNeeded, because the releasedwbruns under Node (bin/index.js's#!/usr/bin/env node, whichbun wbhonors), wherespawnSynchas no such bug.buildIfNeededchanges because its tests run it in-process 25 times per suite run.shared-lib-node's synchronous APIs (readEnvironmentVariables,treeKill) keepspawnSync, because their callers depend on them being synchronousstartCommand.test.tsgets the 30 s timeout of the other CLI-spawning tests, because running the CLI from source took up to 2.7 s on CI against the 5 s defaultCustomer Summary
wb testno longer fails at random because a unit test in this repository waits on a child process that has already exited.docs/expected-repository-rules.mdnow tells every repository's unit tests to launch child processes asynchronously, for the same reason.spawnAsyncnow closes the child's stdin after the input, likespawnSync, so a child that reads stdin no longer waits forever.Technical Summary
wb,wbfy,shared-lib-node,shared-lib, and the roottest/helpersreplacespawnSync/execSync/execFileSync/Bun.spawnSyncwithspawnAsync(Bun.spawninshared-lib, which does not depend onshared-lib-node). Their helpers and callers become async. Command-availability checks useBun.whichinstead of running--version.packages/wb/src/commands/buildIfNeeded.ts: thegit rev-parse,git status, and build-command runs usespawnAsync.packages/shared-lib-node/src/spawn.ts: stdin is always ended.index.tsalso exports theSpawnAsyncOptionsandSpawnAsyncReturnstypes for the tests' helper signatures.docs/expected-repository-rules.md: the unit-test rule asks for asynchronous child processes (Bun.spawnorspawnofnode:child_process, which every repository has).verifyOutput.test.ts's fixturegit init,wbfy'spackageConfig.test.ts) launchgitthroughspawnAsynctoo.Check hardest: the converted tests that relied on
execSync/execFileSyncthrowing on a non-zero exit. They now assertstatusexplicitly (buildIfNeeded.test.ts'sgithelper,prismaScripts.test.ts,gitattributes.test.ts,workerTypesConsumption.test.ts).Testing
1f4740b0):wb: 900 calls (≈360 s) → 12 (0.26 s, all intreeKill,readEnvironmentVariables, andkillListeningProcessesByPort)shared-lib-node: 95 → 56 (readEnvironmentVariables,treeKill)shared-lib: 3 → 0wbfy: 182 (83 s) → 125 (23 s, all inwbfy's own code)workflow_dispatchTest runs on this branch passed. A stress workflow ran the wb unit suite 80 times on GitHub-hostedubuntu-24.04(20 jobs × 4) at this branch: no test hit a timeout orkilled 1 dangling process. The 2 failures were the separatetreeKillrace that fix(shared-lib-node): signal parents before their descendants in treeKill #1435 fixes.spawn.test.tschecks that stdin is closed with and without input. It times out without thespawn.tschange.bun run verifyand each package's lint/typecheck pass. Locally, the unit suites pass forwb(470),shared-lib-node(68), andshared-lib(284).wbfypasses 360 of 362: the 2globalReleaseAgeGate.test.tscases fail onmaintoo, because mise rejects the untrusted global config under the test'sHOME.Notes
wbfyruns under Bun (#!/usr/bin/env bun) and still makes the remaining 125 synchronous spawns, andgetGlobIgnore(#1431, merged after these measurements) adds synchronousgitcalls. #1433 tracks moving both to async spawns.--no-isolatestays (tracked by #1425): the synchronous APIs above and other repositories' tests still callspawnSync.