From e497a9975d36e6282fba55803288b54b5f091168 Mon Sep 17 00:00:00 2001 From: phernandez Date: Thu, 30 Jul 2026 17:24:26 -0500 Subject: [PATCH 1/9] feat(api): add cached QUERY search Signed-off-by: phernandez --- .../api/v2/routers/search_router.py | 227 ++++++++++++------ src/basic_memory/deps/read_cache.py | 3 +- src/basic_memory/mcp/clients/search.py | 4 +- src/basic_memory/mcp/tools/utils.py | 82 +++++++ src/basic_memory/read_cache/contract.py | 1 + src/basic_memory/read_cache/policy.py | 1 + test-int/read_cache/test_search_read_cache.py | 151 ++++++++++++ tests/api/v2/test_search_router.py | 19 ++ tests/mcp/clients/test_clients.py | 12 +- tests/mcp/test_client_telemetry.py | 4 +- tests/mcp/test_tool_utils.py | 33 +++ 11 files changed, 448 insertions(+), 89 deletions(-) create mode 100644 test-int/read_cache/test_search_read_cache.py diff --git a/src/basic_memory/api/v2/routers/search_router.py b/src/basic_memory/api/v2/routers/search_router.py index 83ff1e2bd..1e903d9cb 100644 --- a/src/basic_memory/api/v2/routers/search_router.py +++ b/src/basic_memory/api/v2/routers/search_router.py @@ -5,11 +5,30 @@ """ import asyncio +import json +from contextlib import nullcontext +from typing import Annotated -from fastapi import APIRouter, HTTPException, Path +from fastapi import APIRouter, Depends, HTTPException, Path, Response import logfire from basic_memory.api.v2.utils import to_search_results +from basic_memory.deps import ( + EntityServiceV2ExternalDep, + ProjectExternalIdPathDep, + ReadCacheDep, + SearchReindexSchedulerDep, + SearchServiceV2ExternalDep, + create_model_read_cache, +) +from basic_memory.read_cache import ( + ModelReadCache, + ReadCacheKey, + ReadCacheOperation, + ReadCacheScope, + read_cache_request_digest, +) +from basic_memory.read_cache.policy import SEARCH_READ_CACHE_TTL_SECONDS from basic_memory.repository.semantic_errors import ( RerankProviderContractError, RerankTransientError, @@ -17,26 +36,59 @@ SemanticSearchDisabledError, ) from basic_memory.schemas.search import SearchQuery, SearchResponse, SearchRetrievalMode -from basic_memory.deps import ( - SearchServiceV2ExternalDep, - EntityServiceV2ExternalDep, - SearchReindexSchedulerDep, - ProjectExternalIdPathDep, -) -# Note: No prefix here - it's added during registration as /v2/{project_id}/search +# App registration mounts this router at /v2/projects/{project_id}. router = APIRouter(tags=["search"]) +def get_search_read_cache( + read_cache: ReadCacheDep, +) -> ModelReadCache[SearchResponse] | None: + """Bind search responses to the optional cache backend and short query TTL.""" + return create_model_read_cache( + read_cache, + SearchResponse, + ttl_seconds=SEARCH_READ_CACHE_TTL_SECONDS, + ) + + +SearchReadCacheDep = Annotated[ + ModelReadCache[SearchResponse] | None, + Depends(get_search_read_cache), +] + + +def _search_request_digest(query: SearchQuery, *, page: int, page_size: int) -> str: + """Return one structural identity for POST and QUERY search requests.""" + canonical_request = json.dumps( + { + "query": query.model_dump(mode="json"), + "page": page, + "page_size": page_size, + }, + sort_keys=True, + separators=(",", ":"), + ) + return read_cache_request_digest(canonical_request) + + +@router.api_route( + "/search/", + methods=["QUERY"], + response_model=SearchResponse, + include_in_schema=False, +) @router.post("/search/", response_model=SearchResponse) async def search( query: SearchQuery, search_service: SearchServiceV2ExternalDep, entity_service: EntityServiceV2ExternalDep, + read_cache: SearchReadCacheDep, + response: Response, project_id: str = Path(..., description="Project external UUID"), page: int = 1, page_size: int = 10, -): +) -> SearchResponse: """Search across all knowledge and documents in a project. V2 uses external_id UUIDs for stable API references. @@ -52,6 +104,7 @@ async def search( Returns: SearchResponse with paginated search results """ + response.headers["Accept-Query"] = "application/json" with logfire.span( "api.request.search", entrypoint="api", @@ -70,81 +123,99 @@ async def search( query.note_types or query.entity_types or query.categories or query.metadata_filters ), ): - offset = (page - 1) * page_size - exact_count_available = query.retrieval_mode == SearchRetrievalMode.FTS - try: + cache_key = ReadCacheKey( + project_id=project_id, + operation=ReadCacheOperation.search, + request_digest=_search_request_digest(query, page=page, page_size=page_size), + ) + cache_scope = ( + read_cache.read(key=cache_key) + if read_cache is not None + else nullcontext(ReadCacheScope[SearchResponse]()) + ) + async with cache_scope as cached: + if cached.value is not None: + return cached.value + + offset = (page - 1) * page_size + exact_count_available = query.retrieval_mode == SearchRetrievalMode.FTS + try: + with logfire.span( + "api.search.search.execute_query", + domain="search", + action="search", + phase="execute_query", + page=page, + page_size=page_size, + ): + if exact_count_available: + results, total = await asyncio.gather( + search_service.search(query, limit=page_size, offset=offset), + search_service.count(query), + ) + else: + results = await search_service.search( + query, limit=page_size + 1, offset=offset + ) + total = 0 + except SemanticSearchDisabledError as exc: + raise HTTPException(status_code=400, detail=str(exc)) from exc + except SemanticDependenciesMissingError as exc: + raise HTTPException(status_code=400, detail=str(exc)) from exc + except RerankTransientError as exc: + # Returning raw retrieval order would make pagination inconsistent with + # earlier reranked pages. Preserve ordering semantics and make the outage + # explicitly retryable instead. + raise HTTPException(status_code=503, detail=str(exc)) from exc + except RerankProviderContractError as exc: + # Upstream reranker returned a malformed response — an upstream fault, not a + # client error and not a transient outage (those map to a retryable 503). + raise HTTPException(status_code=502, detail=str(exc)) from exc + except ValueError as exc: + raise HTTPException(status_code=400, detail=str(exc)) from exc + with logfire.span( - "api.search.search.execute_query", + "api.search.search.paginate_results", domain="search", action="search", - phase="execute_query", - page=page, - page_size=page_size, + phase="paginate_results", + result_count=len(results), ): if exact_count_available: - results, total = await asyncio.gather( - search_service.search(query, limit=page_size, offset=offset), - search_service.count(query), - ) + has_more = offset + len(results) < total else: - results = await search_service.search(query, limit=page_size + 1, offset=offset) - total = 0 - except SemanticSearchDisabledError as exc: - raise HTTPException(status_code=400, detail=str(exc)) from exc - except SemanticDependenciesMissingError as exc: - raise HTTPException(status_code=400, detail=str(exc)) from exc - except RerankTransientError as exc: - # Returning raw retrieval order would make pagination inconsistent with - # earlier reranked pages. Preserve ordering semantics and make the outage - # explicitly retryable instead. - raise HTTPException(status_code=503, detail=str(exc)) from exc - except RerankProviderContractError as exc: - # Upstream reranker returned a malformed response — an upstream fault, not a - # client error and not a transient outage (those map to a retryable 503). - raise HTTPException(status_code=502, detail=str(exc)) from exc - except ValueError as exc: - raise HTTPException(status_code=400, detail=str(exc)) from exc - - with logfire.span( - "api.search.search.paginate_results", - domain="search", - action="search", - phase="paginate_results", - result_count=len(results), - ): - if exact_count_available: - has_more = offset + len(results) < total - else: - # Trigger: semantic modes would need another vector/hybrid retrieval to count. - # Why: search requests should not pay for a second semantic pass. - # Outcome: preserve probe pagination, leave total at 0, and mark it unknown. - has_more = len(results) > page_size - if has_more: - results = results[:page_size] - - with logfire.span( - "api.search.search.hydrate_results", - domain="search", - action="search", - phase="hydrate_results", - result_count=len(results), - ): - search_results = await to_search_results(entity_service, results) - with logfire.span( - "api.search.search.build_response", - domain="search", - action="search", - phase="build_response", - result_count=len(search_results), - ): - return SearchResponse( - results=search_results, - current_page=page, - page_size=page_size, - total=total, - total_is_exact=exact_count_available, - has_more=has_more, - ) + # Trigger: semantic modes would need another vector/hybrid retrieval to count. + # Why: search requests should not pay for a second semantic pass. + # Outcome: preserve probe pagination, leave total at 0, and mark it unknown. + has_more = len(results) > page_size + if has_more: + results = results[:page_size] + + with logfire.span( + "api.search.search.hydrate_results", + domain="search", + action="search", + phase="hydrate_results", + result_count=len(results), + ): + search_results = await to_search_results(entity_service, results) + with logfire.span( + "api.search.search.build_response", + domain="search", + action="search", + phase="build_response", + result_count=len(search_results), + ): + result = SearchResponse( + results=search_results, + current_page=page, + page_size=page_size, + total=total, + total_is_exact=exact_count_available, + has_more=has_more, + ) + cached.value = result + return result @router.post("/search/reindex") diff --git a/src/basic_memory/deps/read_cache.py b/src/basic_memory/deps/read_cache.py index ec81b341e..505391891 100644 --- a/src/basic_memory/deps/read_cache.py +++ b/src/basic_memory/deps/read_cache.py @@ -34,6 +34,7 @@ def create_model_read_cache[ModelT: BaseModel]( read_cache: ReadCache | None, model_type: type[ModelT], *, + ttl_seconds: int = READ_CACHE_TTL_SECONDS, max_payload_bytes: int = READ_CACHE_MAX_PAYLOAD_BYTES, ) -> ModelReadCache[ModelT] | None: """Bind one response model to the host cache and Basic Memory's read policy.""" @@ -42,6 +43,6 @@ def create_model_read_cache[ModelT: BaseModel]( return ModelReadCache( backend=read_cache, model_type=model_type, - ttl_seconds=READ_CACHE_TTL_SECONDS, + ttl_seconds=ttl_seconds, max_payload_bytes=max_payload_bytes, ) diff --git a/src/basic_memory/mcp/clients/search.py b/src/basic_memory/mcp/clients/search.py index 6fd3ab047..0d04fda2c 100644 --- a/src/basic_memory/mcp/clients/search.py +++ b/src/basic_memory/mcp/clients/search.py @@ -60,7 +60,7 @@ async def search( Raises: ToolError: If the request fails """ - from basic_memory.mcp.tools.utils import call_post + from basic_memory.mcp.tools.utils import call_query with logfire.span( "mcp.client.search.search", @@ -69,7 +69,7 @@ async def search( page=page, page_size=page_size, ): - response = await call_post( + response = await call_query( self.http_client, f"{self._base_path}/", json=query, diff --git a/src/basic_memory/mcp/tools/utils.py b/src/basic_memory/mcp/tools/utils.py index 8c5078b26..87a11a7be 100644 --- a/src/basic_memory/mcp/tools/utils.py +++ b/src/basic_memory/mcp/tools/utils.py @@ -656,6 +656,88 @@ async def call_post( raise +async def call_query( + client: AsyncClient, + url: URL | str, + *, + client_name: str | None = None, + operation: str | None = None, + path_template: str | None = None, + content: RequestContent | None = None, + data: RequestData | None = None, + files: RequestFiles | None = None, + json: typing.Any | None = None, + params: QueryParamTypes | None = None, + headers: HeaderTypes | None = None, + cookies: CookieTypes | None = None, + auth: AuthTypes | UseClientDefault = USE_CLIENT_DEFAULT, + follow_redirects: bool | UseClientDefault = USE_CLIENT_DEFAULT, + timeout: TimeoutTypes | UseClientDefault = USE_CLIENT_DEFAULT, + extensions: RequestExtensions | None = None, +) -> Response: + """Make a safe, idempotent QUERY request and handle errors appropriately.""" + logger.debug(f"Calling QUERY '{url}'") + error_message = None + request_span: logfire.LogfireSpan | None = None + + try: + with logfire.span( + "mcp.http.request", + method="QUERY", + client_name=client_name, + operation=operation, + path_template=path_template, + phase="request", + has_query=bool(params), + has_body=any(value is not None for value in (content, data, files, json)), + ) as request_span: + response = await client.request( + "QUERY", + url=url, + content=content, + data=data, + files=files, + json=json, + params=params, + headers=_request_headers(headers), + cookies=cookies, + auth=auth, + follow_redirects=follow_redirects, + timeout=timeout, + extensions=extensions, + ) + request_span.set_attributes(_response_span_attrs(response)) + + if response.is_success: + return response + + status_code = response.status_code + response_data = _extract_response_data(response) + error_message = _resolve_error_message(status_code, url, "QUERY", response_data) + + if 400 <= status_code < 500: + if status_code == 429: # pragma: no cover + logger.warning(f"Rate limit exceeded: QUERY {url}: {error_message}") + else: + logger.info(f"Client error: QUERY {url}: {error_message}") + else: + logger.error(f"Server error: QUERY {url}: {error_message}") + + response.raise_for_status() + return response # pragma: no cover + + except HTTPStatusError as e: + raise ToolError(error_message) from e + except TransportError as e: + if request_span is not None: + request_span.set_attributes(_transport_error_span_attrs(e)) + raise ToolError(_transport_error_message(e, url, "QUERY")) from e + except Exception as e: + if request_span is not None: + request_span.set_attributes(_transport_error_span_attrs(e)) + raise + + async def resolve_entity_id(client: AsyncClient, project_external_id: str, identifier: str) -> str: """Resolve a string identifier to an entity external_id using the v2 API. diff --git a/src/basic_memory/read_cache/contract.py b/src/basic_memory/read_cache/contract.py index 1dc99dad4..ecc1f318f 100644 --- a/src/basic_memory/read_cache/contract.py +++ b/src/basic_memory/read_cache/contract.py @@ -15,6 +15,7 @@ class ReadCacheOperation(StrEnum): entity = "entity" resolve = "resolve" resource = "resource" + search = "search" class ReadCacheStoreStatus(StrEnum): diff --git a/src/basic_memory/read_cache/policy.py b/src/basic_memory/read_cache/policy.py index b269120cf..133c208e3 100644 --- a/src/basic_memory/read_cache/policy.py +++ b/src/basic_memory/read_cache/policy.py @@ -3,3 +3,4 @@ READ_CACHE_TTL_SECONDS = 60 READ_CACHE_MAX_PAYLOAD_BYTES = 1024 * 1024 DIRECTORY_READ_CACHE_MAX_PAYLOAD_BYTES = 2 * 1024 * 1024 +SEARCH_READ_CACHE_TTL_SECONDS = 30 diff --git a/test-int/read_cache/test_search_read_cache.py b/test-int/read_cache/test_search_read_cache.py new file mode 100644 index 000000000..14df48431 --- /dev/null +++ b/test-int/read_cache/test_search_read_cache.py @@ -0,0 +1,151 @@ +"""Real Redis coverage for semantic search response caching.""" + +from typing import Any, Protocol + +import pytest +from fastapi import FastAPI +from httpx import AsyncClient +from redis.asyncio import Redis + +from basic_memory.deps.read_cache import get_read_cache +from basic_memory.deps.services import get_search_service_v2_external +from basic_memory.models import Project +from basic_memory.read_cache.redis import RedisReadCache + + +class RedisCacheHarness(Protocol): + """Structural type for the real Redis fixture.""" + + cache: RedisReadCache + client: Redis + namespace: str + prefix: str + + +pytestmark = pytest.mark.redis + + +class CountingSearchService: + """Authoritative empty search whose calls expose cache behavior.""" + + def __init__(self) -> None: + self.search_calls = 0 + self.count_calls = 0 + + async def search(self, query: Any, *, limit: int, offset: int) -> list[Any]: + del query, limit, offset + self.search_calls += 1 + return [] + + async def count(self, query: Any) -> int: + del query + self.count_calls += 1 + return 0 + + +@pytest.mark.asyncio +async def test_post_and_query_share_canonical_real_redis_entry( + app: FastAPI, + client: AsyncClient, + test_project: Project, + redis_cache: RedisCacheHarness, +) -> None: + """Method aliases and JSON map order resolve to one project-scoped search value.""" + search_service = CountingSearchService() + app.dependency_overrides[get_read_cache] = lambda: redis_cache.cache + app.dependency_overrides[get_search_service_v2_external] = lambda: search_service + + project_external_id = str(test_project.external_id) + search_url = f"/v2/projects/{project_external_id}/search/" + pagination = {"page": 1, "page_size": 10} + post_response = await client.post( + search_url, + params=pagination, + json={ + "text": "canonical cache", + "metadata_filters": { + "status": "open", + "nested": {"z": 2, "a": 1}, + }, + }, + ) + query_response = await client.request( + "QUERY", + search_url, + params=pagination, + json={ + "metadata_filters": { + "nested": {"a": 1, "z": 2}, + "status": "open", + }, + "text": "canonical cache", + }, + ) + + assert post_response.status_code == 200 + assert query_response.status_code == 200 + assert query_response.json() == post_response.json() + assert query_response.headers["Accept-Query"] == "application/json" + assert search_service.search_calls == 1 + assert search_service.count_calls == 1 + + search_keys = [ + key async for key in redis_cache.client.scan_iter(match=f"{redis_cache.prefix}:*:search:*") + ] + assert len(search_keys) == 1 + search_ttl = await redis_cache.client.ttl(search_keys[0]) + assert 0 < search_ttl <= 30 + + await redis_cache.cache.invalidate_project(project_external_id) + refreshed_response = await client.request( + "QUERY", + search_url, + params=pagination, + json={ + "text": "canonical cache", + "metadata_filters": { + "status": "open", + "nested": {"z": 2, "a": 1}, + }, + }, + ) + + assert refreshed_response.status_code == 200 + assert search_service.search_calls == 2 + assert search_service.count_calls == 2 + + +@pytest.mark.asyncio +async def test_search_pagination_uses_distinct_real_redis_entries( + app: FastAPI, + client: AsyncClient, + test_project: Project, + redis_cache: RedisCacheHarness, +) -> None: + """Pagination changes result identity even when the query document is unchanged.""" + search_service = CountingSearchService() + app.dependency_overrides[get_read_cache] = lambda: redis_cache.cache + app.dependency_overrides[get_search_service_v2_external] = lambda: search_service + + search_url = f"/v2/projects/{test_project.external_id}/search/" + first_page = await client.request( + "QUERY", + search_url, + params={"page": 1, "page_size": 10}, + json={"text": "pagination"}, + ) + second_page = await client.request( + "QUERY", + search_url, + params={"page": 2, "page_size": 10}, + json={"text": "pagination"}, + ) + + assert first_page.status_code == 200 + assert second_page.status_code == 200 + assert search_service.search_calls == 2 + assert search_service.count_calls == 2 + search_keys = [ + key async for key in redis_cache.client.scan_iter(match=f"{redis_cache.prefix}:*:search:*") + ] + assert len(search_keys) == 2 diff --git a/tests/api/v2/test_search_router.py b/tests/api/v2/test_search_router.py index 8d885296c..95ea7342c 100644 --- a/tests/api/v2/test_search_router.py +++ b/tests/api/v2/test_search_router.py @@ -72,6 +72,25 @@ async def test_search_entities( assert "page_size" in data +@pytest.mark.asyncio +async def test_query_search_uses_same_contract_and_advertises_media_type( + client: AsyncClient, + app, + v2_project_url: str, +): + """QUERY is canonical while POST remains the documented compatibility route.""" + response = await client.request( + "QUERY", + f"{v2_project_url}/search/", + json={"text": "safe search"}, + ) + + assert response.status_code == 200 + assert response.headers["Accept-Query"] == "application/json" + search_path = app.openapi()["paths"]["/v2/projects/{project_id}/search/"] + assert set(search_path) == {"post"} + + @pytest.mark.asyncio async def test_search_with_pagination( client: AsyncClient, diff --git a/tests/mcp/clients/test_clients.py b/tests/mcp/clients/test_clients.py index e126b1b96..f44651918 100644 --- a/tests/mcp/clients/test_clients.py +++ b/tests/mcp/clients/test_clients.py @@ -241,12 +241,12 @@ async def test_search(self, monkeypatch): "page_size": 10, } - async def mock_call_post(client, url, **kwargs): + async def mock_call_query(client, url, **kwargs): assert "/v2/projects/proj-123/search/" in url assert kwargs.get("params") == {"page": 1, "page_size": 10} return mock_response - monkeypatch.setattr("basic_memory.mcp.tools.utils.call_post", mock_call_post) + monkeypatch.setattr("basic_memory.mcp.tools.utils.call_query", mock_call_query) mock_http = MagicMock() client = SearchClient(mock_http, "proj-123") @@ -276,10 +276,10 @@ async def test_search_infers_unknown_total_for_legacy_semantic_response( "has_more": False, } - async def mock_call_post(client, url, **kwargs): + async def mock_call_query(client, url, **kwargs): return mock_response - monkeypatch.setattr("basic_memory.mcp.tools.utils.call_post", mock_call_post) + monkeypatch.setattr("basic_memory.mcp.tools.utils.call_query", mock_call_query) client = SearchClient(MagicMock(), "proj-123") result = await client.search({"text": "query", "retrieval_mode": retrieval_mode}) @@ -301,10 +301,10 @@ async def test_search_preserves_explicit_total_exactness(self, monkeypatch): "has_more": False, } - async def mock_call_post(client, url, **kwargs): + async def mock_call_query(client, url, **kwargs): return mock_response - monkeypatch.setattr("basic_memory.mcp.tools.utils.call_post", mock_call_post) + monkeypatch.setattr("basic_memory.mcp.tools.utils.call_query", mock_call_query) client = SearchClient(MagicMock(), "proj-123") result = await client.search( diff --git a/tests/mcp/test_client_telemetry.py b/tests/mcp/test_client_telemetry.py index df6a90ddd..53443ab82 100644 --- a/tests/mcp/test_client_telemetry.py +++ b/tests/mcp/test_client_telemetry.py @@ -74,7 +74,7 @@ async def test_search_client_emits_client_and_http_spans(monkeypatch) -> None: monkeypatch.setattr(logfire, "span", fake_span) async def handler(request: httpx.Request) -> httpx.Response: - assert request.method == "POST" + assert request.method == "QUERY" return httpx.Response( 200, json={ @@ -96,7 +96,7 @@ async def handler(request: httpx.Request) -> httpx.Response: "mcp.http.request", ] assert spans[1][1] == { - "method": "POST", + "method": "QUERY", "client_name": "search", "operation": "search", "path_template": "/v2/projects/{project_id}/search/", diff --git a/tests/mcp/test_tool_utils.py b/tests/mcp/test_tool_utils.py index d11d240de..da9dfe8d9 100644 --- a/tests/mcp/test_tool_utils.py +++ b/tests/mcp/test_tool_utils.py @@ -12,6 +12,7 @@ call_get, call_patch, call_post, + call_query, call_put, get_error_message, ) @@ -60,6 +61,11 @@ async def post(self, *args, **kwargs): self.calls.append(("post", args, kwargs)) return self._responses["post"] + async def request(self, method, *args, **kwargs): + normalized_method = method.lower() + self.calls.append((normalized_method, args, kwargs)) + return self._responses[normalized_method] + async def put(self, *args, **kwargs): self.calls.append(("put", args, kwargs)) return self._responses["put"] @@ -120,6 +126,32 @@ async def test_call_post_error(mock_response): assert "Internal server error" in str(exc.value) +@pytest.mark.asyncio +async def test_call_query_sends_json_with_query_method(mock_response): + """QUERY carries the validated search document as request content.""" + client = _Client() + client.set_response("query", mock_response()) + + query = {"text": "cache semantics"} + response = await call_query(_client(client), "http://test.com", json=query) + + assert response.status_code == 200 + method, _args, kwargs = client.calls[0] + assert method == "query" + assert kwargs["json"] == query + + +@pytest.mark.asyncio +@pytest.mark.parametrize("status_code", [400, 500]) +async def test_call_query_error(mock_response, status_code): + """QUERY uses the same friendly client and server errors as other helpers.""" + client = _Client() + client.set_response("query", mock_response(status_code)) + + with pytest.raises(ToolError): + await call_query(_client(client), "http://test.com", json={"text": "query"}) + + @pytest.mark.asyncio async def test_call_put_success(mock_response): """Test successful PUT request.""" @@ -200,6 +232,7 @@ async def test_call_post_adds_workspace_permalink_headers_at_request_time(mock_r _ALL_CALL_HELPERS = [ (call_get, "GET"), (call_post, "POST"), + (call_query, "QUERY"), (call_put, "PUT"), (call_patch, "PATCH"), (call_delete, "DELETE"), From 7796b4c5058c8ddcd714863dd4afab86acffa77d Mon Sep 17 00:00:00 2001 From: phernandez Date: Thu, 30 Jul 2026 17:36:48 -0500 Subject: [PATCH 2/9] docs(api): remove staged cache rollout Signed-off-by: phernandez --- docs/REDIS_READ_CACHE_PLAN.md | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/docs/REDIS_READ_CACHE_PLAN.md b/docs/REDIS_READ_CACHE_PLAN.md index 3207d4803..506924567 100644 --- a/docs/REDIS_READ_CACHE_PLAN.md +++ b/docs/REDIS_READ_CACHE_PLAN.md @@ -61,8 +61,7 @@ invalidates the exact scope populated by the request path. ### Cloud host requirements -Basic Memory Cloud must satisfy all of these requirements before enabling cached reads for a -tenant: +Basic Memory Cloud must satisfy all of these requirements when composing cached reads: 1. Derive one stable namespace from authenticated Cloud context. The preferred value is the tenant UUID or workspace UUID already owned by Cloud. Project UUID alone is not a tenant @@ -142,9 +141,10 @@ tenant: 1. Keep `bm:read:v1` separate from rate-limit and Cloud control-plane prefixes, metrics, timeouts, and failure policies. The clients may target one Redis deployment, but a read-cache timeout must bypass while a rate-limit decision keeps its Cloud-owned security behavior. -1. Enable reads only after request and worker invalidation use the same namespace in the target - environment. Roll out by tenant cohort, watch hit/bypass/invalidation outcomes and database - queries, then remove overlapping Cloud gateway response caches only after parity. +1. Activate request reads and worker invalidation together in the same Cloud release, using the + same namespace function. The cache is fail-open and TTL-bounded, so do not add tenant cohorts, + shadow reads, or a second rollout switch; use integration coverage and cache telemetry to + verify the direct activation. 1. Coordinate rolling deployments around the `bm:read:v1` payload/key contract. Bump the prefix for incompatible serialized response changes so mixed application versions never interpret one another's payloads with different schemas. @@ -520,7 +520,7 @@ semantics themselves are asserted only against the real Redis integration fixtur - Wire project invalidation through accepted writes and indexing paths. - Add full-stack API, repeated `read_note`, and directory refresh integration coverage. -### 3. Cloud rollout +### 3. Cloud integration - Inject a Basic Memory-specific Redis client and tenant namespace. - Derive that namespace from trusted request and worker context with one canonical function. @@ -564,11 +564,10 @@ semantics themselves are asserted only against the real Redis integration fixtur can change while every cache identity remains stable. - Put each startup recovery phase inside a cancellation-safe invalidation scope before beginning the next phase. -- Enable reads for a tenant only after every request, worker, partial-index, direct-index, - read-repair, import, accepted-delete, move, and recovery boundary has namespace and invalidation - parity. -- Start with shadow telemetry or a limited tenant cohort. -- Compare hit rate, Redis latency, database query volume, and end-to-end tool latency. +- Land request reads and the worker invalidation boundaries in one Cloud change, using the same + tenant namespace function everywhere. +- Do not add shadow reads, tenant cohorts, or a cache-specific rollout switch. Observe hit rate, + Redis latency, database query volume, and end-to-end tool latency after direct activation. ### 4. Expand from evidence From a38ae4ae9cf7722622f4c93f34d130214872f1f8 Mon Sep 17 00:00:00 2001 From: phernandez Date: Thu, 30 Jul 2026 18:08:57 -0500 Subject: [PATCH 3/9] docs(api): align cloud redis topology Signed-off-by: phernandez --- docs/REDIS_READ_CACHE_PLAN.md | 39 +++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/docs/REDIS_READ_CACHE_PLAN.md b/docs/REDIS_READ_CACHE_PLAN.md index 506924567..031a50691 100644 --- a/docs/REDIS_READ_CACHE_PLAN.md +++ b/docs/REDIS_READ_CACHE_PLAN.md @@ -13,8 +13,8 @@ Redis instance. requirement. - Make cache semantics, typed serialization, TTLs, and invalidation part of Basic Memory. - Let a host such as Basic Memory Cloud inject an existing Redis client and opaque namespace. -- Preserve independent clients, key prefixes, metrics, and failure behavior for read caching and - rate limiting. +- Preserve independent key prefixes, metrics, ownership, and failure behavior for read caching + and rate limiting. ## Ownership Boundary @@ -36,13 +36,13 @@ Cloud owns: - capacity, eviction, and availability decisions when Redis is shared. Physical topology is deliberately outside the Basic Memory cache contract. Cloud may point the -read cache and limiter at separate Redis instances or at separately configured clients on one -instance. +read cache and limiter at separate Redis instances or clients, or reuse one client and connection +pool when both concerns share an instance. ### Tenant isolation on shared Redis -Cloud does not create a Redis instance or connection pool per tenant. It reuses one long-lived, -Basic Memory-specific Redis client and constructs a lightweight `RedisReadCache` adapter from +Cloud does not create a Redis instance or connection pool per tenant. It reuses one long-lived +Redis client and connection pool and constructs a lightweight `RedisReadCache` adapter from trusted request or worker context: - `namespace`: the stable tenant/workspace UUID; @@ -69,11 +69,13 @@ Basic Memory Cloud must satisfy all of these requirements when composing cached 1. Complete authorization and tenant database/schema selection before cache lookup. The namespace prevents key collisions; it does not replace Cloud's access-control boundary. 1. Override the low-level `get_read_cache` dependency at the Cloud composition root. Construct - `RedisReadCache(client=shared_basic_memory_client, namespace=trusted_namespace)` as a - lightweight request-scoped adapter; reuse the long-lived client and connection pool. - FastAPI route dependencies then create lightweight `ModelReadCache` facades from that shared - backend and bind Basic Memory's response type, TTL, and payload-size policy. Cloud should not - duplicate those policy constants or construct Redis clients per response model. + `RedisReadCache(client=shared_redis_client, namespace=trusted_namespace)` as a lightweight + request-scoped adapter; reuse the long-lived client and connection pool. A single Cloud + composition function owns this choice so the cache can move to a separate client or instance + later without changing Basic Memory routes or workers. FastAPI route dependencies then create + lightweight `ModelReadCache` facades from that shared backend and bind Basic Memory's response + type, TTL, and payload-size policy. Cloud should not duplicate those policy constants or + construct Redis clients per response model. 1. Pass the trusted tenant/workspace identity through internal queue payloads, or include enough trusted identifiers for workers to derive the exact same namespace. Never copy a namespace from a public request field. @@ -233,12 +235,12 @@ Phase one: | Directory tree | 60 seconds | Full hierarchy; two MiB measured payload cap | | Paginated directory listing | 60 seconds | Include path, depth, glob, page, and page-size keys | -Phase two, after measuring phase one: +Additional measured surfaces: | Operation | Initial TTL | Constraints | | --------------------------- | ------------: | ---------------------------------------------- | -| Search | 30 seconds | Canonicalize the complete query and pagination | -| Context and recent activity | 15-30 seconds | Normalize or bound time-relative inputs | +| Search | 30 seconds | Implemented; complete query plus pagination key | +| Context and recent activity | 15-30 seconds | Future; normalize or bound time-relative inputs | Do not initially cache failures, missing entities, graph/orphan responses, large or arbitrary binary resources, schema inference, writes, or Cloud control-plane data. @@ -399,9 +401,10 @@ absorb that authorization-aware composition. Cloud should optimize that active-p reconciliation and its own project-list cache independently. Directory caching in Basic Memory is intended to replace overlapping route families after Cloud -reaches namespace, invalidation, and observability parity. During rollout, the inner cache can -also share tenant-project directory results across already-authorized users while Cloud's current -outer key remains user-specific. Do not keep both response-cache layers as the final design. +reaches namespace, invalidation, and observability parity. During the transition away from the +overlapping outer cache, the inner cache can also share tenant-project directory results across +already-authorized users while Cloud's current outer key remains user-specific. Do not keep both +response-cache layers as the final design. ## Failure Behavior @@ -571,7 +574,7 @@ semantics themselves are asserted only against the real Redis integration fixtur ### 4. Expand from evidence -- Add search and graph-context reads when measured reuse supports them. +- Add graph-context and recent-activity reads when measured reuse supports them. - Refine project-wide invalidation only if unrelated writes materially reduce the entity hit rate. From 17ecb670bccb7bbb4492c6858a19de5c7f7f8077 Mon Sep 17 00:00:00 2001 From: phernandez Date: Thu, 30 Jul 2026 18:26:47 -0500 Subject: [PATCH 4/9] test(api): update search telemetry route call Signed-off-by: phernandez --- tests/api/v2/test_search_router_telemetry.py | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/tests/api/v2/test_search_router_telemetry.py b/tests/api/v2/test_search_router_telemetry.py index f4d2e7e54..a3c33a0f1 100644 --- a/tests/api/v2/test_search_router_telemetry.py +++ b/tests/api/v2/test_search_router_telemetry.py @@ -8,6 +8,7 @@ import logfire import pytest +from fastapi import Response from basic_memory.schemas.search import SearchQuery @@ -37,16 +38,20 @@ async def fake_to_search_results(entity_service, results): monkeypatch.setattr(logfire, "span", fake_span) monkeypatch.setattr(router, "to_search_results", fake_to_search_results) - response = await router.search( - SearchQuery(text="hello world"), - FakeSearchService(), - object(), - project_id=123, + http_response = Response() + search_response = await router.search( + query=SearchQuery(text="hello world"), + search_service=FakeSearchService(), + entity_service=object(), + read_cache=None, + response=http_response, + project_id="11111111-1111-1111-1111-111111111111", page=2, page_size=5, ) - assert response.current_page == 2 + assert search_response.current_page == 2 + assert http_response.headers["Accept-Query"] == "application/json" # The root span fires with these attrs; additional nested spans may fire as well. assert operations[0] == ( "api.request.search", From 67e3d5bb46ce1a972a8afebd7508089e30f2cb90 Mon Sep 17 00:00:00 2001 From: phernandez Date: Sat, 1 Aug 2026 15:12:15 -0500 Subject: [PATCH 5/9] perf(mcp): enable optional Redis read caching Signed-off-by: phernandez --- .gitignore | 1 + benchmarks/docs/read-load-benchmark.md | 38 ++ benchmarks/scripts/read_load_bench.py | 590 +++++++++++++++++++++ docs/REDIS_READ_CACHE_PLAN.md | 20 +- justfile | 24 + src/basic_memory/api/container.py | 18 +- src/basic_memory/config_models.py | 5 + src/basic_memory/mcp/container.py | 3 + src/basic_memory/mcp/server.py | 186 ++++--- src/basic_memory/read_cache/lifecycle.py | 37 ++ test-int/read_cache/test_mcp_read_cache.py | 81 +++ tests/api/test_container.py | 16 +- tests/mcp/test_mcp_container.py | 1 + tests/test_read_cache_lifecycle.py | 31 ++ 14 files changed, 956 insertions(+), 95 deletions(-) create mode 100644 benchmarks/docs/read-load-benchmark.md create mode 100644 benchmarks/scripts/read_load_bench.py create mode 100644 src/basic_memory/read_cache/lifecycle.py create mode 100644 test-int/read_cache/test_mcp_read_cache.py create mode 100644 tests/test_read_cache_lifecycle.py diff --git a/.gitignore b/.gitignore index d7c33a50d..49372175e 100644 --- a/.gitignore +++ b/.gitignore @@ -63,6 +63,7 @@ claude-output .mcpregistry_* /.testmondata .benchmarks/ +/.scratch/ # Consolidated package build artifacts /integrations/openclaw/node_modules/ diff --git a/benchmarks/docs/read-load-benchmark.md b/benchmarks/docs/read-load-benchmark.md new file mode 100644 index 000000000..dd7c5fdf8 --- /dev/null +++ b/benchmarks/docs/read-load-benchmark.md @@ -0,0 +1,38 @@ +# Read-path load benchmark + +This benchmark measures repeated, direct-permalink `read_note` calls through the real +`bm mcp` stdio server. It compares an authoritative warm baseline with the same workload after +the standalone Redis read cache is warmed. + +## Workload + +- deterministic 1, 16, and 64 KiB Markdown notes; +- 32 distinct notes per size; +- 128 measured reads at concurrency 1, 8, 32, and 64; +- corpus materialization, indexing, connection setup, and cache warmup outside measurement; +- isolated Basic Memory config, database, project, and home directories for every run; +- JSONL output with p50, p95, p99, throughput, response bandwidth, errors, and workload metadata. + +The uncached run removes any inherited `BASIC_MEMORY_REDIS_URL`. The cached run sets it only in +the spawned MCP process. Output records whether Redis was enabled without recording the URL, +because URLs may contain credentials. + +## Run a paired comparison + +Use the same Redis server for the cached repetitions, but use a distinct scratch directory for +every run. The server must be ready before starting the benchmark; its startup time is not part +of the measurement. + +```bash +just bench-read-cache redis://127.0.0.1:6379/0 run-01 +``` + +The recipe writes `.scratch/read-load-authoritative-run-01.jsonl` and +`.scratch/read-load-redis-warm-run-01.jsonl`, then prints the Markdown comparison. Give each +repetition a distinct run ID so its workload and JSONL artifacts remain available. Use +`just bench-read-load