fix(searcher): probe HNSW divergence in CLI search to avoid silent SIGBUS - #1967
Open
trek-e wants to merge 1 commit into
Open
fix(searcher): probe HNSW divergence in CLI search to avoid silent SIGBUS#1967trek-e wants to merge 1 commit into
trek-e wants to merge 1 commit into
Conversation
…GBUS The MCP path gates vector queries against HNSW divergence via the module-level ``_vector_disabled`` flag (MemPalace#1222): when ``hnsw_capacity_status`` reports a diverged segment, MCP search tools route to the BM25-only SQLite fallback. The CLI ``search()`` path was missing the gate, so ``mempalace search`` on a diverged palace called ``col.query()`` straight into ChromaDB's Rust bindings and segfaulted (exit 139 / SIGBUS at ``chromadb/api/rust.py:_query``) with zero diagnostic output. Add the same gate to the CLI path: before ``_warn_if_legacy_metric`` and ``col.query()``, probe ``hnsw_capacity_status``; if diverged, route to a BM25-only fallback printer that emits a clear NOTICE pointing at ``mempalace repair`` and renders results in the same shape as the vector path. The probe is exception-safe — any failure falls through to the vector path so the probe itself can never be the thing that crashes search. Reproduces deterministically on a ~1M-drawer palace after HNSW compaction fails mid-write (see epic MemPalace#1963 for the broader concurrent-writer corruption cluster this belongs to). Adds two regression tests: - ``test_search_routes_to_bm25_when_hnsw_diverged`` — verifies ``col.query()`` is NOT called and the BM25 fallback fires when the probe reports divergence. - ``test_search_proceeds_to_vector_when_hnsw_healthy`` — paired guard that the probe does not short-circuit on a healthy palace. 🤖 This commit was prepared with AI assistance. The fix design and implementation are mine; the AI pair helped with the test scaffolding.
Member
|
Thanks for this contribution, and apologies for the slow turnaround.
If you'd rather not pick it back up, no problem at all — just say so and I'll close it out, and thanks either way for taking the time to send it. |
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
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.
What does this PR do?
Adds the HNSW divergence gate to the CLI
search()path that the MCP path has had since #1222.Bug:
mempalace searchon a palace whose HNSW segment has diverged from SQLite segfaults ChromaDB's Rust bindings — exit 139 (SIGBUS) atchromadb/api/rust.py:_query, zero stdout, zero stderr. The MCP path avoids this via the module-level_vector_disabledflag (set whenhnsw_capacity_statusreports divergence → route to BM25-only SQLite fallback). The CLIsearch()path was missing the same gate, so it calledcol.query()straight into the diverged segment.Fix: before
_warn_if_legacy_metric(col)andcol.query(), probehnsw_capacity_status. If diverged, route to a new_print_search_results_bm25_only()helper that:mempalace repairThe probe is exception-safe: any failure falls through to
Falseso the probe itself can never be the thing that crashes search.How to test
Two new regression tests in
TestSearchCLI:test_search_routes_to_bm25_when_hnsw_diverged— verifiescol.query()is NOT called and the BM25 fallback fires when the probe reports divergence. Reproduces the bug pattern (without the actual segfault) by mockinghnsw_capacity_statusto return{"diverged": True}.test_search_proceeds_to_vector_when_hnsw_healthy— paired guard that the probe does not short-circuit on a healthy palace (prevents a regression where the gate accidentally always fires).Checklist
uv run pytest tests/test_searcher.py tests/test_hnsw_capacity.py tests/test_backends.py→ 168 passed)uvx --from "ruff==0.15.14" ruff check mempalace/searcher.py tests/test_searcher.py→ All checks passed;ruff format --check→ 2 files already formatted)Scope
Intentionally narrow — one probe + one fallback printer in
searcher.py, plus the two regression tests. No changes tohnsw_capacity_status(the detection probe, owned byd70433), no changes to_bm25_only_via_sqlite(the existing fallback used by the MCP path), no changes to the MCP server's_vector_disabledmechanism.Part of epic #1963 (concurrent-writer HNSW corruption cluster). This is a Tier-1 tactical guardrail — it doesn't prevent divergence, but it stops the CLI from silently crashing when divergence exists.
🤖 This PR was prepared with AI assistance. The fix design and implementation are mine; the AI pair helped with test scaffolding and PR logistics.
Note on test-windows CI
The job failed with
tests/test_daemon.py::test_worker_overrides_client_palace_path - DaemonError: timed out. This is a pre-existing Windows timing flake, not a regression from this PR:mempalace/searcher.pyandtests/test_searcher.py— zero connection to the daemon (grep -iE 'daemon|queue|worker'on the diff is empty).timeout=5. On slower Windows runners that window is too tight; the test passes locally on macOS in 0.94s.--only-rerun "Failed to apply logs to the hnsw segment writer"filter doesn't matchDaemonError: timed out, so the flake never gets an automatic retry.test_mcp_server.pyplus this daemon Windows timing issue are both currently affecting all PRs opened againstdevelop.The four green test jobs (Linux 3.9/3.11/3.13, macOS) cover the actual change in this PR.