mimalloc: one heap delete/destroy teardown protocol (oven-sh/mimalloc#27) - #40138
Conversation
) Bumps to oven-sh/mimalloc claude/heap-lifecycle, which combines mimalloc #22-#26 and replaces their per-reader fixes with a single teardown for mi_heap_delete/mi_heap_destroy that pins a page before claiming it, plus targeted upstream dev3 fixes. No-Verification-Needed: dependency pin bump; verified by the mimalloc test suite (release/debug/ASAN/TSAN) and bun debug tests locally
|
Warning Review limit reached
On-demand reviews are free for the next 28 days. After that, they cost $0.25 per reviewed file. Or wait 33 minutes for your next included review. View limit detailsLimit details: You’ve used the included review currently available. Your 69 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Updated 4:56 PM PT - Aug 23rd, 2026
❌ @Jarred-Sumner, your commit ac16c93 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 40138That installs a local version of the PR into your bun-40138 --bun |
There was a problem hiding this comment.
Thanks — commit 1233bd83 addresses the stale process.versions.mimalloc assertion I flagged earlier; a repo-wide grep for the old hash now returns nothing. The Bun-side diff is correct as-is. Deferring to a human for sign-off since the substance here is the allocator itself (new mi_heap_delete/mi_heap_destroy teardown protocol in oven-sh/mimalloc#27), which lives outside this diff and is the global malloc replacement on Linux.
What was reviewed:
MIMALLOC_COMMITbump and matchingexpectedVersions.mimalloctest entry — hashes agree.- Grepped the repo for the old pin
6a14aee2…— no remaining references in build scripts, CI configs, or tests. - No other build knobs in
scripts/build/deps/mimalloc.tschanged;versionMacroplumbing toprocess.versionsunchanged.
Extended reasoning...
Overview
Two one-line changes on the Bun side: MIMALLOC_COMMIT in scripts/build/deps/mimalloc.ts bumped to e19d8710…, and the deliberate assertion table in test/js/node/process/process.test.js (process.versions test) updated to the same hash. My earlier inline comment flagged the test as stale on the first commit (12236043); the follow-up commit 1233bd83 fixed it, and a full-repo grep confirms no other references to the old hash remain.
Security risks
None introduced by the two Bun-repo lines. The pinned commit is fetched from the same oven-sh/mimalloc fork via the existing github-archive source mechanism — no new source, repo, or build flags. The allocator changes themselves (concurrent heap teardown, THP opt-out gating, checked free-list walks) are memory-safety-relevant, but they live in the mimalloc PR and are not reviewable from this diff.
Level of scrutiny
High. Although the visible diff is a mechanical version pin + test-table sync, mimalloc is Bun's process-wide malloc replacement on Linux and the JS heap allocator everywhere. The bump pulls in a redesigned cross-thread heap teardown protocol plus several upstream fixes. Correctness here depends entirely on oven-sh/mimalloc#27, which a maintainer should sign off on; the PR description's ctest/TSAN/RSS numbers look reasonable but are the author's own claims.
Other factors
- The previously flagged issue is resolved; nothing outstanding on the Bun-side diff.
- No changes to
mimalloc.tsbuild config (defines, cflags, override logic) beyond the commit constant — the surroundingMI_DEFAULT_ALLOW_THP,MI_DEBUG, override, and TLS-model logic are untouched. - REVIEW.md "Dependencies & vendoring" applies: allocator bumps warrant a human look regardless of how small the pinning diff is.
There was a problem hiding this comment.
I re-reviewed after the fix commits — the Bun-repo side is now consistent (the MIMALLOC_COMMIT pin and the process.versions assertion both read d9a4ae0..., and no stale hashes remain anywhere in the tree). No further issues found in this diff. Since the substance of the change is the allocator's heap-teardown concurrency protocol in the oven-sh/mimalloc fork rather than anything visible here, a human sign-off on the mimalloc#27 side is still worthwhile.
What was reviewed:
- Confirmed
scripts/build/deps/mimalloc.tsandprocess.test.js:579agree ond9a4ae0bcb86...; grepped the repo for the prior pins (6a14aee...,e19d8710...) — no hits. - Checked that no build flags/defines in
mimalloc.tschanged alongside the pin bump — only the commit string moved.
Extended reasoning...
Overview
This PR bumps the vendored mimalloc pin in scripts/build/deps/mimalloc.ts and updates the matching expectedVersions.mimalloc assertion in test/js/node/process/process.test.js. The Bun-repo diff is two one-line hash changes. The actual behavioral change lives in oven-sh/mimalloc#27 — a rework of the mi_heap_delete/mi_heap_destroy teardown protocol plus several smaller fixes (THP opt-out gating, checked free-list walks, upstream dev3 backports).
My earlier review flagged that the process.versions test still carried the old hash; that was fixed, and a subsequent commit (00504ca7) bumped the pin once more for an idle_start double-park fix. Both files now agree on d9a4ae0bcb86178f04dba166286b3ee3a678c8bb, and a repo-wide grep confirms no stale references to either the original (6a14aee...) or intermediate (e19d8710...) hashes remain.
Security risks
None introduced by the Bun-repo diff itself. The mimalloc-side changes touch cross-thread heap teardown and free-list walking — memory-safety-critical, but that code is not in this diff and cannot be reviewed here.
Level of scrutiny
The Bun-repo diff is mechanical and verifiably self-consistent. The correct level of scrutiny for the change as a whole, however, is high: mimalloc is Bun's global malloc on Linux and the JS-heap allocator everywhere, and the bump rewrites a concurrency protocol in the allocator's teardown path. Per the repo's Dependencies & vendoring guidance, dep bumps that carry substantive upstream changes need a maintainer to assess the vendored code itself, not just the pin string.
Other factors
The PR description is thorough (mimalloc ctest across Release/Debug/ASAN/TSAN, Bun test-suite runs, RSS measurements with overlapping ranges), and the author already responded to and resolved the earlier test-assertion issue. Nothing on the Bun side blocks this; deferring solely because the load-bearing change is allocator concurrency code outside this diff.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs — the earlier missing process.versions test update is fixed, and both MIMALLOC_COMMIT and the expectedVersions.mimalloc assertion now agree on 4777fa80... with no stale references to the old pins left in the repo. The Bun-side diff is two matching hash literals, but the substance is a heap-teardown concurrency rework in the global allocator (oven-sh/mimalloc#27), which lives outside this diff and is memory-safety-critical, so a human sign-off on the vendored change is still worthwhile.
What was reviewed:
- Confirmed
scripts/build/deps/mimalloc.tsandtest/js/node/process/process.test.jscarry the same commit hash. - Grepped repo-wide for the old pin (
6a14aee2...) and the intermediate pin (e19d8710...) — no remaining hits. - No other consumers of
MIMALLOC_COMMITor the mimalloc build config were touched.
Extended reasoning...
Overview
This PR bumps the pinned mimalloc commit in scripts/build/deps/mimalloc.ts from 6a14aee2... to 4777fa80... and updates the matching expectedVersions.mimalloc literal in test/js/node/process/process.test.js. The Bun-side diff is exactly two lines. The actual behavioral change is in the vendored dependency (oven-sh/mimalloc#27): a unified teardown protocol for mi_heap_delete/mi_heap_destroy addressing cross-thread free / stale-theap races, plus a THP opt-out tweak, checked free-list walks, and selected upstream fixes.
An earlier run of this review flagged that the process.versions test hadn't been updated; that was fixed and the pin was subsequently bumped twice more (per the branch's commit log), with the test kept in sync each time. A repo-wide grep confirms neither the original nor intermediate hashes remain anywhere.
Security risks
None introduced by the Bun-side diff itself (two hash literals). The vendored change, however, is the process-wide malloc replacement on Linux and the per-arena allocator everywhere — a concurrency bug in the new teardown protocol would be a memory-safety issue (UAF, free-list corruption) affecting arbitrary code paths. The PR description reports mimalloc's own ctest passing under Release/Debug/ASAN/TSAN and no RSS regression, which is reassuring, but the protocol change itself is not inspectable from this repo.
Level of scrutiny
High. Per the repo's dependencies-and-vendoring guidance, allocator bumps are not routine version bumps: mimalloc backs every allocation in the runtime, and the described change is specifically a rewrite of the concurrent heap-destroy path. Even though the Bun diff is trivial, the decision to accept this pin is a judgment call about the vendored code's correctness that a maintainer should make (and likely already made in oven-sh/mimalloc#27, but that review is external to this PR).
Other factors
- The prior review feedback (test assertion out of sync) is resolved and the thread is marked resolved.
- No build-config changes in
mimalloc.tsbeyond the commit hash — defines, cflags, TLS model, and override gating are all unchanged. - The PR description is thorough (test matrix, RSS measurements with ranges), which lowers risk, but does not substitute for a maintainer having reviewed the mimalloc-side diff.
There was a problem hiding this comment.
I re-reviewed after the follow-up commits — the Bun-side diff is now consistent (MIMALLOC_COMMIT and the process.versions assertion both at 36ddf78c…, no stale references to the old or intermediate hashes anywhere in the repo). No bugs found in this diff. Given that the pin pulls in a rewrite of mimalloc's heap delete/destroy teardown protocol plus the scavenger start-on-first-use change — the process-wide allocator — a human sign-off on shipping that behavior is still worthwhile; the substantive review lives in oven-sh/mimalloc#27.
Extended reasoning...
Overview
Two one-line changes on the Bun side: scripts/build/deps/mimalloc.ts bumps MIMALLOC_COMMIT from 6a14aee2… to 36ddf78c…, and test/js/node/process/process.test.js updates the expectedVersions.mimalloc assertion to match. The real payload is the referenced oven-sh/mimalloc#27 (heap teardown protocol rework, scavenger lazy start, THP opt-out gating, checked free-list walks, several upstream dev3 fixes), which this PR ships into Bun's global allocator.
What was checked
The earlier finding about the stale process.versions test hash is resolved — both sites now read 36ddf78cbc8bb6d397e32b3ebfe676147453e0f0, and a repo-wide grep confirms no lingering references to 6a14aee2… or the intermediate e19d8710… pin. The build config in mimalloc.ts (defines, cflags, TLS model, override gating) is unchanged, so nothing there needs re-verification against the new source. The PR description notes the scavenger no longer blocks fault signals; I checked that the existing seteuid under GC pressure does not deadlock test (which was originally added around the scavenger's signal handling) is still present and unchanged.
Security risks
None introduced by the Bun-repo diff itself. The allocator changes are memory-safety-adjacent by nature (concurrent heap teardown, page pinning, ABA on theap reuse), but that is what oven-sh/mimalloc#27 addresses and where its review/tests live — not something reviewable from this two-line pin bump.
Level of scrutiny
The Bun-repo diff is mechanical and verified consistent. The scrutiny that matters is on the mimalloc changes, which affect every allocation in the process and change scavenger-thread startup timing. That warrants a maintainer's sign-off rather than an auto-approval based on the pin bump alone.
Other factors
The PR carries an A/B RSS + syscall table and mimalloc ctest / Bun test-suite results, and CI (#103876) is building the current head. My earlier inline comment on this PR is resolved; deferring here is about the nature of the dependency change, not any outstanding issue in the diff.
There was a problem hiding this comment.
I re-reviewed after the pin was bumped further — the Bun-side change is now consistent (MIMALLOC_COMMIT and the process.versions assertion both at 1c68ff9…, no stale hashes left in the tree), and my earlier note is resolved. Deferring to a human for sign-off since the substance here is the allocator itself: a rewritten heap-delete/destroy teardown protocol plus the scavenger-thread lifecycle change, which this repo's diff can't show.
What was reviewed
- Both changed lines carry the same commit hash; grepped the repo for the old and intermediate pins — no remaining references.
mimalloc.tsbuild config (defines, cflags, override gating) is unchanged apart from the pin.- Confirmed
versionMacro: "MIMALLOC"still feedsprocess.versions.mimallocfrom thesource().commit, so the test assertion covers the right value.
Extended reasoning...
Overview
Two-line change in this repo: MIMALLOC_COMMIT in scripts/build/deps/mimalloc.ts bumped to 1c68ff911a8527efb5652de8a3032c9a89f4a311, and the matching literal in the process.versions assertion table in test/js/node/process/process.test.js. My earlier inline comment (test hash out of sync) was addressed; since then the pin was bumped four more times and both sites now agree on the final hash. A repo-wide grep for the previous (6a14aee…) and intermediate (e19d8710…) hashes returns nothing.
Security risks
None introduced by the Bun-side diff — it is two hash literals. The pulled-in mimalloc changes touch memory-safety-relevant paths (heap teardown under concurrent cross-thread mi_free, scavenger thread start/exit handshake, checked free-list walks), but those live in oven-sh/mimalloc#27 and are not reviewable from this diff.
Level of scrutiny
High. mimalloc is Bun's global malloc replacement on Linux and the JS-heap allocator everywhere; the bump rewrites the mi_heap_delete/mi_heap_destroy teardown protocol and changes when the scavenger thread starts. That is squarely "touches critical code paths" even though the in-repo diff is mechanical. Per the repo's dependencies-&-vendoring guidance, allocator bumps warrant a human look at the upstream PR and CI results across the full platform matrix.
Other factors
The PR description is thorough (mimalloc ctest across Release/Debug/ASAN/TSAN, Bun test-suite runs, interleaved n=7 RSS/syscall A/B). The Bun-side change is mechanically correct and self-consistent. I'm deferring rather than approving because the actual behavior change — allocator teardown semantics and scavenger lifecycle — is outside what this two-line diff shows, and a wrong step there manifests as rare UAF/corruption rather than a test failure.
There was a problem hiding this comment.
I re-reviewed after the fix — the process.versions test and MIMALLOC_COMMIT are now both on 680b2379…, and a repo-wide grep confirms no stale references to the old pins remain. The Bun-side diff is mechanically correct.
That said, the substance of this PR lives in oven-sh/mimalloc#27 (concurrent heap-teardown protocol, scavenger startup timing, free-list walk changes), which this automated pass cannot review. Given it's the global allocator, a human sign-off on the mimalloc-side changes is still warranted.
Extended reasoning...
Overview
Two-line change in the Bun repo: bumps MIMALLOC_COMMIT in scripts/build/deps/mimalloc.ts and the matching expectedVersions.mimalloc assertion in test/js/node/process/process.test.js to 680b2379b8abb5bd62e0a37ab83dc458c1149a0b. My earlier finding (test hash not updated alongside the build script) has been addressed and the thread is resolved; a fresh grep for both prior pins (6a14aee2…, e19d8710…) returns no hits anywhere in the tree, and the new hash appears in exactly the two expected places.
Security risks
None introduced by the Bun-repo diff itself (a commit-hash constant + a test literal). The referenced mimalloc changes are memory-safety-adjacent (cross-thread mi_free vs. mi_heap_destroy races, stale-theap ABA, checked free-list walks) rather than security boundaries per se, but allocator correctness bugs are effectively memory-safety bugs for every caller.
Level of scrutiny
High. Per the repo's Dependencies & vendoring guidance, a bump to the process-wide allocator — with a rewritten concurrent teardown protocol and changed scavenger-thread lifecycle — is production-critical. The Bun-side diff is trivially verifiable, but the actual code under review is in oven-sh/mimalloc#27 and is not part of this diff; I cannot audit it from here. The PR description's verification (mimalloc ctest Release/Debug/ASAN/TSAN, Bun test suites, RSS A/B) is thorough, but that is evidence for a human to weigh, not a substitute for reviewing the allocator source.
Other factors
The recent commit history on this branch shows five successive re-pins (idle_start double-park fix, scavenger exit handshake, scavenger-starts-on-first-use, TSAN budget, etc.), which suggests the mimalloc-side changes went through non-trivial iteration. That's normal for allocator work but reinforces that a maintainer should confirm the final oven-sh/mimalloc#27 state before this lands.
There was a problem hiding this comment.
The Bun-side diff is now internally consistent — MIMALLOC_COMMIT and the process.versions test assertion both point at 6c2c244f..., and a repo-wide grep confirms no stale references to the old hashes remain (my earlier concern is resolved). I'm deferring to a human because the substance here is the allocator teardown-protocol rework in oven-sh/mimalloc#27, not the two-line pin bump; that change to the global allocator's cross-thread heap-destroy semantics and scavenger startup timing warrants a maintainer's eyes on the full CI matrix.
Extended reasoning...
Overview
This PR bumps the vendored mimalloc pin in scripts/build/deps/mimalloc.ts and updates the matching expectedVersions.mimalloc assertion in test/js/node/process/process.test.js. Both now reference 6c2c244fc4ae7a206b75127afa11180a57f678ab. The Bun-repo diff is two lines; the actual behavioral change lives in oven-sh/mimalloc#27, which reworks the mi_heap_delete/mi_heap_destroy teardown protocol (detach theaps → abandon pages → pin-then-claim → free), changes when the scavenger thread starts (on first use rather than at process init), adds THP opt-out gating on the kernel setting, and pulls in several upstream dev3 fixes.
Security risks
None introduced by the Bun-side diff. The underlying allocator change touches concurrency-sensitive paths (cross-thread mi_free racing heap teardown, stale-theap ABA), where a defect would manifest as memory corruption rather than a security boundary bypass per se — but allocator UAF is effectively a security concern.
Level of scrutiny
High. mimalloc is Bun's global malloc replacement on Linux and the JS heap allocator everywhere. A teardown-protocol change interacts with every thread that allocates, and the scavenger-startup timing change affects process init on all platforms (the description notes it was made to fix a macOS DYLD_INSERT abort). The Bun-side pin bump is mechanical and correct, but approving it is approving the allocator change it pulls in — that is not something an automated review can meaningfully vet from a commit hash.
Other factors
My previous comment flagged that the process.versions test still had the old hash; that has been fixed and the thread resolved. I grepped for both prior hashes (6a14aee2... and e19d8710...) — zero remaining references. The PR description includes ctest results (Release/Debug/ASAN/TSAN) and Bun test-suite runs plus RSS/syscall A/B numbers, which is thorough, but the full BuildKite matrix (Windows, macOS, musl, ARM64) is what actually gates this and should be checked by a human before merge given the recent-commit history shows five successive pin bumps iterating on CI issues.
…ad of repairing it No-Verification-Needed: dependency pin bump; targeted worker/process/heap-snapshot suites run locally in release
There was a problem hiding this comment.
I reviewed this PR and found no issues in the Bun-side diff — the pin in scripts/build/deps/mimalloc.ts and the process.versions assertion in process.test.js now agree at 94ce608..., and the earlier stale-hash comment is resolved. Because the actual change is a rewrite of mimalloc's heap-teardown concurrency protocol plus scavenger-thread lifecycle changes in the vendored allocator (code not visible in this repo's diff), a human look and full cross-platform/sanitizer CI sign-off is still warranted before merge.
Checked: the two hashes match; grepped the repo for the old pins (6a14aee..., e19d8710...) — no remaining references.
Extended reasoning...
Overview
Two-line change in this repo: bumps MIMALLOC_COMMIT in scripts/build/deps/mimalloc.ts to 94ce60895dea301782a37e999963e9e72c8ad9e7 and updates the matching literal in the process.versions assertion table in test/js/node/process/process.test.js. Both are consistent, and a repo-wide grep confirms no stale references to either the original (6a14aee...) or intermediate (e19d8710...) hashes remain. The earlier inline finding about the stale test assertion has been addressed.
Security risks
None introduced by the Bun-repo diff itself (a commit-hash constant and a test literal). The vendored allocator changes touch concurrent memory management (heap teardown vs. cross-thread mi_free, page pinning, ABA on stale theaps), which is memory-safety-critical — but that code lives in oven-sh/mimalloc#27 and cannot be reviewed from this diff.
Level of scrutiny
High. mimalloc is Bun's global malloc replacement on Linux and the JS-heap allocator everywhere; the PR description describes a rewritten teardown protocol, scavenger-thread startup timing changes, and cherry-picked upstream concurrency fixes. Even with the thorough verification table in the description (ctest under Release/Debug/ASAN/TSAN, bundler/transpiler suites, RSS A/B), an allocator concurrency change is the definition of a critical code path where a human should confirm CI is green across the full platform × sanitizer matrix before merge.
Other factors
The Bun-side diff is trivially correct and self-consistent, so there is nothing further to flag here. Deferring solely because the substance of the change — the allocator rewrite in the vendored dependency — is outside what an automated diff review of this repo can validate, and "a human does not need to look" is not the right signal for an allocator teardown-protocol rewrite.
There was a problem hiding this comment.
I reviewed this PR and found no issues with the Bun-side diff — the pin and the process.versions assertion are now in sync at 94ce60895dea…, and no stale references to the old hash remain. That said, the substance of the change lives in oven-sh/mimalloc#27 (heap teardown protocol, lazy scavenger start, THP gating), which is the process-wide allocator; a human sign-off on the vendored allocator changes themselves is still worthwhile.
Checked: MIMALLOC_COMMIT and expectedVersions.mimalloc match; repo-wide grep for the previous pins (6a14aee…, e19d8710…) returns nothing; mimalloc.ts build config (defines/cflags) is unchanged.
Extended reasoning...
Overview
Two-line change on the Bun side: bumps MIMALLOC_COMMIT in scripts/build/deps/mimalloc.ts from 6a14aee2… to 94ce60895dea…, and updates the matching literal in the process.versions assertion table in test/js/node/process/process.test.js. The earlier bot finding (stale test hash) was addressed and the thread is resolved; a repo-wide grep confirms no remaining references to either the old pin or the intermediate e19d8710… pin.
Security risks
No direct security-sensitive code paths are touched in this repo. The risk surface is entirely in the vendored dependency: mimalloc is Bun's global malloc replacement on Linux and the JS-heap allocator everywhere, so a bug in the new teardown protocol or lazy-scavenger start would manifest as UAF/heap corruption process-wide. That code is not in this diff — it lives in oven-sh/mimalloc#27.
Level of scrutiny
The Bun-repo diff is mechanical and correct. But per the Dependencies & vendoring guidance, an allocator bump that rewrites the concurrent mi_heap_delete/mi_heap_destroy protocol, changes scavenger-thread lifecycle, and alters THP opt-out behavior is not something to auto-approve on a hash match alone. The PR description's verification (ctest across Release/Debug/ASAN/TSAN, A/B RSS and syscall measurements, targeted bun bd test suites) is thorough, but a maintainer who owns the mimalloc fork should confirm the upstream PR and its test coverage before landing.
Other factors
My prior review's only finding is resolved. The mimalloc.ts build config (defines, cflags, override gating, TLS model) is untouched — only the commit constant changed. No CODEOWNERS-gated files are involved on the Bun side. Deferring so a human can sign off on the allocator-side changes rather than the two-line pin bump.
…r lazy start and signal mask) No-Verification-Needed: dependency pin bump; the mimalloc diff is test-only
There was a problem hiding this comment.
Thanks — the process.versions assertion now matches the new pin (6b4a39ec… in both scripts/build/deps/mimalloc.ts and process.test.js), and a repo-wide grep finds no other references to the old hash. I didn't find any issues in the Bun-side diff.
Deferring to a human for the merge decision: the two-line change here is mechanical, but it pulls in an allocator-level rework (heap delete/destroy teardown protocol, lazy scavenger start, THP/signal-mask changes) that affects every allocation path. That's the kind of vendored-dep bump a maintainer should sign off on directly.
Extended reasoning...
Overview
The Bun-repo diff is exactly two lines: MIMALLOC_COMMIT in scripts/build/deps/mimalloc.ts bumped from 6a14aee2… to 6b4a39ec…, and the matching literal in the process.versions assertion table in test/js/node/process/process.test.js. All substantive change lives in oven-sh/mimalloc#27 (heap delete/destroy teardown protocol, lazy scavenger thread, THP opt-out gating, scavenger signal mask, targeted upstream dev3 fixes).
My earlier inline comment flagged the test assertion being stale; the author addressed it and the thread is resolved. Repo-wide grep confirms the old hash (6a14aee2…) and the intermediate hash from my comment (e19d8710…) appear nowhere, and the new hash appears only in the two expected files — no other pinned duplicates (Dockerfiles, CI configs, etc.) to update.
Security risks
None introduced by the Bun-side diff itself. The vendored mimalloc changes are memory-safety-adjacent (concurrent heap teardown, free-list validation), but that code is in the fork PR, not reviewable from this diff.
Level of scrutiny
The in-repo edit is trivial and correct. The effect is a bump of Bun's global malloc replacement (Linux) and JS-heap allocator (everywhere) — as production-critical as a dependency gets. Per the repo's Dependencies & vendoring guidance and the approval guidelines around critical code paths, this should carry a human sign-off rather than a bot approval, even though nothing in the two-line diff is wrong.
Other factors
- The PR description includes ctest results across Release/Debug/ASAN/TSAN and A/B RSS/syscall measurements, and Bun's own test suites were run against the new pin — good signal, but verification of the allocator behavior itself belongs to the mimalloc PR review and CI.
- No outstanding unresolved reviewer comments; the one I raised is resolved.
- No CODEOWNERS concerns for the two touched files that I'm aware of.
No-Verification-Needed: dependency pin bump
There was a problem hiding this comment.
I re-reviewed after the fix — the Bun-side diff is now internally consistent (both MIMALLOC_COMMIT and the process.versions assertion are 8e7d7d49…, and no stale hash remains in the tree). No issues found in this repo's changes. I'm not auto-approving because the substance of the bump — the heap-teardown protocol rewrite and scavenger changes — lives in oven-sh/mimalloc#27 and swaps the process-wide allocator, so a human sign-off on the mimalloc-side diff is still worthwhile.
Extended reasoning...
Overview
Two-line change in this repo: bumps MIMALLOC_COMMIT in scripts/build/deps/mimalloc.ts and updates the matching expectedVersions.mimalloc assertion in test/js/node/process/process.test.js. The actual behavioral change is entirely in the pinned oven-sh/mimalloc commit (heap delete/destroy teardown protocol, lazy scavenger start, THP opt-out gating, scavenger signal mask, and cherry-picked upstream fixes).
Security risks
None introduced by the Bun-repo diff itself. The pulled-in code, however, is the global malloc/free replacement on Linux and the JS-heap allocator everywhere — memory-safety-critical by definition. A regression there manifests as UAF, double-free, or heap corruption across the whole process. That risk lives in the mimalloc fork PR, not in the two lines changed here.
Level of scrutiny
The Bun-side edit is mechanical and I've confirmed both hash references agree and no stale copies of the old (6a14aee…) or intermediate (e19d8710…) hash remain in the tree. But per the repo's dependency-bump guidance and the "touches critical code paths" bar, the allocator itself warrants human review of oven-sh/mimalloc#27. I can't inspect that diff from this checkout, so I'm deferring rather than approving on the strength of the pin change alone.
Other factors
My earlier finding (test assertion not updated) is resolved — the current diff updates both sites atomically. The PR description includes ctest results across Release/Debug/ASAN/TSAN and Bun-side test runs plus RSS/syscall A/B measurements, which is good supporting evidence. CI (#104064) is still building at the time of this review.
No-Verification-Needed: dependency pin bump to the merge commit; tree identical to the previous pin
There was a problem hiding this comment.
I reviewed the current revision and found no issues in the Bun-side diff — the MIMALLOC_COMMIT pin and the process.versions assertion table now both read a178e44af6dc2845c793a39fd62da583b3e9f0ff, and a repo-wide grep confirms no stray references to the old hashes remain. Since the substance of this change is the allocator teardown protocol in oven-sh/mimalloc#27 rather than these two lines, a human sign-off (and a green cross-platform CI run) is still the right gate here.
What was reviewed:
scripts/build/deps/mimalloc.tsandtest/js/node/process/process.test.js— pin and test assertion are consistent.- Grepped the repo for the old (
6a14aee…,e19d8710…) and new (a178e44…) hashes — only the two expected sites carry the new value; nothing stale. - No build-flag or define changes ride along in
mimalloc.ts; the bump is the only change.
Extended reasoning...
Overview
Two one-line changes on the Bun side: MIMALLOC_COMMIT in scripts/build/deps/mimalloc.ts is bumped to a178e44af6dc2845c793a39fd62da583b3e9f0ff, and the matching entry in the expectedVersions table in test/js/node/process/process.test.js is updated to the same hash. My earlier comment about the stale test assertion was addressed, and a subsequent push moved both sites to the final a178e44… pin in lockstep. A repo-wide grep for all three hashes involved (old 6a14aee…, intermediate e19d8710…, new a178e44…) returns exactly the two expected lines.
Security risks
None introduced by the Bun-side diff itself. The pinned source is fetched from the oven-sh/mimalloc fork by full commit hash, same as before.
Level of scrutiny
High. The two-line diff is mechanically trivial, but what it pulls in is a rework of mi_heap_delete/mi_heap_destroy teardown ordering plus scavenger-thread lifecycle changes in the process-wide allocator — concurrency- and memory-safety-critical code that affects every allocation in every build configuration. That work lives in oven-sh/mimalloc#27 and is not reviewable from this diff. Per the repo's Dependencies & vendoring guidance, allocator bumps warrant a maintainer look and a full green CI matrix rather than an automated approval on the pin change alone.
Other factors
The PR description is thorough (mimalloc ctest across Release/Debug/ASAN/TSAN, targeted bun bd test runs, and A/B RSS/syscall numbers), and the earlier review thread is resolved. Nothing outstanding on the Bun side; deferring solely because the load-bearing change is in the vendored allocator.
…33196) ### Problem - Three frames on `/_bun/hmr`, which any client that can reach the dev server may send, drove invalid state. A duplicate `H` (testing-batch) frame hit the `TestingBatchEvents::EnableAfterBundle` arm's `debug_assert!(false)`. A `SetUrl` pattern that does not start with `/` reached `FrameworkRouter::match_slow`'s `debug_assert!(path[0] == b'/')`: `ws.send("n")` indexes an empty slice inside that assert, `ws.send("nfoo")` fails it. - Releasing a batch with `H` while an unrelated bundle was in flight called `start_async_bundle`, whose first line is `debug_assert!(self.current_bundle.is_none())` (`DevServer.rs:3111`). On a release build that assert is compiled out, and the assignment drops the in-flight `CurrentBundle` (its `BundleV2`, and through `Drop for MimallocArena` an `mi_heap_destroy` of its arena) while that bundle's thread-pool tasks are still running. Since 1.4.1 that crashes on several threads at once: ``` panic: Segmentation fault at address 0x1B # canary 1.4.3, 3/3, addresses vary oh no: multiple threads are crashing panic: index out of bounds: the len is 7 but the index is 153309857 ``` ### Fix - Duplicate `H`: drop the assert and keep the `ws.close()` that was already there. - `SetUrl`: close the socket on a pattern that does not start with `/`, like the sibling arms do for their malformed input. `match_slow`'s assert is a correct contract for its HTTP callers, whose request paths always start with `/`; the HMR socket is the one caller feeding it peer bytes. - Batch release: hold the batch in a new `TestingBatchEvents::ReleaseAfterBundle` state and release it from `finalize_bundle_cleanup` once no bundle is running, so the harness still gets its bundle. A further `H` in that state is a protocol violation and closes the socket. - Verified: `test/bake/hmr-socket-protocol.test.ts`, 4 tests. All 4 fail against `bun bd` without the `src/` diff and pass with it. ### Background - `/_bun/hmr` is the dev server's hot-reload websocket. `HmrSocket::on_message` switches on the first byte of each frame, so each arm validates its own payload. - The `H` frame drives the bake test harness's batching: the first turns batching on, a later one releases the files collected since as a single bundle. - Only one bundle runs at a time. `CurrentBundle` owns the arena that the bundler's parse tasks, which run on the thread pool, read from. - A request for a route that is not bundled yet goes through `ensure_route_is_bundled`, which starts a bundle without consulting `testing_batch_events`. That is how a bundle comes to be in flight between two `H` frames. <details><summary>Notes</summary> Rebased onto current `main`. The branch was 1983 commits behind and the file had moved from `src/runtime/bake/DevServer/HmrSocket.rs` to `src/runtime/bake/dev_server/hmr_socket.rs`, so the PR had gone conflicting; it is now a clean diff against `main` and all three bugs were re-confirmed present there. An earlier revision of this PR also gated the visualizer on-subscribe hooks on `cfg!(feature = "bake_debugging_features")` to stop `sM` reaching `emit_memory_visualizer_message`'s `debug_assert!(cfg!(feature = ...))`. That fix is dropped: `main` has since deleted the assert and removed the cargo feature from `src/runtime/Cargo.toml` entirely, so there is nothing left to gate. While re-checking `sM` against current `main` I found a separate, still-open crash that this PR does not touch: `sM`, then ~1s so the 1-second timer fires, then `s` to unsubscribe gives `panic: assertion failed: self.root == v`, the intrusive timer heap's `remove()` on a node the drain had already popped. The cleanup that emptied `emit_memory_visualizer_message_timer` took away the `state = FIRED` + re-insert that kept the node's bookkeeping honest. It reproduces on unmodified `main` with this diff stashed, and fixing it needs a decision about what remains of the memory-visualizer feature, so it is filed separately rather than folded in here. The third test keeps every step condition-based rather than timed: a bundler plugin parks the `/two` bundle on a fetch the test controls, the batch steps wait on the `r0`/`r1` synchronization frames, and the release step waits for the socket close that a further `H` triggers, which is what proves the first `H` was handled while the bundle was still held. The duplicate-`H` and `SetUrl` arms were reported by review on an earlier revision of this PR; the batch-release segfault came with a runnable reproduction from the fuzz lane, and that reproduction now exits 0 with the server still answering, 3/3. Release dating, from the fuzz lane's runs of the same frames: 1.4.0 keeps serving, while 1.4.1, 1.4.2 and canary segfault 3/3 each. The dev-server side of this did not change in that window. `start_async_bundle` is identical between `bun-v1.4.0` and `bun-v1.4.1` apart from an unrelated inspector string `deref`, and `src/bun_alloc/MimallocArena.rs` is byte-identical, so 1.4.0 already destroys the in-flight bundle's arena and gets away with it: a use-after-free that happens to read intact bytes. Of the two changes first suspected, #40478 only touches `StaticRoute` response refs (neither `CurrentBundle` nor `BundleV2` has a refcounted field for it to affect), and #40640 moves out-of-root path text out of the per-bundle arena into a process-lifetime store, which leaves less dangling, not more. What did change on this path is the allocator: mimalloc moved from `6a14aee2` to `6a64e1ba` in that window, including #40138, which replaced the `mi_heap_delete` and `mi_heap_destroy` teardown with a protocol that detaches, claims and frees a heap's pages even when another thread can still reach them. That is the likeliest reason a silent use-after-free became a reliable fault. It is inferred from the diffs, not bisected; running the reproduction against one Bun commit with the old and the new mimalloc pin would settle it. Either way the defect is the second `start_async_bundle`, which is as old as the `Enabled` arm. 1.4.1 made it visible. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 10 · 5 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 4 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/bake/hmr-socket-protocol.test.ts bun test v1.4.3 (4ff9193) test/bake/hmr-socket-protocol.test.ts: 163 | try { 164 | pageStatus = String((await pageFetch).status); 165 | } catch (e) { 166 | pageStatus = `${(e as Error).message}\n--- dev server stderr ---\n${dev.stderr()}`; 167 | } 168 | expect(pageStatus).toBe("200"); ^ error: expect(received).toBe(expected) - "200" + "The socket connection was closed unexpectedly. For more information, pass `verbose: true` in the second argument to fetch() + --- dev server stderr --- + ============================================================ + Bun Debug v1.4.3 (4ff9193) Linux x64 + Linux Kernel v7.0.0 | glibc v2.41 + CPU: sse42 popcnt avx avx2 avx512 + Args: "/workspace/bun/build/debug/bun-debug" "server.ts" + Features: bunfig fetch http_server jsc dev_server + Builtins: "bun:main" + + + panic: assertion failed: false + + " - Expected - 1 + Received + 14 at <anonymous> (/workspace/bun/test/bake/hmr-socket-protocol.tes ... (truncated) release without fix: 3 FAILED bun test v1.4.3-canary.1 (4ff9193) test/bake/hmr-socket-protocol.test.ts: (pass) a duplicate testing-batch frame (H) during an in-flight bundle closes the socket instead of aborting [25.15ms] (fail) a SetUrl frame without a leading slash ("n") closes the socket instead of aborting [5000.12ms] ^ this test timed out after 5000ms. (fail) a SetUrl frame without a leading slash ("nfoo") closes the socket instead of aborting [5000.07ms] ^ this test timed out after 5000ms. (fail) releasing a testing batch while another bundle is in flight defers it [5000.06ms] ^ this test timed out after 5000ms. 1 pass 3 fail 3 expect() calls Ran 4 tests across 1 file. [5.09s] __F:3:S:0 ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/bake/hmr-socket-protocol.test.ts bun test v1.4.3 (4ff9193) test/bake/hmr-socket-protocol.test.ts: (pass) a SetUrl frame without a leading slash ("nfoo") closes the socket instead of aborting [587.07ms] (pass) a duplicate testing-batch frame (H) during an in-flight bundle closes the socket instead of aborting [819.24ms] (pass) a SetUrl frame without a leading slash ("n") closes the socket instead of aborting [745.62ms] (pass) releasing a testing batch while another bundle is in flight defers it [897.64ms] 4 pass 0 fail 8 expect() calls Ran 4 tests across 1 file. [4.13s] __F:0:S:0 release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 993ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/8] gen bake.{client,server,error}.js -> bake.client.js, bake.server.js, bake.error.js [2/6] gen generated_host_exports.rs generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 243 extern-C blocks audited [2/6] cargo bun_runtime → libbun_runtime.a ^[[1m^[[92m Compiling^[[0m bun_runtime v0.0.0 (/workspace/bun/src/runtime) ^[[1m^[[92m Finished^[[0m `release` profile [optimized + debuginfo] target(s) in 6m 25s [3/6] link bun-profile [5/6] strip bun [5/6] bun-profile --revision 1.4.3-canary.1+03f585b7d [build] done bun test v1.4.3-canary.1 (03f585b) test/bake/hmr-socket-protocol.test.ts: (pass) a SetUrl frame without a leading slash ("n") closes the socket instead of aborting [18.87ms] (pass) a SetUrl frame without a leading slash ("nfoo") closes the socket instead of aborting [16.56ms] (pass) a duplicate testing-batch frame (H) during an in-flight bundle closes the socket instead of aborting [28.81ms] (pass) releasing a testing batch while another bundle is in flight defers it [ ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/runtime/bake/DevServer.rs | 37 +++- src/runtime/bake/dev_server/hmr_socket.rs | 33 ++- src/runtime/bake/dev_server/memory_cost.rs | 2 +- src/runtime/bake/dev_server/mod.rs | 2 +- test/bake/hmr-socket-protocol.test.ts | 341 +++++++++++++++++++++++++++++ 5 files changed, 394 insertions(+), 21 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 10 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/runtime/bake/DevServer.rs 9 6 30 src/runtime/bake/dev_server/hmr_socket.rs 2 7 29 src/runtime/bake/dev_server/memory_cost.rs 1 2 29 src/runtime/bake/dev_server/mod.rs 3 1 29 test/bake/hmr-socket-protocol.test.ts 3 8 29 ``` </details> <!-- robobun:evidence:end -->
2 participants
What does this PR do?
Bumps mimalloc to oven-sh/mimalloc#27, which combines oven-sh/mimalloc#22–#26 and replaces their per-reader fixes with one teardown protocol for
mi_heap_delete/mi_heap_destroy.The problem those PRs were circling: a heap is torn down while a concurrent cross-thread
mi_free, or a thread that used the heap earlier and still caches a theap for it, can reach it. The hole in the middle was that the deleter claimed pages by writing to them (atomic_orof the owned bit) with nothing pinning the page, so a concurrent free could release the page and the slice be reused in between. Now: detach theaps → abandon their pages as thread-exit does → pin-then-claim every abandoned page (the same bitmap-as-pin protocol the abandoned-page map already uses) → free theaps → free heap. Details, contract and tests in the mimalloc PR.Also in the bump: mimalloc#22 (THP opt-out only when the system setting is
always— saves amadviseper mmap on Debian/Ubuntu defaults), from mimalloc#23 only the scavenger signal mask (a fault on that thread produced no crash report), the scavenger thread starting lazily (a single-threadedbun -eno longer spawns it: −1 thread, −24 syscalls), and targeted upstream dev3 fixes (thread-locals-after-free guard, #1364, #1371, NUMA node count). Upstream's in-progress page-meta layout rework is deliberately not included.What this does not do: fix the Windows corrupted-free-list crash family (BUN-40BH and siblings). Those lists are written by Bun — #39897 (file read completing into a freed buffer, merged) and #39643 (poll handle freed twice from a nested event loop, open) — and mimalloc is only where the damage surfaces. #23's "validate links and cut the list" is not taken for that reason: it would keep running past the write and hide it.
How did you verify your code works?
ctest: Release 23/23, Debug (MI_DEBUG_FULL) 24/24 (was 20/21 on the old pin), ASAN 22/22, TSAN 19/19 with 0 reports (see mimalloc#27).bun bd test: transpiler (190/190), bundler_edgecase (138/138), bundler_minify (43/43), css (2358 pass; 6 debug-timeout fuzz tests), workers/serve (same 4 failures as amaindebug build on this box).bun -e 1peak RSSbun -e 1syscallsclone3for the scavenger, −6rt_sigprocmask, −5madvise)Bun.servehello RSS after 200k req (c=64)bun build --minify --sourcemapthree.js×10 peak¹ one run at 17212 KB excluded from the range as an outlier. The first two rows come from the scavenger thread now starting on first use (first park / first scheduled purge) instead of at process init — a change made because the eager start aborted macOS processes that
DYLD_INSERTthe dylib (thread created before libobjc initializes);bun -e 1never needs it. Before that change the same A/B was flat (+0.1–0.3%, overlapping), so the teardown protocol itself is RSS-neutral as forecast.