Repository navigation
fix(wb): run parallel unit test workers without per-file isolation - #1421
Conversation
In Bun 1.4.2, the stdout/stderr writers of a global that bun test --parallel discards after each file can be garbage-collected inside a later spawnSync, after which spawnSync in that worker misses its child's exit and hangs until the test times out. Running each worker's files in one global removes that trigger while keeping the worker processes. 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 intermittent CI test failures caused by hangs in Bun's parallel test runner. By enabling the --no-isolate flag, the changes prevent the corruption of the event loop during spawnSync calls, ensuring stability while maintaining the performance benefits of parallel test execution. The documentation has been updated to reflect the requirement for tests to clean up global state due to the shared environment within worker processes. 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. Tests running fast in parallel rows, / But zombie processes cause us woes. / With no-isolate set, the hang is gone, / Now our CI can carry on. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request updates the unit test execution command in wb to use 'bun test --parallel --no-isolate' instead of just '--parallel'. This change is introduced to work around a bug in Bun 1.4.2 (issue #34069) where the garbage collection of stdout/stderr writers in isolated workers can cause spawnSync to hang. Correspondingly, the documentation in docs/expected-repository-rules.md and helper comments in packages/wb/test/helpers/shared.ts have been updated to reflect that test files running in the same worker process will now share a single global state and module registry, requiring them to restore any modified globals or imported modules. There are no review comments to address.
… leave Co-authored-by: WillBooster (Claude Code) <agent@willbooster.com>
… files Workers now run their files in one global, so a file's process.env changes reach the files after it. privateRegistry.test.ts deleted a VERDACCIO_TOKEN that CI defines, which changed the environment buildIfNeeded hashes, so a later buildWb rebuilt dist/ while another worker ran the CLI. Restore the environment in the tests that change it, and build dist/ only once per run. Co-authored-by: WillBooster (Claude Code) <agent@willbooster.com>
…onment Co-authored-by: WillBooster (Claude Code) <agent@willbooster.com>
Why
Since
wbswitched unit tests tobun test --parallel(#1347), CI has failed intermittently on tests that normally take well under a second. They run into their full timeout withkilled 1 dangling process:deployRailway.test.ts(30 s),project.test.ts'smise envcase (5 s), wbfy'spackageJson.test.ts(60 s),buildWb'sbeforeAll(120 s), and others. PR #1420 hit it in 4 of about 10 runs. After #1420, 3 of 7workflow_dispatchruns of the Test workflow onmainfailed the same way.These are hangs, not slowness under CPU load. In the
project.test.tsfailures,mise envhad already exited with status 0 and its full output was parsed, yet thespawnSyncreturned only when Bun killed the "dangling" child at the test timeout. This matches a Bun 1.4.2 bug (oven-sh/bun#34069). Its fix, oven-sh/bun#40078, is unmerged, and no newer stable Bun exists. With--isolate, which--parallelimplies, the stdout/stderr writers of each discarded per-file global can be garbage-collected inside a laterspawnSync. That corruptsspawnSync's private event loop, so it and laterspawnSynccalls in the same worker spin without noticing the child's exit, and the child stays a zombie. The newreleasePlugin.test.tsdoes not cause the hang. Its manyspawnSynccalls only make the hang more likely to show up.--no-isolatekeeps the worker processes but runs each worker's files in one global, like a plainbun testrun, so no per-file global is discarded. The other options were worse:spawnSyncreachable from tests (including production code inwbandwbfy) to async spawns is a large change that stays exposed to the same bug.--parallelslows every repository's CI.Requirements
deployRailway.test.ts) that PR feat(wb): add the draft-first release plugin and complete pending releases in wb release #1420's CI runs hit, with evidence (CI logs, timings on main, local reproduction under load)wb testkeeps running unit test files in parallel worker processes (the contract indocs/expected-repository-rules.md)--no-isolateinstead of rewritingspawnSynccallers, because it removes the trigger with a one-flag change and keeps parallel speed (a forcedBun.gc(true)preload per isolated file was measured and does not help: 10/10 wedged)process.envthey change, andbuildWbbuildsdist/only once per parallel run, because files in a worker now shareprocess.envand a leaked change made a laterbuildWbrebuilddist/under another worker's CLI runCustomer Summary
wb teststill runs unit test files in parallel worker processes, but the files that one worker runs now share a global, a module registry, andprocess.env. Tests that callspawnSync(directly or throughwb/wbfycode) no longer hang until their timeout at random. A test file must now restore the global state, module spies, and environment variables it changes.Technical Summary
packages/wb/src/scripts/execution/baseScripts.ts:testUnitpasses--parallel --no-isolate. E2E commands are unchanged (they never used--parallel).docs/expected-repository-rules.md: the unit-test rule now says that files in a worker share a global and module registry. A file must therefore not depend on state another file leaves behind, and must restore what it changes (spies, globals, environment variables). The suites here already restore their spies, and this matches the contract before feat(wb): run bun unit tests in parallel worker processes #1347, whenbun testran every file in one process.process.env. In this PR's first CI runs,privateRegistry.test.tsdeleted theVERDACCIO_TOKENthat CI defines (empty), which changed the environmentbuildIfNeededhashes. A laterbuildWbthen rebuiltdist/while another worker ran the CLI (Cannot find module .../wb/dist/index.js). Fixes:packages/wb/test/unit/privateRegistry.test.tsrestores the originalVERDACCIO_TOKEN.packages/wb/test/helpers/shared.ts:createTempDirrestoresprocess.envas the file found it, undoinginitializeProjectDirectory's removal of npm/Yarn variables.packages/wb/test/helpers/build.ts: after the first successful build of a parallel run,buildWbskips the build (a marker next to the run's lock), so a future leak can no longer deletedist/mid-run.Check hardest: whether any repository's unit tests rely on a fresh global or environment per file. Such a test would now fail depending on file order, which the rule change above forbids.
Testing
killed 1 dangling processon a normally fast test), apart from network-dependent cases. 3 of 7workflow_dispatchruns onmainfailed with it (deployRailway.test.ts30 s timeouts, and a 120 sbuildWbhang).workflow_dispatch) with no hang. The only 2 failures were thedist/rebuild race described above, and the 9 runs since its fix all passed.execFileSync150 times, run withbun test --parallel=4and a 60 s cap. With--isolate, 7/10 and 9/10 runs wedged (the worker in state R, itsshchild a zombie). With--no-isolate, 0/10 wedged. In that aarch64 container, the real wb suite did not wedge in either mode across 8 alternating runs, which is why the CI runs above are the evidence for the actual suite.bun run verifypasses. The changed test files and the CLI-using files (privateRegistry,project,setup,typecheck,buildIfNeeded,binDotenv,localServerUrl) pass withbun wb test.--no-isolate.Notes
Remove
--no-isolateonce a stable Bun release contains the fix for oven-sh/bun#34069.