refactor(client): response cache stores serialized documents; drop structuredClone#2468
refactor(client): response cache stores serialized documents; drop structuredClone#2468felixweinberger wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: dcd3011 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/core
@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}
There was a problem hiding this comment.
LGTM — small, well-tested capability fallback for the response cache; the inline nits (add a patch changeset, drop the .js import extensions) are trivial to address before merge.
Extended reasoning...
Overview
This PR is a four-file, client-package-only fix: a new cloneResultValue helper (structuredClone with a JSON round-trip fallback), two call-site swaps in the cache read (client.ts) and write (responseCache.ts) paths, and a new test file. It fixes a real fail-open: in environments without the structuredClone global, every cache write threw into the deliberate store-error swallow, silently disabling caching (and output-schema validation derived from cached tools/list) for the whole session. A repo-wide grep confirms no other structuredClone call sites remain in packages/client/src outside the helper — the migration is complete.
Security risks
None identified. The change doesn't touch auth, transport lifecycle, or partitioning logic. The JSON fallback is lossless for wire-decoded JSON-RPC results (the only values that legally reach the cache), and mutation isolation — the invariant the clone exists to uphold — is preserved by both branches and covered by tests in both directions. typeof structuredClone === 'function' is a ReferenceError-safe existence check.
Level of scrutiny
Moderate: it's production client code on the cache hot path, but the change is behaviorally conservative — in structuredClone-present environments (the norm) behavior is byte-identical, and in absent environments the pre-fix behavior was the cache being silently off, so the fallback is strictly an improvement. No public API surface changes, no new exports from package index files, no protocol/spec implications.
Other factors
Test coverage is solid: unit tests for both helper branches plus an end-to-end regression over InMemoryTransport with the global deleted, asserting the error sink stays quiet. The two inline findings are genuine but nit-level and mechanical: the missing .changeset/*.md (needed for the fix to actually ship, per the pattern of #2455/#2453/#2451) and three .js relative-import extensions violating the CLAUDE.md convention. Neither affects correctness at merge time, and both are one-line fixes already flagged inline, so they don't warrant blocking approval.
felixweinberger
force-pushed
the
fweinberger/structured-clone-fallback
branch
from
61e11c6 to
ff980cd
Compare
felixweinberger
changed the title
…structuredClone 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.
felixweinberger
force-pushed
the
fweinberger/structured-clone-fallback
branch
from
ff980cd to
0452138
Compare
… 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.
| const entry = await this._probe(method, params); | ||
| if (entry?.expiresAt === undefined || entry.expiresAt <= this.now()) return undefined; |
There was a problem hiding this comment.
🟡 The freshness gate rewritten into read() inverts NaN handling: pre-PR, a corrupt expiresAt of NaN failed the positive check (expiresAt > now is false) and the entry was never served; post-PR, NaN skips the early return (NaN === undefined and NaN <= now are both false) so the entry is decoded and served — and since NaN <= now is false at every future now, it is served forever, silently defeating the write-side 24h MAX_CACHE_TTL_MS clamp. A one-line fix restores the fail-closed posture: keep the positive comparison (if (!(entry?.expiresAt !== undefined && entry.expiresAt > this.now())) return undefined;) or guard with Number.isFinite(entry.expiresAt).
Extended reasoning...
The bug. This PR moves the freshness gate from _serveFromCache (client.ts) into ClientResponseCache.read() and, in doing so, negates the comparison:
- Pre-PR (deleted hunk in
client.ts):if (entry?.expiresAt !== undefined && entry.expiresAt > this._cache.now()) { …serve… } - Post-PR (
responseCache.ts:563):if (entry?.expiresAt === undefined || entry.expiresAt <= this.now()) return undefined;
These are De Morgan duals only for totally-ordered values. For expiresAt = NaN, every comparison is false: pre-PR NaN > now is false → not served (fail-closed, treated as stale); post-PR NaN === undefined is false and NaN <= now is false → the early return is skipped → the entry is decoded and served. And because NaN <= now stays false at every future now, the entry never expires — it is served on every subsequent call until an explicit cacheMode: 'refresh' or a list_changed eviction dislodges it.
Verified concretely (Node): for now = 1000, the pre-PR serve condition and post-PR miss condition agree for expiresAt of 2000 (fresh, served), 500 (stale, miss), and undefined (miss). They diverge exactly and only at NaN: pre-PR not served, post-PR served — at now = 1000, 10^9, and every later clock value.
The code path that triggers it. expiresAt is typed number and round-trips verbatim through the caller's ResponseCacheStore — the external persistent stores (Redis, disk, KV) this PR's design explicitly targets. A store that persists metadata as strings and rehydrates with Number(record.expiresAt), parseInt(...), or new Date(s).getTime() yields NaN whenever the field is missing or garbled. That is precisely the corrupted-external-store class this PR hardens the value field against (undecodable document → reported, deleted, miss) — but for the metadata field the same corruption class now yields the opposite posture: a permanently-fresh entry. (A secondary in-process path exists too: _freshness in client.ts gates ttlMs on typeof === 'number', which NaN passes, so ttlMs: NaN smuggled over an in-process transport produces now + NaN = NaN from the SDK's own write path — though a Zod-validated wire result can never carry it, since JSON cannot encode NaN.)
Why nothing else catches it. The read() freshness check is the only consumer of expiresAt on the read path; there is no Number.isFinite validation anywhere in responseCache.ts. The 24h MAX_CACHE_TTL_MS clamp is write-side only (_freshness, client.ts), so a store-returned NaN bypasses it verbatim. The PR's new corrupt-document handling (report + _deleteBoth + miss) triggers only on JSON.parse throws of the value — corrupt metadata sails through with no report and no self-heal.
Impact. listTools() / listPrompts() / listResources() / listResourceTemplates() / readResource() serve the pinned entry indefinitely with no wire refetch, no error report, and no eviction. Pre-PR, the identical store bug was harmless: the entry was treated as stale and the client fetched normally. So this hunk converts a fail-closed corrupt-metadata posture into fail-open-forever — counter to the PR's own hardening narrative.
Step-by-step proof.
- A Redis-backed
ResponseCacheStoreholds atools/listentry whoseexpiresAtfield was lost; itsget()rehydrates viaNumber(record.expiresAt)→Number(undefined)→NaN. client.listTools()→_serveFromCache→read('tools/list')→_probereturns{ value: '…', stamp: 1, expiresAt: NaN }.- Line 563:
NaN === undefined→ false;NaN <= now()→ false. The early return is skipped. JSON.parse(entry.value)succeeds → the entry is served. No wire request.- Every later
listTools()repeats 2–4 with the same outcome — at any clock value — so the cached list is pinned forever. Pre-PR the same entry failedNaN > nowand the client fetched over the wire.
Why nit, not blocking. The trigger requires a contract-violating custom store: the SDK's own write path cannot produce NaN over any wire transport (JSON cannot encode it; JSON.stringify(NaN) → null, which both forms treat as stale), and the shipped InMemoryResponseCacheStore round-trips values verbatim. Spec-compliant setups never hit it. But it is squarely the corrupted-external-store class this PR's own description hardens against, the regression from fail-closed to fail-open-forever is introduced by this exact hunk, and the fix is one line.
Fix. Keep the positive comparison: if (!(entry?.expiresAt !== undefined && entry.expiresAt > this.now())) return undefined; — or guard explicitly: if (!Number.isFinite(entry?.expiresAt) || entry.expiresAt <= this.now()) return undefined;. Either restores NaN → stale.
| 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); | ||
| await this._deleteBoth(method, params ?? ''); | ||
| return undefined; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Two wrong-shape gaps remain in the corrupt-document hardening this PR added, both in the valid-JSON-but-wrong-shape class: (1) read() (responseCache.ts:561-571) only guards JSON.parse throws, so a fresh entry holding 'null' / '"str"' / '[]' is served verbatim — listTools()/readResource() return null typed as the result and the caller's next .tools/.contents access is a TypeError in application code, with no report and no drop, unlike the parse-failing sibling; (2) _decodeListTools's new guard validates only Array.isArray(parsed?.tools), not the elements, so '{"tools":[null]}' passes and the index-builder loops throw before the stamp memoization is assigned — re-firing two store probes, two parses, and two onerror reports on every callTool, the exact non-memoized failure mode the inline 'once per stamp' comment and the new codec test claim is closed. Fix both with the existing posture: in read(), reject a decoded value that is not a non-null plain object via the same _reportError + _deleteBoth + miss path; in _decodeListTools, also require the elements to be non-null objects (routed through _reportError + empty-index memoization).
Extended reasoning...
Gap 1 — read() serves a decodable-but-impossible document verbatim. read() (responseCache.ts:561-571) checks freshness, then wraps only JSON.parse in its try/catch. A corrupted external-store document that is still valid JSON — 'null', '"str"', '[]', '{}' — parses without throwing and is returned as { value: <parsed> }. _serveFromCache (client.ts) then does return hit.value as R, and every cacheable verb gates on if (hit !== undefined) return hit (client.ts:1510, 1550, 1583, 1783, 2412) — null !== undefined, so a fresh entry holding 'null' makes listTools() / readResource() return null typed as ListToolsResult / ReadResourceResult. The caller's next result.tools.map(...) or result.contents[0] is a TypeError crash in application code. Unlike the parse-failing sibling this PR handles (reported + _deleteBoth + miss), this document is neither reported nor dropped — it keeps being served until its store-controlled expiresAt passes.\n\nStep-by-step proof (gap 1). (1) A Redis-backed ResponseCacheStore returns { value: 'null', stamp: 1, expiresAt: <future>, scope: 'private' } for tools/list (truncated blob, key collision, or a repair pass that produced valid JSON — the corruption class the string-document contract this PR introduces makes realistic). (2) listTools() → _serveFromCache → read(): probe hits, freshness passes, JSON.parse('null') = null, no throw → { value: null } returned. (3) hit !== undefined → return null as ListToolsResult. (4) The application's result.tools.map(...) throws TypeError: Cannot read properties of null (reading 'tools').\n\nGap 2 — _decodeListTools's shape guard misses element-level corruption. The guard this PR added after the earlier review round (responseCache.ts:668, !Array.isArray(parsed?.tools)) validates only the container. '{"tools":[null]}' passes it, and then both index builders throw before their memoization assignment: toolDefinition's loop does byName.set(tool.name, tool) → TypeError: Cannot read properties of null (reading 'name') before this._toolIndex = { stamp, byName } is assigned (~line 614); outputValidator passes compile(null) into the Client's _compileOutputValidator, whose first statement dereferences tool.outputSchema — again before the line-654 memoization. Verified in Node by multiple independent passes. Because the failure is never memoized against the stamp, it reproduces the exact failure mode the fix was meant to close: the inline comment ('parsed (and reported) once per stamp — not once per callTool') and the _decodeListTools JSDoc ('one parse + report per stamp, not per lookup') are false for this shape, and the new codec test pins only whole-document shapes ('null'/'{}'/'[]'), never an element-level one.\n\nStep-by-step proof (gap 2). (1) External store returns { value: '{"tools":[null]}', stamp: 1 } under the tools/list key. (2) client.callTool({ name: 'route', ... }) → _resolveXMcpHeaderScan → toolDefinition('route') → probe hits, stamp differs from the never-assigned _toolIndex → _decodeListTools returns the parsed object (tools IS an array) → the loop throws on null.name. (3) The throw is contained (try/catch at client.ts:2247-2251), reported to onerror, and the call proceeds without Mcp-Param-* headers. (4) The output-validation read repeats the sequence (.catch(this._reportStoreError) at client.ts:2267-2269, 2321-2323) — a second probe, parse, and report. (5) Every subsequent callTool repeats steps 2-4 for the whole session: two store probes + two full parses + two onerror fires per call, with header mirroring and output-schema validation silently degraded — and the error escapes as a raw TypeError from a different layer instead of the deliberate _reportError route the whole-document case now takes.\n\nWhy existing code doesn't prevent either, and why this is squarely this PR's code. Legal wire results are always non-null objects (Zod-validated before write), and JSON.stringify of them always yields an object document with object elements — so both shapes can only arrive via a corrupted/foreign external store. But that is precisely the corruption class this PR's own hardening targets: the same revision added the container-shape guard to _decodeListTools, the write-side encodeCacheValue guard, and the read-side delete-on-corrupt, and the changeset promises 'an undecodable document in an external store is reported, dropped, and read as a miss'. A decodable-yet-impossible document is served verbatim by read() and throws un-memoized in the index builders — the two remaining holes in a sweep that closed the same class everywhere else.\n\nHow to fix. Both fixes reuse the guards' existing posture. In read(), after JSON.parse succeeds, reject a value that is not a non-null plain object (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) through the same _reportError + _deleteBoth + return-undefined path the catch block already implements. In _decodeListTools, extend the guard to the elements — e.g. if (!Array.isArray(parsed?.tools) || !parsed.tools.every(t => t !== null && typeof t === 'object')) throw new TypeError(...) — so a bad element routes through _reportError and memoizes as an empty index per stamp, exactly like '{}' does now. Codec tests seeding 'null' for read() and '{"tools":[null]}' for the index builders would pin both.\n\nSeverity. Nit, unanimously across six verifier passes: both triggers require an external store returning corrupted-but-valid-JSON with a still-fresh expiresAt (rare), gap 2's throws are all contained so wire results stay correct, and the pre-PR clone-based code had analogous unguarded casts (structuredClone(entry.value) as R would have served a corrupt live value verbatim too) — so nothing regresses at merge. This is defensive hardening that completes the sweep the PR started and makes its own documented per-stamp guarantee true.
1 participant
The response cache held live object graphs and isolated them from caller mutation with
structuredCloneat both edges (write and hit). This PR replaces that with the design this cache was modeled on (mcp.d'scachedFetch): store the JSON-serialized document — serialize on write, parse on read.structuredCloneis deleted from the SDK entirely.Motivation and Context
Three problems with the clone-based shape, all found while embedding the client:
structuredCloneglobal (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.With the document codec: every hit hands the caller a freshly parsed value it owns outright (same mutation isolation, no primitive dependency); in-memory and persistent stores behave identically (
CacheEntry.valueis a string persisted verbatim); non-serializable values fail the write loudly to the error sink; a corrupted document in an external store is a reported miss, memoized per stamp so it costs one parse + report per rewrite, not per lookup.Peer-SDK survey that motivated the shape: mcp.d stores
Jsonand re-materializes per hit (toJson/fromJson); go-sdk and csharp-sdk share references by convention (workable in Go/C#, not in JS where callers legitimately mutate results).How Has This Been Tested?
Full client suite green (29 files, 723 tests). New
responseCacheCodec.test.tspins: cache works with thestructuredCloneglobal deleted; served results are caller-owned (mutating one hit can't reach the next); custom stores receive the serialized string; a cyclic value fails the write loudly and doesn't poison later calls; a corrupted document reads as a reported miss, not a crash. Workspace typecheck + lint green.Breaking Changes
CacheEntry.value(and the storeset()entry value) is nowstring. CustomResponseCacheStoreimplementations: a store that serialized onset/parsed ongetmust stop parsing (return the string verbatim); a store that inspectedentry.valueas an object mustJSON.parseit. Entries persisted by a previous beta fail decode once (reported miss) and self-heal on the next write. Semantics note: values only reachable via in-process transports are normalized to their JSON forms (Date → ISO string, NaN → null), i.e. the same shapes a real wire transport produces.Types of changes
Checklist
Additional context
The list aggregates'
nextCursoris nowdeleted rather than set toundefined, keeping property presence identical between a network response and a cache hit through the codec.