Skip to content

fix(store): make cleanupOrphanedVectors atomic across both vector tables - #766

Closed
fa1k3 wants to merge 1 commit into
tobi:mainfrom
fa1k3:fix/store-orphan-vector-cleanup-tx
Closed

fix(store): make cleanupOrphanedVectors atomic across both vector tables#766
fa1k3 wants to merge 1 commit into
tobi:mainfrom
fa1k3:fix/store-orphan-vector-cleanup-tx

Conversation

@fa1k3

@fa1k3 fa1k3 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Summary

cleanupOrphanedVectors() runs its two DELETEs (vectors_vec, then content_vectors) as separate implicit transactions. An interruption between them — process crash, or SQLITE_BUSY from a concurrent writer hitting the second statement — desyncs the two tables: the embedding rows are already gone from vectors_vec while content_vectors still reports the chunks as embedded.

Since these rows are orphaned (no active document) and vector search post-filters on documents.active = 1, live search is unaffected right away. The failure is latent: qmd is content-addressable, so if the same content later returns and that hash is reactivated, the stale content_vectors rows make getHashesNeedingEmbedding treat it as already embedded — qmd embed skips it, and the document is silently unsearchable by vector with no orphan left for a future cleanup to catch.

What changed

  • The orphan count and both DELETEs now run inside a single db.transaction(), executed as BEGIN IMMEDIATE: the count reads before the DELETEs write, and upgrading a deferred read snapshot under a concurrent WAL writer fails with SQLITE_BUSY_SNAPSHOT instead of honoring the busy timeout. The shared Database interface type now reflects the .immediate variant both drivers already expose.
  • The count shares the transaction so the returned number always matches what the DELETEs actually removed, even when another connection mutates documents concurrently.
  • Nested callers stay safe: both better-sqlite3 and bun:sqlite implement nested transaction functions via savepoints (an immediate transaction invoked inside an outer transaction becomes a savepoint).
  • Changelog entry under [Unreleased].

Verification

  • New tests in test/store.test.ts (cleanupOrphanedVectors atomicity):
    • happy path: orphaned chunks removed from both tables, survivors intact, count correct
    • fault injection between the two DELETEs: the vectors_vec DELETE rolls back (this test fails against the previous code) and the connection stays usable for a plain retry
    • outer-transaction rollback also restores the cleanup's deletions
    • inner-savepoint isolation: a cleanup failure caught inside an outer transaction rolls back alone while the outer transaction's own write commits
  • bun run test:types clean, bun run build passes
  • Suites green under both runtimes (vitest via Node and bun test)

cleanupOrphanedVectors ran its two DELETEs (vectors_vec, then
content_vectors) as separate implicit transactions. An interruption
between them (process crash, SQLITE_BUSY under a concurrent writer)
desyncs the two tables: the embedding rows are gone from vectors_vec
while content_vectors still reports the chunks as embedded, so the
index silently loses vector-search coverage with no way to notice or
repair short of re-embedding.

Wrap the orphan count and both DELETEs in a single db.transaction().
The count shares the transaction so the returned number always matches
what the DELETEs actually removed, even when another connection
mutates documents concurrently. Nested callers are safe: both
better-sqlite3 and bun:sqlite implement nested transaction functions
via savepoints.

Tests: happy path, fault injection between the DELETEs proving the
first one rolls back and the connection stays usable, and savepoint
participation in an outer transaction.
@PowderAddicts

PowderAddicts commented Jul 13, 2026 via email

Copy link
Copy Markdown

@tobi

tobi commented Aug 13, 2026

Copy link
Copy Markdown
Owner

Superseded by #856 — same atomic cleanup, rebased onto current main after #851 extracted countOrphanedVectors (CHANGELOG + that refactor were the conflicts here).

@PowderAddicts

PowderAddicts commented Aug 13, 2026 via email

Copy link
Copy Markdown

tobi added a commit that referenced this pull request Aug 13, 2026
…les (#766) (#856)

An interruption between the two DELETEs could leave content_vectors claiming
chunks were embedded after vectors_vec had already dropped them, so a later
reactivation skipped re-embed. Count and both deletes now share BEGIN IMMEDIATE.
@tobi

tobi commented Aug 13, 2026

Copy link
Copy Markdown
Owner

Closed in favor of #856, which lands this fix on current main.

@tobi tobi closed this Aug 13, 2026
@PowderAddicts

PowderAddicts commented Aug 13, 2026 via email

Copy link
Copy Markdown

@PowderAddicts

PowderAddicts commented Aug 13, 2026 via email

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants