Fix race condition in resolveAutoModeEndpoint causing duplicate telem… - #5027
Fix race condition in resolveAutoModeEndpoint causing duplicate telem…#5027abadawi591 wants to merge 6 commits into
Conversation
…etry Concurrent calls to resolveAutoModeEndpoint for the same conversation (e.g., from getAvailableTools, buildPrompt, and fetch within a single tool-calling round) could all enter _tryRouterSelection before the first call wrote to _autoModelCache, causing the automode.routerFallback telemetry event to fire 2-5x per routing decision instead of once. Add per-conversation promise coalescing: if a resolution is already in-flight for a conversationId, subsequent callers await the same promise instead of starting a new one. This also avoids redundant CAPI token fetches.
There was a problem hiding this comment.
Pull request overview
This PR addresses a race in AutomodeService.resolveAutoModeEndpoint where concurrent calls for the same conversation can all observe an empty cache and redundantly perform routing/token minting, causing duplicate automode.routerFallback telemetry emissions and extra CAPI requests. The fix adds per-conversation promise coalescing so concurrent callers share a single in-flight resolution.
Changes:
- Add
_pendingResolutionsmap to coalesce concurrentresolveAutoModeEndpointcalls perconversationId. - Clear
_pendingResolutionson auth change and dispose alongside existing caches. - Add Vitest coverage for concurrent coalescing, single CAPI request behavior, error propagation, and retry after failure.
Show a summary per file
| File | Description |
|---|---|
| src/platform/endpoint/node/automodeService.ts | Adds in-flight promise coalescing to prevent duplicate router/token work and telemetry emissions. |
| src/platform/endpoint/node/test/automodeService.spec.ts | Adds new tests validating concurrent coalescing behavior and failure/retry semantics. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 3
| sessionId: 'session-concurrent' | ||
| }; | ||
|
|
||
| const [result1, result2, result3] = await Promise.all([ |
There was a problem hiding this comment.
If I understand correctly, shouldn't the real test here be multiple different prompts being asked? Concurrent calls with the same conversationId only breaks our logic if they result in different answers.
My confusion may stem from my lack of understanding on what the mockApiResponse(['gpt-4o', 'gpt-4o-mini']); line does
There was a problem hiding this comment.
Good question! The concurrent calls in the real bug all carry the same prompt and same chatRequest.
they're the same user message being resolved multiple times within a single tool-calling round (for getAvailableTools, buildPrompt, token counting, fetch). They're not different user messages arriving at the same time.
So the test is accurate to the real scenario: same chatRequest, same sessionId, same prompt, fired concurrently.
To clarify mockApiResponse(['gpt-4o', 'gpt-4o-mini']): it stubs the CAPI token endpoint to return those as available_models. It controls which models the auto-mode service can choose from — it's not the router response (that would be mockRouterResponse).
Different prompts within the same conversation are already handled by the skipRouter cache (turnCount > 0 → skip), which is unrelated to this fix. This PR only addresses the race window on the first resolution of a conversation.
There was a problem hiding this comment.
Dug a little deeper into this one 👇
Duplicate automode.routerFallback telemetry from concurrent getChatEndpoint calls
Symptom
automode.routerFallback fires 5× per routing decision instead of once. Bursts appear at the same sub-second timestamp, inflating telemetry counts and causing redundant CAPI token fetches.
Cause
toolCallingLoop.ts calls getChatEndpoint(request) up to 5 times per tool-calling round (for tools, prompt, tokenizer, fetch). On the first resolution of a conversation, these concurrent async calls all see an empty _autoModelCache, all enter _tryRouterSelection, and all emit telemetry before any of them writes the cache.
This pattern dates back to the initial repo when getChatEndpoint was cheap and stateless. As auto-mode features were layered on, existing cross-turn dedup never covered the within-turn concurrent race:
| Date | Hash | Author | Change |
|---|---|---|---|
| 2025-06-27 | af813ee | @kieferrm | Initial repo — multiple getChatEndpoint calls in runOne(), no auto-mode |
| 2025-10-30 | 5aafc56 | @lramos15 | Auto-mode service created, calls now async |
| 2026-01-16 | fe88d62 | @TylerLeonhardt | Model router added, calls now trigger router API |
| 2026-01-20 | 5388442 | @Copilot/@TylerLeonhardt | lastRoutedPrompt dedup — fixes cross-turn but not within-turn race |
| 2026-02-05 | 70b2b4c | @lramos15 | hasImage gate + vision fallback |
| 2026-02-19 | 8f0a331 | @nickthecook | automode.routerFallback telemetry added — made the duplication observable |
| 2026-03-25 | e8d3c5b | @lramos15 | skipRouter/turnCount, conversation-level routing finalized |
Minimal fix (this PR)
Promise coalescing in resolveAutoModeEndpoint: if a resolution is already in-flight for a conversationId, subsequent callers share the existing promise instead of starting a new one.
Proper fix (separate PR)
Resolve the endpoint once at the top of runOne() and pass it down to getAvailableTools, buildPrompt, and fetch. See abadawi/resolve-endpoint-once-per-round. This was attempted in 5388442bf (added _cachedEndpointForTurn to ToolCallingLoop) but reverted in favor of dedup inside automodeService. Now implemented as a standalone refactor.
cc @lramos15 @TylerLeonhardt — adding you both since you've touched this area extensively. Would appreciate a sanity check on these findings.
The finally() cleanup now checks that the map still holds the same promise before deleting. This prevents a race where an auth change clears the map, a new resolution starts, and then the old promise's finally() deletes the newer entry.
… test Assert on model and modelProvider instead of toBe, so the test is resilient to implementation changes that return clones/wrappers.
Count only AutoModels token requests instead of all makeRequest calls, so the test stays correct if router or other CAPI calls are added.
Move the get/check/set logic out of the async resolveAutoModeEndpoint into a dedicated synchronous _singleFlight helper. This makes the no-await-between-check-and-set invariant structural (enforced by the non-async signature) rather than relying on a comment.
Use plain property access instead of typed destructuring in the filter callback to avoid TypeScript overload resolution errors.
|
Thanks for the contribution! This repository has been archived because the project has moved into the main VS Code repository. Could you please reopen/recreate this PR against: We’ll continue reviewing contributions there. Thanks! |
Description
Concurrent calls to
resolveAutoModeEndpointfor the same conversation race on an empty cache, causing theautomode.routerFallbacktelemetry event to fire 2–5× per routing decision instead of once. This inflates telemetry counts by ~2.1×.Root cause
In agent-mode, a single tool-calling round calls
getChatEndpoint(request)multiple times (forgetAvailableTools,buildPrompt, token counting, and fetch). On the first turn of a conversation, the cache entry doesn't exist yet. SinceresolveAutoModeEndpointis async, multiple callers read_autoModelCacheas empty before the first caller finishes and writes the result. Each enters_tryRouterSelection, emits telemetry, and redundantly fetches a CAPI token.Fix
Add per-conversation promise coalescing via a
_pendingResolutionsmap. If a resolution is already in-flight for aconversationId, subsequent callers await the same promise.Impact
automode.routerFallbackevent count reduced ~2× to accurate levelsFiles changed
automodeService.ts— promise coalescing wrapperautomodeService.spec.ts— 5 new tests (concurrent coalescing, single CAPI call, sequential still works, error propagation, retry after failure)