From 93698329774356d7721f590c1255354a948f9c6c Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Mon, 8 Jun 2026 13:52:51 +0200 Subject: [PATCH] refactor(search): structural per-instance query side-channel; doc search billing gap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-7 claude-review (no blockers): - 🟡 query_token_count/query_embedding were class-level defaults on SearchAlgorithm, relying on each subclass's __init__ to shadow them. Added SearchAlgorithm.__init__ that sets both as instance attributes and had BM25HybridSearchAlgorithm + SemanticSearchAlgorithm call super().__init__(), so per-request concurrency isolation is structural, not by convention. - 🟡 Documented the v1 search-path billing gap: record_search_usage fires only on a fully successful search, so if the query embed succeeded (provider billed + Prometheus recorded) but a later step (Qdrant/verify) raised, no tokens_embedded billing row is written. Added a NOTE at the call site. Left as-is (reasons in PR reply): deployment sequencing (CP METRIC_EVENT_NAMES already renamed; pipeline inert); Ollama _detect_dimension double dimension-set (idempotent, same value); SonarQube issues — 1 is the deliberate TODO(#282) (INFO), 4 are S7503 false positives on async test stubs that must be awaitable (gate green). Deck #284. Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/search/algorithms.py | 10 ++++++++++ nextcloud_mcp_server/search/bm25_hybrid.py | 13 ++++++------- nextcloud_mcp_server/search/semantic.py | 1 + nextcloud_mcp_server/server/semantic.py | 8 ++++++++ 4 files changed, 25 insertions(+), 7 deletions(-) diff --git a/nextcloud_mcp_server/search/algorithms.py b/nextcloud_mcp_server/search/algorithms.py index 9c94da63..b4990b0c 100644 --- a/nextcloud_mcp_server/search/algorithms.py +++ b/nextcloud_mcp_server/search/algorithms.py @@ -292,9 +292,19 @@ class SearchAlgorithm(ABC): per-request, so this side-channel is concurrency-safe. """ + # Class-level defaults are a safety net; __init__ shadows them per instance. query_embedding: list[float] | None = None query_token_count: int | None = None + def __init__(self) -> None: + # Set the query-embedding side-channel as instance attributes so + # concurrent SearchAlgorithm instances never share it through the + # class-level defaults above — per-request isolation by construction, + # not just by the convention that each subclass redeclares them. + # Subclasses with their own __init__ should call super().__init__(). + self.query_embedding: list[float] | None = None + self.query_token_count: int | None = None + @abstractmethod async def search( self, diff --git a/nextcloud_mcp_server/search/bm25_hybrid.py b/nextcloud_mcp_server/search/bm25_hybrid.py index bdbd32dc..9b19cb40 100644 --- a/nextcloud_mcp_server/search/bm25_hybrid.py +++ b/nextcloud_mcp_server/search/bm25_hybrid.py @@ -56,16 +56,15 @@ class BM25HybridSearchAlgorithm(SearchAlgorithm): f"Invalid fusion algorithm '{fusion}'. Must be 'rrf' or 'dbsf'" ) + # super() sets the per-instance query_embedding / query_token_count + # side-channel; this adds the cache key for it. + super().__init__() self.score_threshold = score_threshold self.fusion = models.Fusion.RRF if fusion == "rrf" else models.Fusion.DBSF self.fusion_name = fusion - # Per-request query-embedding cache. ``_embedded_query`` is the query - # string whose dense embedding is held in ``query_embedding`` — repeated - # search() calls on this instance (the doc_types loop) reuse it. These - # shadow the class-level defaults on SearchAlgorithm; set here so all - # three cache fields are instance attributes from construction. - self.query_embedding: list[float] | None = None - self.query_token_count: int | None = None + # ``_embedded_query`` is the query string whose dense embedding is held + # in ``query_embedding`` — repeated search() calls on this per-request + # instance (the doc_types loop) reuse it instead of re-embedding. self._embedded_query: str | None = None @property diff --git a/nextcloud_mcp_server/search/semantic.py b/nextcloud_mcp_server/search/semantic.py index ff516688..c81695ef 100644 --- a/nextcloud_mcp_server/search/semantic.py +++ b/nextcloud_mcp_server/search/semantic.py @@ -35,6 +35,7 @@ class SemanticSearchAlgorithm(SearchAlgorithm): Args: score_threshold: Minimum similarity score (0-1, default: 0.7) """ + super().__init__() self.score_threshold = score_threshold @property diff --git a/nextcloud_mcp_server/server/semantic.py b/nextcloud_mcp_server/server/semantic.py index 57274ef9..382236a4 100644 --- a/nextcloud_mcp_server/server/semantic.py +++ b/nextcloud_mcp_server/server/semantic.py @@ -593,6 +593,14 @@ def configure_semantic_tools(mcp: FastMCP): # query is embedded — and metered — exactly once regardless of how # many doc_types were searched. See record_search_usage for the # metric/privacy details. + # + # NOTE (v1 billing gap): this fires only on a fully successful + # search. If the query embed succeeded (provider billed the tokens, + # and Prometheus recorded them via record_embedding_tokens) but a + # later step (Qdrant/verify) raised, no tokens_embedded row is + # written — the embed cost is real but absent from the billing + # ledger. Acceptable for v1 (search failures are rare and the meter + # is not billed today); revisit if billing accuracy needs it. await record_search_usage( enabled=settings.usage_metering_enabled, user_id=username,