diff --git a/packages/core/src/core/loggingContentGenerator/loggingContentGenerator.test.ts b/packages/core/src/core/loggingContentGenerator/loggingContentGenerator.test.ts index 1378d34d75c..845a39c8c00 100644 --- a/packages/core/src/core/loggingContentGenerator/loggingContentGenerator.test.ts +++ b/packages/core/src/core/loggingContentGenerator/loggingContentGenerator.test.ts @@ -25,6 +25,7 @@ import { import { isTelemetrySdkInitialized } from '../../telemetry/index.js'; import { OpenAILogger } from '../../utils/openaiLogger.js'; import type OpenAI from 'openai'; +import { APIUserAbortError } from 'openai'; import { setGenAiUsageProvenance } from '../../telemetry/gen-ai-usage.js'; const activeOtelContext = vi.hoisted(() => ({ current: 'root' })); @@ -764,6 +765,276 @@ describe('LoggingContentGenerator', () => { ).toHaveBeenCalledWith(false); }); + it('does not emit an api_error event when the user cancelled the request', async () => { + // A user cancel surfaces as the SDK's APIUserAbortError with the caller's + // signal aborted. It must not be reported as a qwen-code.api_error — the + // span already records the cancellation. Regression for #8356 at the layer + // the bug is about: the util-level isAbortError fix alone does not gate this + // separate telemetry path. + const wrapped = createWrappedGenerator( + vi + .fn() + .mockRejectedValue( + new APIUserAbortError({ message: 'Request was aborted.' }), + ), + vi.fn(), + ); + const generator = new LoggingContentGenerator(wrapped, createConfig(), { + model: 'test-model', + authType: AuthType.USE_OPENAI, + enableOpenAILogging: false, + }); + const controller = new AbortController(); + controller.abort(); + const request = { + model: 'test-model', + contents: 'Hello', + config: { abortSignal: controller.signal }, + } as unknown as GenerateContentParameters; + + await expect( + generator.generateContent(request, 'prompt-cancel'), + ).rejects.toBeInstanceOf(APIUserAbortError); + + expect(logApiError).not.toHaveBeenCalled(); + }); + + it('still emits an api_error event for a real failure that is not a cancel', async () => { + // The gate is scoped to genuine user cancels: a non-abort failure, even + // with an unrelated aborted flag absent, must still be reported. + const wrapped = createWrappedGenerator( + vi.fn().mockRejectedValue(new Error('upstream-down')), + vi.fn(), + ); + const generator = new LoggingContentGenerator(wrapped, createConfig(), { + model: 'test-model', + authType: AuthType.USE_OPENAI, + enableOpenAILogging: false, + }); + const request = { + model: 'test-model', + contents: 'Hello', + } as unknown as GenerateContentParameters; + + await expect( + generator.generateContent(request, 'prompt-real-error'), + ).rejects.toThrow('upstream-down'); + + expect(logApiError).toHaveBeenCalledTimes(1); + }); + + it('still emits an api_error event for an abort-shaped error the user did not cause', async () => { + // Half of the gate's truth table: an abort-shaped error can also come from + // the network rather than the user. With the caller's signal never fired + // that is a genuine failure and must still be reported, so the gate cannot + // key on the error shape alone. + const wrapped = createWrappedGenerator( + vi + .fn() + .mockRejectedValue( + new APIUserAbortError({ message: 'Request was aborted.' }), + ), + vi.fn(), + ); + const generator = new LoggingContentGenerator(wrapped, createConfig(), { + model: 'test-model', + authType: AuthType.USE_OPENAI, + enableOpenAILogging: false, + }); + const controller = new AbortController(); + const request = { + model: 'test-model', + contents: 'Hello', + config: { abortSignal: controller.signal }, + } as unknown as GenerateContentParameters; + + await expect( + generator.generateContent(request, 'prompt-network-abort'), + ).rejects.toBeInstanceOf(APIUserAbortError); + + expect(logApiError).toHaveBeenCalledTimes(1); + }); + + it('still emits an api_error event for a real failure that races a cancel', async () => { + // The other half: a genuine upstream failure landing just as the user + // cancels is still a real error, so the gate cannot key on the aborted + // signal alone. + const wrapped = createWrappedGenerator( + vi.fn().mockRejectedValue(new Error('rate limited')), + vi.fn(), + ); + const generator = new LoggingContentGenerator(wrapped, createConfig(), { + model: 'test-model', + authType: AuthType.USE_OPENAI, + enableOpenAILogging: false, + }); + const controller = new AbortController(); + controller.abort(); + const request = { + model: 'test-model', + contents: 'Hello', + config: { abortSignal: controller.signal }, + } as unknown as GenerateContentParameters; + + await expect( + generator.generateContent(request, 'prompt-race'), + ).rejects.toThrow('rate limited'); + + expect(logApiError).toHaveBeenCalledTimes(1); + }); + + it('still emits an api_error event for an abort-shaped error when the request has no signal', async () => { + // The remaining truth-table cell: no `config.abortSignal` at all. Nobody + // could have cancelled, so an abort-shaped rejection here is a real + // failure. Pins against relaxing the signal check to `?? true`. + const wrapped = createWrappedGenerator( + vi + .fn() + .mockRejectedValue( + new APIUserAbortError({ message: 'Request was aborted.' }), + ), + vi.fn(), + ); + const generator = new LoggingContentGenerator(wrapped, createConfig(), { + model: 'test-model', + authType: AuthType.USE_OPENAI, + enableOpenAILogging: false, + }); + const request = { + model: 'test-model', + contents: 'Hello', + } as unknown as GenerateContentParameters; + + await expect( + generator.generateContent(request, 'prompt-no-signal-abort'), + ).rejects.toBeInstanceOf(APIUserAbortError); + + expect(logApiError).toHaveBeenCalledTimes(1); + }); + + it('does not emit an api_error event when the user cancels during stream setup', async () => { + // Cancelling before any chunk exists — Esc while the SDK request is still + // being established — rejects stream *setup* rather than the iterator, a + // separate call site from both the non-stream and mid-stream cases. Unlike + // a mid-stream abort the openai SDK does not swallow this one (the swallow + // lives in the stream iterator, not in the create call), so + // APIUserAbortError is the shape that genuinely arrives here. Dropping the + // signal argument at this site would re-emit the #8356 noise. + const controller = new AbortController(); + controller.abort(); + const wrapped = createWrappedGenerator( + vi.fn(), + vi + .fn() + .mockRejectedValue( + new APIUserAbortError({ message: 'Request was aborted.' }), + ), + ); + const generator = new LoggingContentGenerator(wrapped, createConfig(), { + model: 'test-model', + authType: AuthType.USE_OPENAI, + enableOpenAILogging: false, + }); + const request = { + model: 'test-model', + contents: 'Hello', + config: { abortSignal: controller.signal }, + } as unknown as GenerateContentParameters; + + await expect( + generator.generateContentStream(request, 'prompt-stream-setup-cancel'), + ).rejects.toBeInstanceOf(APIUserAbortError); + + expect(logApiError).not.toHaveBeenCalled(); + }); + + it('does not emit an api_error event when the user cancels mid-stream', async () => { + // The stream wrapper's catch is a third call site, distinct from the two + // above, so the gate needs its own coverage here. On the shape: the pinned + // openai SDK swallows a mid-stream abort (`core/streaming.mjs`: + // `if (isAbortError(e)) return;`), so an openai stream cancel ends the + // iterator normally and never reaches this catch at all. This case keeps + // the wrapper's contract honest for a provider that does propagate the SDK + // error; the shape that genuinely lands here is pinned in the test below. + const controller = new AbortController(); + const streamFn = vi.fn().mockResolvedValue( + (async function* () { + yield createResponse('resp-1', 'test-model', [{ text: 'partial' }]); + controller.abort(); + throw new APIUserAbortError({ message: 'Request was aborted.' }); + })(), + ); + const wrapped = createWrappedGenerator(vi.fn(), streamFn); + const generator = new LoggingContentGenerator(wrapped, createConfig(), { + model: 'test-model', + authType: AuthType.USE_OPENAI, + enableOpenAILogging: false, + }); + const request = { + model: 'test-model', + contents: 'Hello', + config: { abortSignal: controller.signal }, + } as unknown as GenerateContentParameters; + + const stream = await generator.generateContentStream( + request, + 'prompt-stream-cancel', + ); + + await expect( + (async () => { + for await (const _ of stream) { + // consume until the cancel surfaces + } + })(), + ).rejects.toBeInstanceOf(APIUserAbortError); + + expect(logApiError).not.toHaveBeenCalled(); + }); + + it('does not emit an api_error event for a DOMException-shaped mid-stream cancel', async () => { + // The shape a mid-stream cancel really takes on the @google/genai path: its + // SSE reader (`processStreamResponse`) has no abort special-case, so an + // abort during a read rejects with the fetch DOMException named + // 'AbortError' and propagates. This pins that the gate's `isAbortError` + // check covers the AbortError-name branch at the stream call site, not + // only the openai `APIUserAbortError`. + const controller = new AbortController(); + const streamFn = vi.fn().mockResolvedValue( + (async function* () { + yield createResponse('resp-1', 'test-model', [{ text: 'partial' }]); + controller.abort(); + throw new DOMException('The operation was aborted.', 'AbortError'); + })(), + ); + const wrapped = createWrappedGenerator(vi.fn(), streamFn); + const generator = new LoggingContentGenerator(wrapped, createConfig(), { + model: 'test-model', + authType: AuthType.USE_OPENAI, + enableOpenAILogging: false, + }); + const request = { + model: 'test-model', + contents: 'Hello', + config: { abortSignal: controller.signal }, + } as unknown as GenerateContentParameters; + + const stream = await generator.generateContentStream( + request, + 'prompt-stream-cancel-dom', + ); + + await expect( + (async () => { + for await (const _ of stream) { + // consume until the cancel surfaces + } + })(), + ).rejects.toMatchObject({ name: 'AbortError' }); + + expect(logApiError).not.toHaveBeenCalled(); + }); + it('forwards usage attached to the final response after it was yielded', async () => { const response = createResponse('r1', 'test-model', [{ text: 'a' }]); const streamFn = vi.fn().mockResolvedValue( diff --git a/packages/core/src/core/loggingContentGenerator/loggingContentGenerator.ts b/packages/core/src/core/loggingContentGenerator/loggingContentGenerator.ts index e119ca98d03..4df5ac701f9 100644 --- a/packages/core/src/core/loggingContentGenerator/loggingContentGenerator.ts +++ b/packages/core/src/core/loggingContentGenerator/loggingContentGenerator.ts @@ -49,6 +49,7 @@ import { getErrorMessage, getErrorStatus, getErrorType, + isAbortError, } from '../../utils/errors.js'; import { startLLMRequestSpan, @@ -262,7 +263,17 @@ export class LoggingContentGenerator implements ContentGenerator { error: unknown, model: string, prompt_id: string, + abortSignal?: AbortSignal, ): void { + // A user cancel is not an API error, so skip the api_error event entirely — + // the span already records the cancellation through its aborted status, so + // the signal isn't lost. Without this gate a user cancel is emitted as a + // `qwen-code.api_error` event with error_type `APIUserAbortError`, which is + // exactly what #8356 reported; `isAbortError` gating the debug log alone + // does not cover this separate telemetry path. + if (abortSignal?.aborted && isAbortError(error)) { + return; + } try { this._logApiError(responseId, durationMs, error, model, prompt_id); } catch (loggingError) { @@ -402,7 +413,14 @@ export class LoggingContentGenerator implements ContentGenerator { config: this.config, }); await context.with(spanContext, async () => { - this.safelyLogApiError('', durationMs, error, req.model, userPromptId); + this.safelyLogApiError( + '', + durationMs, + error, + req.model, + userPromptId, + req.config?.abortSignal, + ); try { await this.safelyLogOpenAIInteraction( await session.resolve(req), @@ -490,7 +508,14 @@ export class LoggingContentGenerator implements ContentGenerator { const durationMs = Date.now() - startTime; const observedFinishReasons = exchange.controller.finalize(false); context.with(spanContext, () => - this.safelyLogApiError('', durationMs, error, req.model, userPromptId), + this.safelyLogApiError( + '', + durationMs, + error, + req.model, + userPromptId, + req.config?.abortSignal, + ), ); const aborted = req.config?.abortSignal?.aborted ?? false; endLLMRequestSpan(llmSpan, { @@ -802,6 +827,7 @@ export class LoggingContentGenerator implements ContentGenerator { error, firstModelVersion || model, userPromptId, + abortSignal, ), ); const openaiRequest = await openaiRequestPromise; diff --git a/packages/core/src/utils/errors.test.ts b/packages/core/src/utils/errors.test.ts index ccac54eb2e5..ec24431ac42 100644 --- a/packages/core/src/utils/errors.test.ts +++ b/packages/core/src/utils/errors.test.ts @@ -5,6 +5,8 @@ */ import { describe, it, expect } from 'vitest'; +import { APIUserAbortError as AnthropicAPIUserAbortError } from '@anthropic-ai/sdk'; +import { APIConnectionError, APIUserAbortError } from 'openai'; import { getErrorMessage, isAbortError, isNodeError } from './errors.js'; describe('getErrorMessage cause unwrapping', () => { @@ -231,6 +233,44 @@ describe('isAbortError', () => { expect(isAbortError(networkError)).toBe(false); }); + + it('should return true for the OpenAI SDK APIUserAbortError (user cancel)', () => { + // The OpenAI SDK is the request path for auth_type=openai; a user cancel + // surfaces as APIUserAbortError. It does not set `.name` (stays 'Error') + // and has no ABORT_ERR code, so the checks above miss it. + const error = new APIUserAbortError({ message: 'Request was aborted.' }); + + // Assert the requirement (the name-based branch can't match it) rather than + // the SDK internal `.name === 'Error'`, which would break if OpenAI/Anthropic + // ever set a name without changing the correct behavior here. + expect(error.name).not.toBe('AbortError'); + expect(isAbortError(error)).toBe(true); + }); + + it('should return true for the Anthropic SDK APIUserAbortError', () => { + // Both SDKs this package depends on are Stainless-generated and share the + // abort class name, so the same check covers auth_type=anthropic. Pinned so + // the cross-SDK coverage is intentional rather than incidental. + const error = new AnthropicAPIUserAbortError({ + message: 'Request was aborted.', + }); + + // Assert the requirement (the name-based branch can't match it) rather than + // the SDK internal `.name === 'Error'`, which would break if OpenAI/Anthropic + // ever set a name without changing the correct behavior here. + expect(error.name).not.toBe('AbortError'); + expect(isAbortError(error)).toBe(true); + }); + + it('should return false for other SDK errors such as APIConnectionError', () => { + // Guards the abort match against being broadened (e.g. to any `API*` + // class): a transient connection failure must stay retryable, not be + // reported as a user cancellation. + const error = new APIConnectionError({ message: 'Connection error.' }); + + expect(error.constructor.name).toBe('APIConnectionError'); + expect(isAbortError(error)).toBe(false); + }); }); describe('isNodeError', () => { diff --git a/packages/core/src/utils/errors.ts b/packages/core/src/utils/errors.ts index 31332f5a499..0b0374f88d2 100644 --- a/packages/core/src/utils/errors.ts +++ b/packages/core/src/utils/errors.ts @@ -17,8 +17,9 @@ export function isNodeError(error: unknown): error is NodeJS.ErrnoException { } /** - * Check if the error is an abort error (user cancellation). - * This handles both DOMException-style AbortError and Node.js abort errors. + * Check if the error is an abort error (user cancellation). Handles + * DOMException-style AbortError, Node.js abort errors, and the provider SDKs' + * `APIUserAbortError` (matched by class name). */ export function isAbortError(error: unknown): boolean { if (!error || typeof error !== 'object') { @@ -35,6 +36,24 @@ export function isAbortError(error: unknown): boolean { return true; } + // A user cancel on a provider SDK path surfaces as `APIUserAbortError`. That + // class does not set `.name` (it stays 'Error') and carries no ABORT_ERR + // code, so the checks above miss it — leaving user cancels logged as + // api_errors and classified 'unknown' rather than 'abort'. Both SDKs this + // package depends on (`openai`, `@anthropic-ai/sdk`) are Stainless-generated + // and share this class name, so one check covers both. Match on the class + // name — as `getErrorType` below already does for SDK errors — so this + // provider-agnostic util needs no SDK import. The CLI bundle preserves class + // names (esbuild `keepNames: true` in esbuild.config.js); other bundles that + // minify without keepNames (e.g. vscode-ide-companion) do not, but they don't + // drive provider SDK requests, so the match holds where it runs. + if ( + error instanceof Error && + error.constructor?.name === 'APIUserAbortError' + ) { + return true; + } + return false; } diff --git a/packages/core/src/utils/retryErrorClassification.test.ts b/packages/core/src/utils/retryErrorClassification.test.ts index b43ff3967a1..000f2ce69fe 100644 --- a/packages/core/src/utils/retryErrorClassification.test.ts +++ b/packages/core/src/utils/retryErrorClassification.test.ts @@ -4,6 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { APIUserAbortError } from 'openai'; import { describe, expect, it } from 'vitest'; import { AuthType } from '../core/contentGenerator.js'; import { @@ -23,6 +24,17 @@ describe('classifyRetryError', () => { }); }); + it('classifies the OpenAI SDK APIUserAbortError as an abort, not unknown', () => { + // A user cancel on the auth_type=openai path surfaces as APIUserAbortError. + // It must be treated as an abort so retries stop and it is not logged as an + // api_error — otherwise it falls through to the 'unknown' classification. + expect( + classifyRetryError( + new APIUserAbortError({ message: 'Request was aborted.' }), + ), + ).toMatchObject({ kind: 'abort' }); + }); + it('classifies HTTP 503 as retryable rate limiting to match stream retry semantics', () => { expect( classifyRetryError({ status: 503, message: 'Provider overloaded' }),