feat(duckdb): migrate duckdbClient onto WorkerBus v2, delete v1 worker - #287
Merged
Conversation
…ests Neither workers/v2/duckdb.worker.ts's handlers nor workers/v2/inference.worker.ts's handleInference were exported or unit-tested against real logic (only webllm.worker.ts had that, tests/unit/webllmWorkerHandler.test.ts) despite both already being registered as live WorkerBus v2 pools with zero production callers — docs/adr/0014-worker-generation-duplication.md's deferred migration needs these to actually match v1 before any consumer cuts over. Testing surfaced a real gap: initDuckDb()'s OPFS-unavailable catch block silently swallowed the failure, unlike v1's out-of-band OPFS_FALLBACK message that lets the UI warn users their analytics won't persist. Fixed by threading ctx.emitProgress into initDuckDb() and emitting an 'opfs-fallback' progress stage from the catch block — reuses the existing progress channel WebLLM already uses for download progress, no new message-protocol/schema change needed. First PR of the ADR-0014 consolidation sprint (5 PRs total, see .claude/plans/worker-generation-v1-vs-enumerated-fox.md). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…S-v3 format CodeRabbit review on #286 found a real bug: initDuckDb()'s OPFS catch block overwrote `connection` with the fallback connection without closing the first (partially-attached) one, leaking a DuckDB connection on every OPFS-attach failure. Fixed by tracking the OPFS attempt in its own local variable and closing it in the catch path before falling back. New regression test asserts close() is called. Also collapsed several multi-line QNBS-v3 comments in workers/v2/duckdb.worker.ts, workers/v2/inference.worker.ts, and both new test files to the required single bracketed-line format. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Second of 5 PRs consolidating the v1/WorkerBus-v2 worker duplication
(ADR-0014). services/duckdb/duckdbClient.ts's public API (init/query/
exec/shutdown/terminate/setOpfsFallbackHandler) is unchanged — its 5
real consumers (useDuckDb.ts, duckdbAnalytics.ts, duckdbMigration.ts,
ragVectorMigration.ts, telemetryService.ts) need no changes. Only the
internals swapped from a raw `new Worker(duckdbWorker.ts)` to routing
through the shared WorkerBus v2 'duckdb' pool via a new
ensureDuckDbPool() (mirrors ensureWebLlmPool()'s decoupling from the
enableWorkerBusV2 flag — analytics is core, not experimental infra).
The v2 worker returns raw values and throws on failure, unlike v1's
{ok,rows,error} wrapper — the adapter in duckdbClient.ts's send()
translates at that one boundary instead of touching ~20 call sites
across 5 files. WorkerBus's default 2-retry is disabled per-call
(retryPolicy: {maxRetries:0}) to avoid stacking under
duckdbAnalytics.ts's existing withDuckDbRetry() app-level retry.
Two things surfaced while wiring this up, both fixed here:
- WorkerBus.shutdown() tears down all 4 pools with no way to scope to
just 'duckdb' — added WorkerBus.terminatePool(poolId) so
duckdbClient.terminate() doesn't kill in-flight inference/webllm/
plugin work on the shared bus.
- send() is async and awaits ensureDuckDbPool() before a task handle
exists; naively attaching the AbortSignal listener after that await
(like a first draft did) can lose a same-tick abort, unlike v1's
synchronous Promise-executor version. Fixed by attaching the
listener synchronously up front and applying it once the handle is
ready — regression-tested.
workers/duckdbWorker.ts deleted; features/featureCatalog.ts's
enableDuckDbAnalytics entry retargeted to workers/v2/duckdb.worker.ts.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughDuckDB transport moved from a dedicated worker to WorkerBus v2. WorkerBus now supports pool detection, targeted termination, and task cancellation. DuckDB handlers support OPFS fallback progress and parameterized SQL, with expanded tests and updated catalog documentation. ChangesDuckDB v2 migration and handler coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant WorkerBus
participant DuckDBWorker
participant DuckDB
Client->>WorkerBus: enqueue db.duckdb.query
WorkerBus->>DuckDBWorker: dispatch query task
DuckDBWorker->>DuckDB: run SQL with params
DuckDB-->>DuckDBWorker: return rows
DuckDBWorker-->>WorkerBus: return task result and progress
WorkerBus-->>Client: return response and latency
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
🏁 CodeAnt Quality Gate ResultsCommit: ✅ Overall Status: PASSEDQuality Gate Details
|
…ailure CodeRabbit's second pass on #286 flagged this as an outside-diff-range comment (not a resolvable inline thread) on bcc41c2's own fix: the new `await opfsConnection?.close()` call could itself reject, which would skip emitProgress('opfs-fallback', ...) and the fallback newDb.connect(), and replace the original ATTACH error with the close failure — turning a graceful degrade into a hard init failure. Wrapped the cleanup in its own try/catch (best-effort, logged via console.warn, never blocks the fallback path) so a failing close() can't prevent the fallback connection or swallow the real error. New regression test asserts both still happen when close() rejects. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
tests/unit/inferenceWorkerHandlerV2.test.ts (1)
28-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReset mock implementations between tests.
vi.clearAllMocks()clears call history but can preservemockPipelineFactory’s implementation. Usevi.resetAllMocks()(ormockPipelineFactory.mockReset()) so future tests cannot inherit behavior from earlier cases.Suggested change
beforeEach(() => { - vi.clearAllMocks(); + vi.resetAllMocks(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/inferenceWorkerHandlerV2.test.ts` around lines 28 - 30, Update the beforeEach setup in inferenceWorkerHandlerV2 tests to reset mock implementations, replacing vi.clearAllMocks() with vi.resetAllMocks() or resetting mockPipelineFactory directly. Preserve the existing per-test mock cleanup while ensuring implementations cannot leak between tests.Source: Coding guidelines
services/duckdb/duckdbClient.ts (1)
54-63: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAbort listener is never detached on completion.
{ once: true }only removes the listener after it fires. A caller reusing oneAbortSignalacross many queries (a common hook/thunk pattern) accumulates one listener per call for the signal's lifetime. Capture the removal and run it in afinally.♻️ Sketch
- signal?.addEventListener( - 'abort', - () => { - abortedEarly = true; - handleRef?.cancel('Aborted'); - }, - { once: true }, - ); + // QNBS-v3: [Detached in the finally below so callers reusing one AbortSignal don't accumulate listeners.] + const onAbort = () => { + abortedEarly = true; + handleRef?.cancel('Aborted'); + }; + signal?.addEventListener('abort', onAbort, { once: true });…and wrap the
try/catchresult mapping withfinally { signal?.removeEventListener('abort', onAbort); }.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/duckdb/duckdbClient.ts` around lines 54 - 63, Update the abort handling around abortedEarly and handleRef to define a named onAbort listener, retain the existing cancellation behavior, and remove that listener in a finally block after the query’s try/catch result mapping completes. Ensure cleanup occurs for successful, failed, and aborted queries while preserving the current abort semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/worker-bus/src/workerBus.ts`:
- Around line 206-211: Update WorkerBus.terminatePool to cancel or reject all
tasks routed to poolId with POOL_TERMINATED before removing the pool, ensuring
awaiting callers settle; in services/workerBusManager.ts, update the DuckDB
initialization path to re-register the duckdb pool when _bus exists but that
pool is absent, rather than returning solely because the bus is non-null.
In `@services/duckdb/duckdbClient.ts`:
- Around line 71-86: The DuckDB worker can respawn without its connection
initialized, causing subsequent QUERY or EXEC tasks to fail. Update the worker
initialization flow around db.duckdb.init so every newly spawned worker
establishes its connection before handling tasks, or configure the pool to
remain warm instead of tearing down with minWorkers: 0; preserve existing task
dispatch behavior.
In `@workers/v2/duckdb.worker.ts`:
- Around line 96-110: Update handleQuery and handleExec to preserve the client
payload’s params instead of silently ignoring them. Use DuckDB’s
prepared-statement binding flow with the requested SQL and parameter values, or
explicitly reject requests containing params before executing; ensure both
handlers avoid passing unbound client SQL directly to connection.query.
- Around line 79-88: Update the OPFS fallback handling in initDuckDb so failures
from opfsConnection?.close() are ignored and cannot abort the catch block.
Ensure emitProgress('opfs-fallback', ...) and newDb.connect() still execute when
cleanup rejects, while preserving the existing error reporting.
---
Nitpick comments:
In `@services/duckdb/duckdbClient.ts`:
- Around line 54-63: Update the abort handling around abortedEarly and handleRef
to define a named onAbort listener, retain the existing cancellation behavior,
and remove that listener in a finally block after the query’s try/catch result
mapping completes. Ensure cleanup occurs for successful, failed, and aborted
queries while preserving the current abort semantics.
In `@tests/unit/inferenceWorkerHandlerV2.test.ts`:
- Around line 28-30: Update the beforeEach setup in inferenceWorkerHandlerV2
tests to reset mock implementations, replacing vi.clearAllMocks() with
vi.resetAllMocks() or resetting mockPipelineFactory directly. Preserve the
existing per-test mock cleanup while ensuring implementations cannot leak
between tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f28cfe7d-1a5d-4202-b94f-d551bf81eddb
📒 Files selected for processing (11)
features/featureCatalog.tspackages/worker-bus/src/workerBus.tspackages/worker-bus/tests/workerBus.test.tsservices/duckdb/duckdbClient.tsservices/workerBusManager.tstests/unit/duckdbClient.test.tstests/unit/duckdbWorkerHandler.test.tstests/unit/inferenceWorkerHandlerV2.test.tsworkers/duckdbWorker.tsworkers/v2/duckdb.worker.tsworkers/v2/inference.worker.ts
💤 Files with no reviewable changes (1)
- workers/duckdbWorker.ts
5 tasks
…QL param binding Three real findings from CodeRabbit's review of #287 (duckdbClient migration onto WorkerBus v2), all confirmed against current code before fixing: 1. CRITICAL — params silently dropped (workers/v2/duckdb.worker.ts). duckdbClient.send() enqueues {sql, params}, but handleQuery/ handleExec only read req.sql and called connection.query(sql) directly, ignoring params entirely and leaving an unbound-query/ SQL-injection surface flagged by static analysis. This was already live: services/ai/telemetryService.ts's writeToDuckDb() passes real bound params ([taskType, backend, modelId, latencyMs, success]) that were being silently discarded. Fixed via DuckDB-WASM prepared statements (connection.prepare(sql) + stmt.query(...params) + stmt.close()) when params is non-empty; unchanged direct connection.query(sql) otherwise. Needed a narrow structural type (PreparableConnection) since tsgo doesn't resolve AsyncDuckDBConnection.prepare() through this package's nested `export *` chain, despite it being present in the package's own .d.ts — same class of tsgo/external-package gap CLAUDE.md already documents for the transformers.js path alias. 2. MAJOR — DuckDB pool worker losing its connection on respawn (services/duckdb/duckdbClient.ts). The duckdb pool's minWorkers:0 lets its worker idle-terminate; a freshly spawned replacement worker's module state has no `connection` until INIT runs again on it, so the next QUERY/EXEC after a respawn failed with "DuckDB not initialized". Fixed by transparently re-sending INIT once and retrying the original call when send() sees that specific error message (bounded to one retry, INIT/SHUTDOWN excluded from the check to avoid any recursive loop). 3. MAJOR — terminatePool() had no lifecycle contract (packages/worker-bus/src/workerBus.ts + services/workerBusManager.ts). Removing a pool didn't settle tasks already routed to it — their result promises would await a RESULT message from a worker that no longer exists. Fixed by tracking task->pool assignment (new taskPools map, set/cleared around runTask's pool-acquire phase) and cancelling matching tasks before deletion. Separately, ensureDuckDbPool() assumed a non-null bus always has every pool, so any call after duckdbClient.terminate() would fail with NO_POOL forever; added WorkerBus.hasPool() and a re-registration path (duckDbPoolOptions()/reRegisterDuckDbPool(), shared with doInitWorkerBus() to avoid option drift between the two registration sites). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
workers/v2/duckdb.worker.ts (1)
79-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute the OPFS-close-failure warning through
services/logger.tsinstead of rawconsole.warn.The best-effort try/catch fix itself is correct (matches the previously-flagged requirement that a rejecting
close()must not block the fallback or replace the original error). However, line 84 logs viaconsole.warndirectly rather than the shared logger, diverging from the convention this same PR already follows inservices/workerBusManager.ts(log.info/log.error).♻️ Proposed fix
+import { log } from '../../services/logger'; ... } catch (closeErr) { - console.warn('[duckdb.worker] Failed to close partial OPFS connection', closeErr); + log.warn('[duckdb.worker] Failed to close partial OPFS connection', closeErr); }As per coding guidelines, "technical details may only be emitted through
services/logger.tsusing Biome-compliantwarnorerror."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workers/v2/duckdb.worker.ts` around lines 79 - 93, Replace the raw console.warn in the OPFS close-error catch within the worker’s fallback flow with the shared logger from services/logger.ts, using its Biome-compliant warn or error method and preserving the existing message and closeErr details.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@workers/v2/duckdb.worker.ts`:
- Around line 79-93: Replace the raw console.warn in the OPFS close-error catch
within the worker’s fallback flow with the shared logger from
services/logger.ts, using its Biome-compliant warn or error method and
preserving the existing message and closeErr details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 10cd09b6-f42b-471b-b59f-40b60c9de8d8
📒 Files selected for processing (9)
README.mdpackages/worker-bus/src/workerBus.tspackages/worker-bus/tests/workerBus.test.tsservices/duckdb/duckdbClient.tsservices/workerBusManager.tstests/unit/duckdbClient.test.tstests/unit/duckdbWorkerHandler.test.tstests/unit/workerBusManager.test.tsworkers/v2/duckdb.worker.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- services/duckdb/duckdbClient.ts
- packages/worker-bus/src/workerBus.ts
- tests/unit/duckdbWorkerHandler.test.ts
This was referenced Jul 29, 2026
Squash-merging PR286 created a new commit on main with no shared ancestry recognized against this stacked branch's pre-squash history, causing an add/add and content conflict on the 2 files PR286 and PR287 both touch. This branch's version is a strict superset (has everything main has, plus PR287's own params-binding fix on top) — resolved by keeping HEAD throughout. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
qnbs
added a commit
that referenced
this pull request
Jul 29, 2026
CodeRabbit review on #291 correctly flagged that the Known Technical Debt line I edited (only updating the file path to workers/v2/inference.worker.ts) still carried a pre-existing recommendation to "restore @ts-expect-error" if the transformers.js path alias breaks — in direct tension with this repo's own suppression-ratchet policy documented a few sections above in the same file. Reworded to point at fixing the alias/type declaration directly instead, consistent with the structural-type workaround this same sprint used for a different tsgo/external-package resolution gap (workers/v2/duckdb.worker.ts's PreparableConnection, PR #287) rather than reaching for a suppression. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Matches the log.warn/log.error convention services/workerBusManager.ts already uses in this PR — services/logger.ts is worker-safe (guarded window access, lazy Tauri import), so raw console.warn was an unnecessary inconsistency, not a technical constraint. Addresses CodeRabbit nitpick on PR #287.
- duckdbClient.ts: cover the String(err) fallback branch in errorMessage(), the opfs-fallback message ?? default, and the reinit.ok === false path (INIT retry itself failing). - workerBus.ts: add direct hasPool() coverage (true/false/after terminatePool) and a throwing-subscriber test that reaches emit()'s isolation catch via the circuit-breaker-open event path (the only two call sites of emit() in the class). Addresses the Codecov "patch coverage" report on PR #287 (4 lines missing coverage across these two files).
Owner
Author
|
@CodeAnt-AI review |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
A caller that reuses one AbortSignal across many queries (a common
hook/thunk pattern) would otherwise accumulate one 'abort' listener per
call for the signal's whole lifetime, since {once:true} only self-removes
when the signal actually fires.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…eWorkerHandlerV2 tests clearAllMocks clears call history but not mockImplementation/mockResolvedValue, so a test could inherit mockPipelineFactory's return value from an earlier test instead of setting its own explicitly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/duckdbClient.test.ts (1)
343-351: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReset the OPFS handler in
afterEachinstead of at the end of the test body.If an assertion throws, line 350 never runs and the handler bleeds into later tests.
♻️ Proposed cleanup
- expect(onFallback).toHaveBeenCalledWith('OPFS unavailable'); - duckdbClient.setOpfsFallbackHandler(null); + expect(onFallback).toHaveBeenCalledWith('OPFS unavailable');Add once per suite:
afterEach(() => { duckdbClient.setOpfsFallbackHandler(null); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/duckdbClient.test.ts` around lines 343 - 351, Move the setOpfsFallbackHandler(null) cleanup from the test body into a suite-level afterEach hook so it runs even when assertions fail. Keep the test’s handler setup and expectations unchanged, and remove the redundant end-of-test reset.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/inferenceWorkerHandlerV2.test.ts`:
- Around line 29-32: Replace the three-line rationale immediately before
vi.resetAllMocks() with a single-line comment using the exact format `//
QNBS-v3: [Grund / Impact / Kreativer Mehrwert]`, while preserving the rationale
that resetAllMocks clears implementations and prevents test state leakage.
---
Nitpick comments:
In `@tests/unit/duckdbClient.test.ts`:
- Around line 343-351: Move the setOpfsFallbackHandler(null) cleanup from the
test body into a suite-level afterEach hook so it runs even when assertions
fail. Keep the test’s handler setup and expectations unchanged, and remove the
redundant end-of-test reset.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: abc8b240-bc74-46ae-ab89-e35a1a4c0a63
📒 Files selected for processing (5)
packages/worker-bus/tests/workerBus.test.tsservices/duckdb/duckdbClient.tstests/unit/duckdbClient.test.tstests/unit/inferenceWorkerHandlerV2.test.tsworkers/v2/duckdb.worker.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/worker-bus/tests/workerBus.test.ts
- duckdbClient.test.ts: move OPFS fallback-handler reset into a suite-level afterEach so it still runs when an assertion throws mid-test, instead of three end-of-test reset calls that could be skipped. - inferenceWorkerHandlerV2.test.ts: collapse the QNBS-v3 rationale comment to the required single-line format. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
qnbs
added a commit
that referenced
this pull request
Jul 30, 2026
* test(worker-bus): export v2 duckdb/inference handlers + parity gate tests Neither workers/v2/duckdb.worker.ts's handlers nor workers/v2/inference.worker.ts's handleInference were exported or unit-tested against real logic (only webllm.worker.ts had that, tests/unit/webllmWorkerHandler.test.ts) despite both already being registered as live WorkerBus v2 pools with zero production callers — docs/adr/0014-worker-generation-duplication.md's deferred migration needs these to actually match v1 before any consumer cuts over. Testing surfaced a real gap: initDuckDb()'s OPFS-unavailable catch block silently swallowed the failure, unlike v1's out-of-band OPFS_FALLBACK message that lets the UI warn users their analytics won't persist. Fixed by threading ctx.emitProgress into initDuckDb() and emitting an 'opfs-fallback' progress stage from the catch block — reuses the existing progress channel WebLLM already uses for download progress, no new message-protocol/schema change needed. First PR of the ADR-0014 consolidation sprint (5 PRs total, see .claude/plans/worker-generation-v1-vs-enumerated-fox.md). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(worker-bus): close leaked OPFS connection on ATTACH failure + QNBS-v3 format CodeRabbit review on #286 found a real bug: initDuckDb()'s OPFS catch block overwrote `connection` with the fallback connection without closing the first (partially-attached) one, leaking a DuckDB connection on every OPFS-attach failure. Fixed by tracking the OPFS attempt in its own local variable and closing it in the catch path before falling back. New regression test asserts close() is called. Also collapsed several multi-line QNBS-v3 comments in workers/v2/duckdb.worker.ts, workers/v2/inference.worker.ts, and both new test files to the required single bracketed-line format. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * feat(duckdb): migrate duckdbClient onto WorkerBus v2, delete v1 worker Second of 5 PRs consolidating the v1/WorkerBus-v2 worker duplication (ADR-0014). services/duckdb/duckdbClient.ts's public API (init/query/ exec/shutdown/terminate/setOpfsFallbackHandler) is unchanged — its 5 real consumers (useDuckDb.ts, duckdbAnalytics.ts, duckdbMigration.ts, ragVectorMigration.ts, telemetryService.ts) need no changes. Only the internals swapped from a raw `new Worker(duckdbWorker.ts)` to routing through the shared WorkerBus v2 'duckdb' pool via a new ensureDuckDbPool() (mirrors ensureWebLlmPool()'s decoupling from the enableWorkerBusV2 flag — analytics is core, not experimental infra). The v2 worker returns raw values and throws on failure, unlike v1's {ok,rows,error} wrapper — the adapter in duckdbClient.ts's send() translates at that one boundary instead of touching ~20 call sites across 5 files. WorkerBus's default 2-retry is disabled per-call (retryPolicy: {maxRetries:0}) to avoid stacking under duckdbAnalytics.ts's existing withDuckDbRetry() app-level retry. Two things surfaced while wiring this up, both fixed here: - WorkerBus.shutdown() tears down all 4 pools with no way to scope to just 'duckdb' — added WorkerBus.terminatePool(poolId) so duckdbClient.terminate() doesn't kill in-flight inference/webllm/ plugin work on the shared bus. - send() is async and awaits ensureDuckDbPool() before a task handle exists; naively attaching the AbortSignal listener after that await (like a first draft did) can lose a same-tick abort, unlike v1's synchronous Promise-executor version. Fixed by attaching the listener synchronously up front and applying it once the handle is ready — regression-tested. workers/duckdbWorker.ts deleted; features/featureCatalog.ts's enableDuckDbAnalytics entry retargeted to workers/v2/duckdb.worker.ts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(worker-bus): make OPFS-connection cleanup best-effort on ATTACH failure CodeRabbit's second pass on #286 flagged this as an outside-diff-range comment (not a resolvable inline thread) on bcc41c2's own fix: the new `await opfsConnection?.close()` call could itself reject, which would skip emitProgress('opfs-fallback', ...) and the fallback newDb.connect(), and replace the original ATTACH error with the close failure — turning a graceful degrade into a hard init failure. Wrapped the cleanup in its own try/catch (best-effort, logged via console.warn, never blocks the fallback path) so a failing close() can't prevent the fallback connection or swallow the real error. New regression test asserts both still happen when close() rejects. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * feat(ai): migrate localEmbeddingService onto WorkerBus v2 Third of 5 PRs consolidating the v1/WorkerBus-v2 worker duplication (ADR-0014). services/ai/localEmbeddingService.ts's public API (embedText/embedBatch/cosineSimilarity/clearEmbeddingCache) is unchanged — its consumers (localRagService.ts, crossProjectIndexService.ts, ragPromptAssembly.ts, ragVectorMigration.ts, proForgeMemoryBank.ts, loraEvaluationService.ts, loraDatasetBuilder.ts, inferenceGateway.ts) need no changes. Internals swapped from a dedicated `new Worker(inference.worker.ts)` instance to the shared WorkerBus v2 'inference' pool via ensureInferencePool() (same decoupling-from- enableWorkerBusV2 pattern as ensureWebLlmPool/ensureDuckDbPool). v2's handler already throws on failure (matching embedText's existing throw-on-failure contract), so no response-shape translation is needed here — just swap the transport. Deleted ~70 lines of hand-rolled health-check/restart/backoff logic (30s ping, exponential backoff up to 5 attempts), superseded by WorkerPool's generic health check. Capped the shared 'inference' pool's maxWorkers to 2 (was MAX_WORKERS_INFERENCE=4) in workerBusManager.ts: each pool replica independently loads its own transformers.js pipeline with no cross-replica cache sharing, so 4 concurrent workers under a burst (e.g. embedBatch's micro-batching) could mean 4x the model memory footprint. workers/inference.worker.ts (v1) is NOT deleted yet — localNlpService.ts still uses it directly; removal is PR 4 of this sequence, after that migration lands too. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(worker-bus): pool-termination lifecycle, idle-respawn recovery, SQL param binding Three real findings from CodeRabbit's review of #287 (duckdbClient migration onto WorkerBus v2), all confirmed against current code before fixing: 1. CRITICAL — params silently dropped (workers/v2/duckdb.worker.ts). duckdbClient.send() enqueues {sql, params}, but handleQuery/ handleExec only read req.sql and called connection.query(sql) directly, ignoring params entirely and leaving an unbound-query/ SQL-injection surface flagged by static analysis. This was already live: services/ai/telemetryService.ts's writeToDuckDb() passes real bound params ([taskType, backend, modelId, latencyMs, success]) that were being silently discarded. Fixed via DuckDB-WASM prepared statements (connection.prepare(sql) + stmt.query(...params) + stmt.close()) when params is non-empty; unchanged direct connection.query(sql) otherwise. Needed a narrow structural type (PreparableConnection) since tsgo doesn't resolve AsyncDuckDBConnection.prepare() through this package's nested `export *` chain, despite it being present in the package's own .d.ts — same class of tsgo/external-package gap CLAUDE.md already documents for the transformers.js path alias. 2. MAJOR — DuckDB pool worker losing its connection on respawn (services/duckdb/duckdbClient.ts). The duckdb pool's minWorkers:0 lets its worker idle-terminate; a freshly spawned replacement worker's module state has no `connection` until INIT runs again on it, so the next QUERY/EXEC after a respawn failed with "DuckDB not initialized". Fixed by transparently re-sending INIT once and retrying the original call when send() sees that specific error message (bounded to one retry, INIT/SHUTDOWN excluded from the check to avoid any recursive loop). 3. MAJOR — terminatePool() had no lifecycle contract (packages/worker-bus/src/workerBus.ts + services/workerBusManager.ts). Removing a pool didn't settle tasks already routed to it — their result promises would await a RESULT message from a worker that no longer exists. Fixed by tracking task->pool assignment (new taskPools map, set/cleared around runTask's pool-acquire phase) and cancelling matching tasks before deletion. Separately, ensureDuckDbPool() assumed a non-null bus always has every pool, so any call after duckdbClient.terminate() would fail with NO_POOL forever; added WorkerBus.hasPool() and a re-registration path (duckDbPoolOptions()/reRegisterDuckDbPool(), shared with doInitWorkerBus() to avoid option drift between the two registration sites). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ai): add the missing ensureInferencePool export that broke prod builds Root cause of both Vercel deployment failures on this branch/PR chain: services/ai/localEmbeddingService.ts imports and calls ensureInferencePool() from workerBusManager.ts, but that function was never actually added there — only ensureDuckDbPool() was. Vitest never caught this because localEmbeddingService.test.ts mocks the entire workerBusManager module (vi.mock(...) replaces it wholesale), so the real file's missing export was invisible to that suite. tsgo's typecheck also passed clean locally for reasons still unclear (worth a follow-up look), but rolldown's stricter static bundling in the actual `pnpm run build:edge` (Vercel's exact build command) correctly failed hard with [MISSING_EXPORT] — reproduced locally with `NODE_OPTIONS=--max-old-space-size=3072 pnpm run build:edge`, which is what actually surfaced this. Added ensureInferencePool(), mirroring ensureDuckDbPool()'s shape (decoupled from enableWorkerBusV2, re-registers the pool if it was removed via terminatePool() while the bus stays alive). Refactored doInitWorkerBus()'s inline 'inference' pool registration to share the same inferencePoolOptions() the new function uses, avoiding the same kind of drift the duckdb pool options already guard against. New tests in workerBusManager.test.ts import the *real* module (only @domain/worker-bus is mocked) — this exact suite would have caught the missing export immediately, unlike the fully-mocked consumer-side test. Verified fixed via a full local `pnpm run build:edge` reproduction of Vercel's exact build command (previously failed with [MISSING_EXPORT], now builds clean). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(worker-bus): guard pool re-registration failures in workerBusManager ensureDuckDbPool() and ensureInferencePool() could reject after _bus was already live because their re-registration helpers awaited a lazy import and called registerPool() without a catch, breaking the documented "null only if init failed" contract. Wrap both paths so a failed re-add logs instead of propagating, and add a maxWorkers:2 assertion to the inference re-registration test so the pool cap regression would be caught. Addresses CodeRabbit review comments on PR #288. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(worker-bus): close Codecov patch-coverage gap on PR #288 The prior CodeRabbit fix (8a63b7c) added try/catch guards in ensureDuckDbPool()/ensureInferencePool() so a pool re-registration failure logs instead of propagating, but no test exercised the catch path — Codecov flagged 4 missing lines (83.33% patch coverage). Adds one test per function that forces registerPool() to throw and asserts the live bus is still returned and the failure is logged via services/logger.ts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(worker-bus): add QNBS-v3 rationale comments per CodeRabbit outside-diff-range review Addresses CodeRabbit's 2 still-valid findings on PR #288: the logger mock (why it's isolated from the real StructuredLogger) and the maxWorkers:2 regression assertion (why 2, not just what). The other 3 flagged spots (reRegisterInferencePool, the inference registry.register block, and the mockLogError.mockClear() line) mirror already-uncommented sibling code in the same files (reRegisterDuckDbPool, the duckdb registry.register block, 6 other .mockClear() calls) — commenting only the new instance would be less consistent, not more, so those are left as-is. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
qnbs
added a commit
that referenced
this pull request
Jul 30, 2026
…nce worker (#290) * test(worker-bus): export v2 duckdb/inference handlers + parity gate tests Neither workers/v2/duckdb.worker.ts's handlers nor workers/v2/inference.worker.ts's handleInference were exported or unit-tested against real logic (only webllm.worker.ts had that, tests/unit/webllmWorkerHandler.test.ts) despite both already being registered as live WorkerBus v2 pools with zero production callers — docs/adr/0014-worker-generation-duplication.md's deferred migration needs these to actually match v1 before any consumer cuts over. Testing surfaced a real gap: initDuckDb()'s OPFS-unavailable catch block silently swallowed the failure, unlike v1's out-of-band OPFS_FALLBACK message that lets the UI warn users their analytics won't persist. Fixed by threading ctx.emitProgress into initDuckDb() and emitting an 'opfs-fallback' progress stage from the catch block — reuses the existing progress channel WebLLM already uses for download progress, no new message-protocol/schema change needed. First PR of the ADR-0014 consolidation sprint (5 PRs total, see .claude/plans/worker-generation-v1-vs-enumerated-fox.md). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(worker-bus): close leaked OPFS connection on ATTACH failure + QNBS-v3 format CodeRabbit review on #286 found a real bug: initDuckDb()'s OPFS catch block overwrote `connection` with the fallback connection without closing the first (partially-attached) one, leaking a DuckDB connection on every OPFS-attach failure. Fixed by tracking the OPFS attempt in its own local variable and closing it in the catch path before falling back. New regression test asserts close() is called. Also collapsed several multi-line QNBS-v3 comments in workers/v2/duckdb.worker.ts, workers/v2/inference.worker.ts, and both new test files to the required single bracketed-line format. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * feat(duckdb): migrate duckdbClient onto WorkerBus v2, delete v1 worker Second of 5 PRs consolidating the v1/WorkerBus-v2 worker duplication (ADR-0014). services/duckdb/duckdbClient.ts's public API (init/query/ exec/shutdown/terminate/setOpfsFallbackHandler) is unchanged — its 5 real consumers (useDuckDb.ts, duckdbAnalytics.ts, duckdbMigration.ts, ragVectorMigration.ts, telemetryService.ts) need no changes. Only the internals swapped from a raw `new Worker(duckdbWorker.ts)` to routing through the shared WorkerBus v2 'duckdb' pool via a new ensureDuckDbPool() (mirrors ensureWebLlmPool()'s decoupling from the enableWorkerBusV2 flag — analytics is core, not experimental infra). The v2 worker returns raw values and throws on failure, unlike v1's {ok,rows,error} wrapper — the adapter in duckdbClient.ts's send() translates at that one boundary instead of touching ~20 call sites across 5 files. WorkerBus's default 2-retry is disabled per-call (retryPolicy: {maxRetries:0}) to avoid stacking under duckdbAnalytics.ts's existing withDuckDbRetry() app-level retry. Two things surfaced while wiring this up, both fixed here: - WorkerBus.shutdown() tears down all 4 pools with no way to scope to just 'duckdb' — added WorkerBus.terminatePool(poolId) so duckdbClient.terminate() doesn't kill in-flight inference/webllm/ plugin work on the shared bus. - send() is async and awaits ensureDuckDbPool() before a task handle exists; naively attaching the AbortSignal listener after that await (like a first draft did) can lose a same-tick abort, unlike v1's synchronous Promise-executor version. Fixed by attaching the listener synchronously up front and applying it once the handle is ready — regression-tested. workers/duckdbWorker.ts deleted; features/featureCatalog.ts's enableDuckDbAnalytics entry retargeted to workers/v2/duckdb.worker.ts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(worker-bus): make OPFS-connection cleanup best-effort on ATTACH failure CodeRabbit's second pass on #286 flagged this as an outside-diff-range comment (not a resolvable inline thread) on bcc41c2's own fix: the new `await opfsConnection?.close()` call could itself reject, which would skip emitProgress('opfs-fallback', ...) and the fallback newDb.connect(), and replace the original ATTACH error with the close failure — turning a graceful degrade into a hard init failure. Wrapped the cleanup in its own try/catch (best-effort, logged via console.warn, never blocks the fallback path) so a failing close() can't prevent the fallback connection or swallow the real error. New regression test asserts both still happen when close() rejects. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * feat(ai): migrate localEmbeddingService onto WorkerBus v2 Third of 5 PRs consolidating the v1/WorkerBus-v2 worker duplication (ADR-0014). services/ai/localEmbeddingService.ts's public API (embedText/embedBatch/cosineSimilarity/clearEmbeddingCache) is unchanged — its consumers (localRagService.ts, crossProjectIndexService.ts, ragPromptAssembly.ts, ragVectorMigration.ts, proForgeMemoryBank.ts, loraEvaluationService.ts, loraDatasetBuilder.ts, inferenceGateway.ts) need no changes. Internals swapped from a dedicated `new Worker(inference.worker.ts)` instance to the shared WorkerBus v2 'inference' pool via ensureInferencePool() (same decoupling-from- enableWorkerBusV2 pattern as ensureWebLlmPool/ensureDuckDbPool). v2's handler already throws on failure (matching embedText's existing throw-on-failure contract), so no response-shape translation is needed here — just swap the transport. Deleted ~70 lines of hand-rolled health-check/restart/backoff logic (30s ping, exponential backoff up to 5 attempts), superseded by WorkerPool's generic health check. Capped the shared 'inference' pool's maxWorkers to 2 (was MAX_WORKERS_INFERENCE=4) in workerBusManager.ts: each pool replica independently loads its own transformers.js pipeline with no cross-replica cache sharing, so 4 concurrent workers under a burst (e.g. embedBatch's micro-batching) could mean 4x the model memory footprint. workers/inference.worker.ts (v1) is NOT deleted yet — localNlpService.ts still uses it directly; removal is PR 4 of this sequence, after that migration lands too. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(worker-bus): pool-termination lifecycle, idle-respawn recovery, SQL param binding Three real findings from CodeRabbit's review of #287 (duckdbClient migration onto WorkerBus v2), all confirmed against current code before fixing: 1. CRITICAL — params silently dropped (workers/v2/duckdb.worker.ts). duckdbClient.send() enqueues {sql, params}, but handleQuery/ handleExec only read req.sql and called connection.query(sql) directly, ignoring params entirely and leaving an unbound-query/ SQL-injection surface flagged by static analysis. This was already live: services/ai/telemetryService.ts's writeToDuckDb() passes real bound params ([taskType, backend, modelId, latencyMs, success]) that were being silently discarded. Fixed via DuckDB-WASM prepared statements (connection.prepare(sql) + stmt.query(...params) + stmt.close()) when params is non-empty; unchanged direct connection.query(sql) otherwise. Needed a narrow structural type (PreparableConnection) since tsgo doesn't resolve AsyncDuckDBConnection.prepare() through this package's nested `export *` chain, despite it being present in the package's own .d.ts — same class of tsgo/external-package gap CLAUDE.md already documents for the transformers.js path alias. 2. MAJOR — DuckDB pool worker losing its connection on respawn (services/duckdb/duckdbClient.ts). The duckdb pool's minWorkers:0 lets its worker idle-terminate; a freshly spawned replacement worker's module state has no `connection` until INIT runs again on it, so the next QUERY/EXEC after a respawn failed with "DuckDB not initialized". Fixed by transparently re-sending INIT once and retrying the original call when send() sees that specific error message (bounded to one retry, INIT/SHUTDOWN excluded from the check to avoid any recursive loop). 3. MAJOR — terminatePool() had no lifecycle contract (packages/worker-bus/src/workerBus.ts + services/workerBusManager.ts). Removing a pool didn't settle tasks already routed to it — their result promises would await a RESULT message from a worker that no longer exists. Fixed by tracking task->pool assignment (new taskPools map, set/cleared around runTask's pool-acquire phase) and cancelling matching tasks before deletion. Separately, ensureDuckDbPool() assumed a non-null bus always has every pool, so any call after duckdbClient.terminate() would fail with NO_POOL forever; added WorkerBus.hasPool() and a re-registration path (duckDbPoolOptions()/reRegisterDuckDbPool(), shared with doInitWorkerBus() to avoid option drift between the two registration sites). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ai): add the missing ensureInferencePool export that broke prod builds Root cause of both Vercel deployment failures on this branch/PR chain: services/ai/localEmbeddingService.ts imports and calls ensureInferencePool() from workerBusManager.ts, but that function was never actually added there — only ensureDuckDbPool() was. Vitest never caught this because localEmbeddingService.test.ts mocks the entire workerBusManager module (vi.mock(...) replaces it wholesale), so the real file's missing export was invisible to that suite. tsgo's typecheck also passed clean locally for reasons still unclear (worth a follow-up look), but rolldown's stricter static bundling in the actual `pnpm run build:edge` (Vercel's exact build command) correctly failed hard with [MISSING_EXPORT] — reproduced locally with `NODE_OPTIONS=--max-old-space-size=3072 pnpm run build:edge`, which is what actually surfaced this. Added ensureInferencePool(), mirroring ensureDuckDbPool()'s shape (decoupled from enableWorkerBusV2, re-registers the pool if it was removed via terminatePool() while the bus stays alive). Refactored doInitWorkerBus()'s inline 'inference' pool registration to share the same inferencePoolOptions() the new function uses, avoiding the same kind of drift the duckdb pool options already guard against. New tests in workerBusManager.test.ts import the *real* module (only @domain/worker-bus is mocked) — this exact suite would have caught the missing export immediately, unlike the fully-mocked consumer-side test. Verified fixed via a full local `pnpm run build:edge` reproduction of Vercel's exact build command (previously failed with [MISSING_EXPORT], now builds clean). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * feat(ai): migrate localNlpService onto WorkerBus v2, delete v1 inference worker Fourth and final migration PR of the worker-generation consolidation (ADR-0014). services/ai/localNlpService.ts's public API (analyzeSentiment/summarizeText/classifyWritingTopic) is unchanged — confirmed zero real production callers beyond its own tests and a barrel re-export in services/ai/index.ts, so this is the lowest-blast-radius of the three migrations. Internals swapped from a dedicated `new Worker(inference.worker.ts)` instance to the shared WorkerBus v2 'inference' pool via ensureInferencePool() (added in the previous PR), routing to the 'inference.text' capability. analyzeSentiment/summarizeText already had their own app-level graceful-degrade behavior on failure (NEUTRAL sentiment / truncated text) — that's preserved unchanged via try/catch around the new throw-on-failure requestInference() call, replacing the old ok:false-check. With both localEmbeddingService.ts (previous PR) and this migrated, workers/inference.worker.ts (v1) has no remaining callers — deleted, along with its dedicated test (tests/unit/inferenceWorker.test.ts). tests/unit/localNlpService.test.ts rewritten against a workerBusManager mock; tests/unit/services/localNlpService.test.ts deleted as a near-total duplicate of the rewritten suite (same module, overlapping classifyWritingTopic/analyzeSentiment coverage, the comprehensive file already had everything it did). Verified via typecheck and a full local `pnpm run build:edge` reproduction of Vercel's exact build command (the lesson from the previous PR's missing-export incident) — both clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ai): omit inferenceOptions key instead of passing undefined (exactOptionalPropertyTypes) localNlpService.requestInference() spread inferenceOptions into the enqueue payload via object-literal shorthand, so calls without a 4th arg (analyzeSentiment) set the key to an explicit `undefined`. With exactOptionalPropertyTypes the optional payload field only accepts the property being absent, not present-with-undefined. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(ai): close Codecov patch-coverage gap on PR #290 Codecov flagged 3 partial branches in localNlpService.ts (83.33% patch coverage). Two were genuinely reachable and now have tests: - ensureInferencePool() returning null (WorkerBus unavailable) - the worker response omitting ":score" (scoreRaw undefined, ?? '0.5' fallback) The third — labelRaw's `?? 'NEUTRAL'` fallback — was dead code: String.split(':') never returns an empty array, so labelRaw (index 0) can never actually be undefined; only TypeScript's noUncheckedIndexedAccess couldn't see that. Replaced with a targeted `as string` assertion (compile-time only, no runtime branch) instead of writing a contrived test for an unreachable path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(test): require an enqueue call before asserting payload shape Addresses a CodeRabbit finding on PR #290: the exactOptionalPropertyTypes test used `requestCalls[0] ?? {}`, so it would still pass on an empty {} object if analyzeSentiment failed to enqueue at all — a regression in the enqueue path wouldn't be caught. Assert the call actually happened first. The other finding on this review (missing QNBS-v3 comment on the two new fallback tests) doesn't hold: none of the 8 tests in this describe block have individual QNBS-v3 comments, only the file-level one — adding it to just the 2 new tests would be less consistent, not more. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Second of 5 PRs consolidating the v1/WorkerBus-v2 worker duplication (ADR-0014). Stacked on #286 (parity gate) — base branch is that PR's branch and will auto-retarget to
mainonce it merges. Plan:.claude/plans/worker-generation-v1-vs-enumerated-fox.md.services/duckdb/duckdbClient.ts's public API (init/query/exec/shutdown/terminate/setOpfsFallbackHandler) is unchanged — its 5 real consumers (useDuckDb.ts,duckdbAnalytics.ts,duckdbMigration.ts,ragVectorMigration.ts,telemetryService.ts) need no changes. Only the internals swapped from a rawnew Worker(duckdbWorker.ts)to the shared WorkerBus v2'duckdb'pool via a newensureDuckDbPool()— mirrorsensureWebLlmPool()'s decoupling fromenableWorkerBusV2(analytics is core, not experimental infra; toggling that flag off must not silently break it).{ok,rows,error}) — the adapter induckdbClient.ts'ssend()translates at that one boundary instead of touching ~20 call sites across 5 files.retryPolicy: {maxRetries:0}) to avoid stacking underduckdbAnalytics.ts's existingwithDuckDbRetry()app-level retry (up to 9 attempts otherwise).Two things surfaced while wiring this up, both fixed here:
WorkerBus.shutdown()tears down all 4 pools with no way to scope to just'duckdb'— addedWorkerBus.terminatePool(poolId)soduckdbClient.terminate()doesn't kill in-flight inference/webllm/plugin work on the shared bus.send()is async and awaitsensureDuckDbPool()before a task handle exists; a first draft attached theAbortSignallistener after that await, which can lose a same-tick abort — unlike v1's synchronous Promise-executor version, where the listener was always attached beforesend()returned. Fixed by attaching the listener synchronously up front and applying it once the handle is ready; added a regression test for exactly this timing.workers/duckdbWorker.tsdeleted.features/featureCatalog.ts'senableDuckDbAnalyticsentry retargeted toworkers/v2/duckdb.worker.ts.Test plan
pnpm exec vitest run tests/unit/duckdbClient.test.ts tests/unit/duckdbWorkerHandler.test.ts packages/worker-bus/tests/workerBus.test.ts tests/unit/ragVectorMigration.test.ts tests/unit/duckdbMigration.test.ts tests/unit/duckdbAnalytics.test.ts tests/unit/services/ai/telemetryService.test.ts tests/unit/hooks/useDuckDb.test.ts— 128/128 passing (rewrittenduckdbClient.test.ts+ 2 newWorkerBus.terminatePooltests + all 5 higher-level consumer suites unaffected, since the public API didn't change)pnpm run typecheck(exact CI command) — cleanpnpm run lint(Biome) — clean via pre-commit hook🤖 Generated with Claude Code
Summary by CodeRabbit