fix(mcp): survive a cross-process OAuth refresh race on connect - #40768
Open
justprosh wants to merge 3 commits into
Open
fix(mcp): survive a cross-process OAuth refresh race on connect#40768justprosh wants to merge 3 commits into
justprosh wants to merge 3 commits into
Conversation
The connect-time provider snapshots the stored credential when it is built and serves tokens from that snapshot. With several processes sharing the credential store, two of them can hold the same refresh token; the first refresh rotates it and updates the row, and the loser's refresh then fails with the stale token. Its invalidate hook removes the whole credential row, destroying the fresh tokens the winner just saved, and the server drops to needs_auth even though working credentials existed a moment earlier. Re-read the row on every tokens() call so a rotation done elsewhere is picked up, and make invalidate keep a row that has rotated since this provider last presented it. Ref anomalyco#34520 anomalyco#34074
Drives one connect against an auth-gated server whose token endpoint always rejects. Another process rotates the shared credential while the first refresh is in flight; the retry must present the rotated token, which only happens if the rejected refresh left the row alone.
Contributor
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
Contributor
|
The following comment was made by an LLM, it may be inaccurate: I found one potentially related PR: #37058 - This PR addresses a similar cross-process OAuth refresh race condition for the xAI provider. The current PR (40768) is fixing the same type of issue for MCP servers. Both deal with credential rotation happening across multiple processes, though they address different providers/contexts. |
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.
Issue for this PR
Closes #34520
Type of change
What does this PR do?
Two opencode processes share one credential row per MCP server, so both can hold the same refresh
token. The first refresh rotates it and saves the new tokens; the second one fails with the token it
still had, and its
invalidatehook deletes the whole row — throwing away the tokens the first onejust wrote, so the server drops to
needs_authright after a successful refresh.The connect provider trusted the snapshot it took when it was built. Now
tokens()re-reads the row,so a rotation done elsewhere is picked up, and
invalidatere-reads before deleting: a row thatchanged since we last presented tokens is kept, a row that really is the rejected one is still gone.
No locking is added — that is the wider single-flight work in #34520.
How did you verify your code works?
packages/core/test/mcp-oauth.test.ts: one connect against an auth-gated server whosetoken endpoint rejects, with the credential rotated by another process mid-refresh. Fails on
v2,passes with the fix.
bun test test/mcp.test.ts test/mcp-oauth.test.tsinpackages/core— 23 pass,tsgo --noEmitclean.credential row instead of deleting it.
Checklist