Skip to content

hot: root the entry-point promise on VirtualMachine - #44233

Open
mq1n wants to merge 1 commit into
oven-sh:mainfrom
mq1n:hot-entry-promise-root
Open

mq1n wants to merge 1 commit into
oven-sh:mainfrom
mq1n:hot-entry-promise-root

Conversation

@mq1n

@mq1n mq1n commented Sep 29, 2026

Copy link
Copy Markdown

What does this PR do?

Fix a use-after-free in the entry-point promise polled by the --hot loop.

VirtualMachine.pending_internal_promise was a raw JSInternalPromise pointer. Once entry-point evaluation settled, the module loader could release the last GC edge while the hot loop continued reading that pointer. After the cell was collected and reused, the hot-only reporter could interpret an unrelated, already-handled rejected promise as the entry-point promise and emit it through unhandledRejection.

Store the value in a VM-owned strong::Optional and route every read, replacement, reload clear, and test-isolation clear through that rooted slot. The local preload guard remains in place because HMR can replace the VM slot while that function still uses its original promise.

The regression test completes entry-module evaluation before the application work starts, forces GC and promise-cell reuse across 100 reloads, and verifies that caught rejections are never reported as unhandled. Genuine unhandled application rejections and genuine rejected entry modules are still reported; this does not add an isHandled() filter to the hot reporter.

This implements the VM-owned Strong alternative discussed in #41146 on current main. #41146 first identified the dangling entry-promise pointer; the reproduction here additionally observes the stale pointer aliasing the exact caught Promise and Error. Keeping the root on VirtualMachine also follows REVIEW.md's guidance for per-VM state and avoids adding a field to ZigGlobalObject.

How did you verify your code works?

  • macOS arm64, Bun 1.4.2 release ThinLTO build:
    • baseline 100-reload regression: failed 3/3 runs; the emitted event had caught=true, owned=true, and sameReason=true
    • patched targeted regression: passed (100 reloads)
    • patched independent two-boot stress runs: 500/500 passed
    • patched test/cli/hot/hot.test.ts: 13/13 passed, 467 assertions
    • negative controls: a deliberately unhandled application promise and a genuinely rejected hot entry module were both still reported
  • current main:
    • cargo check -p bun_runtime
    • cargo fmt -p bun_jsc -p bun_runtime -- --check
    • Prettier check for test/cli/hot/hot.test.ts
    • Windows x64 debug build completed all compile, link, export, dependency, hardening, revision, and duplicate-symbol checks

The hot loop keeps polling the entry-point evaluation promise after module evaluation settles. A raw pointer can outlive its last GC edge and later alias an unrelated handled rejection after the cell is reused. Store the promise in a VM-owned strong slot and route every read and write through that rooted owner. Add a GC-stress hot-reload regression that verifies handled rejections stay handled.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@coderabbitai

coderabbitai Bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: oven-sh/bun/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 759a0004-6a8b-4a65-bbb3-2e780310bbb9

📥 Commits

Reviewing files that changed from the base of the PR and between 9f70da0 and aff39a5.

📒 Files selected for processing (4)
  • src/jsc/VirtualMachine.rs
  • src/runtime/hw_exports.rs
  • src/runtime/jsc_hooks.rs
  • test/cli/hot/hot.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


Walkthrough

The VM replaces its public pending-promise pointer and protection flag with a private rooted slot. Entry loading, preload handling, hot reload, and test-runner paths use slot accessors. A hot-reload regression test checks for unhandled rejections across repeated boots and garbage collection.

Changes

Pending promise tracking

Layer / File(s) Summary
Rooted slot and entry loading
src/jsc/VirtualMachine.rs, src/runtime/hw_exports.rs, src/runtime/jsc_hooks.rs
The VM stores the pending promise in a private rooted slot and exposes accessors to read, set, and clear it. Entry loading, preload handling, and the Module.runMain override use those accessors.
Hot-reload promise tracking
src/jsc/VirtualMachine.rs, src/runtime/jsc_hooks.rs
Hot-reload polling and rejection handling read and clear the pending promise through the accessors. The preload watcher checks the current slot around event-loop ticks and falls back to the original promise.
Test-runner tracking and regression test
src/jsc/VirtualMachine.rs, test/cli/hot/hot.test.ts
Test-runner preload, polling, return, and isolation cleanup use the accessors. The hot-reload test runs 100 boots with repeated promise rejection and garbage-collection waves, then checks for unhandled rejections.

Suggested reviewers: jarred-sumner

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to aff39

This change makes the hot-reload entry promise safer by keeping it rooted through garbage collection. No actionable merge-blocking issue was identified, and a regression test covers repeated reloads.

Security Architecture Review

Security architecture risk: 🔵 Low · up to aff39

Rooting the entry promise addresses a hot-reload lifetime hazard. The remaining design question is whether worker shutdown fully releases the new VM-owned root.

Retained concerns

  • Low · architecture · inferred: Worker teardown does not explicitly release the newly VM-owned strong slot before raw VM deallocation. Whether JSVM shutdown reclaims its root block is unresolved; a persistent leak is not established.
Security review details

Security Blast Radius

  • inferred — The changed ownership applies to promises from entry loading, overrides, hot reload, and test-runner loading within a VM; inspected callers do not show a new privilege or cross-service boundary.

Trust Boundaries and Controls

  • observed — The override helper stores a supplied promise only when the VM slot is empty; hot-reload reporting continues to consume the VM-selected pending promise.

Resilience and Maintainability Implications

  • inferred — Rooting the selected promise reduces stale-cell interpretation during hot reload, while the final ownership transition at worker shutdown remains unresolved.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description includes both required sections. It clearly explains the use-after-free fix, implementation approach, regression coverage, verification results, and retained behavior for genuine unhan…
Title check ✅ Passed The title clearly and concisely describes the main change: rooting the entry-point promise on VirtualMachine.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@mq1n

mq1n commented Sep 29, 2026

Copy link
Copy Markdown
Author

Downstream validation update (2026-09-29):

  • macOS 26.2 arm64, release ThinLTO Bun 1.4.2: the unpatched 100-reload forced-GC regression failed in 3/3 runs. The backport of this PR passed 500/500 independent two-boot probes and the full test/cli/hot/hot.test.ts file (13 tests, 467 assertions). Ulak's original two-case --hot sequence then passed 50/50, and its full slow suite passed with 0 failures (283 pass, 28 platform skips).
  • WSL2 Ubuntu 24.04 x64, release ThinLTO from the same backport: Ulak's original sequence passed 50/50, and the full slow suite passed with 0 failures (282 pass, 29 platform skips).
  • Both binaries report 1.4.2-canary.1+744846f84. The downstream crash handler was unchanged. Its intentional genuine-unhandled-rejection control still exits 70, and Bun's genuine entry-module rejection coverage still passes.

On the retained worker-teardown question from the automated architecture review: strong::Optional stores its handle in the VM-owned StrongRootBlock, the same ownership mechanism already used by overridden_main. Strong deliberately skips individual slot deletion after vm.is_shutting_down() because VM destruction releases the root block and client data as part of heap teardown; the slot cannot outlive its containing VirtualMachine. An explicit worker-teardown clear would call root bookkeeping during JSC teardown without changing that lifetime, so I have left the VM-owned root to VM destruction.

@robobun

robobun commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator

Thank you for this PR, and for the handled-rejection case. It reproduces here as well: on a Linux x64 release build of main without a root, your test reports a caught rejection as unhandled in 7 of 7 runs, within 12 boots.

CI for this PR is still blocked, so I carried your commit unchanged on #41146 (449404e, under your name) to run it there. These are the results. They apply to this PR because the source change is the same.

The root fixes the bug. On release ASAN builds of main at ad60a9b with and without your commit, your test, the test "should keep reloading after a GC runs between reloads" and the bun test route test of #41146 all fail without the commit and pass with it.

Two existing tests fail with it on every lane that runs them (build 121863):

  • test/js/web/timers/timer-gc-roots.test.ts, "heapStats still reports protected Timeout counts": 2 StrongRootBlocks remain after the timers are cleared, and the test allows 1.
  • test/js/bun/module-graph/module-graph-isolation.test.ts, "a Response whose body was still arriving ...": the fixture prints protectedPromises: 1, and the test expects 0.

The slot holds the entry promise for the life of the process, so one Strong slot stays occupied at rest and heapStats counts it. The two ASAN builds confirm it: both tests fail with the commit and pass without it.

Three facts about the test in this PR:

  • On a debug build with the fix, all 100 boots are correct, but stderr: "" fails. A debug build prints DEBUG: Reloading... on each reload (99 lines). The test takes 382 s and 417 s there.
  • On a release ASAN build in the CI lane's environment, 100 boots take 39 to 49 seconds. The timeout of the file for non-debug builds is 30 seconds, and the test timed out in 3 of 3 runs.
  • With 20 boots on hot: keep the entry-point promise alive while the reload loop polls it #41146, the sequence check failed on the first attempt on three lanes (debian 13 x64, alpine 3.23 x64 and aarch64). No rejection was reported. One save caused more than one reload (21 and 23 boots for 20 saves), and the test requires boot N to carry revision N-1.

The details and the April history of a Strong on this field are in this comment on #41146. Which PR carries the fix is a maintainer's decision, not mine.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants