Conversation
bun install's find_commit runs `git log` via bun_spawn::run on the main
thread while git clones/checkouts are live on the install threadpool.
With BUN_FEATURE_FLAG_NO_ORPHANS set (every test on the x64-asan CI
lane), sync::spawn_posix on the main thread arms PR_SET_CHILD_SUBREAPER,
drains wait4(-1), and on return runs kill_subreaper_adoptees, which
SIGKILLs any direct child that was forked after the pre-spawn snapshot.
A threadpool git clone forked in that window dies with:
error: git failed with signal 9
error: "git clone" for "install-test1" failed
which surfaced as near-constant flakes of
test/cli/install/migration/complex-workspace.test.ts on 13 x64-asan.
Add sync::Options::no_orphans_reap (default true) and set it false in
bun_spawn::run: that helper is a capture-output utility for
repository::exec, not the bun run/bunx script path the subreaper
cleanup is meant for.
|
Updated 7:11 AM PT - Jul 11th, 2026
❌ @robobun, your commit 2458f69 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 33982That installs a local version of the PR into your bun-33982 --bun |
WalkthroughChangesOrphan Reaping Control
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/run/no-orphans.test.ts`:
- Around line 1002-1017: Update the marker assertion in the no-orphans test to
include the expected “clone-bare parent” entry alongside the existing
clone-no-checkout marker. Keep the fake Git behavior in the command-case script
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c08c365f-0b9c-423b-870b-414bb99bd49c
📒 Files selected for processing (3)
src/spawn/lib.rssrc/spawn/process.rstest/cli/run/no-orphans.test.ts
git+file:// matches neither try_https nor try_ssh, so Repository::download (and its git clone --bare) is never reached for this fixture; the first git invocation is find_commit's git log. Remove the dead case arm and document why so the marker expectation reads unambiguously.
|
CI status:
Ready for review. |
There was a problem hiding this comment.
Doc-comment feedback from my earlier pass was addressed in 2458f69. I didn't find further issues, but this touches the --no-orphans subreaper/SIGKILL machinery, so worth a human glance at whether a blanket opt-out for bun_spawn::run is the right layer.
Checked: the new no_orphans_reap field defaults to true and is AND-gated, so every other sync::Options construction site (run_command, bunx, create, upgrade, patch, publish, open, ChangedFilesFilter — all via ..Default::default()) keeps unchanged behavior.
Verified bun_spawn::run's sole caller is repository::exec (src/install/repository.rs:376), matching the "sole caller" claim in the comment.
The new test observes pgid deterministically rather than racing; the git+file:// fixture path skipping --bare was already litigated and resolved in-thread.
Extended reasoning...
Overview
Adds a no_orphans_reap: bool field to process::sync::Options (default true), AND-gates the existing no_orphans computation in spawn_posix on it, and sets it to false in bun_spawn::run. Adds a Linux-only test in test/cli/run/no-orphans.test.ts that puts a fake git on PATH and asserts bun install's internal git log invocation is not placed in its own process group under BUN_FEATURE_FLAG_NO_ORPHANS=1.
Security risks
None identified. The change strictly narrows when the subreaper/adoptee-SIGKILL path arms — it can only result in fewer SIGKILLs of child processes, not more. No new external input is parsed; the fake-git test fixture is hermetic (temp dir, PATH prepend, no network).
Level of scrutiny
Medium-high. The diff is small (~20 lines of production code) and structurally conservative — a new option that defaults to the pre-PR behavior, with exactly one caller opting out. However, the surrounding --no-orphans machinery (PR_SET_CHILD_SUBREAPER, wait4(-1) reap loop, kill_subreaper_adoptees) is subtle process-management infrastructure with cross-thread implications, and the fix encodes a design decision: "bun_spawn::run is a capture-output utility, never a user-script runner, so it should never arm the reap path." That's currently true (grep confirms repository::exec is the only caller), but a maintainer familiar with #30875's design should confirm the opt-out belongs on run() rather than, say, threading a flag down from repository::exec specifically or making the subreaper snapshot thread-aware.
Other factors
- My prior inline comment (doc-comment polarity read as inverted) was addressed in 2458f69; the field doc now correctly leads with "When
true(default)…". - CodeRabbit's
--bareassertion suggestion was correctly rebutted (thegit+file://fixture never reachesRepository::download) and the unreachable case arm was removed in b3fe04d. - CI on the previously-flaking
x64-asanlane passed all 20 shards per the author's build-71921 report. - Grep of all
sync::Options { … }construction sites confirms none would change behavior (Rust would fail to compile without..Default::default(), and the default istrue). - The only residual concern is architectural (right layer for the opt-out), not a correctness bug in the diff as written.
BUN_FEATURE_FLAG_NO_ORPHANS=1 (set on ASAN CI lanes) arms a subreaper around the main-thread git spawn inside bun install that SIGKILLs concurrent threadpool clone tasks (#33982); with 16 clones in flight this test hits that window reliably. Strip the flag from the spawned install's env since the test exercises install task bookkeeping, not orphan reaping.
BUN_FEATURE_FLAG_NO_ORPHANS=1 (set on ASAN CI lanes) arms a subreaper around the main-thread git spawn inside bun install that SIGKILLs concurrent threadpool clone tasks (#33982); with 16 clones in flight this test hits that window reliably. Strip the flag from the spawned install's env since the test exercises install task bookkeeping, not orphan reaping.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-11, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
test/cli/install/migration/complex-workspace.test.tshas been flaking near-constantly on the:debian: 13 x64-asanlane (dozens of builds since ~71375, one hard red in 71888) with:Cause
scripts/runner.node.mjssetsBUN_FEATURE_FLAG_NO_ORPHANS=1for every test on ASAN lanes (since #30875), whichbunEnvpropagates into thebun installthe test spawns.Inside that
bun install:Repository::find_commitrunsgit logviabun_spawn::run->process::sync::spawn_posixon the main thread (reached fromrunTasks.rs:1375andPackageManagerEnqueue.rs:1237).git clone/git checkouttasks run via the same helper on threadpool workers.sync::spawn_posixon the watchdog-arming (main) thread with no-orphans enabled:PR_SET_CHILD_SUBREAPER,wait4(-1, WNOHANG)reap loop while waiting on the child,kill_subreaper_adoptees(snapshot), which SIGKILLs every direct child not in the snapshot.That machinery is intended for
bun run/bunx, where the script is the only interesting subprocess. Whenfind_commitreaches it, any threadpoolgit cloneforked between steps 1 and 4 is SIGKILLed, and any that exits during step 3 has its status stolen. ASAN slows the threadpool worker's path from "task picked" to "fork" enough to hit this window regularly.The existing
is_arming_thread()guard only covers calls issued from worker threads; it does not cover a main-thread caller running alongside worker-thread children.Fix
Add
sync::Options::no_orphans_reap(defaulttrue) and gate the subreaper/pgroup/adoptee-kill path on it inspawn_posix.bun_spawn::runsets it tofalse: its sole caller isrepository::exec, which is a capture-output utility, not the user-script path.bun run/bunx/create/etc. keep the default.Verification
New test in
test/cli/run/no-orphans.test.tsobserves the mechanism deterministically rather than the race: it puts a fakegitonPATHthat records whether each invocation was placed in its own process group (new_process_group = no_orphansinspawn_posix). WithBUN_FEATURE_FLAG_NO_ORPHANS=1:find_commit'sgit logreportsown(subreaper path armed),parent, same as the threadpool steps.Introduced by #30875.
no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/migration/complex-workspace.test.ts test/cli/run/no-orphans.test.ts