Fix #97: move blocking embed out of SQLite write transactions - #125
Fix #97: move blocking embed out of SQLite write transactions#125emp3thy wants to merge 2 commits into
Conversation
The five write paths listed in #97 all opened an implicit BEGIN (or a SAVEPOINT) and then called SyncEmbedder inside it. In WAL mode that holds the single writer lock across a multi-second Ollama call, so any other connection times out on its 5s busy_timeout with 'database is locked'. Fix: compute the embedding BEFORE the write lock is taken, then pass the vector into _store_embedding once inside the transaction. For apply_decision, which fans out N embeds across _apply_new / _apply_augment inside one SAVEPOINT, the two helpers now collect (reflection_id, source_text) into an optional embed_tasks list; the caller commits first, then does one batched embed and a short second write. Missing vectors already self-heal on retrieval via _heal_missing_embeddings, preserving the existing best-effort contract.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 Claude BugBot Analysis
Found 1 potential bug in this PR.
medium: 1
One real regression found: SemanticMemoryService.update_text now performs the blocking Ollama embed call before verifying the target id exists, turning a fast-fail on invalid ids into a multi-second stall; the rest of the WAL-lock reordering (reflection.py, semantic.py's create/create_from_observation, and the deferred batched embed in apply_decision) is correctly validate-before-embed and consistent with the codebase's existing self-healing embedding pattern.
| now = self._clock().isoformat() | ||
| # Compute the embedding BEFORE the UPDATE opens sqlite3's implicit | ||
| # write transaction — see #97. | ||
| vector = (self._sync_embedder.embed_text(content) |
There was a problem hiding this comment.
🟡 MEDIUM: update_text now blocks on Ollama embed before validating the id exists
update_text computes the embedding (a blocking Ollama call, up to SyncEmbedder's ~15s worker timeout) before running the UPDATE and before checking cur.rowcount == 0. Previously the embed call ran only after rowcount == 0 was confirmed false, so an update against a nonexistent id failed instantly with ValueError. Now every call — including ones for stale/deleted ids — pays the full blocking embed cost first. This path is reachable directly from user input: mcp/handlers/semantics.py:119 calls self._semantic.update_text(id=args["id"], content=args["content"]) with an MCP-tool-supplied id, and ui/app.py:634 calls it from the UI with a possibly-stale row id (e.g. a concurrently deleted memory). Compare with create_from_observation in the same file (lines ~170-193), which validates the source row with a SELECT and raises before computing the embedding — establishing the intended validate-then-embed order that update_text fails to follow because it validates via UPDATE rowcount, after the embed has already run. Fix: look up existence (e.g. SELECT 1 FROM semantic_memories WHERE id = ?) before computing the embedding, or compute the embedding only after confirming the row exists.
BugBot review on PR #125 caught a regression: after moving the embed call before the UPDATE (the #97 fix), a call with a stale/deleted id now pays the full ~15s Ollama worker timeout before discovering the row is gone. update_text is reachable from MCP (memory.update) and the UI drawer with an id chosen by the caller, so the fast-fail path matters. Restore fast-fail by doing a SELECT 1 before the embed. The rowcount check on UPDATE still handles the (rare) race where the row is deleted between the SELECT and the UPDATE.
There was a problem hiding this comment.
🟢 Claude BugBot Analysis
No new bugs found; the diff correctly reorders embedding computation to happen outside sqlite3's implicit write-transaction lock across reflection.py and semantic.py, and fixes the previously-reported issue.
No bugs were detected in this PR.
2 participants
Closes #97
Summary
All five write paths listed in #97 opened an implicit
BEGIN(or aSAVEPOINT) and then calledSyncEmbedderinside it. Under WAL thatholds the single writer lock across a multi-second Ollama call, so any
other connection times out on its 5s
busy_timeoutwithdatabase is locked.The fix computes the embedding before the write lock is taken, then
passes the resulting vector into
_store_embeddingonce inside thetransaction:
SemanticMemoryService.create/update_text/create_from_observationnow embed before their
INSERT/UPDATE/SAVEPOINT.ReflectionService.update_textembeds after the existence-and-statuscheck but before the
UPDATE.ReflectionSynthesisService.apply_decisionfans out N embeds across_apply_new/_apply_augmentinside oneSAVEPOINT, so those twohelpers now accept an optional
embed_taskslist to collect(reflection_id, source_text)pairs.apply_decisionreleases thesavepoint, commits, then runs one batched
embed_batchand a shortsecond write. Direct callers (existing tests) still use the inline
embed path — the parameter defaults to
None, so the change isbackward compatible.
Missing vectors already self-heal on the next retrieval via
_heal_missing_embeddings, so the pre-existing best-effort contract ispreserved. A new
TestApplyDecisionDefersEmbeddingOutsideWriteLockclass covers the deferred path end-to-end (new, augment, and
embedder-failure cases).
Confidence
~92%. The mechanical change is small (four files, +196/-32) and
localized to the write paths. Existing tests all pass (151 in the two
touched service modules, 1171 across the non-mcp suite; the pre-existing
41 mcp failures reproduce on
mainand are an MCP SDK version mismatchunrelated to this diff). Rationale: the fix mirrors the pattern the issue
itself recommends, and the atomicity delta between DB row and vector is
already the operating contract that
_heal_missing_embeddingswasbuilt to cover.
This PR was opened automatically by a scheduled Claude routine that
scans open issues in this repo and files fixes it can complete at ≥90%
confidence.
Generated by Claude Code