fix(mcp): keep one cached catalog per protocol era - #109
Open
carldebilly wants to merge 2 commits into
Open
carldebilly wants to merge 2 commits into
carldebilly wants to merge 2 commits into
Conversation
Issue #101 reports that one snapshot slot per session lets publishing one era's catalog evict the other's: alternating requests would rebuild on every switch, and a legacy build that failed transiently would find only a modern entry and lose its availability fallback. Both consequences need a modern catalog to be published after a legacy one on the same connection, and the SDK does not allow that. McpServerImpl (2.2.0) sets a session's protocol version on its first request that carries one. initialize supersedes a version set earlier, and after that SetNegotiatedProtocolVersion rejects any request naming a different version with InvalidRequest. So one connection goes through modern requests, then an initialize, then legacy requests only. Nothing can evict the legacy entry the fallback reads, there is one switch at most, and a legacy build that fails before any legacy success has nothing to fall back to with one slot or two. The new test, written as #101's regression, could not reproduce it: the modern request after initialize came back as InvalidRequest (-32600). It now pins that premise instead. If the SDK ever lets a connection return to the modern era, the test fails and the cache needs one slot per era. Pointing the fourth request at no version at all turns it red, so the assertion is not swallowed by its null-conditional chain. SnapshotCacheEntry's documentation now says why one slot is enough. Refs #101
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce942d6347
ℹ️ 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".
The previous commit closed #101 as unreachable: after initialize, the SDK rejects a new modern request, so it looked as if no modern catalog could ever be published after a legacy one. Review on #109 showed the hole in that. A modern request accepted before initialize can still be in flight, since the SDK dispatches one connection's requests concurrently, and its build then publishes after the legacy one. With one slot, that replaces the legacy entry the availability fallback reads, which is exactly what #101 describes. The window is narrow but real. McpSessionContext now keeps one entry per era, reached through GetSnapshotCache(sessionless), and publishing writes only its own era's slot. The era checks at each read are gone, since the slot already is the era. ThrowSanitizedIfAClientAlreadyHasASchema asks HasServedSnapshot, meaning either era, so a client that has seen any schema still gets the sanitised message. The test that pinned the one-way premise is removed, along with the documentation paragraph built on it. TDD: the new When_BothErasPublishOnOneSession_Then_EachKeepsItsOwnEntry was written against the new lookup first implemented over the single slot, and was red there — the modern publication evicted the legacy entry — before the slots were split. Refs #101
carldebilly
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #101.
McpSessionContextkept one cached catalog per session, and publishing it for one protocol era overwrote the other era's entry. A connection is served modern requests, then aninitializeand legacy ones. A modern request accepted beforeinitializecan still be in flight, because the SDK dispatches one connection's requests concurrently, and its build then publishes after the legacy one. That replaced the legacy entry the availability fallback reads. A legacy build that then failed transiently found only a modern entry and failed closed, on a connection that had been serving a legacy catalog a moment earlier.The context now keeps one entry per era, read through
GetSnapshotCache(sessionless), and each publication writes only its own era's slot. The per-read era checks are gone, since the slot now is the era.ThrowSanitizedIfAClientAlreadyHasASchemaasksHasServedSnapshot(either era), so a client that has seen any schema still gets the sanitised message.History: this PR first closed #101 as unreachable. Its test showed that a newly submitted modern request is rejected once
initializehas negotiated the legacy version. Codex pointed out that this misses a modern request already in flight, which is right, so the second commit implements the fix and drops that test.Test plan
When_BothErasPublishOnOneSession_Then_EachKeepsItsOwnEntrywas written against the new lookup while it still sat on the single slot, and was red there (the modern publication evicted the legacy entry). It went green once the slots were split.When_OneConnectionIsServedBothEras_Then_EachGetsItsOwnCatalogstill passes.