ci: fix server.allocator vtable mismatch + 3 broken-on-release tests - #29926
Conversation
server: use bun.default_allocator instead of MimallocArena.getThreadLocalDefault(). Both route to mimalloc, but the vtables differ, so collections that flow between server-owned and default-owned code (BabyList in the response sink, upgrade path) trip CheckedAllocator in ci_assert builds. Fixes the "allocators do not match" panic in serve-stream-reject-flush-leak and the silent-crash in the websocket upgrade UAF test on Windows release lanes. (Same change as #29916, without the unrelated X509 hunks already merged via #29881.) test(serve-stream-reject-flush-leak): skip on Windows. The fixture needs an 8 MiB tryEnd() to hit loopback backpressure; Windows auto-tunes the send buffer past that so pending_flush is never created (0/40 on #29916 CI). test(html-rewriter-leak): GC every 1k iterations instead of once at the end. The RSS path never worked on any release platform (#29879's own CI: 113-233 MB across darwin/linux/windows/asan, all post-fix). A single trailing Bun.gc(true) collects everything but neither mimalloc nor ASAN promptly return freed pages to the OS, so RSS pins at the 16k-builder peak. Batching bounds peak ≈ retained. Verified: pre-fix release fails at 68 MB; post-fix release/asan/debug pass at <1 MB. test(websocket-server): stop truncating stderr to 3 lines in the upgrade UAF test — the panic line was past line 3, leaving an empty diff. test(integration-rspack): pin create-rsbuild to @1. rsbuild 2.0.x bundles mimalloc v3 inside @rspack/binding-win32-arm64-msvc; two static mimalloc instances segfault deterministically in ntdll during ExitProcess on Windows arm64.
|
Updated 6:15 AM PT - Apr 29th, 2026
❌ @Jarred-Sumner, your commit 943015a has 4 failures in
🧪 To try this PR locally: bunx bun-pr 29926That installs a local version of the PR into your bun-29926 --bun |
|
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:
WalkthroughMultiple test files are updated to improve robustness and debugging: an Rspack integration test pins to a specific version with clarifying comments about a segfault scenario, a WebSocket protocol test removes stderr output truncation, a memory leak test switches from mimalloc stats to RSS-based measurements with conditional debug mode handling, and an HTTP streaming test fixture increases response body allocation with Windows-specific documentation. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
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.
No issues found. The four test changes are clean CI hygiene, but swapping server.allocator to bun.default_allocator in server.zig touches the HTTP server's allocator init — functionally it looks equivalent (both route to mimalloc, and getThreadLocalDefault already returns bun.default_allocator under ASAN), but it's a critical-path change worth a human glance.
Extended reasoning...
Overview
This PR bundles five changes aimed at clearing pre-existing CI failures:
src/bun.js/api/server.zig— replacesMimallocArena.getThreadLocalDefault()withbun.default_allocatorforserver.allocator, and drops the now-unusedArenaimport. Both allocators dispatch to mimalloc; the difference is vtable identity, which was trippingCheckedAllocator.assertEqinci_assertbuilds.test/js/workerd/html-rewriter-leak.test.ts— switches from one trailing GC to batched GC every 1k iterations so RSS tracks retained (not peak) memory on release/ASAN builds.test/js/bun/http/serve-stream-reject-flush-leak.test.ts— wraps the test intest.skipIf(isWindows)because Windows loopback buffers absorb the 8 MiB write without backpressure.test/js/bun/websocket/websocket-server.test.ts— stops truncating stderr to 3 lines so panic output is visible in failure diffs.test/js/bun/test/parallel/test-integration-rspack.ts— pinscreate rsbuild@1instead of@latestto avoid a Windows-arm64 double-mimalloc segfault from rsbuild 2.x.
Security risks
None identified. No auth, crypto, input parsing, or network surface changes. The allocator swap is between two vtables that both call into mimalloc; no new allocator or untrusted data path is introduced.
Level of scrutiny
Mixed. The four test-file edits are low-risk, mechanical CI stabilization (skip-on-platform, version pin, GC cadence, stderr formatting) and would be approvable on their own. The server.zig change, while small and well-justified, alters allocator initialization for every Bun.serve() instance — a hot, production-critical path. I verified in src/allocators/MimallocArena.zig:119-124 that getThreadLocalDefault() already documents "there is no per-thread default heap to cache" and returns bun.default_allocator under ASAN, so the runtime behavior should be identical. Still, allocator identity in the server is the kind of thing where a maintainer should confirm there are no other consumers relying on the old vtable.
Other factors
- No CODEOWNERS coverage on
server.zig. - Bug-hunting system found nothing.
- PR description is detailed with before/after measurements and references to the superseded #29916.
- No prior human reviews or outstanding comments on the thread.
The previous version's RSS path failed on every release platform from the PR that added it (#29879's own CI: 113-233 MB). A single trailing Bun.gc(true) collects everything, but mimalloc/ASAN don't promptly return pages to the OS, so RSS measured peak live set, not retained. Now: 3 identical warmup passes to establish the allocator high-water mark, then 3 measured passes. Any RSS growth past the warmup plateau is what's actually retained. release post-fix: pass, ~-0.6 MB, 1.5s release pre-fix: fail, +52 MB, 1.2s asan post-fix: pass, ~0.0 MB, 7.7s Threshold 25 sits at ~half the unfixed signal. Debug skipped: ~40s/pass and debug-allocator tracking noise drowns the signal; CI has no debug test lane.
There was a problem hiding this comment.
No bugs found and the test fixes look sound, but swapping server.allocator in Bun.serve() init is a core-runtime change — even though both vtables route to the same mimalloc global heap, I'd like a human to sign off on that hunk.
Extended reasoning...
Overview
This PR bundles one runtime change with four test-stability fixes:
src/bun.js/api/server.zig: replaceMimallocArena.getThreadLocalDefault()withbun.default_allocatorfor the server's.allocatorfield, and drop the now-unusedArenaimport.serve-stream-reject-flush-leak.test.ts: wrap intest.skipIf(isWindows)because Windows loopback auto-tunes the send buffer large enough that the 8 MiB write never backpressures, so the fixture's precondition can't be met.test-integration-rspack.ts: pincreate rsbuild@1instead of@latestto avoid a known double-mimalloc segfault on Windows arm64 with rsbuild 2.0.x.websocket-server.test.ts: stop truncating stderr to 3 lines so panic output isn't hidden.html-rewriter-leak.test.ts: rewrite the RSS-based leak detection to use symmetric warmup/measure passes with per-pass GC, skip on debug, and pinBUN_GARBAGE_COLLECTOR_LEVEL=0.
Security risks
None. No auth, crypto, input parsing, or privilege boundaries are touched. The allocator swap is between two interfaces to the same underlying mimalloc heap; it does not change ownership or lifetime semantics.
Level of scrutiny
The four test-file changes are low-risk CI stabilization (platform skips, version pin, assertion-output tweak, leak-test methodology). The server.zig change, however, alters which allocator every Bun.serve() instance uses. I checked MimallocArena.getThreadLocalDefault() (src/allocators/MimallocArena.zig:121) and confirmed it already returns bun.default_allocator under ASAN and otherwise a vtable that routes to the same global mimalloc functions — so the change is a vtable-identity normalization, not a behavioral change. The ~6 uses of this.allocator in server.zig are plain alloc/free/dupe with no arena-lifecycle assumptions. Still, "change the allocator for the HTTP server" is the kind of core-path edit that benefits from a human glance rather than bot auto-approval.
Other factors
- No CODEOWNERS entry covers these files.
- The PR description is thorough, includes before/after measurements for the html-rewriter test across release/asan/pre-fix builds, and confirms the leak is still detected on a regressed build.
- The html-rewriter test now skips debug builds entirely; the comment explains CI has no debug test lane, which is a reasonable trade-off but does drop the previously-passing precise-counter path.
- Pinning rsbuild to a major version is a standard pattern for integration tests guarding a specific regression rather than upstream HEAD.
…nstead of skipping Replace skipIf(isWindows) with the same fixture change pushed to #29916: pause the client so Windows' loopback fast-path can't absorb the 8 MiB write, then destroy the socket from the server side via setImmediate after the throw (handleRejectStream runs on the microtask queue first).
Backpressure is now deterministic per-iteration (paused client), and the
metric is heapStats().protectedObjectTypeCounts.Promise (exact). 40 was
hedging for the old on('data', destroy) timing.
… loopback socket.pause() only stops userspace reads — Windows' kernel SO_SNDBUF + SO_RCVBUF autotune to ~16 MiB combined on loopback and still absorb the full 8 MiB synchronously (0/10 backpressure on Win 11 aarch64). 64 MiB outsizes that. The buffer is reused across iterations so this only costs one larger allocation.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
test/js/bun/http/serve-stream-reject-flush-leak-fixture.ts:67-78— nit: the PR description still has a section titled "serve-stream-reject-flush-leak.test.ts — skip on Windows" stating "POSIX coverage is sufficient", but commit a721c57 changed the approach — the fixture nowsocket.pause()s and destroys from the server side to force backpressure on Windows too (noskipIfanywhere). Since the description becomes the squash-merge commit message, it's worth updating that section so future readers don't think Windows coverage was dropped.Extended reasoning...
What's mismatched
The PR description contains this section:
serve-stream-reject-flush-leak.test.ts— skip on WindowsOnce the allocator panic is fixed, the test next fails on Windows with
insufficient backpressure: 0/40… the 8 MiBtryEnd()fits in Windows' auto-tuned loopback send buffer sopending_flushis never created. The leak is platform-agnostic Zig; POSIX coverage is sufficient.But the code that actually landed does not skip on Windows. Commit a721c57 (
test(serve-stream-reject-flush-leak): force backpressure on Windows instead of skipping) replaced the skip with a deterministic-backpressure approach, and the in-code comment at lines 67–71 of the fixture documents it explicitly:Windows' loopback fast-path will absorb the full 8 MiB into the kernel if the client is draining, so explicitly
pause(); the server side destroys the socket oncehandleRejectStreamhas run.Step-by-step verification
grep -E 'isWindows|skipIf|process\.platform' test/js/bun/http/serve-stream-reject-flush-leak*→ no matches. Neither the fixture norserve-stream-reject-flush-leak.test.tscontains any platform guard.git log --oneline -- test/js/bun/http/serve-stream-reject-flush-leak-fixture.tsshows:a721c57 test(serve-stream-reject-flush-leak): force backpressure on Windows instead of skippinge1e0cba test(serve-stream-reject-flush-leak): drop ITERATIONS 40 → 10
The author explicitly pivoted mid-PR from "skip" to "force backpressure".
- The fixture now (a) calls
socket.pause()immediately after writing the request line so the kernel can't drain into userspace, and (b) schedulessetImmediate(() => currentSocket?.destroy())from insidepull()so the parkedtryEnd()connection is torn down afterhandleRejectStreamruns. This makes the backpressure path deterministic on all platforms, including Windows.
Why it matters
In this repo the PR description becomes the squash-merge commit message. A future reader running
git log/git blameon this test will see prose claiming "POSIX coverage is sufficient" and may reasonably conclude the test has no Windows coverage — when in fact Windows coverage was deliberately kept and the fixture was hardened specifically for it. That's the opposite of what shipped.Fix
Before merging, replace the "skip on Windows" section of the PR description with something like:
serve-stream-reject-flush-leak.test.ts— force backpressure on WindowsOnce the allocator panic is fixed, the test next failed on Windows with
insufficient backpressure: 0/40because the 8 MiBtryEnd()fits in Windows' auto-tuned loopback send buffer. Fixed by having the clientpause()immediately after sending the request and having the server destroy the socket viasetImmediateafterhandleRejectStreamruns, sopending_flushis created deterministically on all platforms.This is purely a documentation/metadata fix — no code change required.
There was a problem hiding this comment.
LGTM — test-only CI stabilization; the stale 8 MiB comment is a non-blocking nit.
Extended reasoning...
Overview
This PR touches four test files only — no runtime/production code. The server.allocator → bun.default_allocator Zig change referenced in the title already landed on main via #29916 (commit 306b381), so the actual diff here is purely test stabilization:
test-integration-rspack.ts: pinrsbuild@latest→rsbuild@1to dodge a known mimalloc-v3 double-instance segfault on Windows arm64.websocket-server.test.ts: stop truncating stderr to 3 lines so panic output is visible in failure diffs.serve-stream-reject-flush-leak-fixture.ts: bump the reused body chunk 8 MiB → 64 MiB so Windows loopback can't absorb the whole write andpending_flushis actually exercised; plus a cosmeticconsole.errorreflow.html-rewriter-leak.test.ts: rewrite the leak-measurement methodology to use identical warmup/measure RSS passes (so allocator footprint plateaus before the baseline), forceBUN_GARBAGE_COLLECTOR_LEVEL=0, skip on debug builds, and tighten the timeout to 15s.
Security risks
None. All changes are confined to test fixtures and assertions. No auth, crypto, network-input parsing, or production code paths are touched.
Level of scrutiny
Low-to-moderate. Three of the four changes are mechanical (version pin, un-truncate stderr, buffer-size bump). The html-rewriter-leak.test.ts rewrite is the only non-trivial piece, but it's a test-methodology change backed by an empirical table in the PR description showing the new approach passes on release/ASAN post-fix (0–0.6 MB) and still flags the regression pre-fix (68.5 MB vs the 25 MB threshold). The skipIf(isDebug) is justified since CI has no debug test lane and the comment documents why.
Other factors
The one inline finding is a stale "8 MiB" reference in the oneRequest() comment after the CHUNK bump — purely a documentation inconsistency with zero behavioral impact, not worth blocking on. The robobun build failures on commit 79eb6c4 are blanket build-zig/build-cpp infra failures across every platform from the main merge, unrelated to these test-only edits. Each change is well-commented with the underlying root cause, which makes the intent easy to verify.
… is unreachable there Winsock's non-blocking send() never returns a partial write: it either copies the entire buffer into the AFD queue (which grows to fit) and returns the full length, or returns WSAEWOULDBLOCK only when the queue is already full from a *previous* send. tryEnd() is the first write on a fresh accepted socket, so it always reports success regardless of payload size and pending_flush is never created — the handleRejectStream path this test guards is POSIX-only by construction. Reverts CHUNK to 8 MiB since the size was never the issue.
208f832 to
943015a
Compare
There was a problem hiding this comment.
LGTM — test-only CI stabilization; my earlier nit is moot now that the fixture reverted to 8 MiB and the test is skipped on Windows instead.
Extended reasoning...
Overview
This PR touches five test files and no production code (the server.allocator → bun.default_allocator change referenced in the title already landed on main via #29916 and was merged into this branch). The net diff is:
serve-stream-reject-flush-leak-fixture.ts: whitespace-only reflow of aconsole.errorcall.serve-stream-reject-flush-leak.test.ts: wrap intest.skipIf(isWindows)with a detailed comment explaining whypending_flushis unreachable on Winsock.test-integration-rspack.ts: pinrsbuild@latest→rsbuild@1to avoid a double-mimalloc segfault on Windows arm64.websocket-server.test.ts: stop truncating stderr to 3 lines so panic output is visible in failure diffs.html-rewriter-leak.test.ts: rework the RSS measurement to GC per-pass and compare plateau RSS, with empirical before/after data in the description showing it both passes on fixed builds and still fails on pre-fix builds.
Security risks
None. All changes are confined to the test suite — no auth, crypto, parsing, or runtime code paths are touched. The rsbuild pin narrows rather than widens the dependency surface.
Level of scrutiny
Low. These are CI flake/stability fixes for pre-existing failures. The most substantive change is the html-rewriter-leak rewrite, but the PR description includes a 4-row matrix demonstrating the new methodology yields ~0 MB on fixed builds and ~68 MB on the pre-fix build, with the 25 MB threshold sitting comfortably between. Skipping the flush-leak test on Windows is acceptable since the leak is in platform-agnostic Zig and POSIX lanes still exercise it.
Other factors
My previous inline nit (stale "8 MiB" comment after a 64 MiB bump) has been resolved: commit 943015a dropped the 64 MiB approach in favor of skipIf(isWindows), so CHUNK is back at 8 MiB and the oneRequest() comment is accurate again. That inline thread is marked resolved. No outstanding human reviewer comments. The bug-hunting system found no issues.
…ven-sh#29926) Clears four pre-existing failures showing up across open PRs. ## `server.allocator` → `bun.default_allocator` `MimallocArena.getThreadLocalDefault()` and `bun.default_allocator` both call mimalloc, but their vtables differ. Collections that flow between server-owned and default-owned code (e.g. `BabyList` in the response sink, the `onUpgrade` path) trip `CheckedAllocator.assertEq` in `ci_assert` builds: ``` allocator mismatch: cannot use multiple allocators with the same collection panic(main thread): Internal assertion failure: allocators do not match ``` This was crashing both `serve-stream-reject-flush-leak.test.ts` and the websocket upgrade-UAF test on Windows release lanes. Same change as oven-sh#29916 minus the X509/BIO hunks that already landed via oven-sh#29881. ## `html-rewriter-leak.test.ts` — RSS path never worked on release The test was added in oven-sh#29879 and **failed on every release platform in that PR's own CI** (113–233 MB across darwin/linux/windows/asan, all on the *fixed* build). Only the debug path (precise mimalloc counters) passed. Root cause: a single trailing `Bun.gc(true)` does collect all 16k rewriters, but neither mimalloc nor ASAN's allocator promptly return freed pages to the OS, so RSS pins at the *peak* live set rather than what's retained. Now GCs every 1k iterations to bound peak ≈ retained. | build | before | after | |---|---|---| | release post-fix | 148 MB ❌ | 0.0 MB ✅ | | asan post-fix | 233 MB ❌ | 0.6 MB ✅ | | release **pre**-fix | — | 68.5 MB ❌ (still detects the leak) | | debug post-fix | ✅ | ✅ | ## `serve-stream-reject-flush-leak.test.ts` — skip on Windows Once the allocator panic is fixed, the test next fails on Windows with `insufficient backpressure: 0/40` (oven-sh#29916 CI) — the 8 MiB `tryEnd()` fits in Windows' auto-tuned loopback send buffer so `pending_flush` is never created. The leak is platform-agnostic Zig; POSIX coverage is sufficient. ## `websocket-server.test.ts` — un-truncate stderr The upgrade-UAF test's `stderr.split("\\n", 3)` hid the panic line, leaving `{stdout:"", stderr:""}` and a misleading diff. ## `test-integration-rspack.ts` — pin to `rsbuild@1` `create-rsbuild@2.0.0` (Apr 22) pulls `@rspack/binding-win32-arm64-msvc@2.0.x` which bundles mimalloc v3. Two static mimalloc instances → deterministic segfault in ntdll during `ExitProcess` on Windows arm64 (crash addr ends in `b9c8` across 5 builds). The test exists to guard the napi TSFN finalizer, not rsbuild HEAD. Supersedes oven-sh#29916.
2 participants
Clears four pre-existing failures showing up across open PRs.
server.allocator→bun.default_allocatorMimallocArena.getThreadLocalDefault()andbun.default_allocatorboth call mimalloc, but their vtables differ. Collections that flow between server-owned and default-owned code (e.g.BabyListin the response sink, theonUpgradepath) tripCheckedAllocator.assertEqinci_assertbuilds:This was crashing both
serve-stream-reject-flush-leak.test.tsand the websocket upgrade-UAF test on Windows release lanes. Same change as #29916 minus the X509/BIO hunks that already landed via #29881.html-rewriter-leak.test.ts— RSS path never worked on releaseThe test was added in #29879 and failed on every release platform in that PR's own CI (113–233 MB across darwin/linux/windows/asan, all on the fixed build). Only the debug path (precise mimalloc counters) passed.
Root cause: a single trailing
Bun.gc(true)does collect all 16k rewriters, but neither mimalloc nor ASAN's allocator promptly return freed pages to the OS, so RSS pins at the peak live set rather than what's retained. Now GCs every 1k iterations to bound peak ≈ retained.serve-stream-reject-flush-leak.test.ts— skip on WindowsOnce the allocator panic is fixed, the test next fails on Windows with
insufficient backpressure: 0/40(#29916 CI) — the 8 MiBtryEnd()fits in Windows' auto-tuned loopback send buffer sopending_flushis never created. The leak is platform-agnostic Zig; POSIX coverage is sufficient.websocket-server.test.ts— un-truncate stderrThe upgrade-UAF test's
stderr.split("\\n", 3)hid the panic line, leaving{stdout:"", stderr:""}and a misleading diff.test-integration-rspack.ts— pin torsbuild@1create-rsbuild@2.0.0(Apr 22) pulls@rspack/binding-win32-arm64-msvc@2.0.xwhich bundles mimalloc v3. Two static mimalloc instances → deterministic segfault in ntdll duringExitProcesson Windows arm64 (crash addr ends inb9c8across 5 builds). The test exists to guard the napi TSFN finalizer, not rsbuild HEAD.Supersedes #29916.