refactor(search): structural per-instance query side-channel; doc search billing gap
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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
973f80e7b9
commit
9369832977
@@ -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,
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user