Skip to content

mcp: do not cache resources/read results of multi round-trip retries - #1371

Merged
guglielmo-san merged 3 commits into
modelcontextprotocol:mainfrom
vbcherepanov:fix/readresource-mrtr-cache
Oct 12, 2026
Merged

guglielmo-san merged 3 commits into
modelcontextprotocol:mainfrom
vbcherepanov:fix/readresource-mrtr-cache

Conversation

@vbcherepanov

Copy link
Copy Markdown
Contributor

ClientSession.ReadResource cached every successful result under its URI and looked every request up by URI alone. The multi round-trip middleware runs inside handleSend, so the result of a retry carrying inputResponses and requestState was cached and served on the next plain read, which bypassed the elicitation that gated the resource. With the middleware disabled, a caller's manual retry was answered from the cache with the interim input_required result of the first read and never reached the server.

This change keeps multi round-trip traffic out of the cache in both directions:

  • A request that carries InputResponses or RequestState is neither served from the cache nor stored.
  • The client middleware marks a ReadResourceResult it obtained by retrying, through a small unexported retriedResult interface, and ReadResource does not store such a result.
  • An input_required result is not stored.

Results that needed no input are cached as before. There is no change to the exported API or to the wire format; the marker is an unexported field.

The specification says results of retries "MUST NOT be cached, as they depend on inputs that are not part of the cache key", that a cached response must not be served "for a request whose method or parameters differ from the request that produced it", and that interim results "are not cacheable" (2026-07-28, Caching).

Fixes #1369

Tested with go test ./... -race and the new tests at -race -count=50. TestMultiRoundTrip_ReadResource_RetryResultNotCached covers the automatic retry and includes a positive control for a plain cached resource. TestMultiRoundTrip_ReadResource_ManualRetryBypassesCache covers the manual retry loop. Both fail on main and pass with this change.

ReadResource cached every result under its URI and looked every request up by URI alone, so the result of a retry carrying inputResponses was served on the next plain read, and a manual retry was answered with the cached interim result. Skip the cache for requests carrying input responses or request state, and do not store retried or input_required results. The caching specification forbids all three.

Fixes modelcontextprotocol#1369
@vbcherepanov
vbcherepanov force-pushed the fix/readresource-mrtr-cache branch from 98ebe54 to 4ebf5f4 Compare October 12, 2026 06:20
Comment thread mcp/mrtr.go
Comment on lines 104 to 106
if reqMap == nil && !mrtrResult.NeedsInput() {
return res, nil
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if reqMap == nil && !mrtrResult.NeedsInput() {
if r, ok := res.(retriedResult); ok && retries > 1 {
r.markRetried()
}
return res, nil
}

@vbcherepanov

Copy link
Copy Markdown
Contributor Author

Applied in 8aa288c. I kept the !res.isNil() check from the earlier version: inputRequests() is nil-safe, so a typed nil *ReadResourceResult can reach this branch, and markRetried() would dereference it. go test ./mcp/ passes, and the multi round-trip tests pass with -race.

@guglielmo-san

Copy link
Copy Markdown
Contributor

@vbcherepanov thank you for the contribution!

@guglielmo-san
guglielmo-san merged commit 12c67a6 into modelcontextprotocol:main Oct 12, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mcp: ReadResource caches the result of a multi round-trip retry and serves a retry request from the cache

2 participants