GarbageCollectionController: replace per-tick heap sampler with idle timer only - #35356
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:
WalkthroughChangesThe pull request centralizes dotenv truthiness, adds configurable and adaptive GC growth thresholds, updates GC timer transitions, and adds GC cadence and MySQL regression test coverage. Garbage collection runtime
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
|
Updated 8:49 PM PT - Jul 29th, 2026
@Jarred-Sumner, your commit c588b56 is building: |
|
Found 5 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
021ba29 to
09390bc
Compare
CI status at 9323008Remaining
The cadence test itself is now |
9323008 to
0fd4f8e
Compare
Fail-before proof (for manual verification)The cadence test is Manual fail-before against any release ≤ 1.3.14: With the fix (release build at HEAD): The automated fail-before check's release-without-fix step is running the binary the previous iteration's with-fix step built (header shows |
0fd4f8e to
87ee43a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/js/bun/gc/gc-controller-cadence.test.ts`:
- Around line 50-55: Update the cadence helper that collects stdout, stderr, and
exitCode to return exitCode alongside eden and full instead of asserting it
internally. In each test caller, assert the GC cadence/count invariant first,
then assert the returned exitCode is 0.
- Around line 71-87: Extend the GC controller tests around countEdenCollections
to cover non-default BUN_GC_TIMER_INTERVAL and
BUN_GC_RUNS_UNTIL_SKIP_RELEASE_ACCESS values deterministically, preserving
assertions that prove each setting is applied. Parameterize BUN_GC_TIMER_DISABLE
coverage for the requested falsy representations ("false", empty, and
quoted-empty values) alongside existing "1"/"0" cases, and assert the controller
remains enabled for each falsy input while retaining the disabled behavior for
truthy input.
In `@test/regression/issue/28632.test.ts`:
- Around line 57-62: Remove the multi-line explanatory comment above the query
loop in the regression test, leaving the test logic unchanged. Ensure any
remaining regression-test comment contains only the issue URL.
🪄 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: 783a3991-cde8-42f5-b1ad-fcc3f2c492f6
📒 Files selected for processing (4)
src/dotenv/env_loader.rssrc/jsc/GarbageCollectionController.rstest/js/bun/gc/gc-controller-cadence.test.tstest/regression/issue/28632.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/jsc/GarbageCollectionController.rs (2)
304-308: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winApply
grewin theScheduledbranch.This branch can call
perform_gc()without the new threshold gate. For example, withprev = 16 MiBandthis_heap_size = 33 MiB, the default 32 MiB threshold is not reached, butthis_heap_size > prev * 2is true. Gate this branch withgrew &&before performing GC.🤖 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 `@src/jsc/GarbageCollectionController.rs` around lines 304 - 308, Update the GCTimerState::Scheduled branch in the garbage-collection controller to require grew alongside the existing heap-growth condition before updating the repeat timer or calling perform_gc().
273-275: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the growth threshold inclusive.
The stated contract says the timer re-arms when growth reaches the threshold, but
>requires one additional byte. Preserve thethreshold = 0behavior while using an overflow-safe growth calculation, for examplegrowth > 0 && growth >= threshold.🤖 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 `@src/jsc/GarbageCollectionController.rs` around lines 273 - 275, Update process_gc_timer_with_heap_size so heap growth reaching the calculated threshold re-arms the timer by using an inclusive comparison. Compute growth overflow-safely, preserve the threshold = 0 behavior, and require positive growth before applying the threshold check.
🤖 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.
Outside diff comments:
In `@src/jsc/GarbageCollectionController.rs`:
- Around line 304-308: Update the GCTimerState::Scheduled branch in the
garbage-collection controller to require grew alongside the existing heap-growth
condition before updating the repeat timer or calling perform_gc().
- Around line 273-275: Update process_gc_timer_with_heap_size so heap growth
reaching the calculated threshold re-arms the timer by using an inclusive
comparison. Compute growth overflow-safely, preserve the threshold = 0 behavior,
and require positive growth before applying the threshold check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1becb971-5c5d-4284-8c1c-a74bbb978e73
📒 Files selected for processing (1)
src/jsc/GarbageCollectionController.rs
There was a problem hiding this comment.
No new issues found, but this changes GC pacing runtime-wide (the 32 MB / prev/4 growth gate is a heuristic tuning decision that affects every workload's RSS/CPU trade-off), and the author's own CI-status comment above lists two still-unexplained [new] failures (bun-server.test.ts on x64-asan, test-fs-filehandle.js on Windows) plausibly tied to the cadence change. Deferring to a maintainer to sign off on the constants and the remaining CI signal.
What was reviewed:
process_gc_timer_with_heap_sizestate machine —saturating_add/saturating_mulguard the arithmetic; thePending/Scheduledpaths still reachperform_gc()on> prev*2.get_envfallback +Loader::is_truthyextraction —has()semantics unchanged;ZStrderef-coercion checked.BUN_GC_TIMER_THRESHOLDnow overrides both terms viaOption::unwrap_or_else, so the cadence test's>= 5bound is decoupled from startup heap size.- 28632.test.ts periodic
Bun.gc(true)— keeps the 12 MB bound valid; verified it doesn't mask the original leak (500-query stride vs. per-query leak).
Extended reasoning...
Overview
The PR replaces the heap_size != prev re-arm test in GarbageCollectionController::process_gc_timer_with_heap_size with a growth threshold of max(32 MB, prev/4) (or an exact BUN_GC_TIMER_THRESHOLD override), fixes the four BUN_GC_* env knobs to fall back to bun_core::getenv_z when the dotenv loader is unpopulated at init time, extracts Loader::is_truthy so the falsy set is shared, adds a release-only cadence test, and adds periodic Bun.gc(true) to 28632.test.ts to keep its RSS bound valid under the new cadence.
Security risks
None identified. No user-controlled input reaches new parsing paths; the env-var reads use existing parse_decimal helpers with the same validation as before.
Level of scrutiny
High. This is a global runtime heuristic change: every Bun process's opportunistic-GC cadence drops from ~60/s to threshold-gated. The 32 MB floor and prev >> 2 shift are tuning constants chosen by analogy to JSC's Heap::updateAllocationLimits, but they trade CPU for peak RSS across the entire user base and already required patching one RSS-bound regression test (28632). That's exactly the kind of design decision REVIEW.md flags for maintainer agreement. The change has also produced two CI failures the author could not reproduce locally and left open (bun-server.test.ts handler-GC-tracing on x64-asan, test-fs-filehandle.js on Windows x64), which need a maintainer call on whether they're pre-existing flakes or real cadence fallout.
Other factors
All prior inline findings from earlier review passes are resolved at HEAD (single-ZStr get_env, THRESHOLD overriding both terms, 28632 periodic collect). The cadence test is skipIf(isDebug) with a documented reason (debug+ASAN coalesces collectAsync requests so counts can't distinguish fixed/unfixed); release lanes carry the fail-before proof (128 → 3). CodeRabbit's three findings were withdrawn or reasonably declined. The bug-hunting system found nothing new this run. The code itself looks correct for what it intends; the open question is whether the intended behavior (and its CI collateral) is what maintainers want.
|
I independently landed on the same root cause and fix class in #35431 (now closed in favour of this one) and ran the allocation-heavy HTTP benchmark from the originating report against it, so posting the numbers here. Allocation-heavy handler (20 small objects +
(Measured with Two design differences between the branches, for consideration:
The |
…timer only The controller sampled blockBytesAllocated+extraMemorySize on every event-loop tick and armed a 16 ms one-shot whenever that value changed at all. The collection's own perturbation of those counters re-armed the timer, so any active event loop ran ~62 stop-the-world eden collections per second regardless of allocation volume. JSC already paces eden/full collections against allocation rate via GCActivityCallback::didAllocate / Heap::collectIfNecessaryOrDefer (bridged onto Bun's timer heap through WTFTimer). Remove the per-tick heap sampler, the 16 ms one-shot, GCTimerState, and the per-request process_gc_timer calls from Bun.serve / fetch / h2; keep only the 1 s / 30 s idle repeating timer so a process that stops allocating still releases memory. Also lower JSC::Options::largeHeapSize from 32 MB to 8 MB so the 1.24x growth factor engages sooner on medium-sized heaps. BUN_GC_TIMER_DISABLE / BUN_GC_TIMER_INTERVAL were read via the dotenv loader before load_process() had run, so they were silently ignored; fall back to the process environment via bun_core::getenv_z. Extract Loader::is_truthy so both Loader::has and the fallback share one definition.
robobun
changed the title
dbd2821 to
1bd336b
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/js/bun/gc/gc-controller-cadence.test.ts`:
- Line 63: Run the cadence measurements sequentially by replacing
test.concurrent with test in both affected tests in
test/js/bun/gc/gc-controller-cadence.test.ts: lines 63-63 and 76-76. No other
changes are needed.
🪄 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: f4fecba7-9c88-49fe-8082-3516495ab847
📒 Files selected for processing (12)
src/dotenv/env_loader.rssrc/event_loop/EventLoopTimer.rssrc/jsc/GarbageCollectionController.rssrc/jsc/bindings/ZigGlobalObject.cppsrc/jsc/event_loop.rssrc/runtime/api/bun/h2_frame_parser.rssrc/runtime/dispatch.rssrc/runtime/jsc_hooks.rssrc/runtime/server/mod.rssrc/runtime/webcore/fetch/FetchTasklet.rstest/js/bun/gc/gc-controller-cadence.test.tstest/regression/issue/28632.test.ts
💤 Files with no reviewable changes (5)
- src/runtime/server/mod.rs
- src/runtime/api/bun/h2_frame_parser.rs
- src/runtime/webcore/fetch/FetchTasklet.rs
- src/jsc/event_loop.rs
- src/runtime/dispatch.rs
…er-GC drain request an async collect Review nits: the fast/slow comment block was spliced with a stray empty doc line; convert it to a proper doc block. on_request_complete and on_static_request_complete became byte-identical after the GC hint was removed; delegate the static path through the dynamic one. bun-server handler-GC-tracing tests failed on release+ASAN (reproduced locally on first run post-build): the server wrapper is not collected by 30 iterations of Bun.gc(true)+fullGC() alone on that lane. The old per-tick sampler's collect_async() made it collect; request one explicitly in the drain loop so the test does not depend on controller cadence.
eab7308 to
b0cf7fa
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/GarbageCollectionController.rs (1)
84-101: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winPrevent slow mode from becoming more aggressive than fast mode.
BUN_GC_TIMER_INTERVAL=60000makes fast mode fire every 60 seconds, but slow mode is hard-coded to 30 seconds. After 30 unchanged ticks, the controller therefore increases GC frequency. Either cap the configured interval at 30 seconds or make slow mode usemax(configured_interval, 30s). Also document fast mode as the configured interval, defaulting to one second, and add a boundary test.Suggested cap
if parsed > 0 { - gc_timer_interval = parsed; + gc_timer_interval = parsed.min(SLOW_REPEAT_INTERVAL_MS); }Also applies to: 164-168, 179-193
🤖 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 `@src/jsc/GarbageCollectionController.rs` around lines 84 - 101, Update the GarbageCollectionController logic around gc_timer_interval so slow mode uses at least the 30-second interval, preventing it from becoming more aggressive than fast mode; either cap the configured value at 30 seconds or derive slow mode with the maximum of the configured interval and 30 seconds. Document that fast mode uses the configured BUN_GC_TIMER_INTERVAL, defaulting to one second, and add a boundary test covering the 30-second threshold.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/jsc/GarbageCollectionController.rs`:
- Around line 84-101: Update the GarbageCollectionController logic around
gc_timer_interval so slow mode uses at least the 30-second interval, preventing
it from becoming more aggressive than fast mode; either cap the configured value
at 30 seconds or derive slow mode with the maximum of the configured interval
and 30 seconds. Document that fast mode uses the configured
BUN_GC_TIMER_INTERVAL, defaulting to one second, and add a boundary test
covering the 30-second threshold.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 16b671bb-438b-4c58-b005-c878415e61b5
📒 Files selected for processing (3)
src/jsc/GarbageCollectionController.rssrc/runtime/server/mod.rstest/js/bun/http/bun-server.test.ts
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/perf/tracy.rs:1-3— This PR adds two new files —src/perf/tracy.rs(490 lines) andsrc/analytics/error.rs(29 lines) — that are unrelated to the GC controller change and are not wired into the build: neithersrc/perf/lib.rsnorsrc/analytics/lib.rsdeclares them as modules, and nothing in the tree references them. The PR description says "Net -50 lines across 12 files", but the diff touches 15 files and these two alone add +519 lines of orphaned uncompiled code — they look like accidentally-staged profiling scaffolding and should be dropped from this PR.Extended reasoning...
What's wrong
Two brand-new files ride along on this PR that have nothing to do with replacing the per-tick heap sampler:
src/perf/tracy.rs— 490 lines of Tracy profiler bindings (dlopen/dlsym libtracy,tracy_trace!/tracy_trace_named!macros, zone/frame emit wrappers, aBUN_TRACY_PATHenv var, a#[cfg(test)]block).src/analytics/error.rs— 29 lines defining ananalytics::Errorenum +Resultalias.
Neither is reachable from the crate roots:
src/perf/lib.rsdeclares onlypub mod generated_perf_trace_events;andpub mod system_timer;— there is nopub mod tracy;.src/analytics/lib.rsdeclaresfeatures,generate_header, andschema— there is nopub mod error;.rg tracy src/returns onlysrc/perf/tracy.rsitself;rg 'analytics::error' src/returns nothing.
Rust ignores
.rsfiles that aren't reachable via amoddeclaration, so these 519 lines are not compiled, not type-checked, not tested — they're inert bytes in the tree.Why this is flagged
The PR description explicitly scopes the change as "Net -50 lines across 12 files", but the diff touches 15 files and these two account for +519 of the discrepancy. That's a strong signal they were accidentally staged — most likely the Tracy integration was scaffolded to gather the benchmark numbers in the PR description and got swept up in
git add.REVIEW.md → Correctness: "Every line you add must be demonstrably live." REVIEW.md → Code style & idioms: "Delete dead code in the same PR that makes it dead." These files are dead on arrival — orphaned modules with zero callers.
Additionally, if
tracy.rswere wired in with apub mod tracy;, its#[cfg(test)]block invokes$crate::tracy_trace!()and references$crate::tracy::___tracy_source_location_data, which resolve only iftracyis a module of the invoking crate — socargo test -p bun_perfwould need themoddeclaration to compile it at all. The file also readsenv_var::BUN_TRACY_PATH, which doesn't exist inbun_core::env_var(grep confirms no other reference), so wiring it in as-is would likely fail to compile.Step-by-step proof
git diff --name-onlyon this PR lists 15 files, includingsrc/perf/tracy.rsandsrc/analytics/error.rsas new files.head src/perf/lib.rs→ module declarations arepub mod generated_perf_trace_events;andpub mod system_timer;only. Nomod tracy.rg '^(pub )?mod ' src/analytics/lib.rs→pub mod features,pub mod generate_header,pub mod schema. Nomod error.rg tracy src/→ the only hit issrc/perf/tracy.rsitself.- Therefore rustc never visits either file;
cargo build/bun bdsucceed with or without them. - The PR description's "12 files" claim vs the actual 15-file diff, with the extras being unrelated infra, confirms accidental inclusion rather than intentional scope.
Impact
None at runtime or build time — orphaned modules are inert. The cost is 519 lines of unreviewed, uncompiled infrastructure landing in a focused perf PR, contradicting both the PR's own scope statement and the repo's dead-code rule. Hence nit: the author will obviously want these gone, but merging them causes no crash, incorrect behavior, or regression.
Fix
git rm src/perf/tracy.rs src/analytics/error.rsand re-push. If the Tracy integration is intended for a follow-up, it belongs in its own PR wherepub mod tracy;, theBUN_TRACY_PATHenv-var declaration, and callers all land together and get reviewed as a unit.
|
Heads up: the per-tick sampler was masking a latent |
…during VM shutdown When a server's JS wrapper survives to lastChanceToFinalize (BUN_DESTRUCT_VM_ON_EXIT=1, set by the CI runner on ASAN lanes), finalize() -> deinit_if_we_can() -> schedule_deinit() enqueued a ManagedTask that the now-exiting event loop never ran, and EventLoop::deinit dropped the task box without running it. The orphaned NewServer Box and everything it owns (static_routes, html_bundle::Route, route path strings) leaked; the Route.server back-pointer made it a pointer cycle so LSan reported every allocation as an indirect leak. Always reachable but rarely seen before #35356 removed the per-tick GC sampler, which usually collected the wrapper before global_exit(). schedule_deinit() now sets DEINIT_SCHEDULED and returns without enqueueing when is_shutting_down(); finalize() then frees the Box synchronously via its Box::into_raw pointer (not through a &mut self frame, whose FnEntry protector would make the dealloc Stacked-Borrows UB). Other callers reaching schedule_deinit past shutdown (a last request draining inside close_all_socket_groups) only set the flag and leave the Box, since NewApp::destroy there would free the socket group mid-iteration.
…during VM shutdown When a server's JS wrapper survives to lastChanceToFinalize (BUN_DESTRUCT_VM_ON_EXIT=1, set by the CI runner on ASAN lanes), finalize() -> deinit_if_we_can() -> schedule_deinit() enqueued a ManagedTask that the now-exiting event loop never ran, and EventLoop::deinit dropped the task box without running it. The orphaned NewServer Box and everything it owns (static_routes, html_bundle::Route, route path strings) leaked; the Route.server back-pointer made it a pointer cycle so LSan reported every allocation as an indirect leak. Always reachable but rarely seen before #35356 removed the per-tick GC sampler, which usually collected the wrapper before global_exit(). schedule_deinit() now sets DEINIT_SCHEDULED and returns without enqueueing when is_shutting_down(); finalize() then frees the Box synchronously via its Box::into_raw pointer (not through a &mut self frame, whose FnEntry protector would make the dealloc Stacked-Borrows UB). Other callers reaching schedule_deinit past shutdown (a last request draining inside close_all_socket_groups) only set the flag and leave the Box, since NewApp::destroy there would free the socket group mid-iteration.
…during VM shutdown When a server's JS wrapper survives to lastChanceToFinalize (BUN_DESTRUCT_VM_ON_EXIT=1, set by the CI runner on ASAN lanes), finalize() -> deinit_if_we_can() -> schedule_deinit() enqueued a ManagedTask that the now-exiting event loop never ran, and EventLoop::deinit dropped the task box without running it. The orphaned NewServer Box and everything it owns (static_routes, html_bundle::Route, route path strings) leaked; the Route.server back-pointer made it a pointer cycle so LSan reported every allocation as an indirect leak. Always reachable but rarely seen before #35356 removed the per-tick GC sampler, which usually collected the wrapper before global_exit(). schedule_deinit() now sets DEINIT_SCHEDULED and returns without enqueueing when is_shutting_down(); finalize() then frees the Box synchronously via its Box::into_raw pointer (not through a &mut self frame, whose FnEntry protector would make the dealloc Stacked-Borrows UB). Other callers reaching schedule_deinit past shutdown (a last request draining inside close_all_socket_groups) only set the flag and leave the Box, since NewApp::destroy there would free the socket group mid-iteration.
…during VM shutdown (#36750) Fixes `test/js/bun/http/bun-serve-html-405.test.ts` going red on x64-asan (build [87498](https://buildkite.com/bun/bun/builds/87498) and several unrelated PR builds since ~87000). ## Repro ``` BUN_DESTRUCT_VM_ON_EXIT=1 ASAN_OPTIONS=detect_leaks=1 \ LSAN_OPTIONS=suppressions=test/leaksan.supp \ bun-debug test test/js/bun/http/bun-serve-html-405.test.ts ``` ``` Indirect leak of 2104 byte(s) in 1 object(s) allocated from: ... #11 new<bun_runtime::server::NewServer<false, true>> #12 init<false, true> src/runtime/server/mod.rs:2009:47 #13 bun_runtime::api::bun_object::serve src/runtime/api/BunObject.rs:1564:26 ... #21 BunObject_callback_serve src/runtime/api/BunObject.rs:230:25 SUMMARY: AddressSanitizer: 2942 byte(s) leaked in 7 allocation(s). ``` 10/10 without this change, 0/10 with it (local debug+ASAN). ## Cause `using server = Bun.serve({ development: true, routes: { "/": html } })` disposes via `stop(true)`, which makes `deinit_if_we_can()` downgrade `js_value` to `Weak` and return. The `NewServer` Box is only freed once the JS wrapper's `finalize()` fires and `schedule_deinit()` enqueues the actual `deinit()` as a `ManagedTask`. When the wrapper survives to `lastChanceToFinalize` (`BUN_DESTRUCT_VM_ON_EXIT=1`, which the CI runner sets on ASAN lanes), `global_exit()` has already had its last event-loop tick. The enqueued task never runs, and `EventLoop::deinit()` drops the task box without a cleanup (`ManagedTask::new` sets `cleanup: None`). The 2104-byte `NewServer<false, true>` Box, its `config.static_routes` Vec, the `html_bundle::Route` it refcounts, and the route's path strings are all orphaned. `Route.server: Cell<Option<AnyServer>>` points back at the server so LSan sees a pointer cycle and reports every allocation as indirect. The path has always existed, but before #35356 the per-tick GC sampler usually collected the wrapper during the handful of event-loop ticks between the test body and `global_exit()`, so `schedule_deinit()` ran while the loop was still live. With only the 1s idle-timer GC, a single fast test like this one reaches shutdown with the wrapper still alive more often (about half the PR builds since the merge). ## Fix `schedule_deinit()` now sets `DEINIT_SCHEDULED` and returns without enqueueing when `is_shutting_down()`. `finalize()` then frees the Box synchronously when the server has been fully drained: it unboxes via `Box::into_raw` first so the dealloc goes through the raw owning pointer rather than a `&mut self` frame (whose FnEntry protector would make the dealloc Stacked-Borrows UB, same pattern as `Listener::finalize` / `UDPSocket::finalize`). Every JSC handle on the Drop chain (`JSPromiseStrong`, `JsRef`, `UserRouteBuilder.callback: Strong`) funnels through `Strong::Impl::destroy`, which is a no-op past `is_shutting_down()`, so freeing here is safe. The inline free is gated on `TERMINATED`: `NewApp::destroy` runs `us_socket_group_deinit`, which unlinks the socket group from the loop's list without closing any sockets still in it. A graceful `stop()` only closes the listener and leaves keep-alive sockets open in the group; destroying the app there would orphan them (seen as a 280-byte `us_poll_t` direct leak on `vendor/elysia/test/core/before-handle-arrow.test.ts` with an earlier revision of this PR, and a `US_ASSERT(head_sockets==NULL)` abort on the debug build). `TERMINATED` is set only once `app.close()` has run, so the inline free is taken for abruptly-stopped servers (what `using server` does) and skipped for gracefully-stopped ones, which is identical to `main`'s behaviour for them. Other callers that can reach `schedule_deinit()` past shutdown (a last request draining inside `close_all_socket_groups`, which runs before `lastChanceToFinalize`) only set the flag and leave the Box, since `NewApp::destroy` there would delete the uws socket group mid-iteration. ## Verification Two ASAN-only subprocess tests added: - abrupt `stop(true)` of a dev server with an HTML route under `BUN_DESTRUCT_VM_ON_EXIT=1` + `detect_leaks=1`: fails on `main` with the 7-allocation LSan report above; passes with this change. - graceful `stop()` of a plain server with a keep-alive client connection: passes on both `main` and this change (asserts `us_socket_group_deinit`'s `head_sockets==NULL` precondition, which an earlier revision of this change violated). <!-- robobun:evidence:begin --> --- **no test proof** · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/http/bun-serve-html-405.test.ts <!-- robobun:evidence:end -->
…ctors Review of the WebKit bump surfaced two gaps: Bun__StrongRef__new/set/delete mutate the StrongRootBlock list that the Srb marking constraint scans, with the same API-lock requirement as JSC's HandleSet but no assertion. Add the mirror asserts, checking the owner VM (the block's), so a thread holding some other VM's lock still trips them. Strong.rs's existing TLS check covers only the delete path and only proves the thread has a VM, not the owner VM's lock; the C++ Bun::StrongRef deleter bypasses it entirely. Neither detector had a liveness test, which is how the previous probabilistic guard for #30185 died silently (#35356, measured in #36952). jscInternals gains a debug-only crossThreadStrongHandleMutation hook that violates the contract from a spawned thread, and the new test asserts the child aborts with each guard's message. If a future WebKit bump or binding refactor drops an assertion, the test fails instead of the coverage silently disappearing.
### Problem - `node:zlib` one-shot calls got up to 2x slower for small inputs in 1.4.0: `gunzipSync` of 1 KB went from 3.1 us to 5.0 us. #35356 lowered `largeHeapSize` to 8 MB, so these calls collect 4 times as often. - A third of that load comes from `buffer.slice()` in `processChunkSync` and `processCallback` (`src/js/node/zlib.ts`). The 16 KB output chunk has no `ArrayBuffer` until the slice creates one. JSC then counts the chunk as allocated again, and only a full collection corrects the heap size. A 1 KB result also keeps its 16 KB chunk alive. ### Fix - Both paths call `takeOutput`. It hands over a full chunk as it is and copies a part of at most 16 KB with `Buffer.copyBytesFrom`. A larger part of a custom `chunkSize` chunk, or any part with a fractional `chunkSize`, is still a slice. - Correct because the consumer gets the same bytes. Sync output of several chunks still goes through `Buffer.concat`, which minizlib (used by `tar`) replaces. - Verified: `test/js/node/zlib/zlib.test.js` ("one-shot results"), where main gives a 1 KB result a 16384-byte backing store. Also all of `test/js/node/zlib/`, the 61 Node `test-zlib-*.js` files, and a minizlib round trip. Self-reviewed: 3 concerns raised, 3 addressed (see Notes). - Release build, main to this PR: `gunzipSync` 1 KB 4.99 to 2.97 us, callback `gunzip` 39.0 to 34.0 us, 64 MB through gzip streams 166 to 131 ms. 120k `gunzipSync` calls: 410 eden + 136 full collections to 318 + 0. ### Background - A typed array above 1000 bytes owns a malloc'ed vector and gets an `ArrayBuffer` only when JS needs one (`.buffer`, `subarray`, Buffer's `slice`). `JSArrayBufferView::slowDownAndWasteMemory` then reports the vector to the GC again. It has a FIXME for this. - JSC starts an eden collection when the bytes allocated since the last one pass a budget. On a small heap that budget is `largeHeapSize`. - `nativeDecodePullResult` in `BunStreamSource.cpp` already uses this rule. <details><summary>Notes</summary> - Found from a perf ledger entry: node:zlib sync one-shot calls on 1 KB inputs got 1.5x to 3.4x slower at 1.4.0 while `Bun.gunzipSync` got faster. The JS wrapper and the native handle did not get slower. `BUN_JSC_largeHeapSize=33554432` on main gives the 1.3.14 collection counts exactly (105 eden + 34 full per 120k `gunzipSync`) and the 1.3.14 speed. The zstd rows have a second cause, fixed in #42914. - GC-visible bytes for one `gunzipSync` call, from collections per 100k calls: `Buffer.allocUnsafe(16384)` 16.4 KB, the slice 16.4 KB more, the native handle 3.6 KB, the stream objects about 1 KB. `Buffer.allocUnsafe(16384).subarray(0, 10)` alone runs 262 eden + 130 full collections per 100k, against 195 + 0 without the `subarray`. The JSC side of this has its own follow-up. oven-sh/WebKit#273 fixes the same double charge for transferred contents. - All rows, ns per call, 1.3.14 / main / this PR, machine under load: `gunzipSync` 1 KB 3056 / 4994 / 2974. `inflateSync` 1 KB 2837 / 4633 / 3450. `brotliDecompressSync` 1 KB 5308 / 8680 / 6170. `gzipSync` 1 KB 8267 / 10211 / 8347. `zstdDecompressSync` 1 KB 5021 / 15193 / 13701. `gunzip` callback 1 KB 33154 / 38970 / 34016. `gzip` callback 1 KB 39866 / 49531 / 45815. `gunzip` callback 1 MB 1.78 / 2.06 / 1.74 ms. 20k flushed `createDeflate` writes 506 / 570 / 442 ms. - A focused run of 8 alternating rounds for the sizes between the rows above, main to this PR: `gunzipSync` 4 KB 8.12 to 7.26 us, 12 KB 12.27 to 12.38 us (CPU time 15.66 to 14.19 us), 1 MB 804 to 716 us, 600 KB with `chunkSize: 1 << 20` 395 to 399 us (that one still takes the slice path). - Callback path: 30k `gunzip` calls of 1 KB run 129 eden + 43 full collections on main and 86 + 0 with this PR. - The 16 KB bound: with `chunkSize: 1 << 20` and 600 KB of output, a copy of the whole part measured 5 to 10% slower than the slice and raised peak RSS, so a large part stays a slice. - A fractional `chunkSize` (valid in Node and in Bun) makes `offset` and `have` fractional. `slice` truncates them and `Buffer.copyBytesFrom` throws `ERR_OUT_OF_RANGE`, so the helper only copies integer ranges. The new tests cover `chunkSize: 100.5` for the sync, callback, and `_processChunk` paths. - Self-review, all three addressed: a fractional `chunkSize` made `Buffer.copyBytesFrom` throw (now an integer guard, with tests). The callback path still sliced while the sync path did not (now one helper for both, with tests for both). The code comment said a full collection frees the chunk, when it only corrects the heap size (reworded). - Not part of this PR: `src/js/internal/streams/iter/transform.ts` already hands over a full chunk and copies a partial fill. It still uses `subarray` for the tail of a chunk that it replaces, which is at most once for each chunk. - Node 26 returns a view into a 64 KB slab (`result.buffer.byteLength` is 65536, `byteOffset` 16384). The size of the backing store is not a compat contract. - minizlib calls the sync `_processChunk` on a long-lived engine with `Buffer.concat` replaced and the handle's `close` stubbed. It copies `result[0]` because that one can be `_outBuffer`, which the next call writes again. That still holds: a full first chunk is `_outBuffer` itself, a small first part is now a fresh copy. pngjs reads `_outBuffer` and `_outOffset` once in its constructor. Both fields keep their values and meaning. - `leak.test.ts` exceeds its `beforeAll` hook timeout on a debug ASAN build with and without this change. It passes on the release build. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 2 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 12 failed, 2 skipped $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/zlib/zlib.test.js bun test v1.4.3 (c6b7fcb) test/js/node/zlib/zlib.test.js: (pass) prototype and name and constructor > Gzip > Gzip.prototype should be instanceof Gzip.__proto__ [1.98ms] (pass) prototype and name and constructor > Gzip > Gzip.prototype.constructor should be Gzip [1.92ms] (pass) prototype and name and constructor > Gzip > Gzip.name should be Gzip [1.15ms] (pass) prototype and name and constructor > Gzip > Gzip.prototype.__proto__.constructor.name should be Zlib [2.74ms] (pass) prototype and name and constructor > Gunzip > Gunzip.prototype should be instanceof Gunzip.__proto__ [0.47ms] (pass) prototype and name and constructor > Gunzip > Gunzip.prototype.constructor should be Gunzip [0.27ms] (pass) prototype and name and constructor > Gunzip > Gunzip.name should be Gunzip [0.29ms] (pass) prototype and name and constructor > Gunzip > Gunzip.prototype.__proto__.constructor.name should be Zlib [0.30ms] (pass) prototype and name and constructor > Deflate > Deflate.prototype should be instanceof Deflate.__proto ... (truncated) release without fix: 2 skipped bun test v1.4.3-canary.1 (a8e4e90) test/js/node/zlib/zlib.test.js: (pass) prototype and name and constructor > Gzip > Gzip.prototype should be instanceof Gzip.__proto__ [0.02ms] (pass) prototype and name and constructor > Gzip > Gzip.prototype.constructor should be Gzip [0.01ms] (pass) prototype and name and constructor > Gzip > Gzip.name should be Gzip (pass) prototype and name and constructor > Gzip > Gzip.prototype.__proto__.constructor.name should be Zlib [0.02ms] (pass) prototype and name and constructor > Gunzip > Gunzip.prototype should be instanceof Gunzip.__proto__ (pass) prototype and name and constructor > Gunzip > Gunzip.prototype.constructor should be Gunzip (pass) prototype and name and constructor > Gunzip > Gunzip.name should be Gunzip (pass) prototype and name and constructor > Gunzip > Gunzip.prototype.__proto__.constructor.name should be Zlib (pass) prototype and name and constructor > Deflate > Deflate.prototype should be instanceof Deflate.__proto__ (pass) prototype and name and constructor > Deflate > Deflate.prototype.constructor should be Deflate (pass) prototype and name and constructor > Deflate > Deflate.name should be Deflate (pass) pr ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: 2 skipped $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/zlib/zlib.test.js bun test v1.4.3 (c6b7fcb) test/js/node/zlib/zlib.test.js: (pass) prototype and name and constructor > Gzip > Gzip.prototype should be instanceof Gzip.__proto__ [1.83ms] (pass) prototype and name and constructor > Gzip > Gzip.prototype.constructor should be Gzip [1.63ms] (pass) prototype and name and constructor > Gzip > Gzip.name should be Gzip [0.98ms] (pass) prototype and name and constructor > Gzip > Gzip.prototype.__proto__.constructor.name should be Zlib [1.81ms] (pass) prototype and name and constructor > Gunzip > Gunzip.prototype should be instanceof Gunzip.__proto__ [0.44ms] (pass) prototype and name and constructor > Gunzip > Gunzip.prototype.constructor should be Gunzip [0.26ms] (pass) prototype and name and constructor > Gunzip > Gunzip.name should be Gunzip [0.27ms] (pass) prototype and name and constructor > Gunzip > Gunzip.prototype.__proto__.constructor.name should be Zlib [0.31ms] (pass) prototype and name and constructor > Deflate > Deflate.prototype should be instanceof Deflate.__proto ... (truncated) release with fix: 2 skipped $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 786ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/142] gen JS modules (bundle-modules) Preprocess modules (7327ms) Bundle modules (114ms) Postprocesss modules (21ms) Bundle Functions (478ms) Generate Code (52ms) [8.00s] Bundled "src/js" for production 2600 kb 197 internal modules 13 native modules 50 internal functions across 16 files [1/141] cargo bun_runtime → libbun_runtime.a �[1m�[33mwarning�[0m�[1m: binary `bun_shim_impl` should have a kebab-case name�[0m �[1m�[94m|�[0m �[1m�[94m 1�[0m �[1m�[94m|�[0m /workspace/bun/build/release/rust-target/.../bun_shim_impl �[1m�[94m|�[0m �[1m�[33m^^^^^^^^^^^^^�[0m �[1m�[94m|�[0m �[1m�[94m= �[0m�[1mnote�[0m: `cargo::non_kebab_case_bins` is set to `warn` by default �[1m�[96mhelp�[0m: to change the binary name to `bun-shim-impl`, convert `bin.name` �[1m�[94m--> �[0msrc/install/windows-shim/Cargo.toml:41:8 �[1m�[94m|�[0m �[1m�[94m41�[0m �[91m- �[0mname = �[91m"bun_shim_impl"�[0m �[1m�[94m41�[0m �[92m+ �[0mname = �[92m"bun-shim-impl"�[0m �[1m ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/js/node/zlib.ts | 20 +++++++++- test/js/node/zlib/zlib.test.js | 83 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 2 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/js/node/zlib.ts 3 3 18 test/js/node/zlib/zlib.test.js 2 4 15 ``` </details> <!-- robobun:evidence:end -->
3 participants
A production report of "switched bun→node, CPU and loop-lag p95 dropped" traced to Bun's
GarbageCollectionControllerrunning an eden collection roughly every 16 ms of event-loop activity regardless of allocation volume. With a non-trivial live set each of those is a multi-millisecond stop-the-world pause, so a steady-stateBun.serveat 150 rps with a ~300 MB live heap spent up to ~40% of wall time in GC at ~62 collections/s (vs ~1.4% for Node's scavenger at the same load).BUN_JSC_logGC=trueon a baresetInterval(() => alloc(~50KB), 20)shows ~128EdenCollectionover 2 s;BUN_GC_TIMER_DISABLE=1has no effect on the count.Cause
process_gc_timer_with_heap_sizesampledvm.heap.blockBytesAllocated() + vm.heap.extraMemorySize()on every event-loop tick and re-armed the 16 ms one-shot whenever that value!= prev. Both counters move on every 16 KB block allocation and everyreportExtraMemorycall, and the collection itself perturbs them, so theRunOnNextTick→collect_async()→!= prev→ re-arm 16 ms loop self-perpetuated at the 1000/16 ≈ 62.5/s ceiling for as long as the event loop was active.Separately,
init()readBUN_GC_TIMER_DISABLE/BUN_GC_TIMER_INTERVAL/BUN_GC_RUNS_UNTIL_SKIP_RELEASE_ACCESSviavm.env_loader_opt(); atensure_waker()time the dotenv loader exists butload_process()has not run yet, so its map is empty and the knobs were silently ignored.Fix
Delete the 16 ms one-shot and the per-tick / per-request heap sampler entirely (the
GcOneShottag, theGCTimerStatemachine,request_gc_hint/drain_pending_gc_hint, and theprocess_gc_timer()calls fromon_request_complete/auto_tick/ the fetch/h2 completion paths). JSC's own allocation-rate-budgetedGCActivityCallback::didAllocate/Heap::collectIfNecessaryOrDeferis already wired onto Bun's timer heap viaWTFTimerand is the primary eden driver; with the sampler gone, nothing is left to self-perpetuate.Keep the controller's 1 s / 30 s repeating idle timer, which requests a
collect_async()once per interval so a process that stops allocating still releases memory.process_gc_timer()now just arms that timer on first call.Lower
JSC::Options::largeHeapSizefrom 32 MB to 8 MB so JSC's proportional-heap budget engages sooner on small heaps, trimming the peak-RSS plateau the sampler used to mask.Declare the three env-var knobs in
bun_core::env_varsoinit()reads them via the typed, cached, process-env accessors; the dotenv-loader ordering issue does not arise on that path.Follow-on fix exposed by the sampler removal
moduleLoaderFetch/functionFulfillModuleSync/moduleLoaderImportModuledropGCOwnedDataScopebefore transpileThe
auto moduleKey = jsString->value(globalObject)locals wereGCOwnedDataScope<const String&>, alive acrossfetchESMSourceCode→ synchronous transpile → an async macro'swait_for_promiseevent-loop spin.IncrementalSweepercan fire during that spin withvm.entryScopenull and trips theASSERT(!m_topGCOwnedDataScope)inHeap::clearConcurrentRetainedDataIfPossible(x64-asanmacro-test.test.ts). Take a plainWTF::Stringso the scope temporary dies at the statement. 5/5 pass on linux debug+asan.Test adjustments
test/js/bun/gc/gc-controller-cadence.test.ts(new):setInterval(~50 KB / 20 ms)× 100 ticks, counts=> EdenCollectionlines fromBUN_JSC_logGC. Assertseden < 30for the default case (was ~128) andeden < 5withBUN_GC_TIMER_DISABLE=1(was ~128; env var ignored).skipIf(isDebug)since debug+ASAN's ~100 ms GC cycles coalescecollectAsync()requests and the count cannot distinguish fixed from unfixed there.test/js/bun/http/bun-server.test.ts: addBun.gc(false)(=collect_async()) to the four server-wrapper-count drain loops. On release+ASAN the wrapper was only collected by the old per-requestcollect_async();Bun.gc(true)+fullGC()alone do not collect it there.test/regression/issue/28632.test.ts:Bun.gc(true)every 500 queries so the RSS delta measures thename_or_indexleak rather than the peak MarkedBlock high-water mark between opportunistic passes.Open item
test/js/node/test/parallel/test-fs-filehandle.json Windows x64 release: the abandoned FileHandle is not collected by 20× syncglobalThis.gc()within 200 ms.jsc.generateHeapSnapshotForDebugging()reports FileHandle with a singleWeakRef[Internal]retainer and not inroots(noConservativeScanroot reason recorded for any node), so per the snapshot it is unreachable;globalThis.gc()disagrees. PassingthisValue=undefinedtoJSC::profiledCallinBun__JSTimeout__callmakes the Immediate wrapper itself collectible but not callback-locals or the FileHandle. AsanitizeStackForVM(*vm)atBun__JSC_onBeforeWaitmakes the test pass 10/10 but was rejected in review; an#[inline(never)]split ofrun_immediate_taskdid not change the outcome. Needs a root-cause fix or the lane accepted as a known failure.Fixes #27365
Related: #8897, #26784
no test proof · iteration 36 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/http/bun-server.test.ts