diff --git a/packages/cli/src/nonInteractiveCli.ts b/packages/cli/src/nonInteractiveCli.ts index 00b48be9540..45c3027232e 100644 --- a/packages/cli/src/nonInteractiveCli.ts +++ b/packages/cli/src/nonInteractiveCli.ts @@ -451,6 +451,13 @@ export async function runNonInteractive( } if (toolCallRequests.length > 0) { + // Re-check the abort signal before scheduling any tool calls. + // A SIGINT can fire after the stream loop has already buffered + // a tool-call event; without this guard the side effect would + // execute even though the user has cancelled. + if (abortController.signal.aborted) { + handleCancellationError(config); + } textOutput.ensureTrailingNewline(); const completedToolCalls = await scheduler.schedule( toolCallRequests, diff --git a/packages/core/src/core/turn.test.ts b/packages/core/src/core/turn.test.ts index cd769abb0dd..65972bd5ae6 100644 --- a/packages/core/src/core/turn.test.ts +++ b/packages/core/src/core/turn.test.ts @@ -237,6 +237,54 @@ describe('Turn', () => { expect(turn.getDebugResponses().length).toBe(1); }); + it('should not yield tool_call_request when signal is aborted before a function-call chunk is materialized', async () => { + // Regression test for #28091: a SIGINT can fire after the top-of-loop + // abort check but before we materialize tool calls from a chunk that + // arrived after cancellation. Without the extra guard the side effect + // runs after the user cancelled. + const abortController = new AbortController(); + const mockResponseStream = (async function* () { + yield { + type: StreamEventType.CHUNK, + value: { + candidates: [{ content: { parts: [{ text: 'First part' }] } }], + } as GenerateContentResponse, + }; + abortController.abort(); + yield { + type: StreamEventType.CHUNK, + value: { + functionCalls: [ + { + id: 'late-call', + name: 'late_after_cancel', + args: {}, + isClientInitiated: false, + }, + ], + } as unknown as GenerateContentResponse, + }; + })(); + mockSendMessageStream.mockResolvedValue(mockResponseStream); + + const events = []; + const reqParts: Part[] = [{ text: 'Test late tool call' }]; + for await (const event of turn.run( + { model: 'gemini' }, + reqParts, + abortController.signal, + )) { + events.push(event); + } + + expect(events).toEqual([ + { type: GeminiEventType.Content, value: 'First part' }, + { type: GeminiEventType.UserCancelled }, + ]); + // No tool call should have been queued for the scheduler. + expect(turn.pendingToolCalls).toEqual([]); + }); + it('should yield InvalidStream event if sendMessageStream throws InvalidStreamError', async () => { const error = new InvalidStreamError( 'Test invalid stream', diff --git a/packages/core/src/core/turn.ts b/packages/core/src/core/turn.ts index 74771c4478e..c37113a512d 100644 --- a/packages/core/src/core/turn.ts +++ b/packages/core/src/core/turn.ts @@ -364,8 +364,18 @@ export class Turn { yield { type: GeminiEventType.Content, value: text, traceId }; } - // Handle function calls (requesting tool execution) + // Handle function calls (requesting tool execution). + // + // Re-check the abort signal here: a SIGINT can fire between the + // top-of-loop check above and the moment we materialize tool calls + // from a chunk that arrived after cancellation. Without this guard + // a delayed function-call chunk is yielded to the scheduler and + // the side effect runs after the user cancelled. const functionCalls = resp.functionCalls ?? []; + if (functionCalls.length > 0 && signal?.aborted) { + yield { type: GeminiEventType.UserCancelled }; + return; + } for (const fnCall of functionCalls) { const event = this.handlePendingFunctionCall(fnCall, traceId); if (event) { diff --git a/packages/core/src/scheduler/scheduler.test.ts b/packages/core/src/scheduler/scheduler.test.ts index 9b5935cdbeb..4ba7304ceb0 100644 --- a/packages/core/src/scheduler/scheduler.test.ts +++ b/packages/core/src/scheduler/scheduler.test.ts @@ -612,6 +612,39 @@ describe('Scheduler (Orchestrator)', () => { expect(mockStateManager.dequeue).not.toHaveBeenCalled(); // Loop broke }); + it('should not enqueue or validate tool calls when scheduled with an already-aborted signal (regression #28091)', async () => { + // If a delayed tool-call chunk reaches the scheduler after the user + // cancelled, we must not invoke the tool registry / validators or + // enqueue the call — the late side effect would run before the queue + // processor's own abort check kicked in. + abortController.abort(); + + await scheduler.schedule(req1, signal); + + expect(mockStateManager.enqueue).not.toHaveBeenCalled(); + expect(mockStateManager.cancelAllQueued).toHaveBeenCalledWith( + 'Operation cancelled', + ); + }); + + it('should not leak queued batches when scheduled with an already-aborted signal (regression #28091)', async () => { + // The aborted-signal short-circuit must still drain the request + // queue via the `finally` block — otherwise a follow-up batch + // queued behind it would never resolve and hang the caller. + abortController.abort(); + + // First batch is rejected by the abort guard. + await scheduler.schedule(req1, signal); + + // A second batch scheduled with a fresh, non-aborted signal must + // still make progress (i.e. the scheduler must not be stuck in the + // "busy" state after the early-return path). + const fresh = new AbortController(); + await expect( + scheduler.schedule(req2, fresh.signal), + ).resolves.toBeDefined(); + }); + it('cancelAll() should cancel active call and clear queue', () => { const activeCall: ValidatingToolCall = { status: CoreToolCallStatus.Validating, diff --git a/packages/core/src/scheduler/scheduler.ts b/packages/core/src/scheduler/scheduler.ts index 76529c14d36..53d338e32e1 100644 --- a/packages/core/src/scheduler/scheduler.ts +++ b/packages/core/src/scheduler/scheduler.ts @@ -306,16 +306,29 @@ export class Scheduler { this.isProcessing = true; this.isCancelling = false; this.state.clearBatch(); - const currentApprovalMode = this.config.getApprovalMode(); - - // Sort requests to ensure Topic changes happen before actions in the same batch. - const sortedRequests = [...requests].sort((a, b) => { - if (a.name === UPDATE_TOPIC_TOOL_NAME) return -1; - if (b.name === UPDATE_TOPIC_TOOL_NAME) return 1; - return 0; - }); try { + // Guard against late-arriving requests scheduled after cancellation: + // if the caller's signal is already aborted, short-circuit instead of + // validating + enqueuing tool calls that would then execute their + // local side effects before the queue processor sees the abort. + // Match the cancellation pattern used by `_processNextItem`. + // Inside the `try` so the `finally` still drains the request queue — + // otherwise concurrent batches waiting in `requestQueue` would hang. + if (signal.aborted) { + this.state.cancelAllQueued('Operation cancelled'); + return this.state.completedBatch; + } + + const currentApprovalMode = this.config.getApprovalMode(); + + // Sort requests to ensure Topic changes happen before actions in the same batch. + const sortedRequests = [...requests].sort((a, b) => { + if (a.name === UPDATE_TOPIC_TOOL_NAME) return -1; + if (b.name === UPDATE_TOPIC_TOOL_NAME) return 1; + return 0; + }); + const toolRegistry = this.context.toolRegistry; const newCalls: ToolCall[] = sortedRequests.map((request) => { const enrichedRequest: ToolCallRequestInfo = {