fix: reindex search after import (fix #938)#971
Open
serhiizghama wants to merge 2 commits into
Open
Conversation
A replace import wipes KV but leaves the BM25/vector indices holding the deleted entries, and data imported by any strategy is written straight to KV without being indexed. Until the next process restart, smart-search returns ghosts of replaced-out records and misses everything just imported. Rebuild both indices from KV at the end of a successful import via the existing rebuildIndex helper, which clears and repopulates them to match KV exactly. A reindex failure is logged but does not fail the import, since the imported data is already durable in KV.
Asserts that a replaced-out memory no longer surfaces from search and an imported memory is searchable immediately, without a process restart.
|
@serhiizghama is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
📝 WalkthroughWalkthroughAfter a successful ChangesPost-import search reindex
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/functions/export-import.ts (1)
580-585: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove WHAT-style inline comments in this source block.
This block adds explanatory inline comments that describe behavior directly in code; the project rule asks to avoid that in
src/**/*.ts.♻️ Proposed cleanup
- // KV is now the source of truth, but the in-memory search indices still - // reflect the pre-import state: a "replace" wipe leaves ghosts of deleted - // entries in BM25/vector, and freshly imported entries from any strategy - // were written straight to KV without being indexed. Rebuild both indices - // from KV so smart-search reflects the imported data immediately instead - // of only after a process restart. try { const indexed = await rebuildIndex(kv); logger.info("Import reindex complete", { strategy, indexed }); } catch (err) { - // The import itself succeeded and is durable in KV; a reindex failure - // must not fail the import. Surface it so the gap is diagnosable. logger.warn("Import reindex failed", { strategy, error: err instanceof Error ? err.message : String(err), }); }As per coding guidelines,
In TypeScript source code, avoid code comments explaining WHAT — use clear naming instead.Also applies to: 590-592
🤖 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/functions/export-import.ts` around lines 580 - 585, Remove the WHAT-style inline comments in the block at lines 580-585 and 590-592 that explain the behavior of the search index rebuild logic. According to project guidelines, avoid code comments that describe WHAT the code does in src/**/*.ts files. Instead, refactor the code to be self-documenting through clear naming conventions and logical structure that makes the intent obvious without requiring explanatory comments. Consider whether the code sections being commented could benefit from better variable names, helper functions, or restructuring to eliminate the need for these inline comments altogether.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/functions/export-import.ts`:
- Around line 580-585: Remove the WHAT-style inline comments in the block at
lines 580-585 and 590-592 that explain the behavior of the search index rebuild
logic. According to project guidelines, avoid code comments that describe WHAT
the code does in src/**/*.ts files. Instead, refactor the code to be
self-documenting through clear naming conventions and logical structure that
makes the intent obvious without requiring explanatory comments. Consider
whether the code sections being commented could benefit from better variable
names, helper functions, or restructuring to eliminate the need for these inline
comments altogether.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: edeb6963-d7f5-4647-b655-6dfde917eba5
📒 Files selected for processing (2)
src/functions/export-import.tstest/export-import.test.ts
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.
Problem
After
mem::import,smart-searchis wrong until the process restarts (#938):replacestrategy wipes KV across every namespace insrc/functions/export-import.ts, but never touches the search indices — BM25 and the vector index keep holding the deleted entries, so search returns ghosts of records that no longer exist.kv.set(...)without being indexed, so freshly imported memories/observations are invisible to search.The net effect for a replace-import: search surfaces deleted records and misses everything just imported, for the whole process lifetime.
Solution
Rebuild both indices from KV at the end of a successful import, using the existing
rebuildIndex(kv)helper exported fromsearch.ts. It clears BM25 + vector and repopulates them from KV, which is exactly the correct post-import state and fixes both halves (ghosts removed, imported data indexed) for every strategy.A reindex failure is caught and logged rather than failing the import — the imported data is already durable in KV, so the import itself should still report success.
Testing
test/export-import.test.ts: after areplaceimport, a replaced-out memory no longer surfaces fromgetSearchIndex().search(...), and an imported memory is searchable immediately (no restart).rebuildIndexcall makes the new test fail.npm test).Summary by CodeRabbit
Bug Fixes
Tests