Handle CDN fetch failures in BYOK known-models lookup#312862
Handle CDN fetch failures in BYOK known-models lookup#312862
Conversation
… error Agent-Logs-Url: https://github.com/microsoft/vscode/sessions/0ef1e357-14a4-46b7-9b9c-5499bdd6ca30 Co-authored-by: bryanchen-d <41454397+bryanchen-d@users.noreply.github.com>
Agent-Logs-Url: https://github.com/microsoft/vscode/sessions/0ef1e357-14a4-46b7-9b9c-5499bdd6ca30 Co-authored-by: bryanchen-d <41454397+bryanchen-d@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens BYOK provider initialization in the Copilot extension by ensuring CDN fetch failures (for the shared “known models” list) and other async errors don’t surface as unhandled promise rejections, allowing providers to register with built-in defaults when the CDN is unreachable.
Changes:
- Adds
.catch(...)handlers at fire-and-forget_authChangecall sites to prevent unhandled rejections. - Wraps BYOK provider creation/registration in
_authChangewithtry/catchand resets internal registration state on failure to enable retries. - Makes
fetchKnownModelListresilient to fetch/JSON parse failures and unexpected response shapes, defaulting to{}.
Show a summary per file
| File | Description |
|---|---|
| extensions/copilot/src/extension/byok/vscode-node/byokContribution.ts | Adds error handling and safer parsing around known-models CDN fetch and BYOK provider registration lifecycle. |
Copilot's findings
Comments suppressed due to low confidence (1)
extensions/copilot/src/extension/byok/vscode-node/byokContribution.ts:110
fetchKnownModelListlogs "fetched successfully" even when the response is missing/invalid and you default to an empty list (after logging a warn). This can make logs misleading during outages or schema changes. Consider logging success only whenversion === 1andmodelInfois present, and using a different info/trace message when defaulting to{}.
if (!data || data.version !== 1 || !data.modelInfo) {
this._logService.warn('BYOK: Copilot Chat known models list is not in the expected format. Defaulting to empty list.');
knownModels = {};
} else {
knownModels = data.modelInfo;
}
this._logService.info('BYOK: Copilot Chat known models list fetched successfully.');
return knownModels;
- Files reviewed: 1/1 changed files
- Comments generated: 1
| if (byokEnabled && !this._byokProvidersRegistered) { | ||
| this._byokProvidersRegistered = true; | ||
| // Update known models list from CDN so all providers have the same list | ||
| const knownModels = await this.fetchKnownModelList(this._fetcherService); | ||
| if (this._store.isDisposed) { | ||
| return; | ||
| } | ||
| this._providers.set(OllamaLMProvider.providerName.toLowerCase(), instantiationService.createInstance(OllamaLMProvider, this._byokStorageService)); | ||
| this._providers.set(AnthropicLMProvider.providerName.toLowerCase(), instantiationService.createInstance(AnthropicLMProvider, knownModels[AnthropicLMProvider.providerName], this._byokStorageService)); | ||
| this._providers.set(GeminiNativeBYOKLMProvider.providerName.toLowerCase(), instantiationService.createInstance(GeminiNativeBYOKLMProvider, knownModels[GeminiNativeBYOKLMProvider.providerName], this._byokStorageService)); | ||
| this._providers.set(XAIBYOKLMProvider.providerName.toLowerCase(), instantiationService.createInstance(XAIBYOKLMProvider, knownModels[XAIBYOKLMProvider.providerName], this._byokStorageService)); | ||
| this._providers.set(OAIBYOKLMProvider.providerName.toLowerCase(), instantiationService.createInstance(OAIBYOKLMProvider, knownModels[OAIBYOKLMProvider.providerName], this._byokStorageService)); | ||
| this._providers.set(OpenRouterLMProvider.providerName.toLowerCase(), instantiationService.createInstance(OpenRouterLMProvider, this._byokStorageService)); | ||
| this._providers.set(AzureBYOKModelProvider.providerName.toLowerCase(), instantiationService.createInstance(AzureBYOKModelProvider, this._byokStorageService)); | ||
| this._providers.set(CustomOAIBYOKModelProvider.providerName.toLowerCase(), instantiationService.createInstance(CustomOAIBYOKModelProvider, this._byokStorageService)); | ||
| try { | ||
| // Update known models list from CDN so all providers have the same list | ||
| const knownModels = await this.fetchKnownModelList(this._fetcherService); | ||
| if (this._store.isDisposed) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Potential race: _authChange captures byokEnabled before awaiting fetchKnownModelList(). If auth changes to disabled while the await is in-flight, the disable branch in a subsequent _authChange call can clear registrations, but the original call will still proceed to register providers afterward (leaving BYOK providers enabled even though BYOK is now disabled). Consider guarding with a “generation” token / in-flight promise check (like CopilotCLIModels does) or re-checking isBYOKEnabled(...) after the await and bailing out if it changed.
BYOKContrib._authChangeinvokesfetchKnownModelList(a fetch tomain.vscode-cdn.net/extensions/copilotChat.json) fire-and-forget. When the CDN is unreachable,TypeError: fetch failedsurfaces asunhandlederror-fetch failedin the errors dashboard.Changes in
extensions/copilot/src/extension/byok/vscode-node/byokContribution.ts:fetchKnownModelList: try/catch around the fetch; logs and returns{}on failure so providers register with their built-in defaults. Also null/undefined-guards the parsed JSON before readingversion/modelInfo._authChange: try/catch around provider registration; on failure resets_byokProvidersRegisteredand clears registrations/providers so a subsequent auth change can retry cleanly.onDidAuthenticationChange):.catchhandlers with descriptive messages prevent any future async failure inside_authChangefrom escaping as an unhandled rejection.anyfrom.json()with{ version?: number; modelInfo?: Record<string, BYOKKnownModels> }.