feat(client): minimal response-cache substrate (ResponseCacheStore + aggregate-then-write list*())#2336
feat(client): minimal response-cache substrate (ResponseCacheStore + aggregate-then-write list*())#2336felixweinberger wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: eab632c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
felixweinberger
force-pushed
the
fweinberger/response-cache
branch
from
4280d2d to
0e94d53
Compare
…t*() unchanged) ResponseCacheStore / InMemoryResponseCacheStore (new file responseCache.ts) and the internal ClientResponseCache collaborator back the Client's derived views. The cache is populated by an internal aggregating walk (_listAllPages — repeated-cursor guard, listMaxPages cap-throw, captured-generation race guard) reachable via the internal _ensureCachedToolsList; the public listTools / listPrompts / listResources / listResourceTemplates methods keep their per-page contract byte-identical to before. list_changed notifications evict the matching method (no refetch); _resetConnectionState resets the collaborator. ClientResponseCache.toolDefinition(name) is the derived name->Tool view over the cached tools/list entry, memoized against the entry's stamp (mcp.d's cachedTool pattern) — the substrate ships only this seam; the stacked SEP-2243 PR is its consumer. New ClientOptions: responseCacheStore (defaults to a fresh per-instance InMemoryResponseCacheStore — a store MUST NOT be shared across Client instances at all in v2.0.x; entries are keyed by method only) and listMaxPages (cap on the internal walk, default 64; does not affect the public per-page methods).
felixweinberger
force-pushed
the
fweinberger/response-cache
branch
from
0e94d53 to
eab632c
Compare
|
|
||
| /** | ||
| * The response-cache store backing the client's derived views (the cached | ||
| * `tools/list` result that {@linkcode Client.callTool | callTool}'s output | ||
| * validation and SEP-2243 header mirroring read). Defaults to a fresh | ||
| * {@linkcode InMemoryResponseCacheStore} per client. | ||
| * | ||
| * **Do not share one store across clients at all in v2.0.x** — entries | ||
| * are keyed by method + params only, so two clients connected to | ||
| * different servers (even under the same credential) collide on | ||
| * `tools/list`, and one client's `list_changed` evicts every co-tenant's | ||
| * entry. Supply your own only as a single-client backing store. | ||
| * Per-principal partitioning that enables safe sharing is #39. | ||
| */ | ||
| responseCacheStore?: ResponseCacheStore; |
There was a problem hiding this comment.
🟡 The JSDoc on ClientOptions.responseCacheStore (and the _cache field comment) still claims, in present tense, that the cached tools/list entry is what callTool's output validation and SEP-2243 header mirroring read — but in this PR callTool() still validates via the legacy _cachedToolOutputValidators map populated by cacheToolMetadata() from the public listTools(), and the SEP-2243 mirroring is the not-yet-merged stacked PR. The changeset and the responseCache.ts toolDefinition JSDoc were already reworded to future tense; please do the same for these two comments (and drop the "output validators" claim from the _cache field doc, since validators don't live in ClientResponseCache).
Extended reasoning...
What the bug is. Two doc comments in packages/client/src/client/client.ts describe behavior this PR does not ship, in present tense:
- The
ClientOptions.responseCacheStoreJSDoc (~lines 270–283) says the store backs "the cachedtools/listresult thatcallTool's output validation and SEP-2243 header mirroring read." - The
_cachefield comment (~lines 382–390) says theClientResponseCachecollaborator owns "the stamp-memoized derived indices (name → Tool, output validators)".
Why neither claim is true in this diff. callTool() validates structured output via getToolOutputValidator(params.name), which reads this._cachedToolOutputValidators — a map populated by cacheToolMetadata(result.tools) directly from the public listTools() return value, never from the ResponseCacheStore. And no SEP-2243 Mcp-Param-* / x-mcp-header mirroring code exists anywhere in packages/client/src in this PR — that is the stacked #2327. Likewise, ClientResponseCache holds only the name → Tool index (_toolIndex); output validators stay in the legacy _cachedToolOutputValidators map on Client, so "output validators" does not belong in the list of indices the collaborator owns.
Step-by-step proof. (1) Construct a Client with a custom responseCacheStore pre-seeded with a tools/list entry containing a tool whose outputSchema differs from the live server's. (2) Call listTools() — cacheToolMetadata() compiles validators from the server response, not the seeded store entry. (3) Call callTool() — validation runs against _cachedToolOutputValidators; the seeded store entry is never consulted. The store therefore does not "back" what callTool's validation reads, contrary to the option JSDoc. The same divergence shows up after a list_changed: the eviction clears the cache entry but leaves _cachedToolOutputValidators populated, so the two stores demonstrably hold different state.
Why existing fixes don't cover it. The author already responded to the earlier review feedback by rewording the changeset ("will be the source for callTool's output validation and SEP-2243 Mcp-Param-* mirroring once the stacked SEP-2243 PR lands; this commit ships only the seam") and the responseCache.ts toolDefinition JSDoc ("until that lands callTool() still reads its validators from the legacy _cachedToolOutputValidators map"). These two sites in client.ts were missed in that pass and now contradict the corrected prose elsewhere in the same diff.
Impact. Documentation-only, no runtime effect — but responseCacheStore is a new public option, and its JSDoc is the first thing a consumer sees in IDE hover. A user reading it would conclude that supplying a store controls (or at least reflects) what callTool validation uses today, which is false until #2327 lands.
How to fix. Reword the responseCacheStore JSDoc to future tense, mirroring the changeset (e.g. "the cached tools/list result that callTool's output validation and SEP-2243 header mirroring will read once the stacked SEP-2243 PR lands"), and trim the _cache field comment to claim only the name → Tool index (dropping "output validators").
| return acc; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
🟡 A custom ResponseCacheStore whose set() throws or rejects makes _listAllPages / _ensureCachedToolsList reject even though the result was already fully fetched from the server, because the terminal await this._cache.write(...) (and ClientResponseCache.write()'s await this._store.set(...)) has no error handling. This is inconsistent with the eviction path, where a custom store's evict() failure is routed to onerror via _reportStoreError so dispatch proceeds — consider catching the store failure here, reporting via _reportStoreError, and still returning acc. No production caller reaches this path yet (only tests via a private cast), so this is a non-blocking consistency fix worth making while the seam is being established.
Extended reasoning...
What the issue is. _listAllPages finishes its walk with await this._cache.write(method, acc, generation); return acc; (client.ts:1414-1416), and ClientResponseCache.write() does await this._store.set({ method }, { value }) with no try/catch anywhere on the path. _ensureCachedToolsList's initial await this._cache.read('tools/list') is similarly unguarded. The ResponseCacheStore interface is explicitly advertised as async-ready so that "a Redis-style store can implement the same interface" — meaning a set() that rejects on a transient network error is a realistic scenario, not a hypothetical. When that happens, the rejection propagates out of _listAllPages and the caller loses a result that was already fully fetched from the server, purely because of cache bookkeeping.\n\nWhy this looks like an oversight rather than a deliberate choice. The same diff establishes the opposite posture in two places: (a) the eviction path in _onnotification wraps the store call — void this._cache.evict(method).catch(error => this._reportStoreError(error)) — with the comment "Route a custom-store failure to onerror without aborting the surrounding dispatch", and the test suite covers "a custom store whose evict() throws is routed to onerror and dispatch still runs"; and (b) the generation-guard comment inside _listAllPages itself says "the result still returns to the caller — it just is not cached", i.e. the stated design intent is that cache bookkeeping never costs the caller the fetched data. A failing set() does exactly what that comment says should not happen. There is no equivalent handling or test for a failing set()/get().\n\nStep-by-step proof. (1) Construct a Client with responseCacheStore whose set() returns Promise.reject(new Error('redis down')) (a valid implementation under MaybePromise<number>). (2) Call _ensureCachedToolsList() (today via the test seam; after the stacked SEP-2243 PR, via callTool). (3) The cache miss routes into _listAllPages('tools/list', ...), which fetches every page successfully — acc now holds the complete aggregated tool list. (4) The terminal await this._cache.write('tools/list', acc, generation) calls await this._store.set(...), which rejects. (5) The rejection propagates through write() → _listAllPages → _ensureCachedToolsList, and the caller gets Error: redis down instead of the tool list it just paid N round-trips to assemble. Compare: the same store's evict() rejecting during a list_changed notification is caught, reported to onerror, and dispatch continues.\n\nAddressing the refutation. One reviewer argued (1) there is no production caller in this PR and (2) the asymmetry with eviction is justified because eviction has no awaiting caller while the write path does, so fail-loud is a coherent posture the SEP-2243 follow-up can decide. Point (1) is accurate and is why this is filed as a nit rather than a blocking finding — today only tests reach _listAllPages/_ensureCachedToolsList via a private cast, and the public list*() methods never touch the cache. But point (2) is undercut by the PR's own prose: the in-method comment commits to "the result still returns to the caller — it just is not cached" for the analogous mid-walk-eviction case, so the substrate has already chosen fail-open for cache bookkeeping; letting a store rejection discard the fetched aggregate contradicts that choice within the same function. Deciding the posture now, while the seam is being established and before #2327 inherits it as tool-call failures, is cheaper than retrofitting it later.\n\nImpact and fix. Impact today is limited to the internal seam (hence non-blocking); once the stacked SEP-2243 PR wires callTool through _ensureCachedToolsList, a flaky custom store would turn cache-write errors into spurious tool-call failures. The fix is small and matches the existing pattern: wrap the terminal write in a try/catch (or .catch) that routes to this._reportStoreError(...) and still return acc, e.g.\n\nts\ntry {\n await this._cache.write(method, acc, generation);\n} catch (error) {\n this._reportStoreError(error);\n}\nreturn acc;\n\n\nand consider giving _ensureCachedToolsList's read() the same treatment (fall through to the walk on a store get() failure). A test mirroring the existing "custom store whose evict() throws" case — a store whose set() rejects, asserting the aggregate is still returned and onerror fires — would lock the posture in.
1 participant
Adds a minimal response-cache substrate to
Client— the foundation that SEP-2243 header mirroring (#2327) and the future client-sidecacheHintshonoring both build on.Motivation and Context
SEP-2243's client algorithm requires the client to know each tool's
inputSchema(to findx-mcp-headerdeclarations). #2327's first iteration built a SEP-2243-specific tool-definition cache with its own population/invalidation logic; review rounds repeatedly surfaced lifecycle edges (page-1 wipes, concurrent races, listChanged interactions). This PR moves to the right abstraction: a general response cache thatlistTools()/listPrompts()/listResources()write to, and_toolDefinition(name)reads as a stamp-memoized derived view. Lifecycle is solved once at the cache layer.How Has This Been Tested?
Client suite (550), full e2e matrix (2545p/205xf), conformance
client:all. NewresponseCache.test.tscovers store contract, partition keying,listMaxPagescap-throw, generation-guard, and theObject.prototypemethod-name guard.Breaking Changes
listTools()/listPrompts()/listResources()/listResourceTemplates()called without acursornow aggregate all pages internally and return the complete result. Callers that paginated manually should drop their loop. ExplicitlistTools({ cursor })keeps the per-page contract.ClientOptions.listMaxPages(default 64) now throws instead of returning a truncated result.Types of changes
Checklist
Additional context
Public surface added:
ResponseCacheStore(interface,MaybePromise<…>returns),InMemoryResponseCacheStore(default),CacheKey,CacheEntry,CacheScope,ClientOptions.responseCacheStore. The interface is shaped for the eventualcacheHintswork to be additive:set(key, { value, expiresAt?, scope? })carries the freshness slot now, even though nothing populates it yet.Do not share a store across
Clientinstances in v2.0.x — the store is keyed without a server-identity or principal partition, so cross-instance sharing causes server-identity confusion andclear()/evict()cross-talk. Per-principal partitioning that enables safe sharing is the follow-up work.Lifecycle model:
list_changednotifications evict the matching entry (they do not eagerly refetch). A per-method generation counter prevents an in-flight aggregation from caching a stale result after an eviction has fired._resetConnectionStateclears only the per-instance default store, never a user-supplied one.