refactor(client): response cache stores serialized documents; drop structuredClone#2468
Conversation
🦋 Changeset detectedLatest commit: a59bc6a The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
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.
…guards - A NaN expiresAt passed both freshness comparisons and was served forever; the gate is now the positive check the doc already states. - read() served decodable-but-non-object documents verbatim; they now route through the same report + drop + miss path as parse failures. - _decodeListTools validated only the container, so a null element threw in the index builders before memoization, re-firing per call; the guard now requires object elements.
| 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.
🟡 read()'s decode-failure handling routes through _deleteBoth(method, params ?? ''), deleting BOTH partitions even though the JSON.parse failure is evidence about exactly one physical entry — the one _probe returned. On a shared multi-principal store (the per-user cachePartition deployment docs/clients/caching.md documents), one principal's corrupt private entry therefore also wipes a never-probed, healthy public entry at [serverIdentity, ''], costing every co-tenant (and the reader's own scope === 'public' fallback) a wire refetch. Fix: delete only the partition the corrupt entry was probed from — e.g. have _probe return the partition alongside the entry, or derive it in read() (the entry came from the shared partition iff the own probe missed).
Extended reasoning...
The bug. The delete-on-corrupt hardening this PR adds to ClientResponseCache.read() (responseCache.ts:561-571) handles a fresh entry whose stored document fails JSON.parse by reporting the error and calling _deleteBoth(method, params ?? '') — an unconditional two-partition delete of both [serverIdentity, cachePartition] and [serverIdentity, '']. But _probe (responseCache.ts:385-394) is own-partition-first: it returns the own (private) entry without ever looking at the shared partition. So when the own-partition entry is the corrupt one, read() deletes the corrupt entry (correct) and a shared-partition entry it never probed, which may be a perfectly healthy public document. The reverse direction is safe by construction: a corrupt shared entry implies the own probe already missed, so the own-partition delete is a no-op.
Why the two-partition breadth is right elsewhere but wrong here. evict() and evictKey() legitimately use _deleteBoth because their triggers (list_changed, notifications/resources/updated) are statements about the logical resource — both partition copies are stale by definition. write()'s opposite-partition delete is also correct because the caller holds fresh authoritative data and a stale sibling would shadow it on the own-first probe. A decode failure carries no such information: it says one physical entry is corrupt and says nothing about the sibling — deleting it is pure cache loss. This also contradicts the co-tenant-preservation posture the PR's own evict() JSDoc articulates ("on a shared store one server's list_changed cannot wipe a co-tenant's entry").
The coexistence precondition is reachable through pure SDK writes. write()'s opposite-partition delete only spans the writer's two partitions (partition === ownPartition ? sharedPartition : ownPartition). On a shared store with per-principal cachePartition — exactly the deployment docs/clients/caching.md ('Partition the store per user') and examples/caching/README.md document — alice's private-scoped write stores at [srv,'alice'] and deletes [srv,'']; bob's later public-scoped write for the same {method, params} stores at [srv,''] and deletes only [srv,'bob']. Both entries now legitimately coexist.
Step-by-step proof (verified by verifiers running the real ClientResponseCache + InMemoryResponseCacheStore):
- alice (
cachePartition: 'alice') holds a private-scopedresources/readentry forres://xat[srv,'alice']. - bob reads
res://x; the server stampscacheScope: 'public'→ stored at[srv,'']; bob's write deletes only[srv,'bob']. Both entries coexist. - alice's private entry gets corrupted in the external store — the exact corruption class this PR's changeset targets.
- alice reads:
_probereturns her fresh-but-corrupt own entry →JSON.parsethrows → reported →_deleteBothremoves[srv,'alice']and[srv,'']. - The shared public entry existed before and is gone after: bob's next read and alice's own next read (which the own-miss → shared-probe →
scope === 'public'fallback would have served) both become wire refetches.
Why the new test doesn't catch it. The codec test ('a fresh undecodable entry is dropped on read') uses a single-entry mock store whose get/delete ignore the key's partition field entirely, so it cannot observe the collateral delete. And the earlier review finding that requested this delete explicitly recommended 'mirroring evictKey's guarded two-partition delete' — the recommendation and the implementation both miss the breadth issue.
Impact and fix. Impact is self-healing cache loss, not incorrect data: one principal's corrupted private slot silently converts cache hits into wire round trips for every co-tenant of that server on the store, until the next fetch rewrites the public entry. That plus the narrow trigger (external-store corruption of one partition's copy while the sibling stays healthy, on a shared multi-principal store with mixed scopes) is why this is a nit rather than a blocker. The fix is small: delete only the partition the corrupt entry was probed from — have _probe return the partition alongside the entry (internal seam), or derive it in read() (the entry came from the shared partition iff the own probe missed) — one guarded store.delete against the offending key, keeping the report + miss semantics.
There was a problem hiding this comment.
Will address in followup
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.