Skip to content

fix(search): never load the embedding model inside a read snapshot - #24

Merged
RMANOV merged 2 commits into
mainfrom
fix/search-model-load-outside-snapshot-20261014
Oct 14, 2026
Merged

RMANOV merged 2 commits into
mainfrom
fix/search-model-load-outside-snapshot-20261014

Conversation

@RMANOV

@RMANOV RMANOV commented Oct 14, 2026

Copy link
Copy Markdown
Owner

Problem

vector_search and task_vector_search embed the query inside the caller's SQLite read transaction (get_conn() opens an explicit BEGIN). The first call in a server process also loads the sentence-transformers model.

Observed on 14 Oct 2026: one search_nodes call waited 4 h 14 min for that first load while holding its WAL snapshot. The reader held read slot 0, so no checkpoint could start (nBackfill = 0, nBackfillAttempted = 0), the WAL grew to 76 MB, and the MCP client aborted the call after 30 minutes while the server kept running it. The snapshot was released only when the load finished.

Fix

  • New vec_search.model_ready(): returns True once the model is loaded; otherwise starts one background warm-up thread (it holds no connection) and returns False.
  • vector_search and task_vector_search return no vector rows until the model is ready, so callers fall back to FTS5 exactly as they already do when the optional dependencies are missing.
  • Write paths are unchanged; task embeddings are already computed outside the write transaction (vec_sync_task_detached).

Verification

  • tests/test_vec_search.py: 8 passed, including two new tests. One checks that query paths do not embed before the model is ready and start exactly one warm-up. The other checks that the vector path still runs once the model is ready. The existing embedding-failure test now marks the model as loaded so it keeps testing that path.
  • Control mutant: with the gate removed from vector_search, the new test fails with query path embedded before the model was ready.
  • End-to-end probe on a WAL database, with a simulated 6 s model load. While search_nodes runs, another connection writes 50 rows and runs PRAGMA wal_checkpoint(PASSIVE):
    • main: 0 of 102 frames checkpointed (snapshot pinned), search_nodes 6.04 s;
    • this change: 102 of 102 frames checkpointed, search_nodes 0.04 s.
  • Search-related test files (vec, search, context packer, task search, link suggestions, smart retrieval, recall, server): 125 passed. test_server_access_log.py::test_best_effort_access_log_does_not_block_read_path failed once under load and then passed twice alone on both this branch and main (timing-sensitive).
  • ruff check . and git diff --check: clean. Local runs: Windows, Python 3.12.13, SQLite 3.53.1.

Not covered

The cause of the 4 h load itself was not established. With this change a slow or hung load only keeps search on FTS5; it no longer blocks checkpoints or the caller.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Hdie6u9nXibq7PSu6xv9RT

RMANOV and others added 2 commits October 14, 2026 13:50
vector_search and task_vector_search embedded the query inside the
caller's SQLite read transaction. The first call in a server process
loads the sentence-transformers model, and on 14 Oct 2026 one
search_nodes call waited 4h14m for that load while holding its WAL
snapshot: no checkpoint could run, the WAL grew to 76 MB, and the
client gave up after 30 minutes.

Query paths now return no vector rows until the model is ready and start
loading it once in a background thread that holds no connection, so
search falls back to FTS5 as it already does without the optional
dependencies. Write paths are unchanged.

Probe on a WAL database with a 6 s simulated load: main pins the
checkpoint (0 of 102 frames) and search_nodes takes 6.04 s; with this
change 102 of 102 frames are checkpointed and search_nodes returns in
0.04 s.

Refs: ClickUp 869fghj8b

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Hdie6u9nXibq7PSu6xv9RT
@RMANOV
RMANOV marked this pull request as ready for review October 14, 2026 11:17
Copilot AI balanced review requested due to automatic review settings October 14, 2026 11:17
@RMANOV
RMANOV merged commit f1f1c24 into main Oct 14, 2026
2 checks passed
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-14T11:18:59.750364Z fe79243 Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@RMANOV
RMANOV deleted the fix/search-model-load-outside-snapshot-20261014 branch October 14, 2026 11:20

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

It introduces background-thread/lock concurrency and a runtime change to search-degradation behavior, which warrants final human review despite being well-tested.

0 open findings

What changed in this PR

This PR fixes a reliability problem where the first vector_search / task_vector_search call in a server process would load the sentence-transformers model inside the caller's SQLite WAL read snapshot. Because the load can take minutes (one observed case held the snapshot for 4h14m), it pinned read slot 0, blocked all WAL checkpoints, and let the WAL grow unbounded. The fix moves model loading off the read path: query paths now gate on a new model_ready() helper that warms the model in a background thread (holding no DB connection) and returns False until it is loaded, so searches transparently fall back to FTS5 — the same degradation already used when optional dependencies are absent.

Changes:

  • Added vec_search.model_ready() plus a _warm_model() background warm-up (single daemon thread guarded by _warmup_lock).
  • Gated vector_search and task_vector_search on model_ready() so they never trigger a model load while inside a read snapshot.
  • Added two tests (query paths don't embed before the model is ready and start exactly one warm-up; vector path still runs once ready) and updated the embedding-failure test to mark the model loaded.
File Description
vec_search.py Adds model_ready()/_warm_model() background warm-up and gates both query paths so the model never loads inside a caller's read snapshot.
tests/​test_vec_search.py Adds coverage for the warm-up gating behavior and updates the embedding-failure test to set the model as loaded.

Notes from verification: server.py's _vector_search is simply an alias import of the now-gated vector_search, and all callers (task_search.py, task_tray.py, context_packer.py, link_suggestions.py) already handle empty vector results by merging with FTS5. The write paths (vec_sync_entity, task sync) still embed within their transactions, which the PR explicitly scopes out. The model_ready() double-checked locking mirrors the existing _get_model() pattern and is sound under the GIL; on a fast-failing load _get_model sets VEC_AVAILABLE = False, short-circuiting further retries.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

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.

2 participants