From f2aeea52c4c3bb1870c9c04591ea31c71d756fe0 Mon Sep 17 00:00:00 2001 From: Ahmed Badawi Date: Tue, 7 Apr 2026 00:55:37 +0000 Subject: [PATCH 1/6] Fix race condition in resolveAutoModeEndpoint causing duplicate telemetry 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. --- src/platform/endpoint/node/automodeService.ts | 17 ++++ .../node/test/automodeService.spec.ts | 97 +++++++++++++++++++ 2 files changed, 114 insertions(+) diff --git a/src/platform/endpoint/node/automodeService.ts b/src/platform/endpoint/node/automodeService.ts index 1341b8d932..deb9b358ae 100644 --- a/src/platform/endpoint/node/automodeService.ts +++ b/src/platform/endpoint/node/automodeService.ts @@ -117,6 +117,7 @@ export interface IAutomodeService { export class AutomodeService extends Disposable implements IAutomodeService { readonly _serviceBrand: undefined; private readonly _autoModelCache: Map = new Map(); + private readonly _pendingResolutions: Map> = new Map(); private _reserveTokens: DisposableMap = new DisposableMap(); private readonly _routerDecisionFetcher: RouterDecisionFetcher; @@ -137,6 +138,7 @@ export class AutomodeService extends Disposable implements IAutomodeService { entry.tokenBank.dispose(); } this._autoModelCache.clear(); + this._pendingResolutions.clear(); const keys = Array.from(this._reserveTokens.keys()); this._reserveTokens.clearAndDisposeAll(); for (const location of keys) { @@ -152,6 +154,7 @@ export class AutomodeService extends Disposable implements IAutomodeService { entry.tokenBank.dispose(); } this._autoModelCache.clear(); + this._pendingResolutions.clear(); this._reserveTokens.dispose(); super.dispose(); } @@ -175,6 +178,20 @@ export class AutomodeService extends Disposable implements IAutomodeService { } const conversationId = chatRequest?.sessionResource?.toString() ?? chatRequest?.sessionId ?? 'unknown'; + + // Coalesce concurrent calls for the same conversation to avoid duplicate + // router calls and telemetry emissions. + const pending = this._pendingResolutions.get(conversationId); + if (pending) { + return pending; + } + + const promise = this._resolveAutoModeEndpointCore(chatRequest, conversationId, knownEndpoints); + this._pendingResolutions.set(conversationId, promise); + return promise.finally(() => this._pendingResolutions.delete(conversationId)); + } + + private async _resolveAutoModeEndpointCore(chatRequest: ChatRequest | undefined, conversationId: string, knownEndpoints: IChatEndpoint[]): Promise { const entry = this._autoModelCache.get(conversationId); const tokenBank = this._acquireTokenBank(entry, chatRequest?.location, conversationId); const token = await tokenBank.getToken(); diff --git a/src/platform/endpoint/node/test/automodeService.spec.ts b/src/platform/endpoint/node/test/automodeService.spec.ts index 3d66f3cece..1a291ed643 100644 --- a/src/platform/endpoint/node/test/automodeService.spec.ts +++ b/src/platform/endpoint/node/test/automodeService.spec.ts @@ -916,4 +916,101 @@ describe('AutomodeService', () => { ); }); }); + + describe('concurrent resolution coalescing', () => { + it('should return the same endpoint for concurrent calls with the same conversationId', async () => { + mockApiResponse(['gpt-4o', 'gpt-4o-mini']); + automodeService = createService(); + + const chatRequest: Partial = { + location: ChatLocation.Panel, + prompt: 'hello', + sessionId: 'session-concurrent' + }; + + const [result1, result2, result3] = await Promise.all([ + automodeService.resolveAutoModeEndpoint(chatRequest as ChatRequest, [mockChatEndpoint]), + automodeService.resolveAutoModeEndpoint(chatRequest as ChatRequest, [mockChatEndpoint]), + automodeService.resolveAutoModeEndpoint(chatRequest as ChatRequest, [mockChatEndpoint]), + ]); + + expect(result1).toBe(result2); + expect(result2).toBe(result3); + }); + + it('should make only one CAPI token request for concurrent calls', async () => { + mockApiResponse(['gpt-4o', 'gpt-4o-mini']); + automodeService = createService(); + + const chatRequest: Partial = { + location: ChatLocation.Panel, + prompt: 'hello', + sessionId: 'session-concurrent-token' + }; + + await Promise.all([ + automodeService.resolveAutoModeEndpoint(chatRequest as ChatRequest, [mockChatEndpoint]), + automodeService.resolveAutoModeEndpoint(chatRequest as ChatRequest, [mockChatEndpoint]), + ]); + + // Only one CAPI request for the token (not two) + const capiCalls = (mockCAPIClientService.makeRequest as ReturnType).mock.calls; + expect(capiCalls.length).toBe(1); + }); + + it('should allow a new resolution after the first one completes', async () => { + mockApiResponse(['gpt-4o', 'gpt-4o-mini']); + automodeService = createService(); + + const chatRequest: Partial = { + location: ChatLocation.Panel, + prompt: 'hello', + sessionId: 'session-sequential' + }; + + const result1 = await automodeService.resolveAutoModeEndpoint(chatRequest as ChatRequest, [mockChatEndpoint]); + + // Second call after first completes should succeed (hits cache, not pending map) + const result2 = await automodeService.resolveAutoModeEndpoint(chatRequest as ChatRequest, [mockChatEndpoint]); + expect(result1.model).toBe(result2.model); + }); + + it('should propagate errors to all concurrent callers', async () => { + (mockCAPIClientService.makeRequest as ReturnType).mockRejectedValue(new Error('token fetch failed')); + automodeService = createService(); + + const chatRequest: Partial = { + location: ChatLocation.Panel, + prompt: 'hello', + sessionId: 'session-error' + }; + + const results = await Promise.allSettled([ + automodeService.resolveAutoModeEndpoint(chatRequest as ChatRequest, [mockChatEndpoint]), + automodeService.resolveAutoModeEndpoint(chatRequest as ChatRequest, [mockChatEndpoint]), + ]); + + expect(results[0].status).toBe('rejected'); + expect(results[1].status).toBe('rejected'); + }); + + it('should allow retry after a failed concurrent resolution', async () => { + (mockCAPIClientService.makeRequest as ReturnType).mockRejectedValueOnce(new Error('transient failure')); + automodeService = createService(); + + const chatRequest: Partial = { + location: ChatLocation.Panel, + prompt: 'hello', + sessionId: 'session-retry' + }; + + // First call fails + await expect(automodeService.resolveAutoModeEndpoint(chatRequest as ChatRequest, [mockChatEndpoint])).rejects.toThrow(); + + // Pending promise should be cleared; retry should work + mockApiResponse(['gpt-4o', 'gpt-4o-mini']); + const result = await automodeService.resolveAutoModeEndpoint(chatRequest as ChatRequest, [mockChatEndpoint]); + expect(result.model).toBeDefined(); + }); + }); }); From 8c484210550ea1a7ebdd2979f6e0de94eb5c9129 Mon Sep 17 00:00:00 2001 From: Ahmed Badawi Date: Tue, 7 Apr 2026 06:51:15 +0000 Subject: [PATCH 2/6] Guard pending resolution cleanup against stale map entries 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. --- src/platform/endpoint/node/automodeService.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/platform/endpoint/node/automodeService.ts b/src/platform/endpoint/node/automodeService.ts index deb9b358ae..0c51841a89 100644 --- a/src/platform/endpoint/node/automodeService.ts +++ b/src/platform/endpoint/node/automodeService.ts @@ -186,9 +186,14 @@ export class AutomodeService extends Disposable implements IAutomodeService { return pending; } - const promise = this._resolveAutoModeEndpointCore(chatRequest, conversationId, knownEndpoints); - this._pendingResolutions.set(conversationId, promise); - return promise.finally(() => this._pendingResolutions.delete(conversationId)); + const corePromise = this._resolveAutoModeEndpointCore(chatRequest, conversationId, knownEndpoints); + const pendingPromise = corePromise.finally(() => { + if (this._pendingResolutions.get(conversationId) === pendingPromise) { + this._pendingResolutions.delete(conversationId); + } + }); + this._pendingResolutions.set(conversationId, pendingPromise); + return pendingPromise; } private async _resolveAutoModeEndpointCore(chatRequest: ChatRequest | undefined, conversationId: string, knownEndpoints: IChatEndpoint[]): Promise { From f8a535549e019d1c4f7db267c64c5bf7fbb0da73 Mon Sep 17 00:00:00 2001 From: Ahmed Badawi Date: Tue, 7 Apr 2026 06:52:32 +0000 Subject: [PATCH 3/6] Use property assertions instead of referential equality in coalescing test Assert on model and modelProvider instead of toBe, so the test is resilient to implementation changes that return clones/wrappers. --- src/platform/endpoint/node/test/automodeService.spec.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/platform/endpoint/node/test/automodeService.spec.ts b/src/platform/endpoint/node/test/automodeService.spec.ts index 1a291ed643..1cd2207b9f 100644 --- a/src/platform/endpoint/node/test/automodeService.spec.ts +++ b/src/platform/endpoint/node/test/automodeService.spec.ts @@ -918,7 +918,7 @@ describe('AutomodeService', () => { }); describe('concurrent resolution coalescing', () => { - it('should return the same endpoint for concurrent calls with the same conversationId', async () => { + it('should return consistent model selection for concurrent calls with the same conversationId', async () => { mockApiResponse(['gpt-4o', 'gpt-4o-mini']); automodeService = createService(); @@ -934,8 +934,9 @@ describe('AutomodeService', () => { automodeService.resolveAutoModeEndpoint(chatRequest as ChatRequest, [mockChatEndpoint]), ]); - expect(result1).toBe(result2); - expect(result2).toBe(result3); + expect(result1.model).toBe(result2.model); + expect(result2.model).toBe(result3.model); + expect(result1.modelProvider).toBe(result2.modelProvider); }); it('should make only one CAPI token request for concurrent calls', async () => { From 66897f7aa849cf04ef5605870dbbec4449bd6805 Mon Sep 17 00:00:00 2001 From: Ahmed Badawi Date: Tue, 7 Apr 2026 06:54:10 +0000 Subject: [PATCH 4/6] Filter CAPI call assertion to AutoModels request type Count only AutoModels token requests instead of all makeRequest calls, so the test stays correct if router or other CAPI calls are added. --- src/platform/endpoint/node/test/automodeService.spec.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/platform/endpoint/node/test/automodeService.spec.ts b/src/platform/endpoint/node/test/automodeService.spec.ts index 1cd2207b9f..3a8415d06e 100644 --- a/src/platform/endpoint/node/test/automodeService.spec.ts +++ b/src/platform/endpoint/node/test/automodeService.spec.ts @@ -954,9 +954,10 @@ describe('AutomodeService', () => { automodeService.resolveAutoModeEndpoint(chatRequest as ChatRequest, [mockChatEndpoint]), ]); - // Only one CAPI request for the token (not two) + // Only one CAPI token request for auto models should be made (not two) const capiCalls = (mockCAPIClientService.makeRequest as ReturnType).mock.calls; - expect(capiCalls.length).toBe(1); + const autoModelsCalls = capiCalls.filter(([, requestType]: [unknown, { type?: RequestType }]) => requestType?.type === RequestType.AutoModels); + expect(autoModelsCalls.length).toBe(1); }); it('should allow a new resolution after the first one completes', async () => { From 59875e8af73b523ab4da47cb84229123a0b0805d Mon Sep 17 00:00:00 2001 From: Ahmed Badawi Date: Tue, 7 Apr 2026 07:32:49 +0000 Subject: [PATCH 5/6] Extract coalescing into synchronous _singleFlight method 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. --- src/platform/endpoint/node/automodeService.ts | 37 +++++++++++++------ 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/src/platform/endpoint/node/automodeService.ts b/src/platform/endpoint/node/automodeService.ts index 0c51841a89..e474e5db85 100644 --- a/src/platform/endpoint/node/automodeService.ts +++ b/src/platform/endpoint/node/automodeService.ts @@ -179,21 +179,34 @@ export class AutomodeService extends Disposable implements IAutomodeService { const conversationId = chatRequest?.sessionResource?.toString() ?? chatRequest?.sessionId ?? 'unknown'; - // Coalesce concurrent calls for the same conversation to avoid duplicate - // router calls and telemetry emissions. - const pending = this._pendingResolutions.get(conversationId); - if (pending) { - return pending; - } + // Coalesce concurrent calls for the same conversation so we don't fire + // duplicate router requests or telemetry. Concurrent callers within a + // single turn always share the same prompt, so reusing the result is safe. + return this._singleFlight(conversationId, () => this._resolveAutoModeEndpointCore(chatRequest, conversationId, knownEndpoints)); + } - const corePromise = this._resolveAutoModeEndpointCore(chatRequest, conversationId, knownEndpoints); - const pendingPromise = corePromise.finally(() => { - if (this._pendingResolutions.get(conversationId) === pendingPromise) { - this._pendingResolutions.delete(conversationId); + /** + * Single-flight coalescer: if a resolution for `key` is already in progress, + * return that pending promise instead of starting a new one. + * + * This function MUST remain synchronous (not `async`). The atomicity of the + * `get -> check -> set` sequence relies on no `await` running between them, and + * keeping the function non-`async` makes that invariant structural rather than + * a comment a future edit might miss. + */ + private _singleFlight(key: string, fn: () => Promise): Promise { + const existing = this._pendingResolutions.get(key); + if (existing) { + return existing; + } + const promise = fn().finally(() => { + // Reference-equality guard: only clear if this entry is still ours. + if (this._pendingResolutions.get(key) === promise) { + this._pendingResolutions.delete(key); } }); - this._pendingResolutions.set(conversationId, pendingPromise); - return pendingPromise; + this._pendingResolutions.set(key, promise); + return promise; } private async _resolveAutoModeEndpointCore(chatRequest: ChatRequest | undefined, conversationId: string, knownEndpoints: IChatEndpoint[]): Promise { From e315b723c1061965090b99bebec0be9286e1ae78 Mon Sep 17 00:00:00 2001 From: Ahmed Badawi Date: Tue, 7 Apr 2026 08:14:03 +0000 Subject: [PATCH 6/6] Fix type error in CAPI call filter assertion Use plain property access instead of typed destructuring in the filter callback to avoid TypeScript overload resolution errors. --- src/platform/endpoint/node/test/automodeService.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/platform/endpoint/node/test/automodeService.spec.ts b/src/platform/endpoint/node/test/automodeService.spec.ts index 3a8415d06e..6239b7a5c3 100644 --- a/src/platform/endpoint/node/test/automodeService.spec.ts +++ b/src/platform/endpoint/node/test/automodeService.spec.ts @@ -956,7 +956,7 @@ describe('AutomodeService', () => { // Only one CAPI token request for auto models should be made (not two) const capiCalls = (mockCAPIClientService.makeRequest as ReturnType).mock.calls; - const autoModelsCalls = capiCalls.filter(([, requestType]: [unknown, { type?: RequestType }]) => requestType?.type === RequestType.AutoModels); + const autoModelsCalls = capiCalls.filter(call => call[1]?.type === RequestType.AutoModels); expect(autoModelsCalls.length).toBe(1); });