Repository navigation
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR fixes undefined behavior in ChangesBatch::pop UB fix and regression tests
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/regression/issue/30774.test.ts`:
- Around line 1-19: Replace the long multi-paragraph regression preamble with
the repo's standard two-line issue header: keep the GitHub URL line and add one
concise bug-summary line; for example, a single line noting that
ThreadPool::Batch::pop did a UB-causing atomic pointer cast and was fixed by
reading self.len directly (references: Batch::pop, ThreadPool::Batch,
HTTPThread::schedule, fetch()). Ensure no other explanatory paragraphs remain.
🪄 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: 60796828-197f-4a5b-b8b8-ad392c64d457
📒 Files selected for processing (11)
src/crash_handler/lib.rssrc/errno/lib.rssrc/perf/tracy.rssrc/runtime/cli/Arguments.rssrc/runtime/cli/run_command.rssrc/runtime/cli/upgrade_command.rssrc/runtime/jsc_hooks.rssrc/runtime/webview/ChromeProcess.rssrc/spawn/process.rssrc/spawn_sys/spawn_process.rstest/regression/issue/30774.test.ts
| // https://github.com/oven-sh/bun/issues/30774 | ||
| // | ||
| // `ThreadPool::Batch::pop` used to pointer-cast `&raw const self.len` (where | ||
| // `self.len: usize`) to `*const AtomicUsize` and call `.load(Ordering::Relaxed)`. | ||
| // That was a mechanical port of Zig's `@atomicLoad(usize, &this.len, .monotonic)`, | ||
| // but it is UB under Rust's memory model: `AtomicUsize` wraps `UnsafeCell<usize>` | ||
| // and the two types are not interchangeable via a pointer cast for atomic | ||
| // operations. `Batch` is not shared across threads (`pop` takes `&mut self`), | ||
| // so no atomic is needed — the fix is a plain `self.len` read. | ||
| // | ||
| // The `Batch::pop` code path is the batch-drain loop in `HTTPThread::schedule` | ||
| // (`src/http/HTTPThread.rs:992`), reached by every `fetch()` call. Unit tests | ||
| // in `src/threading/ThreadPool.rs` exercise `Batch::pop/push` directly; this | ||
| // TS smoke test confirms end-to-end that many concurrent `fetch()` requests | ||
| // all complete with correctly-attributed bodies. Observable behavior is | ||
| // unchanged by the fix (relaxed atomic load of a `usize` compiles to the same | ||
| // machine code as a plain load on x86/aarch64), so this test is a guard | ||
| // against future regressions in the batch code rather than a before/after | ||
| // demonstration of the UB. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Trim the regression header to the standard two-line issue comment pattern.
This preamble is much longer than the repo convention and includes details that can go stale. Please keep the URL line and one concise bug-summary line only.
Proposed edit
// https://github.com/oven-sh/bun/issues/30774
-//
-// `ThreadPool::Batch::pop` used to pointer-cast `&raw const self.len` (where
-// `self.len: usize`) to `*const AtomicUsize` and call `.load(Ordering::Relaxed)`.
-// That was a mechanical port of Zig's `@atomicLoad(usize, &this.len, .monotonic)`,
-// but it is UB under Rust's memory model: `AtomicUsize` wraps `UnsafeCell<usize>`
-// and the two types are not interchangeable via a pointer cast for atomic
-// operations. `Batch` is not shared across threads (`pop` takes `&mut self`),
-// so no atomic is needed — the fix is a plain `self.len` read.
-//
-// The `Batch::pop` code path is the batch-drain loop in `HTTPThread::schedule`
-// (`src/http/HTTPThread.rs:992`), reached by every `fetch()` call. Unit tests
-// in `src/threading/ThreadPool.rs` exercise `Batch::pop/push` directly; this
-// TS smoke test confirms end-to-end that many concurrent `fetch()` requests
-// all complete with correctly-attributed bodies. Observable behavior is
-// unchanged by the fix (relaxed atomic load of a `usize` compiles to the same
-// machine code as a plain load on x86/aarch64), so this test is a guard
-// against future regressions in the batch code rather than a before/after
-// demonstration of the UB.
+// Regression: concurrent fetch() requests must fully drain HTTPThread batch work without loss/duplication.Based on learnings: in test/regression/issue/, document issue context with the URL line plus a single one-line bug description, not multi-line prose.
🤖 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 `@test/regression/issue/30774.test.ts` around lines 1 - 19, Replace the long
multi-paragraph regression preamble with the repo's standard two-line issue
header: keep the GitHub URL line and add one concise bug-summary line; for
example, a single line noting that ThreadPool::Batch::pop did a UB-causing
atomic pointer cast and was fixed by reading self.len directly (references:
Batch::pop, ThreadPool::Batch, HTTPThread::schedule, fetch()). Ensure no other
explanatory paragraphs remain.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // TS smoke test confirms end-to-end that many concurrent `fetch()` requests | ||
| // all complete with correctly-attributed bodies. Observable behavior is | ||
| // unchanged by the fix (relaxed atomic load of a `usize` compiles to the same | ||
| // machine code as a plain load on x86/aarch64), so this test is a guard | ||
| // against future regressions in the batch code rather than a before/after | ||
| // demonstration of the UB. |
There was a problem hiding this comment.
🟡 This file is misplaced per repo conventions: test/regression/issue/${issueNumber}.test.ts is reserved for true regressions (worked in a previous release, then broke — CLAUDE.md:66, test/CLAUDE.md:153), but the file's own header states "Observable behavior is unchanged by the fix … this test is a guard against future regressions … rather than a before/after demonstration of the UB." That also means it passes with USE_SYSTEM_BUN=1, which CLAUDE.md:130 marks as CRITICAL grounds for the test being invalid. The Rust batch_tests are the real coverage here — this TS smoke test should be moved into an existing fetch/HTTP test file or dropped.
Extended reasoning...
What the convention says
Root CLAUDE.md:66 (and test/CLAUDE.md:153 verbatim) reserves test/regression/issue/${issueNumber}.test.ts for bugs that are true regressions: "worked in a previous release, then broke. An issue number alone is not enough — if the behavior was never correct, it's not a regression and the test belongs in the existing file for that module."
Root CLAUDE.md:130 separately states (marked CRITICAL): "Verify your test fails with USE_SYSTEM_BUN=1 bun test <file> and passes with bun bd test <file>. Your test is NOT VALID if it passes with USE_SYSTEM_BUN=1."
Why this file violates both rules
The PR description and the test file's own header comment (lines 15–19) explicitly state:
Observable behavior is unchanged by the fix (relaxed atomic load of a
usizecompiles to the same machine code as a plain load on x86/aarch64), so this test is a guard against future regressions in the batch code rather than a before/after demonstration of the UB.
And the PR body adds: "the miscompile is latent under today's LLVM/rustc targets."
So by the author's own account: (1) there is no release where this behavior worked and then broke — the UB never manifested observably — so it is not a "true regression" under the placement rule; and (2) the test will pass on a pre-fix build and on system bun, which directly fails the CRITICAL USE_SYSTEM_BUN=1 validity check.
Step-by-step proof
- Before this PR,
Batch::popdid(*(&raw const self.len).cast::<AtomicUsize>()).load(Ordering::Relaxed). On every shipped target (x86_64, aarch64) this lowers to the same plain load asself.len— the PR description confirms this. test/regression/issue/30774.test.tsfires 200 concurrentfetch()s and asserts each body round-trips. This exercisesHTTPThread::schedule→Batch::pop, but since the machine code is identical pre- and post-fix, the assertion holds on both builds.- Run
USE_SYSTEM_BUN=1 bun test test/regression/issue/30774.test.tson the released bun (pre-fix) → passes. Per CLAUDE.md:130, the test is therefore "NOT VALID." - Because step 3 passes on the released build, there is no "worked in release N, broke in release N+1" history — i.e., not a true regression — so per CLAUDE.md:66 the file does not belong under
test/regression/issue/.
Why existing safeguards don't catch this
The test/regression/issue/ directory is a convention enforced only by CLAUDE.md, not by CI tooling — there's no automated check that a regression-test file actually fails on system bun, so the misplacement lands silently.
Impact
No runtime correctness issue — purely a test-organization/repo-convention violation. But it's one the repo explicitly labels CRITICAL, and it adds a TS test to the regression suite that can never distinguish a fixed build from an unfixed one, so it provides no signal for #30774 specifically. The actual coverage for the fix is the Rust batch_tests module added in src/threading/ThreadPool.rs (pop_single_task / pop_drains_pushed_tasks_fifo), which directly exercises Batch::pop.
Fix
Either drop test/regression/issue/30774.test.ts (the Rust unit tests already cover the change), or — if the end-to-end fetch-batch-drain coverage is wanted as a forward guard — move it into an existing fetch/HTTP module test file (e.g., alongside other HTTPThread/fetch() concurrency tests) rather than the issue-numbered regression directory.
There was a problem hiding this comment.
Thanks for the rewrite, but this still doesn't address the placement concern and arguably trades one validity problem for another:
-
Still misplaced.
test/regression/issue/${n}.test.tsis for behavior that worked in a prior release and then broke (CLAUDE.md:66). unsafe: usize cast to AtomicUsize via pointer in ThreadPool #30774 was never observably broken — the file's own header still says "This UB is latent … there is no runtime reproducer a black-box test can drive." Rewriting the body doesn't change that it isn't a regression. -
Still passes with
USE_SYSTEM_BUN=1. The test now grepssrc/threading/ThreadPool.rsfrom the working tree — it tests the checked-out source, not the bun binary under test.USE_SYSTEM_BUN=1 bun test test/regression/issue/30774.test.tson this branch reads the already-fixed source and passes, so CLAUDE.md:130's CRITICAL check is still violated. -
A regex over Rust source isn't a runtime test of bun at all — it's a lint. It will also silently rot if
Batch::popis refactored (the^ \}body-end anchor and thelet len = self.len;positive match are both brittle).
The Rust batch_tests in src/threading/ThreadPool.rs already cover Batch::pop directly, and Miri/clippy are the right layer for the "don't reintroduce the pointer-cast" guard. Suggest dropping this file; if you want to keep an end-to-end concurrent-fetch guard, fold it into an existing test/js/bun/http/ or fetch test file rather than the issue-numbered regression dir.
0569b2d to
3442e54
Compare
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/regression/issue/30774.test.ts`:
- Around line 38-39: The test is over-constraining the expected fix by requiring
the exact statement "let len = self.len;"; update the positive assertion that
references popBody so it only confirms direct usage of self.len (e.g., assert
popBody contains the token "self.len" via a looser regex like /\bself\.len\b/ or
similar) rather than matching a specific assignment form, so equivalent safe
refactors still pass.
🪄 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: e9d84857-7f88-4a03-b3da-d7a5e4f38c52
📒 Files selected for processing (1)
test/regression/issue/30774.test.ts
| // Positive: the fix reads `self.len` directly. | ||
| expect(popBody).toMatch(/let\s+len\s*=\s*self\.len\s*;/); |
There was a problem hiding this comment.
Avoid over-constraining the fix to one exact statement form.
The positive assertion currently requires let len = self.len;, which can fail on equivalent safe refactors (e.g., direct if self.len == 0). Assert presence of direct self.len usage without pinning exact syntax.
Suggested change
- expect(popBody).toMatch(/let\s+len\s*=\s*self\.len\s*;/);
+ expect(popBody).toMatch(/\bself\.len\b/);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Positive: the fix reads `self.len` directly. | |
| expect(popBody).toMatch(/let\s+len\s*=\s*self\.len\s*;/); | |
| // Positive: the fix reads `self.len` directly. | |
| expect(popBody).toMatch(/\bself\.len\b/); |
🤖 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 `@test/regression/issue/30774.test.ts` around lines 38 - 39, The test is
over-constraining the expected fix by requiring the exact statement "let len =
self.len;"; update the positive assertion that references popBody so it only
confirms direct usage of self.len (e.g., assert popBody contains the token
"self.len" via a looser regex like /\bself\.len\b/ or similar) rather than
matching a specific assignment form, so equivalent safe refactors still pass.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
test/regression/issue/30774.test.ts:24-45— This file was rewritten (since the earlier review) toreadFileSyncsrc/threading/ThreadPool.rsand regex-match its text — it no longer exercises the bun binary at all, so it still passes withUSE_SYSTEM_BUN=1(CLAUDE.md:130 CRITICAL) and is still not a true regression (CLAUDE.md:66, test/CLAUDE.md:153). It also introduces a novel anti-pattern with no precedent intest/(grepping.rssource via brittle regexes like/^ \}/mthat break on reformatting or whensrc/isn't co-located withtest/). The earlier suggestion to "move into a fetch/HTTP test file" no longer applies — this file should be dropped entirely; the Rustbatch_testsmodule (and Miri) are the right place to guard this UB.Extended reasoning...
What changed since the earlier review
The previous inline comment on this file (claude[bot] id 3247113968) addressed a fetch-based smoke test — 200 concurrent
fetch()requests throughHTTPThread::schedule— and recommended either dropping it or moving it into an existing fetch/HTTP test file. The author has since rewritten the file completely: it now doesreadFileSync(join(import.meta.dir, "../../../src/threading/ThreadPool.rs")), narrows to theBatch::popbody via/impl Batch\s*\{[\s\S]*?pub fn pop\([^)]*\)[^{]*\{([\s\S]*?)^ \}/m, and asserts.not.toMatch(/&raw const self\.len.*cast::<AtomicUsize>/s)plus a positive.toMatch(/let\s+len\s*=\s*self\.len\s*;/). The file's own header now states "there is no runtime reproducer a black-box test can drive … this test inspectssrc/threading/ThreadPool.rsdirectly." This is a fresh comment on the rewritten version — the prior comment's quoted text and recommendation are both stale.Why it still violates CLAUDE.md:130 (CRITICAL —
USE_SYSTEM_BUN=1)CLAUDE.md:130 reads verbatim: "CRITICAL: Verify your test fails with
USE_SYSTEM_BUN=1 bun test <file>and passes withbun bd test <file>. Your test is NOT VALID if it passes withUSE_SYSTEM_BUN=1." The rewrite makes this strictly worse than the fetch version: the fetch test at least exercised the binary under test (even if behavior was identical pre/post-fix); the source-grep test readssrc/threading/ThreadPool.rsfrom the working tree, which is the post-PR source regardless of whichbunexecutable runs the test.USE_SYSTEM_BUN=1 bun test test/regression/issue/30774.test.tson the PR branch reads the same fixed file and passes — the test cannot distinguish a fixed build from an unfixed one by construction.Why it still violates CLAUDE.md:66 / test/CLAUDE.md:153 (true-regression rule)
Both files reserve
test/regression/issue/for bugs that "worked in a previous release, then broke." The new header explicitly admits "This UB is latent: relaxed atomic load of ausizecompiles to the same machine code as a plain load on x86 and AArch64, so there is no runtime reproducer." No release ever exhibited different behavior, so this is not a regression and does not belong undertest/regression/issue/.Novel anti-pattern with no precedent
A grep of
test/forreadFileSync.*src/.*\.(rs|zig)returns zero matches — no other test asserts on the textual content of a Rust/Zig source file. This is brittle in two specific ways: (a) the body-extraction regex terminates on/^ \}/m(four-space-indented closing brace) and the positive assertion requireslet len = self.len;literally — any reformatting ofThreadPool.rs(different indentation, inlining thelenlocal intoif self.len == 0) breaks the test without touching the UB it claims to guard; (b)join(import.meta.dir, "../../../src/...")hard-fails in any environment wheresrc/is not shipped alongsidetest/(e.g. packaged test artifacts).Step-by-step proof
- On the PR branch,
src/threading/ThreadPool.rs:387-393containslet len = self.len;and nocast::<AtomicUsize>inBatch::pop. - Run
USE_SYSTEM_BUN=1 bun test test/regression/issue/30774.test.ts. The system bun (pre-fix) executesreadFileSync("../../../src/threading/ThreadPool.rs"), which reads the checkout's post-fix source. - The regex extracts the
popbody, finds nocast::<AtomicUsize>, findslet len = self.len;→ all assertions pass. - Run
bun bd test test/regression/issue/30774.test.ts. Same file read, same result → pass. - Both runs pass identically → the test is "NOT VALID" per CLAUDE.md:130, and provides zero signal about the binary under test.
Fix
Drop
test/regression/issue/30774.test.tsentirely. The earlier "move to a fetch/HTTP test file" suggestion no longer applies (there is no fetch code left to move). The Rust#[cfg(test)] mod batch_testsadded insrc/threading/ThreadPool.rs(pop_single_task,pop_drains_pushed_tasks_fifo) already directly exercisesBatch::pop, and the class of UB this guards against (atomic-via-pointer-cast on non-atomic storage) is exactly whatcargo +nightly miri test -p bun_threadingis for — a regex on source text is a strictly inferior guard. - On the PR branch,
| // JSON.stringify so the raw CSS (which starts with `*`) parses as | ||
| // a JSON string literal. Relying on the define auto-quote recovery | ||
| // path for the raw value works post-#30679 but fails on any | ||
| // bootstrap bun older than that. | ||
| OVERLAY_CSS: JSON.stringify(css("../runtime/bake/client/overlay.css", !!debug)), |
There was a problem hiding this comment.
🟡 Wrapping OVERLAY_CSS in JSON.stringify(...) means bake-codegen no longer passes raw *{...} CSS through the define auto-quote recovery path, but two comments still cite it as the live motivating example: test/bundler/bun-build-api.test.ts:76 ("src/codegen/bake-codegen.ts passes verbatim as OVERLAY_CSS") and src/parsers/json_lexer.rs:1304 ("bake-codegen.ts's OVERLAY_CSS"). Both should be rephrased as historical motivation or have the bake-codegen reference dropped — a reader following them will now find JSON.stringify and be confused. Documentation drift only; no runtime impact.
Extended reasoning...
What changed and what went stale
This PR changes src/codegen/bake-codegen.ts:60 from OVERLAY_CSS: css(...) to OVERLAY_CSS: JSON.stringify(css(...)). The new comment at lines 56-59 explains why: "JSON.stringify so the raw CSS (which starts with *) parses as a JSON string literal. Relying on the define auto-quote recovery path for the raw value works post-#30679 but fails on any bootstrap bun older than that." In other words, bake-codegen now produces a properly-quoted JSON string that parses cleanly on the first try and never reaches the JSON-lexer auto-quote recovery path.
However, two comments elsewhere in the tree (both added by #30679, commit 314d044) still document the old behavior — passing raw *{...} CSS verbatim — as the live, present-tense motivating example for the auto-quote feature:
test/bundler/bun-build-api.test.ts:73-76: "a raw minified CSS string starts with*{...}, which src/codegen/bake-codegen.ts passes verbatim asOVERLAY_CSS."src/parsers/json_lexer.rs:1302-1304: "e.g. aBun.builddefine:whose value is a raw minified CSS string starting with*{...}(bake-codegen.ts'sOVERLAY_CSS)."
This PR's commit 8f79295 invalidates both. The PR's own new comment at bake-codegen.ts:56-59 shows the author is aware of exactly this relationship (it explicitly mentions "the define auto-quote recovery path" and #30679), so updating the two back-references is in scope but was overlooked.
Step-by-step proof
- Before this PR,
css("../runtime/bake/client/overlay.css", ...)returns a string like*{box-sizing:border-box}.... That raw string was passed as theOVERLAY_CSSdefine value. Bun.build'sdefine:parser feeds each value through the JSON lexer. A leading*is not valid JSON, so JSON lexer: tokenize?/*/(/)sodefineauto-quote can recover #30679 made the lexer tokenize*/?/(/)without erroring, allowingJSONLikeParser::parse_expr's auto-quote fallback to wrap the whole thing as a string literal. Both comments above were written to explain why that recovery path exists, citing bake-codegen'sOVERLAY_CSSas the concrete in-tree caller.- After this PR, the define value is
JSON.stringify("*{...}")→"\"*{box-sizing:border-box}...\"". This is a valid JSON string literal; the lexer producesTStringLiteralon the first byte ("), andparse_exprreturns it directly. The*/?/(/)tokenization arms injson_lexer.rs:1299-1310are never reached for this input. - A maintainer reading either stale comment, then opening
src/codegen/bake-codegen.tsto see the example in action, will findJSON.stringify(css(...))with a comment explicitly saying it avoids the auto-quote path — directly contradicting what they were just told.
Why nothing catches this
These are prose comments, not code. The auto-quote feature itself, the lexer arms, and the parametrized test cases at bun-build-api.test.ts:77-81 ("*{box-sizing:...}", "?foo", "(parenthesized)", ")close") all remain correct and continue to exercise the recovery path. Only the comment text pointing readers at the motivating in-tree caller is now dead. No CI check validates that cross-file comment references stay accurate.
Impact
Documentation drift only — no runtime, build, or test-correctness impact. The cost is future-maintainer confusion: someone investigating the auto-quote path follows the comment to bake-codegen.ts, finds the opposite of what was described, and has to git-blame to reconcile.
Fix
Rephrase both comments to either drop the bake-codegen reference entirely (the test cases and "raw minified CSS string starting with *{...}" description stand on their own) or make it past-tense/historical, e.g.:
bun-build-api.test.ts:76: "...starts with*{...}(the original motivation was bake-codegen'sOVERLAY_CSSbefore it was JSON.stringify-wrapped)."json_lexer.rs:1304: "...starting with*{...}(originally hit bybake-codegen.ts'sOVERLAY_CSS)."
4b5e844 to
b1a3e7d
Compare
| // of a plain `usize` to `AtomicUsize` would be UB under Rust's memory | ||
| // model, and there is no concurrent writer that would require an | ||
| // atomic here. A plain read preserves the observable behavior. | ||
| let len = self.len; |
There was a problem hiding this comment.
🟡 Nit: since you're already cleaning up Batch::pop, the trailing if len == 0 { self.tail = None; } at lines 413-415 is provably dead — len is the immutable local bound at line 394 and the function already returned at 395-397 when it was zero. The real last-element tail clear happens in the else arm at 408-409. (Same dead code exists in ThreadPool.zig:124-126, so feel free to keep it for Zig diffability — but this PR already broke 1:1 parity by dropping the atomic load.)
Extended reasoning...
What the issue is
Batch::pop binds let len = self.len; at line 394, then immediately returns None at lines 395-397 if len == 0. len is an immutable let binding that is never reassigned. Yet at lines 412-415 the function still does:
self.len -= 1;
if len == 0 {
self.tail = None;
}Since execution can only reach line 413 when len >= 1, the if len == 0 branch is unreachable dead code. This is not a bug introduced by this PR — the previous let len = unsafe { ...atomic load... } was equally immutable and had the same early return — but this PR's express purpose is cleaning up exactly this function, it rewrites the let len = ... line, and it adds batch_tests covering both the single-task and multi-task drain paths, so the leftover is worth flagging.
Step-by-step proof
- Line 394:
let len = self.len;—lenis bound immutably.self.lenis some value N. - Lines 395-397:
if len == 0 { return None; }— if N = 0, we return. Past this point, N ≥ 1. - Lines 398-410: pop the head task. If it was the last task (
next.is_null()), theelsearm at 404-410 already setsself.tail = Noneandself.head = None. Otherwiseself.headadvances andself.tailis left alone. - Line 412:
self.len -= 1;— mutatesself.len, not the locallen.lenis still N ≥ 1. - Line 413:
if len == 0— tests N, which is ≥ 1. The condition is always false; line 414 never executes.
The new tests confirm the actual tail-clearing works without this branch: pop_single_task asserts batch.tail.is_none() after popping the sole element, which is satisfied by line 408, not line 414.
Why nothing else catches it
This is a faithful port of the identical dead code in src/threading/ThreadPool.zig:124-126 — Zig captures const len = @atomicLoad(...), early-returns on 0, then re-checks the stale len after this.len -= 1. The Rust port preserved it line-for-line. rustc silently dead-code-eliminates the branch (no dead_code lint fires because the lint targets unused items, not unreachable expressions guarded by a runtime-looking condition), and the new batch_tests pass either way since the branch contributes nothing.
Impact
Zero runtime impact — the optimizer removes it. Purely a code-clarity nit: a reader following the function has to convince themselves the second len == 0 check is dead rather than a subtle re-check of mutated state.
Fix
Delete lines 413-415. Both new batch_tests (pop_single_task, pop_drains_pushed_tasks_fifo) and the 30774.test.ts source-grep test continue to pass. The only counter-argument is preserving line-for-line parity with ThreadPool.zig (the file carries many PORT NOTE comments to that effect) — but this PR already diverged from Zig at line 394 by replacing @atomicLoad with a plain read, so the parity argument is weakened for this function specifically. If parity is preferred, the Zig side could drop its lines 124-126 in the same PR.
`Batch::pop` reads its `len` field via `(&raw const self.len).cast::<AtomicUsize>().load(Relaxed)`, a mechanical port of Zig's `@atomicLoad(usize, &this.len, .monotonic)`. That pointer cast is UB under Rust's memory model: `AtomicUsize` wraps `UnsafeCell<usize>` and the two types are not interchangeable via a pointer cast for atomic operations. `Batch` is not shared across threads. `pop` takes `&mut self`, and every mutation of `len` already goes through `&mut self` (`self.len -= 1` in `pop`, `self.len += batch.len` in `push`). Nothing requires an atomic here; replace the cast with a plain `self.len` read. Adds a unit test in `batch_tests` covering the single-task and multi-task `pop` paths (same pattern as `RwLock.rs`'s inline tests), plus a smoke test for the `fetch()` batch-drain path (`HTTPThread::schedule` calls `Batch::pop` on every incoming request). Observable behavior is unchanged — relaxed atomic load of a `usize` compiles to the same machine code as a plain load on x86/aarch64 — so the tests are a guard against future regressions rather than a before/after demonstration.
`Bun.build.define` values are parsed as JSON, with an auto-quote recovery path for non-JSON strings. Passing the raw minified CSS as the value relies on that recovery, which the JSON lexer only supports post-#30679 (the leading `*` previously aborted the lexer before recovery could run). Any bootstrap bun older than that fails the build with `Unsupported syntax: Operators are not allowed in JSON`. Wrap the value in `JSON.stringify` so it parses as a JSON string literal on every bun version, matching the sibling `side: JSON.stringify(side)` line. Explicit is better than implicit here either way.
6cb074e to
1778491
Compare
|
Stale PR review: closing. This PR has had no human review since it opened on 2026-05-15. The open PR #40438 changes the same line ( Reopen if this evidence is wrong. |
1 participant
Repro
src/threading/ThreadPool.rs:389:This pointer-cast + atomic-load is a mechanical port of Zig's
@atomicLoad(usize, &this.len, .monotonic)fromsrc/threading/ThreadPool.zig:110.Cause
Under Rust's memory model,
AtomicUsizewrapsUnsafeCell<usize>and isnot interchangeable with a plain
usizevia a pointer cast for atomicoperations. The sibling atomic types already satisfy
repr(C)+ same sizeand alignment as the underlying integer, but the validity / aliasing rules
for atomic ops require the storage itself to be an
AtomicUsize— forming&AtomicUsizefrom a&usizeand calling.load()is UB, even underOrdering::Relaxed.There is no concurrent writer in the first place:
Batch::poptakes&mut self, and every other mutation oflenis also&mut self-gated(
self.len -= 1inpop,self.len += batch.leninpush). The Zig@atomicLoadcarried no synchronization contract either; it was just theZig idiom. Making the field an
AtomicUsizewould be the wrong fix — it'dforce
push/poponto atomic RMW ops for no reason.Fix
Replace the pointer cast with a plain
self.lenread. Observable behavioris unchanged: relaxed atomic load of a
usizecompiles to the samemachine code as a plain load on x86 and AArch64.
Verification
cargo test -p bun_threading batch_tests— 2/2 pass (covers single-taskand multi-task drain paths).
bun bd test test/regression/issue/30774.test.ts— pass; 200 concurrentfetch()requests all round-trip through theHTTPThread::schedulebatchdrain (
src/http/HTTPThread.rs:992is the only external caller ofBatch::pop) with correctly-attributed bodies.The TS smoke test is a forward guard against future regressions in the
batch code, not a before/after demonstration of the UB — the miscompile
is latent under today's LLVM/rustc targets. The Rust unit tests in
batch_testsare the direct exercise of the fix.Rebase notes
Rebased twice onto moving main. The second rebase conflicted in
src/threading/ThreadPool.rs: main had cleaned up porting commentsfile-wide (including shortening the comment on the exact line this PR
replaces). Resolved by taking main's file and re-applying only the
Batch::popchange and thebatch_testsmodule, so none of main'scomment cleanup is reverted. The TS test and
bake-codegen.tshunksapplied cleanly.
Closes #30774