From 80a94c5c1cb2eca926f9dd1da143c8f87145eba1 Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Mon, 13 Jul 2026 16:51:51 +0000 Subject: [PATCH] Scope corrupt-cache-entry deletion to the probed partition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit read()'s decode-failure cleanup deleted both partitions, but corruption is evidence about the one physical entry _probe returned — on a shared multi-principal store the unconditional two-partition delete evicted healthy co-tenant entries. _probe now reports which partition it hit and the cleanup deletes exactly that entry, with store delete failures routed to the error sink per the class contract. --- .changeset/partition-scoped-corrupt-delete.md | 5 +++ packages/client/src/client/responseCache.ts | 26 +++++++---- .../test/client/responseCacheCodec.test.ts | 44 +++++++++++++++++++ 3 files changed, 66 insertions(+), 9 deletions(-) create mode 100644 .changeset/partition-scoped-corrupt-delete.md diff --git a/.changeset/partition-scoped-corrupt-delete.md b/.changeset/partition-scoped-corrupt-delete.md new file mode 100644 index 0000000000..b5d7b94937 --- /dev/null +++ b/.changeset/partition-scoped-corrupt-delete.md @@ -0,0 +1,5 @@ +--- +'@modelcontextprotocol/client': patch +--- + +A corrupt cached document now deletes only the partition it was read from. The previous cleanup deleted both partitions, so on a shared multi-principal store one principal's corrupt private entry also evicted the healthy shared entry. Store delete failures during this cleanup are reported through the error sink instead of rejecting the read. diff --git a/packages/client/src/client/responseCache.ts b/packages/client/src/client/responseCache.ts index 50abd37bc8..6221f6edda 100644 --- a/packages/client/src/client/responseCache.ts +++ b/packages/client/src/client/responseCache.ts @@ -382,15 +382,15 @@ export class ClientResponseCache { * `cachePartition` is `''` the two partitions are identical and only one * probe is issued. */ - private async _probe(method: string, params?: string): Promise { + private async _probe(method: string, params?: string): Promise<{ entry: CacheEntry; partition: string } | undefined> { const key = { method, params: params ?? '' }; const ownPartition = this._partitionFor('private'); const own = await this._store.get({ ...key, partition: ownPartition }); - if (own !== undefined) return own; + if (own !== undefined) return { entry: own, partition: ownPartition }; const sharedPartition = this._partitionFor('public'); if (sharedPartition === ownPartition) return undefined; const shared = await this._store.get({ ...key, partition: sharedPartition }); - return shared?.scope === 'public' ? shared : undefined; + return shared?.scope === 'public' ? { entry: shared, partition: sharedPartition } : undefined; } /** @@ -560,17 +560,23 @@ export class ClientResponseCache { * `expiresAt` passes. */ 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; + const probed = await this._probe(method, params); + if (probed?.entry.expiresAt === undefined || !(probed.entry.expiresAt > this.now())) return undefined; try { - const parsed: unknown = JSON.parse(entry.value); + const parsed: unknown = JSON.parse(probed.entry.value); if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) { throw new TypeError('cached document is not an object'); } return { value: parsed }; } catch (error) { + // Corruption is evidence about one physical entry; the sibling + // partition was never probed and may be a co-tenant's healthy copy. this._reportError(error); - await this._deleteBoth(method, params ?? ''); + try { + await this._store.delete({ method, params: params ?? '', partition: probed.partition }); + } catch (deleteError) { + this._reportError(deleteError); + } return undefined; } } @@ -604,7 +610,8 @@ export class ClientResponseCache { * and, via {@linkcode outputValidator}, its output-schema validation. */ async toolDefinition(name: string): Promise { - const entry = await this._probe('tools/list'); + const probed = await this._probe('tools/list'); + const entry = probed?.entry; if (entry === undefined) { this._toolIndex = undefined; return undefined; @@ -644,7 +651,8 @@ export class ClientResponseCache { * the lot. */ async outputValidator(name: string, compile: (tool: Tool) => V | undefined): Promise { - const entry = await this._probe('tools/list'); + const probed = await this._probe('tools/list'); + const entry = probed?.entry; if (entry === undefined) { this._toolOutputValidatorIndex = undefined; return undefined; diff --git a/packages/client/test/client/responseCacheCodec.test.ts b/packages/client/test/client/responseCacheCodec.test.ts index 6d73941fb5..b7cb4e5e41 100644 --- a/packages/client/test/client/responseCacheCodec.test.ts +++ b/packages/client/test/client/responseCacheCodec.test.ts @@ -248,6 +248,50 @@ describe('response cache document codec', () => { } }); + test('a corrupt own-partition entry deletes only the probed partition, not the shared sibling', async () => { + const reported: unknown[] = []; + const gets: string[] = []; + const deletes: string[] = []; + const store: ResponseCacheStore = { + // Own probe (first get) hits a corrupt entry; the shared partition is never touched. + get: key => { + gets.push(key.partition ?? ''); + return { value: '{ not json', stamp: 1, expiresAt: Date.now() + 60_000, scope: 'private' as const }; + }, + set: () => 1, + delete: key => { + deletes.push(key.partition ?? ''); + }, + evict: () => {}, + clear: () => {} + }; + // Non-empty cachePartition: own !== shared, so a two-partition delete + // (the pre-fix behavior) would be observable as a second delete. + const cache = new ClientResponseCache(store, true, error => reported.push(error), 'alice'); + expect(await cache.read('tools/list')).toBeUndefined(); + expect(reported).toHaveLength(1); + expect(gets).toHaveLength(1); + expect(deletes).toEqual([gets[0]]); + expect(gets[0]).toContain('alice'); + }); + + test('a failing store delete during corrupt-entry cleanup is reported, not thrown to the caller', async () => { + const reported: unknown[] = []; + const store: ResponseCacheStore = { + get: () => ({ value: '{ not json', stamp: 1, expiresAt: Date.now() + 60_000, scope: 'private' as const }), + set: () => 1, + delete: () => { + throw new Error('store connection lost'); + }, + evict: () => {}, + clear: () => {} + }; + const cache = new ClientResponseCache(store, true, error => reported.push(error)); + await expect(cache.read('tools/list')).resolves.toBeUndefined(); + expect(reported).toHaveLength(2); + expect(String(reported[1])).toMatch(/connection lost/); + }); + test('a tools/list document with non-object elements is reported once per stamp, not thrown per lookup', async () => { const reported: unknown[] = []; const store: ResponseCacheStore = {