fix(search): run embedding pass in background so server starts immediately#218
Merged
Conversation
…ately
rebuildFromVault now returns after FTS indexing (Passes 1+2) and fires
the embedding pass (Pass 3) in the background. The server accepts
requests immediately — search works with FTS-only until vectors are
ready. Previously the server blocked for ~5 minutes on first startup
while embedding every note.
Return type changes from number to { count, embedding } so callers
can optionally await the embedding promise (tests do, server doesn't).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JPDgHMKBvJ51V2YrrMYZET
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…bed chain error propagation
EMBEDDING_ENABLED was documented in .env.example files but never passed
through any docker-compose environment section — a user setting it to
false would have it silently ignored. Added to all 5 compose files plus
synced CLI templates.
The file-watcher's promise chain for per-path embed serialization used
.then() directly, which propagates a rejected previous embed and skips
the current one. Added .catch(() => {}) before .then() so a transient
failure can't cascade.
Also added EMBEDDING_ENABLED to the README Configuration table.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…fication comments Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughRefactors ChangesEmbedding Pipeline Resilience and rebuildFromVault Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Stop wiping note_chunks and note_vectors on rebuild. Content-hash gating in embedAndStoreChunks already skips unchanged chunks, so only new/modified notes re-embed. Deleted notes are cleaned up by diffing note_chunks paths against the current vault file list. Reduces startup embedding time from ~26 min (full re-embed on Lightsail) to near-zero when vault content hasn't changed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JPDgHMKBvJ51V2YrrMYZET
…dary - embedder: pipeline load failure + retry (error path, pipelineLoading reset) - chunker: exact 500-token threshold boundary (confirms < not <=) - search-index: embedNote error propagation, rebuild resilience on per-note failure Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The embedding pipeline paragraph claimed "Full rebuilds re-embed everything (the rebuild clears vector tables first)" — both halves are wrong. Vector tables persist across rebuilds (content-hash gating applies to full rebuilds too, not just incremental updates), and only new/modified chunks re-embed. Updated to match the actual code path in rebuildFromVault Pass 3. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…memory retention - rebuildFromVault now follows the (params, logger) two-arg convention - Add MEMORY_ENABLED to all 6 docker-compose files (pre-existing gap) - Extract notesForEmbedding before background pass so the full noteContents array can be garbage-collected during embedding - Remove unused module-level logger import (all callers pass logger) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JPDgHMKBvJ51V2YrrMYZET
Owner
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![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 (1)
src/vault-mcp/search/file-watcher.ts (1)
65-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer an async wrapper over the
.catch().then()chain.This preserves the sequencing, but an async wrapper is easier to reason about and matches the repo’s preferred style.
♻️ Proposed refactor
const previousEmbed = pendingEmbeds.get(relativePath) ?? Promise.resolve() - const currentEmbed = previousEmbed - .catch(() => {}) - .then(() => - search.embedNote( - { notePath: relativePath, rawContent: content }, - logger, - ), - ) + const currentEmbed = (async (): Promise<void> => { + try { + await previousEmbed + } catch { + // Let a later change retry after a transient embed failure. + } + await search.embedNote( + { notePath: relativePath, rawContent: content }, + logger, + ) + })()As per coding guidelines, "Prefer async/await over
.then()/.catch()for promise handling."🤖 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 `@src/vault-mcp/search/file-watcher.ts` around lines 65 - 73, Refactor the promise chaining in file-watcher’s embedding flow to use an async wrapper instead of the current `.catch().then()` sequence. Update the logic around `pendingEmbeds`, `previousEmbed`, and `currentEmbed` in the file-watcher handler so it preserves the same sequencing while using `async/await` for readability and consistency with the repo style.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.
Inline comments:
In `@ARCHITECTURE.md`:
- Line 266: Update the embedding pipeline documentation to explicitly state that
Pass 3 runs asynchronously in the background after `rebuildFromVault` completes
Passes 1+2. In the `ARCHITECTURE.md` embedding section, clarify that startup
returns before embeddings finish, and that `createEmbedder(logger)` continues
processing chunks and vector updates after the rebuild contract completes.
Reference `rebuildFromVault`, Pass 3, and `createEmbedder(logger)` so the
background behavior is unmistakable.
In `@docker-compose.yml`:
- Around line 61-62: The Compose environment entries for EMBEDDING_ENABLED and
MEMORY_ENABLED are using empty-string expansion, which can override the boolean
defaults expected by config.ts. Update the Docker Compose manifest(s) so these
variables resolve to explicit boolean defaults instead of an empty string,
matching the .default("true").asBool() parsing used in the configuration loader.
Apply the same fix to the other Compose manifests that use this pattern, and
keep the env var names aligned with the config symbols EMBEDDING_ENABLED and
MEMORY_ENABLED.
In `@src/vault-mcp/search/__tests__/chunker.test.ts`:
- Around line 31-39: The threshold test in chunker.test.ts is too loose because
chunkNoteContent is only checked with toBeGreaterThan(1), so it won’t fail if
the splitter behavior regresses. Update the exact-threshold case in the
chunkNoteContent spec to assert the deterministic full chunk output for the
500-token fixture, using the existing generateTokens helper and the
chunkNoteContent function, so the test verifies the precise number and shape of
chunks instead of just “more than one.”
In `@src/vault-mcp/search/search-index.ts`:
- Around line 920-988: Pass 3 is writing stale embedding data after startup and
can overwrite newer watcher-driven state. Update the background embedding flow
in search-index.ts so each note is validated before committing writes, or route
the pass through the same per-path serialization used by live embed/remove
operations. Use the existing symbols notesForEmbedding, embedAndStoreChunks, and
the startup embeddingPromise loop to re-check that the note still exists and its
content matches before storing chunks/vectors, and skip or retry any path that
is no longer fresh.
---
Nitpick comments:
In `@src/vault-mcp/search/file-watcher.ts`:
- Around line 65-73: Refactor the promise chaining in file-watcher’s embedding
flow to use an async wrapper instead of the current `.catch().then()` sequence.
Update the logic around `pendingEmbeds`, `previousEmbed`, and `currentEmbed` in
the file-watcher handler so it preserves the same sequencing while using
`async/await` for readability and consistency with the repo style.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 71942494-f85c-46da-8942-3b662d39082d
📒 Files selected for processing (16)
ARCHITECTURE.mdREADME.mdcli/templates/local/docker-compose.ymlcli/templates/remote/docker-compose.ymldeploy/local/docker-compose.ymldeploy/remote/docker-compose.ymldocker-compose.local.ymldocker-compose.ymlsrc/vault-mcp/search/__tests__/chunker.test.tssrc/vault-mcp/search/__tests__/embedder.test.tssrc/vault-mcp/search/__tests__/search-index.test.tssrc/vault-mcp/search/chunker.tssrc/vault-mcp/search/embedder.tssrc/vault-mcp/search/file-watcher.tssrc/vault-mcp/search/search-index.tssrc/vault-mcp/server.ts
… extraction - docker-compose: use :-true defaults (empty string crashes env-var) - file-watcher: log previous embed failure instead of silent catch - search-index: extract multi-clause if to named hasDeletedNotes boolean - search-index: add let-justification comment for embedding counters - ARCHITECTURE.md: document that Pass 3 runs in the background - AGENTS.md: add conditional-extraction convention Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JPDgHMKBvJ51V2YrrMYZET
…for embedding pipeline Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ssertion Coverage gap: toFloat32Array error path was untested — pipeline returning non-Float32Array data now has a regression test (mutation-verified). Assertion quality: embedBatch slice test collapsed from two index-based assertions into a single toEqual on the full array. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Pass 3 (background embedding) consumes a startup snapshot while the file watcher runs concurrently. Without a guard, Pass 3 could overwrite fresh embeddings from the watcher with stale snapshot content, or recreate vectors for notes deleted after the snapshot. Check the notes table mtime before each embed — skip when the note was removed or updated by the watcher since the snapshot was taken. Also fixes the file-watcher error message from "failed to index file" to "failed to process file change" — when embedding fails, FTS indexing has already succeeded, so the old message was misleading. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The pipeline type is a 24-member union that vi.fn() can't satisfy. Cast through unknown with a comment explaining why — test-only mock, production code has the runtime toFloat32Array guard. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JPDgHMKBvJ51V2YrrMYZET
The as cast on selectNoteMtimeStmt.get() follows the pre-existing pattern used by all 9 other .get() calls in this file — better-sqlite3 returns unknown and the codebase casts to the expected row shape. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JPDgHMKBvJ51V2YrrMYZET
Owner
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/vault-mcp/search/__tests__/embedder.test.ts (1)
73-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the double cast from the mocked pipeline.
as unknown as Awaited<ReturnType<typeof mockedPipeline>>sidesteps the strict typing this test file is supposed to preserve and can hide a broken mock shape. Please replace it with a typed test helper/factory instead of forcing the value throughunknown. As per coding guidelines,src/**/*.{ts,tsx}should "avoidascasts".🤖 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 `@src/vault-mcp/search/__tests__/embedder.test.ts` around lines 73 - 76, The mocked pipeline setup in the embedder test is bypassing type safety by using a double cast through unknown, which should be removed. Update the test around mockedPipeline and badPipeline to use a properly typed helper/factory that returns the expected mocked pipeline shape instead of forcing it with as unknown as Awaited<ReturnType<typeof mockedPipeline>>. Keep the test strictly typed so the mock matches the real pipeline contract without any casts.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 `@src/vault-mcp/search/__tests__/embedder.test.ts`:
- Around line 73-76: The mocked pipeline setup in the embedder test is bypassing
type safety by using a double cast through unknown, which should be removed.
Update the test around mockedPipeline and badPipeline to use a properly typed
helper/factory that returns the expected mocked pipeline shape instead of
forcing it with as unknown as Awaited<ReturnType<typeof mockedPipeline>>. Keep
the test strictly typed so the mock matches the real pipeline contract without
any casts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0fad7cf3-dbe8-4906-9bf0-181f19c669ad
📒 Files selected for processing (14)
.devin/wiki.jsonAGENTS.mdARCHITECTURE.mdcli/templates/local/docker-compose.ymlcli/templates/remote/docker-compose.ymldeploy/local/docker-compose.ymldeploy/remote/docker-compose.ymldocker-compose.local.ymldocker-compose.ymlserver.jsonsrc/vault-mcp/config.tssrc/vault-mcp/search/__tests__/embedder.test.tssrc/vault-mcp/search/file-watcher.tssrc/vault-mcp/search/search-index.ts
✅ Files skipped from review due to trivial changes (5)
- AGENTS.md
- src/vault-mcp/config.ts
- docker-compose.local.yml
- ARCHITECTURE.md
- .devin/wiki.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/vault-mcp/search/file-watcher.ts
- src/vault-mcp/search/search-index.ts
aliasunder
added a commit
that referenced
this pull request
Jun 29, 2026
…ately (#218) - **Server blocked for ~5 min on first startup** while embedding every note — FTS was ready but the server didn't accept requests until Pass 3 finished - `rebuildFromVault` now returns after FTS indexing (Passes 1+2) and fires embedding (Pass 3) in the background - Return type changes from `number` to `{ count, embedding }` — server ignores the promise, tests can await it - [x] 1131 tests pass - [x] TypeScript compiles clean - [x] Embedding test awaits the background promise to verify embeddings complete - [ ] Deploy and verify server responds to health check within seconds of startup (not minutes) 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01JPDgHMKBvJ51V2YrrMYZET <!-- This is an auto-generated comment: release notes by coderabbit.ai --> * **New Features** * Added `EMBEDDING_ENABLED` feature flag (default: `true`) to disable the embedding pipeline and run search using FTS5 only. * Added `MEMORY_ENABLED` feature flag (default: `true`) to the local/remote compose setups. * **Bug Fixes** * Improved incremental indexing so unchanged content isn’t re-embedded and rebuilds preserve existing vector data. * Fixed missed re-embeds when an earlier embedding attempt fails, ensuring subsequent updates still proceed. * **Documentation / Tests** * Updated indexing documentation and environment-variable references; expanded search/indexing test coverage. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
aliasunder
added a commit
that referenced
this pull request
Jun 30, 2026
…ately (#218) - **Server blocked for ~5 min on first startup** while embedding every note — FTS was ready but the server didn't accept requests until Pass 3 finished - `rebuildFromVault` now returns after FTS indexing (Passes 1+2) and fires embedding (Pass 3) in the background - Return type changes from `number` to `{ count, embedding }` — server ignores the promise, tests can await it - [x] 1131 tests pass - [x] TypeScript compiles clean - [x] Embedding test awaits the background promise to verify embeddings complete - [ ] Deploy and verify server responds to health check within seconds of startup (not minutes) 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01JPDgHMKBvJ51V2YrrMYZET <!-- This is an auto-generated comment: release notes by coderabbit.ai --> * **New Features** * Added `EMBEDDING_ENABLED` feature flag (default: `true`) to disable the embedding pipeline and run search using FTS5 only. * Added `MEMORY_ENABLED` feature flag (default: `true`) to the local/remote compose setups. * **Bug Fixes** * Improved incremental indexing so unchanged content isn’t re-embedded and rebuilds preserve existing vector data. * Fixed missed re-embeds when an earlier embedding attempt fails, ensuring subsequent updates still proceed. * **Documentation / Tests** * Updated indexing documentation and environment-variable references; expanded search/indexing test coverage. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
5 tasks
4 tasks
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
rebuildFromVaultnow returns after FTS indexing (Passes 1+2) and fires embedding (Pass 3) in the backgroundnumberto{ count, embedding }— server ignores the promise, tests can await itTest plan
🤖 Generated with Claude Code
https://claude.ai/code/session_01JPDgHMKBvJ51V2YrrMYZET
Summary by CodeRabbit
EMBEDDING_ENABLEDfeature flag (default:true) to disable the embedding pipeline and run search using FTS5 only.MEMORY_ENABLEDfeature flag (default:true) to the local/remote compose setups.