Skip to content

fix(fetch): remove latent cross-thread UB in FetchTasklet shutdown#30943

Open
DagmawiKK wants to merge 1 commit into
oven-sh:mainfrom
DagmawiKK:claude/fetch-shutdown-leak
Open

fix(fetch): remove latent cross-thread UB in FetchTasklet shutdown#30943
DagmawiKK wants to merge 1 commit into
oven-sh:mainfrom
DagmawiKK:claude/fetch-shutdown-leak

Conversation

@DagmawiKK
Copy link
Copy Markdown

What does this PR do?

Removes a latent data race and JSC thread-safety violation in FetchTasklet::deref_from_thread during VM shutdown.

Currently, if is_shutting_down() is true, deref_from_thread synchronously calls FetchTasklet::deinit(this) on the background HTTP thread. This is Undefined Behavior because it destroys jsc::Weak handles and modifies single-threaded structures (Response::unref(response) mutates a Cell<u32>, and readable_stream_ref.deinit() drops a ReadableStream::Strong) off the main JS thread.

This branch is currently unreachable dead code because the JS thread leaks its initial reference when the event loop is halted during process.exit. However, if Bun were to implement graceful shutdown, eager GC for aborted fetches, or clean up the shutdown memory leak, the HTTP thread would become the final reference holder, triggering a guaranteed segfault.

The fix replaces the synchronous deinit call with an intentional leak (explicit abandonment). If the VM is shutting down, the HTTP thread will simply return and safely abandon the allocation for the OS to reclaim, avoiding the cross-thread UB.

How did you verify your code works?

  • bun bd test test/js/web/fetch/exiting.test.ts runs 500 concurrent fetches with process.exit(0); passes with exit code 0 and no panic
  • Traced the reference lifecycle of FetchTasklet (initial refs, deref_from_thread drops, JS thread drops in on_progress_update) to confirm this code is currently dead but dangerous.
  • Confirmed with standard JSC practices that explicitly abandoning/leaking the memory during VM shutdown is the correct and safest approach to avoid cross-thread heap corruption.
  • Ensured it compiles successfully locally.

Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

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
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 17, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ba4be777-4d1d-4bf4-ba5d-69005afc7af9

📥 Commits

Reviewing files that changed from the base of the PR and between 172afa5 and c8fb9c4.

📒 Files selected for processing (1)
  • src/runtime/webcore/fetch/FetchTasklet.rs

Walkthrough

The pull request modifies FetchTasklet::deref_from_thread to prevent undefined behavior during VM shutdown. When the VM is shutting down and deref is called from the background HTTP thread, the code now intentionally leaks the allocation instead of calling synchronous cleanup, avoiding destruction of JS-bound handles off the main JavaScript thread.

Changes

Fetch Tasklet VM Shutdown Handling

Layer / File(s) Summary
VM-shutdown deref path avoids off-thread JS cleanup
src/runtime/webcore/fetch/FetchTasklet.rs
When javascript_vm.is_shutting_down() is true in the HTTP-thread deref path, FetchTasklet::deinit(this) is removed and replaced with intentional leak comments and early return, preventing destruction of JS-bound handles off the main JS thread.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely summarizes the main change: fixing undefined behavior in FetchTasklet shutdown by removing cross-thread destruction.
Description check ✅ Passed The PR description comprehensively covers both required template sections with detailed technical context, implementation rationale, and verification steps.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

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.

1 participant