⚡ refactor: Fast-Fail MCP Tool Discovery on 401 for Non-OAuth Servers#12395
Conversation
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR fixes a tool discovery hang when MCP servers that are configured as non-OAuth respond with auth errors (e.g., 401). It ensures discovery “fast-fails” by always attaching an oauthRequired handler that emits oauthFailed, unblocking the connection’s internal OAuth-handling wait.
Changes:
- Always register an
oauthRequiredlistener indiscoverToolsInternal()(independent ofuseOAuth). - Always remove the
oauthRequiredlistener during discovery cleanup (independent ofuseOAuth).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ 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". |
4c10c6a to
283f6ca
Compare
|
@codex review again |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ 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". |
Always attach oauthHandler in discoverToolsInternal regardless of useOAuth flag. Previously, non-OAuth servers hitting 401 would hang for 30s because connectClient's oauthHandledPromise had no listener to emit oauthFailed, waiting until withTimeout killed it.
283f6ca to
fd38d6a
Compare
danny-avila
changed the title
danny-avila
changed the title
…danny-avila#12395) * fix: fast-fail MCP discovery for non-OAuth servers on auth errors Always attach oauthHandler in discoverToolsInternal regardless of useOAuth flag. Previously, non-OAuth servers hitting 401 would hang for 30s because connectClient's oauthHandledPromise had no listener to emit oauthFailed, waiting until withTimeout killed it. * chore: import order
…danny-avila#12395) * fix: fast-fail MCP discovery for non-OAuth servers on auth errors Always attach oauthHandler in discoverToolsInternal regardless of useOAuth flag. Previously, non-OAuth servers hitting 401 would hang for 30s because connectClient's oauthHandledPromise had no listener to emit oauthFailed, waiting until withTimeout killed it. * chore: import order
…danny-avila#12395) * fix: fast-fail MCP discovery for non-OAuth servers on auth errors Always attach oauthHandler in discoverToolsInternal regardless of useOAuth flag. Previously, non-OAuth servers hitting 401 would hang for 30s because connectClient's oauthHandledPromise had no listener to emit oauthFailed, waiting until withTimeout killed it. * chore: import order
Summary
Fixed a 30-second hang during MCP tool discovery triggered when a non-OAuth server returns a 401. Previously,
connectClient()emittedoauthRequiredand awaited an internaloauthHandledPromise, but whenuseOAuthwasfalse, no listener was registered to dispatchoauthFailed— leaving the promise unresolved until the outerwithTimeoutkilled it after 30s (with an additional ~90s background floating execution and leakedsetTimeout).if (this.useOAuth)guards aroundconnection.on/removeListenercalls indiscoverToolsInternal, makingoauthHandlerregistration unconditional.oauthHandlerfromasyncto a plain synchronous function, ensuringoauthFailedis emitted synchronously within theoauthRequireddispatch and preventing an unhandled-rejection footgun for future maintainers.connection.on('oauthRequired', oauthHandler)withconnection.once(...)to express the one-shot semantics of this event in the discovery lifecycle.onceregistration documenting why it is unconditional, protecting the fix from being mistakenly reverted as dead code.MCPConnectionFactory.test.tsverifying thatdiscoverToolsresolves in under 5 seconds with{ tools: null, oauthRequired: true }when a non-OAuth server returns 401.MCPOAuthConnectionEvents.test.tsexercising the fix against a real OAuth-gated MCP server viacreateOAuthMCPServer, asserting sub-5s resolution and correct result shape.oauthRequiredtest inMCPConnectionFactory.test.tsto interceptonceinstead ofonand invoke the handler synchronously, matching the new implementation.Change Type
Testing
Verified via two test layers:
Unit test (
MCPConnectionFactory.test.ts): MocksMCPConnectionwithonceinterceptingoauthRequired, simulates a 401 throw fromconnect(), and asserts the result resolves in under 5s with{ tools: null, oauthRequired: true, oauthUrl: null, connection: null }.Integration test (
MCPOAuthConnectionEvents.test.ts): Spins up a real OAuth-gated MCP server viacreateOAuthMCPServer, callsMCPConnectionFactory.discoverToolsagainst it with no OAuth options (simulating a non-OAuth client hitting a protected server), and asserts sub-5s resolution with the correct result shape.Test Configuration:
packages/api—npx jest MCPConnectionFactorypackages/api—npx jest MCPOAuthConnectionEventsChecklist