fix(sqlite): transactional integrity, FTS5 for memories, decay rotation - #63
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d5b04b11b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const vector = await embeddingService.embedWithTimeout(memory.content); | ||
| const scope = memory.containerTag.includes("_user_") ? "user" : "project"; | ||
| const hash = memory.containerTag.split("_").slice(2).join("_"); | ||
| const newShard = shardManager.getWriteShard(scope, hash); | ||
| const newDb = connectionManager.getConnection(newShard.dbPath); | ||
|
|
||
| await vectorSearch.insertVector( | ||
| newDb, | ||
| { | ||
| id: memory.id, | ||
| content: memory.content, | ||
| vector, | ||
| containerTag: memory.containerTag, | ||
| type: memory.type || undefined, | ||
| createdAt: memory.createdAt, | ||
| updatedAt: memory.updatedAt, | ||
| metadata: memory.metadata || undefined, | ||
| displayName: memory.displayName || undefined, | ||
| userName: memory.userName || undefined, | ||
| userEmail: memory.userEmail || undefined, | ||
| projectPath: memory.projectPath || undefined, | ||
| projectName: memory.projectName || undefined, | ||
| gitRepoUrl: memory.gitRepoUrl || undefined, | ||
| }, | ||
| newShard | ||
| ); | ||
|
|
||
| if (memory.isPinned === 1) { | ||
| vectorSearch.pinMemory(newDb, memory.id); | ||
| } | ||
|
|
||
| shardManager.incrementVectorCount(newShard.id); | ||
| await vectorSearch.updateVector(db, memory.id, vector); |
There was a problem hiding this comment.
For a live migration to a different model with the same embedding dimensions, this call omits shard, so updateVector updates only SQLite and never marks the shard rebuild-dirty or updates the native index. If the default USearch index was initialized by an earlier search, maybeRebuild skips it and USearchBackend.rebuildFromShard also preserves initialized indexes, so subsequent searches use the old embeddings even though the shard metadata reports the new model.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #66 (1ec9af5): the migration now resolves full ShardInfo per dbPath and passes it to updateVector, re-embeds the tags text with the capture-time prompt shape, and marks the shard dirty so the next search force-replaces any initialized index with new-dimension data. Regression-asserted via a markShardDirty spy in tests/reembed-honesty.test.ts.
| CREATE TRIGGER IF NOT EXISTS memories_fts_update | ||
| AFTER UPDATE ON memories BEGIN |
There was a problem hiding this comment.
Because this trigger is AFTER UPDATE ON memories, every unrelated update rewrites the FTS entry: normal searches update access_count/last_accessed for each result, and decay and scoring jobs update every memory. That turns routine metadata updates into full delete-and-reinsert tokenization work, increasing WAL growth and write contention; scope the trigger to UPDATE OF content, tags (and id if it can change).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #66 (7b2835b): the trigger is now AFTER UPDATE OF content, tags (DROP+CREATE in ensureMemoriesFts upgrades databases created with the unscoped trigger); tests/memories-fts.test.ts asserts metadata updates no longer rewrite FTS while content updates stay synced.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 3 potential issues.
3 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| if (shard && backend) { | ||
| try { | ||
| await backend.insert({ id: record.id, vector: record.vector, shard, kind: "content" }); | ||
| if (record.tagsVector) { | ||
| await backend.insert({ id: record.id, vector: record.tagsVector, shard, kind: "tags" }); | ||
| } | ||
| } finally { | ||
| this.rebuildDirty.set(`${shard.id}:content`, true); | ||
| this.rebuildDirty.set(`${shard.id}:tags`, true); |
There was a problem hiding this comment.
🔴 Committed writes report failure
When backend indexing fails after COMMIT, insertVector, updateVector, and replaceVector reject despite persisting the SQLite change. Callers then skip count updates or report failure, so retries can create duplicate memories. rebuildDirty cannot repair initialized USearch indexes because rebuildFromShard returns immediately.
Prompt for agents
The SQLite mutation is now committed before backend indexing in VectorSearch.insertVector, updateVector, and replaceVector. A backend exception still rejects the public operation after the durable write. LocalMemoryClient then omits incrementVectorCount and returns failure, while update handlers report failure although the update landed. The dirty flag is not a reliable recovery mechanism for USearch because USearchBackend.rebuildFromShard exits when an index is already initialized. Make SQLite the source of truth after commit: define successful API behavior and metadata count updates accordingly, and provide a repair path that truly rebuilds or invalidates initialized backend indexes after partial indexing failures. Cover insert, update, and replace failures with regression tests.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in #66 (cc41f56 + 1ec9af5): post-COMMIT backend failures are now warn + mark-dirty (sqlite is the durable truth, so committed writes no longer report failure), and rebuildFromShard gained a force path so a dirty shard actually replaces an initialized index instead of early-returning. tests/backend-repair.test.ts covers both.
| } | ||
|
|
||
| shardManager.incrementVectorCount(newShard.id); | ||
| await vectorSearch.updateVector(db, memory.id, vector); |
There was a problem hiding this comment.
🔴 Re-embedding leaves live searches stale
After an index serves a search, re-embedding calls updateVector without the shard and only changes SQLite. The loaded backend keeps every old vector because this path neither updates it nor marks rebuildDirty. Searches use old embeddings until restart.
Prompt for agents
MigrationService._reEmbedSingleMemory calls VectorSearch.updateVector without ShardInfo, so the backend update branch is skipped. A USearch index already initialized in this process continues serving old-model vectors after the migration reports success. Pass enough shard context through reEmbedMigration to update or invalidate the content index, and ensure invalidation forces a real rebuild even when USearchBackend considers its cache initialized. Add a regression test that initializes an index before migration and searches immediately afterward without restarting.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Same as above — fixed in #66 (1ec9af5): shard context now flows through the re-embed and the shard is marked dirty on success, forcing an index rebuild from sqlite on the next search.
| } | ||
|
|
||
| shardManager.incrementVectorCount(newShard.id); | ||
| await vectorSearch.updateVector(db, memory.id, vector); |
There was a problem hiding this comment.
🟡 Re-embedding deletes tag embeddings
Each migrated memory calls updateVector without tagsVector, which writes NULL into tags_vector. Semantic tag relevance disappears for migrated memories and is never regenerated.
Prompt for agents
The re-embed migration only embeds memory.content, then VectorSearch.updateVector writes both vector and tags_vector. Because tagsVector is omitted, tags_vector becomes NULL for every migrated memory. Preserve semantic tag search by loading each memory's tags and generating a new tags embedding with the configured model, or change the update API so content-only updates do not erase tag vectors while arranging a compatible tag-vector migration. Add a regression test with a populated tags_vector.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in #66 (54f4bc3 + 1ec9af5): updateVector now uses tags_vector = COALESCE(?, tags_vector) so content-only updates preserve stored tag embeddings, and the migration re-embeds the tags text (same Topics: … prompt shape as capture) so new-dimension indexes aren't poisoned by old-dims blobs.
There was a problem hiding this comment.
🟡 Changes recommended
Re-embed migration updates sqlite vectors without updating/invalidating the vector backend index, which can leave similarity search operating on stale vectors after a “successful” migration.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Improves SQLite-backed storage correctness and lifecycle behavior by tightening transaction boundaries, adding/repairing missing memories_fts (FTS5) support for keyword search, and preventing decay/archival starvation and connection-pool hazards. This fits into the repo’s shard-based persistence + vector-index search pipeline by making sqlite the reliable source of truth and ensuring supporting indexes stay consistent.
Changes:
- Add/ensure
memories_fts(FTS5 external-content + triggers) on shard open, with one-time backfill for existing shards. - Move vector-backend updates out of sqlite write transactions; make decay/archival ordering deterministic and commit before async vector-index deletes.
- Harden connection/batch write behavior (flush after eviction, eviction avoids in-txn connections) and make exact-dedup deletions transactional, plus add regression tests.
File summaries
| File | Description |
|---|---|
| tests/wal-batch.test.ts | Adds regression test ensuring queued batch writes survive connection eviction. |
| tests/vector-insert-txn.test.ts | Regression test preventing sqlite txn spanning an await during vector insert. |
| tests/reembed-honesty.test.ts | Validates re-embed migration updates vectors in place and reports success/failure correctly. |
| tests/memories-fts.test.ts | Verifies memories_fts creation/backfill and that keyword search uses MATCH rather than LIKE. |
| tests/exact-dedup-txn.test.ts | Ensures exact-dup deletion is transactional (no partial deletes on error). |
| tests/deduplication-service.test.ts | Updates mocks/expectations to reflect transactional exact-dedup behavior. |
| tests/decay-batch-starvation.test.ts | Regression test for decay batch rotation past decayBatchSize. |
| tests/connection-eviction-txn.test.ts | Ensures pool eviction does not close connections with open transactions. |
| tests/archive-ordering.test.ts | Verifies archive commits before async vector-index deletes. |
| src/services/sqlite/vector-search.ts | Moves backend index updates out of sqlite txns; marks shard indexes dirty for rebuild. |
| src/services/sqlite/shard-manager.ts | Ensures last_decay_at index + calls ensureMemoriesFts for new shards. |
| src/services/sqlite/schema.ts | Adds ensureMemoriesFts to create FTS5 table, triggers, and backfill. |
| src/services/sqlite/connection-manager.ts | Tracks in-txn connections for eviction safety; reopens connection on flushBatch after eviction; ensures FTS/index on open. |
| src/services/migration-service.ts | Makes migration success reporting honest; re-embed updates vectors in place. |
| src/services/memory-lifecycle.ts | Orders decay batches; archives in sqlite txn then performs vector deletes after COMMIT. |
| src/services/deduplication-service.ts | Makes exact-dedup deletes transactional and removes logging of memory content snippets. |
| docs/CHANGELOG.md | Documents the correctness/storage fixes under Unreleased. |
| docs/ARCHITECTURE.md | Updates architecture doc to describe memories_fts and how it’s kept in sync/backfilled. |
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| try { | ||
| const vector = await embeddingService.embedWithTimeout(memory.content); | ||
| const scope = memory.containerTag.includes("_user_") ? "user" : "project"; | ||
| const hash = memory.containerTag.split("_").slice(2).join("_"); | ||
| const newShard = shardManager.getWriteShard(scope, hash); | ||
| const newDb = connectionManager.getConnection(newShard.dbPath); | ||
|
|
||
| await vectorSearch.insertVector( | ||
| newDb, | ||
| { | ||
| id: memory.id, | ||
| content: memory.content, | ||
| vector, | ||
| containerTag: memory.containerTag, | ||
| type: memory.type || undefined, | ||
| createdAt: memory.createdAt, | ||
| updatedAt: memory.updatedAt, | ||
| metadata: memory.metadata || undefined, | ||
| displayName: memory.displayName || undefined, | ||
| userName: memory.userName || undefined, | ||
| userEmail: memory.userEmail || undefined, | ||
| projectPath: memory.projectPath || undefined, | ||
| projectName: memory.projectName || undefined, | ||
| gitRepoUrl: memory.gitRepoUrl || undefined, | ||
| }, | ||
| newShard | ||
| ); | ||
|
|
||
| if (memory.isPinned === 1) { | ||
| vectorSearch.pinMemory(newDb, memory.id); | ||
| } | ||
|
|
||
| shardManager.incrementVectorCount(newShard.id); | ||
| await vectorSearch.updateVector(db, memory.id, vector); | ||
| const nextCount = processedCount + 1; |
There was a problem hiding this comment.
Duplicate of the Codex/Devin finding — fixed in #66 (1ec9af5).
| if (this.connections.size >= MAX_CONNECTIONS) { | ||
| const oldestPath = this.accessOrder.shift(); | ||
| if (oldestPath) { | ||
| this.closeConnection(oldestPath); | ||
| log("ConnectionManager: evicted oldest connection", { path: oldestPath }); | ||
| const idlePath = this.accessOrder.find((p) => !this.inTxn.has(p)); | ||
| if (idlePath) { | ||
| this.closeConnection(idlePath); | ||
| log("ConnectionManager: evicted oldest idle connection", { path: idlePath }); | ||
| } else { | ||
| log("ConnectionManager: skipped eviction, all connections in transaction"); | ||
| } |
There was a problem hiding this comment.
Verified as real but deliberately not fixed (see #66): the overshoot is transient and self-healing — LRU eviction on the next getConnection shrinks it and transactions are millisecond-scale — so a slot-wait queue wasn't worth the machinery. Documented in the PR body as a revisit-if-txn-duration-grows decision.
| CREATE TRIGGER IF NOT EXISTS memories_fts_update | ||
| AFTER UPDATE ON memories BEGIN | ||
| INSERT INTO memories_fts(memories_fts, rowid, id, content, tags) |
There was a problem hiding this comment.
Duplicate of the Codex P2 — fixed in #66 (7b2835b).
Decay SELECT had LIMIT without ORDER BY, so the same first N rows were reprocessed every cycle and rows past the cap never decayed.
Shards were created without the FTS5 table, so keyword search silently fell back to a full-table LIKE scan. Create memories_fts on new and existing shards with triggers and a one-time backfill.
SQLite write transactions were held open across await of the vector backend, so concurrent captures/decay could nested-BEGIN. Commit sqlite first, then update the index.
LRU eviction could checkpoint-close a connection with an open write transaction. Skip in-txn handles and evict an idle connection instead.
…never-opened db flushBatch deleted the queued batch when the connection was already gone, so those writes vanished. Reopen the connection at flush time instead.
Archival ran async vector deletes inside an open sqlite transaction and ignored collected ids. Copy/delete in sqlite first, commit, then delete from the vector index.
Exact-duplicate deletes ran one-by-one without a transaction, so a crash left a partial purge. Batch them in one txn and stop logging memory content on ingest-dedup.
Re-embed used insert on the same shard PK and always reported success. Update vectors in place and return success only when every shard actually succeeded.
4d5b04b to
86585c8
Compare
|
Devin finding on #63: updateVector always wrote tags_vector, so a content-only re-embed (migration, admin re-embed) passed undefined and null'd the tag embeddings — semantic tag search silently disappeared for every updated memory. COALESCE preserves the stored blob when the caller provides none.
Copilot/Codex finding on #63: AFTER UPDATE ON memories (unscoped) fired on every UPDATE — every access_count bump on a search hit and every decay/score touch — re-tokenizing the full content via FTS delete+reinsert. Scoped to UPDATE OF content, tags; DROP+CREATE in ensureMemoriesFts upgrades databases created with the old trigger.
…hards Devin finding on #63: after COMMIT, a failing backend insert rejected the whole write although the sqlite row was durable — callers reported failure and retried into duplicates. Worse, the rebuild-dirty flag could never repair an initialized index: rebuildFromShard returned early for initialized indexes, so a dirty shard stayed missing entries until process restart. - rebuildFromShard gains force: replaces an initialized index by rebuilding from sqlite (the durable truth). - maybeRebuild distinguishes bootstrap (first use) from repair (dirty) and passes force only for the latter — steady-state searches never pay a rebuild. - insertVector/updateVector/replaceVector swallow post-commit backend failures with a warn + dirty mark; pre-commit sqlite failures still throw (nothing persisted). - markShardDirty() public — the re-embed migration uses it (next commit).
Copilot/Codex P1 + Devin findings on #63: _reEmbedSingleMemory called updateVector without the shard, so the backend-update branch was skipped entirely — a 'successful' migration left any initialized in-memory index serving old embeddings until restart, and left tags_vector NULL (and after a dimension change, old-dims tag blobs would poison a rebuilt tags index). - resolve full ShardInfo per migrated dbPath and pass it through - re-embed the tags text with the exact capture-time prompt shape (Topics: joined tags) when the row stores tags - after a fully successful shard, markShardDirty() — the next search force-replaces initialized indexes with new-dimension data
The sonarcloud job uploaded the analysis but never waited for the SonarCloud quality gate, and its check was not required on main — PR #63 merged with a red gate (66.9% new-code coverage vs 80% target) because 'check' was the only required context. - sonar.qualitygate.wait=true makes the job itself fail on a red gate - the sonarcloud context is now required by branch protection - dependabot PRs skip this job; a skipped required check still passes
Summary
Full codebase audit batch 2/3 (storage correctness). Every fix ships with a regression test that fails without it. Gates green:
format:check+typecheck+ 831 tests passed (baseline 821) +build.fix(lifecycle)ORDER BY last_decay_at, id+ index). A shard overdecayBatchSizere-processed the same first rows every cycle — rows past the cap never decayed or archivedfix(sqlite)memories_ftsis now created — it was queried since inception but never existed, so every keyword search silently fell to aLIKE %q%full scan (the tag/FTS boost never applied). External-content FTS5 + triggers, plus one-time backfill for existing shardsfix(sqlite)insertVector/updateVectorno longer holdBEGIN IMMEDIATEacrossawait backend.insert()— nested-transaction/SQLITE_BUSY hazard removed (sqlite work is sync inside the txn; index insert happens after COMMIT and marks the shard rebuild-dirty on failure, so the next search self-heals)fix(sqlite)fix(sqlite)flushBatchreopens the connection instead of deleting the batch and throwingfix(lifecycle)fix(dedup)fix(migration)success: true); success now means every shard succeededVerification
bun run format:check && bun run typecheck && bun run test && bun run build— all green (831 passed / 1 skipped, 7 new test files)Notes
IF NOT EXISTSand apply on shard open.