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

Closed
RO
3mo ago
+113-0

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 @ 074656dd73. 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