jsc: require T: Send/T: Sync for JsCell<T>'s Send/Sync impls

[robobun]

Closes #31498.

What

JsCell<T> (src/jsc/JSCell.rs) is a #[repr(transparent)] wrapper over UnsafeCell<T> whose get(&self) -> &T is a safe function. Its Send/Sync impls were unconditional:

unsafe impl<T> Sync for JsCell<T> {}   // no T: Sync
unsafe impl<T> Send for JsCell<T> {}   // no T: Send

Because get() is safe and hands out &T, an unconditional Sync lets a &JsCell<T> cross threads (via any Sync container) and surface a &T to a !Sync T (Rc, Cell, RefCell) on another thread — UB with zero unsafe at the call site. Anyone writing JsCell<Rc<…>> directly, or through a struct field, would break the single-JS-thread model with no unsafe marker. This is location #1 from the issue, and the one the reporter flagged as the real leak (its dangerous surface is a public, safe fn).

Cause

The single-JS-thread invariant is a runtime contract, not something the unconditional impls enforce. The Sync impl is a deliberate, documented escape hatch so &'static VirtualMachine (which embeds JsCell fields) satisfies 'static trait objects without hand-written impls cascading everywhere — but it was stated as <T> rather than <T: Sync>, so it covered !Sync inner types it never should have.

Fix

Gate each impl on the corresponding inner bound:

unsafe impl<T: Sync> Sync for JsCell<T> {}
unsafe impl<T: Send> Send for JsCell<T> {}

This keeps the escape hatch intact — VirtualMachine carries its own unsafe impl Sync/Send and is unaffected, and every other JsCell<!Sync> field in the tree (JsCell<Strong>, JsCell<KeepAlive>, JsCell<EventLoopTimer>, JsCell<Vec<u8>>, …) lives in a struct that is already !Sync via a Cell/RefCount field and is never required to be Send. A JsCell<!Sync>/JsCell<!Send> is now correctly !Sync/!Send, so such a field can no longer silently make its container cross-thread-shareable. The full bun bd build stays green (this was the open question — it compiles workspace-wide).

ConcurrentPromiseTask / WorkTask (issue locations #2 and #3)

The issue proposed adding a Send bound (on the trait or the unsafe impl) for these, on the premise that "every implementor is already Send." That premise does not hold — I tried it and the build fails. All four/three implementors are genuinely !Send:

• TransformTask, WalkTask, PipelineTask hold a &JSGlobalObject (an opaque !Sync FFI handle) plus JS-heap raw pointers.
• GetAddrInfoRequest holds *mut DNSLookup / *mut Resolver.

These fields are JS-thread-bound and are touched by then() back on the JS thread; the worker only runs run(), which operates on the thread-safe subset. Fields that genuinely cross to the worker are already wrapped at the field level in bun_jsc::ThreadSafe (e.g. TransformTask::input_code). A C: Send bound would reject this correct-by-construction design. These unsafe impl Sends are the same class of audited escape hatch as JsCell / VirtualMachine, so they keep the hand-written impl and I expanded their SAFETY docs to name the single-JS-thread contract and the strict schedule → run_from_thread_pool → on_finish → run_from_js sequencing, rather than adding a bound that breaks compilation.

Verification

• Full bun bd debug build is green with the tightened JsCell bounds — the JsCell<!Send/!Sync>-heavy modules (Response, Request, FileSink/FileReader, sockets, valkey, dns) all compile.
• Smoke-tested the two task-wrapper paths the changed SAFETY comments cover: async Bun.Transpiler().transform() (ConcurrentPromiseTask<TransformTask>) and dns.lookup (WorkTask<GetAddrInfoRequest>) — both work.
• Regression guard: a co-located compile-time proof module (jscell_send_sync_proof) asserts both directions — JsCell<u32> stays Send + Sync, and JsCell<Rc<u32>> is !Send + !Sync via the auto-trait-ambiguity trick (same shape as src/runtime/shell/subproc.rs's __pipe_reader_thread_confined). I confirmed with a standalone rustc that this negative assertion fails to compile under the old unconditional impls (E0283, "multiple impls satisfying JsCell<Rc<u32>>: …") and passes under the tightened bounds. It is not behind #[cfg(test)], so it compiles on every build and rust:check/rust:clippy run — reverting the bound turns CI red.

Tests

test/internal/jscell-send-sync-bounds.test.ts checks the invariant with the compiler, since it has no runtime surface: it cargo checks a throwaway crate that depends on the real bun_jsc and asserts JsCell<Rc<u32>> is !Send + !Sync via the auto-trait-ambiguity trick. cargo check does not link (no C++/JSC objects needed — only the codegen the debug build already emitted), so it's cheap and self-contained. Verified fail-before/pass-after: with the old unconditional impls JsCell<Rc<u32>> is Send + Sync, both ambiguity blankets apply, and the check fails with E0283; with the tightened bounds it passes. The test skips on lanes without the debug codegen / cargo / pinned toolchain (e.g. release-only), and runs fully offline.

A co-located compile-time proof (jscell_send_sync_proof in JSCell.rs, same shape as src/runtime/shell/subproc.rs's __pipe_reader_thread_confined) additionally guards both directions on every build, so reverting the bound also turns the rust:check/rust:clippy CI lanes red.

[coderabbitai]

Walkthrough

This PR strengthens thread-safety guarantees for JsCell<T> by making its Send and Sync trait implementations conditional on the inner type bounds, replacing prior blanket unsafe impls. A compile-time proof module and integration test verify the bounds are correct, while SAFETY documentation expands clarifications around thread-handoff sequencing in concurrent task structures.

Changes

JsCell Send/Sync Safety Tightening

Layer / File(s)	Summary
JsCell trait bounds implementation src/jsc/JSCell.rs	JsCell<T> now requires T: Send for Send impl and T: Sync for Sync impl, replacing unconditional blanket implementations and restricting cross-thread sharing to thread-safe inner types.
Compile-time Send/Sync proof src/jsc/JSCell.rs	Embedded proof module uses trait ambiguity to assert JsCell<Rc<u32>> is !Send and !Sync while JsCell<u32> is Send and Sync, causing compilation failure if safety invariants regress.
Integration test for Send/Sync bounds test/internal/jscell-send-sync-bounds.test.ts	Bun test builds an isolated Rust crate outside the workspace to verify JsCell<T> trait bounds are correctly gated, running cargo check offline and asserting no trait-bound errors.
Thread-safety documentation updates src/jsc/ConcurrentPromiseTask.rs, src/jsc/WorkTask.rs	SAFETY documentation expanded to detail thread handoff sequencing and clarify that Context is intentionally not required to be Send because JS-thread-bound state is only accessed on the JS thread.

Possibly related issues

• Unsound Send impls — generic Context/T lacks required bounds #31498 — Directly addresses the same unsound unconditional Send/Sync impls for JsCell, ConcurrentPromiseTask, and WorkTask by adding proper trait bounds, compile-time proofs, and integration tests.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly and accurately summarizes the main change: tightening JsCell's Send/Sync trait impls to require the inner type bounds.
Description check	✅ Passed	The description comprehensively covers 'What' with detailed context, cause, and fix; verification includes full build success, smoke tests, and compile-time proofs; both template sections are well-addressed.
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.}

---

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

[robobun]

^{Updated 12:02 AM PT - May 28th, 2026}

❌ @robobun, your commit 06034bb has 1 failures in Build #58602 (All Failures):

• linker stderr: clang++: argument unused during compilation: '-no-pie' [-Wunused-command-line-argument] on 🐧 aarch64-android - build-rust
• linker stderr: clang++: argument unused during compilation: '-no-pie' [-Wunused-command-line-argument] on :freebsd: x64 - build-rust
• linker stderr: clang++: argument unused during compilation: '-no-pie' [-Wunused-command-line-argument] on :freebsd: aarch64 - build-rust
• linker stderr: clang++: argument unused during compilation: '-no-pie' [-Wunused-command-line-argument] on 🐧 x64-android - build-rust

---

🧪   To try this PR locally:

bunx bun-pr 31500

That installs a local version of the PR into your bun-31500 executable, so you can run:

bun-31500 --bun

[github-actions]

This PR may be a duplicate of:

1. JsCell: make get() unsafe, add closure-scoped with(), drop the Send/Sync impls #31181 - Also fixes JsCell Send/Sync soundness in src/jsc/JSCell.rs, but takes a different approach (makes get() unsafe, adds closure-scoped with(), drops Send/Sync impls entirely)

🤖 Generated with Claude Code

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 004e7ef4-7588-4292-a5ff-e3aaaa3a62ca

📥 Commits

Reviewing files that changed from the base of the PR and between 3ec6669 and 06034bb.

📒 Files selected for processing (4)

• src/jsc/ConcurrentPromiseTask.rs
• src/jsc/JSCell.rs
• src/jsc/WorkTask.rs
• test/internal/jscell-send-sync-bounds.test.ts

[robobun]

Closing in favor of #31181 (by @Jarred-Sumner), which addresses #31498's JsCell<T> soundness more completely: it makes get() an unsafe fn, adds a safe closure-scoped with(), and drops the unsafe impl Send/Sync entirely so JsCell<T> inherits the correct auto-trait behavior (Send iff T: Send, never Sync). That design also closes the replace()/set()/with_mut() leak @claude flagged here — which my narrower T: Sync bound missed (it would have needed T: Send + Sync).

On the other two locations in #31498 (ConcurrentPromiseTask/WorkTask): a Send bound there is not applicable — their context implementors (TransformTask, WalkTask, PipelineTask, GetAddrInfoRequest) are genuinely !Send by design (they hold a &JSGlobalObject / JS-heap raw pointers that then() uses on the JS thread; fields that truly cross to the worker already use bun_jsc::ThreadSafe). Adding C: Send fails to compile and would reject the correct design, so those unsafe impls are sound as-is. No code change is warranted there beyond the expanded SAFETY docs, which #31181 doesn't need.

Superseded; no reason to keep a competing, narrower PR open.