fix(mcp): stabilize authenticated tool discovery#5754
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview The server refresh API calls OAuth start returns 422 with a clear message when dynamic client registration is unsupported. Reviewed by Cursor Bugbot for commit b9fe771. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR stabilizes authenticated MCP tool discovery and publication. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (3): Last reviewed commit: "fix(mcp): acquire a fresh cache mutation..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- preserve typed static-header authentication failures\n- order discovery publication by start time\n- atomically publish tools and failure cache state
Match configuration generations within the JavaScript millisecond window so PostgreSQL microseconds do not suppress valid publication.
Addressed in 89dce25: both status-publication predicates now match the JavaScript millisecond window, accepting PostgreSQL sub-millisecond precision while still rejecting the next generation. A boundary regression covers the behavior. |
- fall back to best-effort cache writes when mutation ordering is unavailable\n- share bounded redacted MCP error diagnostics across client and service
Retry cache mutation ownership before publishing discovery state, avoid unordered publication that older writers can supersede, and keep explicit invalidation best effort.
Use timestamp-based mutation tokens for both cache ownership and database CAS ordering, and invalidate Redis mutation owners before deleting entries during a full clear.
Recompute consecutive failures after status CAS conflicts, reload winning cached tools for superseded discoveries, and return live tools without unordered publication when cache ordering is unavailable.
Expose discovery publication metadata to refresh callers, report live tools during cache degradation, and compare lastToolsRefresh mutation tokens instead of wall clocks when preserving a newer success.
Probe mutation ownership after database CAS misses, keep valid live tools for metadata-only edits, and invalidate ownership for transport and auth-type changes.
Hash connection-affecting server fields and recheck the fresh revision after database CAS misses so metadata edits retain valid tools while URL, credential, and transport changes fail closed.
MCP servers are commonly behind HTTP/2 fronts (CDNs, cloud LBs), but the SSRF-pinned undici Agent is HTTP/1.1-only unless it opts into h2 via ALPN. Add an allowH2 option to createPinnedFetch (default false, so LLM-provider callers are unchanged) and centralize the MCP h2 decision in one createPinnedMcpFetch helper routed through the transport, the OAuth probe, and the SSRF-guarded discovery/revocation fetch. Pinning is unaffected: the pinned lookup forces every connection to the resolved IP regardless of the negotiated protocol.
…lpers - Delete the unused setIfCurrentMutation/deleteIfCurrentMutation cache methods (interface + memory + redis adapters) and their two Redis Lua scripts; production only uses applyMutationIfCurrent. Tests rewritten onto applyMutationIfCurrent (ordering coverage preserved) or dropped where they only exercised the removed Lua. - Drop the vestigial fallback param from getDiscoveryFailureMessage (all callers coerced to 'Connection failed'). - Route isDynamicClientRegistrationUnsupported through getErrorMessage to match the module's error-inspection convention.
|
@cursor review |
A retried discovery took mutation ownership once before the retry loop, so a retry that succeeded after a concurrent clearCache published under a stale ownership id, lost the CAS, and dropped otherwise-valid tools (empty fail-closed result). Begin a fresh mutation per attempt so a retried result publishes under a current id. Adds a regression test.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit cdc3f46. Configure here.
Reacquire cache mutation ownership for each discovery retry and cover cache invalidation during retry backoff. Note: the full app suite has pre-existing failures in unrelated workflow, webhook, chat, and UI tests; the focused MCP suite passes 38/38.
…x/mcp-discovery-races
Publish cache invalidation tokens into database ordering and fail refresh closed when discovery is superseded. Focused MCP suites pass 48/48 and API validation passes. Note: the full app suite retains the same 41 pre-existing failures in unrelated workflow, webhook, chat, and UI tests.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b9fe771. Configure here.
| refreshedServer.lastConnected > discoveryStartedAt | ||
| refreshedServer?.lastToolsRefresh != null && | ||
| (server.lastToolsRefresh == null || | ||
| refreshedServer.lastToolsRefresh > server.lastToolsRefresh) |
There was a problem hiding this comment.
Invalidation hides refresh failures
Medium Severity
newerSuccessWonRace treats any newer lastToolsRefresh as a successful discovery, but markServerCacheInvalidated also advances that token while leaving connectionStatus as connected. A failed refresh racing list_changed or clearCache can therefore return connected with a null error and hide the real discovery failure.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b9fe771. Configure here.
There was a problem hiding this comment.
“
newerSuccessWonRacetreats any newerlastToolsRefreshas a successful discovery, butmarkServerCacheInvalidatedalso advances that token while leavingconnectionStatusasconnected.”
This is valid: lastToolsRefresh now orders success, failure, OAuth, and invalidation, so a newer value alone cannot prove that a success won. This is also the third review cycle in the same publication-race class, so I’m pausing before another automatic patch.
Two bounded options:
- Require both a newer
lastToolsRefreshand a newerlastConnectedbefore preserving connected status. This uses existing fields; invalidation does not advancelastConnected. - Persist an explicit publication outcome/generation and make the route read that. This is clearer long-term but requires a larger schema/API change.
My lean is option 1 for this PR: it closes the reported race without a migration, while retaining the mutation token as ordering rather than overloading it as success provenance.
2 participants
Summary
Authenticated MCP servers now keep the newest valid discovery result instead of resurfacing stale tools, stale status, or an OAuth prompt from an older request. Static-header authorization failures remain header-auth failures, while OAuth servers that do not advertise dynamic client registration return an actionable 422 directing the user to configure a token.
Discovery publication is ordered end to end. Redis and memory cache adapters allocate a monotonic millisecond mutation token; conditional cache publication and the database
lastToolsRefreshCAS use that exact same token, so retries cannot make the two layers choose different winners. A credential-safe SHA-256 discovery revision covers every connection-affecting field; after a status CAS race, discovery re-reads that revision so metadata-only edits keep valid live tools while connection changes invalidate stale results. Failure counters recompute through an optimistic JSONB CAS when a success races them, and a full cache clear invalidates existing mutation owners before deleting entries.Mutation ownership is retried once. If ordering or an atomic cache transition remains unavailable, freshly fetched tools are returned to the caller but unordered cache/database state is not persisted. If another mutation supersedes the request, discovery reloads the winning positive-cache entry; stale-configuration results with no valid winner still fail closed. Explicit configuration and list-change invalidation remains best-effort during cache degradation.
Bulk discovery awaits each publication before returning, failures persist a zero tool count, and refresh consumes the discovery publication metadata. It re-reads current server metadata, reports unavailable or winner-cache results with the live tool count, and compares the published
lastToolsRefreshtoken instead of a wall-clock timestamp before applying a failure. Raw upstream errors are replaced with allowlisted UI messages plus shared credential-free structural diagnostics.Investigation confirmed
@modelcontextprotocol/sdk1.29 already capturesMcp-Session-Idfrom initialize and replays it ontools/list. The client records the request phase and whether a session id was present without logging the identifier or credentials.Related: #5665 and #5595.
Type of Change
Testing
DATABASE_URL=postgresql://postgres:postgres@localhost:5432/sim bun run --cwd apps/sim test -- lib/mcp 'app/api/mcp/servers/[id]/refresh/route.test.ts'— 357 tests passed across 19 files.bun run check:api-validation:strict— passed with no boundary-policy drift.git diff --check— passed.tools/listwith 47 tools using both the normal and SSRF-pinned transports; the SDK transport carried the session id.bun run --cwd apps/sim type-checkcurrently reaches only unrelated shared-worktree dependency errors in fork audit events,recordAuditBatch, and@google-cloud/storage; no MCP TypeScript errors are reported.Reviewer focus: the unified cache/database publication token, explicit discovery revision, status JSONB CAS loop, Redis conditional-mutation scripts, and degraded-cache response path are the highest-value concurrency review points.
Checklist
Screenshots/Videos
N/A — API and backend behavior only.