From afd5286cf658a7899124b73f48d2f6c53fb2db47 Mon Sep 17 00:00:00 2001 From: Jiangzhou He Date: Thu, 6 Aug 2026 15:28:00 -0700 Subject: [PATCH] fix(search): surface daemon-side tracebacks and reject NULL KNN distances Issue #270 reported `ccc search --path` crashing with `TypeError: unsupported operand type(s) for *: 'NoneType' and 'NoneType'`. The crash could not be reproduced, and the reported root cause does not hold: `vec_distance_L2` never returns NULL, it raises (verified against sqlite-vec 0.1.6-0.1.9, SQLite 3.46/3.53, multi-chunk tables and re-index churn). What the report did expose is that the failure was undiagnosable. Two gaps, both fixed here: - The daemon's search handler discarded the traceback (`ErrorResponse(message=str(e))`), so the reporter saw only the client's re-raise frames. It now sends `traceback.format_exc()` and logs the exception; `_dispatch` does the same. On the client, the `raise RuntimeError(f"Daemon error: ...")` pattern was duplicated at five sites and only `doctor()` appended `resp.traceback` -- the search path, which the reporter came through, dropped it. Consolidated into one `_daemon_error()` helper used by all five. - `_knn_query` now rejects rows with a NULL distance, raising a specific error naming the query shape and offending file instead of dying in `_l2_to_score`. `distance` is a hidden vec0 column that sqlite-vec populates only under the KNN query plan and returns NULL for on a full scan, so a NULL means the plan we asked for is not the plan we got. The guard lives in `_knn_query` rather than the caller because the multi-language merge path sorts on `r[5]` in `heapq.nsmallest`, where a NULL would fail on `None < float` before any caller-side check ran. `_full_scan_query` needs no guard: it computes `vec_distance_L2(...)` itself, which works under any plan and raises rather than returning NULL on bad input. The new tests build a real in-memory vec0 table with the indexer's exact DDL (no embedding model, ~0.2s) and cover both that `--path` filtering yields usable distances and that the bare-`distance`-under-full-scan shape is the one that does not. Co-Authored-By: Claude Fable 5 --- src/cocoindex_code/client.py | 26 +++++--- src/cocoindex_code/daemon.py | 5 +- src/cocoindex_code/query.py | 59 ++++++++++++---- tests/test_client.py | 19 ++++++ tests/test_daemon.py | 33 +++++++++ tests/test_query_filters.py | 126 +++++++++++++++++++++++++++++++++++ 6 files changed, 245 insertions(+), 23 deletions(-) create mode 100644 tests/test_query_filters.py diff --git a/src/cocoindex_code/client.py b/src/cocoindex_code/client.py index f65dafb..12594da 100644 --- a/src/cocoindex_code/client.py +++ b/src/cocoindex_code/client.py @@ -228,6 +228,19 @@ def _handle_vanished_daemon() -> None: ) +def _daemon_error(resp: ErrorResponse) -> RuntimeError: + """Build the exception for an ``ErrorResponse`` from the daemon. + + The daemon-side traceback is appended when present: without it the caller + only ever sees the client's own re-raise frames, which says nothing about + where the failure actually happened (issue #270). + """ + detail = f"Daemon error: {resp.message}" + if resp.traceback: + detail += f"\n{resp.traceback}" + return RuntimeError(detail) + + class _HandshakeResult(NamedTuple): conn: Connection resp: HandshakeResponse @@ -261,7 +274,7 @@ def _raw_connect_and_handshake() -> _HandshakeResult: raise DaemonProtocolError(f"Undecodable handshake reply from daemon: {e}") from e if isinstance(resp, ErrorResponse): conn.close() - raise RuntimeError(f"Daemon error: {resp.message}") + raise _daemon_error(resp) if not isinstance(resp, HandshakeResponse): conn.close() raise RuntimeError(f"Unexpected handshake response: {type(resp).__name__}") @@ -340,7 +353,7 @@ def _send(req: Request) -> Response: conn.close() resp = decode_response(data) if isinstance(resp, ErrorResponse): - raise RuntimeError(f"Daemon error: {resp.message}") + raise _daemon_error(resp) return resp @@ -366,7 +379,7 @@ def index( raise RuntimeError("Connection to daemon lost during indexing") resp = decode_response(data) if isinstance(resp, ErrorResponse): - raise RuntimeError(f"Daemon error: {resp.message}") + raise _daemon_error(resp) if isinstance(resp, IndexWaitingNotice): if on_waiting is not None: on_waiting() @@ -419,7 +432,7 @@ def search( raise RuntimeError("Connection to daemon lost during search") resp = decode_response(data) if isinstance(resp, ErrorResponse): - raise RuntimeError(f"Daemon error: {resp.message}") + raise _daemon_error(resp) if isinstance(resp, IndexWaitingNotice): if on_waiting is not None: on_waiting() @@ -496,10 +509,7 @@ def doctor( raise RuntimeError("Connection to daemon lost during doctor checks") resp = decode_response(data) if isinstance(resp, ErrorResponse): - detail = f"Daemon error: {resp.message}" - if resp.traceback: - detail += f"\n{resp.traceback}" - raise RuntimeError(detail) + raise _daemon_error(resp) if isinstance(resp, DoctorResponse): results.append(resp.result) if on_result is not None: diff --git a/src/cocoindex_code/daemon.py b/src/cocoindex_code/daemon.py index a291956..4a97554 100644 --- a/src/cocoindex_code/daemon.py +++ b/src/cocoindex_code/daemon.py @@ -346,7 +346,8 @@ async def _search_with_wait( offset=req.offset, ) except Exception as e: - yield ErrorResponse(message=str(e)) + logger.exception("Error handling search request") + yield ErrorResponse(message=str(e), traceback=traceback.format_exc()) async def _handle_doctor( @@ -605,7 +606,7 @@ async def _dispatch( return ErrorResponse(message=f"Unknown request type: {type(req).__name__}") except Exception as e: logger.exception("Error dispatching request") - return ErrorResponse(message=str(e)) + return ErrorResponse(message=str(e), traceback=traceback.format_exc()) # --------------------------------------------------------------------------- diff --git a/src/cocoindex_code/query.py b/src/cocoindex_code/query.py index a2991ee..d7243d5 100644 --- a/src/cocoindex_code/query.py +++ b/src/cocoindex_code/query.py @@ -16,6 +16,33 @@ def _l2_to_score(distance: float) -> float: return 1.0 - distance * distance / 2.0 +def _checked(rows: list[tuple[Any, ...]], query_shape: str) -> list[tuple[Any, ...]]: + """Return *rows*, failing loudly if any of them carries a NULL distance. + + ``code_chunks_vec`` is a vec0 virtual table whose ``distance`` is a *hidden* + column: sqlite-vec populates it only under the KNN query plan and yields + NULL for it on a plain full scan. A NULL therefore means the plan we asked + for is not the plan we got, which is a bug worth reporting rather than a + result worth ranking. Left unchecked, the NULL flows into ``_l2_to_score`` + and surfaces as an unrelated-looking ``TypeError: unsupported operand + type(s) for *: 'NoneType' and 'NoneType'`` (issue #270). + + Only KNN queries need this guard: ``_full_scan_query`` computes its own + ``vec_distance_L2(...)``, which works under any plan and raises (never + returns NULL) on bad input. + """ + bad = next((row for row in rows if row[5] is None), None) + if bad is not None: + raise RuntimeError( + f"Vector index returned a row with no distance ({query_shape}, " + f"file_path={bad[0]!r}) — the sqlite-vec KNN query plan was not " + "used. Please report this at " + "https://github.com/cocoindex-io/cocoindex-code/issues along with " + "the output of `ccc doctor`." + ) + return rows + + def _knn_query( conn: sqlite3.Connection, embedding_bytes: bytes, @@ -24,24 +51,30 @@ def _knn_query( ) -> list[tuple[Any, ...]]: """Run a vec0 KNN query, optionally constrained to a language partition.""" if language is not None: - return conn.execute( + return _checked( + conn.execute( + """ + SELECT file_path, language, content, start_line, end_line, distance + FROM code_chunks_vec + WHERE embedding MATCH ? AND k = ? AND language = ? + ORDER BY distance + """, + (embedding_bytes, k, language), + ).fetchall(), + f"knn language={language!r}", + ) + return _checked( + conn.execute( """ SELECT file_path, language, content, start_line, end_line, distance FROM code_chunks_vec - WHERE embedding MATCH ? AND k = ? AND language = ? + WHERE embedding MATCH ? AND k = ? ORDER BY distance """, - (embedding_bytes, k, language), - ).fetchall() - return conn.execute( - """ - SELECT file_path, language, content, start_line, end_line, distance - FROM code_chunks_vec - WHERE embedding MATCH ? AND k = ? - ORDER BY distance - """, - (embedding_bytes, k), - ).fetchall() + (embedding_bytes, k), + ).fetchall(), + "knn unfiltered", + ) def _full_scan_query( diff --git a/tests/test_client.py b/tests/test_client.py index 5b1f453..736ed1b 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -387,3 +387,22 @@ def test_daemon_version_error_message_reflects_cause() -> None: ) assert "stale global settings" in str(settings_err) assert "version mismatch" not in str(settings_err) + + +def test_daemon_error_carries_daemon_side_traceback() -> None: + """Daemon-side tracebacks reach the caller — the client frames alone say nothing. + + Regression guard for issue #270, where a daemon search crash surfaced as a + bare `Daemon error: ` and the reporter had no frame pointing at the + code that actually failed. + """ + from cocoindex_code.protocol import ErrorResponse + + err = client._daemon_error(ErrorResponse(message="boom", traceback="Traceback: frame\nfoo")) + assert "Daemon error: boom" in str(err) + assert "Traceback: frame\nfoo" in str(err) + + # No traceback recorded (e.g. a deliberate ErrorResponse, not an exception): + # the message stands alone, with no trailing noise. + plain = client._daemon_error(ErrorResponse(message="run `ccc init` first")) + assert str(plain) == "Daemon error: run `ccc init` first" diff --git a/tests/test_daemon.py b/tests/test_daemon.py index d6a893c..355ec12 100644 --- a/tests/test_daemon.py +++ b/tests/test_daemon.py @@ -357,3 +357,36 @@ def test_daemon_search_waits_for_load_time_indexing(daemon_sock: str) -> None: assert isinstance(resp2, SearchResponse) assert resp2.success is True conn2.close() + + +async def test_search_failure_reports_daemon_side_traceback() -> None: + """A crash inside search reaches the client with the daemon's own frames. + + Without this the client sees only `Daemon error: ` and its own + re-raise frames, which is why issue #270 arrived with nothing pointing at + the code that actually failed. + """ + from typing import Any, cast + + from cocoindex_code.daemon import _search_with_wait + from cocoindex_code.project import Project + from cocoindex_code.protocol import ErrorResponse, SearchRequest + + class _FailingProject: + async def wait_for_indexing_done(self) -> None: + return None + + async def search(self, **_kwargs: Any) -> None: + raise RuntimeError("simulated query failure") + + req = SearchRequest(project_root="/tmp/whatever", query="anything") + responses = [resp async for resp in _search_with_wait(cast(Project, _FailingProject()), req)] + + error = responses[-1] + assert isinstance(error, ErrorResponse) + assert error.message == "simulated query failure" + assert error.traceback is not None + assert "simulated query failure" in error.traceback + # The daemon-side frames, not just the exception text. + assert "_search_with_wait" in error.traceback + assert "in search" in error.traceback diff --git a/tests/test_query_filters.py b/tests/test_query_filters.py new file mode 100644 index 0000000..8bc1097 --- /dev/null +++ b/tests/test_query_filters.py @@ -0,0 +1,126 @@ +"""Tests for the SQL layer of codebase search (issue #270). + +These run against a real in-memory ``vec0`` table built with the same DDL the +indexer produces, so they exercise actual sqlite-vec behaviour rather than a +mock: which query shapes yield a usable ``distance``, and which yield NULL. +No embedding model is involved — vectors are supplied directly. +""" + +from __future__ import annotations + +import sqlite3 +import struct + +import pytest + +from cocoindex_code.query import _checked, _full_scan_query, _knn_query + +DIM = 4 + +# Mirrors the vec0 table the indexer mounts: an INTEGER primary key, `language` +# as the partition key, the payload columns as auxiliary (`+`) columns, and the +# vector last. See `indexer_main` in cocoindex_code/indexer.py. +_DDL = f""" +CREATE VIRTUAL TABLE "code_chunks_vec" USING vec0( + id INTEGER primary key, + +file_path TEXT, + language TEXT partition key, + +content TEXT, + +start_line INTEGER, + +end_line INTEGER, + embedding float[{DIM}] +) +""" + +_ROWS = [ + (0, "src/main.py", "python", "fibonacci", 1, 5, (1.0, 0.0, 0.0, 0.0)), + (1, "src/util.py", "python", "parse csv", 1, 5, (0.0, 1.0, 0.0, 0.0)), + (2, "lib/db.py", "python", "connect", 1, 5, (0.0, 0.0, 1.0, 0.0)), + (3, "lib/api.rs", "rust", "handler", 1, 5, (0.0, 0.0, 0.0, 1.0)), +] + + +def _vec(values: tuple[float, ...]) -> bytes: + return struct.pack(f"{len(values)}f", *values) + + +@pytest.fixture +def conn() -> sqlite3.Connection: + sqlite_vec = pytest.importorskip("sqlite_vec") + c = sqlite3.connect(":memory:") + c.enable_load_extension(True) + sqlite_vec.load(c) + c.enable_load_extension(False) + c.execute(_DDL) + c.executemany( + "INSERT INTO code_chunks_vec" + "(id, file_path, language, content, start_line, end_line, embedding)" + " VALUES (?,?,?,?,?,?,?)", + [(*row[:6], _vec(row[6])) for row in _ROWS], + ) + return c + + +def test_full_scan_query_with_path_filter_returns_usable_distances( + conn: sqlite3.Connection, +) -> None: + """`--path` filtering must produce real distances, not NULLs (issue #270).""" + rows = _full_scan_query(conn, _vec((1.0, 0.0, 0.0, 0.0)), limit=10, offset=0, paths=["src/*"]) + + assert [row[0] for row in rows] == ["src/main.py", "src/util.py"] + assert all(isinstance(row[5], float) for row in rows) + # Nearest first: main.py is the exact match. + assert rows[0][5] == pytest.approx(0.0) + + +def test_full_scan_query_combines_language_and_path_filters(conn: sqlite3.Connection) -> None: + rows = _full_scan_query( + conn, + _vec((0.0, 0.0, 1.0, 0.0)), + limit=10, + offset=0, + languages=["python"], + paths=["lib/*"], + ) + + assert [row[0] for row in rows] == ["lib/db.py"] + assert rows[0][5] == pytest.approx(0.0) + + +def test_knn_query_returns_usable_distances(conn: sqlite3.Connection) -> None: + unfiltered = _knn_query(conn, _vec((1.0, 0.0, 0.0, 0.0)), k=4) + assert len(unfiltered) == 4 + assert all(isinstance(row[5], float) for row in unfiltered) + + partitioned = _knn_query(conn, _vec((0.0, 0.0, 0.0, 1.0)), k=4, language="rust") + assert [row[0] for row in partitioned] == ["lib/api.rs"] + + +def test_bare_distance_column_is_null_outside_the_knn_plan(conn: sqlite3.Connection) -> None: + """The one shape that yields NULL distances — the failure `_checked` guards. + + `distance` is a hidden vec0 column: sqlite-vec fills it in only under the + KNN plan and returns NULL for it on a full scan. Locking this in documents + *why* the guard exists, and would catch sqlite-vec changing the contract. + """ + rows = conn.execute( + "SELECT file_path, language, content, start_line, end_line, distance " + "FROM code_chunks_vec WHERE file_path GLOB 'src/*'" + ).fetchall() + + assert rows, "expected the full scan to match rows" + assert all(row[5] is None for row in rows) + + with pytest.raises(RuntimeError) as excinfo: + _checked(rows, "knn language='python'") + + message = str(excinfo.value) + assert "no distance" in message + assert "knn language='python'" in message + assert "src/main.py" in message + assert "issues" in message + + +def test_checked_passes_through_rows_with_distances() -> None: + rows = [("src/main.py", "python", "body", 1, 5, 0.5)] + assert _checked(rows, "knn unfiltered") is rows