feat(ai): migrate localEmbeddingService onto WorkerBus v2 - #288
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>
…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>
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>
|
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:
📝 WalkthroughWalkthroughEmbedding generation now uses WorkerBus v2’s shared inference pool instead of a directly managed worker. Inference pool configuration and recovery are centralized, and tests cover task routing, errors, caching, batching, and pool re-registration. ChangesWorkerBus inference flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant embedText
participant requestEmbedding
participant ensureInferencePool
participant WorkerBus
participant InferenceWorker
embedText->>requestEmbedding: submit embedding input
requestEmbedding->>ensureInferencePool: acquire inference pool
requestEmbedding->>WorkerBus: enqueue inference.embed
WorkerBus->>InferenceWorker: execute task
InferenceWorker-->>WorkerBus: return raw vector
WorkerBus-->>requestEmbedding: return vector
requestEmbedding-->>embedText: normalize vector
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…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>
4 tasks
…uilds 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>
qnbs
added a commit
that referenced
this pull request
Jul 29, 2026
Root-caused the 2 Vercel build failures that prompted the mid-term pause: not a platform issue, but a real missing-export bug (services/ai/localEmbeddingService.ts imported ensureInferencePool() from workerBusManager.ts, which never actually defined it — invisible to Vitest because the consumer test mocks the whole module). The fix landed in the worker-generation-consolidation PR chain (PR #288). Reverts git.deploymentEnabled to the default (enabled) now that the actual cause is fixed and verified via a clean local `build:edge` reproduction of Vercel's exact build command. TODO.md's entry updated from "paused, cause unknown" to a resolved write-up, including the `pnpm run build` vs `pnpm run build:edge` command-mismatch lesson that delayed finding this. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Jul 29, 2026
🏁 CodeAnt Quality Gate ResultsCommit: ✅ Overall Status: PASSEDQuality Gate Details
|
…ing-migration # Conflicts: # packages/worker-bus/tests/workerBus.test.ts # services/duckdb/duckdbClient.ts # services/workerBusManager.ts # tests/unit/duckdbClient.test.ts # tests/unit/inferenceWorkerHandlerV2.test.ts # tests/unit/workerBusManager.test.ts # workers/v2/duckdb.worker.ts
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@services/workerBusManager.ts`:
- Around line 86-91: Update ensureInferencePool() and ensureDuckDbPool() so
failures from reRegisterInferencePool() and the corresponding DuckDB
re-registration helper are caught after _bus is initialized. Log each
re-registration error and preserve the live bus instead of propagating the
rejection, while retaining null returns only for initial initialization
failures.
In `@tests/unit/workerBusManager.test.ts`:
- Around line 222-238: Add an assertion to the re-registration test around
ensureInferencePool and mockRegisterPool verifying the registration options
include maxWorkers: 2, so the test fails if the inference pool cap regresses to
the previous default.
🪄 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: 5cc31091-4682-4854-aed5-005697a92001
📒 Files selected for processing (4)
services/ai/localEmbeddingService.tsservices/workerBusManager.tstests/unit/localEmbeddingService.test.tstests/unit/workerBusManager.test.ts
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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>
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/workerBusManager.ts (1)
87-90: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winApply the required adjacent QNBS-v3 rationale comments consistently.
services/workerBusManager.ts#L87-L90: add a one-line QNBS-v3 comment explaining missing-pool inference re-registration.services/workerBusManager.ts#L125-L130: add a one-line QNBS-v3 comment explaining centralized inference-pool configuration.tests/unit/workerBusManager.test.ts#L69-L79: add a one-line QNBS-v3 comment explaining logger mocking for recovery assertions.tests/unit/workerBusManager.test.ts#L91-L91: add a one-line QNBS-v3 comment explaining mock reset isolation.tests/unit/workerBusManager.test.ts#L268-L272: add a one-line QNBS-v3 comment explaining regression coverage for the inference worker cap.As per coding guidelines, “Bei jeder inhaltlich relevanten Änderung in TypeScript- oder JavaScript-Dateien einen einzeiligen Kommentar im Format
// QNBS-v3: [Grund / Impact / Kreativer Mehrwert]unmittelbar bei der Änderung ergänzen.”🤖 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/workerBusManager.ts` around lines 87 - 90, Apply one-line comments in the format “// QNBS-v3: [Grund / Impact / Kreativer Mehrwert]” immediately beside each relevant change: in services/workerBusManager.ts lines 87-90, explain missing inference-pool re-registration; at lines 125-130, explain centralized inference-pool configuration; in tests/unit/workerBusManager.test.ts lines 69-79, explain logger mocking for recovery assertions; at line 91, explain mock-reset isolation; and at lines 268-272, explain regression coverage for the inference worker cap.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.
Outside diff comments:
In `@services/workerBusManager.ts`:
- Around line 87-90: Apply one-line comments in the format “// QNBS-v3: [Grund /
Impact / Kreativer Mehrwert]” immediately beside each relevant change: in
services/workerBusManager.ts lines 87-90, explain missing inference-pool
re-registration; at lines 125-130, explain centralized inference-pool
configuration; in tests/unit/workerBusManager.test.ts lines 69-79, explain
logger mocking for recovery assertions; at line 91, explain mock-reset
isolation; and at lines 268-272, explain regression coverage for the inference
worker cap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b0eace8-c875-471a-8942-b9ff6eeb63f9
📒 Files selected for processing (2)
services/workerBusManager.tstests/unit/workerBusManager.test.ts
…de-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>
Owner
Author
|
Re: outside-diff-range QNBS-v3 comment finding (5 locations) — addressed in b3e5d1a. Fixed (2): the logger mock and the Not changed, with reasoning (3): |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
qnbs
added a commit
that referenced
this pull request
Jul 30, 2026
…ilures (#289) * chore: pause Vercel auto-deploy mid-term after 2 undiagnosed build failures Two consecutive Vercel preview deployments failed (different deployment IDs, different commits) with only a generic "Deployment has failed" message. The exact same commits build cleanly via `pnpm run build` locally, and neither the GitHub Checks API nor `npx vercel inspect --logs` (requires an interactive account login not available here) surfaced any further detail — this looks platform-side, not a code regression. Vercel is not a required branch-protection check, so this was purely about stopping a noisy, undiagnosable failure status rather than unblocking anything. Disabled via vercel.json's documented `git.deploymentEnabled: false` (the standard, reversible, dashboard- free way to pause git-triggered deployments). GitHub Pages — already documented as the always-on fallback mirror — is promoted to primary in README.md/CLAUDE.md for the duration. TODO.md tracks re-enabling once someone with Vercel dashboard/CLI access diagnoses the cause. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * revert: re-enable Vercel auto-deploy — root cause found and fixed Root-caused the 2 Vercel build failures that prompted the mid-term pause: not a platform issue, but a real missing-export bug (services/ai/localEmbeddingService.ts imported ensureInferencePool() from workerBusManager.ts, which never actually defined it — invisible to Vitest because the consumer test mocks the whole module). The fix landed in the worker-generation-consolidation PR chain (PR #288). Reverts git.deploymentEnabled to the default (enabled) now that the actual cause is fixed and verified via a clean local `build:edge` reproduction of Vercel's exact build command. TODO.md's entry updated from "paused, cause unknown" to a resolved write-up, including the `pnpm run build` vs `pnpm run build:edge` command-mismatch lesson that delayed finding this. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: correct README.md/CLAUDE.md revert (was reverted to own branch HEAD, not main) The previous commit's `git checkout -- README.md CLAUDE.md` restored from this branch's own HEAD (which still had the "paused" wording from the first commit), not from main — so the pause note survived uncommitted. Re-applied via `git checkout main -- ...` this time. 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
Third of 5 PRs consolidating the v1/WorkerBus-v2 worker duplication (ADR-0014). Stacked on #287 (DuckDB migration), which is stacked on #286 (parity gate). Plan:
.claude/plans/worker-generation-v1-vs-enumerated-fox.md.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 dedicatednew Worker(inference.worker.ts)instance to the shared WorkerBus v2'inference'pool viaensureInferencePool()(same decoupling-from-enableWorkerBusV2pattern asensureWebLlmPool/ensureDuckDbPool).embedText's existing throw-on-failure contract, so no response-shape translation adapter was needed here (unlike DuckDB's), just a transport swap.WorkerPool's generic health check.'inference'pool'smaxWorkersto 2 (wasMAX_WORKERS_INFERENCE=4) inworkerBusManager.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.tsstill uses it directly; removal lands in the next PR of this sequence once that migration is done too.Test plan
pnpm exec vitest run tests/unit/localEmbeddingService.test.ts— 18/18 (rewritten against aworkerBusManagermock)localRagService.test.ts,localRagService.duckdb.test.ts,ragPromptAssembly.test.ts,crossProjectIndexService.test.ts,crossProjectIndexDuckDb.test.ts,ragVectorMigration.test.ts,useConsistencyCheckerView.test.tsx,inferenceGateway.test.ts,proForgeMemoryBank.test.ts,loraEvaluationService.test.ts,loraDatasetBuilder.test.ts— 161/161 passing, unaffected since the public API didn't changepnpm run typecheck(exact CI command) — cleanpnpm run lint(Biome) — clean via pre-commit hook🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Performance
Tests