Repository navigation
fix(mcp): preserve independent OAuth refresh cancellation - #59114
craigalbrecht wants to merge 2 commits into
Conversation
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix works. I ran the reproduction from #59109 from source: on the base, aborting the follower still gave it the token, and aborting the leader failed both callers. With this PR, only the caller that aborted rejects, the other gets 200, and the token endpoint is still hit once. mcp-oauth.test.ts passes (24/24) and tsc is clean. The new tests are small and gate the response on a promise rather than sleeping.
The public API is unchanged.
One regression should be fixed before merging: a caller whose signal is already aborted now leaves the shared request's promise rejecting with no handler, which triggers an unhandled rejection the base didn't have (see the inline comment). The other inline comment is about a small cancellation window that's easy to close now. #53275 also changes send and the refreshes map, so one of the two will need a rebase.
Related: #53275
| const cleanup = () => signal?.removeEventListener("abort", onAbort) | ||
| const onAbort = () => { | ||
| current.waiting-- | ||
| if (current.waiting === 0) current.controller.abort(signal?.reason) |
There was a problem hiding this comment.
When waiting reaches 0, the entry stays in refreshes until the aborted fetch settles. A caller that arrives in that window with the same refresh token joins a request that is already cancelled and gets AbortError, even though its own signal is fine. That's the same as the base, but now that the shared controller exists, it's cheap to delete the entry here, e.g. if (refreshes.get(key) === current) refreshes.delete(key). That would need the key stored on PendingRefresh.
When the caller's signal is already aborted, this returns before current.pending.then(...) is attached. If it is the only caller, waiting drops to 0, the controller aborts, and current.pending (the .finally-wrapped promise) rejects with no handler. I checked: a single fetcher(url, { body: refreshBody, signal: AbortSignal.abort() }) produces unhandledRejection: AbortError with this PR but not on the base. Could you check init?.signal?.aborted in send before starting the shared request, or attach a no-op .catch to current.pending?
There was a problem hiding this comment.
Thanks—both cases are fixed in 1a142e9c73.
sendnow rejects an already-aborted signal before starting the shared request, so there is no internal unhandled rejection.- When the last waiter aborts, its entry is removed from
refreshesbefore the shared controller is aborted. A later caller therefore starts a fresh request rather than joining the cancelled one.
Added regressions for both cases. Validation: full OAuth suite 26/26, cancellation cases 40/40 over ten reruns, core tsc, focused oxlint, and root bun run check all pass.
There was a problem hiding this comment.
Both earlier points are fixed. A caller whose signal is already aborted is now rejected before any request starts, and I no longer see an unhandled rejection. When the last waiter aborts, its entry is removed, so a caller arriving right after starts a fresh refresh. I re-ran the #59109 reproduction: only the caller that aborted rejects, the other gets the token, and the endpoint is hit once. mcp-oauth.test.ts passes (26/26, and 40/40 for the abort tests with --rerun-each 10), and tsc is clean. The public API is unchanged. I have no further concerns.
|
Superseded by #53275; the signal path I guarded is not exercised by the SDK. Branch left in place for reference. |
Issue for this PR
Closes #59109
Type of change
What does this PR do?
Concurrent refreshes of the same token still share one network request, preventing token-rotation races. Each caller now keeps its own abort subscription. The shared request uses a dedicated controller and is aborted only after every waiting caller has cancelled.
This fixes both directions of the previous coupling: follower aborts no longer get ignored, and a leader abort no longer cancels independent followers.
How did you verify your code works?
The two new loopback-server regressions failed before the fix (
0 pass, 2 fail) and pass afterward.packages/core/test/mcp-oauth.test.ts: 26 passed, 0 failed.--rerun-each 10: 40 passed, 0 failed.bun run typecheck:tscinpackages/core: passed.bun run typecheckwith pinned Bun 1.4.4: 34 tasks passed.bun run check: passed.oxlinton both changed files: 0 warnings, 0 errors.bun script/sdk-docs.ts: 0 problems.Screenshots / recordings
N/A — MCP OAuth fetch behavior.
Checklist
Added by the opencode bot (AI-generated).
Related: #53275