Skip to content
This repository was archived by the owner on May 20, 2026. It is now read-only.

Fix race condition in resolveAutoModeEndpoint causing duplicate telem… #5027

Open
abadawi591 wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Fix race condition in resolveAutoModeEndpoint causing duplicate telem… #5027

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f2aeea5
Fix race condition in resolveAutoModeEndpoint causing duplicate telem…
abadawi591 Apr 7, 2026
8c48421
Guard pending resolution cleanup against stale map entries
abadawi591 Apr 7, 2026
f8a5355
Use property assertions instead of referential equality in coalescing…
abadawi591 Apr 7, 2026
66897f7
Filter CAPI call assertion to AutoModels request type
abadawi591 Apr 7, 2026
59875e8
Extract coalescing into synchronous _singleFlight method
abadawi591 Apr 7, 2026
e315b72
Fix type error in CAPI call filter assertion
abadawi591 Apr 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions src/platform/endpoint/node/automodeService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ export interface IAutomodeService {
export class AutomodeService extends Disposable implements IAutomodeService {
readonly _serviceBrand: undefined;
private readonly _autoModelCache: Map<string, AutoModelCacheEntry> = new Map();
private readonly _pendingResolutions: Map<string, Promise<IChatEndpoint>> = new Map();
private _reserveTokens: DisposableMap<ChatLocation, AutoModeTokenBank> = new DisposableMap();
private readonly _routerDecisionFetcher: RouterDecisionFetcher;

Expand All @@ -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) {
Expand All @@ -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();
}
Expand All @@ -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);
Comment thread
abadawi591 marked this conversation as resolved.
Outdated
if (pending) {
return pending;
}

const promise = this._resolveAutoModeEndpointCore(chatRequest, conversationId, knownEndpoints);
this._pendingResolutions.set(conversationId, promise);
return promise.finally(() => this._pendingResolutions.delete(conversationId));
Comment thread
abadawi591 marked this conversation as resolved.
Outdated
}

private async _resolveAutoModeEndpointCore(chatRequest: ChatRequest | undefined, conversationId: string, knownEndpoints: IChatEndpoint[]): Promise<IChatEndpoint> {
const entry = this._autoModelCache.get(conversationId);
const tokenBank = this._acquireTokenBank(entry, chatRequest?.location, conversationId);
const token = await tokenBank.getToken();
Expand Down
97 changes: 97 additions & 0 deletions src/platform/endpoint/node/test/automodeService.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<ChatRequest> = {
location: ChatLocation.Panel,
prompt: 'hello',
sessionId: 'session-concurrent'
};

const [result1, result2, result3] = await Promise.all([

@jinu-jang jinu-jang Apr 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@abadawi591 abadawi591 Apr 7, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@abadawi591 abadawi591 Apr 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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);
});
Comment thread
abadawi591 marked this conversation as resolved.

it('should make only one CAPI token request for concurrent calls', async () => {
mockApiResponse(['gpt-4o', 'gpt-4o-mini']);
automodeService = createService();

const chatRequest: Partial<ChatRequest> = {
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<typeof vi.fn>).mock.calls;
expect(capiCalls.length).toBe(1);
Comment thread
abadawi591 marked this conversation as resolved.
Outdated
});

it('should allow a new resolution after the first one completes', async () => {
mockApiResponse(['gpt-4o', 'gpt-4o-mini']);
automodeService = createService();

const chatRequest: Partial<ChatRequest> = {
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<typeof vi.fn>).mockRejectedValue(new Error('token fetch failed'));
automodeService = createService();

const chatRequest: Partial<ChatRequest> = {
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<typeof vi.fn>).mockRejectedValueOnce(new Error('transient failure'));
automodeService = createService();

const chatRequest: Partial<ChatRequest> = {
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();
});
});
});
Loading