-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Restore smol-gated retain-with-limit reset for the module loader transpile arena #31855
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
45f0982
cda54bc
74a487f
8502792
e3719e8
53c583d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import { expect, test } from "bun:test"; | ||
| import { isASAN, isDebug } from "harness"; | ||
| import { bunEnv, bunExe, isASAN, isDebug, tempDir } from "harness"; | ||
|
|
||
| const asanIsSlowMultiplier = isASAN ? 0.2 : 1; | ||
| const count = Math.floor(10000 * asanIsSlowMultiplier); | ||
|
|
@@ -34,6 +34,101 @@ test( | |
| isDebug || isASAN ? 20_000 : 5000, | ||
| ); | ||
|
|
||
| // ModuleLoader::reset_arena: --smol destroys the transpile arena every cycle; | ||
| // otherwise it retains the warm heap under an 8 MiB cap and recycles when over. | ||
| // The over-cap branch is only reachable via the parse-error path (success | ||
| // resets the arena before parking it), hence the oversized broken modules. | ||
| // Debug builds assert the branch taken via the BUN_DEBUG_ModuleLoader log. | ||
| for (const smol of [false, true]) { | ||
| test( | ||
| `transpile arena reset policy (${smol ? "--smol" : "default"})`, | ||
| async () => { | ||
| const iters = isASAN || isDebug ? 50 : 200; | ||
| const brokenCount = isASAN || isDebug ? 1 : 2; | ||
|
|
||
| // 150k statements pushes the transpile arena well past the 8 MiB cap. | ||
| const bigLines: string[] = []; | ||
| for (let i = 0; i < 150_000; i++) { | ||
| bigLines.push(`const v${i} = ${i};`); | ||
| } | ||
| const bigValid = bigLines.join("\n") + "\nexport const sum = v0 + v149999;"; | ||
| // Syntax error at the end so the full AST is in the arena before failing. | ||
| const bigBroken = bigLines.join("\n") + "\n}"; | ||
|
|
||
| const files: Record<string, string> = { | ||
| "big_valid.ts": bigValid, | ||
| "driver.ts": ` | ||
| let total = 0; | ||
| let caught = 0; | ||
| for (let i = 0; i < ${iters}; i++) { | ||
| // require(), not import(): dynamic import skips the synchronous | ||
| // arena reset (concurrent transpiler store). | ||
| const m = require("./small_" + i + ".ts"); | ||
| total += m.value; | ||
| if (i % 10 === 0) Bun.gc(true); | ||
| } | ||
| for (let i = 0; i < ${brokenCount}; i++) { | ||
| try { | ||
| require("./big_broken_" + i + ".ts"); | ||
| } catch { | ||
| caught++; | ||
| } | ||
| Bun.gc(true); | ||
| } | ||
| total += require("./big_valid.ts").sum; | ||
| Bun.gc(true); | ||
| console.log("total=" + total + " caught=" + caught); | ||
| `, | ||
| }; | ||
| for (let i = 0; i < iters; i++) { | ||
| files[`small_${i}.ts`] = `export const value: number = 1;\n`; | ||
| } | ||
| for (let i = 0; i < brokenCount; i++) { | ||
| files[`big_broken_${i}.ts`] = bigBroken; | ||
| } | ||
|
|
||
| using dir = tempDir("transpile-arena-reset", files); | ||
|
|
||
| const cmd = [bunExe()]; | ||
| if (smol) cmd.push("--smol"); | ||
| cmd.push("driver.ts"); | ||
|
|
||
| await using proc = Bun.spawn({ | ||
| cmd, | ||
| // The scoped log is compiled out of release builds. | ||
| env: isDebug ? { ...bunEnv, BUN_DEBUG_ModuleLoader: "1" } : bunEnv, | ||
| cwd: String(dir), | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(stdout).toContain(`total=${iters + 149999} caught=${brokenCount}`); | ||
| if (isDebug) { | ||
| const logs = stdout + stderr; | ||
| const occurrences = (needle: string) => logs.split(needle).length - 1; | ||
| if (smol) { | ||
| expect(occurrences("reset_arena: free_all")).toBeGreaterThanOrEqual(iters + brokenCount); | ||
| expect(occurrences("reset_arena: retained")).toBe(0); | ||
| expect(occurrences("reset_arena: recycled")).toBe(0); | ||
| } else { | ||
| // Each oversized parse failure must trip the over-cap recycle; if the | ||
| // broken fixture stops clearing the cap, this fails rather than | ||
| // silently losing branch coverage. | ||
| expect(occurrences("reset_arena: retained")).toBeGreaterThanOrEqual(iters); | ||
| expect(occurrences("reset_arena: recycled")).toBeGreaterThanOrEqual(brokenCount); | ||
| expect(occurrences("reset_arena: free_all")).toBe(0); | ||
| } | ||
| } else { | ||
| expect({ stderr, exitCode }).toEqual({ stderr: "", exitCode: 0 }); | ||
| } | ||
|
Comment on lines
+107
to
+125
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-debug runs don’t actually assert the reset-policy regression. Outside debug builds this only checks the happy-path totals plus As per coding guidelines, "Verify your test fails with 🤖 Prompt for AI Agents
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is intentional: the branch taken (free_all vs retained/recycled) is only observable via the BUN_DEBUG_ModuleLoader scoped logger, which is compiled out of release builds — there is nothing for a release binary to assert beyond clean output and exit 0, so the non-debug branch is a smoke test. The arena-policy assertions run on every debug lane in CI.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
✏️ Learnings added
🧠 Learnings used |
||
| expect(exitCode).toBe(0); | ||
| }, | ||
| isDebug || isASAN ? 120_000 : 30_000, | ||
| ); | ||
| } | ||
|
|
||
| test(`load the same empty JS file ${count} times`, async () => { | ||
| const prev = Bun.unsafe.gcAggressionLevel(); | ||
| Bun.unsafe.gcAggressionLevel(0); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 The 53c583d commit message says the 30205 LSan flake "is now handled by releasing queued NapiFinalizerTasks in
__bun_release_task_at_shutdown", but this arm runs atrelease_queued_tasks_for_shutdown(VirtualMachine.rs:1579) — beforedestructOnExit'scollectNow()(:1581) where your e3719e8 root-cause says the leaked allocations originate, and on thebun testexit pathon_exit()is never called soschedule()routes those boxes intorare_data.cleanup_hooks(napi_body.rs:4280-4285), notevent_loop.tasks. The arm itself is fine, but it likely doesn't cover the path the revertedsetHasTerminationRequest()fix did — worth re-running 30205.test.ts on x64-asan against 53c583d (the robobun results above are for e3719e8).Extended reasoning...
What changed and what it claims
53c583d reverted the e3719e8
vm.setHasTerminationRequest()line inZig__GlobalObject__destructOnExit(because it trippedvalidateIsNotSweepingintest_cannot_run_js) and added thistask_tag::NapiFinalizerTaskarm to__bun_release_task_at_shutdown, with the commit/PR comment: "The leak is now handled by releasing queued NapiFinalizerTasks in__bun_release_task_at_shutdowninstead."The arm itself is correct and harmless — it drops a
Box<NapiFinalizerTask>and releases itsNapiEnvRefwhile the env is still alive, symmetric with the adjacentFetchTasklet/AsyncFSTaskarms. The concern is only whether it actually intercepts the allocations LSan flagged in 30205.Ordering: this arm runs before the leak point
__bun_release_task_at_shutdownis reached only viaEventLoop::release_queued_tasks_for_shutdown(event_loop.rs:801), called at VirtualMachine.rs:1579.Zig__GlobalObject__destructOnExit— and its finalcollectNow()— runs at :1581, after the drain. The author's own e3719e8 root-cause states the leakedNapiFinalizerTaskboxes are allocated "innapi_internal_enqueue_finalizerduringdestructOnExit's finalcollectNow()". Anything allocated there is after this arm has already walkedevent_loop.tasks.Where those boxes actually go on the
bun testexit path30205.test.ts runs under
bun test --isolate. The test-runner exit paths (test_command.rs:2942,parallel/runner.rs:712) setvm.is_shutting_down = trueand callglobal_exit()without callingon_exit(), sohas_run_cleanup_hooksstaysfalse. DuringdestructOnExit'scollectNow(),NapiFinalizerTask::schedule()(napi_body.rs:4266-4289) therefore evaluates:is_main_thread→ true (sweep runs on the JS thread)vm.is_shutting_down()→ truevm.has_run_cleanup_hooks()→ false…and takes the
push_cleanup_hookbranch at :4280-4285 —heap::into_raw(self)is stashed as a rawctxpointer inrare_data.cleanup_hooks. That list is never walked again (on_exit()never ran), and whenrare_datais dropped during teardown theVec<CleanupHook>storage is freed but the rawctxpointers are orphaned → LSan direct leak, matching the reportedDirect leak of 32000 byte(s) in 1000 object(s). The new arm drainsevent_loop.tasks; it never touchesrare_data.cleanup_hooks.This also resolves the apparent contradiction the refutation raised: e3719e8's
setHasTerminationRequest()worked because it flipsmustDeferFinalizers()tofalse, so finalizers ran inline during the sweep andnapi_internal_enqueue_finalizer→schedule()was never reached — a different mechanism that bypassed thepush_cleanup_hookpath entirely. And it explains why LSan reported a direct leak rather than "reachable via static-rooted VM": the boxes weren't inself.tasks, they were behind raw pointers in aVecthat gets freed.What about tasks enqueued before shutdown?
For
NapiFinalizerTasks that landed inevent_loop.tasksvia theenqueue_taskbranch (:4288) beforeis_shutting_downflipped — the only window the new arm does cover — without this arm they'd returnfalse, get re-queued (event_loop.rs:810-816), and stay inself.tasks, a field of the never-dealloc'd static-rooted VM box. Per the codebase's own comments (event_loop.rs:796-800, 822-827) LSan treats those as reachable and does not flag them. So the arm changes nothing LSan-observable on that path either.Step-by-step proof for 30205
bun test --isolatefinishes;test_command.rs:2942setsis_shutting_down=trueand callsglobal_exit().on_exit()is not called →has_run_cleanup_hooks=false.global_exit()reaches VirtualMachine.rs:1579:release_queued_tasks_for_shutdown()walksevent_loop.tasks. AnyNapiFinalizerTaskalready there is dropped by the new arm. ✅Zig__GlobalObject__destructOnExit→collectNow(). GC sweeps 1000NapiRef-backed objects; each callsnapi_internal_enqueue_finalizer→Finalizer::enqueue→NapiFinalizerTask::schedule().schedule():is_shutting_down=true,has_run_cleanup_hooks=false→heap::into_raw(self)pushed intorare_data.cleanup_hooksas a rawctxpointer.__bun_release_task_at_shutdownarm already ran in step 2; it never sees these.rare_datais dropped; the 1000 rawctxpointers are orphaned. LSan:Direct leak of 32000 byte(s) in 1000 object(s)fromnapi_internal_enqueue_finalizer— same stack the author observed.Impact & suggestion
The 30205 flake is pre-existing (the author confirmed it reproduces on a control binary without the arena change), so this PR doesn't regress anything — hence nit. But the verified-working fix was reverted and the stated replacement operates on a different queue at an earlier point than where the analysis (and the author's own root-cause) places the leak. The robobun results in this PR are for e3719e8, not 53c583d. Worth either re-verifying 30205.test.ts on x64-asan against 53c583d, or — if the
push_cleanup_hookpath is indeed the culprit — having thebun testexit path sethas_run_cleanup_hooks=true(or callon_exit()) soschedule()takes thedrop(self)branch at :4276 instead.