Skip to content

fix(client): discard cache reads invalidated while pending - #3072

Open
ilovvveu wants to merge 6 commits into
modelcontextprotocol:mainfrom
ilovvveu:fix/cache-read-invalidation
Open

ilovvveu wants to merge 6 commits into
modelcontextprotocol:mainfrom
ilovvveu:fix/cache-read-invalidation

Conversation

@ilovvveu

@ilovvveu ilovvveu commented Oct 22, 2026 •

Copy link
Copy Markdown

An asynchronous ResponseCacheStore.get() can return a snapshot after the SDK has processed a change notification and deleted that entry. Client.listTools() then returns the old catalogue without refetching. The same lookup feeds resource reads and the derived tool-definition/output-validator indexes.

Motivation and Context

Capture the existing per-key eviction generation before probing the store and recheck it after each awaited lookup. If invalidation intervened, treat the result as a cache miss. Unrelated keys retain their cache hits. A connection-generation check also discards reads pending across resetForReconnect(), even when a new read recreates the same key at generation 0. A retained user-supplied store remains available to fresh reads. This addresses pending reads; asynchronous store mutation ordering is the separate concern in #2688.

How Has This Been Tested?

Deterministic tests park a cache read, change the scripted server catalogue, deliver notifications/tools/list_changed, then release the old snapshot. Both private and shared-public cache paths are exercised through Client.listTools(). Additional cases cover per-URI invalidation, derived tool metadata and unrelated-key controls. No sleeps are used.

On macOS, Node 22.23.3, pnpm 10.26.1:

  • Invalidation cases: 5 regressions fail on upstream; 2 controls pass. Fixed source: all 7 pass.
  • Reconnect cases: 8 cases cover private/public probes, same/different server identity, and a fresh same-key read before the old one resumes. The original PR fails 4; this follow-up passes all 8. Retained store entries remain reusable by fresh reads.
  • Response-cache tests: 67 pass. Full client suite: 44 files / 1,083 tests pass.
  • Client typecheck and lint pass. The initial revision also passed workspace lint:all.
  • Integration rerun after the reconnect fix: 23 files / 444 tests pass. The initial broader run also passed core, codemod, server, middleware and example suites.
  • Initial full E2E run: 3,120 pass, 141 expected failures, 1 failure plus 2 unhandled rejections in protocol:timeout:max-total [sse 2026-07-28]. The same failure reproduces with the original cache source when running the complete protocol test file, matching [v2] e2e protocol:timeout:max-total is flaky over SSE: fake timers advance before the request reaches the server #3045. The isolated max-total cases pass. Full E2E is not claimed green.

Breaking Changes

None. The public API and store interface are unchanged. Includes a client patch changeset.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

AI assistance was used for investigation, implementation and tests.

@ilovvveu
ilovvveu requested a review from a team as a code owner October 22, 2026 17:30
@changeset-bot

changeset-bot Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 15e9641

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@modelcontextprotocol/client Patch
@modelcontextprotocol/codemod Patch
@modelcontextprotocol/core Patch
@modelcontextprotocol/server-legacy Patch
@modelcontextprotocol/server Patch
@modelcontextprotocol/core-internal Patch

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

@pkg-pr-new

pkg-pr-new Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@3072

@modelcontextprotocol/codemod

npm i https://pkg.pr.new/@modelcontextprotocol/codemod@3072

@modelcontextprotocol/core

npm i https://pkg.pr.new/@modelcontextprotocol/core@3072

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@3072

@modelcontextprotocol/server-legacy

npm i https://pkg.pr.new/@modelcontextprotocol/server-legacy@3072

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@3072

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@3072

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@3072

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@3072

commit: 15e9641

@arhancanli

Copy link
Copy Markdown

Read the diff against responseCache.ts on main (code read only; I did not run the suite).

One edge the new isCurrent check treats differently from write: resetForReconnect() calls this._evictionGeneration.clear(). A probe that is pending across a reconnect then compares undefined === generation and returns undefined, so the read is discarded as a miss. write handles the same state the other way: it does get(...) ?? 0, so an absent key counts as generation 0 and a write captured at 0 still goes through. For a store whose backing data survives the reconnect (the user-supplied case in the docs), the two paths disagree on whether the same pending operation is still valid.

Discarding the read is probably the safer behaviour, since the entry may belong to the old connection's server identity. If it is intended, a test with a parked get, a resetForReconnect(), and the expected result would pin it. If it is not, isCurrent could use ?? 0 like write.

Is the discard-on-reconnect result deliberate?

@ilovvveu

Copy link
Copy Markdown
Author

@arhancanli Thanks for catching this. The original revision did not explicitly document or test the reconnect case. I agree that a lookup started on an old connection should be discarded after reset, even when a user-supplied store retains its entries; fresh reads can still reuse the retained data.

Adding the requested tests exposed another edge: if a fresh read recreates the same key at generation 0 before the old get() resolves, the old lookup passes the original check again. I added a separate connection generation, incremented by resetForReconnect(), so both the connection and per-key generation must still match after each awaited lookup.

The 8 new cases cover private/public probes, reconnecting to the same or a different server identity, and a fresh same-key read before releasing the parked one. Four fail on the previous PR revision; all 8 pass with this change. They also verify that fresh reads can use the retained store.

I have left write()'s pre-existing ?? 0 behavior unchanged; this PR fixes pending reads and does not claim to resolve cross-reset writes. Copying that fallback into the read guard would allow old snapshots through.

Validation: 67 cache tests; 44 client test files / 1,083 tests; 23 integration files / 444 tests; client typecheck and lint all pass. The PR description and changeset now state the reset behavior explicitly.

@arhancanli

Copy link
Copy Markdown

Thanks for the detailed follow-up. I read the new diff: capturing _connectionGeneration at the start of _probe and requiring both it and the per-key generation after each awaited store.get covers the case I was worried about, and the same-key-recreated-at-0 edge you found is exactly why the per-key counter alone is not enough after _evictionGeneration.clear(). Checking both isCurrent() calls (before the own return and again after the shared lookup) also means a shared-partition hit can't slip through after the private lookup passed. I did not run the suite.

On leaving write()'s ?? 0 alone: agreed that changing it is a separate decision. But it means a write captured before a reset at generation 0 still lands after resetForReconnect() on a user-supplied store, so the old-connection data can come back by that route even though reads are now guarded. Is that tracked as its own issue? A write guard that also compares a captured _connectionGeneration would be a small follow-up, without changing the ?? 0 fallback itself.

@ilovvveu

Copy link
Copy Markdown
Author

@arhancanli Tracked separately in #3075 now. Thanks for flagging it.

I reproduced the coordinator behavior on both pinned main and this PR head in all 8 combinations of private/public scope, same/different server identity, and with/without a fresh write before the old response reaches write(). The old generation-0 write is accepted; after an identity change it is even stored in the current server's partition. The issue includes a minimal reproduction and explicitly distinguishes this coordinator evidence from an end-to-end transport reconnect test.

I agree a connection-generation guard is the right direction. Its snapshot needs to be taken before the asynchronous fetch and carried through to write(); capturing it only at write entry would miss this ordering. The issue also distinguishes that token lifetime from the already-pending store mutation ordering in #2688. I'll keep #3072 focused on the pending-read fix.

@claude claude Bot added the v2 Ideas, requests and plans for v2 of the SDK which will incorporate major changes and fixes label Oct 23, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Ideas, requests and plans for v2 of the SDK which will incorporate major changes and fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants