From 0452138186d7d510b7327c7a2aeaf3b3b8ec3f82 Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Thu, 9 Jul 2026 10:04:45 +0000 Subject: [PATCH 1/2] refactor(client): store cached results as serialized documents; drop structuredClone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The response cache held live object graphs and isolated them from caller mutation with structuredClone at both edges. That primitive is the wrong contract for this seam: in environments without the global (jest+jsdom, Node < 17, some workers) every cache write threw into the deliberate store-error swallow — caching and output-schema lookups silently turned off for the whole session — and its extra type fidelity (Dates, Maps surviving) was a promise only the in-memory store could keep, since a persistent store must serialize anyway. Store the JSON-serialized document instead: serialize on write, parse on read (mcp.d's toJson/fromJson boundary, which this cache is modeled on). Every hit hands the caller a freshly parsed value it owns outright; in-memory and Redis-style stores behave identically; a value that is not JSON-serializable fails the write loudly to the error sink; a corrupted document in an external store reads as a reported miss, memoized per stamp so it costs one parse + report per rewrite, not per lookup. CacheEntry.value and the store set() entry are now typed string. The list aggregates' nextCursor is now deleted rather than set to undefined, keeping property presence identical between a network response and a cache hit through the codec. --- .changeset/response-cache-document-codec.md | 7 + docs/clients/caching.md | 2 +- examples/caching/README.md | 5 +- packages/client/src/client/client.ts | 31 ++-- packages/client/src/client/responseCache.ts | 107 ++++++++++--- .../test/client/mcpParamMirroring.test.ts | 24 +-- .../client/test/client/responseCache.test.ts | 71 +++++---- .../test/client/responseCacheCodec.test.ts | 150 ++++++++++++++++++ 8 files changed, 311 insertions(+), 86 deletions(-) create mode 100644 .changeset/response-cache-document-codec.md create mode 100644 packages/client/test/client/responseCacheCodec.test.ts diff --git a/.changeset/response-cache-document-codec.md b/.changeset/response-cache-document-codec.md new file mode 100644 index 0000000000..0b97272336 --- /dev/null +++ b/.changeset/response-cache-document-codec.md @@ -0,0 +1,7 @@ +--- +'@modelcontextprotocol/client': minor +--- + +The response cache now stores results as their JSON-serialized documents (serialize on write, parse on read) instead of holding live object graphs isolated with `structuredClone`. Every cache hit hands the caller a freshly parsed value it owns outright — same mutation isolation as before, without depending on the `structuredClone` global (whose absence in jest+jsdom and Node < 17 previously made every cache write throw into the store-error swallow, silently disabling caching and output-schema lookups for the whole session). In-memory and persistent stores now behave identically: `CacheEntry.value` is a string a Redis-style store persists verbatim. A value that is not JSON-serializable (only reachable via in-process transports handing over non-wire objects) now fails the write loudly to the error sink instead of silently, and a corrupted document in an external store reads as a reported miss instead of an uncaught throw. + +Migration for custom `ResponseCacheStore` implementations: `CacheEntry.value` (and the `set()` entry) is now `string`. A store that serialized on `set` and parsed on `get` must stop parsing — return the stored string verbatim. A store that inspected `entry.value` as an object must `JSON.parse` it. Entries persisted by a previous SDK version fail decode once (reported miss) and self-heal on the next write. diff --git a/docs/clients/caching.md b/docs/clients/caching.md index 7616255742..6c495d9416 100644 --- a/docs/clients/caching.md +++ b/docs/clients/caching.md @@ -64,7 +64,7 @@ const store = new InMemoryResponseCacheStore({ maxEntries: 2048 }); const client = new Client({ name: 'my-client', version: '1.0.0' }, { responseCacheStore: store }); ``` -Every method on the `ResponseCacheStore` interface may return a promise, so a Redis-style store implements the same five methods. Entries are keyed by connected-server identity, so one store can back many clients: connections to different servers never collide. +Every method on the `ResponseCacheStore` interface may return a promise, so a Redis-style store implements the same five methods. `CacheEntry.value` is the JSON-serialized result document — a persistent store persists the string verbatim, no extra serialization step, and behaves identically to the in-memory default. Entries are keyed by connected-server identity, so one store can back many clients: connections to different servers never collide. ## Partition the store per user diff --git a/examples/caching/README.md b/examples/caching/README.md index 840602a815..f3715344d9 100644 --- a/examples/caching/README.md +++ b/examples/caching/README.md @@ -32,8 +32,9 @@ class MyStore implements ResponseCacheStore { async get(key: CacheKey): Promise { /* read {value, stamp, expiresAt, scope} from your backend */ } - async set(key: CacheKey, entry: { value: unknown; expiresAt?: number; scope?: CacheScope }): Promise { - /* write entry under key; return a monotonically-increasing stamp */ + async set(key: CacheKey, entry: { value: string; expiresAt?: number; scope?: CacheScope }): Promise { + /* write entry under key (value is the JSON-serialized result document — + persist the string verbatim); return a monotonically-increasing stamp */ } async delete(key: CacheKey): Promise { /* drop the single entry under key (no-op if absent) */ diff --git a/packages/client/src/client/client.ts b/packages/client/src/client/client.ts index 5028e937cd..fd9bf757e7 100644 --- a/packages/client/src/client/client.ts +++ b/packages/client/src/client/client.ts @@ -1642,7 +1642,11 @@ export class Client extends Protocol { cursor = page.nextCursor; pages++; } - acc.nextCursor = undefined; + // delete (not `= undefined`): the aggregate round-trips through the + // cache's JSON codec, which drops explicit-undefined properties — a + // deleted key keeps `'nextCursor' in result` identical between the + // network response and a cache hit. + delete acc.nextCursor; finalize?.(acc); if (bypass) return acc; // The aggregate is ALWAYS written: even when the resolved TTL is ≤0 @@ -1681,12 +1685,15 @@ export class Client extends Protocol { /** * The cache-serving front of every cacheable verb (mcp.d's `cachedFetch` - * read half): under `cacheMode: 'use'` (the default), a held entry whose - * `expiresAt` is in the future is returned as a `structuredClone` and the - * round trip is skipped. `'refresh'` and `'bypass'` always fetch (the - * caller decides whether to write). A custom store whose `get()` rejects - * is routed to `onerror` and treated as a miss — cache bookkeeping never - * blocks a request from reaching the wire. + * read half): under `cacheMode: 'use'` (the default), a fresh held entry + * is served and the round trip is skipped. `'refresh'` and `'bypass'` + * always fetch (the caller decides whether to write). Freshness and + * decoding live in {@linkcode ClientResponseCache.read}: the served value + * is freshly parsed from the stored document, so the caller owns it + * outright — mutating it (e.g. `result.tools.sort(...)`) cannot reach the + * cache or the stamp-memoized indices derived from it. A custom store + * whose `get()` rejects is routed to `onerror` and treated as a miss — + * cache bookkeeping never blocks a request from reaching the wire. */ private async _serveFromCache( method: string, @@ -1694,8 +1701,8 @@ export class Client extends Protocol { options: CacheableRequestOptions | undefined ): Promise { if (options?.cacheMode === 'bypass' || options?.cacheMode === 'refresh') return undefined; - const entry = await this._cache.read(method, params).catch(error => void this._reportStoreError(error)); - if (entry?.expiresAt !== undefined && entry.expiresAt > this._cache.now()) { + const hit = await this._cache.read(method, params).catch(error => void this._reportStoreError(error)); + if (hit !== undefined) { // A pre-aborted caller signal must reject the same way it would on // the wire path (`Protocol.request()` wraps an already-aborted // signal as `SdkError(RequestTimeout, reason)`); without this guard @@ -1705,11 +1712,7 @@ export class Client extends Protocol { const reason = options.signal.reason; throw reason instanceof SdkError ? reason : new SdkError(SdkErrorCode.RequestTimeout, String(reason)); } - // Clone on the way out so a caller mutating the returned aggregate - // (e.g. `result.tools.sort(...)`) cannot reach the cache or the - // stamp-memoized indices derived from it — the same invariant - // `_cache.write` upholds on the way in. - return structuredClone(entry.value) as R; + return hit.value as R; } return undefined; } diff --git a/packages/client/src/client/responseCache.ts b/packages/client/src/client/responseCache.ts index 171d4cfbc7..78504c85b2 100644 --- a/packages/client/src/client/responseCache.ts +++ b/packages/client/src/client/responseCache.ts @@ -52,16 +52,21 @@ export interface CacheKey { } /** - * One cached response body. `value` is the verbatim decoded result; `stamp` is - * the store-generated monotonically increasing write counter — opaque to - * callers. Derived views (e.g. a `name → Tool` index) memoize against it and - * re-derive only when it changes. `expiresAt` (absolute ms epoch, `now + - * ttlMs`) and `scope` are the client-computed freshness metadata; the store - * MUST persist them and hand them back on `get` so the read path can decide - * freshness and gate the shared-partition fallback on `scope === 'public'`. + * One cached response body. `value` is the JSON-serialized result document + * (the cache serializes on write and parses on read — mcp.d's + * `toJson`/`fromJson` boundary), so a store is a dumb string carrier: an + * in-memory store holds the string, a persistent store (Redis, disk) persists + * it verbatim with no extra serialization step, and both behave identically. + * `stamp` is the store-generated monotonically increasing write counter — + * opaque to callers. Derived views (e.g. a `name → Tool` index) memoize + * against it and re-derive only when it changes. `expiresAt` (absolute ms + * epoch, `now + ttlMs`) and `scope` are the client-computed freshness + * metadata; the store MUST persist them and hand them back on `get` so the + * read path can decide freshness and gate the shared-partition fallback on + * `scope === 'public'`. */ export interface CacheEntry { - readonly value: unknown; + readonly value: string; readonly stamp: number; readonly expiresAt?: number; readonly scope?: CacheScope; @@ -96,7 +101,7 @@ export interface ResponseCacheStore { * `scope` (the client-computed freshness metadata); the store MUST persist * them and hand them back on `get`. */ - set(key: CacheKey, entry: { value: unknown; expiresAt?: number; scope?: CacheScope }): MaybePromise; + set(key: CacheKey, entry: { value: string; expiresAt?: number; scope?: CacheScope }): MaybePromise; /** * Drop the single entry under `key` (no-op if absent). Called for both * `notifications/resources/updated` (per-URI) and the `list_changed` @@ -175,7 +180,7 @@ export class InMemoryResponseCacheStore implements ResponseCacheStore { return this._entries.get(keyOf(key)); } - set(key: CacheKey, entry: { value: unknown; expiresAt?: number; scope?: CacheScope }): number { + set(key: CacheKey, entry: { value: string; expiresAt?: number; scope?: CacheScope }): number { const k = keyOf(key); const exempt = CAP_EXEMPT_METHODS.has(key.method); const isNew = !this._entries.has(k); @@ -487,13 +492,17 @@ export class ClientResponseCache { * overwriting the eviction with the stale aggregate would lose the * invalidation. * - * The stored value is a `structuredClone` of `value`, so a caller + * The value is stored as its JSON-serialized document (mcp.d's `toJson` + * half): serialization is simultaneously the mutation barrier — a caller * mutating the aggregate it was returned (e.g. `result.tools.sort(...)`) - * cannot reach the cache or the stamp-memoized indices derived from it. A - * custom store whose `set()` throws or rejects is routed to the - * `reportError` sink and the write resolves — cache bookkeeping never - * costs the caller a result it already fetched (consistent with the - * eviction path). + * cannot reach the cache or the stamp-memoized indices derived from it — + * and the exact representation a persistent store needs. A value that is + * not JSON-serializable (only reachable via in-process transports handing + * over non-wire objects) fails the write loudly into the `reportError` + * sink rather than silently disabling the cache. A custom store whose + * `set()` throws or rejects is routed to the same sink and the write + * resolves — cache bookkeeping never costs the caller a result it + * already fetched (consistent with the eviction path). * * `freshness` carries the client-computed `expiresAt` (absolute ms epoch, * `now + ttlMs`) and the server-reported `cacheScope`. The storage @@ -529,7 +538,7 @@ export class ClientResponseCache { try { await this._store.set( { method, params, partition }, - { value: structuredClone(value), expiresAt: freshness?.expiresAt, scope: freshness?.scope } + { value: encodeCacheValue(value), expiresAt: freshness?.expiresAt, scope: freshness?.scope } ); } catch (error) { this._reportError(error); @@ -548,13 +557,25 @@ export class ClientResponseCache { } /** - * Read the cached entry for `{method, params}` via the two-probe lookup - * (own-partition then this server's shared partition, gated on - * `scope === 'public'`). The caller owns the freshness check - * (`entry.expiresAt > now()`); a missing `expiresAt` is never fresh. + * Serve the fresh cached result for `{method, params}`, or `undefined`. + * Lookup is the two-probe order (own-partition then this server's shared + * partition, gated on `scope === 'public'`); freshness is + * `entry.expiresAt > now()` (a missing `expiresAt` is never fresh — the + * retain-for-schema posture), checked BEFORE decoding so stale entries + * cost no parse. The returned value is freshly parsed from the stored + * document (mcp.d's `fromJson` half), so every hit hands the caller an + * aggregate it owns outright. An entry whose document does not parse + * (a corrupted external store) is reported and treated as a miss. */ - async read(method: string, params?: string): Promise { - return this._probe(method, params); + async read(method: string, params?: string): Promise<{ value: unknown } | undefined> { + const entry = await this._probe(method, params); + if (entry?.expiresAt === undefined || entry.expiresAt <= this.now()) return undefined; + try { + return { value: JSON.parse(entry.value) }; + } catch (error) { + this._reportError(error); + return undefined; + } } /** @@ -592,8 +613,12 @@ export class ClientResponseCache { return undefined; } if (this._toolIndex?.stamp !== entry.stamp) { + const list = this._decodeListTools(entry); const byName = new Map(); - for (const tool of (entry.value as ListToolsResult).tools) byName.set(tool.name, tool); + if (list !== undefined) for (const tool of list.tools) byName.set(tool.name, tool); + // A failed decode memoizes an EMPTY index under the same stamp, so + // the corrupt document is parsed (and reported) once per stamp — + // not once per callTool — and a later rewrite re-derives. this._toolIndex = { stamp: entry.stamp, byName }; } return this._toolIndex.byName.get(name); @@ -628,8 +653,9 @@ export class ClientResponseCache { return undefined; } if (this._toolOutputValidatorIndex?.stamp !== entry.stamp) { + const list = this._decodeListTools(entry) ?? { tools: [] }; const byName = new Map(); - for (const tool of (entry.value as ListToolsResult).tools) { + for (const tool of list.tools) { const compiled = compile(tool); if (compiled !== undefined) byName.set(tool.name, compiled); } @@ -637,4 +663,35 @@ export class ClientResponseCache { } return this._toolOutputValidatorIndex.byName.get(name) as V | undefined; } + + /** Parse a held `tools/list` document for the index builders; a document + * that does not parse (corrupted external store) is reported and treated + * as if nothing were held. Callers memoize the outcome against the + * entry's stamp, so a corrupt document costs one parse + report per + * stamp, not per lookup. */ + private _decodeListTools(entry: CacheEntry): ListToolsResult | undefined { + try { + return JSON.parse(entry.value) as ListToolsResult; + } catch (error) { + this._reportError(error); + return undefined; + } + } +} + +/** + * Serialize a result for storage. Legal wire results are JSON-serializable by + * construction. Non-wire values are only reachable via in-process transports + * handing over live objects: the non-serializable ones (cycles, functions, + * BigInt) throw a TypeError the write path reports, while JSON-representable + * ones (Date, Map, NaN, explicit-undefined properties) are silently + * normalized to their JSON forms — the same shapes a real wire transport + * would have produced. + */ +function encodeCacheValue(value: unknown): string { + try { + return JSON.stringify(value); + } catch (error) { + throw new TypeError(`cache value is not JSON-serializable: ${error instanceof Error ? error.message : String(error)}`); + } } diff --git a/packages/client/test/client/mcpParamMirroring.test.ts b/packages/client/test/client/mcpParamMirroring.test.ts index 8402aad515..7a567d16ed 100644 --- a/packages/client/test/client/mcpParamMirroring.test.ts +++ b/packages/client/test/client/mcpParamMirroring.test.ts @@ -132,9 +132,9 @@ describe('SEP-2243 Mcp-Param-* mirroring (modern era)', () => { const { tools } = await client.listTools(); expect(tools.map(t => t.name)).toEqual(['route']); expect(warn).toHaveBeenCalledWith(expect.stringContaining("excluding tool 'broken'")); - expect((store.get({ method: 'tools/list', partition: PART })?.value as { tools: Tool[] }).tools.map(t => t.name)).toEqual([ - 'route' - ]); + expect( + (JSON.parse(store.get({ method: 'tools/list', partition: PART })!.value) as { tools: Tool[] }).tools.map(t => t.name) + ).toEqual(['route']); // The explicit-cursor per-page path is filtered too (the spec's MUST // has no carve-out for paginated reads). const page = await client.listTools({ cursor: '0' }); @@ -178,14 +178,14 @@ describe('SEP-2243 Mcp-Param-* mirroring (modern era)', () => { store.set( { method: 'tools/list', partition: PART }, { - value: { + value: JSON.stringify({ tools: [ { name: 'route', inputSchema: { type: 'object', properties: { region: { type: 'string', 'x-mcp-header': 'Stale-Region' } } } } ] - } + }) } ); @@ -197,7 +197,7 @@ describe('SEP-2243 Mcp-Param-* mirroring (modern era)', () => { expect(callHeaders).toEqual([{ 'Mcp-Param-Stale-Region': 'ap' }, { 'Mcp-Param-Region': 'ap' }]); // The recovery refetch wrote a fresh cache entry (REGION_TOOL, with the declaration). expect( - (store.get({ method: 'tools/list', partition: PART })?.value as { tools: Tool[] }).tools[0]?.inputSchema.properties + (JSON.parse(store.get({ method: 'tools/list', partition: PART })!.value) as { tools: Tool[] }).tools[0]?.inputSchema.properties ).toHaveProperty('region'); }); @@ -217,14 +217,14 @@ describe('SEP-2243 Mcp-Param-* mirroring (modern era)', () => { store.set( { method: 'tools/list', partition: PART }, { - value: { + value: JSON.stringify({ tools: [ { name: 'route', inputSchema: { type: 'object', properties: { region: { type: 'string', 'x-mcp-header': 'Stale-Region' } } } } ] - }, + }), expiresAt: Date.now() + 60_000, scope: 'public' } @@ -240,7 +240,7 @@ describe('SEP-2243 Mcp-Param-* mirroring (modern era)', () => { // The refetch's write overwrote the stale entry (the no-op delete // never dropped it; the `'refresh'` write replaced it). expect( - (store.get({ method: 'tools/list', partition: PART })?.value as { tools: Tool[] }).tools[0]?.inputSchema.properties + (JSON.parse(store.get({ method: 'tools/list', partition: PART })!.value) as { tools: Tool[] }).tools[0]?.inputSchema.properties ).toHaveProperty(['region', 'x-mcp-header'], 'Region'); }); @@ -309,7 +309,7 @@ describe('SEP-2243 Mcp-Param-* mirroring (modern era)', () => { store.set( { method: 'tools/list', partition: PART }, { - value: { + value: JSON.stringify({ tools: [ PAGE1, { @@ -317,7 +317,7 @@ describe('SEP-2243 Mcp-Param-* mirroring (modern era)', () => { inputSchema: { type: 'object', properties: { region: { type: 'string', 'x-mcp-header': 'Stale-Region' } } } } ] - } + }) } ); @@ -344,7 +344,7 @@ describe('SEP-2243 Mcp-Param-* mirroring (modern era)', () => { // and sends without headers — callTool never refetches on its own. store.set( { method: 'tools/list', partition: PART }, - { value: { tools: [{ name: 'route', inputSchema: { type: 'object', properties: {} } }] } } + { value: JSON.stringify({ tools: [{ name: 'route', inputSchema: { type: 'object', properties: {} } }] }) } ); expect(store.get({ method: 'tools/list', partition: PART })).toBeDefined(); await serverTx.send({ jsonrpc: '2.0', method: 'notifications/tools/list_changed' } as JSONRPCMessage); diff --git a/packages/client/test/client/responseCache.test.ts b/packages/client/test/client/responseCache.test.ts index 5006f383cd..b33ce70c13 100644 --- a/packages/client/test/client/responseCache.test.ts +++ b/packages/client/test/client/responseCache.test.ts @@ -33,25 +33,25 @@ const PRE = JSON.stringify(['', '']); describe('InMemoryResponseCacheStore', () => { it('get/set/evict/clear round-trip; evict is method-scoped; set returns the store-generated stamp', () => { const store = new InMemoryResponseCacheStore(); - const s1 = store.set({ method: 'tools/list' }, { value: 1 }); - const s2 = store.set({ method: 'prompts/list' }, { value: 2 }); - const s3 = store.set({ method: 'resources/read', params: 'file:///a' }, { value: 3, expiresAt: 123, scope: 'private' }); + const s1 = store.set({ method: 'tools/list' }, { value: '1' }); + const s2 = store.set({ method: 'prompts/list' }, { value: '2' }); + const s3 = store.set({ method: 'resources/read', params: 'file:///a' }, { value: '3', expiresAt: 123, scope: 'private' }); // Store owns the stamp counter: monotonic, opaque to callers, surfaced on the entry. expect(s2).toBeGreaterThan(s1); expect(s3).toBeGreaterThan(s2); - expect(store.get({ method: 'tools/list' })).toEqual({ value: 1, stamp: s1 }); + expect(store.get({ method: 'tools/list' })).toEqual({ value: '1', stamp: s1 }); // Store persists caller-supplied freshness metadata. expect(store.get({ method: 'resources/read', params: 'file:///a' })).toEqual({ - value: 3, + value: '3', stamp: s3, expiresAt: 123, scope: 'private' }); - expect(store.get({ method: 'tools/list', params: '', partition: '' })?.value).toBe(1); + expect(store.get({ method: 'tools/list', params: '', partition: '' })?.value).toBe('1'); store.evict('tools/list'); expect(store.get({ method: 'tools/list' })).toBeUndefined(); - expect(store.get({ method: 'prompts/list' })?.value).toBe(2); - expect(store.get({ method: 'resources/read', params: 'file:///a' })?.value).toBe(3); + expect(store.get({ method: 'prompts/list' })?.value).toBe('2'); + expect(store.get({ method: 'resources/read', params: 'file:///a' })?.value).toBe('3'); store.clear(); expect(store.get({ method: 'prompts/list' })).toBeUndefined(); }); @@ -75,10 +75,10 @@ describe('InMemoryResponseCacheStore', () => { // A NUL in `partition` cannot smuggle into `params` (and vice versa) — // the `[partition, params]` JSON-array encoding escapes every control // and quote character. - store.set({ method: 'resources/read', partition: 'a\0b', params: 'c' }, { value: 1 }); - store.set({ method: 'resources/read', partition: 'a', params: 'b\0c' }, { value: 2 }); - expect(store.get({ method: 'resources/read', partition: 'a\0b', params: 'c' })?.value).toBe(1); - expect(store.get({ method: 'resources/read', partition: 'a', params: 'b\0c' })?.value).toBe(2); + store.set({ method: 'resources/read', partition: 'a\0b', params: 'c' }, { value: '1' }); + store.set({ method: 'resources/read', partition: 'a', params: 'b\0c' }, { value: '2' }); + expect(store.get({ method: 'resources/read', partition: 'a\0b', params: 'c' })?.value).toBe('1'); + expect(store.get({ method: 'resources/read', partition: 'a', params: 'b\0c' })?.value).toBe('2'); // Same for the partition's own JSON-shaped content: the outer // JSON.stringify escapes the inner quotes. store.set({ method: 'tools/list', partition: '["x",""]' }, { value: 'real' }); @@ -104,7 +104,7 @@ describe('InMemoryResponseCacheStore', () => { // 0 disables the bound. const unbounded = new InMemoryResponseCacheStore({ maxEntries: 0 }); - for (let i = 0; i < 1000; i++) unbounded.set({ method: 'resources/read', params: String(i) }, { value: i }); + for (let i = 0; i < 1000; i++) unbounded.set({ method: 'resources/read', params: String(i) }, { value: String(i) }); expect(unbounded.size).toBe(1000); }); @@ -119,7 +119,7 @@ describe('InMemoryResponseCacheStore', () => { // Five exempt entries already exceed maxEntries=3; a resources/read // write does NOT evict any of them — the cap counts only non-exempt // keys. - for (let i = 0; i < 5; i++) store.set({ method: 'resources/read', params: String(i) }, { value: i }); + for (let i = 0; i < 5; i++) store.set({ method: 'resources/read', params: String(i) }, { value: String(i) }); expect(store.get({ method: 'tools/list' })?.value).toBe('T'); expect(store.get({ method: 'prompts/list' })?.value).toBe('P'); expect(store.get({ method: 'resources/list' })?.value).toBe('R'); @@ -128,21 +128,21 @@ describe('InMemoryResponseCacheStore', () => { // Only 3 resources/read entries survive (oldest two evicted). expect(store.get({ method: 'resources/read', params: '0' })).toBeUndefined(); expect(store.get({ method: 'resources/read', params: '1' })).toBeUndefined(); - expect(store.get({ method: 'resources/read', params: '2' })?.value).toBe(2); - expect(store.get({ method: 'resources/read', params: '4' })?.value).toBe(4); + expect(store.get({ method: 'resources/read', params: '2' })?.value).toBe('2'); + expect(store.get({ method: 'resources/read', params: '4' })?.value).toBe('4'); expect(store.size).toBe(8); // An exempt-method write at capacity never evicts a resources/read entry. store.set({ method: 'tools/list', partition: 'p2' }, { value: 'T2' }); - expect(store.get({ method: 'resources/read', params: '2' })?.value).toBe(2); + expect(store.get({ method: 'resources/read', params: '2' })?.value).toBe('2'); }); it('delete(key) drops the single entry; no-op when absent', () => { const store = new InMemoryResponseCacheStore(); - store.set({ method: 'resources/read', params: 'a', partition: 'p' }, { value: 1 }); - store.set({ method: 'resources/read', params: 'b', partition: 'p' }, { value: 2 }); + store.set({ method: 'resources/read', params: 'a', partition: 'p' }, { value: '1' }); + store.set({ method: 'resources/read', params: 'b', partition: 'p' }, { value: '2' }); store.delete({ method: 'resources/read', params: 'a', partition: 'p' }); expect(store.get({ method: 'resources/read', params: 'a', partition: 'p' })).toBeUndefined(); - expect(store.get({ method: 'resources/read', params: 'b', partition: 'p' })?.value).toBe(2); + expect(store.get({ method: 'resources/read', params: 'b', partition: 'p' })?.value).toBe('2'); // Absent key: no-op. store.delete({ method: 'resources/read', params: 'a', partition: 'p' }); }); @@ -194,12 +194,11 @@ describe('ClientResponseCache', () => { await cache.write('tools/list', value, cache.captureGeneration('tools/list')); // Mutate the caller's reference (the same object _listAllPages returns). value.tools.length = 0; - // The cached entry is a structuredClone, so the store and the + // The cache serialized the value on write, so the store and the // stamp-memoized index are unaffected. - expect((store.get({ method: 'tools/list', partition: PRE })?.value as { tools: Tool[] }).tools.map(t => t.name)).toEqual([ - 'a', - 'b' - ]); + expect( + (JSON.parse(store.get({ method: 'tools/list', partition: PRE })!.value) as { tools: Tool[] }).tools.map(t => t.name) + ).toEqual(['a', 'b']); expect((await cache.toolDefinition('a'))?.name).toBe('a'); expect((await cache.toolDefinition('b'))?.name).toBe('b'); }); @@ -212,17 +211,25 @@ describe('ClientResponseCache', () => { // keyed per URI). const gen = cache.captureGeneration('resources/read', 'res://a'); await cache.evictKey('resources/read', 'res://a'); - await cache.write('resources/read', { contents: [] }, gen, { expiresAt: 1e9, scope: 'private', params: 'res://a' }); + await cache.write('resources/read', { contents: [] }, gen, { expiresAt: Date.now() + 60_000, scope: 'private', params: 'res://a' }); expect(store.get({ method: 'resources/read', params: 'res://a', partition: PRE })).toBeUndefined(); // A sibling URI's generation is independent: evictKey('a') does not // suppress a write for 'b'. const genB = cache.captureGeneration('resources/read', 'res://b'); await cache.evictKey('resources/read', 'res://a'); - await cache.write('resources/read', { contents: [] }, genB, { expiresAt: 1e9, scope: 'private', params: 'res://b' }); + await cache.write('resources/read', { contents: [] }, genB, { + expiresAt: Date.now() + 60_000, + scope: 'private', + params: 'res://b' + }); expect(store.get({ method: 'resources/read', params: 'res://b', partition: PRE })).toBeDefined(); // A fresh capture after the evictKey writes through. const gen2 = cache.captureGeneration('resources/read', 'res://a'); - await cache.write('resources/read', { contents: [] }, gen2, { expiresAt: 1e9, scope: 'private', params: 'res://a' }); + await cache.write('resources/read', { contents: [] }, gen2, { + expiresAt: Date.now() + 60_000, + scope: 'private', + params: 'res://a' + }); expect(store.get({ method: 'resources/read', params: 'res://a', partition: PRE })).toBeDefined(); }); @@ -264,7 +271,7 @@ describe('ClientResponseCache', () => { }; const cache = new ClientResponseCache(store, true); await cache.write('tools/list', { tools: [TOOL_A] }, cache.captureGeneration('tools/list'), { - expiresAt: 1e9, + expiresAt: Date.now() + 60_000, scope: 'private' }); // The read path finds the entry the write path stored. @@ -292,7 +299,7 @@ describe('ClientResponseCache', () => { const cache = new ClientResponseCache(store, true); expect(await cache.toolDefinition('a')).toBeUndefined(); - store.set({ method: 'tools/list', partition: PRE }, { value: { tools: [TOOL_A, TOOL_B] } }); + store.set({ method: 'tools/list', partition: PRE }, { value: JSON.stringify({ tools: [TOOL_A, TOOL_B] }) }); const hit = await cache.toolDefinition('a'); expect(hit?.name).toBe('a'); // Same backing entry → identical reference (memoized index, not re-derived). @@ -300,7 +307,7 @@ describe('ClientResponseCache', () => { // A fresh write bumps the store stamp → the index re-derives (the new // entry's tool instance is what comes back, not the memoized one). - store.set({ method: 'tools/list', partition: PRE }, { value: { tools: [{ ...TOOL_A }, { ...TOOL_B }] } }); + store.set({ method: 'tools/list', partition: PRE }, { value: JSON.stringify({ tools: [{ ...TOOL_A }, { ...TOOL_B }] }) }); const hit2 = await cache.toolDefinition('a'); expect(hit2?.name).toBe('a'); expect(hit2).not.toBe(hit); @@ -426,7 +433,7 @@ describe('Client response-cache substrate', () => { expect(listCount()).toBe(3); const entry = store.get({ method: 'tools/list', partition: part() }); - expect((entry?.value as { tools: Tool[] }).tools.map(t => t.name)).toEqual(['a', 'b']); + expect((JSON.parse(entry!.value) as { tools: Tool[] }).tools.map(t => t.name)).toEqual(['a', 'b']); }); it('the auto-aggregate path threads caller params (e.g. _meta trace context) into every page request', async () => { diff --git a/packages/client/test/client/responseCacheCodec.test.ts b/packages/client/test/client/responseCacheCodec.test.ts new file mode 100644 index 0000000000..4962eed76d --- /dev/null +++ b/packages/client/test/client/responseCacheCodec.test.ts @@ -0,0 +1,150 @@ +import type { JSONRPCRequest } from '@modelcontextprotocol/core-internal'; +import { InMemoryTransport } from '@modelcontextprotocol/core-internal'; +import { afterEach, describe, expect, test } from 'vitest'; +import { Client } from '../../src/client/client'; +import type { ResponseCacheStore } from '../../src/client/responseCache'; +import { ClientResponseCache, InMemoryResponseCacheStore } from '../../src/client/responseCache'; + +const savedStructuredClone = globalThis.structuredClone; + +afterEach(() => { + globalThis.structuredClone = savedStructuredClone; +}); + +// Scripted server over a raw linked transport pair (the sibling +// responseCache.test.ts pattern) — the client package's tests must not +// depend on @modelcontextprotocol/server. +async function connectedPair() { + const [clientTx, serverTx] = InMemoryTransport.createLinkedPair(); + serverTx.onmessage = m => { + const r = m as JSONRPCRequest; + if (r.id === undefined) return; + if (r.method === 'server/discover') { + void serverTx.send({ + jsonrpc: '2.0', + id: r.id, + result: { + resultType: 'complete', + supportedVersions: ['2026-07-28'], + capabilities: { tools: {} }, + serverInfo: { name: 's', version: '1.0.0' } + } + }); + } else if (r.method === 'tools/list') { + void serverTx.send({ + jsonrpc: '2.0', + id: r.id, + result: { + resultType: 'complete', + ttlMs: 60_000, + cacheScope: 'private', + tools: [{ name: 'echo', description: 'echo', inputSchema: { type: 'object' } }] + } + }); + } + }; + await serverTx.start(); + const client = new Client({ name: 'c', version: '1.0.0' }, { versionNegotiation: { mode: { pin: '2026-07-28' } } }); + await client.connect(clientTx); + const server = { close: () => serverTx.close() }; + return { client, server }; +} + +describe('response cache document codec', () => { + test('the cache does not depend on structuredClone existing (jest-jsdom / Node < 17)', async () => { + // Pre-codec, both cache edges called the global: environments without + // it threw every write into the store-error swallow, silently + // disabling caching. The codec (JSON round-trip) has no environment + // dependency; this pins that the global is never required again. + // @ts-expect-error simulating environments without the global + delete globalThis.structuredClone; + + const { client, server } = await connectedPair(); + const errors: unknown[] = []; + client.onerror = e => errors.push(e); + const first = await client.listTools(); + const second = await client.listTools(); + expect(first.tools.map(t => t.name)).toEqual(['echo']); + expect(second.tools.map(t => t.name)).toEqual(['echo']); + expect(errors).toEqual([]); + await client.close(); + await server.close(); + }); + + test('served results are caller-owned: mutating one hit cannot reach the next', async () => { + const { client, server } = await connectedPair(); + const first = await client.listTools(); + first.tools.length = 0; + const second = await client.listTools(); + expect(second.tools.map(t => t.name)).toEqual(['echo']); + await client.close(); + await server.close(); + }); + + test('custom stores receive the serialized document, not a live object graph', async () => { + const seen: unknown[] = []; + const store = new InMemoryResponseCacheStore(); + const originalSet = store.set.bind(store); + store.set = (key, entry) => { + seen.push(entry.value); + return originalSet(key, entry); + }; + const cache = new ClientResponseCache(store, true); + await cache.write( + 'tools/list', + { tools: [{ name: 'a', inputSchema: { type: 'object' } }] }, + cache.captureGeneration('tools/list'), + { + expiresAt: Date.now() + 60_000, + scope: 'private' + } + ); + expect(seen).toHaveLength(1); + expect(typeof seen[0]).toBe('string'); + expect(JSON.parse(seen[0] as string).tools[0].name).toBe('a'); + }); + + test('a non-JSON-serializable value fails the write loudly and does not poison later calls', async () => { + const reported: unknown[] = []; + const store = new InMemoryResponseCacheStore(); + const cache = new ClientResponseCache(store, true, error => reported.push(error)); + const cyclic: { tools: unknown[]; self?: unknown } = { tools: [] }; + cyclic.self = cyclic; + + await cache.write('tools/list', cyclic, cache.captureGeneration('tools/list'), { + expiresAt: Date.now() + 60_000, + scope: 'private' + }); + // Reported (TypeError naming the cause), nothing stored, read is a miss. + expect(reported).toHaveLength(1); + expect(String(reported[0])).toMatch(/not JSON-serializable/); + expect(await cache.read('tools/list')).toBeUndefined(); + + // The cache stays functional for well-formed values afterwards. + await cache.write( + 'tools/list', + { tools: [{ name: 'b', inputSchema: { type: 'object' } }] }, + cache.captureGeneration('tools/list'), + { + expiresAt: Date.now() + 60_000, + scope: 'private' + } + ); + expect(((await cache.read('tools/list'))?.value as { tools: { name: string }[] }).tools[0]?.name).toBe('b'); + }); + + test('a corrupted document in an external store reads as a miss, not a crash', async () => { + const reported: unknown[] = []; + const store: ResponseCacheStore = { + get: () => ({ value: '{ definitely not json', stamp: 1, expiresAt: Date.now() + 60_000, scope: 'private' as const }), + set: () => 1, + delete: () => {}, + evict: () => {}, + clear: () => {} + }; + const cache = new ClientResponseCache(store, true, error => reported.push(error)); + expect(await cache.read('tools/list')).toBeUndefined(); + expect(await cache.toolDefinition('a')).toBeUndefined(); + expect(reported.length).toBeGreaterThan(0); + }); +}); From dcd3011ee6cce880771d3bd3f0d749284f56f769 Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Thu, 9 Jul 2026 11:43:38 +0000 Subject: [PATCH 2/2] fix(client): harden response cache codec against silent-undefined and corrupt documents - encodeCacheValue: JSON.stringify returns undefined (rather than throwing) for a top-level function/symbol/undefined; guard so both failure modes surface as the same loud TypeError instead of persisting { value: undefined }. - read(): drop an undecodable fresh entry from the store so it is reported once, not re-parsed and re-reported on every read until its expiresAt passes. - _decodeListTools: validate the parsed document has a tools array; a valid-JSON wrong-shape document is now reported and memoized per stamp instead of throwing out of the index builders. - Extract the guarded two-partition delete shared by evict/evictKey/read. - Reword remaining clone-era prose (test comments, nextCursor JSDoc/doc sites now say the aggregate has no nextCursor), tighten cache JSDoc, and document the string-valued CacheEntry contract in the migration guide. --- .changeset/response-cache-document-codec.md | 4 +- docs/clients/calling.md | 2 +- docs/migration/upgrade-to-v2.md | 5 +- packages/client/src/client/client.ts | 21 ++-- packages/client/src/client/responseCache.ts | 100 +++++++++--------- .../client/test/client/responseCache.test.ts | 6 +- .../test/client/responseCacheCodec.test.ts | 67 +++++++++++- 7 files changed, 133 insertions(+), 72 deletions(-) diff --git a/.changeset/response-cache-document-codec.md b/.changeset/response-cache-document-codec.md index 0b97272336..3584eaa04d 100644 --- a/.changeset/response-cache-document-codec.md +++ b/.changeset/response-cache-document-codec.md @@ -2,6 +2,6 @@ '@modelcontextprotocol/client': minor --- -The response cache now stores results as their JSON-serialized documents (serialize on write, parse on read) instead of holding live object graphs isolated with `structuredClone`. Every cache hit hands the caller a freshly parsed value it owns outright — same mutation isolation as before, without depending on the `structuredClone` global (whose absence in jest+jsdom and Node < 17 previously made every cache write throw into the store-error swallow, silently disabling caching and output-schema lookups for the whole session). In-memory and persistent stores now behave identically: `CacheEntry.value` is a string a Redis-style store persists verbatim. A value that is not JSON-serializable (only reachable via in-process transports handing over non-wire objects) now fails the write loudly to the error sink instead of silently, and a corrupted document in an external store reads as a reported miss instead of an uncaught throw. +The response cache now stores results as JSON-serialized documents (serialize on write, parse on read) instead of live object graphs isolated with `structuredClone`. Same mutation isolation, but no dependency on the `structuredClone` global — whose absence (jest+jsdom, Node < 17) previously made every cache write throw into the store-error swallow, silently disabling caching and output-schema lookups for the session. A value without a JSON representation now fails the write loudly to the error sink, and an undecodable document in an external store is reported, dropped, and read as a miss. -Migration for custom `ResponseCacheStore` implementations: `CacheEntry.value` (and the `set()` entry) is now `string`. A store that serialized on `set` and parsed on `get` must stop parsing — return the stored string verbatim. A store that inspected `entry.value` as an object must `JSON.parse` it. Entries persisted by a previous SDK version fail decode once (reported miss) and self-heal on the next write. +Migration for custom `ResponseCacheStore` implementations: `CacheEntry.value` (and the `set()` entry value) is now `string` — persist and return it verbatim, `JSON.parse` to inspect. Entries persisted by a previous SDK version fail decode once (reported, dropped) and are rewritten on the next fetch. diff --git a/docs/clients/calling.md b/docs/clients/calling.md index bb3864d20a..581abe4aea 100644 --- a/docs/clients/calling.md +++ b/docs/clients/calling.md @@ -30,7 +30,7 @@ A failed tool call is still a result: check `isError` on it before trusting `con ## Let the SDK walk the pages -That `listTools()` already walked every page: when a server splits its list, the SDK follows `nextCursor` page by page and returns one aggregated list with `nextCursor: undefined`. `listPrompts()`, `listResources()`, and `listResourceTemplates()` aggregate the same way. +That `listTools()` already walked every page: when a server splits its list, the SDK follows `nextCursor` page by page and returns one aggregated list with no `nextCursor`. `listPrompts()`, `listResources()`, and `listResourceTemplates()` aggregate the same way. Pass a `cursor` — a page's `nextCursor` your application held on to — and `listTools` returns exactly that page, raw. diff --git a/docs/migration/upgrade-to-v2.md b/docs/migration/upgrade-to-v2.md index 89188aa893..bb4b12a594 100644 --- a/docs/migration/upgrade-to-v2.md +++ b/docs/migration/upgrade-to-v2.md @@ -1587,7 +1587,7 @@ rewrite required unless noted. instead of sending the request. Set `enforceStrictCapabilities: true` in `ClientOptions` to restore the v1 throw. - Called **without a `cursor`**, the same methods now **auto-aggregate every page** and - return `nextCursor: undefined`. Passing `{ cursor }` still fetches one page. Manual + return an aggregate with no `nextCursor`. Passing `{ cursor }` still fetches one page. Manual pagination loops keep working (the first iteration returns everything); replace them with the bare no-arg call. The walk is capped at `ClientOptions.listMaxPages` (default 64); overrun throws `SdkError(ListPaginationExceeded)`. There is no way to fetch only @@ -1605,6 +1605,9 @@ rewrite required unless noted. trip. Per-call override: `{ cacheMode: 'refresh' | 'bypass' }`. New `ClientOptions`: `cachePartition`, `defaultCacheTtlMs`. `ResponseCacheStore` gained `delete(key)`; `InMemoryResponseCacheStore` is now bounded (`{ maxEntries }`, default 512). + `CacheEntry.value` (and the `set()` entry value) is the JSON-serialized result + document (`string`, not `unknown`): persist and return it verbatim, `JSON.parse` + it to inspect. #### Server (Streamable HTTP transport) diff --git a/packages/client/src/client/client.ts b/packages/client/src/client/client.ts index fd9bf757e7..15737f2e2b 100644 --- a/packages/client/src/client/client.ts +++ b/packages/client/src/client/client.ts @@ -1475,7 +1475,7 @@ export class Client extends Protocol { * Lists available prompts. * * Called without a `cursor` (the common case), this walks every page and - * returns the complete aggregated list with `nextCursor: undefined`; the + * returns the complete aggregated list with no `nextCursor`; the * aggregate is also written to the {@linkcode ResponseCacheStore}. Pass an * explicit `{ cursor }` to fetch a single page and walk pagination * yourself — the per-page path returns the server's raw page (with @@ -1515,7 +1515,7 @@ export class Client extends Protocol { * Lists available resources. * * Called without a `cursor` (the common case), this walks every page and - * returns the complete aggregated list with `nextCursor: undefined`; the + * returns the complete aggregated list with no `nextCursor`; the * aggregate is also written to the {@linkcode ResponseCacheStore}. Pass an * explicit `{ cursor }` to fetch a single page and walk pagination * yourself — the per-page path returns the server's raw page (with @@ -1557,7 +1557,7 @@ export class Client extends Protocol { * Lists available resource URI templates for dynamic resources. * * Called without a `cursor`, this walks every page and returns the - * complete aggregated list with `nextCursor: undefined`; the aggregate is + * complete aggregated list with no `nextCursor`; the aggregate is * also written to the {@linkcode ResponseCacheStore}. Pass an explicit * `{ cursor }` to fetch a single page — see * {@linkcode listResources | listResources()} for the per-page contract. @@ -1642,10 +1642,9 @@ export class Client extends Protocol { cursor = page.nextCursor; pages++; } - // delete (not `= undefined`): the aggregate round-trips through the - // cache's JSON codec, which drops explicit-undefined properties — a - // deleted key keeps `'nextCursor' in result` identical between the - // network response and a cache hit. + // delete, not `= undefined`: the cache's JSON codec drops + // explicit-undefined properties, so only absence keeps + // `'nextCursor' in result` identical between wire and cache hit. delete acc.nextCursor; finalize?.(acc); if (bypass) return acc; @@ -1688,10 +1687,8 @@ export class Client extends Protocol { * read half): under `cacheMode: 'use'` (the default), a fresh held entry * is served and the round trip is skipped. `'refresh'` and `'bypass'` * always fetch (the caller decides whether to write). Freshness and - * decoding live in {@linkcode ClientResponseCache.read}: the served value - * is freshly parsed from the stored document, so the caller owns it - * outright — mutating it (e.g. `result.tools.sort(...)`) cannot reach the - * cache or the stamp-memoized indices derived from it. A custom store + * decoding live in {@linkcode ClientResponseCache.read}; every hit is + * freshly parsed, so the caller owns it outright. A custom store * whose `get()` rejects is routed to `onerror` and treated as a miss — * cache bookkeeping never blocks a request from reaching the wire. */ @@ -2372,7 +2369,7 @@ export class Client extends Protocol { * Lists available tools. * * Called without a `cursor` (the common case), this walks every page and - * returns the complete aggregated list with `nextCursor: undefined`; the + * returns the complete aggregated list with no `nextCursor`; the * aggregate is also written to the {@linkcode ResponseCacheStore} (the * source for {@linkcode callTool | callTool()}'s output-schema validation * and SEP-2243 `Mcp-Param-*` header mirroring). Pass an explicit diff --git a/packages/client/src/client/responseCache.ts b/packages/client/src/client/responseCache.ts index 78504c85b2..22262c47fa 100644 --- a/packages/client/src/client/responseCache.ts +++ b/packages/client/src/client/responseCache.ts @@ -52,12 +52,9 @@ export interface CacheKey { } /** - * One cached response body. `value` is the JSON-serialized result document - * (the cache serializes on write and parses on read — mcp.d's - * `toJson`/`fromJson` boundary), so a store is a dumb string carrier: an - * in-memory store holds the string, a persistent store (Redis, disk) persists - * it verbatim with no extra serialization step, and both behave identically. - * `stamp` is the store-generated monotonically increasing write counter — + * One cached response body. `value` is the JSON-serialized result document — + * a store is a dumb string carrier and persists it verbatim; the cache owns + * both codec halves. `stamp` is the store-generated monotonically increasing write counter — * opaque to callers. Derived views (e.g. a `name → Tool` index) memoize * against it and re-derive only when it changes. `expiresAt` (absolute ms * epoch, `now + ttlMs`) and `scope` are the client-computed freshness @@ -414,16 +411,25 @@ export class ClientResponseCache { */ async evict(method: string): Promise { this._evictionGeneration.set(method, (this._evictionGeneration.get(method) ?? 0) + 1); + await this._deleteBoth(method, ''); + } + + /** + * Guarded two-partition delete of `{method, params}`: each partition's + * `delete` is independently wrapped so a custom store's failure on one is + * reported and does not skip the other, and the call always resolves. + */ + private async _deleteBoth(method: string, params: string): Promise { const ownPartition = this._partitionFor('private'); const sharedPartition = this._partitionFor('public'); try { - await this._store.delete({ method, params: '', partition: ownPartition }); + await this._store.delete({ method, params, partition: ownPartition }); } catch (error) { this._reportError(error); } if (sharedPartition !== ownPartition) { try { - await this._store.delete({ method, params: '', partition: sharedPartition }); + await this._store.delete({ method, params, partition: sharedPartition }); } catch (error) { this._reportError(error); } @@ -454,20 +460,7 @@ export class ClientResponseCache { // `resetForReconnect`). const current = this._evictionGeneration.get(gk); if (current !== undefined) this._evictionGeneration.set(gk, current + 1); - const ownPartition = this._partitionFor('private'); - const sharedPartition = this._partitionFor('public'); - try { - await this._store.delete({ method, params, partition: ownPartition }); - } catch (error) { - this._reportError(error); - } - if (sharedPartition !== ownPartition) { - try { - await this._store.delete({ method, params, partition: sharedPartition }); - } catch (error) { - this._reportError(error); - } - } + await this._deleteBoth(method, params); } /** @@ -492,17 +485,14 @@ export class ClientResponseCache { * overwriting the eviction with the stale aggregate would lose the * invalidation. * - * The value is stored as its JSON-serialized document (mcp.d's `toJson` - * half): serialization is simultaneously the mutation barrier — a caller - * mutating the aggregate it was returned (e.g. `result.tools.sort(...)`) - * cannot reach the cache or the stamp-memoized indices derived from it — - * and the exact representation a persistent store needs. A value that is - * not JSON-serializable (only reachable via in-process transports handing - * over non-wire objects) fails the write loudly into the `reportError` - * sink rather than silently disabling the cache. A custom store whose - * `set()` throws or rejects is routed to the same sink and the write - * resolves — cache bookkeeping never costs the caller a result it - * already fetched (consistent with the eviction path). + * The value is stored as its JSON-serialized document; serialization + * doubles as the mutation barrier, so a caller mutating the returned + * aggregate cannot reach the cache or its derived indices. A value that + * is not JSON-serializable (reachable only via in-process transports) + * fails the write loudly into the `reportError` sink. A custom store + * whose `set()` throws or rejects is routed to the same sink and the + * write resolves — cache bookkeeping never costs the caller a result it + * already fetched. * * `freshness` carries the client-computed `expiresAt` (absolute ms epoch, * `now + ttlMs`) and the server-reported `cacheScope`. The storage @@ -560,12 +550,13 @@ export class ClientResponseCache { * Serve the fresh cached result for `{method, params}`, or `undefined`. * Lookup is the two-probe order (own-partition then this server's shared * partition, gated on `scope === 'public'`); freshness is - * `entry.expiresAt > now()` (a missing `expiresAt` is never fresh — the - * retain-for-schema posture), checked BEFORE decoding so stale entries - * cost no parse. The returned value is freshly parsed from the stored - * document (mcp.d's `fromJson` half), so every hit hands the caller an - * aggregate it owns outright. An entry whose document does not parse - * (a corrupted external store) is reported and treated as a miss. + * `entry.expiresAt > now()` (a missing `expiresAt` is never fresh), + * checked BEFORE decoding so stale entries cost no parse. Every hit is + * freshly parsed, so the caller owns the value outright. An entry whose + * document does not parse (corrupted external store) is reported, + * deleted, and treated as a miss — deleted because a fresh-but-corrupt + * entry would otherwise re-parse and re-report on every read until its + * `expiresAt` passes. */ async read(method: string, params?: string): Promise<{ value: unknown } | undefined> { const entry = await this._probe(method, params); @@ -574,6 +565,7 @@ export class ClientResponseCache { return { value: JSON.parse(entry.value) }; } catch (error) { this._reportError(error); + await this._deleteBoth(method, params ?? ''); return undefined; } } @@ -665,13 +657,16 @@ export class ClientResponseCache { } /** Parse a held `tools/list` document for the index builders; a document - * that does not parse (corrupted external store) is reported and treated - * as if nothing were held. Callers memoize the outcome against the - * entry's stamp, so a corrupt document costs one parse + report per - * stamp, not per lookup. */ + * that does not parse OR parses to something without a `tools` array + * (both mean a corrupted external store) is reported and treated as if + * nothing were held. Callers memoize the outcome against the entry's + * stamp, so a corrupt document costs one parse + report per stamp, not + * per lookup. */ private _decodeListTools(entry: CacheEntry): ListToolsResult | undefined { try { - return JSON.parse(entry.value) as ListToolsResult; + const parsed = JSON.parse(entry.value) as ListToolsResult | null; + if (!Array.isArray(parsed?.tools)) throw new TypeError('cached tools/list document has no tools array'); + return parsed; } catch (error) { this._reportError(error); return undefined; @@ -680,18 +675,19 @@ export class ClientResponseCache { } /** - * Serialize a result for storage. Legal wire results are JSON-serializable by - * construction. Non-wire values are only reachable via in-process transports - * handing over live objects: the non-serializable ones (cycles, functions, - * BigInt) throw a TypeError the write path reports, while JSON-representable - * ones (Date, Map, NaN, explicit-undefined properties) are silently - * normalized to their JSON forms — the same shapes a real wire transport - * would have produced. + * Serialize a result for storage; throws TypeError for anything without a + * JSON representation (legal wire results always have one). Two failure + * modes need folding into that one throw: `JSON.stringify` throws on cycles + * and BigInt, but silently returns `undefined` (not a string) for a + * top-level function, symbol, or `undefined`. */ function encodeCacheValue(value: unknown): string { + let json: string | undefined; try { - return JSON.stringify(value); + json = JSON.stringify(value); } catch (error) { throw new TypeError(`cache value is not JSON-serializable: ${error instanceof Error ? error.message : String(error)}`); } + if (typeof json !== 'string') throw new TypeError('cache value is not JSON-serializable: it has no JSON representation'); + return json; } diff --git a/packages/client/test/client/responseCache.test.ts b/packages/client/test/client/responseCache.test.ts index b33ce70c13..510c6ff510 100644 --- a/packages/client/test/client/responseCache.test.ts +++ b/packages/client/test/client/responseCache.test.ts @@ -466,8 +466,8 @@ describe('Client response-cache substrate', () => { // Common previously-harmless caller patterns. result.tools.sort((x, y) => y.name.localeCompare(x.name)); result.tools.length = 0; - // ClientResponseCache.write stored a structuredClone, so neither the - // backing entry nor the stamp-memoized name → Tool index moved. + // The cache serialized the value on write, so neither the backing + // entry nor the stamp-memoized name → Tool index moved. expect((await toolDef(client, 'a'))?.name).toBe('a'); expect((await toolDef(client, 'b'))?.name).toBe('b'); }); @@ -649,7 +649,7 @@ describe('Client honours cacheHints (SEP-2549)', () => { const second = await client.listTools(); expect(second.tools.map(t => t.name)).toEqual(['a', 'b']); expect(listCount()).toBe(1); - // Clone-on-serve: hit is a fresh copy, not the stored object. + // Parse-on-serve: hit is a fresh copy, not the stored object. expect(second).not.toBe(first); // After TTL → stale, refetch. diff --git a/packages/client/test/client/responseCacheCodec.test.ts b/packages/client/test/client/responseCacheCodec.test.ts index 4962eed76d..bfdc2a7c45 100644 --- a/packages/client/test/client/responseCacheCodec.test.ts +++ b/packages/client/test/client/responseCacheCodec.test.ts @@ -2,7 +2,7 @@ import type { JSONRPCRequest } from '@modelcontextprotocol/core-internal'; import { InMemoryTransport } from '@modelcontextprotocol/core-internal'; import { afterEach, describe, expect, test } from 'vitest'; import { Client } from '../../src/client/client'; -import type { ResponseCacheStore } from '../../src/client/responseCache'; +import type { CacheEntry, CacheKey, ResponseCacheStore } from '../../src/client/responseCache'; import { ClientResponseCache, InMemoryResponseCacheStore } from '../../src/client/responseCache'; const savedStructuredClone = globalThis.structuredClone; @@ -147,4 +147,69 @@ describe('response cache document codec', () => { expect(await cache.toolDefinition('a')).toBeUndefined(); expect(reported.length).toBeGreaterThan(0); }); + + test('a value JSON.stringify silently cannot represent (top-level undefined) fails the write loudly, not as a stored "undefined"', async () => { + const reported: unknown[] = []; + const sets: unknown[] = []; + const store = new InMemoryResponseCacheStore(); + const originalSet = store.set.bind(store); + store.set = (key, entry) => { + sets.push(entry.value); + return originalSet(key, entry); + }; + const cache = new ClientResponseCache(store, true, error => reported.push(error)); + await cache.write('tools/list', undefined, cache.captureGeneration('tools/list'), { + expiresAt: Date.now() + 60_000, + scope: 'private' + }); + expect(reported).toHaveLength(1); + expect(String(reported[0])).toMatch(/not JSON-serializable/); + expect(sets).toHaveLength(0); + expect(await cache.read('tools/list')).toBeUndefined(); + }); + + test('a fresh undecodable entry is dropped on read, so it is reported once, not on every read until expiry', async () => { + const reported: unknown[] = []; + const deletes: CacheKey[] = []; + let entry: CacheEntry | undefined = { value: '{ not json', stamp: 1, expiresAt: Date.now() + 60_000, scope: 'private' }; + const store: ResponseCacheStore = { + get: () => entry, + set: () => 1, + delete: key => { + deletes.push(key); + entry = undefined; + }, + evict: () => {}, + clear: () => {} + }; + const cache = new ClientResponseCache(store, true, error => reported.push(error)); + expect(await cache.read('resources/read', 'file:///a')).toBeUndefined(); + expect(reported).toHaveLength(1); + expect(deletes.length).toBeGreaterThan(0); + expect(deletes[0]).toMatchObject({ method: 'resources/read', params: 'file:///a' }); + // Entry gone: the next read is a clean miss, no second report. + expect(await cache.read('resources/read', 'file:///a')).toBeUndefined(); + expect(reported).toHaveLength(1); + }); + + test('a valid-JSON but wrong-shape tools/list document is reported once per stamp and treated as absent, not thrown', async () => { + for (const document of ['null', '{}', '[]']) { + const reported: unknown[] = []; + const store: ResponseCacheStore = { + get: () => ({ value: document, stamp: 7, scope: 'private' as const }), + set: () => 1, + delete: () => {}, + evict: () => {}, + clear: () => {} + }; + const cache = new ClientResponseCache(store, true, error => reported.push(error)); + await expect(cache.toolDefinition('a')).resolves.toBeUndefined(); + await expect(cache.toolDefinition('a')).resolves.toBeUndefined(); + await expect(cache.outputValidator('a', () => undefined)).resolves.toBeUndefined(); + // Memoized against the unchanged stamp: one report for the tool + // index, one for the validator index — not one per lookup. + expect(reported).toHaveLength(2); + expect(String(reported[0])).toMatch(/no tools array/); + } + }); });