fix: handle non-success HTTP responses in MCP gallery query (fixes #321963)#321966
Open
vs-code-engineering[bot] wants to merge 1 commit into
Open
fix: handle non-success HTTP responses in MCP gallery query (fixes #321963)#321966vs-code-engineering[bot] wants to merge 1 commit into
vs-code-engineering[bot] wants to merge 1 commit into
Conversation
…21963) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
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.
Summary
Telemetry bucket
8c52e78a-53e7-b4d1-4111-f24a79ae8cda(unhandlederror—Error: Server returned 404) fires when the MCP gallery server responds to a server-listing query with a non-success HTTP status. InMcpGalleryService.queryRawGalleryMcpServers, the response is passed toasJson(context)with no status-code guard and outside the surrounding network-errortry/catch.asJsonthrowsServer returned <status>for any non-2xx response, and because that call sits outside the guarded path the throw escapes as an unhandled error into telemetry.The sibling method
getMcpServerin the same file already guards 4xx responses before callingasJson;queryRawGalleryMcpServerswas missing the equivalent guard. The fix adds a guard clause that covers all non-success statuses (4xx and 5xx), logs the failure, and degrades gracefully to an empty result — matching the existing network-error handling in the same method.Observed at version 1.125.0 (shipped commit
93cfdd489c3b228840d0f86ec77c3636277c93ea): 423 hits / 180 users, first seen 2026-06-17.Fixes #321963
Recommended reviewer:
@sandy081Culprit Commit
Most relevant commit:
1a2b823d90ed7eebf6f03b5be2525d61ef4fbe66(2026-05-14) —fix: handle network errors in MCP gallery query gracefully (fixes #315094) (#315096), authored by the release automation accountvs-code-engineering[bot](co-authored by Copilot andbryanchen-d). It wrappedrequestService.request(...)in atry/catchto gracefully handle network/transport failures, but left the response-body parse (asJson) outside that guarded path and added no HTTP status-code check. As a result a response that is successfully received but carries a non-2xx status (e.g. 404) still reachesasJsonand throws.Classification: the unhandled-non-success path is effectively pre-existing / latent —
asJsonhas always thrown on non-success status. The bucket appears only at 1.125.0 (single release, first seen 2026-06-17), which points to a server- or manifest-side change as the proximate spike trigger: the query URL is resolved from the gallery manifest viagetMcpGalleryManifestResourceUri(...), which is external to this repository. The commit above is the closest contributing change in this repo because it established graceful degradation for this exact method but stopped short of covering HTTP status failures.Code Flow
flowchart TD A["McpGalleryService.query / getMcpServersFromGallery"] --> B["queryRawGalleryMcpServers()"] B --> C["requestService.request(GET gallery query URL)"] C -->|network/transport error| D["catch: logService.error + return empty result"] C -->|HTTP response received| E["context (res.statusCode = 404)"] E --> F["await asJson(context)"] F --> G{"isSuccess(context)?"} G -->|no| H["throw new Error('Server returned 404')"] H --> I["unhandled error → telemetry bucket 8c52e78a..."] G -->|yes| J["parse JSON body → serialize result"] E -. fix adds guard here .-> K["if (!isSuccess(context)) → logService.error + return empty result"]Affected Files
src/vs/platform/mcp/common/mcpGalleryService.tsisSuccessto the import from../../request/common/request.js.queryRawGalleryMcpServers(immediately after the requesttry/catch, beforeawait asJson(context)) that returns an empty result and logs when the response is not a success status.Repro Steps
IMcpGalleryService.query→queryRawGalleryMcpServers).queryRawGalleryMcpServerscallsasJson(context), which throwsError: Server returned 404because the status is not a success status.try/catch, so it propagates as an unhandled error and is recorded in telemetry (bucket8c52e78a-53e7-b4d1-4111-f24a79ae8cda).How the Fix Works
Chosen approach (
src/vs/platform/mcp/common/mcpGalleryService.ts): Insert a guard clause immediately after the requesttry/catchand beforeconst data = await asJson(context);(the producer site that consumes the unguarded response):isSuccessis the exact predicateasJsonevaluates internally before deciding to throw, so checking it at the producer guarantees the throw is never reached. On a non-success response the method now logs vialogService.error(preserving the diagnostic signal rather than silencing it) and returns the same empty result ({ servers: [], metadata: { count: 0 } }) the method already returns for network errors and empty payloads. This places the fix at the data producer that consumes the response — not at the shared crash site inrequest.ts(the data-producer principle) — and uses a guard clause rather than atry/catchthat would mask the error from telemetry (the prefer-guard-clauses principle). It also brings the method in line with the siblinggetMcpServer, which already guards before callingasJson.Alternatives considered:
getMcpServerdoes): rejected because 5xx gallery responses would still throw the same unhandled error;!isSuccesscovers all non-2xx, matching exactly the conditionasJsonthrows on.asJsonin atry/catch: rejected because it would hide the failure from the diagnostic/telemetry path instead of preventing the throw at the producer, violating the no-silencing principle.asJson/request.ts(the crash site): rejected — that is the shared crash site used by many callers, and changing its throw contract would mask legitimate failures elsewhere (do not fix at the crash site).Recommended Owner
@sandy081— owns MCP (MCP Gallery, MCP Management) and Extensions Management per the team working-areas registry, and authored nearly all recent commits tomcpGalleryService.ts(actively maintained, most recent commits dated 2026-06-18). The culprit commit's author is the release automation accountvs-code-engineering[bot], which is not a routable human owner, so ownership falls to the feature-area maintainer.