fix(client): scope corrupt-cache-entry deletion to the probed partition#2485
fix(client): scope corrupt-cache-entry deletion to the probed partition#2485felixweinberger wants to merge 1 commit into
Conversation
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 detectedLatest commit: 80a94c5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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: |
felixweinberger
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| --- | ||
| '@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. |
There was a problem hiding this comment.
🔴 The PR title and description describe a completely different change — adding @modelcontextprotocol/codemod to the changesets fixed group — but the diff contains none of that: it is a client responseCache fix (partition-scoped corrupt-entry deletion) with a client-only changeset, and .changeset/config.json is untouched. If merged as-is, the claimed codemod-versioning fix silently does not ship (the npm install @modelcontextprotocol/codemod@2.0.0-beta.4 failure the description promises to fix remains), and the merge record misdescribes the change — either push the intended diff or replace the title/description with ones describing the actual responseCache fix. Separately (minor), the changeset's last sentence ('reported through the error sink instead of rejecting the read') implies delete failures used to reject the read; they never did — the old _deleteBoth already caught and reported them — so reword it to say the existing error-sink reporting is preserved.
Extended reasoning...
The PR metadata describes a change that is not in the diff
Title/description vs. actual content. The title is chore: version the codemod with the core SDK fixed group, and the description explains adding @modelcontextprotocol/codemod to the changesets fixed group (plus a codemod changeset) so the pending beta.4 release includes it. The diff, however, contains exactly three files: .changeset/partition-scoped-corrupt-delete.md (patching only @modelcontextprotocol/client), packages/client/src/client/responseCache.ts, and packages/client/test/client/responseCacheCodec.test.ts. There is no change to .changeset/config.json, no codemod changeset, and no mention of the codemod package anywhere in the diff.
Corroborating evidence. The changeset-bot comment on this PR confirms the release contents: 5 packages bumped (client, core, server, server-legacy, core-internal) — @modelcontextprotocol/codemod is absent. The bot's link also shows the branch name is fix/partition-scoped-corrupt-delete, which matches the diff's content, not the title. And at HEAD, .changeset/config.json's fixed group still lists only core/client/server/server-legacy — so the described change is not present anywhere; it simply does not ship. The most likely explanation is the title/description were copy-pasted from a different PR (the branch and code are internally consistent as the responseCache fix).
Step-by-step consequence if merged as-is:
- The merge commit records "chore: version the codemod with the core SDK fixed group" — a change that never happened. Anyone auditing history or generating release notes from PR titles records a phantom change.
- The author (and reviewers) believe the codemod-versioning problem is fixed. It is not: the
fixedgroup is unchanged. - The next release round runs: changesets bumps the fixed group to
2.0.0-beta.4, the codemod stays at2.0.0-beta.3(no changeset, not in the fixed group). - A user follows the migration guide's "all v2 packages share one version" guarantee and runs
npm install @modelcontextprotocol/codemod@2.0.0-beta.4→ no matching version — exactly the failure the description claims to fix.
Why this is normal severity rather than a description nit. The in-diff code is correct and well-tested, and its changeset accurately describes it — the defect is purely at the PR-metadata level. But this is not a stale or imprecise description of the code in the diff; it is a wholesale mismatch where an intended fix silently fails to ship while the record says it shipped. The author needs to act before merge: either the wrong branch/diff was pushed (push the intended .changeset/config.json change), or the metadata was pasted from another PR (retitle/re-describe this one as the responseCache partition-scoped corrupt-delete fix and file the codemod change separately).
Secondary (nit): one changeset sentence implies a behavior change that isn't one
.changeset/partition-scoped-corrupt-delete.md ends with: "Store delete failures during this cleanup are reported through the error sink instead of rejecting the read." In a changelog, "instead of" reads as a before/after contrast — implying the pre-fix path rejected read() on a store delete failure. It did not: the old corrupt path called this._deleteBoth(method, params ?? ''), and _deleteBoth (still present at packages/client/src/client/responseCache.ts:431) wraps each partition's delete in try/catch, routes failures to this._reportError, and always resolves. Verified against the pre-fix code (git show 80a94c5~1): a delete failure never rejected the read before this PR. Error-sink reporting is preserved pre-existing behavior, not part of this change. Suggested reword: "Store delete failures during this cleanup continue to be reported through the error sink." Nothing breaks if this merges as-is, but changelog readers would otherwise infer a rejection bug existed.
1 participant
Follow-up to #2468 review discussion:
read()'s corrupt-document cleanup deleted both partitions, but corruption is evidence about the one physical entry_probereturned — on a shared multi-principal store (the per-usercachePartitiondeployment the caching guide documents), one principal's corrupt private entry also evicted the healthy shared entry for every co-tenant._probenow reports which partition it hit and the cleanup deletes exactly that entry. Store delete failures during cleanup are reported through the error sink instead of rejecting the read, per the class contract.How Has This Been Tested?
Unit tests pin the single-partition delete (with a non-empty
cachePartitionso a two-partition regression is observable — mutation-checked), the failing-delete-is-reported path, and the existing corrupt-document suite; the full client suite passes cold. Driven end-to-end through the built dist via the publicresponseCacheStoreoption: corrupt entry → one report, one delete targeting the probed partition, clean wire refetch.Breaking Changes
None.
Types of changes
Checklist