ai slop - #29363
Conversation
…t thread The Zig WebWorker struct is destroyed on the worker thread in exitAndDeinit() once the worker's event loop finishes. The C++ Worker object outlives it and keeps a raw impl_ pointer. If the parent thread calls worker.ref()/unref()/terminate() while (or after) the worker thread tears down, WebWorker__setRef / WebWorker__notifyNeedTermination read from the freed struct. Guard impl_ with a lock: clear it from WebWorker__dispatchExit (called on the worker thread right before the Zig struct is freed), and have setKeepAlive()/terminate() take the same lock and early-return when impl_ is null. Also clear impl_ when WebWorker__updatePtr fails to spawn the thread.
|
Updated 10:27 PM PT - Apr 15th, 2026
❌ @robobun, your commit 1b946a8 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 29363That installs a local version of the PR into your bun-29363 --bun |
|
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)
WalkthroughAdds a lock (m_implLock) to protect 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/web/workers/worker-terminate-race.test.ts`:
- Around line 17-18: Refactor the combined assertion that checks
stdout/stderr/exitCode into the CLAUDE.md house-style: after awaiting stdout,
stderr, and exited into the variables stdout, stderr, exitCode, first assert
stdout is "" (expect(stdout).toBe("")), then add the conditional check if
(exitCode !== 0) { expect(stderr).toBe(""); } to surface stderr on failure, and
finally assert the exit code (expect(exitCode).toBe(0)); update the assertion
block that currently references stdout/stderr/exitCode together to use these
separate checks.
🪄 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: 10675949-abaa-4ed9-9c88-bf59d8d18917
📒 Files selected for processing (3)
src/bun.js/bindings/webcore/Worker.cppsrc/bun.js/bindings/webcore/Worker.htest/js/web/workers/worker-terminate-race.test.ts
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/js/web/workers/worker-terminate-race.test.ts (1)
17-18:⚠️ Potential issue | 🟡 MinorUse house-style split assertions for subprocess results.
Line 18 combines
stdout/stderr/exitCodeinto one assertion, which conflicts with repo test style and makes failure diagnostics less useful. Keepstdoutassertion first, conditionally print-checkstderron failure, then assert exit code last.♻️ Suggested change
- const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - expect({ stdout, stderr, exitCode }).toEqual({ stdout: "", stderr: expect.any(String), exitCode: 0 }); + const [stdout, stderr, exitCode] = await Promise.all([ + proc.stdout.text(), + proc.stderr.text(), + proc.exited, + ]); + expect(stdout).toBe(""); + if (exitCode !== 0) { + expect(stderr).toBe(""); + } + expect(exitCode).toBe(0);As per coding guidelines,
expect(stdout)should come beforeexpect(exitCode), and based on learnings this file follows theif (exitCode !== 0) { expect(stderr).toBe(""); }house style for better failure output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/js/web/workers/worker-terminate-race.test.ts` around lines 17 - 18, Split the combined assertion into the repo's house-style split checks: first assert stdout is empty using expect(stdout).toBe(""), then if (exitCode !== 0) assert stderr is empty with expect(stderr).toBe("") to surface stderr only on failure, and finally assert the exit code with expect(exitCode).toBe(0); apply these changes around the variables stdout, stderr, exitCode returned from Promise.all so diagnostics match project style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/js/web/workers/worker-terminate-race.test.ts`:
- Around line 17-18: Split the combined assertion into the repo's house-style
split checks: first assert stdout is empty using expect(stdout).toBe(""), then
if (exitCode !== 0) assert stderr is empty with expect(stderr).toBe("") to
surface stderr only on failure, and finally assert the exit code with
expect(exitCode).toBe(0); apply these changes around the variables stdout,
stderr, exitCode returned from Promise.all so diagnostics match project style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1f0f628b-6bee-4fc8-a71f-90271dbde6a6
📒 Files selected for processing (1)
test/js/web/workers/worker-terminate-race.test.ts
![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 genuine use-after-free with a straightforward mutex-guard pattern, but given it touches C++ cross-thread worker lifecycle management, it warrants a human look — especially to confirm the lock ordering in terminate() (flag set atomically before lock acquired) is intentional and safe.
Extended reasoning...
Overview\nThis PR adds a Lock m_implLock to guard the impl_ pointer in WebCore::Worker, which points to the Zig WebWorker struct that is freed on the worker thread during teardown. Three methods (setKeepAlive, terminate, updatePtr) now hold the lock before dereferencing impl_ and early-return if it is null. WebWorker__dispatchExit calls clearZigImpl() (which locks and nulls the pointer) before dispatching exit and freeing the Zig struct. The create() site also locks before assigning impl_.\n\n### Security risks\nNo security risks. This is a memory-safety fix (use-after-free → use-after-poison in ASAN terms), not a security-sensitive code path.\n\n### Level of scrutiny\nThe pattern is textbook — protect a raw pointer with a mutex, clear it before the pointee is freed, early-return on null — but this is production C++ concurrency code on a critical path (worker thread lifecycle). One detail worth a human eye: in terminate(), m_terminationFlags.fetch_or(TerminateRequestedFlag) is done before acquiring m_implLock. This appears intentional (the description says event semantics are unchanged), but the ordering deserves explicit sign-off.\n\n### Other factors\nThe only bug found is a minor nit: the new test uses stderr: expect.any(String) instead of expect(stderr).toBe(""), which silently accepts non-empty stderr. This is a test quality issue, not a correctness issue with the fix itself. No prior reviews on this PR.
| }); | ||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect({ stdout, stderr, exitCode }).toEqual({ stdout: "", stderr: expect.any(String), exitCode: 0 }); | ||
| } |
There was a problem hiding this comment.
🟡 The run() helper in worker-terminate-race.test.ts checks stderr with expect.any(String) instead of expect(stderr).toBe(""), violating the convention explicitly documented in test/CLAUDE.md. This silently masks non-fatal warnings or error output during teardown races that don't cause a non-zero exit code.
Extended reasoning...
Bug description: The run() helper at line 19 of worker-terminate-race.test.ts asserts stderr using stderr: expect.any(String), which accepts any string content—including non-empty error output. The documented convention in test/CLAUDE.md (lines 48 and 94) explicitly shows the pattern expect(stderr).toBe("") for subprocess tests.
Specific code path: The assertion object is { stdout: "", stderr: expect.any(String), exitCode: 0 }. The expect.any(String) matcher passes as long as stderr is a string, making it equivalent to asserting nothing about stderr content. All three test cases share this single run() helper, so all three inherit the permissive check.
Why existing code doesn't prevent it: Jest/Bun test's toEqual matcher treats expect.any(String) as a type-only check ("is this a string?"), not a value check. Since stderr is always a string from proc.stderr.text(), the assertion can never fail regardless of what the subprocess prints.
Impact: The tests exist specifically to exercise race conditions during worker thread teardown (ref/unref/terminate while the worker exits). If the fix is incomplete or regresses, the process might print non-fatal diagnostic messages, ASAN symbolization notes, Zig panic-style warnings, or other runtime output to stderr before exiting cleanly with code 0. These would be silently swallowed. ASAN crashes are still caught via exit code, but softer issues are not.
Step-by-step proof: Suppose a regression causes WebWorker__setRef to print "WARNING: potential race detected\n" to stderr before returning normally (exit 0). With expect.any(String), the test passes. With expect(stderr).toBe(""), the test fails and the regression is caught. The empty workers ("data:text/javascript,") produce no console output by design, so there is no valid reason to tolerate non-empty stderr.
Fix: Change line 19 from stderr: expect.any(String) to stderr: "" (or restructure the assertion to use expect(stderr).toBe("") separately), matching the pattern used in neighboring tests like message-port-closed-leak.test.ts:51.
There was a problem hiding this comment.
The attempted fix (commit 1b946a8) improves failure diagnostics by splitting the combined assertion, but still does not fully address the original concern. The current code only checks expect(stderr).toBe("") when exitCode \!== 0, which means stderr is still silently ignored on clean exits (exitCode 0).
The original comment's core concern was: if a regression causes a non-fatal warning to be printed to stderr but the process still exits with code 0, the test would still pass silently. That scenario is unchanged.
The fix should unconditionally assert expect(stderr).toBe("") regardless of exit code:
expect(stdout).toBe("");
expect(stderr).toBe("");
expect(exitCode).toBe(0);The empty data:text/javascript, workers produce no legitimate stderr output, so there is no valid reason to conditionally skip the assertion based on exit code.
There was a problem hiding this comment.
The conditional is intentional. Debug/ASAN builds print WARNING: ASAN interferes with JSC signal handlers; useWasmFastMemory and useWasmFaultSignalHandler will be disabled. to stderr on every invocation, so an unconditional expect(stderr).toBe("") would make the test fail on every debug CI run. The exit-code check catches the actual use-after-free (ASAN aborts with non-zero exit), and stderr is surfaced in the failure output when that happens.
|
|
||
| void Worker::setKeepAlive(bool keepAlive) | ||
| { | ||
| Locker locker { m_implLock }; | ||
| if (!impl_) | ||
| return; | ||
| WebWorker__setRef(impl_, keepAlive); | ||
| } | ||
|
|
||
| void Worker::clearZigImpl() | ||
| { | ||
| Locker locker { m_implLock }; | ||
| impl_ = nullptr; | ||
| } | ||
|
|
||
| bool Worker::updatePtr() | ||
| { | ||
| Locker locker { m_implLock }; | ||
| if (!WebWorker__updatePtr(impl_, this)) { | ||
| impl_ = nullptr; | ||
| m_onlineClosingFlags = ClosingFlag; | ||
| m_terminationFlags.fetch_or(TerminatedFlag); | ||
| return false; |
There was a problem hiding this comment.
🟣 Pre-existing ref count leak when Worker thread spawn fails. In Worker::create(), worker->ref() is called unconditionally before checking if impl is null; the matching worker->deref() only fires inside WebWorker__dispatchExit, which is never reached on the two early-failure paths (null impl from WebWorker__create, or thread spawn failure via WebWorker__updatePtr). The Worker object is permanently leaked in both cases. This PR correctly adds impl_=nullptr in the updatePtr() failure path (fixing the use-after-free), but the companion ref count imbalance is left unaddressed.
Extended reasoning...
What the bug is and how it manifests
In Worker::create() (Worker.cpp), worker->ref() is called unconditionally at line 222 with the comment "now referenced by Zig". This extra reference is the Zig-side ownership token that is balanced only by worker->deref() inside WebWorker__dispatchExit (Worker.cpp:493). There are two code paths where WebWorker__dispatchExit is never called, leaving the refcount permanently inflated by 1 and the Worker object unreachable for destruction.
The specific code paths that trigger it
Path A — WebWorker__create returns null: worker->ref() is called unconditionally, raising refcount to 2 (adoptRef starts it at 1). If WebWorker__create() returns null (e.g., a preload module URL fails to resolve), the code executes return Exception { TypeError, ... }. The local Ref destructs, decrementing refcount to 1. No code ever calls deref() again — the Worker leaks.
Path B — WebWorker__updatePtr fails (thread spawn fails): In web_worker.zig, when std.Thread.spawn() fails, the Zig side calls worker.deinit() (line 76) and returns false. The deinit() function only frees Zig-side memory; it does NOT call WebWorker__dispatchExit. Without dispatchExit, worker->deref() is never called. The JS wrapper is eventually GC'd (one deref: refcount 2 to 1), but it never reaches 0.
Why existing code does not prevent it
The ref/deref pattern relies entirely on WebWorker__dispatchExit being the single point of cleanup. There is no guard or RAII wrapper around the worker->ref() call in create() that would ensure a matching deref() on early-exit paths. The two failure paths diverge before a Zig thread is ever started, so the normal exit/deinit lifecycle never runs.
Impact
Every new Worker() call that fails at thread-spawn time permanently leaks a Worker C++ object (and all memory it holds: options, URL strings, pending task queue, etc.). In environments where worker creation can transiently fail (resource limits, preload module errors), this accumulates unboundedly.
How to fix it
For Path A: call worker->deref() before returning the Exception, or move worker->ref() to after the null check so it is only called when a thread is actually starting. For Path B: add worker->deref() in the updatePtr() failure branch in Worker::create(), mirroring what WebWorker__dispatchExit would have done.
Step-by-step proof for Path A
- new Worker() is called with a URL whose preload module fails to resolve.
- adoptRef(*new Worker(...)) — refcount = 1.
- WebWorker__create(...) returns null.
- worker->ref() is called — refcount = 2.
- if (!impl) is true, return Exception { TypeError, ... }.
- The local Ref destructor runs — refcount = 1.
- No further owner; Worker::~Worker() is never called. Object is permanently leaked.
There was a problem hiding this comment.
Agreed this is a pre-existing leak — leaving it out of this PR to keep the diff focused on the use-after-free.
|
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
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.