Skip to content

jsc: back JsRef::Weak with a real JSC::Weak handle - #36966

Closed
robobun wants to merge 8 commits into
mainfrom
farm/ee3bfb04/jsref-weak-handle
Closed

robobun wants to merge 8 commits into
mainfrom
farm/ee3bfb04/jsref-weak-handle

jsc: back JsRef::Weak with a real JSC::Weak handle

ac3edcd
Select commit
Loading
Failed to load commit list.
Claude / Claude Code Review completed Aug 5, 2026 in 43m 21s

Code review found 3 potential issues

Found 4 candidates, confirmed 3. See review comments for details.

Details

Severity Count
🔴 Important 0
🟡 Nit 3
🟣 Pre-existing 0
Severity File:Line Issue
🟡 Nit src/jsc/JSRef.rs:117-119 init_weak() now heap-allocates a Bun::WeakRef on every Request/Response construction (per-request hot path)
🟡 Nit test/js/bun/http/bun-server.test.ts:735-738 Lifecycle-canary test's 'at least one child died' assertion is scheduling-dependent (flake risk)
🟡 Nit src/jsc/JSRef.rs:117-119 debug_assert in init_weak/set_weak/update is now weaker than the invariant it guards

Annotations

Check warning on line 119 in src/jsc/JSRef.rs

See this annotation in the file changed.

@claude claude / Claude Code Review

init_weak() now heap-allocates a Bun::WeakRef on every Request/Response construction (per-request hot path)

`init_weak()` now heap-allocates a `Bun::WeakRef` (TZone node + `JSC::Weak` WeakImpl slot) where it was previously a zero-cost tag write, and it runs at wrapper *construction* — notably `Request::to_js` (every Bun.serve incoming request) and the `Response` constructor / `Response::to_js` (every handler return) — so ≥2 extra allocations land on every served HTTP request. The PR body's Perf paragraph only covers "each downgraded holder", which understates this; also `Response::construct_error` at 

Check warning on line 738 in test/js/bun/http/bun-server.test.ts

See this annotation in the file changed.

@claude claude / Claude Code Review

Lifecycle-canary test's 'at least one child died' assertion is scheduling-dependent (flake risk)

The `expect(results.some(r => r.died)).toBe(true)` assertion is scheduling-dependent: it requires JSC's async GC (`Bun.gc(false)` + allocation churn) to escalate to a full collection that reaps the promoted wrapper within 150 rounds in at least 1 of 3 children — the test's own comment concedes this is "scheduling-dependent per child", and GC threshold behavior differs across debug/release/ASAN/platform without any per-lane branching here. Since the PR body frames this test as "a canary for the c

Check warning on line 119 in src/jsc/JSRef.rs

See this annotation in the file changed.

@claude claude / Claude Code Review

debug_assert in init_weak/set_weak/update is now weaker than the invariant it guards

nit: the `debug_assert!(!value.is_empty_or_undefined_or_null())` here (and the same assert in `set_weak` at line 148 and `update`'s Weak arm at line 232) is now weaker than the actual precondition — `Weak::create_passive()` requires `value.is_object()` and silently returns an empty handle for any non-object (JSString, Symbol, BigInt, primitives). Consider tightening to `debug_assert!(value.is_object())` so a future caller passing a non-object trips loudly in debug builds instead of getting a mys