no-orphans(windows): allow CREATE_BREAKAWAY_FROM_JOB and set DIE_ON_UNHANDLED_EXCEPTION on the Job - #36414
Conversation
…NHANDLED_EXCEPTION on the Job
The --no-orphans Job Object set only JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE.
Because enable() self-assigns Bun into the Job, that Job becomes the
immediate job for every descendant, which had two observable effects:
* A descendant that calls CreateProcess(CREATE_BREAKAWAY_FROM_JOB) got
ERROR_ACCESS_DENIED, because the immediate job did not permit
breakaway. That is a hard spawn failure --no-orphans should not cause.
* A descendant with no top-level exception filter that crashed could
reach UnhandledExceptionFilter's default WER path (dialog / JIT
debugger prompt), which on a headless box parks the process forever.
Add JOB_OBJECT_LIMIT_BREAKAWAY_OK and
JOB_OBJECT_LIMIT_DIE_ON_UNHANDLED_EXCEPTION to the Job's LimitFlags.
Bun's own crash reporter is unaffected: it is installed via
SetUnhandledExceptionFilter and runs before the Job flag takes effect.
SILENT_BREAKAWAY_OK is deliberately omitted because the --no-orphans Job
relies on inheritance to cover the whole tree (libuv's global job sets it
because libuv assigns each child explicitly, which is the opposite model).
Apply the same flags to the parallel test runner's kill-on-close Job in
Coordinator.rs, which has the same shape and the same failure modes.
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
WalkthroughChangesWindows Job Object configurations now include exception termination and process breakaway flags. A Windows-only Windows Job Object behavior
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/io/lib.rs (1)
106-148: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract the shared Windows Job Object flags into one constant.
src/io/lib.rs:131-133andsrc/runtime/cli/test/parallel/Coordinator.rs:738-740both spell the sameKILL_ON_JOB_CLOSE | DIE_ON_UNHANDLED_EXCEPTION | BREAKAWAY_OKmask. Hoist it into a sharedpub constand reference it from both sites to avoid silent drift if the policy changes.🤖 Prompt for 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. In `@src/io/lib.rs` around lines 106 - 148, Extract the shared Windows Job Object limit mask into a public constant, then replace the inline flag expression in src/io/lib.rs:106-148 and the matching expression in src/runtime/cli/test/parallel/Coordinator.rs:722-753 with that constant. Define it in an appropriate shared Windows-accessible module and preserve the existing KILL_ON_JOB_CLOSE, DIE_ON_UNHANDLED_EXCEPTION, and BREAKAWAY_OK policy.
🤖 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.
Outside diff comments:
In `@src/io/lib.rs`:
- Around line 106-148: Extract the shared Windows Job Object limit mask into a
public constant, then replace the inline flag expression in
src/io/lib.rs:106-148 and the matching expression in
src/runtime/cli/test/parallel/Coordinator.rs:722-753 with that constant. Define
it in an appropriate shared Windows-accessible module and preserve the existing
KILL_ON_JOB_CLOSE, DIE_ON_UNHANDLED_EXCEPTION, and BREAKAWAY_OK policy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c5e8e6f8-99b5-4a7c-a451-dfa9109a66bc
📒 Files selected for processing (3)
src/io/lib.rssrc/runtime/cli/test/parallel/Coordinator.rstest/cli/run/no-orphans.test.ts
…GS_KILL_TREE_ON_CLOSE; fix FFI HANDLE arg type in test fixture Declare the WaitForSingleObject/CloseHandle HANDLE parameters as FFIType.u64 (not FFIType.ptr) so the BigInt handle values read from PROCESS_INFORMATION round-trip correctly; matches the pattern in test/js/bun/windows/appcontainer.test.ts.
|
Addressed in 4d8f022:
On the find-issues bot suggestion: this PR does not fix #31603. That issue reproduces without CI on 44f75af: the only |
There was a problem hiding this comment.
No bugs found; the earlier FFI HANDLE-type nit was addressed in 4d8f022. Deferring to a maintainer only because this is a semantic policy change to --no-orphans on Windows — BREAKAWAY_OK lets descendants that explicitly pass CREATE_BREAKAWAY_FROM_JOB escape the kill-on-close Job, and DIE_ON_UNHANDLED_EXCEPTION changes WER/crash behavior for every descendant in the Job. The reasoning looks sound and matches standard practice (libuv, Chromium), but it's worth a human confirming the trade-off.
What was reviewed:
- Verified the flag bit values (0x2000/0x400/0x800) match winnt.h and the shared constant is used at both inheritance-based Job sites;
become_watcher_manager's Job correctly stays separate (it usesSILENT_BREAKAWAY_OK+ explicitPROC_THREAD_ATTRIBUTE_JOB_LISTassignment, different model). - Checked the test's FFI struct offsets/sizes (JOBOBJECT_EXTENDED_LIMIT_INFORMATION=144, LimitFlags@16, STARTUPINFOW=104, PROCESS_INFORMATION=24) against x64 layout.
- Confirmed the existing "cmd.exe-spawned descendant is reaped" tests still guard against
SILENT_BREAKAWAY_OKregressions.
Extended reasoning...
Overview
Adds two Win32 Job Object limit flags (JOB_OBJECT_LIMIT_BREAKAWAY_OK, JOB_OBJECT_LIMIT_DIE_ON_UNHANDLED_EXCEPTION) to the kill-on-close Jobs created by ParentDeathWatchdog::enable() (--no-orphans) and the parallel-test Coordinator. Hoisted into a shared pub const JOB_LIMIT_FLAGS_KILL_TREE_ON_CLOSE in src/sys/windows/mod.rs with a doc comment explaining why SILENT_BREAKAWAY_OK is deliberately omitted. A Windows-only test in test/cli/run/no-orphans.test.ts probes LimitFlags via QueryInformationJobObject(NULL) and behaviorally verifies CreateProcess(CREATE_BREAKAWAY_FROM_JOB) succeeds. Net src/ delta: one 3-flag constant + two one-line call-site swaps, all under #[cfg(windows)].
Security risks
None identified. The change loosens the Job's containment only for children that explicitly pass CREATE_BREAKAWAY_FROM_JOB — the same contract every other kill-on-close Job in the ecosystem offers. DIE_ON_UNHANDLED_EXCEPTION terminates crashed descendants instead of leaving them parked on a WER prompt; no new attack surface.
Level of scrutiny
Moderate. The code change is trivially correct (flag OR into an existing DWORD), but it alters the observable semantics of an opt-in runtime feature: (1) descendants can now escape --no-orphans by explicit breakaway, and (2) any descendant's unhandled exception now terminates rather than reaching WER. Both are defensible and well-argued in the PR body (fixes a hard ERROR_ACCESS_DENIED compat break; prevents headless-CI hangs), and the crash-reporter interaction is analyzed (Bun's own SetUnhandledExceptionFilter runs first, crash_handler() is -> !). Still, it's a user-facing policy trade-off on Windows that a maintainer should sign off on rather than a bot.
Other factors
- Sibling coverage: grepped for other
JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSEsites. The third site,become_watcher_manager(), already setsBREAKAWAY_OK | SILENT_BREAKAWAY_OK | DIE_ON_UNHANDLED_EXCEPTIONand assigns children viaPROC_THREAD_ATTRIBUTE_JOB_LIST, so the new inheritance-based constant intentionally does not apply there. The two changed sites are the complete set of inheritance-based Jobs. - Regression guard: the pre-existing "cmd.exe-spawned descendant" tests in the same file rely on Job-membership inheritance, so they would fail if someone later added
SILENT_BREAKAWAY_OK; the new test additionally asserts that bit is clear. - Prior feedback: my earlier nit (BigInt→
FFIType.ptrproducing garbage handles in the fixture's cleanup path) was fixed by switching toFFIType.u64; comment-cop feedback on doc length was addressed. All threads resolved. - Fail-before/pass-after evidence was captured on Windows Server 2019 (Linux CI can't observe the change since everything is
#[cfg(windows)]/skipIf(!isWindows)).
|
Updated 6:57 PM PT - Jul 29th, 2026
❌ @robobun, your commit 1a4325f has 1 failures in 🧪 To try this PR locally: bunx bun-pr 36414That installs a local version of the PR into your bun-36414 --bun |
There was a problem hiding this comment.
LGTM — the prior FFI HANDLE-type feedback is addressed (FFIType.u64), and the src/ change is now a single shared constant referenced from both call sites.
What was reviewed:
- Verified the Win32 flag values (0x2000/0x400/0x800/0x1000) and
CREATE_BREAKAWAY_FROM_JOB=0x01000000 against winnt.h. - Checked the FFI struct offsets in the probe fixture:
LimitFlagsat offset 16,sizeof(JOBOBJECT_EXTENDED_LIMIT_INFORMATION)=144,sizeof(STARTUPINFOW)=104,sizeof(PROCESS_INFORMATION)=24 on x64. - Confirmed
become_watcher_manager()was correctly not switched to the new constant (it uses explicit assignment viaPROC_THREAD_ATTRIBUTE_JOB_LIST, soSILENT_BREAKAWAY_OKis right there). - Confirmed omitting
SILENT_BREAKAWAY_OKhere keeps the existing "cmd.exe-spawned descendant" tests valid.
Extended reasoning...
Overview
Windows-only behavioral tweak to the Job Object LimitFlags used by --no-orphans (src/io/lib.rs) and the parallel-test coordinator (src/runtime/cli/test/parallel/Coordinator.rs). Previously both set only KILL_ON_JOB_CLOSE; now they OR in DIE_ON_UNHANDLED_EXCEPTION | BREAKAWAY_OK via a new shared pub const JOB_LIMIT_FLAGS_KILL_TREE_ON_CLOSE in src/sys/windows/mod.rs. A new Windows-only test in test/cli/run/no-orphans.test.ts probes the immediate Job's flags via QueryInformationJobObject(NULL) and behaviorally verifies CreateProcess(CREATE_BREAKAWAY_FROM_JOB) succeeds. Net src/ delta: +8/-2, all under #[cfg(windows)].
Security risks
None. This is Win32 Job Object flag configuration for process-tree cleanup. BREAKAWAY_OK slightly weakens the --no-orphans guarantee (a descendant that explicitly passes CREATE_BREAKAWAY_FROM_JOB escapes), but that's the documented Win32 semantics for explicit opt-out, and the alternative is a hard ERROR_ACCESS_DENIED spawn failure that --no-orphans should not cause. SILENT_BREAKAWAY_OK — which would defeat the feature — is deliberately excluded, documented on the constant, and asserted-false in the test.
Level of scrutiny
Low-medium. The src/ change is a 3-flag bitmask constant with two one-line call-site swaps. The PR description gives a clear mechanistic account of both problems (breakaway spawn failure with err=5; WER parking a crashed descendant on headless boxes) with before/after evidence on Windows Server 2019, and explains why DIE_ON_UNHANDLED_EXCEPTION doesn't interfere with Bun's own crash reporter (SetUnhandledExceptionFilter runs first, crash_handler() is -> !). The reasoning for excluding SILENT_BREAKAWAY_OK (inheritance-based membership vs. libuv's explicit-assignment model) is consistent with the existing become_watcher_manager() job, which does use explicit assignment and was correctly left untouched.
Other factors
- I previously flagged that the test fixture passed BigInt handles through
FFIType.ptr; that was fixed in 4d8f022 (nowFFIType.u64, matchingappcontainer.test.ts). All review threads are resolved. - The comment-cop bot flagged the constant's doc comment; the author's response (durable rationale on a
pub const, not a workaround justification) is reasonable per REVIEW.md's "comments carry only durable non-obvious content" — omittingSILENT_BREAKAWAY_OKvs. libuv is exactly the kind of deliberate deviation worth recording. - The test asserts both the flag bits (structured object diff for readable failure) and the behavioral
CreateProcessresult, plusSILENT_BREAKAWAY_OK: falseto guard against a future regression that would silently break the existing inheritance tests. - Fail-before/pass-after evidence was captured on Windows; the change cannot be observed on Linux CI, so Windows CI is the gate.
|
CI summary across builds 85519 / 85526 / 85546: the new Windows test passed on every Windows lane in every run. Remaining red is unrelated to this diff (every src/ line here is under
Ready for review. |
* upstream/main: (422 commits) install: drop packages held only by optional-peer resolution slots from bun.lock (oven-sh#35681) Update mimalloc to the upstream dev3 (v3.4.3) sync (oven-sh#36431) compile(pe): ftruncate the Windows --compile output after writing (oven-sh#36430) Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC (oven-sh#35849) test(harness): replace toRun matcher with async bunRun + toSpawn (oven-sh#36424) test: measure memory via harness rss() instead of process.memoryUsage.rss() (oven-sh#36429) Deflake a few tests no-orphans(windows): allow CREATE_BREAKAWAY_FROM_JOB and set DIE_ON_UNHANDLED_EXCEPTION on the Job (oven-sh#36414) GarbageCollectionController: replace per-tick heap sampler with idle timer only (oven-sh#35356) exe_format(pe): write a valid OptionalHeader.CheckSum for --compile output (oven-sh#36383) FileSink: flush buffered bytes when process.exit() runs in the same tick as write() (oven-sh#36250) test(http): speed up and de-flake serve-async-stream-client-abort.test.ts (oven-sh#35919) test(20144): stop racing child startup against the 1s SIGKILL guard (oven-sh#34166) test(no-orphans): skip fast-exit perl daemon test on macOS (oven-sh#36413) fs: return negative BigIntStats *Ns for pre-epoch timestamps (oven-sh#36187) event_loop: make DeferredTaskQueue::run tolerate re-entrant map mutation (oven-sh#32703) dotenv: stop panicking on nested `${...}` inside `${VAR:-default}` (oven-sh#36199) fetch: make the idle timer an absolute deadline for the response header block (oven-sh#36145) bundler: don't panic on unterminated naming template placeholders (oven-sh#36325) Buffer#indexOf/lastIndexOf: rare-byte SIMD filter with a Two-Way O(n+m) fallback (oven-sh#36420) ... # Conflicts: # src/jsc/bindings/BunDebugger.cpp
* upstream/main: (422 commits) install: drop packages held only by optional-peer resolution slots from bun.lock (oven-sh#35681) Update mimalloc to the upstream dev3 (v3.4.3) sync (oven-sh#36431) compile(pe): ftruncate the Windows --compile output after writing (oven-sh#36430) Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC (oven-sh#35849) test(harness): replace toRun matcher with async bunRun + toSpawn (oven-sh#36424) test: measure memory via harness rss() instead of process.memoryUsage.rss() (oven-sh#36429) Deflake a few tests no-orphans(windows): allow CREATE_BREAKAWAY_FROM_JOB and set DIE_ON_UNHANDLED_EXCEPTION on the Job (oven-sh#36414) GarbageCollectionController: replace per-tick heap sampler with idle timer only (oven-sh#35356) exe_format(pe): write a valid OptionalHeader.CheckSum for --compile output (oven-sh#36383) FileSink: flush buffered bytes when process.exit() runs in the same tick as write() (oven-sh#36250) test(http): speed up and de-flake serve-async-stream-client-abort.test.ts (oven-sh#35919) test(20144): stop racing child startup against the 1s SIGKILL guard (oven-sh#34166) test(no-orphans): skip fast-exit perl daemon test on macOS (oven-sh#36413) fs: return negative BigIntStats *Ns for pre-epoch timestamps (oven-sh#36187) event_loop: make DeferredTaskQueue::run tolerate re-entrant map mutation (oven-sh#32703) dotenv: stop panicking on nested `${...}` inside `${VAR:-default}` (oven-sh#36199) fetch: make the idle timer an absolute deadline for the response header block (oven-sh#36145) bundler: don't panic on unterminated naming template placeholders (oven-sh#36325) Buffer#indexOf/lastIndexOf: rare-byte SIMD filter with a Two-Way O(n+m) fallback (oven-sh#36420) ...
* upstream/main: (422 commits) install: drop packages held only by optional-peer resolution slots from bun.lock (oven-sh#35681) Update mimalloc to the upstream dev3 (v3.4.3) sync (oven-sh#36431) compile(pe): ftruncate the Windows --compile output after writing (oven-sh#36430) Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC (oven-sh#35849) test(harness): replace toRun matcher with async bunRun + toSpawn (oven-sh#36424) test: measure memory via harness rss() instead of process.memoryUsage.rss() (oven-sh#36429) Deflake a few tests no-orphans(windows): allow CREATE_BREAKAWAY_FROM_JOB and set DIE_ON_UNHANDLED_EXCEPTION on the Job (oven-sh#36414) GarbageCollectionController: replace per-tick heap sampler with idle timer only (oven-sh#35356) exe_format(pe): write a valid OptionalHeader.CheckSum for --compile output (oven-sh#36383) FileSink: flush buffered bytes when process.exit() runs in the same tick as write() (oven-sh#36250) test(http): speed up and de-flake serve-async-stream-client-abort.test.ts (oven-sh#35919) test(20144): stop racing child startup against the 1s SIGKILL guard (oven-sh#34166) test(no-orphans): skip fast-exit perl daemon test on macOS (oven-sh#36413) fs: return negative BigIntStats *Ns for pre-epoch timestamps (oven-sh#36187) event_loop: make DeferredTaskQueue::run tolerate re-entrant map mutation (oven-sh#32703) dotenv: stop panicking on nested `${...}` inside `${VAR:-default}` (oven-sh#36199) fetch: make the idle timer an absolute deadline for the response header block (oven-sh#36145) bundler: don't panic on unterminated naming template placeholders (oven-sh#36325) Buffer#indexOf/lastIndexOf: rare-byte SIMD filter with a Two-Way O(n+m) fallback (oven-sh#36420) ... # Conflicts: # src/js/internal/debugger.ts
* upstream/main: (422 commits) install: drop packages held only by optional-peer resolution slots from bun.lock (oven-sh#35681) Update mimalloc to the upstream dev3 (v3.4.3) sync (oven-sh#36431) compile(pe): ftruncate the Windows --compile output after writing (oven-sh#36430) Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC (oven-sh#35849) test(harness): replace toRun matcher with async bunRun + toSpawn (oven-sh#36424) test: measure memory via harness rss() instead of process.memoryUsage.rss() (oven-sh#36429) Deflake a few tests no-orphans(windows): allow CREATE_BREAKAWAY_FROM_JOB and set DIE_ON_UNHANDLED_EXCEPTION on the Job (oven-sh#36414) GarbageCollectionController: replace per-tick heap sampler with idle timer only (oven-sh#35356) exe_format(pe): write a valid OptionalHeader.CheckSum for --compile output (oven-sh#36383) FileSink: flush buffered bytes when process.exit() runs in the same tick as write() (oven-sh#36250) test(http): speed up and de-flake serve-async-stream-client-abort.test.ts (oven-sh#35919) test(20144): stop racing child startup against the 1s SIGKILL guard (oven-sh#34166) test(no-orphans): skip fast-exit perl daemon test on macOS (oven-sh#36413) fs: return negative BigIntStats *Ns for pre-epoch timestamps (oven-sh#36187) event_loop: make DeferredTaskQueue::run tolerate re-entrant map mutation (oven-sh#32703) dotenv: stop panicking on nested `${...}` inside `${VAR:-default}` (oven-sh#36199) fetch: make the idle timer an absolute deadline for the response header block (oven-sh#36145) bundler: don't panic on unterminated naming template placeholders (oven-sh#36325) Buffer#indexOf/lastIndexOf: rare-byte SIMD filter with a Two-Way O(n+m) fallback (oven-sh#36420) ...
What
ParentDeathWatchdog::enable()on Windows creates a Job Object for--no-orphansand self-assigns Bun into it, so it becomes the immediate job for every descendant. It set onlyJOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE.Why it matters
Two observable problems:
CreateProcess(CREATE_BREAKAWAY_FROM_JOB)fails. Any descendant that passes that flag getsERROR_ACCESS_DENIEDbecause the immediate job does not permit breakaway. On a Windows 2019 box against current main:That is a hard spawn failure
--no-orphansshould not cause. libuv's own comment on its global job notes the same hazard.WER can park a crashed descendant. A non-Bun child that crashes with no top-level filter reaches
UnhandledExceptionFilter's default path (WER dialog / JIT debugger prompt), which on a headless box blocks forever.JOB_OBJECT_LIMIT_DIE_ON_UNHANDLED_EXCEPTIONmakes it exit with the NTSTATUS instead.Fix
Set
LimitFlags = KILL_ON_JOB_CLOSE | DIE_ON_UNHANDLED_EXCEPTION | BREAKAWAY_OK.Why this is safe for Bun itself: Bun's crash reporter is installed via
SetUnhandledExceptionFilter(handle_unhandled_exception_windowsinsrc/crash_handler/lib.rs). That callback runs before the Job flag applies;crash_handler()is-> !. The flag only changes the behavior for exception codes Bun's classifier declines and for non-Bun descendants, both of which should terminate rather than prompt under--no-orphans.Why not
SILENT_BREAKAWAY_OK: libuv's global job sets it because libuv explicitly assigns each child, so grandchildren intentionally escape. The--no-orphansJob relies on inheritance to cover the whole descendant tree;SILENT_BREAKAWAY_OKwould make every child escape and break the feature. The existing "cmd.exe-spawned descendant" tests already rely on this.Applied the same flag set to the parallel test runner's kill-on-close Job in
Coordinator.rs(same shape, same failure modes).Verification
New test in
test/cli/run/no-orphans.test.tsprobes the immediate Job'sLimitFlagsviaQueryInformationJobObject(NULL)and exercisesCreateProcess(CREATE_BREAKAWAY_FROM_JOB)via FFI.fail-before (canary on Windows Server 2019)
pass-after (`bun bd test` on Windows Server 2019)
The src/ change is entirely
#[cfg(windows)]and the test isskipIf(!isWindows), so a Linux-only gate cannot observe fail-before; the evidence above was captured on windows-x64.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/run/no-orphans.test.ts