fix: Keep model metadata cache until refresh succeeds #3294
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request fixes a race condition in model metadata caching by preserving the last-known-good cache until a refresh successfully completes. The changes prevent a transient "no models" window that could occur when authentication changes trigger a refresh.
Changes:
- Removed premature cache clearing on authentication changes, only resetting the fetch timestamp to force a refresh
- Implemented atomic cache updates by building new maps first, then swapping them in after successful hydration
- Added comprehensive comments explaining the rationale for preserving the cache
| const data: IModelAPIResponse[] = (await response.json()).data; | ||
| this._requestLogger.logModelListCall(requestId, requestMetadata, data); | ||
|
|
||
| // Build new maps and swap them in only once we have a fully-hydrated model list. | ||
| // This avoids a transient empty-model window during refresh. | ||
| const nextFamilyMap: Map<string, IModelAPIResponse[]> = new Map(); | ||
| const nextCompletionsFamilyMap: Map<string, IModelAPIResponse[]> = new Map(); | ||
| let nextCopilotBaseModel: IModelAPIResponse | undefined; | ||
|
|
||
| for (let model of data) { | ||
| model = await this._hydrateResolvedModel(model); | ||
| const isCompletionModel = isCompletionModelInformation(model); | ||
| // The base model is whatever model is deemed "fallback" by the server | ||
| if (model.is_chat_fallback && !isCompletionModel) { | ||
| this._copilotBaseModel = model; | ||
| nextCopilotBaseModel = model; | ||
| } | ||
| const family = model.capabilities.family; | ||
| const familyMap = isCompletionModel ? this._completionsFamilyMap : this._familyMap; | ||
| const familyMap = isCompletionModel ? nextCompletionsFamilyMap : nextFamilyMap; | ||
| if (!familyMap.has(family)) { | ||
| familyMap.set(family, []); | ||
| } | ||
| familyMap.get(family)?.push(model); | ||
| } | ||
|
|
||
| this._familyMap = nextFamilyMap; | ||
| this._completionsFamilyMap = nextCompletionsFamilyMap; | ||
| this._copilotBaseModel = nextCopilotBaseModel; |
There was a problem hiding this comment.
If the server returns an empty data array (line 267), the code will create empty maps and swap them in (lines 291-293), effectively clearing the model cache. This could happen during transient server issues or misconfigurations. Consider adding a check to only swap in the new maps if the data array is non-empty, similar to how the rate-limit case at line 260 checks for existing models before returning early. This would preserve the last-known-good cache even when the server returns an empty but successful response.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Prevent clearing the last-known-good cache on authentication changes to avoid transient empty-model windows. Ensure new maps are only swapped in after a successful model list fetch.