From 8fa6aa24ded22acf4fc1877761406b509b74b6ef Mon Sep 17 00:00:00 2001 From: ShellyBot Date: Fri, 28 Aug 2026 10:03:32 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20address=20CodeRabbit=20review=20?= =?UTF-8?q?=E2=80=94=20clarify=20docs=20and=20strengthen=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Remove 'full top_k' guarantee from docs/config/db comments: pgvector may stop at max_scan_tuples or scan_mem_multiplier thresholds. 2. Strengthen test_connect_listener_not_registered_when_disabled: AssertingCursor raises AssertionError if execute is called. 3. Strengthen test_version_gate_skipped_when_off: invoke init_db with fake engine, assert pg_extension query never executes. --- docs/v3/contributing/configuration.mdx | 5 +- src/config.py | 6 ++- src/db.py | 6 ++- tests/test_hnsw_iterative_scan.py | 66 ++++++++++++++++++++++---- 4 files changed, 70 insertions(+), 13 deletions(-) diff --git a/docs/v3/contributing/configuration.mdx b/docs/v3/contributing/configuration.mdx index 7bfde3d4..f9b99ced 100644 --- a/docs/v3/contributing/configuration.mdx +++ b/docs/v3/contributing/configuration.mdx @@ -560,7 +560,10 @@ DB_CONNECT_TIMEOUT_SECONDS=2 # pgvector HNSW iterative scan (requires pgvector >= 0.8.0) # When set, each new pool connection gets the GUC applied so filtered -# HNSW queries return full top_k results instead of silently under-returning. +# HNSW queries scan additional candidates instead of silently +# under-returning when out-of-scope rows consume the initial scan budget. +# Note: pgvector may still stop before reaching top_k if hnsw.max_scan_tuples +# or hnsw.scan_mem_multiplier thresholds are exceeded. # Valid values: "off", "strict_order", "relaxed_order", or unset (None). DB_HNSW_ITERATIVE_SCAN=strict_order ``` diff --git a/src/config.py b/src/config.py index a80614be..5aedca5f 100644 --- a/src/config.py +++ b/src/config.py @@ -741,8 +741,10 @@ class DBSettings(HonchoSettings): # pgvector HNSW iterative scan mode for filtered approximate searches. # When set (default "strict_order"), applied as a per-connection server - # setting so filtered HNSW queries return full top_k results instead of - # silently under-returning when out-of-scope rows consume the scan budget. + # setting so filtered HNSW queries scan additional candidates instead of + # silently under-returning when out-of-scope rows consume the initial scan + # budget. Note: pgvector may still stop before reaching top_k if + # hnsw.max_scan_tuples or hnsw.scan_mem_multiplier thresholds are exceeded. # Requires pgvector >= 0.8.0. # See: https://github.com/pgvector/pgvector#iterative-index-scans HNSW_ITERATIVE_SCAN: Literal["off", "strict_order", "relaxed_order"] | None = ( diff --git a/src/db.py b/src/db.py index f50a0fd0..933fcf58 100644 --- a/src/db.py +++ b/src/db.py @@ -95,9 +95,11 @@ def _set_hnsw_iterative_scan_on_connect( """Apply pgvector HNSW iterative scan GUC per-connection. Registered when ``DB.HNSW_ITERATIVE_SCAN`` is set. Fires once per new - pool connection so filtered HNSW queries return full top_k results + pool connection so filtered HNSW queries scan additional candidates instead of silently under-returning when out-of-scope rows consume the - scan budget. Uses ``set_config`` with a bind parameter (same pattern as + initial scan budget. Note: pgvector may still stop before reaching top_k + if ``hnsw.max_scan_tuples`` or ``hnsw.scan_mem_multiplier`` thresholds are + exceeded. Uses ``set_config`` with a bind parameter (same pattern as ``_set_application_name_on_checkout``) rather than an f-string ``SET`` to avoid special-casing utility-statement parameter binding. diff --git a/tests/test_hnsw_iterative_scan.py b/tests/test_hnsw_iterative_scan.py index 4315c31d..dcab1e88 100644 --- a/tests/test_hnsw_iterative_scan.py +++ b/tests/test_hnsw_iterative_scan.py @@ -88,24 +88,74 @@ def test_connect_listener_not_registered_when_disabled( from src import db as db_module - dummy_conn = types.SimpleNamespace(autocommit=False, cursor=lambda: types.SimpleNamespace(execute=lambda *a, **k: None, close=lambda: None)) + execute_calls: list[tuple[tuple[object, ...], dict[str, object]]] = [] + + class AssertingCursor: + def execute(self, *args: object, **kwargs: object) -> None: + execute_calls.append((args, kwargs)) + raise AssertionError( + "execute should not be called when HNSW_ITERATIVE_SCAN is None" + ) + + def close(self) -> None: + pass + + dummy_conn = types.SimpleNamespace( + autocommit=False, cursor=lambda: AssertingCursor() + ) # The function reads settings.DB.HNSW_ITERATIVE_SCAN at call time, # so with monkeypatch it should return early without executing SQL. - # pyright: ignore[reportPrivateUsage] - db_module._set_hnsw_iterative_scan_on_connect(dummy_conn, None) - # No exception raised — early return worked. + db_module._set_hnsw_iterative_scan_on_connect(dummy_conn, None) # type: ignore[attr-defined] + assert execute_calls == [], "No SQL should execute when HNSW_ITERATIVE_SCAN is None" def test_version_gate_skipped_when_off(monkeypatch: pytest.MonkeyPatch) -> None: """The pgvector version check should NOT run when HNSW_ITERATIVE_SCAN is 'off'. Only 'strict_order' and 'relaxed_order' require pgvector >= 0.8.0. + + Uses a fake engine/connection to assert that the pg_extension + query is never executed when the setting is 'off'. """ + from unittest.mock import AsyncMock, MagicMock, patch + from src.config import settings + from src import db as db_module monkeypatch.setattr(settings.DB, "HNSW_ITERATIVE_SCAN", "off") assert settings.DB.HNSW_ITERATIVE_SCAN == "off" - # The version gate condition is: - # settings.DB.HNSW_ITERATIVE_SCAN in ("strict_order", "relaxed_order") - # "off" is not in that tuple, so the check is skipped. - assert settings.DB.HNSW_ITERATIVE_SCAN not in ("strict_order", "relaxed_order") + + execute_calls: list[str] = [] + + class FakeResult: + def fetchone(self) -> None: + return None + + async def fake_execute(stmt: object, *args: object, **kwargs: object) -> FakeResult: + execute_calls.append(str(stmt)) + return FakeResult() + + fake_conn = MagicMock() + fake_conn.execute = fake_execute + fake_conn.commit = AsyncMock() + + class FakeAsyncCM: + async def __aenter__(self) -> MagicMock: + return fake_conn + + async def __aexit__(self, *args: object) -> None: + pass + + fake_engine = MagicMock() + fake_engine.connect = MagicMock(return_value=FakeAsyncCM()) + + with patch.object(db_module, "engine", fake_engine), \ + patch("alembic.command.upgrade"), \ + patch("alembic.config.Config"): + import asyncio + asyncio.run(db_module.init_db()) + + pg_extension_calls = [c for c in execute_calls if "pg_extension" in c] + assert pg_extension_calls == [], ( + "pg_extension version query should not execute when HNSW_ITERATIVE_SCAN is 'off'" + )