Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the WebSocket chat transport to expose handshake request IDs (including GitHub’s request id) to higher layers, so callers can attach that metadata to errors/telemetry.
Changes:
- Make
IChatWebSocketManager.getOrCreateConnectionsynchronous and require callers to explicitlyconnect()the returned connection. - Expose
requestIdandgitHubRequestIdonIChatWebSocketConnectionand use them in WebSocket telemetry. - Attach
gitHubRequestIdto thrown errors whenChatMLFetcherfails to establish a WebSocket connection.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/platform/networking/node/test/chatWebSocketManager.spec.ts | Updates test helper to explicitly connect() before sending requests. |
| src/platform/networking/node/chatWebSocketManager.ts | Refactors connection lifecycle (caller-managed connect), and surfaces request IDs for telemetry/consumers. |
| src/extension/prompt/node/chatMLFetcher.ts | Connects explicitly and propagates GitHub request id on connect failure. |
Comments suppressed due to low confidence (2)
src/platform/networking/node/chatWebSocketManager.ts:137
getOrCreateConnectiononly reuses an existing connection whenisOpenis true. With the new API where callers initiateconnect(), a second call for the same conversation/turn while the first connection is still connecting will dispose the in-flight connection and create a new one, which can lead to duplicate connection attempts and leaked sockets/promises. Consider reusing the existing connection while it is connecting (or tracking a per-connectionconnect()promise/state) instead of disposing it just because it isn't open yet.
This issue also appears on line 335 of the same file.
// Reuse the connection if it's for the same turn and still open.
if (existing?.turnId === turnId && existing.connection.isOpen) {
this._logService.debug(`[ChatWebSocketManager] Reusing connection for conversation ${conversationId} turn ${turnId}`);
return existing.connection;
}
src/platform/networking/node/chatWebSocketManager.ts:340
- With
getOrCreateConnectionnow returning an unconnected object, it becomes more likely thatconnect()could be invoked multiple times (orgetOrCreateConnectioncalled again) while a connection attempt is still in progress. Currentlyconnect()does not guard against theConnectingstate (onlyOpen), so concurrent calls can trigger multiplecreateResponsesWebSocket()attempts and multiple event listener sets. Consider makingconnect()idempotent by returning the same in-flight promise while connecting, and ensure disposal during setup prevents the underlying socket from being left open.
const onClose = (event: CloseEvent) => {
cleanup();
this._state = ConnectionState.Closed;
this._responseHeaders = connection.responseHeaders;
this._responseStatusCode = connection.responseStatusCode;
this._responseStatusText = connection.responseStatusText;
pwang347
approved these changes
Mar 24, 2026
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.
microsoft/vscode#298236