ai slop - #29322
Conversation
…r thread exits The Zig WebWorker struct was destroyed on the worker thread inside exitAndDeinit(), but the owning C++ Worker kept a raw pointer to it in impl_. A subsequent Worker.terminate() from JS on the parent thread would call WebWorker__notifyNeedTermination(impl_) and read the freed struct (status, requested_terminate, vm, parent_poll_ref), tripping ASAN with use-after-poison. Tie the struct's lifetime to the C++ Worker instead: deinit() now only releases owned resources (specifier, preloads, parent poll ref) and the allocation itself is freed from ~Worker() via a new WebWorker__destroy export. The worker thread now runs deinit() before WebWorker__dispatchExit drops the Zig-held ref on the C++ Worker so the struct is never touched after it can be freed. Worker::terminate() also short-circuits once any termination flag is set, avoiding a redundant cross-thread poke.
|
Updated 11:14 PM PT - Apr 14th, 2026
❌ @autofix-ci[bot], your commit c934eb3 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 29322That installs a local version of the PR into your bun-29322 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdded explicit Zig/C destroy for WebWorker and adjusted teardown ordering; ensured C++ Worker destructor calls the Zig destroy when present; made Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/web_worker.zig`:
- Around line 405-422: Add idempotent guards in WebWorker.deinit to avoid
double-free: check unresolved_specifier.len > 0 before calling
bun.default_allocator.free(this.unresolved_specifier) and set
this.unresolved_specifier = "" after freeing; similarly iterate preloads only if
this.preloads.len > 0, free each entry and free this.preloads only when len > 0,
then reset this.preloads = &.{}; keep WebWorker__destroy unchanged. Ensure
references are to deinit, unresolved_specifier, preloads, and WebWorker__destroy
so future calls from exitAndDeinit remain safe.
In `@test/js/web/workers/worker-terminate-after-exit.test.ts`:
- Around line 38-43: Replace the current indirect stderr assertion with an
explicit failure message when a run fails: inside the for loop over runs, check
if exitCode !== 0 and if so throw or call fail with a clear message like `Worker
failed: exitCode ${exitCode}, stderr: ${stderr}` (referencing the variables
stderr, exitCode and the runs iterator), then assert expect(exitCode).toBe(0) as
the normal success check; this will surface stderr directly on failure instead
of producing the confusing empty-string assertion.
🪄 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: 6e0078b3-1c64-4742-b534-6715c846e7d8
📒 Files selected for processing (3)
src/bun.js/bindings/webcore/Worker.cppsrc/bun.js/web_worker.zigtest/js/web/workers/worker-terminate-after-exit.test.ts
|
Not linking #23194 — that crash is in |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR fixes a real use-after-free in the worker lifecycle and the approach is sound, but the change is non-trivial (cross-thread C++/Zig lifetime management) and merits human review — particularly the interaction between the new WebWorker__destroy destructor path and the pre-existing name field leak that deinit() still doesn't address.
Extended reasoning...
Overview
The PR restructures WebWorker struct lifetime to fix a use-after-free: previously exitAndDeinit() destroyed the struct on the worker thread while the C++ Worker kept a raw impl_ pointer reachable from terminate() on the parent thread. The fix ties struct deallocation to ~Worker() (ref-counted, parent-thread lifetime) via a new WebWorker__destroy export, and makes deinit() idempotent so it can safely run before WebWorker__dispatchExit drops the Zig-held C++ ref.
Security risks
The bug being fixed is a memory safety issue (use-after-free / use-after-poison). The fix is strictly additive in terms of security posture: it eliminates a UAF, adds no new attack surface, and the new WebWorker__destroy export is only called from ~Worker() which has correct lifetime semantics relative to JS-reachable terminate() calls.
Level of scrutiny
This warrants close human review. The change is in cross-thread, cross-language (C++/Zig) code managing raw pointer lifetimes. Key questions a reviewer should satisfy themselves on: (1) Is the ~Worker() destructor guaranteed to run after all possible terminate() calls? (2) Is there any path where impl_ is used after WebWorker__destroy is called? (3) The Worker::terminate() short-circuit now returns early if any flag is set — does this correctly handle the case where TerminatedFlag is set without TerminateRequestedFlag (normal event-loop-death exit)?
Other factors
Two bugs were flagged by the automated review: a pre-existing name field leak that deinit() omits despite now being the canonical resource-release function, and an inverted test assertion order that produces misleading failure output on crash (though no false passes). Neither was introduced by this PR, but the first is worth addressing here since deinit() is being restructured. The test coverage and 80-run ASAN verification are solid signals, but the complexity of the lifetime change means a human familiar with the Worker/Zig FFI boundary should sign off.
| if (exitCode !== 0) { | ||
| expect(stderr).toBe(""); | ||
| } |
There was a problem hiding this comment.
🟡 The assertion at lines 39-41 is semantically inverted: when the process exits with a non-zero code (e.g., ASAN abort with exit code 134), stderr holds the crash trace, so expect(stderr).toBe("") fires first with the misleading message "Expected empty stderr" rather than surfacing the actual failure (non-zero exit code). The test still catches the use-after-free (no false negatives), but failure diagnostics are backwards. The fix is expect(exitCode, stderr: ${stderr}).toBe(0), which makes the exit code the assertion and the crash output the context message.
Extended reasoning...
What the bug is and how it manifests
In the new test file worker-terminate-after-exit.test.ts, lines 39-41 read:
if (exitCode \!== 0) {
expect(stderr).toBe("");
}
expect(exitCode).toBe(0);When the worker process crashes (e.g., due to ASAN detecting a use-after-poison with exit code 134), stderr contains the full ASAN trace. Because expect(stderr).toBe("") appears first inside the if (exitCode \!== 0) block, it fires before expect(exitCode).toBe(0) is ever reached, producing a misleading failure message: 'Expected "" but received "[ASAN trace]"'.
The specific code path that triggers it
A developer running the test against an unpatched ASAN build sees the test runner report a failure about unexpected stderr content rather than a clear 'process crashed with exit code 134' message. The intent appears to have been to surface stderr as diagnostic context when the process fails — but it is expressed as an assertion rather than a message parameter.
Why existing code doesn't prevent it
The test framework evaluates assertions eagerly and stops at the first failure. Since the stderr assertion is placed before the exit-code assertion inside the conditional block, it wins the race to produce the failure output whenever exitCode is non-zero.
Impact
No false passes are produced — every crash scenario is caught. However, a developer debugging a failure sees 'stderr should be empty' and may spend time investigating spurious output rather than immediately identifying the root cause as a use-after-free crash.
How to fix it
Replace the inverted assertion block with:
expect(exitCode, `stderr: ${stderr}`).toBe(0);This makes the exit code the subject of the assertion (so the failure message clearly states 'Expected 0, received 134') and attaches the ASAN trace as context, which is exactly what a developer needs when diagnosing a crash.
Step-by-step proof
- Run test against unpatched ASAN build; worker crashes: exitCode = 134, stderr = '==ERROR: AddressSanitizer...'
if (exitCode \!== 0)is true, soexpect(stderr).toBe("")executes- Assertion fails: 'Expected "" but received "==ERROR: AddressSanitizer..."'
- Test runner stops;
expect(exitCode).toBe(0)is never reached - Developer reads: 'Expected empty stderr' — misleading; the real issue is the crash, not the output
| fn deinit(this: *WebWorker) void { | ||
| log("[{d}] deinit", .{this.execution_context_id}); | ||
| this.parent_poll_ref.unrefConcurrently(this.parent); | ||
| bun.default_allocator.free(this.unresolved_specifier); | ||
| this.unresolved_specifier = ""; | ||
| for (this.preloads) |preload| { | ||
| bun.default_allocator.free(preload); | ||
| } | ||
| bun.default_allocator.free(this.preloads); | ||
| this.preloads = &.{}; | ||
| } |
There was a problem hiding this comment.
🟣 The name field is heap-allocated in create() when a non-empty name is provided, but deinit() never frees it, leaking the string for every named worker. This is a pre-existing issue, but the PR explicitly restructures deinit() as the canonical resource-release function (adding idempotency guards for unresolved_specifier and preloads), making the omission of name a conspicuous gap worth fixing at this point.
Extended reasoning...
What the bug is: In WebWorker.create(), the name field is conditionally heap-allocated using std.fmt.allocPrintSentinel(bun.default_allocator, ...) when name_str is non-empty, producing a [:0]const u8 slice owned by the default allocator. However, deinit() — now the canonical resource-release function per this PR — never calls bun.default_allocator.free(this.name). Similarly, WebWorker__destroy only calls bun.default_allocator.destroy(this), which frees the struct allocation itself but leaves the bytes pointed to by the name field unreleased.
The specific code path: In create() (web_worker.zig ~line 246):
.name = brk: {
if (\!name_str.isEmpty()) {
break :brk bun.handleOom(std.fmt.allocPrintSentinel(bun.default_allocator, "{f}", .{name_str}, 0));
}
break :brk "";
},The non-empty branch produces a heap allocation. In deinit() (lines 408–418), the function frees unresolved_specifier and each preloads entry (with idempotency zero-guards), but has no corresponding bun.default_allocator.free(this.name).
Why existing code does not prevent it: WebWorker__destroy is the sole remaining deallocation site (called from ~Worker()) and only issues a struct-level destroy, not a field-level free of the name slice. There is no other caller of bun.default_allocator.free on this.name anywhere in web_worker.zig.
Impact: Every new Worker(url, { name: "something" }) call leaks the name string allocation for the lifetime of the process. In long-running servers or test suites that spawn many named workers, this accumulates unboundedly.
How to fix it: Add a free in deinit() guarded to avoid freeing the static "" literal used for unnamed workers. Since only non-empty name_str produces a heap allocation, checking this.name.len > 0 is sufficient:
if (this.name.len > 0) {
bun.default_allocator.free(this.name);
this.name = "";
}A more robust alternative is a dedicated name_is_owned: bool field.
Step-by-step proof:
new Worker("./worker.js", { name: "my-worker" })callsWorker::create→WebWorker__create→WebWorker.create().name_stris non-empty, sostd.fmt.allocPrintSentinel(bun.default_allocator, ...)allocates N+1 bytes on the heap.this.namenow points to that allocation.- The worker thread eventually calls
exitAndDeinit()→this.deinit().deinit()freesunresolved_specifierandpreloads, but returns without touchingthis.name. - Later,
~Worker()callsWebWorker__destroy(impl_)→bun.default_allocator.destroy(this). This frees theWebWorkerstruct bytes but the name string bytes remain allocated and unreachable — a confirmed leak.
|
This PR has been closed because it was flagged as AI slop. Many AI-generated PRs are fine, but this one was identified as having one or more of the following issues:
If you believe this was done in error, please leave a comment explaining why. |
github-actions
Bot
changed the title
| } | ||
|
|
||
| export fn WebWorker__destroy(this: *WebWorker) void { | ||
| bun.default_allocator.destroy(this); | ||
| } |
There was a problem hiding this comment.
🔴 The thread-spawn failure path in WebWorker__updatePtr now leaks the WebWorker struct permanently. Before this PR, deinit() ended with bun.default_allocator.destroy(this), so the struct was freed in the catch block when std.Thread.spawn failed; after this PR, destroy was moved exclusively to WebWorker__destroy, which is only reachable via ~Worker(), but ~Worker() requires the C++ ref count to reach 0 — and the Zig-held ref (bumped by worker->ref() in Worker::create()) is only ever released inside WebWorker__dispatchExit, which is unreachable when the thread never starts. The fix is to call bun.default_allocator.destroy(worker) directly in the catch block after worker.deinit(), or have Worker::updatePtr() call worker->deref() when WebWorker__updatePtr returns false.
Extended reasoning...
What the bug is and how it manifests
In WebWorker__updatePtr (web_worker.zig lines 68–78), when std.Thread.spawn fails, the catch block calls worker.deinit() and returns false. Before this PR, deinit() ended with bun.default_allocator.destroy(this), so the WebWorker struct was freed immediately on the spawn-failure path. This PR moved the struct-free into the new exported function WebWorker__destroy (lines 420–422), which is called only from ~Worker() in C++.
The specific code path that triggers it
~Worker() is only invoked when the C++ Worker ref count reaches zero. In Worker::create(), after WebWorker__create succeeds, the code calls worker->ref() with the comment "now referenced by Zig". This Zig-held ref is only ever released inside WebWorker__dispatchExit via worker->deref(). WebWorker__dispatchExit is only called from exitAndDeinit(), which runs at the end of the worker thread's lifetime. If std.Thread.spawn fails, the worker thread never starts, exitAndDeinit() is never called, WebWorker__dispatchExit is never called, and the Zig ref stays permanently at 1.
Why existing code doesn't prevent it
Worker::updatePtr() in C++ sets TerminatedFlag when WebWorker__updatePtr returns false, but does NOT call worker->deref(). Without that deref, the C++ Worker ref count never drops to zero (even after the JS Worker object is GC'd and drops its own ref), so ~Worker() never runs and WebWorker__destroy is never called.
What the impact is
Any OOM condition or OS thread-limit hit during new Worker(url) construction causes both the C++ Worker and the Zig WebWorker struct to leak permanently. This is strictly worse than before the PR: previously the C++ Worker leaked (pre-existing) but the Zig struct was freed; now both leak.
How to fix it
Option A (minimal, restores old behavior for this path): in the WebWorker__updatePtr catch block, after worker.deinit(), add bun.default_allocator.destroy(worker).
Option B (cleaner): in Worker::updatePtr() (Worker.cpp line 141–148), after WebWorker__updatePtr returns false, call worker->deref() (and optionally null impl_ to prevent the double-destroy in ~Worker()).
Step-by-step proof
Worker::create()succeeds; callsworker->ref()— Zig ref = 1, JS ref = 1.Worker::updatePtr()callsWebWorker__updatePtr(impl_, this).std.Thread.spawnfails (OOM or thread limit).- Catch block:
worker.deinit()freesunresolved_specifierandpreloads, unrefs the parent poll. Does NOT callbun.default_allocator.destroy(this). WebWorker__updatePtrreturnsfalse;Worker::updatePtr()setsTerminatedFlagand returnsfalse. Noworker->deref()call.- JS throws an exception; JS
Workerobject eventually GC'd, dropping the JS ref. Zig ref (=1) remains. ~Worker()requires ref count == 0. Since Zig ref = 1,~Worker()never runs.WebWorker__destroynever called. TheWebWorkerstruct leaks indefinitely.
| // collects the wrapper, `~Worker()` will free this struct, so it must not be touched | ||
| // afterwards. | ||
| this.deinit(); | ||
|
|
||
| WebWorker__dispatchExit(globalObject, cpp_worker, exit_code); |
There was a problem hiding this comment.
🔴 After this PR's reordering, deinit() runs before WebWorker__dispatchExit, so parent_poll_ref.unrefConcurrently() fires before the close event reaches the parent thread; if a close event handler then calls worker.ref(), setRef() passes its only guard (hasRequestedTerminate(), which is never set on the natural-exit path) and re-activates parent_poll_ref, a ref that is never balanced — WebWorker__destroy only frees the struct — causing the parent process to hang indefinitely. Fix: add a this.status.load(.acquire) == .terminated check in setRef(), mirroring the guard already present in notifyNeedTermination().
Extended reasoning...
What the bug is and how it manifests
When a worker's event loop goes idle without an explicit terminate() call (the natural exit path), requested_terminate is never set. This PR moves this.deinit() to run before WebWorker__dispatchExit in exitAndDeinit(). deinit() calls parent_poll_ref.unrefConcurrently(this.parent), which transitions the KeepAlive status from .active → .inactive. WebWorker__dispatchExit then posts the close task to the parent thread asynchronously.
The specific code path that triggers it
When the close task runs on the parent thread and JS fires the close event, a handler that calls worker.ref() (a valid node:worker_threads API) invokes setRef(this, true). The only guard in setRef() (web_worker.zig ~line 588) is hasRequestedTerminate(), which reads the requested_terminate atomic — never set in the natural-exit path. The guard passes, so setRefInternal(true) calls parent_poll_ref.ref(parent). Since KeepAlive.status is now .inactive, KeepAlive.ref() does not no-op; it transitions status back to .active and increments the parent event loop's ref count.
Why existing code doesn't prevent it
notifyNeedTermination() (web_worker.zig ~line 612) already guards on this.status.load(.acquire) == .terminated, which IS set at the top of exitAndDeinit() before deinit() is called. But setRef() has no such status check — only the requested_terminate check, which is insufficient for the natural-exit path. In the old code order, deinit() ran after dispatchExit, so any worker.ref() call in the close handler would see KeepAlive.status == .active and KeepAlive.ref() would no-op; deinit() would then unref and balance it. After this PR the unref always precedes the close event, making the subsequent ref() permanently unbalanced.
Impact
WebWorker__destroy (called from ~Worker()) only calls bun.default_allocator.destroy(this) — it performs no KeepAlive cleanup. The event loop ref is permanently leaked. The parent process never exits, hanging indefinitely. This only triggers when a worker.ref() call is made inside a close event handler on a naturally-exited worker without a matching worker.unref(), but that is entirely valid API usage.
How to fix it
In setRef(), add a status check before calling setRefInternal:
pub fn setRef(this: *WebWorker, value: bool) callconv(.c) void {
if (this.hasRequestedTerminate()) return;
if (this.status.load(.acquire) == .terminated) return; // <-- add this
this.setRefInternal(value);
}this.status is set to .terminated at the top of exitAndDeinit(), before deinit() is called, so this guard is always true by the time parent_poll_ref.unrefConcurrently() fires.
Step-by-step proof
- Worker runs normally; event loop drains;
spin()reachesexitAndDeinit(). exitAndDeinit()setsthis.status = .terminatedandrequested_terminateis stillfalse.this.deinit()callsparent_poll_ref.unrefConcurrently(parent);KeepAlive.status → .inactive.WebWorker__dispatchExitposts the close task to the parent thread.- Parent thread fires the
closeevent; JS handler callsworker.ref(). setRef(this, true)checkshasRequestedTerminate() == false— guard passes.setRefInternal(true)callsparent_poll_ref.ref(parent);KeepAlive.status → .active, ref count +1.- Worker JS wrapper is eventually GC'd;
~Worker()callsWebWorker__destroy→bun.default_allocator.destroy(this). No unref. - Parent event loop ref count is permanently +1; parent process hangs forever.
2 participants
This PR has been marked as AI slop and the description has been updated to avoid confusion or misleading reviewers.
Many AI PRs are fine, but sometimes they submit a PR too early, fail to test if the problem is real, fail to reproduce the problem, or fail to test that the problem is fixed. If you think this PR is not AI slop, please leave a comment.