fix(mcp): preserve cache invalidation during refresh - #4807
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36e85b1dff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
cbbcd49 to
eb1c136
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb1c136e94
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| refresh_generation == self._tools_cache_generation | ||
| and refresh_sequence == self._tools_refresh_sequence |
There was a problem hiding this comment.
Let a surviving refresh publish after a newer attempt fails
When two cache-miss list_tools() calls overlap in the same generation, the second call immediately changes _tools_refresh_sequence; if that newer call is then cancelled or raises while the first call succeeds, this condition prevents the successful survivor from publishing. The server therefore retains a stale or absent _tools_list, remains dirty, performs another remote fetch on the next listing, and skips local required-parameter validation in the interim. Track the newest successful refresh rather than letting a failed attempt permanently disqualify surviving work, and cover the A pending -> B starts -> B fails -> A succeeds ordering.
AGENTS.md reference: AGENTS.md:L104-L104
Useful? React with 👍 / 👎.
seratch
left a comment
There was a problem hiding this comment.
Please remove _tools_refresh_sequence and use the captured invalidation generation to decide whether a completed refresh can publish. With cache_tools_list=False, a newer failed or cancelled refresh currently prevents an older successful refresh from updating the cache. That can leave an obsolete schema marked clean, causing call_tool() to reject arguments that match the successfully returned tool definition.
Please add a regression covering a previously cached schema requiring q, an updated schema without that requirement, and the ordering A pending -> B starts -> B fails -> A succeeds. Assert that the subsequent valid tool call reaches the server.
This pull request fixes an MCP tools-cache race where an in-flight
list_tools()refresh could clear a newer invalidation or leave a retainedstale schema authoritative for
call_tool()validation.Each refresh captures the current cache generation and only publishes its
result when that generation remains current. While the cache is dirty, local
required-parameter validation is skipped until a later refresh establishes a
clean authoritative snapshot.
A deterministic concurrency regression covers the stale refresh result,
retained cache snapshot, dirty-cache call-through, subsequent refresh, and
restored clean-cache validation.
Validation:
tests/mcp/test_caching.pymake formatmake lintmake typecheckmake testsgit diff --check