diff --git a/src/jsc/VirtualMachine.rs b/src/jsc/VirtualMachine.rs index a0395b3e401c..7bea06592f22 100644 --- a/src/jsc/VirtualMachine.rs +++ b/src/jsc/VirtualMachine.rs @@ -292,8 +292,7 @@ pub struct VirtualMachine { pub rare_data: Option>, pub proxy_env_storage: crate::rare_data::ProxyEnvStorage, pub(crate) resolved_path_dups: Vec>, - pub pending_internal_promise: Option<*mut JSInternalPromise>, - pub pending_internal_promise_is_protected: bool, + pending_internal_promise: crate::strong::Optional, pub pending_internal_promise_reported_at: u32, pub(crate) hot_reload_deferred: bool, pub entry_point_result: EntryPointResult, @@ -3404,6 +3403,23 @@ impl VirtualMachine { } } + #[inline] + pub fn pending_internal_promise(&self) -> Option<*mut JSInternalPromise> { + self.pending_internal_promise.get()?.as_promise() + } + + /// Replace the entry-point promise while keeping the published pointer + /// rooted for as long as the VM slot holds it. + pub fn set_pending_internal_promise(&mut self, promise: *mut JSInternalPromise) { + debug_assert!(!promise.is_null()); + self.pending_internal_promise + .set(self.global(), JSValue::from_cell(promise)); + } + + pub(crate) fn clear_pending_internal_promise(&mut self) { + self.pending_internal_promise.clear_without_deallocation(); + } + /// `reloadEntryPoint(entry_path)` — set `main`, generate the synthetic /// `bun:main` entry, run preloads, and kick off module evaluation. pub(crate) fn reload_entry_point( @@ -3470,10 +3486,7 @@ impl VirtualMachine { // SAFETY: hook contract. let p = unsafe { (hooks.load_preloads)(self) }?; if !p.is_null() { - JSValue::from_cell(p).ensure_still_alive(); - JSValue::from_cell(p).protect(); - self.pending_internal_promise = Some(p); - self.pending_internal_promise_is_protected = true; + self.set_pending_internal_promise(p); return Ok(p); } } @@ -3481,8 +3494,7 @@ impl VirtualMachine { // Check if Module.runMain was patched. if self.has_patched_run_main { bun_core::hint::cold(); - self.pending_internal_promise = None; - self.pending_internal_promise_is_protected = false; + self.clear_pending_internal_promise(); let global_ref = self.global(); let argv1 = bun_string_jsc::create_utf8_for_js(global_ref, MAIN_FILE_NAME) .map_err(|_| crate::CrateError::JSError)?; @@ -3492,7 +3504,7 @@ impl VirtualMachine { .map_err(|_| crate::CrateError::JSError)?; // If the override stored a promise itself, use that; otherwise // wrap its return value. - if let Some(stored) = self.pending_internal_promise { + if let Some(stored) = self.pending_internal_promise() { return Ok(stored); } // `Promise.resolve(ret)` reads `ret.constructor` / `ret.then`, @@ -3501,8 +3513,7 @@ impl VirtualMachine { JSC__JSInternalPromise__resolvedPromise(global_ref, ret) }) .map_err(|_| crate::CrateError::JSError)?; - self.pending_internal_promise = Some(resolved); - self.pending_internal_promise_is_protected = false; + self.set_pending_internal_promise(resolved); return Ok(resolved); } } @@ -3529,9 +3540,7 @@ impl VirtualMachine { p }; - self.pending_internal_promise = Some(promise); - self.pending_internal_promise_is_protected = false; - JSValue::from_cell(promise).ensure_still_alive(); + self.set_pending_internal_promise(promise); Ok(promise) } else { self.entry_evaluation_started = false; @@ -3541,9 +3550,7 @@ impl VirtualMachine { jsc::JSModuleLoader::load_and_evaluate_module_ptr(global, Some(&main_str)) .map(NonNull::as_ptr) .ok_or(crate::CrateError::JSError)?; - self.pending_internal_promise = Some(promise); - self.pending_internal_promise_is_protected = false; - JSValue::from_cell(promise).ensure_still_alive(); + self.set_pending_internal_promise(promise); Ok(promise) } } @@ -3559,7 +3566,7 @@ impl VirtualMachine { // pending_internal_promise can change if hot module reloading is enabled if self.is_watcher_enabled() { loop { - let Some(p) = self.pending_internal_promise else { + let Some(p) = self.pending_internal_promise() else { break; }; // SAFETY: `p` is a live JSC heap cell tracked by the VM. @@ -3567,7 +3574,7 @@ impl VirtualMachine { break; } self.event_loop_mut().tick(); - let Some(p) = self.pending_internal_promise else { + let Some(p) = self.pending_internal_promise() else { break; }; // SAFETY: see above. @@ -3583,7 +3590,7 @@ impl VirtualMachine { let _ = self.wait_for_promise(jsc::AnyPromise::Internal(promise)); } - Ok(self.pending_internal_promise.unwrap_or(promise)) + Ok(self.pending_internal_promise().unwrap_or(promise)) } } @@ -4453,7 +4460,7 @@ impl VirtualMachine { /// After a hot reload, surfaces the entry-point promise's rejection (if any) and re-arms the watcher. pub fn report_exception_in_hot_reloaded_module_if_needed(&mut self) { - let promise = match self.pending_internal_promise { + let promise = match self.pending_internal_promise() { Some(p) => p, None => { self.add_main_to_watcher_if_needed(); @@ -4474,7 +4481,7 @@ impl VirtualMachine { let global_ref = self.global(); // `JSPromise` is an `opaque_ffi!` ZST handle; `opaque_mut` // is the centralised non-null deref proof (live JSC heap - // cell tracked by the VM's strong-ref slot). + // cell tracked by the VM's strong pending-entry slot). let result = crate::JSPromise::opaque_mut(promise).result(global_ref.vm()); let promise_js = JSValue::from_cell(promise); self.unhandled_rejection(global_ref, result, promise_js); @@ -4564,7 +4571,7 @@ impl VirtualMachine { bun_core::reload_process(should_clear_terminal, false); } - if let Some(p) = self.pending_internal_promise { + if let Some(p) = self.pending_internal_promise() { // SAFETY: `p` is a live JSC heap cell tracked by the VM. match crate::JSPromise::status_ptr(p) { crate::js_promise::Status::Pending => { @@ -4602,12 +4609,7 @@ impl VirtualMachine { // the JSC module loader registry. self.global().reload().expect("Failed to reload"); self.hot_reload_counter += 1; - if self.pending_internal_promise_is_protected { - if let Some(p) = self.pending_internal_promise { - JSValue::from_cell(p).unprotect(); - } - self.pending_internal_promise_is_protected = false; - } + self.clear_pending_internal_promise(); // reload_entry_point() stores into pending_internal_promise on every return path. let main = self.main; // Note: reshaped for borrowck — copy the `RawSlice` first to avoid @@ -5546,10 +5548,7 @@ impl VirtualMachine { // SAFETY: hook contract. let p = unsafe { (hooks.load_preloads)(self) }?; if !p.is_null() { - JSValue::from_cell(p).ensure_still_alive(); - self.pending_internal_promise = Some(p); - JSValue::from_cell(p).protect(); - self.pending_internal_promise_is_protected = true; + self.set_pending_internal_promise(p); return Ok(p); } } @@ -5561,9 +5560,7 @@ impl VirtualMachine { let promise = jsc::JSModuleLoader::load_and_evaluate_module_ptr(global, Some(&main_str)) .map(NonNull::as_ptr) .ok_or(crate::CrateError::JSError)?; - self.pending_internal_promise = Some(promise); - self.pending_internal_promise_is_protected = false; - JSValue::from_cell(promise).ensure_still_alive(); + self.set_pending_internal_promise(promise); Ok(promise) } @@ -5596,7 +5593,7 @@ impl VirtualMachine { // pending_internal_promise can change if hot module reloading is enabled if self.is_watcher_enabled() { loop { - let Some(p) = self.pending_internal_promise else { + let Some(p) = self.pending_internal_promise() else { break; }; // SAFETY: `p` is a live JSC heap cell tracked by the VM. @@ -5604,7 +5601,7 @@ impl VirtualMachine { break; } self.event_loop_mut().tick(); - let Some(p) = self.pending_internal_promise else { + let Some(p) = self.pending_internal_promise() else { break; }; // SAFETY: see above. @@ -5623,7 +5620,7 @@ impl VirtualMachine { // Pre-arm the waker so this settled-promise tick cannot park (#36450). self.wakeup(); self.auto_tick(); - Ok(self.pending_internal_promise.unwrap()) + Ok(self.pending_internal_promise().unwrap()) } /// Tracks a listening socket so watch-mode reloads can close it. @@ -5788,13 +5785,7 @@ impl VirtualMachine { self.entry_point_result.value.deinit(); self.entry_point_result.cjs_set_value = false; self.entry_point_result.evaluated_as_cjs = false; - if let Some(promise) = self.pending_internal_promise { - if self.pending_internal_promise_is_protected { - JSValue::from_cell(promise).unprotect(); - self.pending_internal_promise_is_protected = false; - } - self.pending_internal_promise = None; - } + self.clear_pending_internal_promise(); self.has_patched_run_main = false; self.set_main(b""); self.main_hash = 0; diff --git a/src/runtime/hw_exports.rs b/src/runtime/hw_exports.rs index e2236c2db093..6aa799534550 100644 --- a/src/runtime/hw_exports.rs +++ b/src/runtime/hw_exports.rs @@ -109,9 +109,8 @@ pub(crate) fn set_override_module_run_main_promise( vm: &mut VirtualMachine, promise: *mut JSInternalPromise, ) { - if vm.pending_internal_promise.is_none() { - vm.pending_internal_promise = Some(promise); - vm.pending_internal_promise_is_protected = false; + if vm.pending_internal_promise().is_none() { + vm.set_pending_internal_promise(promise); } } diff --git a/src/runtime/jsc_hooks.rs b/src/runtime/jsc_hooks.rs index e8425d5a5134..c2c29d03f648 100644 --- a/src/runtime/jsc_hooks.rs +++ b/src/runtime/jsc_hooks.rs @@ -708,8 +708,8 @@ unsafe fn load_preloads(vm: *mut VirtualMachine) -> bun_jsc::CrateResult<*mut JS // SAFETY: per fn contract — `vm` is the live per-thread VM. unsafe { (*vm).is_in_preload = true }; // Note: copy the raw ptr into a guard-owned local so the defer body - // doesn't borrow the fn param — later `(*vm).pending_internal_promise = …` - // would otherwise alias the guard's capture. + // doesn't borrow the fn param — later mutable VM access would otherwise + // alias the guard's capture. let vm_for_guard = vm; scopeguard::defer! { // SAFETY: per fn contract. @@ -820,7 +820,7 @@ unsafe fn load_preloads(vm: *mut VirtualMachine) -> bun_jsc::CrateResult<*mut JS }; // SAFETY: per fn contract. - unsafe { (*vm).pending_internal_promise = Some(promise) }; + unsafe { (*vm).set_pending_internal_promise(promise) }; let _protected = JSValue::from_cell(promise).protected(); // ── wait ──────────────────────────────────────────────────────── @@ -837,16 +837,20 @@ unsafe fn load_preloads(vm: *mut VirtualMachine) -> bun_jsc::CrateResult<*mut JS // SAFETY: `pending_internal_promise` was set just above (or // swapped by HMR to another live cell); `status()` is a // read-only FFI call on a live JSC heap cell. - let pip = unsafe { &*vm }.pending_internal_promise.unwrap_or(promise); + let pip = unsafe { &*vm } + .pending_internal_promise() + .unwrap_or(promise); // SAFETY: `pip` is a live JSC heap cell (set just above or - // the protected `promise` fallback). + // the `_protected` promise fallback). if unsafe { &*pip }.status() != PromiseStatus::Pending { break; } // SAFETY: `el` is the live per-thread event loop. unsafe { (*el).tick() }; // SAFETY: per fn contract — `vm` is the live per-thread VM. - let pip = unsafe { &*vm }.pending_internal_promise.unwrap_or(promise); + let pip = unsafe { &*vm } + .pending_internal_promise() + .unwrap_or(promise); // SAFETY: `pip` is a live JSC heap cell (see above). if unsafe { &*pip }.status() == PromiseStatus::Pending { // SAFETY: per fn contract — short-lived `&mut *vm` for the @@ -862,7 +866,8 @@ unsafe fn load_preloads(vm: *mut VirtualMachine) -> bun_jsc::CrateResult<*mut JS } } - // SAFETY: `promise` is a live (still-protected) JSC heap cell. + // SAFETY: `promise` is rooted by `_protected` even if HMR swapped the + // VM's pending-entry slot while the loop ran. if unsafe { &*promise }.status() == PromiseStatus::Rejected { return Ok(promise); } @@ -872,7 +877,7 @@ unsafe fn load_preloads(vm: *mut VirtualMachine) -> bun_jsc::CrateResult<*mut JS if !unsafe { &*vm }.script_allowed() { return Ok(core::ptr::null_mut()); } - // `_protected` drops here → unprotect. + // `_protected` drops here. } // Under --isolate each test file gets diff --git a/test/cli/hot/hot.test.ts b/test/cli/hot/hot.test.ts index 83d61eb602f3..7bf4f9413fe3 100644 --- a/test/cli/hot/hot.test.ts +++ b/test/cli/hot/hot.test.ts @@ -137,6 +137,129 @@ it("preload not found should exit with code 1 and not time out", async () => { expect(await new Response(runner.stderr).text()).toContain("preload not found"); }); +it( + "does not report handled rejections after hot reload collects the entry promise", + async () => { + const entry = join(cwd, "entry-promise-gc.js"); + const revisionFile = join(cwd, "entry-promise-gc-revision.js"); + // LTO can determine when the stale pointer stops being conservatively rooted. + // Repeat enough reloads to reliably force collection and cell reuse in release builds. + // Every measured run without the root failed within 12 boots. A debug boot takes about 4 seconds. + const totalBoots = isDebug ? 6 : 20; + writeFileSync(revisionFile, "export const revision = 0;\n"); + writeFileSync( + entry, + `import { revision } from "./entry-promise-gc-revision.js"; + +const state = (globalThis.__entryPromiseGC ??= { + boots: 0, + caught: new Set(), + owners: new Map(), + unhandled: [], +}); + +if (state.boots === 0) { + process.on("unhandledRejection", (reason, promise) => { + const failure = { + kind: "unhandled", + caught: state.caught.has(reason), + owned: state.owners.has(promise), + sameReason: state.owners.get(promise) === reason, + }; + state.unhandled.push(failure); + console.log(JSON.stringify(failure)); + }); +} + +const boot = ++state.boots; +setTimeout(async () => { + state.caught.clear(); + state.owners.clear(); + for (let wave = 0; wave < 3; wave++) { + Bun.gc(true); + const observed = []; + for (let i = 0; i < 5000; i++) { + const reason = new Error("caught-" + boot + "-" + wave + "-" + i); + const promise = Promise.reject(reason); + state.owners.set(promise, reason); + observed.push(promise.catch(error => state.caught.add(error))); + } + await Promise.all(observed); + await Bun.sleep(1); + } + console.log(JSON.stringify({ kind: "ready", boot, revision, unhandled: state.unhandled.length })); + if (state.unhandled.length > 0 || boot === ${totalBoots}) { + setTimeout(() => { + console.log(JSON.stringify({ kind: "result", boots: state.boots, unhandled: state.unhandled.length })); + process.exit(state.unhandled.length === 0 ? 0 : 70); + }, 0); + } +}, 0); +`, + ); + + await using runner = spawn({ + cmd: [bunExe(), "--hot", "--no-clear-screen", entry], + env: bunEnv, + cwd, + stdout: "pipe", + stderr: "pipe", + stdin: "ignore", + }); + + type Message = + | { kind: "unhandled"; caught: boolean; owned: boolean; sameReason: boolean } + | { kind: "ready"; boot: number; revision: number; unhandled: number } + | { kind: "result"; boots: number; unhandled: number }; + const messages: Message[] = []; + const driveReload = (async () => { + const decoder = new TextDecoder(); + let buffered = ""; + for await (const chunk of runner.stdout) { + buffered += decoder.decode(chunk, { stream: true }); + let newline; + while ((newline = buffered.indexOf("\n")) !== -1) { + const line = buffered.slice(0, newline); + buffered = buffered.slice(newline + 1); + if (!line) continue; + const message = JSON.parse(line) as Message; + messages.push(message); + if (message.kind === "ready" && message.boot < totalBoots) { + writeFileSync(revisionFile, `export const revision = ${message.boot};\n`); + } + } + } + buffered += decoder.decode(); + if (buffered) messages.push(JSON.parse(buffered) as Message); + })(); + + const [, stderr, exitCode] = await Promise.all([driveReload, new Response(runner.stderr).text(), runner.exited]); + const unhandled = messages.filter(message => message.kind === "unhandled"); + const ready = messages.filter(message => message.kind === "ready"); + const result = messages.find(message => message.kind === "result"); + + expect({ + exitCode, + // A debug build prints this line on every reload. + stderr: stderr.split("\n").filter(line => line !== "" && line !== "DEBUG: Reloading..."), + unhandled, + readyCount: ready.length, + readySequenceIsValid: ready.every( + (message, index) => message.boot === index + 1 && message.revision === index && message.unhandled === 0, + ), + result, + }).toEqual({ + exitCode: 0, + stderr: [], + unhandled: [], + readyCount: totalBoots, + readySequenceIsValid: true, + result: { kind: "result", boots: totalBoots, unhandled: 0 }, + }); + }, + longTimeout, +); + it( "should hot reload when file is overwritten", async () => { @@ -192,6 +315,81 @@ it( timeout, ); +it( + "should keep reloading after a GC runs between reloads", + async () => { + // Once the entry point's load promise settles, the only reference left to + // it is the VM slot the --hot loop polls on every tick. After a reload, + // collect that promise from an fs callback and fill the heap with pending + // promises so its cell is reused. If the slot does not root the promise, + // it now reads "pending" and every later reload is deferred forever. + const root = join(cwd, "hot-entry-promise-gc.js"); + const source = ` + import { readFile } from "fs"; + globalThis.counter ??= 0; + const n = ++globalThis.counter; + console.write(\`[#!root] Reloaded: \${n}\\n\`); + if (n > 1) { + readFile(import.meta.path, () => { + Bun.gc(true); + globalThis.pending = Array.from({ length: 20_000 }, () => new Promise(() => {})); + console.write(\`[#!root] gc done: \${n}\\n\`); + }); + } + setTimeout(() => {}, 9_999_999); + `; + writeFileSync(root, source); + + await using runner = spawn({ + cmd: [bunExe(), "--hot", "run", root], + env: bunEnv, + cwd, + stdout: "pipe", + stderr: "pipe", + stdin: "ignore", + }); + + let stdout = ""; + let stderr = ""; + const drained = Promise.all([ + (async () => { + for await (const chunk of runner.stdout) stdout += new TextDecoder().decode(chunk); + })(), + (async () => { + for await (const chunk of runner.stderr) stderr += new TextDecoder().decode(chunk); + })(), + ]); + const reloads = () => [...stdout.matchAll(/\[#!root\] Reloaded: (\d+)\n/g)].map(m => Number(m[1])); + const stepTimeout = isDebug ? 120_000 : 20_000; + async function waitForOutput(what: string, done: () => boolean) { + const deadline = Date.now() + stepTimeout; + while (!done()) { + const exit = runner.exitCode ?? runner.signalCode; + if (exit !== null) { + throw new Error(`child exited (${exit}) while waiting for ${what}\nstdout:\n${stdout}\nstderr:\n${stderr}`); + } + if (Date.now() > deadline) { + throw new Error(`timed out waiting for ${what}\nstdout:\n${stdout}\nstderr:\n${stderr}`); + } + await Bun.sleep(20); + } + } + + await waitForOutput("the first load", () => reloads().length > 0); + writeFileSync(root, source); + // A save can fire more than one watcher event, so count reloads instead of + // expecting exactly one. + await waitForOutput("the GC after a reload", () => stdout.includes("[#!root] gc done: ")); + const before = Math.max(...reloads()); + writeFileSync(root, source); + await waitForOutput(`a reload after reload ${before}`, () => Math.max(...reloads()) > before); + + runner.kill(); + await drained; + }, + longTimeout, +); + it.each(["hot-file-loader.file", "hot-file-loader.css"])( "should hot reload when `%s` is overwritten", async (targetFilename: string) => { diff --git a/test/cli/test/bun-test.test.ts b/test/cli/test/bun-test.test.ts index 49a4905e9b14..1011f7264ef8 100644 --- a/test/cli/test/bun-test.test.ts +++ b/test/cli/test/bun-test.test.ts @@ -2092,3 +2092,31 @@ describe.concurrent("test file discovery (scanner)", () => { expect(exitCode).toBe(0); }); }); + +// The preload runs a full collection at the start of every event loop iteration, so one +// runs after the load promise rejects and before its BuildMessage is printed. Without a +// root on that promise, an optimized ASAN build reports a read of the freed BuildMessage. +// A debug build keeps the promise in a stack slot and passes either way. +test("prints a test file's build error when a GC runs before it is reported", async () => { + using dir = tempDir("bun-test-build-error-gc", { + "gc.cjs": `(function loop() { Bun.gc(true); setImmediate(loop); })();`, + "broken.test.mjs": ` + import { test } from "bun:test"; + console.log(typeof module, typeof exports); + test("t", () => {}); + `, + }); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "test", "--preload", "./gc.cjs", "./broken.test.mjs"], + env: bunEnv, + cwd: String(dir), + stdout: "ignore", + stderr: "pipe", + }); + const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); + + expect(stderr).toContain("error: Cannot use import statement with CommonJS-only features"); + expect(stderr).toContain(" 1 error"); + expect(exitCode).toBe(1); +});