Repository navigation
event loop: sanitize the stack on every tick so objects dropped by earlier turns get collected - #37853
event loop: sanitize the stack on every tick so objects dropped by earlier turns get collected#37853robobun wants to merge 1 commit into
Conversation
…y when parking JSC scans the machine stack conservatively. The region a finished turn's frames occupied keeps its JSValues until something overwrites it, and a GC run from a later turn whose frames overlay that region without writing every slot still sees them. JSC's own sanitize sites (allocation slow paths, call linking, the collector) all run inside JS, below the frames that hold a turn's values, so only the embedder can clear that region, between turns. Bun did that as a side effect of Bun__JSC_onBeforeWait: heap.stopIfNecessary() sanitizes while the mutator holds the GC conn. That covers less than it looks like. On POSIX the hook has only run on ticks that park since #26821, so a loop kept busy by setImmediate never got it, and a synchronous collection (Bun.gc(true)) gives the conn back, after which stopIfNecessary() does not sanitize at all, which is what #35356 ran into on Windows. Add Bun__JSC_onLoopTick, which calls sanitizeStackForVM once per tick from us_loop_run_bun_tick (POSIX) and us_loop_run / us_loop_pump (Windows), whether or not the tick parks. It is a no-op unless JS sanitized deeper since the last tick, and then it zeroes exactly the stack the turn used. With this, objects dropped in one turn are collected by the next tick's GC (at most the one after) regardless of which primitive drives the loop. Previously a setImmediate-driven GC loop needed ~17 Bun.gc(true) calls, until JIT tier-up happened to overwrite the stale slot. Restores test-fs-filehandle.js, which #35356 deleted because of this.
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
|
Status Reproduced on main (Linux x64, release 1.4.0 and debug+ASAN) with the script in the description: a Fix is in this PR (#37853): |
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The root-cause analysis is thorough and the fix is small, but it adds a call to every JS-thread event-loop tick (the hottest path in the runtime) and changes GC-observable timing across all three eventing backends — a maintainer should sign off on the per-tick cost claim and the cross-platform placement.
What was reviewed
Bun__JSC_onLoopTickis guarded onloop->data.jsc_vmat all three call sites, so non-JS loops (HTTP thread) are unaffected.- Placement before the
num_polls == 0early-out and beforeus_internal_loop_pre— checked that nothing between tick entry and the new call can allocate or enter JS. - The new test's
gcs <= 2bound is justified in the description as a hard bound (not a heuristic threshold), and the loop caps at 8 so the failure mode is a clear assertion, not a hang.
Extended reasoning...
Overview
Adds Bun__JSC_onLoopTick (a thin wrapper around JSC::sanitizeStackForVM) and calls it once per JS-thread event-loop tick from us_loop_run_bun_tick (epoll/kqueue) and from us_loop_run / us_loop_pump (libuv/Windows). This zeroes the stack region the previous turn's frames occupied so the conservative GC scan stops finding stale JSValues there. Previously only Bun__JSC_onBeforeWait (which runs only on parking ticks, and only sanitizes while the mutator holds the GC conn) covered this, so a setImmediate-driven loop never sanitized between turns. Two tests are added: a 2×2 matrix asserting collection within 2 GCs, and the restoration of test-fs-filehandle.js that #35356 deleted for this reason.
Security risks
None. No user input is parsed, no new surface is exposed. The new function reads a VM pointer already stored on the loop and calls an existing JSC internal.
Level of scrutiny
High. us_loop_run_bun_tick is the innermost body of the JS event loop; every microtask drain, every setImmediate, every I/O dispatch passes through it. The PR argues the added sanitizeStackForVM is amortized (it early-outs when nothing ran deeper than the last sanitize, and moves zeroing from JSC's in-turn sites to the tick boundary rather than adding it), and cites BUN_JSC_verboseSanitizeStack=1 measurements. That argument is plausible and well-sourced, but per-tick overhead on this path is exactly the kind of thing a maintainer should confirm against the benchmarks referenced in the neighboring Bun__JSC_onBeforeWait comment (#14885, context-switch counts). The change also touches three platform backends in parallel, and the new test asserts a tight GC-timing bound (<= 2) that depends on the exact ordering of the sanitize relative to the immediates/timers phases — if that ordering ever shifts, this becomes a flake source.
Other factors
- The root-cause analysis is unusually rigorous: heap snapshot → stack scan → frame walk → two independent confirming experiments. It correctly frames this as an embedder responsibility rather than a scanner bug.
- The
jsc_vmnull-guard mirrors the existingBun__JSC_onBeforeWaitcall sites, so the HTTP thread's loop (which passes throughus_loop_run_bun_tickwith no VM) is unaffected. - The restored
test-fs-filehandle.jsusessetTimeout(notsetImmediate) in its retry loop and caps at 20 iterations, so it doesn't directly exercise thesetImmediatepath this PR fixes — it validates the second failure mode (sync GC handing back the conn) instead. - No prior human review on the timeline; only a rate-limited CodeRabbit stub.
|
On the four suggested issues: I am not adding
On the review notes above: the per-tick call is a compare-and-return unless JS reached one of JSC's own sanitize points during the turn, and in that case the zero-fill covers the stack the turn actually used, which JSC would otherwise have zeroed piecemeal at its next shallower sanitize; the measurement in the description (48 calls over 50 |
|
Updated 1:05 PM PT - Aug 12th, 2026
❌ @robobun, your commit 32d303e has some failures in 🧪 To try this PR locally: bunx bun-pr 37853That installs a local version of the PR into your bun-37853 --bun |
…ng to occupy A microtask checkpoint is where a turn of the embedder's event loop resumes an async function: a timer or an I/O callback settles a promise, and the queue is drained. Every turn does that from the same stack depth, so the frames of the checkpoint (MicrotaskQueue::drain*(), runInternalMicrotask(), the job's, the entry to JS) are at the same addresses in every turn. They stay for as long as the turn's JS runs, and they lie over whatever ran at that depth since the last checkpoint: that checkpoint's own frames, or the embedder's. What they do not write is in reach of the conservative scan of every collection made from under them, and sanitizeStackForVM(), which such a collection calls, clears only what is below the frame that calls it. #673 took the words that ASan poisons (redzones, locals out of scope) out of the scan. This is about the words it cannot take out: spill slots and locals of a live frame that this job's path through the function does not write. Builds without ASan have only those, and ASan builds have them too. runInternalMicrotask() is one switch over every kind of job, with callMicrotask() inlined into several arms, and a job uses one arm. Seen as: what a finished async function held is not collected by collections made from later turns. async function makeGarbage() { /* two objects in a list, one await per object */ } await makeGarbage(); for (;;) { await turn(); fullGC(); } // turn(): a promise that a timer resolves jsc shell of c281568 (linux amd64, lto): 32 turns until the objects are collected (the loop tiers up and the frames change), 60 of 60 turns not collected with --useJIT=0. Bun at that WebKit (release, x86_64): not collected in 8 of 8 turns when the timer is Bun.sleep(1); Bun's debug ASan build, which has #673: not collected in 8 of 8 turns for the module shape with any timer. A heap snapshot has the objects held by `list`, held by the lexical environment of the finished function's JSAsyncFunctionGenerator, which has no incoming edge and no root. In a debugger, the one word of the scanned span that holds the generator's address is at rbp-64 of runInternalMicrotask()'s frame (272 bytes), under drainWithUseCallOnEachMicrotask() (624 bytes). The job that runs in the later turns (AsyncModuleExecutionResume for the module, AsyncFunctionResume when the loop is in an async function of its own) does not write that slot. Which shape shows it moves with the compiler: the same script with setTimeout() kept the objects with clang 21 and does not with clang 23. Earlier sightings of this frame in Bun's leak tests: oven-sh/bun#37853 (the same generator word), oven-sh/bun#41607 (darwin arm64, release). So a checkpoint that has jobs to run first clears MicrotaskQueue::stackBytesClearedForCheckpoint bytes of the stack below performMicrotaskCheckpoint(), before the first of those frames exists: 2 KB in a release build (from there to the JS frame is 1.5 KB for x86_64), 32 KB with assertions or ASan (12.6 KB with both). It does that with a function that is never inlined and whose frame is an array of that size, which it zeroes with memset(): that frame lies exactly where the caller's next callee has its own, on every ABI, and the compiler probes the stack for it where a platform needs that. The array's address goes through an empty asm statement. Without it the stores are dead to the compiler (zeroBytes() and secureZeroBytes() both compile to a bare `ret` here: the memory clobber of secureZeroSpan() does not keep stores to a local whose address does not escape). callMicrotask() asserts that it runs within seven eighths of the window below the checkpoint's MicrotaskCallCache, which is a local of drainImpl(). Not sanitizeStackForVM() at the checkpoint: it clears from where it was last called, and nothing need have called it from below the checkpoint since. The jsc shell does not change with it (JSLock::didAcquireLock() resets VM::m_lastStackTop for every task). Not sanitizeStackForVMImpl() with m_lastStackTop lowered to the bottom of the window, which was the first version: its loop for x86_64 stores 8 bytes per iteration, and a checkpoint with one job went from 106 ns to 195 ns. Cost, Xeon 8375C, clang 23: `promise.then(noop); drainMicrotasks()` 5 M times in the jsc shell, 101 ns per iteration without the clear and 122 ns with it. Bun, 1 M turns of `await new Promise(r => setImmediate(r))`: 2531 ms without and 2547 ms with (medians of 11, same binary, the option). 20 M awaits in one checkpoint: 602 ms and 602 ms. An empty checkpoint clears nothing. Once per checkpoint and not once per job, which would cost every job that much: what one job leaves is still there for the later jobs of the same checkpoint, and is gone at the next one. Not when the window would reach below the soft stack limit. An embedder that holds the API lock for the life of its thread can get much of this from a sanitizeStackForVM() per event loop turn instead (oven-sh/bun#37853): in Bun that collects the same six cases at the same cost. It depends on something having sanitized from below the stale word since it was written, clears per turn and not per checkpoint, and covers the embedder's own frames as well. The two do not exclude each other. Options::clearStackForMicrotaskCheckpoint (default on) is for comparisons. Tests: JSTests/modules/microtask-checkpoint-clears-its-stack.js (an async function, then the module) and JSTests/stress/microtask-checkpoint-clears-its-stack.js (a promise reaction, then an async generator, each followed by an async function). Both fail in the shell of c281568 and with --clearStackForMicrotaskCheckpoint=0, and pass in the 28 configurations that run-javascriptcore-tests runs them in (release, x86_64).
…ng to occupy A microtask checkpoint is where a turn of the embedder's event loop resumes an async function: a timer or an I/O callback settles a promise, and the queue is drained. Every turn does that from the same stack depth, so the frames of the checkpoint (MicrotaskQueue::drain*(), runInternalMicrotask(), the job's, the entry to JS) are at the same addresses in every turn. They stay for as long as the turn's JS runs, and they lie over whatever ran at that depth since the last checkpoint: that checkpoint's own frames, or the embedder's. What they do not write is in reach of the conservative scan of every collection made from under them, and sanitizeStackForVM(), which such a collection calls, clears only what is below the frame that calls it. #673 took the words that ASan poisons (redzones, locals out of scope) out of the scan. This is about the words it cannot take out: spill slots and locals of a live frame that this job's path through the function does not write. Builds without ASan have only those, and ASan builds have them too. runInternalMicrotask() is one switch over every kind of job, with callMicrotask() inlined into several arms, and a job uses one arm. Seen as: what a finished async function held is not collected by collections made from later turns. async function makeGarbage() { /* two objects in a list, one await per object */ } await makeGarbage(); for (;;) { await turn(); fullGC(); } // turn(): a promise that a timer resolves jsc shell of c281568 (linux amd64, lto): 32 turns until the objects are collected (the loop tiers up and the frames change), 60 of 60 turns not collected with --useJIT=0. Bun at that WebKit (release, x86_64): not collected in 8 of 8 turns when the timer is Bun.sleep(1); Bun's debug ASan build, which has #673: not collected in 8 of 8 turns for the module shape with any timer. A heap snapshot has the objects held by `list`, held by the lexical environment of the finished function's JSAsyncFunctionGenerator, which has no incoming edge and no root. In a debugger, the one word of the scanned span that holds the generator's address is at rbp-64 of runInternalMicrotask()'s frame (272 bytes), under drainWithUseCallOnEachMicrotask() (624 bytes). The job that runs in the later turns (AsyncModuleExecutionResume for the module, AsyncFunctionResume when the loop is in an async function of its own) does not write that slot. Which shape shows it moves with the compiler: the same script with setTimeout() kept the objects with clang 21 and does not with clang 23. Earlier sightings of this frame in Bun's leak tests: oven-sh/bun#37853 (the same generator word), oven-sh/bun#41607 (darwin arm64, release). So a checkpoint that has jobs to run first clears MicrotaskQueue::stackBytesClearedForCheckpoint bytes of the stack below performMicrotaskCheckpoint(), before the first of those frames exists: 2 KB in a release build (from there to the JS frame is 1.5 KB for x86_64), 32 KB with assertions or ASan (12 KB with both). It does that with a function that is never inlined and whose frame is an array of that size, which it zeroes with memset(): that frame lies exactly where the caller's next callee has its own, on every ABI, and the compiler probes the stack for it where a platform needs that. The array's address goes through an empty asm statement. Without it the stores are dead to the compiler (zeroBytes() and secureZeroBytes() both compile to a bare `ret` here: the memory clobber of secureZeroSpan() does not keep stores to a local whose address does not escape). callMicrotask() asserts that it runs within seven eighths of the window below the checkpoint's MicrotaskCallCache, which is a local of drainImpl(). Not sanitizeStackForVM() at the checkpoint: it clears from where it was last called, and nothing need have called it from below the checkpoint since. The jsc shell does not change with it (JSLock::didAcquireLock() resets VM::m_lastStackTop for every task). Not sanitizeStackForVMImpl() with m_lastStackTop lowered to the bottom of the window, which was the first version: its loop for x86_64 stores 8 bytes per iteration, and a checkpoint with one job went from 106 ns to 195 ns. Cost, Xeon 8375C, clang 23: `promise.then(noop); drainMicrotasks()` 5 M times in the jsc shell, 101 ns per iteration without the clear and 122 ns with it. Bun, 1 M turns of `await new Promise(r => setImmediate(r))`: 2531 ms without and 2547 ms with (medians of 11, same binary, the option). 20 M awaits in one checkpoint: 602 ms and 602 ms. An empty checkpoint clears nothing. Once per checkpoint and not once per job, which would cost every job that much: what one job leaves is still there for the later jobs of the same checkpoint, and is gone at the next one. Not when the window would reach below the soft stack limit. An embedder that holds the API lock for the life of its thread can get much of this from a sanitizeStackForVM() per event loop turn instead (oven-sh/bun#37853): in Bun that collects the same six cases at the same cost. It depends on something having sanitized from below the stale word since it was written, clears per turn and not per checkpoint, and covers the embedder's own frames as well. The two do not exclude each other. Options::clearStackForMicrotaskCheckpoint (default on) is for comparisons. Tests: JSTests/modules/microtask-checkpoint-clears-its-stack.js (an async function, then the module) and JSTests/stress/microtask-checkpoint-clears-its-stack.js (a promise reaction, then an async generator, each followed by an async function). Both fail in the shell of c281568 and with --clearStackForMicrotaskCheckpoint=0, and pass in the 28 configurations that run-javascriptcore-tests runs them in (release, x86_64).
|
Related: oven-sh/WebKit#681 clears the stack at the microtask checkpoint in JSC, and #42975 pins its preview build. It is about the same stale word in the I compared the two on one release binary (linux x64, clang 23) with the six cases of
Differences: the per-tick call needs something to have sanitized from below the stale word since the word was written, it clears per tick and not per checkpoint, and it also covers Bun's own frames and entries that are not checkpoints. The two changes do not exclude each other. |
|
Context for whoever reviews this PR:
|
Problem
Bun.gc(true)for many turns (usually 17, sometimes 8 or 51) when the event loop is kept busy bysetImmediate. The same loop driven bysetTimeout(0)collects them on the first GC, so GC and leak tests that poll viasetImmediateconverge slowly or look like leaks (test-fs-filehandle.jswas deleted in GarbageCollectionController: replace per-tick heap sampler with idle timer only #35356 over this).Bun__JSC_onBeforeWait, which on POSIX since perf(event_loop): avoid eventfd wakeup for setImmediate on POSIX #26821 runs only on ticks that park, and which stops zeroing after a synchronous GC hands the GC conn back (the Windows case GarbageCollectionController: replace per-tick heap sampler with idle timer only #35356 hit). A loop that is always ready never parks, so the region is never cleared.Fix
Bun__JSC_onLoopTick, which callsJSC::sanitizeStackForVMonce per tick, and call it from the POSIX and Windows tick paths whether or not the tick parks. The park-only hook is unchanged.setImmediatechain it zeroed on 8 of 48 calls.setImmediateorsetTimeout) asserts collection within 2 GCs. BothsetImmediateloop cells fail deterministically on main; with the fix the cells take 1, 2, 1, 1 GCs on Linux and Windows.test-fs-filehandle.jsis restored unchanged and passes on both.Background
sanitizeStackForVMzero-fills the stack between the deepest point JSC last sanitized and the current stack pointer, then records the new mark, so it is a no-op when nothing has run deeper since. Its callers inside JSC all sit below the frames that hold a turn's values.heap.stopIfNecessary()sanitizes only while the mutator holds it, and a synchronous collection hands it to the collector.us_loop_run_bun_tick(epoll/kqueue) andus_loop_run/us_loop_pump(libuv on Windows) are the per-tick entry points of the JS thread's usockets loop;Bun__JSC_onBeforeWaitis called from them only when the tick is about to block.Original description
What
Objects that are already unreachable keep surviving
Bun.gc(true)for many event loop turns when the loop is being driven bysetImmediate, while the same loop driven bysetTimeout(0)collects them on the first GC. GC/leak tests that poll withBun.gc(true); await new Promise(r => setImmediate(r))converge very slowly or look like leaks (that is how this was found, while writing aBun.serveleak test; theBun.gc(false); await setImmediatepreludes inbun-server.test.tsand the deletion oftest-fs-filehandle.jsin #35356 are the same thing).On main (Linux x64, release and debug), any combination where the GC loop yields via
setImmediatetakes 17 GCs (sometimes 8, once 51); yielding viasetTimeouttakes 1. The 17 does not move with wall time (busy-waiting 20 ms per iteration leaves it at 17); it lines up with the module's code block reaching its baseline JIT threshold at that point, after which the changed stack traffic happens to overwrite the slot.Cause
Bun.generateHeapSnapshot()shows the two objects held bylist, held by the finished async function'sAsyncFunctionGenerator, which has no retainer at all. Scanning the machine stack at everygatherFromCurrentThreadin a debug build finds exactly one word equal to that generator's address, at the same address for all 17 collections, and it is gone on the collection that finally frees them. Walking the frame chain puts that word inside the frame of JSC'srunInternalMicrotask: theAsyncFunctionResumecase wrote the generator into one of its locals while resumingmakeGarbagein an earlier turn; every later turn runs the module continuation (AsyncModuleExecutionResume) through the same native path, so the same frame lands at the same address, that local belongs to a switch arm that is not taken, and the conservative scan of the live frame sees the stale value on everyBun.gc(true).JSC relies on the embedder to clean this up between turns. Its own
sanitizeStackForVMcalls (allocation slow paths,llint_default_call/operationDefaultCall,Heap::runCurrentPhase) zero the stack between the last sanitize point and the current stack pointer, and all of them run inside JS, below the frames that hold a turn's values, so none of them can ever reach this region. In WebKit the task boundary does it: releasing the JSLock releases heap access, which relinquishes the GC conn, which sanitizes at the shallow depth of the run loop. Bun holds the lock and heap access for the life of the thread, and instead callsheap.stopIfNecessary()fromBun__JSC_onBeforeWait, which sanitizes as a side effect while the mutator holds the conn. That covers less than it looks like, in two ways:setImmediate(or by anything else that is always ready) never parks, so nothing shallow ever sanitizes. This is the case above. Two experiments pin it down:BUN_GC_RUNS_UNTIL_SKIP_RELEASE_ACCESS=0, which only disables thatstopIfNecessary()call, makes thesetTimeout-driven loop take exactly 17 GCs as well; and tracingsanitizeStackForVMin the failing case shows every call between collections happening at least 0x4500 bytes below the event loop frame while the stale slot sits 0x31f0 below it.stopIfNecessary()only sanitizes while the mutator holds the conn, and a synchronous collection (Bun.gc(true),collectSync) hands it back inHeap::waitForCollector. So after the first sync GC the hook stops sanitizing even on parking ticks, until some asynchronous collection takes the conn again. This is what GarbageCollectionController: replace per-tick heap sampler with idle timer only #35356 hit on Windows, where the hook does run on everyus_loop_run(its notes record that an explicitsanitizeStackForVMat the hook made the test pass).REVIEW.md's rule that this is never the scanner's fault holds here: the scanner is doing what it is documented to do, and the missing piece is Bun's, the sanitize between turns that every other embedder gets from its lock/access cycle.
Fix
Add
Bun__JSC_onLoopTick, which callsJSC::sanitizeStackForVMonce per tick, and call it fromus_loop_run_bun_tick(POSIX, before thenum_pollsearly-out) and fromus_loop_runandus_loop_pump(Windows), whether or not the tick parks.Bun__JSC_onBeforeWaitand #26821's zero-timeout poll are unchanged:stopIfNecessary()and the idle sweep still only run on ticks that park.Cost:
sanitizeStackForVMImplcompares the saved low-water mark against the stack pointer and returns when nothing ran deeper since the previous tick; otherwise it zero-fills the stack the turn actually used and records the new mark, after which JSC's own in-turn sanitizes zero nothing until the stack is deeper again, so the zeroing moves to the tick boundary rather than being added. WithBUN_JSC_verboseSanitizeStack=1on a 50-turnsetImmediatechain (debug build), the hook ran 48 times and zeroed anything on 8 of them.After this, anything dropped by a turn is gone by the GC that follows the next tick. The one case that takes two collections is garbage dropped in the timers phase being collected from the following tick's immediates phase, which runs before that tick's poll; after that GC the stack is sanitized down to the collector's own depth, so the collection after the next tick cannot see it. Measured (Linux debug, 15 to 20 runs per cell, identical values in every run; the same values on Windows): garbage made across
setImmediateorsetTimeoutturns, loop viasetImmediate: 1 and 2 GCs; loop viasetTimeout: 1 and 1. The original reproduction goes from 8 to 17 iterations to 1.Tests
test/js/web/timers/timer-gc-roots.test.ts: the 2x2 matrix above, asserting collection within 2 GCs. On main the twosetImmediate-loop cells fail deterministically (both objects still alive after 8 GCs; 5/5 runs on release, 3/3 on debug+ASAN); the twosetTimeoutcells pass on main and document that both primitives now behave the same.test/js/node/test/parallel/test-fs-filehandle.jsis restored unchanged from before GarbageCollectionController: replace per-tick heap sampler with idle timer only #35356, which deleted it because of this; it passes on Linux and Windows here. TheBun.gc(false); await setImmediatepreludes GarbageCollectionController: replace per-tick heap sampler with idle timer only #35356 added to four drain loops inbun-server.test.tsare no longer needed for the same reason, but are left alone in this PR.test/js/bun/gcandserve-body-leaksuites on Linux debug; the only failures are the same ones the unmodified tree has in this container (thesetTimeout/setIntervalleak fixtures, which key their ASAN threshold off the binary name, and four slow-machine timeouts inworker.test.ts). Built and ran the new test, the restored test and the reproduction on Windows x64.