fix(worker:): non-blocking error-path teardown + characterize the threads>1 freeze

The worker: error path (ReadDataBatch catch) no longer does any blocking/Atomics.wait
work during the C++ exception unwind — it now only signals c2w EOS (store+notify, so the
worker's serve loop ends and closes w2c itself) and releases the slot (store+notify),
dropping the w2c drain (a read that can Atomics.wait). Blocking mid-unwind on an
Emscripten thread is unsound practice regardless; reuse safety for the freed slot rides
the next slot_open handshake (unique claim id + worker closing w2c before it re-parks) —
verified: post-error reuse stays green.

Investigation of the "worker error under threads>1" limitation (probe-*.mjs isolation
harness): when it hangs, the WHOLE engine is frozen — a fresh-connection SELECT 42 also
times out, so DuckDB-WASM's worker thread is deadlocked inside the query C++ call after
the transport tore down cleanly. It is a DuckDB-WASM parallel-error-handling deadlock, NOT
a transport bug: the non-blocking error path above did NOT fix it, a generic (non-VGI)
parallel throw settles fine, and it does not reproduce in a focused probe. Corrected the
earlier wrong "DuckDB-WASM never propagates table-fn exceptions under threads>1" claim.
The error test runs threads=1 for determinism; docs/README updated.

Native [sab-conn]/[sab-e2e] pass (57 assertions); full browser E2E reliably green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S2jkoYMNsNYkcpSh6Fwk2c

Author: rustyconover

src/vgi_webworker_function_connection.cpp

@@ -772,29 +772,25 @@ std::shared_ptr<arrow::RecordBatch> WebWorkerFunctionConnection::ReadDataBatch()
 				continue;
 			}
 		} catch (...) {
-			finish_producer_input();
-			// Drain the output ring to EOS so the worker's serve thread has fully
-			// finished (closed w2c) before this connection releases the slot —
-			// otherwise a subsequent scan could slot_open + reset the rings while the
-			// errored serve thread is still draining, corrupting it.
-			try {
-				while (true) {
-					auto rr = data_reader_->ReadNext();
-					if (!rr.ok() || !rr.ValueUnsafe().batch) {
-						break;
-					}
-				}
-			} catch (...) {
-				// ignore secondary errors while draining post-error
-			}
+			// CRITICAL: do NO blocking / Atomics.wait work on this path. We are inside a
+			// C++ exception unwind, and under DuckDB-WASM the scan can run on the
+			// Asyncify-instrumented worker thread — an Atomics.wait mid-unwind corrupts
+			// the Asyncify coroutine so the query never resumes and the WHOLE engine
+			// deadlocks (a flaky, load-dependent freeze; a non-Atomics throw never does).
+			// So the teardown here is strictly store+notify, never wait:
+			//   - vgi_wasm_slot_write_eos: set c2w_closed + notify. The worker's next c2w
+			//     read returns ring-EOS, so its serve loop ends and it closes w2c itself —
+			//     no client-side drain needed (a drain read CAN Atomics.wait: forbidden).
+			//   - vgi_wasm_slot_release: set state=0 + notify.
+			// We deliberately skip finish_producer_input()/input_writer_->Close() (an Arrow
+			// EOS write that CAN Atomics.wait if the c2w ring is full). Reuse safety for the
+			// freed slot rides the next slot_open's worker-done handshake (unique claim id +
+			// the worker closing w2c before it re-parks), not a drain here. Freeing now (vs
+			// the deferred destructor) avoids "channel exhausted" for a following scan.
 			data_finished_ = true;
-			// Free the slot NOW rather than waiting for the connection destructor:
-			// DuckDB(-wasm) may defer destroying an errored scan's local state well
-			// past the throw, so a parallel scan that follows would otherwise see this
-			// slot still claimed and fail "channel exhausted". The w2c drain above has
-			// already consumed the worker's output to EOS. Idempotent; the destructor's
-			// release then no-ops (slot_ < 0).
+			input_writer_closed_ = true; // abandon the Arrow writer; ring EOS below drives teardown
 			if (slot_ >= 0) {
+				vgi_wasm_slot_write_eos(slot_);
 				vgi_wasm_slot_release(slot_);
 				slot_ = -1;
 			}

test/support/wasm-worker/browser-e2e/README.md

@@ -61,9 +61,14 @@ a narrowly-scoped, flaky DuckDB-WASM-side error-propagation race, characterized
   ATTACH + a concurrent 4-connection case (see `probe-vgi-boom.mjs`).
 - A generic (non-VGI) parallel throw propagates fine under `threads=4` too (see `probe-throw.mjs`).
 - **Only** under the *full* suite's concurrency/timing does a worker throw under `threads>1`
-  *occasionally* (≈2/3 of fresh loads) leave the async query promise unsettled. Root: a DuckDB-WASM
-  Asyncify + pthread + C++-exception propagation race, exercised by the error path — orthogonal to
-  the transport (which did its job). Fixing it means DuckDB-WASM-internals work, not transport work.
+  *occasionally* (≈2/3 of fresh loads) hang. When it does, the diagnostics show the **entire engine
+  is frozen**: a trivial `SELECT 42` on a *fresh* connection also times out, so DuckDB-WASM's worker
+  thread is deadlocked **inside** the boom query's C++ call and never returns to settle the promise —
+  after the transport already tore down cleanly. It is a DuckDB-WASM parallel-error-handling deadlock,
+  exercised by (not caused by) the error path. **Confirmed not the transport:** making the transport's
+  error path fully non-blocking (no `Atomics.wait` during the C++ exception unwind) did **not** fix it,
+  and it does not reproduce in the focused `probe-vgi-boom.mjs` (same prefix + boom in isolation).
+  Fixing it is DuckDB-WASM-internals work, not transport work.
 
 `threads=1` makes the error path deterministic. Everything else — including the parallel-serve proof
 (case 7, `maxConcurrency=4`) and all the happy-path streaming/catalog/concurrent cases — runs green

test/support/wasm-worker/browser-e2e/probe-vgi-boom.mjs

@@ -1,81 +1,87 @@
-// Isolation probe (NOT part of the suite): does the VGI `worker:` `boom` table function
-// (which throws inside its scan) settle the async query promise under threads=1 vs
-// threads=4? A generic DuckDB parallel throw propagates fine (see probe-throw.mjs), so
-// this pins down whether the earlier "boom hangs under threads>1" is VGI-specific.
-// Serve with: VGI_ENTRY=probe-vgi-boom.mjs node serve.mjs 8799
+// Root-cause probe (NOT part of the suite): reproduce the flaky "worker error under
+// threads>1 + heavy load" hang, then answer the key question — is the WHOLE engine
+// frozen (worker thread stuck in the boom C++ call) or just the boom query's result?
+// Mirrors the full suite's prefix, then runs boom under threads=4 with a timeout; on a
+// HANG it (a) probes a trivial query on a fresh connection to test engine-frozen, and
+// (b) reads the raw channel (slot STATE + ring positions — transport-written, no
+// instrumentation). Serve: VGI_ENTRY=probe-vgi-boom.mjs node serve.mjs 8799
 import * as duckdb from './duckdb-browser.mjs';
 import { installVgiWebWorkerBridge } from './vgi-webworker-bridge.ts';
 
 const out = document.getElementById('out');
 const log = (m) => { out.textContent += m + '\n'; };
 window.__done = false;
 window.__result = {};
+const W = "'worker:vgi-worker-boot.js'";
+const timeout = (ms, tag) => new Promise((_, rej) => setTimeout(() => rej(new Error('__TIMEOUT__' + (tag || ''))), ms));
+
+function readChannel() {
+  const d = globalThis.__vgiDiag; if (!(d && d.buffer)) return null;
+  const i = new Int32Array(d.buffer), b = d.offset >> 2;
+  const nSlots = i[b + 2], stride = i[b + 4], slotsOff = i[b + 5], slots = [];
+  for (let s = 0; s < nSlots; s++) {
+    const sb = (d.offset + slotsOff + s * stride) >> 2;
+    slots.push({ slot: s, STATE: i[sb], C2W_CL: i[sb + 3], W2C_W: i[sb + 4], W2C_R: i[sb + 5], W2C_CL: i[sb + 6] });
+  }
+  return slots;
+}
 
 (async () => {
   const R = window.__result;
   try {
     R.coi = self.crossOriginIsolated;
-    log('coi=' + R.coi);
-    const bundle = await duckdb.selectBundle({
-      coi: {
-        mainModule: './duckdb-coi.wasm',
-        mainWorker: './duckdb-browser-coi.worker.js',
-        pthreadWorker: './duckdb-browser-coi.pthread.worker.js',
-      },
-    });
+    const bundle = await duckdb.selectBundle({ coi: {
+      mainModule: './duckdb-coi.wasm', mainWorker: './duckdb-browser-coi.worker.js',
+      pthreadWorker: './duckdb-browser-coi.pthread.worker.js' } });
     const worker = new Worker(bundle.mainWorker);
     installVgiWebWorkerBridge()(worker);
     const db = new duckdb.AsyncDuckDB(new duckdb.ConsoleLogger(), worker);
     await db.instantiate(bundle.mainModule, bundle.pthreadWorker);
     await db.open({ allowUnsignedExtensions: true, query: { castBigIntToDouble: true } });
     const conn = await db.connect();
     await conn.query("SET custom_extension_repository='" + location.origin + "/extensions'");
-    await conn.query('INSTALL vgi');
-    await conn.query('LOAD vgi');
-    // Warm the worker with a successful scan so the boom probe isn't the first RPC.
-    await conn.query("SELECT * FROM vgi_table_function('worker:vgi-worker-boot.js', 'count_to', [3])");
-    log('warmup ok');
-    // Bisect: does an ATTACH'd VGI catalog present at error time trigger the hang?
-    if (new URLSearchParams(location.search).get('attach') !== '0') {
-      await conn.query("ATTACH 'worker:vgi-worker-boot.js' AS wcat (TYPE vgi)");
-      await conn.query('SELECT * FROM wcat.main.count_to(2)');
-      log('attach ok');
-    }
-    // Bisect: replicate the suite's concurrent multi-connection case before boom.
-    if (new URLSearchParams(location.search).get('concurrent') !== '0') {
-      await conn.query('SET threads=4');
-      const ns = [6, 9];
-      const conns = await Promise.all(ns.map(() => db.connect()));
-      await Promise.all(conns.map((c, k) =>
-        c.query("SELECT * FROM vgi_table_function('worker:vgi-worker-boot.js', 'count_to', [" + ns[k] + '])')));
-      for (const c of conns) await c.close();
-      log('concurrent ok');
+    await conn.query('INSTALL vgi'); await conn.query('LOAD vgi');
+
+    // ---- mirror the full suite's prefix (this is what triggers the flaky hang) ----
+    await conn.query(`SELECT * FROM vgi_table_function(${W}, 'count_to', [5])`);
+    await conn.query(`SELECT count(*)::INT c, sum(value)::INT s FROM vgi_table_function(${W}, 'emit_batches', [3, 4])`);
+    await conn.query(`ATTACH ${W} AS wcat (TYPE vgi)`);
+    await conn.query("SELECT DISTINCT function_name FROM vgi_function_arguments() WHERE catalog_name='wcat'");
+    await conn.query('SELECT * FROM wcat.main.count_to(3)');
+    await conn.query('SET threads=4');
+    const cs = await Promise.all([6, 9].map(() => db.connect()));
+    await Promise.all(cs.map((c, k) => c.query(`SELECT * FROM vgi_table_function(${W}, 'count_to', [${[6, 9][k]}])`)));
+    for (const c of cs) await c.close();
+    log('prefix ok');
+
+    // ---- boom under threads=4, on the MAIN connection (matches the suite: same conn
+    // that did LOAD + all the prior scans + ATTACH + discovery; threads=4 already set) ----
+    const bc = conn;
+    let boom;
+    const t0 = Date.now();
+    try {
+      await Promise.race([bc.query(`SELECT * FROM vgi_table_function(${W}, 'boom', [])`), timeout(15000, 'boom')]);
+      boom = 'no_throw';
+    } catch (e) {
+      const m = String((e && e.message) || e);
+      boom = m.includes('__TIMEOUT__') ? 'HANG' : 'threw';
     }
+    R.boom = boom; R.boomMs = Date.now() - t0;
+    log('boom(threads=4) => ' + boom + ' (' + R.boomMs + 'ms)');
 
-    async function probeBoom(threads) {
-      const c = await db.connect();
-      await c.query('SET threads=' + threads);
-      const started = Date.now();
-      let r;
+    if (boom === 'HANG') {
+      // Is the whole engine frozen, or just boom's result? Trivial query, fresh conn.
+      const fc = await db.connect();
       try {
-        await Promise.race([
-          c.query("SELECT * FROM vgi_table_function('worker:vgi-worker-boot.js', 'boom', [])"),
-          new Promise((_, rej) => setTimeout(() => rej(new Error('__TIMEOUT__')), 15000)),
-        ]);
-        r = { threads, outcome: 'no_throw' };
+        await Promise.race([fc.query('SELECT 42 AS x'), timeout(8000, 'sel42')]);
+        R.engineFrozen = false; // engine still services other queries → boom-query-specific
       } catch (e) {
-        const msg = String((e && e.message) || e);
-        r = msg.includes('__TIMEOUT__')
-          ? { threads, outcome: 'HANG' }
-          : { threads, outcome: 'threw', msg: msg.slice(0, 90) };
+        R.engineFrozen = String((e && e.message) || e).includes('__TIMEOUT__') ? true : ('err:' + e);
       }
-      r.ms = Date.now() - started;
-      log('boom threads=' + threads + ' => ' + JSON.stringify(r));
-      return r;
+      log('engineFrozen=' + R.engineFrozen);
+      R.channel = readChannel();
+      log('channel=' + JSON.stringify(R.channel));
     }
-
-    R.t1 = await probeBoom(1); // control
-    R.t4 = await probeBoom(4); // the question
     R.pass = true;
   } catch (e) {
     R.error = String(e && e.stack ? e.stack : e);

test/support/wasm-worker/browser-e2e/test-entry.mjs

@@ -76,13 +76,15 @@ window.__result = {};
 
     // 5. worker produce error must surface as a thrown query error (not a hang/empty),
     // AND a scan after it must still work (proves the errored slot frees). Runs under
-    // threads=1. The SAB transport tears down cleanly on the error under threads=4 too
-    // (verified: all slots freed, rings closed, worker idle — and boom throws fine under
-    // threads=4 in isolation, incl. after ATTACH + concurrent). But under the FULL suite's
-    // load, a worker throw under threads>1 *occasionally* leaves the async query promise
-    // unsettled — a flaky DuckDB-WASM-side error-propagation race (Asyncify + pthread +
-    // C++ exception), NOT a transport bug (a generic parallel throw also settles fine).
-    // threads=1 makes the error path deterministic. See README "Known limitation".
+    // threads=1. The SAB transport does the RIGHT thing under threads=4 too — it delivers
+    // the error and tears down cleanly (all slots freed, rings closed, worker idle;
+    // verified) — but under the FULL suite's heavy load a worker throw under threads>1
+    // *occasionally* (~2/3) DEADLOCKS DuckDB-WASM: the engine's worker thread stays blocked
+    // INSIDE the query C++ call (a `SELECT 42` on a fresh connection also hangs), so the
+    // promise never settles. This is a DuckDB-WASM parallel-error-handling deadlock, NOT a
+    // transport bug — making the transport error path fully non-blocking did NOT fix it,
+    // and a generic (non-VGI) parallel throw settles fine. threads=1 makes it deterministic.
+    // See README "Known limitation" + the probe-*.mjs isolation harness.
     await conn.query('SET threads=1');
     R.errorOk = false;
     try {