valkey: skip JS error construction in on_close when the worker VM has stopped

[robobun]

What

worker.terminate() while a Worker-owned Bun.RedisClient has commands in flight aborts assert-enabled builds:

ASSERTION FAILED: vm.hasTerminationRequest()
vendor/WebKit/Source/JavaScriptCore/runtime/VMTraps.cpp(539) : void JSC::VMTraps::deferTerminationSlow(DeferAction)

5/5 whole-process SIGABRT within the first 10 iterations on release-asan main @ 074656d. Release builds compile the assert out and silently continue building JS errors / rejecting promises on a terminated VM.

Repro

No redis server required; inline RESP3 responder:

import { isMainThread } from "worker_threads";
if (!isMainThread || process.env.AS_WORKER) {
  const c = new Bun.RedisClient(process.env.RURL, { autoReconnect: false });
  await c.connect(); postMessage("armed");
  for (;;) { const ps = []; for (let i = 0; i < 2000; i++) ps.push(c.incr("k").catch(() => {})); await Promise.all(ps); }
}
const HELLO = "%3\r\n+server\r\n+fake\r\n+version\r\n+7.4.0\r\n+proto\r\n:3\r\n";
const server = Bun.listen({ hostname: "127.0.0.1", port: 0, socket: {
  data(s, d) { let out = ""; for (const line of d.toString("latin1").split("\r\n")) { if (line[0] !== "*") continue; out += s.helloDone ? ":1\r\n" : HELLO; s.helloDone = true; } if (out) s.write(out); },
  open(s) { s.helloDone = false; }, close() {}, error() {} } });
const RURL = `redis://127.0.0.1:${server.port}`;
for (let i = 0; i < 80; i++) {
  const w = new Worker(import.meta.url, { env: { ...process.env, AS_WORKER: "1", RURL } });
  await new Promise(res => {
    w.onmessage = e => { if (e.data === "armed") setTimeout(() => w.terminate(), (i * 7) % 60); };
    w.addEventListener("close", res); w.onerror = () => {};
  });
}

Cause

WebWorker::shutdown() clears hasTerminationRequest() (so process.on('exit') can run) and then drains every socket group via RareData::close_all_socket_groups. That reaches the valkey socket's on_close, which for every in-flight / queued command calls valkey_error_to_js -> ErrorCode::fmt -> Bun__createErrorWithCode -> Bun::createError -> globalObject->nodeErrorCache(). With the TerminationException still pending but hasTerminationRequest() now false, the LazyProperty init's DeferTermination scope trips ASSERT(vm.hasTerminationRequest()) in VMTraps::deferTerminationSlow.

lldb backtrace of the aborting worker thread:

deferTerminationSlow <- LazyProperty::callFunc <- Zig::GlobalObject::nodeErrorCache
  <- Bun::createError <- Bun__createErrorWithCode <- ErrorCode::fmt
  <- valkey_error_to_js <- reject_in_flight_commands <- ValkeyClient::on_close
  <- SocketHandler::on_close <- us_socket_group_close_all_ex
  <- RareData::close_all_socket_groups <- WebWorker::shutdown

Same socket-group-drain entry that #34414 / #36579 guarded for Bun.listen / Bun.connect socket handlers; valkey's on_close was unguarded.

Fix

Guard ValkeyClient::on_close() on script_execution_status(): when the VM is not Running, drop the in-flight and offline queues without building JS errors (their JSPromiseStrong handles release via Drop), then route through on_valkey_close() which now also short-circuits the JS work (connection-promise reject, onclose callback) after adopting connect()'s socket keep-alive ref. Mirrors the Bun.listen handler guards added in #36579.

Verification

New test in test/js/web/workers/worker-terminate-lifetime.test.ts runs the inline-responder repro (alternating autoReconnect: false / default so both the fail and reconnect branches of on_close are exercised) for 20 iterations. Debug-only (test.skipIf(!isDebug)) since release WebKit compiles the assert out.

• Without this patch: aborts in ~2.6s on iteration 1 with the exact ASSERTION FAILED: vm.hasTerminationRequest().
• With this patch: 20/20 workers terminate cleanly (~38s).

valkey-gc.test.ts and the rest of worker-terminate-lifetime.test.ts pass unchanged.

---

no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/workers/worker-terminate-lifetime.test.ts

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 14 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 76d0de10-9af0-4c00-95d2-1d3304d8a955

📥 Commits

Reviewing files that changed from the base of the PR and between 074656d and fd1d250.

📒 Files selected for processing (3)

• src/runtime/valkey_jsc/js_valkey.rs
• src/runtime/valkey_jsc/valkey.rs
• test/js/web/workers/worker-terminate-lifetime.test.ts

---

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

[claude]

No issues found, but this touches worker-shutdown ref-counting on the valkey client and I'd like a human to confirm the socket keep-alive ref balance and that update_poll_ref() (still run by the _defer scopeguard on the new early return in on_valkey_close) is safe on a stopped VM.

What was reviewed:

• Guard matches the established script_execution_status() != Running pattern from socket_body.rs (#36579) and ipc.rs.
• Stopped-VM queue drain mirrors shutdown(None)'s finalizing branch — JSPromiseStrong handles release via Drop, no leak.
• on_valkey_close() still runs ScopedRef::adopt before the early return, so connect()'s socket keep-alive ref is released on both fail and reconnect paths.
• Test is hermetic (inline RESP3 responder, no real redis), debug-only, and follows the neighboring tests' shape exactly.

Extended reasoning...

Overview

Fixes a whole-process SIGABRT when worker.terminate() lands while a Worker-owned Bun.RedisClient has commands in flight. The worker shutdown's socket-group drain reaches ValkeyClient::on_close(), which was building coded JS errors (via valkey_error_to_js → nodeErrorCache lazy init → DeferTermination) with a TerminationException pending and hasTerminationRequest() already cleared, tripping ASSERT(vm.hasTerminationRequest()) in VMTraps::deferTerminationSlow.

Two guards are added: one in ValkeyClient::on_close() (valkey.rs) that drops both queues without building JS errors when script_execution_status() != Running, then routes to on_valkey_close(); and one in JSValkeyClient::on_valkey_close() (js_valkey.rs) that early-returns after adopting the socket keep-alive ref, skipping the connection-promise reject and onclose callback. A debug-only regression test with an inline RESP3 responder is added to worker-terminate-lifetime.test.ts.

Security risks

None. This is a worker-shutdown crash fix; no user input parsing, auth, or trust-boundary changes.

Level of scrutiny

Medium-high. The change is small (~30 LOC) and follows the exact pattern already used for Bun.listen/Bun.connect socket handlers in socket_body.rs, but it sits at the intersection of worker VM termination, JSC's sticky TerminationException, and intrusive ref-counting on JSValkeyClient. The socket keep-alive ref taken by connect() must be released exactly once by either on_valkey_close() or on_valkey_reconnect(); the new stopped-VM branch routes through on_valkey_close() which adopts it before the early return, so the balance holds. The queue-drain-without-JS mirrors shutdown(None)'s finalizing branch (JSPromiseStrong releases via Drop). The one thing I did not exhaustively verify is that update_poll_ref() — which still runs via the pre-existing _defer scopeguard on the new early return — is fully safe on a stopped VM (it reads JSMap::size() and may call JsRef::upgrade()); this is pre-existing behavior since the outer SocketHandler::on_close _defer already calls it, but worth a maintainer glance.

Other factors

• PR description includes a full lldb backtrace and explicit before/after verification (aborts iteration 1 unpatched → 20/20 clean patched).
• Test alternates autoReconnect: true/false to exercise both the fail and reconnect branches of on_close, is hermetic, and matches the conventions of the four neighboring terminate()-during-X regression tests in the same file.
• test.skipIf(!isDebug) is appropriate since release WebKit compiles the assert out, matching the sibling tests.
• No prior human reviews on the PR.

[robobun]

On the two points raised:

Socket keep-alive ref balance. connect() takes exactly one +1 (js_valkey.rs:1531 socket_ref, forgotten into the socket on success). The only releasers are on_valkey_close() and on_valkey_reconnect(), each via ScopedRef::adopt(self.as_ctx_ptr()). Before this patch, ValkeyClient::on_close() routed to one of those on every branch (manual-close / auto-reconnect-disabled / max-retries / reconnect). The new stopped-VM branch routes to on_valkey_close() unconditionally, and on_valkey_close() adopts the ref before the new early return, so the balance is unchanged: +1 in, exactly one adopt out.

update_poll_ref() on a stopped VM. The _defer scopeguard that runs it is pre-existing (and SocketHandler::on_close at js_valkey.rs:1977 also calls it on the way out); before this patch the process aborted earlier and never reached it. The chain is has_any_pending_commands() (native field reads), flags.finalized (native bool, false here), has_subscriptions() -> subscription_callback_map().size(). The map slot is seated unconditionally at construction (js_valkey.rs:844), so the .unwrap()s hold; JSC__JSMap__size is just map->size() in C++ (bindings.cpp:6005), no JS entry. Its check_slow wrapper constructs a TopExceptionScope, which observes the pending TerminationException and returns Err; .unwrap_or(false) swallows that and subs_deletable becomes true. poll_ref.ref_()/unref() are native uWS loop ops. Exercised by the new test under debug+asan for both autoReconnect modes (40/40 clean locally; CI green).