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

src/jsc/ConcurrentPromiseTask.rs

@@ -48,10 +48,22 @@ pub struct ConcurrentPromiseTask<'a, Context: ConcurrentPromiseTaskContext> {
 
 bun_threading::intrusive_work_task!(['a, Context: ConcurrentPromiseTaskContext] ConcurrentPromiseTask<'a, Context>, task);
 
-// SAFETY: `ConcurrentPromiseTask` is heap-allocated and only its address crosses
-// threads via the intrusive `task` node and the concurrent queue. All access to
-// `ctx` / `promise` / `global_this` is sequenced by the work-pool → on_finish →
-// run_from_js hand-off; raw pointers are inert.
+// SAFETY: `ConcurrentPromiseTask` is heap-allocated and only its *address* crosses
+// threads, via the intrusive `task` node and the concurrent queue; it is never
+// aliased on two threads at once. The hand-off is strictly sequenced:
+// `schedule` (JS thread) → `run_from_thread_pool` (one worker, exclusive) →
+// `on_finish` re-enqueues onto the JS event loop → `run_from_js` (JS thread).
+//
+// `Context` is deliberately NOT required to be `Send`: its implementors hold
+// JS-thread-bound state — a `&JSGlobalObject` (opaque `!Sync` FFI handle) and
+// raw pointers into the JS heap — that `then()` touches back on the JS thread.
+// The only method the worker runs is `ctx.run()`, which operates on the
+// thread-safe subset; any value that genuinely needs to cross to the worker is
+// wrapped at the field level in `bun_jsc::ThreadSafe` (e.g. `TransformTask::input_code`),
+// which protects the JS value and unprotects on drop. Requiring `C: Send` would
+// reject this correct-by-construction design, so the invariant is discharged here
+// rather than by an auto-trait bound — the same single-JS-thread contract that
+// backs `JsCell` and `VirtualMachine`'s hand-written `unsafe impl`s.
 unsafe impl<C: ConcurrentPromiseTaskContext> Send for ConcurrentPromiseTask<'_, C> {}
 
 impl<Context: ConcurrentPromiseTaskContext> Taskable for ConcurrentPromiseTask<'_, Context> {

src/jsc/JSCell.rs

@@ -119,11 +119,21 @@
 // SAFETY: see type-level docs — `JsCell` is only dereferenced on the owning
 // JS thread; the `Sync` impl exists so `&'static VirtualMachine` (which
 // contains `JsCell` fields) satisfies `'static`-bound trait objects and
-// `thread_local!` accessors without `T: Sync` cascading everywhere. It is NOT
-// a license for cross-thread `get_mut()`.
-unsafe impl<T> Sync for JsCell<T> {}
-// SAFETY: same single-thread-owner invariant as `Sync` above.
-unsafe impl<T> Send for JsCell<T> {}
+// `thread_local!` accessors without hand-written `unsafe impl`s cascading
+// everywhere. It is NOT a license for cross-thread `get_mut()`.
+//
+// The `T: Sync` / `T: Send` bounds are load-bearing: `get()` is a *safe* fn
+// that hands out `&T`, so an unconditional `Sync` would let 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. Gating on the inner bound keeps the thread-affinity escape
+// hatch while making `JsCell<!Sync>`/`JsCell<!Send>` correctly `!Sync`/`!Send`,
+// so such a field can never silently make its container cross-thread-shareable.
+unsafe impl<T: Sync> Sync for JsCell<T> {}
+// SAFETY: same single-thread-owner invariant as `Sync` above; `T: Send`
+// because `into_inner()` moves the owned `T` out across whatever thread holds
+// the cell.
+unsafe impl<T: Send> Send for JsCell<T> {}
 
 impl<T> JsCell<T> {
     #[inline(always)]
@@ -225,3 +235,40 @@
         self.get().fmt(f)
     }
 }
+
+// Compile-time proof that the `Send`/`Sync` bounds on `JsCell<T>` are honored.
+//
+// The safe `get()` makes unconditional `Send`/`Sync` unsound (see the SAFETY
+// note on the impls): a `&JsCell<!Sync>` must not be shareable across threads.
+// Stable Rust has no negative bounds, so the `!Send`/`!Sync` direction uses the
+// auto-trait-ambiguity trick — if `JsCell<Rc<u32>>` ever regains `Send`/`Sync`,
+// both blanket impls below apply and the anonymous `const` fails to compile
+// with "conflicting implementations" (same shape as `subproc.rs`'s
+// `__pipe_reader_thread_confined`).
+mod jscell_send_sync_proof {
+    use super::JsCell;
+
+    // `JsCell<T>` stays `Send`/`Sync` when `T` is — the escape hatch still works.
+    const _: fn() = || {
+        fn assert_send<T: Send>() {}
+        fn assert_sync<T: Sync>() {}
+        assert_send::<JsCell<u32>>();
+        assert_sync::<JsCell<u32>>();
+    };
+
+    // `JsCell<T>` is NOT `Send`/`Sync` when `T` is not (`Rc` is `!Send + !Sync`).
+    trait NotSend<A> {
+        const OK: () = ();
+    }
+    impl<T: ?Sized> NotSend<()> for T {}
+    impl<T: ?Sized + Send> NotSend<u8> for T {}
+
+    trait NotSync<A> {
+        const OK: () = ();
+    }
+    impl<T: ?Sized> NotSync<()> for T {}
+    impl<T: ?Sized + Sync> NotSync<u8> for T {}
+
+    const _: () = <JsCell<std::rc::Rc<u32>> as NotSend<_>>::OK;
+    const _: () = <JsCell<std::rc::Rc<u32>> as NotSync<_>>::OK;
+}

src/jsc/WorkTask.rs

@@ -51,10 +51,20 @@ pub struct WorkTask<Context: WorkTaskContext> {
 
 bun_threading::intrusive_work_task!([Context: WorkTaskContext] WorkTask<Context>, task);
 
-// SAFETY: `WorkTask` is moved into the thread pool's queue (intrusive `task`
-// node) and back via the concurrent task queue. All access to `ctx` /
-// `global_this` is sequenced by the work-pool → on_finish → run_from_js
-// hand-off; raw pointers are inert.
+// SAFETY: `WorkTask` is heap-allocated; only its *address* crosses threads, via
+// the intrusive `task` node and the concurrent queue, and it is never aliased on
+// two threads at once. The hand-off is strictly sequenced: `schedule` (JS thread)
+// → `run_from_thread_pool` (one worker, exclusive) → `on_finish` re-enqueues onto
+// the JS event loop → `run_from_js` (JS thread).
+//
+// `Context` is deliberately NOT required to be `Send`: `ctx` is a raw
+// `*mut Context` and its implementors (e.g. `GetAddrInfoRequest`) hold
+// JS-thread-bound raw pointers (`*mut DNSLookup`, `*mut Resolver`) that `then()`
+// uses back on the JS thread. The worker only runs `Context::run`, which touches
+// the thread-safe subset; the raw pointer is inert while the address is in flight.
+// Requiring `C: Send` would reject this correct-by-construction design — the same
+// single-JS-thread contract that backs `JsCell` and `VirtualMachine`'s
+// hand-written `unsafe impl`s.
 unsafe impl<C: WorkTaskContext> Send for WorkTask<C> {}
 
 impl<Context: WorkTaskContext> Taskable for WorkTask<Context> {

test/internal/jscell-send-sync-bounds.test.ts

@@ -0,0 +1,132 @@
+// `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 must be gated on the inner type:
+//
+//   unsafe impl<T: Sync> Sync for JsCell<T> {}
+//   unsafe impl<T: Send> Send for JsCell<T> {}
+//
+// If they were unconditional (`unsafe impl<T> ...`), a `&JsCell<T>` would be
+// shareable across threads via any `Sync` container, and the safe `get()` would
+// then hand out a `&T` to a `!Sync` `T` (`Rc`, `Cell`, `RefCell`) on another
+// thread — UB with zero `unsafe` at the call site (oven-sh/bun#31498).
+//
+// This is a type-level invariant with no runtime surface, so the compiler checks
+// it: a throwaway crate depends on the real `bun_jsc` and asserts `JsCell<Rc<u32>>`
+// is `!Send + !Sync` via the auto-trait-ambiguity trick (stable Rust has no
+// negative bounds). `cargo check` succeeds iff the bounds are present — with the
+// unconditional impls `JsCell<Rc<u32>>` is `Send + Sync`, both blanket impls apply,
+// and the build fails with E0283. `cargo check` does not link, so no C++/JSC objects
+// are needed; it only needs the codegen the debug build already emitted.
+import { expect, test } from "bun:test";
+import { tempDir } from "harness";
+import { existsSync, readFileSync } from "node:fs";
+import path from "node:path";
+
+const cargo = Bun.which("cargo");
+const repoRoot = path.resolve(import.meta.dir, "..", "..");
+const jscCrate = path.join(repoRoot, "src", "jsc");
+
+// `bun_jsc`'s build.rs pulls generated Rust from here; emitted by `bun bd`.
+const codegenReady =
+  existsSync(path.join(repoRoot, "build", "debug", "codegen", "cpp.rs")) &&
+  existsSync(path.join(repoRoot, "build", "debug", "codegen", "generated_resolved_source_tag.rs"));
+
+// Pin the same toolchain `bun bd` uses (the temp crate lives outside the repo, so
+// it wouldn't otherwise pick up rust-toolchain.toml).
+function pinnedToolchain(): string | undefined {
+  try {
+    const toml = readFileSync(path.join(repoRoot, "rust-toolchain.toml"), "utf8");
+    return toml.match(/channel\s*=\s*"([^"]+)"/)?.[1];
+  } catch {
+    return undefined;
+  }
+}
+const toolchain = pinnedToolchain();
+
+// Host triple, for `--target` so the build.rs codegen path lines up.
+function hostTriple(): string | undefined {
+  if (!cargo) return undefined;
+  const out = Bun.spawnSync({ cmd: [cargo, "-vV"], env: process.env }).stdout.toString();
+  return out.match(/host:\s*(\S+)/)?.[1];
+}
+const triple = hostTriple();
+
+const proof = `
+use bun_jsc::JsCell;
+use std::rc::Rc;
+
+trait NotSend<A> { const OK: () = (); }
+impl<T: ?Sized> NotSend<()> for T {}
+impl<T: ?Sized + Send> NotSend<u8> for T {}
+
+trait NotSync<A> { const OK: () = (); }
+impl<T: ?Sized> NotSync<()> for T {}
+impl<T: ?Sized + Sync> NotSync<u8> for T {}
+
+// JsCell<Rc<u32>> must be !Send and !Sync. If either impl is unconditional the
+// type gains the trait, both blanket impls apply, and this fails to compile (E0283).
+const _: () = <JsCell<Rc<u32>> as NotSend<_>>::OK;
+const _: () = <JsCell<Rc<u32>> as NotSync<_>>::OK;
+
+// Positive direction: a Send/Sync inner type keeps the thread-affinity escape hatch.
+const _: fn() = || {
+    fn assert_send<T: Send>() {}
+    fn assert_sync<T: Sync>() {}
+    assert_send::<JsCell<u32>>();
+    assert_sync::<JsCell<u32>>();
+};
+
+fn main() {}
+`;
+
+test.skipIf(!cargo || !codegenReady || !toolchain || !triple)(
+  "JsCell<T>'s Send/Sync impls require T: Send/Sync (issue #31498)",
+  async () => {
+    // `[workspace]` detaches this crate from the repo's workspace; the absolute
+    // path dep resolves `bun_jsc` regardless of cwd. The target dir lives inside
+    // the temp dir so the run is self-contained and leaves the build untouched.
+    using dir = tempDir("jscell-send-sync", {
+      "Cargo.toml": `[package]
+name = "jscell_send_sync_proof"
+version = "0.0.0"
+edition = "2021"
+
+[[bin]]
+name = "jscell_send_sync_proof"
+path = "main.rs"
+
+[dependencies]
+bun_jsc = { path = ${JSON.stringify(jscCrate)} }
+
+[workspace]
+`,
+      "main.rs": proof,
+    });
+
+    await using proc = Bun.spawn({
+      // --offline: every dep is already in the cargo cache from `bun bd`; never
+      // touch the network in CI.
+      cmd: [cargo!, "check", "--quiet", "--offline", "--target", triple!],
+      cwd: String(dir),
+      env: {
+        ...process.env,
+        RUSTUP_TOOLCHAIN: toolchain!,
+        CARGO_TARGET_DIR: path.join(String(dir), "target"),
+        CARGO_TERM_COLOR: "never",
+      },
+      stdout: "pipe",
+      stderr: "pipe",
+    });
+
+    const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+
+    // `cargo check` must succeed — the negative assertions compile only when
+    // `JsCell<Rc<u32>>` is correctly `!Send + !Sync`. E0283 is the signature of
+    // the unconditional impls (both ambiguity blankets apply).
+    const output = stdout + stderr;
+    expect(output).not.toContain("E0283");
+    expect(output).not.toMatch(/^error/m);
+    expect(exitCode).toBe(0);
+  },
+  300_000,
+);