Repository navigation
perf(core): per-note read-cache eviction groups on fastapi-redis-sdk, QUERY-only search and resolve - #1752
Conversation
Replace the per-route read-through cache and its single project generation with fastapi-redis-sdk route caching keyed by versioned eviction groups (one per note, one for content, a project epoch, and a global epoch for resets). Writers record exactly what they changed, so a body edit retires that note and content-wide reads only. Search, resolve and links/resolve are QUERY only. Adds bm cache reset. WIP: older read-cache test suites still target the removed API. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016N9c6ouiT4Wid3arGR41Dv Signed-off-by: phernandez <paul@basicmachines.co>
Rewrite the read-cache suites against the fastapi-redis-sdk design: version tokens per note, content and project epoch instead of one project generation, and route caching observed through X-Redis-Cache. - test_redis_read_cache: version store (scoped vs whole-project bumps, tenant/project isolation, reset_all, TTLs, recorder scope, shielded cancellation, Redis unavailable and capacity failures). - test_cache_rules: add slice-per-entry and one-span-per-read rules. - API, runtime, materialization, MCP, search and benchmark suites read versions instead of generation keys; body edits assert per-note scope; the benchmark asserts a hit runs zero SQL. - Resources are no longer cached; their tests assert that. - Search, resolve and links/resolve calls in tests use QUERY. - Fakes implement invalidate(project_id, change) and return RelationPublishOutcome. src: RelationGenerationPublisher validates relations before the pre-publication target read (fail before I/O). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016N9c6ouiT4Wid3arGR41Dv Signed-off-by: phernandez <paul@basicmachines.co>
ResolveRelationsResult carries the source entity ids a pass resolved. resolved_relations_read_cache_change maps them to the notes whose response changed (the sources, and resolved targets rendered from their entity row), so the local resolver and hosted resolve jobs stop clearing the whole project. A pass that resolves nothing retires nothing. Drop POST /v2/search/; scoped search is QUERY only, like project search and resolve. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016N9c6ouiT4Wid3arGR41Dv Signed-off-by: phernandez <paul@basicmachines.co>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4782c12e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cached_read( | ||
| ReadCacheOperation.search, | ||
| group=content_group, | ||
| ttl_seconds=SEARCH_READ_CACHE_TTL_SECONDS, | ||
| ) |
There was a problem hiding this comment.
When Redis caching is enabled, the second equivalent QUERY request hits this dependency and raises CacheHitException before the endpoint executes, so response.headers["Accept-Query"] = "application/json" at line 112 never runs. Since fastapi-redis-sdk 0.8.1 does not replay the original response headers, cache hits violate the search response contract and omit Accept-Query; the integration test currently checks this header only on the initial miss. Add the header in the cache-hit response path or avoid caching this route until headers can be preserved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3db3d40. cached_read now takes hit_headers, which the hit path adds to every cached response (the route never runs on a hit and SDK 0.8.1 doesn't replay stored headers). Search passes Accept-Query: application/json. test_search_read_cache now asserts the header on the HIT response too; it fails with the header line removed and passes with it.
phernandez
changed the title
A hit returns before the search route runs, and fastapi-redis-sdk 0.8.1 does not replay stored headers, so hits dropped Accept-Query. cached_read takes hit_headers and adds them to every cached response; search passes Accept-Query. The search cache test now checks the hit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016N9c6ouiT4Wid3arGR41Dv Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3db3d40134
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try: | ||
| # Raises the SDK's CacheHitException on a hit; its handler turns | ||
| # it into the cached response before the endpoint runs. | ||
| await anext(steps) |
There was a problem hiding this comment.
When Redis caching is enabled, warm an entity response and then delete its non-default project with delete_notes=true; this cache-hit path raises before ProjectExternalIdPathDep can verify that the project still exists, while delete_project_by_id removes the project without advancing its cache epoch. The same entity URL can therefore continue returning the deleted note content for the seven-day read TTL instead of returning 404. Project deletion must invalidate the project epoch, or project existence and access must be checked before serving a hit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e711c6f. DELETE /v2/projects/{id} now retires the project epoch around remove_project (also when removal fails partway, since what it committed is unknown). New rule test test_deleting_a_project_retires_its_cached_reads warms a note in a second project, deletes the project, and asserts the same URL returns 404 and not a HIT; it fails with the invalidation removed.
| query = "&".join( | ||
| f"{name}={value}" for name, value in sorted(request.query_params.multi_items()) | ||
| ) |
There was a problem hiding this comment.
Flattening decoded query pairs without escaping creates cache-key collisions: /directory/list?dir_name=foo%26page%3D2 and /directory/list?dir_name=foo&page=2 both produce dir_name=foo&page=2, although one lists the literal directory foo&page=2 on page 1 and the other lists foo on page 2. Whichever response is cached first is then served for the other request. Including every query parameter also lets ignored parameters such as unique nonce values create unlimited duplicate entries retained for a week; serialize the declared response-shaping parameters structurally rather than concatenating all pairs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e711c6f. The key now includes only the matched route's declared query parameters (from FastAPI's flattened dependant, so it can't drift from the signature), serialized as JSON together with the path, canonical QUERY body and workspace context, so an encoded & can't collide with a real parameter and undeclared parameters like a nonce don't split entries. New rule test test_query_values_are_keyed_structurally_and_only_when_declared covers both; it fails with the old concatenation.
A hit is served before a route checks that its project exists, so deleting a project must retire its cached reads: the delete route now invalidates the project epoch, also when removal fails partway. Cache keys took every query pair and joined them unescaped, so an encoded & inside a value collided with a real second parameter, and undeclared parameters (a nonce) split entries. Keys now hold only the route's declared query parameters, serialized as JSON with the path, QUERY body and workspace context. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016N9c6ouiT4Wid3arGR41Dv Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e711c6f10c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
|
|
||
| sdk_dependency = cache(ttl=ttl_seconds, key_builder=key_builder, cache_prefix="") | ||
|
|
There was a problem hiding this comment.
When an entity, search result, or directory tree is large, this SDK dependency stores the entire serialized response because only a TTL is configured. This removes the previous 1 MiB general and 2 MiB directory payload limits; with the new one-to-seven-day TTLs, repeated slice/search variants or content invalidations can retain many oversized entries and exhaust Redis memory or evict useful hot entries. Add an equivalent capture-side size check or bypass caching for oversized responses.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 775c6f5 / adb463c: cached responses are capped at 2 MiB (READ_CACHE_MAX_PAYLOAD_BYTES). fastapi-redis-sdk reads MAX_CACHEABLE_BODY_SIZE from redis_fastapi.cache on every capture; the app now sets it at startup. Larger responses are still served, just not stored. New rule test test_oversized_responses_are_served_but_not_stored asserts the configured cap and, with a lowered cap, that an oversized note read never becomes a HIT. It fails with the setting removed.
Larger responses are served but not stored. fastapi-redis-sdk reads MAX_CACHEABLE_BODY_SIZE (10 MiB default) from redis_fastapi.cache on every capture; the package's re-exported cache() function shadows that submodule on attribute access, so the module is fetched with importlib. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016N9c6ouiT4Wid3arGR41Dv Signed-off-by: phernandez <paul@basicmachines.co>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016N9c6ouiT4Wid3arGR41Dv Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adb463cda2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| invalidate_recorded_change( | ||
| self.read_cache, | ||
| self.project.project_external_id, | ||
| ) |
There was a problem hiding this comment.
On the local watcher path, every processed file has already replaced the entire project epoch at lines 180–184 before this new per-note relation invalidation runs. The runtime factory also wraps the file indexer in InvalidatingIndexFileExecutor, which has already retired the precise FileIndexResult.read_cache_change; consequently, every ordinary watched body edit still evicts unrelated notes, searches, resolves, and listings, defeating the per-note eviction design and its expected hit-rate improvement. Remove the broad completion invalidation and retain the narrow index and relation invalidations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in e0e894f. The watcher's index_file_completed no longer clears the project; InvalidatingIndexFileExecutor already retires the file's precise change, and the relation pass retires only the notes it resolved. New test test_watcher_index_completion_leaves_unrelated_reads_cached asserts the project epoch is unchanged after a processed watched edit, and fails with the old clear restored. The cancelled-completion test now covers delete only (still whole project); the cancelled index commit stays covered by test_cancelled_watcher_index_commit_finishes_real_redis_invalidation.
The watcher's file indexer is wrapped in InvalidatingIndexFileExecutor, which already retires exactly what the file changed. The extra project-wide clear on completion evicted every unrelated cached read on each watched body edit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016N9c6ouiT4Wid3arGR41Dv Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0e894f17e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| invalidation_scope = ( | ||
| invalidate_cache(read_cache, old_project.external_id) | ||
| if read_cache is not None | ||
| else nullcontext() |
There was a problem hiding this comment.
When a configured standalone cache contains warm project reads, bm project remove --local executes this route through the CLI's lifespan-less ASGI app, whose fresh ApiContainer always has read_cache=None; consequently this invalidation scope is skipped. A running MCP process can then keep serving the deleted notes because cache hits short-circuit project validation, and even restarting does not repair it because _invalidate_persisted_read_cache() enumerates only active projects, excluding the deleted ID. The earlier route-level invalidation therefore does not cover this separate deletion entrypoint; connect local CLI mutations to the configured invalidator or reset/tombstone deleted-project epochs.
AGENTS.md reference: AGENTS.md:L157-L161
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4360615, both halves:
- CLI deletes (and every other CLI write) now reach the cache.
_prepared_local_asgi_database(the shared, ref-counted local ASGI setup) opens the configured standalone Redis cache and installs a container when no MCP/API host has installed one. The last client to exit closes it with the database state.test_cli_project_delete_retires_the_standalone_cachecreates and deletes a project through_asgi_clientand asserts the project epoch the MCP server reads has moved; it fails with'0.0' != '0.0'without the change. - Restart retires deleted projects. MCP startup now calls
reset_all()(global epoch, in every key) instead of looping over active projects.test_mcp_startup_retires_entries_of_deleted_projectscovers it.
…s deleted projects A plain CLI process (bm project remove --local, bm tool writes) runs the API through local ASGI without a lifespan, so its container had no cache and its mutations retired nothing. A running MCP server, which serves hits before project validation, then kept answering reads for a deleted project. - The shared local ASGI preparation opens the configured standalone cache and installs a container when no MCP or API host has one; the last client to exit closes it with the database state. - MCP startup advances the global epoch instead of retiring only active projects, so entries of projects deleted while no cache was open are retired. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016N9c6ouiT4Wid3arGR41Dv Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4360615bb7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # The file indexer is wrapped in InvalidatingIndexFileExecutor, which already | ||
| # retired exactly what this file changed; a project-wide clear here would | ||
| # evict every unrelated note on each watched edit. |
There was a problem hiding this comment.
When a watched file is indexed with embeddings enabled, the index executor retires the content group before index_file_completed, but this callback subsequently awaits sync_entity_vectors_batch without another invalidation. A concurrent VECTOR/HYBRID search can therefore fill the new content generation from the old or partially updated vector index, and that result remains reachable for the 24-hour search TTL after embedding completes. The fresh evidence beyond the earlier watcher finding is the retained vector sync later in this callback after the broad completion invalidation was removed; retire the content group after this sync, as LocalEntityVectorSyncScheduler already does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c3f1fa5. The watcher now retires content reads after sync_entity_vectors_batch (whole project if it fails). While here, the API-path LocalEntityVectorSyncScheduler had the opposite problem: it cleared the whole project after every write's embedding. Both now record ReadCacheChange.search_index() (content group only), since embeddings change VECTOR/HYBRID results, not note responses. The watcher test gains an embeddings case asserting content moves and the project epoch does not; the scheduler unit test asserts the recorded change.
| body = None | ||
| if request.method == "QUERY": | ||
| raw = await request.body() | ||
| if raw: | ||
| body = json.loads(raw) |
There was a problem hiding this comment.
Although the follow-up now filters undeclared query parameters, the QUERY body is still keyed from raw json.loads output. SearchQuery and EntityResolveRequest use Pydantic's default extra-field behavior, so bodies such as {"text":"x","nonce":"1"} and {"text":"x","nonce":"2"} execute the same search but create distinct retained entries; repeated ignored fields can therefore duplicate a large response until the 24-hour or seven-day TTL and exhaust Redis. Parse or project the body onto the route's declared body model before constructing the identity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c3f1fa5. _request_identity validates a QUERY body against the route's single body model (TypeAdapter, cached per model) and keys the JSON dump, so ignored fields, key order and spelled-out defaults share one entry; an invalid body bypasses the cache and gets the route's 422. test_query_bodies_are_keyed_by_their_validated_model sends two searches differing only by a nonce and order (second is a HIT) and fails without the change.
…r model - Vector publication (API write path scheduler and watcher) retires content reads only: embeddings change VECTOR/HYBRID results, not note responses. The scheduler cleared the whole project after every write's embedding, and the watcher did not retire anything after embedding. - A QUERY body is validated against the route's body model before keying, so ignored fields, key order and spelled-out defaults share one entry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016N9c6ouiT4Wid3arGR41Dv Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3f1fa519e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| request_digest=read_cache_request_digest(target.scope.user_key, target.identity), | ||
| ) | ||
|
|
||
| sdk_dependency = cache(ttl=ttl_seconds, key_builder=key_builder, cache_prefix="") |
There was a problem hiding this comment.
When a browser or intermediary receives a cached GET /knowledge/entities/{id} or directory response, this SDK call emits Cache-Control: max-age=<ttl> by default—up to seven days here, as documented in the SDK caching guide. A later note mutation only advances Redis versions; it cannot invalidate the browser's still-fresh response, so the request never reaches this dependency and users can continue seeing old canonical note content for the full TTL. Override the emitted policy to require revalidation (and mark authenticated responses private) while retaining the long server-side Redis TTL.
AGENTS.md reference: AGENTS.md:L157-L161
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining. No current consumer of these responses keeps an HTTP cache: the CLI and MCP use httpx (no HTTP cache), Cloud's web app goes through its own BFF endpoints that build fresh responses and don't forward core's caching headers, and raw /proxy and /bmc API-key clients are scripts and agents on httpx/requests. Fly's proxy doesn't cache and there is no CDN caching on API responses. If a browser or caching intermediary is ever put in front of the raw gateway, Cache-Control will be set at that edge.
| pipe.set( | ||
| self._version_key(project_id, group), | ||
| uuid4().hex, | ||
| ex=READ_CACHE_TTL_SECONDS, |
There was a problem hiding this comment.
When a seven-day read obtains the initial 0 token immediately before an invalidation, reads the old note, and finishes after the invalidation, the SDK stores that response under 0 later than this replacement token was created. Exactly seven days after invalidation this version key expires back to 0, while the racing response remains alive until seven days after its later store, temporarily making stale canonical content reachable again. The version must not revert to a predecessor while any response captured under that predecessor can still exist.
AGENTS.md reference: AGENTS.md:L157-L161
Useful? React with 👍 / 👎.
The CLI project-delete cache test failed only in CI: a container left installed by an earlier test made the local ASGI client look like it ran inside a host, so it did not open the standalone cache. An autouse fixture clears the API container global per test, and the test asserts it starts without a host container. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016N9c6ouiT4Wid3arGR41Dv Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df5ea41095
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| database_state = await resources.enter_async_context( | ||
| _resolve_local_asgi_database(app) | ||
| ) | ||
| await _open_standalone_read_cache(resources) |
There was a problem hiding this comment.
When a separate CLI process first opens after a project has been removed from local config, _resolve_local_asgi_database() runs reconcile_projects_with_config_once() and ProjectService.synchronize_projects() deletes the DB-only project before _open_standalone_read_cache() is installed. No epoch is advanced for the deleted external ID, so a concurrently running MCP server can continue serving a warmed entity response—now short-circuited before project validation—for up to seven days. Unlike the fixed explicit project-delete path, this reconciliation deletion remains untracked; have reconciliation return and retire removed project IDs, or reset the cache when it removes a project.
AGENTS.md reference: AGENTS.md:L157-L161
Useful? React with 👍 / 👎.
1 participant
Closes #1747. Builds the read cache the way #1747 proposes, on fastapi-redis-sdk.
Why
In production the read cache almost never hit: notes 1.4%, search 0.1%, resolve 22%.
What changed
One cache layer, before routing. Cached routes use a
cached_read(...)route dependency on top of fastapi-redis-sdk, pinned at 0.8.1.X-Redis-Cacheheader.Versioned eviction groups instead of a project-wide generation. Each key embeds the current version token of its group, plus the project epoch and a global epoch.
GET knowledge/entities/{id}directory/tree,directory/list,memory/recentknowledge/resolve,directory/structureWriters record exactly what they changed (
ReadCacheChange,invalidate_recorded_change).FileIndexResult(identity change → project; otherwise note + content)Keys include the user (supplied by the host through
app.state.read_cache_scope_resolver), the workspace context, the path and query, and the canonical QUERY body.TTLs only bound memory now: 7 days for notes, resolve and listings; 24 h for search; 15 min for recent activity, which is relative to now.
QUERY only. Project search, scoped search,
knowledge/resolveandknowledge/links/resolvedrop their POST routes, with no compatibility shim. Core clients use QUERY.bm cache resetreplaces the global epoch, for after an upgrade that changes responses.Notes for review
redisat import time and route dependencies are declared at import time. redis-py is pure Python.application/jsonwith no response headers; header replay and the RFC 9111 store rules are on itsmainbut unreleased. Resources can be cached again after the next SDK release.Testing
test-int/read_cache/test_cache_rules.pycovers the rules end to end through the API against real Redis, using theX-Redis-Cacheheader:Load-bearing check: forcing body edits to clear the whole project makes the per-note test fail.
The older read-cache suites were migrated to version assertions (
just test-read-cache: 69 passed). The benchmark now asserts a hit runs zero SQL.New real-database test for
note_ids_touched_by_resolution.Targeted unit and API runs pass.
just fast-checkis clean apart from the pre-existingpymilvusimport errors (the optional extra isn't installed locally).🤖 Generated with Claude Code
https://claude.ai/code/session_016N9c6ouiT4Wid3arGR41Dv