improvement(mcp): push-driven settings freshness + lifecycle-audit fixes#5842
Conversation
…t over re-probing The list_changed → SSE push pipeline was already wired end-to-end but only mounted in the workflow-editor tool picker. Subscribe the settings MCP page to the same shared, reference-counted EventSource so tool changes reflect in real time there too — the reference-client model (discover once, refresh on push). With push now active on the settings page, raise MCP_SERVER_TOOLS_STALE_TIME from 30s to 5min (matching the server-side cache TTL) so revisiting the page leans on push + stored state instead of re-probing every connected server. There is no background poll (no refetchInterval) — this only affects refetch-on-visit-if-stale; real changes still arrive instantly via push.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryLow Risk Overview
The MCP OAuth callback wraps previously unbounded steps (
Tests add a FakeEventSource, clear shared SSE globals between runs, and count discovery calls explicitly now that the tools query also opens SSE. Reviewed by Cursor Bugbot for commit 1815e21. Configure here. |
Greptile SummaryThis PR improves MCP settings freshness and lifecycle handling. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (9): Last reviewed commit: "fix(mcp): drop the success-path status r..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…'t strand stale tools A dropped/reconnected EventSource may miss a tools_changed event during the gap, which — with a longer stale time — could leave the page showing old tools until a remount or manual refresh. Invalidate the workspace tools on reconnect (never on the first open), the standard resync-on-reconnect pattern for push clients.
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…losed reconcile Leaving the settings tab tears down the shared EventSource; remounting created a new connection whose first open was skipped, so a tools_changed fired while unsubscribed wasn't reconciled (the 5min stale time also won't refetch on remount). Track a per-workspace 'ever subscribed' flag: skip resync only on the session's first subscription (queries fetch fresh then); resync on the first open of any re-subscription and on every reconnect.
|
@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 f8fc2ed. Configure here.
…ry + fix stale comment Lifecycle-audit findings: - The per-server tools queryFn invalidated the server list only on error. A server that recovered via the stale re-probe returned fresh tools while the cached status still showed failed → 'tools present but row red' until the list independently refetched (a window the 5min stale-time widened). Now a successful probe against a server the cache still shows non-connected refreshes the list so the row clears. - Correct the keep/drop comment: tools drop once the stored status leaves 'connected' (disconnected/error), not at a 3-failure threshold.
|
@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 82d32c4. Configure here.
…uth callback steps Lifecycle-audit follow-ups (merged-code): - validateMcpServerSsrf's return docstring described 'pin subsequent connections', which no longer holds for the public path — the returned IP is a policy signal selecting the validate-at-connect guarded fetch; redirect/rebind safety comes from per-connect validation + followRedirectsGuarded, not pinning (only the self-hosted private carve-out still literally pins). Corrected so a future reader can't reintroduce a real pin from the misleading text. - The callback wrapped only the five post-auth steps in timedStep; the earlier loadOauthRowByState, getSession, and server SELECT (plus the provider_error clearState) were unbounded, contradicting the 'every awaited step bounded' invariant. Wrapped them so a wedged DB read surfaces as a labeled timeout.
waleedlatif1
changed the title
|
@cursor review |
…onnection error If the initial EventSource connection errors before opening (and the initial tools query fails with retry:false), the first successful onopen was skipped, leaving the query stale. Track erroredBeforeOpen so a first open that followed a connection error also resyncs — only a clean first subscription still skips.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
1 issue from previous review remains unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 0b0658c. Configure here.
The 5min stale time assumed push, but useMcpToolsEvents was only mounted in the settings page and the tool picker — other consumers of the tools query (dynamic args, tool selector, canvas block via useMcpToolsQuery/useMcpTools) had no subscription, so list_changed updates could stay invisible for the full window. Mount useMcpToolsEvents inside useMcpToolsQuery so every tools consumer gets real-time push from the shared, reference-counted connection — no consumer is left re-probing. Removed the now-redundant explicit mounts from the settings page and tool-input.
|
@cursor review |
…obalThis mcp.ts captures the connections Map and subscribed Set in module consts at import, so setting the globalThis property to undefined didn't reset what the module uses — subscription state leaked across tests. Clear the shared instances instead.
|
@cursor review |
The client-side serversList invalidation on a successful probe assumed the probe updated the stored status, but a server-side cache hit returns tools without touching status — so in the failed-cache-delete edge it fired a pointless refetch. The benefit (instant vs the 60s serversList stale-time for status recovery) doesn't justify the incorrect assumption; rely on the existing 60s stale-time + SSE push for status recovery instead. Keeps the corrected keep/drop comment.
|
@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 1815e21. Configure here.
1 participant
Summary
Aligns the MCP settings surface with the reference-client refresh model and folds in the fixes from a full end-to-end lifecycle audit. One clean PR covering the settings freshness work plus every audit finding worth acting on.
Push-driven freshness (settings page)
list_changed→ SSE push (useMcpToolsEvents) — the pipeline (connection-manager → pubsub →/api/mcp/events→ browserEventSource) already existed but was only mounted in the workflow-editor tool picker. Same shared, reference-counted connection. Real-time tool updates on the settings page.MCP_SERVER_TOOLS_STALE_TIME30s → 5min (matching the server-side cache TTL) so revisiting leans on push + stored state instead of re-probing. There is no background poll (norefetchInterval); this only affects refetch-on-visit-if-stale — real changes arrive instantly via push.EventSource, or leaving and returning to the tab (which tears the connection down), could miss atools_changedevent; a per-workspacesseEverSubscribedflag re-syncs on the first open of a re-subscription and on every reconnect, while skipping only the session's first subscribe (queries fetch fresh then).Lifecycle-audit fixes
queryFnrefreshed the server list only on error; a server recovering via the stale re-probe showed "tools present but row red" until the list independently refetched. A successful probe against a still-failed server now reconciles the status. (+ corrected an overstated keep/drop comment.)validateMcpServerSsrf's docstring described "pin subsequent connections," which no longer holds post validate-at-connect — the returned IP is a policy signal, and redirect/rebind safety comes from per-connect validation +followRedirectsGuarded, not pinning (only the self-hosted private carve-out still pins). Corrected so no one reintroduces a real pin from the text. Doc-only.loadOauthRowByState,getSession, the server SELECT, and theprovider_errorclearStatewere unbounded. Now wrapped intimedStepso a wedged DB read surfaces as a labeled timeout.Validated against Claude Code / LibreChat / OpenCode / VS Code + the MCP spec: discover-once +
list_changedpush, no timer polling, resync-on-reconnect. Three-pass adversarial lifecycle audit found no security escapes, count leaks, or double-release; the findings above are everything worth acting on.Type of Change
Testing
Checklist