From 4d9f5522ea03f706826c87584cd9393e23b157e0 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Wed, 19 Aug 2026 23:26:07 +0800 Subject: [PATCH 1/5] feat(api): add abort signal support for bedrock (completePrompt + createMessage) Wires external abort signals into the AWS Bedrock provider on both request paths. - completePrompt: merge options.abortSignal and options.timeoutMs via mergeAbortSignalAndTimeout (merged utils API, no cleanup) and forward the resulting signal as client.send abortSignal; sendOptions is undefined when no signal/timeout applies. - createMessage: bridge metadata?.abortSignal into the existing internal AbortController (pre-aborted guard + { once: true } listener), preserving the existing 10-minute request timeout. Tests: ports the reference spec additions (abort/timeout propagation to client.send, backward compatibility, empty response handling) and adds createMessage abort coverage (pre-aborted signal and mid-stream abort both reject with an error whose name === "AbortError"). --- src/api/providers/__tests__/bedrock.spec.ts | 367 ++++++++++++++++++++ src/api/providers/bedrock.ts | 22 +- 2 files changed, 388 insertions(+), 1 deletion(-) diff --git a/src/api/providers/__tests__/bedrock.spec.ts b/src/api/providers/__tests__/bedrock.spec.ts index fd9c92a438..9711ee5993 100644 --- a/src/api/providers/__tests__/bedrock.spec.ts +++ b/src/api/providers/__tests__/bedrock.spec.ts @@ -68,6 +68,7 @@ import { NodeHttpHandler } from "@smithy/node-http-handler" import { HttpProxyAgent } from "http-proxy-agent" import { HttpsProxyAgent } from "https-proxy-agent" +import { makeCreateMessageMetadata } from "../../../test-utils/api" import { clearAllMocks } from "../../../test-utils/reset" // Get access to the mocked functions @@ -1819,6 +1820,372 @@ describe("AwsBedrockHandler", () => { expect(isAdaptiveThinkingModel("anthropic.claude-3-5-sonnet-20241022-v2:0")).toBe(false) expect(isAdaptiveThinkingModel("amazon.nova-lite-v1:0")).toBe(false) }) + it("should pass abort signal through to client.send", async () => { + const mockSend = vi.fn() + + const handler = new AwsBedrockHandler({ + apiModelId: "anthropic.claude-3-5-sonnet-20241022-v2:0", + awsAccessKey: "test-access-key", + awsSecretKey: "test-secret-key", + awsRegion: "us-east-1", + }) + + // Set up the mock on the handler's client instance directly + const clientInstance = handler["client"] + expect(clientInstance).toBeDefined() + clientInstance.send = mockSend + + const controller = new AbortController() + mockSend.mockResolvedValueOnce({ + output: { message: { content: [{ type: "text", text: "response" }] }, stopReason: null }, + }) + + await handler.completePrompt("test prompt", { abortSignal: controller.signal }) + + expect(mockSend).toHaveBeenCalledWith(expect.any(Object), { abortSignal: controller.signal }) + }) + + it("should work without options (backward compatible)", async () => { + const mockSend = vi.fn() + + const handler = new AwsBedrockHandler({ + apiModelId: "anthropic.claude-3-5-sonnet-20241022-v2:0", + awsAccessKey: "test-access-key", + awsSecretKey: "test-secret-key", + awsRegion: "us-east-1", + }) + + const clientInstance = handler["client"] + expect(clientInstance).toBeDefined() + clientInstance.send = mockSend + + mockSend.mockResolvedValueOnce({ + output: { message: { content: [{ type: "text", text: "response" }] }, stopReason: null }, + }) + + const result = await handler.completePrompt("test prompt") + + expect(result).toBe("response") + expect(mockSend).toHaveBeenCalledWith(expect.any(Object), undefined) + }) + + it("completePrompt should pass timeoutMs through to client", async () => { + const mockSend = vi.fn() + + const handler = new AwsBedrockHandler({ + apiModelId: "anthropic.claude-3-5-sonnet-20241022-v2:0", + awsAccessKey: "test-access-key", + awsSecretKey: "test-secret-key", + awsRegion: "us-east-1", + }) + + const clientInstance = handler["client"] + clientInstance.send = mockSend + + mockSend.mockResolvedValueOnce({ + output: { message: { content: [{ type: "text", text: "response" }] }, stopReason: null }, + }) + + await handler.completePrompt("test prompt", { timeoutMs: 5000 }) + + expect(mockSend).toHaveBeenCalled() + // Verify the second argument (sendOptions) contains an abortSignal derived from timeoutMs + const sendOptions = mockSend.mock.calls[0][1] + expect(sendOptions).toBeDefined() + expect(sendOptions?.abortSignal).toBeDefined() + }) + + it("completePrompt should merge abortSignal and timeoutMs", async () => { + const mockSend = vi.fn() + + const handler = new AwsBedrockHandler({ + apiModelId: "anthropic.claude-3-5-sonnet-20241022-v2:0", + awsAccessKey: "test-access-key", + awsSecretKey: "test-secret-key", + awsRegion: "us-east-1", + }) + + const clientInstance = handler["client"] + clientInstance.send = mockSend + + mockSend.mockResolvedValueOnce({ + output: { message: { content: [{ type: "text", text: "response" }] }, stopReason: null }, + }) + + const controller = new AbortController() + await handler.completePrompt("test prompt", { abortSignal: controller.signal, timeoutMs: 5000 }) + + expect(mockSend).toHaveBeenCalled() + const sendOptions = mockSend.mock.calls[0][1] + expect(sendOptions?.abortSignal).toBeDefined() + }) + + it("should abort internal controller when external abortSignal is triggered", async () => { + const mockResult = { + output: { message: { content: [{ type: "text", text: "response" }] }, stopReason: null }, + } + const mockSend = vi.fn().mockResolvedValue(mockResult) + + const handler = new AwsBedrockHandler({ + apiModelId: "anthropic.claude-3-5-sonnet-20241022-v2:0", + awsAccessKey: "test-access-key", + awsSecretKey: "test-secret-key", + awsRegion: "us-east-1", + }) + + const clientInstance = handler["client"] + clientInstance.send = mockSend + + const controller = new AbortController() + let internalSignalCaptured: AbortSignal | undefined + + // Spy on the send call to capture the abortSignal + mockSend.mockImplementation(async (_command: unknown, options?: { abortSignal?: AbortSignal }) => { + internalSignalCaptured = options?.abortSignal + return mockResult + }) + + await handler.completePrompt("test prompt", { abortSignal: controller.signal }) + + expect(internalSignalCaptured).toBeDefined() + expect(internalSignalCaptured).toBeInstanceOf(AbortSignal) + + // Abort the external signal and verify it propagates to the captured signal + controller.abort() + await new Promise((resolve) => setTimeout(resolve, 10)) + expect(internalSignalCaptured?.aborted).toBe(true) + }) + + it("should abort immediately when signal is already aborted and timeoutMs > 0", async () => { + const mockSend = vi.fn() + + const handler = new AwsBedrockHandler({ + apiModelId: "anthropic.claude-3-5-sonnet-20241022-v2:0", + awsAccessKey: "test-access-key", + awsSecretKey: "test-secret-key", + awsRegion: "us-east-1", + }) + + const clientInstance = handler["client"] + clientInstance.send = mockSend + + mockSend.mockResolvedValueOnce({ + output: { message: { content: [{ type: "text", text: "response" }] }, stopReason: null }, + }) + + const controller = new AbortController() + controller.abort() // Pre-abort the signal + + await handler.completePrompt("test prompt", { abortSignal: controller.signal, timeoutMs: 5000 }) + + expect(mockSend).toHaveBeenCalled() + const sendOptions = mockSend.mock.calls[0][1] + expect(sendOptions?.abortSignal).toBeDefined() + expect(sendOptions?.abortSignal.aborted).toBe(true) + }) + + it("should return undefined sendOptions when timeoutMs is 0 and no signal", async () => { + const mockSend = vi.fn() + + const handler = new AwsBedrockHandler({ + apiModelId: "anthropic.claude-3-5-sonnet-20241022-v2:0", + awsAccessKey: "test-access-key", + awsSecretKey: "test-secret-key", + awsRegion: "us-east-1", + }) + + const clientInstance = handler["client"] + clientInstance.send = mockSend + + mockSend.mockResolvedValueOnce({ + output: { message: { content: [{ type: "text", text: "response" }] }, stopReason: null }, + }) + + await handler.completePrompt("test prompt", { timeoutMs: 0 }) + + expect(mockSend).toHaveBeenCalled() + const sendOptions = mockSend.mock.calls[0][1] + // When timeoutMs is 0 and no abortSignal, bedrock.ts returns undefined (no signal created) + expect(sendOptions).toBeUndefined() + }) + + it("should return empty string when response content is empty", async () => { + const mockSend = vi.fn() + + const handler = new AwsBedrockHandler({ + apiModelId: "anthropic.claude-3-5-sonnet-20241022-v2:0", + awsAccessKey: "test-access-key", + awsSecretKey: "test-secret-key", + awsRegion: "us-east-1", + }) + + const clientInstance = handler["client"] + clientInstance.send = mockSend + + mockSend.mockResolvedValueOnce({ + output: { message: { content: [{ type: "text", text: "" }] }, stopReason: null }, + }) + + const result = await handler.completePrompt("test prompt") + + expect(result).toBe("") + }) + + it("should return empty string when response text extraction throws after validation", async () => { + const mockSend = vi.fn() + + const handler = new AwsBedrockHandler({ + apiModelId: "anthropic.claude-3-5-sonnet-20241022-v2:0", + awsAccessKey: "test-access-key", + awsSecretKey: "test-secret-key", + awsRegion: "us-east-1", + }) + + const clientInstance = handler["client"] + clientInstance.send = mockSend + + let textAccessCount = 0 + const contentBlock = { + type: "text", + get text() { + textAccessCount++ + if (textAccessCount >= 3) { + throw new Error("text getter failed") + } + return "response" + }, + } + + mockSend.mockResolvedValueOnce({ + output: { message: { content: [contentBlock] }, stopReason: null }, + }) + + const result = await handler.completePrompt("test prompt") + + expect(result).toBe("") + }) + + it("should return empty string when response content array is empty", async () => { + const mockSend = vi.fn() + + const handler = new AwsBedrockHandler({ + apiModelId: "anthropic.claude-3-5-sonnet-20241022-v2:0", + awsAccessKey: "test-access-key", + awsSecretKey: "test-secret-key", + awsRegion: "us-east-1", + }) + + const clientInstance = handler["client"] + clientInstance.send = mockSend + + mockSend.mockResolvedValueOnce({ + output: { message: { content: [] }, stopReason: null }, + }) + + const result = await handler.completePrompt("test prompt") + + expect(result).toBe("") + }) + + it("createMessage should reject with an AbortError when the external signal is already aborted", async () => { + const handler = new AwsBedrockHandler({ + apiModelId: "anthropic.claude-3-5-sonnet-20241022-v2:0", + awsAccessKey: "test-access-key", + awsSecretKey: "test-secret-key", + awsRegion: "us-east-1", + }) + + let sendAbortSignal: AbortSignal | undefined + const mockSend = vi + .fn() + .mockImplementation(async (_command: unknown, options?: { abortSignal?: AbortSignal }) => { + sendAbortSignal = options?.abortSignal + if (options?.abortSignal?.aborted) { + const abortError = new Error("The operation was aborted") + abortError.name = "AbortError" + throw abortError + } + return { stream: [] } + }) + handler["client"].send = mockSend + + const controller = new AbortController() + controller.abort() + + const generator = handler.createMessage( + "You are a helpful assistant", + [{ role: "user", content: "Hello" }], + makeCreateMessageMetadata({ abortSignal: controller.signal }), + ) + + let thrown: unknown + try { + for await (const _chunk of generator) { + // error chunks are yielded before the rethrow + } + } catch (error) { + thrown = error + } + + expect(mockSend).toHaveBeenCalledTimes(1) + // The internal controller must have been aborted by the pre-aborted external signal + expect(sendAbortSignal).toBeDefined() + expect(sendAbortSignal?.aborted).toBe(true) + expect(thrown).toMatchObject({ name: "AbortError" }) + }) + + it("createMessage should abort the in-flight request when the external signal is aborted mid-stream", async () => { + const handler = new AwsBedrockHandler({ + apiModelId: "anthropic.claude-3-5-sonnet-20241022-v2:0", + awsAccessKey: "test-access-key", + awsSecretKey: "test-secret-key", + awsRegion: "us-east-1", + }) + + let internalSignal: AbortSignal | undefined + const mockSend = vi + .fn() + .mockImplementation((_command: unknown, options?: { abortSignal?: AbortSignal }) => { + internalSignal = options?.abortSignal + return new Promise((_resolve, reject) => { + internalSignal?.addEventListener( + "abort", + () => { + const abortError = new Error("The operation was aborted") + abortError.name = "AbortError" + reject(abortError) + }, + { once: true }, + ) + }) + }) + handler["client"].send = mockSend + + const controller = new AbortController() + + const generator = handler.createMessage( + "You are a helpful assistant", + [{ role: "user", content: "Hello" }], + makeCreateMessageMetadata({ abortSignal: controller.signal }), + ) + + const consumed = (async () => { + for await (const _chunk of generator) { + // ignore chunks + } + })() + + // Wait until the request is in flight and the internal signal is captured + await vi.waitFor(() => { + expect(internalSignal).toBeDefined() + }) + expect(internalSignal?.aborted).toBe(false) + + // Abort the external signal mid-flight; the stream must reject with an AbortError + controller.abort() + + await expect(consumed).rejects.toMatchObject({ name: "AbortError" }) + }) }) }) }) diff --git a/src/api/providers/bedrock.ts b/src/api/providers/bedrock.ts index 0d39e843c4..b81bcab22b 100644 --- a/src/api/providers/bedrock.ts +++ b/src/api/providers/bedrock.ts @@ -50,6 +50,7 @@ import { shouldUseReasoningBudget } from "../../shared/api" import { normalizeToolSchema } from "../../utils/json-schema" import { getSystemProxyUrl } from "../../utils/networkProxy" import type { SingleCompletionHandler, ApiHandlerCreateMessageMetadata, CompletePromptOptions } from "../index" +import { mergeAbortSignalAndTimeout } from "./utils/abort-signal" /************************************************************************************ * @@ -561,6 +562,18 @@ export class AwsBedrockHandler extends BaseProvider implements SingleCompletionH const controller = new AbortController() let timeoutId: NodeJS.Timeout | undefined + // Bridge external abort signal to our controller using the Bedrock pattern: + // - pre-aborted guard: check if already aborted before adding listener + // - { once: true }: remove listener after first abort to avoid leaks + const externalAbortSignal = metadata?.abortSignal + if (externalAbortSignal) { + if (externalAbortSignal.aborted) { + controller.abort() + } else { + externalAbortSignal.addEventListener("abort", () => controller.abort(), { once: true }) + } + } + try { timeoutId = setTimeout( () => { @@ -865,7 +878,14 @@ export class AwsBedrockHandler extends BaseProvider implements SingleCompletionH } const command = new ConverseCommand(payload) - const response = await this.client.send(command) + + // Build request options with abortSignal and/or timeoutMs. + // The shared helper keeps Bedrock aligned with other providers: + // positive timeout values create request-local cancellation, while + // zero/negative timeout values mean "no timeout". + const mergedAbortSignal = mergeAbortSignalAndTimeout(options?.abortSignal, options?.timeoutMs) + const sendOptions = mergedAbortSignal ? { abortSignal: mergedAbortSignal } : undefined + const response = await this.client.send(command, sendOptions) if ( response?.output?.message?.content && From 5cd34ea7b4687e202da4a63ce030b13fff02d3b4 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Thu, 20 Aug 2026 02:38:38 +0800 Subject: [PATCH 2/5] fix(api): address CodeRabbit review on bedrock createMessage abort lifecycle The external abort bridge listener was only removed when the signal actually aborted; a completed request left the listener (and its closure over the request controller) attached to the caller's signal. Make the controller request-local and detach the listener in a finally block so the external signal keeps no reference after the request ends (success or error). Test: createMessage regression - first request completes normally, a second request starts with a different external signal; the first signal's listener is removed on completion and aborting it late does not cancel the second stream. --- src/api/providers/__tests__/bedrock.spec.ts | 71 +++++++++++++++++++++ src/api/providers/bedrock.ts | 24 +++++-- 2 files changed, 88 insertions(+), 7 deletions(-) diff --git a/src/api/providers/__tests__/bedrock.spec.ts b/src/api/providers/__tests__/bedrock.spec.ts index 9711ee5993..7c33724a92 100644 --- a/src/api/providers/__tests__/bedrock.spec.ts +++ b/src/api/providers/__tests__/bedrock.spec.ts @@ -2186,6 +2186,77 @@ describe("AwsBedrockHandler", () => { await expect(consumed).rejects.toMatchObject({ name: "AbortError" }) }) + + it("createMessage should detach the external abort listener when the request completes", async () => { + const handler = new AwsBedrockHandler({ + apiModelId: "anthropic.claude-3-5-sonnet-20241022-v2:0", + awsAccessKey: "test-access-key", + awsSecretKey: "test-secret-key", + awsRegion: "us-east-1", + }) + + const streamChunks = [ + JSON.stringify({ contentBlockDelta: { delta: { text: "hello" } } }), + JSON.stringify({ messageStop: {} }), + ] + + let secondSendSignal: AbortSignal | undefined + const mockSend = vi + .fn() + .mockResolvedValueOnce({ stream: streamChunks }) + .mockImplementation(async (_command: unknown, options?: { abortSignal?: AbortSignal }) => { + secondSendSignal = options?.abortSignal + return { stream: streamChunks } + }) + handler["client"].send = mockSend + + // First request completes normally with its own external signal + const firstController = new AbortController() + const firstRemoveSpy = vi.spyOn(firstController.signal, "removeEventListener") + const firstGenerator = handler.createMessage( + "You are a helpful assistant", + [{ role: "user", content: "Hello" }], + makeCreateMessageMetadata({ abortSignal: firstController.signal }), + ) + const firstText = await (async () => { + let text = "" + for await (const chunk of firstGenerator) { + if (chunk.type === "text") { + text += chunk.text + } + } + return text + })() + expect(firstText).toBe("hello") + + // The bridge listener must be detached as soon as the request completes + expect(firstRemoveSpy).toHaveBeenCalledWith("abort", expect.any(Function)) + + // Second request starts with a DIFFERENT external signal + const secondController = new AbortController() + const secondGenerator = handler.createMessage( + "You are a helpful assistant", + [{ role: "user", content: "Hello" }], + makeCreateMessageMetadata({ abortSignal: secondController.signal }), + ) + const secondText = await (async () => { + let text = "" + for await (const chunk of secondGenerator) { + if (chunk.type === "text") { + text += chunk.text + } + } + return text + })() + expect(secondText).toBe("hello") + + // Aborting the first (already completed) signal late must not cancel the second request + firstController.abort() + expect(secondSendSignal).toBeDefined() + expect(secondSendSignal?.aborted).toBe(false) + + firstRemoveSpy.mockRestore() + }) }) }) }) diff --git a/src/api/providers/bedrock.ts b/src/api/providers/bedrock.ts index b81bcab22b..65a013ae95 100644 --- a/src/api/providers/bedrock.ts +++ b/src/api/providers/bedrock.ts @@ -558,33 +558,37 @@ export class AwsBedrockHandler extends BaseProvider implements SingleCompletionH ...(useServiceTier && { [SERVICE_TIER_KEY]: this.options.awsBedrockServiceTier }), } - // Create AbortController with 10 minute timeout - const controller = new AbortController() + // Create a request-local AbortController with 10 minute timeout. Keeping it + // request-local (and detaching the bridge listener in the finally block) means + // a completed request can never leave a stale listener on the caller's signal. + const requestController = new AbortController() let timeoutId: NodeJS.Timeout | undefined - // Bridge external abort signal to our controller using the Bedrock pattern: + // Bridge external abort signal to the request controller using the Bedrock pattern: // - pre-aborted guard: check if already aborted before adding listener // - { once: true }: remove listener after first abort to avoid leaks + let abortListener: (() => void) | undefined const externalAbortSignal = metadata?.abortSignal if (externalAbortSignal) { if (externalAbortSignal.aborted) { - controller.abort() + requestController.abort() } else { - externalAbortSignal.addEventListener("abort", () => controller.abort(), { once: true }) + abortListener = () => requestController.abort() + externalAbortSignal.addEventListener("abort", abortListener, { once: true }) } } try { timeoutId = setTimeout( () => { - controller.abort() + requestController.abort() }, 10 * 60 * 1000, ) const command = new ConverseStreamCommand(payload) const response = await this.client.send(command, { - abortSignal: controller.signal, + abortSignal: requestController.signal, }) if (!response.stream) { @@ -833,6 +837,12 @@ export class AwsBedrockHandler extends BaseProvider implements SingleCompletionH } else { throw new Error("An unknown error occurred") } + } finally { + // Detach the bridge listener once the request ends (success or error) so the + // external signal keeps no reference to this request's controller. + if (abortListener) { + externalAbortSignal?.removeEventListener("abort", abortListener) + } } } From 65e2a3371afaf1025f9d024102d7f7e3fd01794d Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Thu, 20 Aug 2026 03:57:51 +0800 Subject: [PATCH 3/5] fix(api): clear bedrock createMessage request timeout in finally When a caller stops consuming the generator early (break/destroy), the generator enters the finally block without reaching the stream-completion timeout-clearing path, leaving the 10-minute request timer active and retaining the request controller until it expires. Clear the timeout at the start of the finally block, before the abort-listener removal. Test: createMessage regression - the generator is terminated early mid-stream and the 10-minute timer handle (captured via typed spies on setTimeout/clearTimeout) is asserted to have been cleared. --- src/api/providers/__tests__/bedrock.spec.ts | 53 +++++++++++++++++++++ src/api/providers/bedrock.ts | 5 ++ 2 files changed, 58 insertions(+) diff --git a/src/api/providers/__tests__/bedrock.spec.ts b/src/api/providers/__tests__/bedrock.spec.ts index 7c33724a92..a9f476c90b 100644 --- a/src/api/providers/__tests__/bedrock.spec.ts +++ b/src/api/providers/__tests__/bedrock.spec.ts @@ -2257,6 +2257,59 @@ describe("AwsBedrockHandler", () => { firstRemoveSpy.mockRestore() }) + + it("createMessage should clear the request timeout when the generator is terminated early", async () => { + const handler = new AwsBedrockHandler({ + apiModelId: "anthropic.claude-3-5-sonnet-20241022-v2:0", + awsAccessKey: "test-access-key", + awsSecretKey: "test-secret-key", + awsRegion: "us-east-1", + }) + + // The stream yields one chunk and then hangs until released, so the + // generator is suspended mid-stream when it is terminated early. + let release: (() => void) | undefined + const pendingChunk = new Promise((resolve) => { + release = resolve + }) + const mockStream = async function* (): AsyncGenerator { + yield JSON.stringify({ contentBlockDelta: { delta: { text: "hello" } } }) + await pendingChunk + } + const mockSend = vi.fn().mockImplementation(() => Promise.resolve({ stream: mockStream() })) + handler["client"].send = mockSend + + const setTimeoutSpy = vi.spyOn(globalThis, "setTimeout") + const clearTimeoutSpy = vi.spyOn(globalThis, "clearTimeout") + + const generator = handler.createMessage( + "You are a helpful assistant", + [{ role: "user", content: "Hello" }], + makeCreateMessageMetadata(), + ) + + // Consume the first chunk; the generator is now suspended mid-stream + const firstResult = await generator.next() + expect(firstResult.done).toBe(false) + expect(firstResult.value).toEqual({ type: "text", text: "hello" }) + + // Terminate the generator early (before the stream completes) + const returnPromise = generator.return(undefined) + release?.() + await returnPromise + + // The 10-minute request timer scheduled by createMessage must have been cleared + const timerIndex = setTimeoutSpy.mock.calls.findIndex(([, delay]) => delay === 10 * 60 * 1000) + expect(timerIndex).toBeGreaterThanOrEqual(0) + const timeoutHandle: NodeJS.Timeout | undefined = setTimeoutSpy.mock.results[timerIndex]?.value + expect(timeoutHandle).toBeDefined() + expect(clearTimeoutSpy).toHaveBeenCalledWith(timeoutHandle) + + // Belt and braces: make sure no real 10-minute timer survives the test + setTimeoutSpy.mockRestore() + clearTimeoutSpy.mockRestore() + clearTimeout(timeoutHandle) + }) }) }) }) diff --git a/src/api/providers/bedrock.ts b/src/api/providers/bedrock.ts index 65a013ae95..cc9ab1c9d8 100644 --- a/src/api/providers/bedrock.ts +++ b/src/api/providers/bedrock.ts @@ -838,6 +838,11 @@ export class AwsBedrockHandler extends BaseProvider implements SingleCompletionH throw new Error("An unknown error occurred") } } finally { + // Clear the request timeout as soon as the generator ends. This also covers + // early termination by the caller (break/destroy), which bypasses the normal + // timeout-clearing path after the stream completes. + clearTimeout(timeoutId) + // Detach the bridge listener once the request ends (success or error) so the // external signal keeps no reference to this request's controller. if (abortListener) { From ce076d2124e17c77d3336690ab745bd4d259473b Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Thu, 20 Aug 2026 19:36:23 +0800 Subject: [PATCH 4/5] test(api): close changed-line coverage gaps in bedrock --- src/api/providers/__tests__/bedrock.spec.ts | 64 +++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/src/api/providers/__tests__/bedrock.spec.ts b/src/api/providers/__tests__/bedrock.spec.ts index a9f476c90b..29f6e156b9 100644 --- a/src/api/providers/__tests__/bedrock.spec.ts +++ b/src/api/providers/__tests__/bedrock.spec.ts @@ -2310,6 +2310,70 @@ describe("AwsBedrockHandler", () => { clearTimeoutSpy.mockRestore() clearTimeout(timeoutHandle) }) + + it("createMessage should abort the in-flight request when the 10 minute request timeout fires", async () => { + const handler = new AwsBedrockHandler({ + apiModelId: "anthropic.claude-3-5-sonnet-20241022-v2:0", + awsAccessKey: "test-access-key", + awsSecretKey: "test-secret-key", + awsRegion: "us-east-1", + }) + + let internalSignal: AbortSignal | undefined + const mockSend = vi + .fn() + .mockImplementation((_command: unknown, options?: { abortSignal?: AbortSignal }) => { + internalSignal = options?.abortSignal + return new Promise((_resolve, reject) => { + internalSignal?.addEventListener( + "abort", + () => { + const abortError = new Error("The operation was aborted") + abortError.name = "AbortError" + reject(abortError) + }, + { once: true }, + ) + }) + }) + handler["client"].send = mockSend + + // Capture the 10-minute request timer scheduled by createMessage + const setTimeoutSpy = vi.spyOn(globalThis, "setTimeout") + + const generator = handler.createMessage( + "You are a helpful assistant", + [{ role: "user", content: "Hello" }], + makeCreateMessageMetadata(), + ) + const consumed = (async () => { + for await (const _chunk of generator) { + // ignore chunks + } + })() + + // Wait until the request is in flight and the 10-minute timer is scheduled + let timeoutHandle: NodeJS.Timeout | undefined + let timeoutCallback: (() => void) | undefined + await vi.waitFor(() => { + const timerIndex = setTimeoutSpy.mock.calls.findIndex(([, delay]) => delay === 10 * 60 * 1000) + expect(timerIndex).toBeGreaterThanOrEqual(0) + expect(internalSignal).toBeDefined() + timeoutHandle = setTimeoutSpy.mock.results[timerIndex]?.value as NodeJS.Timeout + timeoutCallback = setTimeoutSpy.mock.calls[timerIndex]?.[0] as () => void + }) + expect(internalSignal?.aborted).toBe(false) + + // Fire the 10-minute request timeout: it must abort the request-local + // controller, which cancels the in-flight request with an AbortError. + timeoutCallback?.() + + await expect(consumed).rejects.toMatchObject({ name: "AbortError" }) + + // Belt and braces: make sure no real 10-minute timer survives the test + setTimeoutSpy.mockRestore() + clearTimeout(timeoutHandle) + }) }) }) }) From 178fdec42e46c22e4faa34d950f99ef7e467ced3 Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Tue, 1 Sep 2026 02:49:26 +0000 Subject: [PATCH 5/5] test(bedrock): strengthen abort signal coverage and add CodeRabbit rules --- .coderabbit.yaml | 7 + src/api/providers/__tests__/bedrock.spec.ts | 179 +++++++++++++++----- src/api/providers/bedrock.ts | 12 +- 3 files changed, 156 insertions(+), 42 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 6d8232e6ac..eddec3a0bf 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -56,6 +56,13 @@ reviews: Check cleanup and deterministic async behavior and prefer shared typed test helpers. Visible webview changes require a durable Playwright component snapshot; behavior-only changes do not. + Reject weak assertions on values that could take multiple forms: .toBeDefined() or + .toHaveBeenCalled() alone are not sufficient when the actual type, value, or object + identity is verifiable. For listener registration and removal, assert the same function + reference was added and removed (not expect.any(Function)). + Flag tests that assert in-flight behavior only after the call completes — these cannot + prove the behavior fires during execution. Check that describe block names match the + actual subjects of the tests they contain. - path: "apps/vscode-e2e/**" instructions: >- diff --git a/src/api/providers/__tests__/bedrock.spec.ts b/src/api/providers/__tests__/bedrock.spec.ts index 29f6e156b9..7733aa1031 100644 --- a/src/api/providers/__tests__/bedrock.spec.ts +++ b/src/api/providers/__tests__/bedrock.spec.ts @@ -1820,6 +1820,9 @@ describe("AwsBedrockHandler", () => { expect(isAdaptiveThinkingModel("anthropic.claude-3-5-sonnet-20241022-v2:0")).toBe(false) expect(isAdaptiveThinkingModel("amazon.nova-lite-v1:0")).toBe(false) }) + }) + + describe("completePrompt and createMessage: abort signal and listener lifecycle", () => { it("should pass abort signal through to client.send", async () => { const mockSend = vi.fn() @@ -1893,6 +1896,9 @@ describe("AwsBedrockHandler", () => { const sendOptions = mockSend.mock.calls[0][1] expect(sendOptions).toBeDefined() expect(sendOptions?.abortSignal).toBeDefined() + // The signal must not be aborted yet (i.e. a real timeout signal was created, + // not a no-op placeholder) + expect(sendOptions?.abortSignal.aborted).toBe(false) }) it("completePrompt should merge abortSignal and timeoutMs", async () => { @@ -1918,14 +1924,18 @@ describe("AwsBedrockHandler", () => { expect(mockSend).toHaveBeenCalled() const sendOptions = mockSend.mock.calls[0][1] expect(sendOptions?.abortSignal).toBeDefined() + // AbortSignal.any() returns a new composite object; if the merge were skipped and + // the external signal returned directly, this assertion would fail + expect(sendOptions?.abortSignal).not.toBe(controller.signal) + // The merged signal propagates the external abort + controller.abort() + expect(sendOptions?.abortSignal.aborted).toBe(true) }) - it("should abort internal controller when external abortSignal is triggered", async () => { - const mockResult = { - output: { message: { content: [{ type: "text", text: "response" }] }, stopReason: null }, - } - const mockSend = vi.fn().mockResolvedValue(mockResult) - + it("should abort the merged signal mid-flight when the external abortSignal fires", async () => { + // This test keeps client.send pending so it can verify that aborting the external + // signal while the request is in flight propagates through the composite signal + // and cancels the SDK call — a post-completion check cannot prove this. const handler = new AwsBedrockHandler({ apiModelId: "anthropic.claude-3-5-sonnet-20241022-v2:0", awsAccessKey: "test-access-key", @@ -1933,27 +1943,43 @@ describe("AwsBedrockHandler", () => { awsRegion: "us-east-1", }) - const clientInstance = handler["client"] - clientInstance.send = mockSend - const controller = new AbortController() let internalSignalCaptured: AbortSignal | undefined - // Spy on the send call to capture the abortSignal - mockSend.mockImplementation(async (_command: unknown, options?: { abortSignal?: AbortSignal }) => { - internalSignalCaptured = options?.abortSignal - return mockResult + const mockSend = vi + .fn() + .mockImplementation((_command: unknown, options?: { abortSignal?: AbortSignal }) => { + internalSignalCaptured = options?.abortSignal + return new Promise((_resolve, reject) => { + internalSignalCaptured?.addEventListener( + "abort", + () => reject(new DOMException("The operation was aborted.", "AbortError")), + { once: true }, + ) + }) + }) + handler["client"].send = mockSend + + // Pass timeoutMs so mergeAbortSignalAndTimeout creates a composite via AbortSignal.any() + const sendPromise = handler.completePrompt("test prompt", { + abortSignal: controller.signal, + timeoutMs: 5000, }) - await handler.completePrompt("test prompt", { abortSignal: controller.signal }) + // Wait until client.send is in flight and the composite signal is captured + await vi.waitFor(() => { + expect(internalSignalCaptured).toBeDefined() + }) - expect(internalSignalCaptured).toBeDefined() - expect(internalSignalCaptured).toBeInstanceOf(AbortSignal) + // The composite is a distinct object — not the same reference as the external signal + expect(internalSignalCaptured).not.toBe(controller.signal) + expect(internalSignalCaptured?.aborted).toBe(false) - // Abort the external signal and verify it propagates to the captured signal + // Abort mid-flight; the composite must propagate it immediately (synchronous) controller.abort() - await new Promise((resolve) => setTimeout(resolve, 10)) expect(internalSignalCaptured?.aborted).toBe(true) + + await expect(sendPromise).rejects.toMatchObject({ name: "AbortError" }) }) it("should abort immediately when signal is already aborted and timeoutMs > 0", async () => { @@ -1982,6 +2008,9 @@ describe("AwsBedrockHandler", () => { const sendOptions = mockSend.mock.calls[0][1] expect(sendOptions?.abortSignal).toBeDefined() expect(sendOptions?.abortSignal.aborted).toBe(true) + // AbortSignal.any() always returns a new composite; this distinguishes the merged + // path from a mutation that returns the pre-aborted external signal directly + expect(sendOptions?.abortSignal).not.toBe(controller.signal) }) it("should return undefined sendOptions when timeoutMs is 0 and no signal", async () => { @@ -2087,6 +2116,54 @@ describe("AwsBedrockHandler", () => { expect(result).toBe("") }) + it("completePrompt should reject with AbortError when the signal is aborted while client.send is in flight", async () => { + const handler = new AwsBedrockHandler({ + apiModelId: "anthropic.claude-3-5-sonnet-20241022-v2:0", + awsAccessKey: "test-access-key", + awsSecretKey: "test-secret-key", + awsRegion: "us-east-1", + }) + + const controller = new AbortController() + let rejectSend: ((err: unknown) => void) | undefined + + // mockSend hangs until the abort signal fires + const mockSend = vi + .fn() + .mockImplementation((_command: unknown, options?: { abortSignal?: AbortSignal }) => { + return new Promise((_resolve, reject) => { + rejectSend = reject + options?.abortSignal?.addEventListener( + "abort", + () => { + const abortError = new DOMException("The operation was aborted.", "AbortError") + reject(abortError) + }, + { once: true }, + ) + }) + }) + handler["client"].send = mockSend + + const sendPromise = handler.completePrompt("test prompt", { abortSignal: controller.signal }) + + // Wait until send is in flight + await vi.waitFor(() => { + expect(rejectSend).toBeDefined() + }) + + // Abort mid-flight; completePrompt must reject with AbortError. + // Also assert the ABORT classification is applied: the error message must come + // from the ABORT template ("Request was aborted"), not the GENERIC fallback. + // This proves "ABORT" sits in errorTypeOrder before competing patterns. + controller.abort() + + await expect(sendPromise).rejects.toMatchObject({ + name: "AbortError", + message: expect.stringContaining("Request was aborted"), + }) + }) + it("createMessage should reject with an AbortError when the external signal is already aborted", async () => { const handler = new AwsBedrockHandler({ apiModelId: "anthropic.claude-3-5-sonnet-20241022-v2:0", @@ -2163,28 +2240,42 @@ describe("AwsBedrockHandler", () => { const controller = new AbortController() - const generator = handler.createMessage( - "You are a helpful assistant", - [{ role: "user", content: "Hello" }], - makeCreateMessageMetadata({ abortSignal: controller.signal }), - ) + // Spy before createMessage so we capture the exact listener the production code registers + const addSpy = vi.spyOn(controller.signal, "addEventListener") + const removeSpy = vi.spyOn(controller.signal, "removeEventListener") - const consumed = (async () => { - for await (const _chunk of generator) { - // ignore chunks - } - })() + try { + const generator = handler.createMessage( + "You are a helpful assistant", + [{ role: "user", content: "Hello" }], + makeCreateMessageMetadata({ abortSignal: controller.signal }), + ) + + const consumed = (async () => { + for await (const _chunk of generator) { + // ignore chunks + } + })() - // Wait until the request is in flight and the internal signal is captured - await vi.waitFor(() => { - expect(internalSignal).toBeDefined() - }) - expect(internalSignal?.aborted).toBe(false) + // Wait until the request is in flight and the internal signal is captured + await vi.waitFor(() => { + expect(internalSignal).toBeDefined() + }) + expect(internalSignal?.aborted).toBe(false) - // Abort the external signal mid-flight; the stream must reject with an AbortError - controller.abort() + // Abort the external signal mid-flight; the stream must reject with an AbortError + controller.abort() - await expect(consumed).rejects.toMatchObject({ name: "AbortError" }) + await expect(consumed).rejects.toMatchObject({ name: "AbortError" }) + + // Verify the finally block removed the exact listener it registered (error path cleanup) + const registeredListener = addSpy.mock.calls.find(([type]) => type === "abort")?.[1] + expect(registeredListener).toBeDefined() + expect(removeSpy).toHaveBeenCalledWith("abort", registeredListener) + } finally { + addSpy.mockRestore() + removeSpy.mockRestore() + } }) it("createMessage should detach the external abort listener when the request completes", async () => { @@ -2210,8 +2301,12 @@ describe("AwsBedrockHandler", () => { }) handler["client"].send = mockSend - // First request completes normally with its own external signal + // First request completes normally with its own external signal. + // Spy on both add and remove so we can assert the exact same function + // reference was registered and then detached — expect.any(Function) would + // pass even if a different listener were removed, leaving the real one attached. const firstController = new AbortController() + const firstAddSpy = vi.spyOn(firstController.signal, "addEventListener") const firstRemoveSpy = vi.spyOn(firstController.signal, "removeEventListener") const firstGenerator = handler.createMessage( "You are a helpful assistant", @@ -2229,8 +2324,13 @@ describe("AwsBedrockHandler", () => { })() expect(firstText).toBe("hello") - // The bridge listener must be detached as soon as the request completes - expect(firstRemoveSpy).toHaveBeenCalledWith("abort", expect.any(Function)) + // The bridge listener must be detached as soon as the request completes. + // Extract the exact function reference that was registered so we can assert + // the same reference (not just any function) was passed to removeEventListener. + const abortAddCall = firstAddSpy.mock.calls.find(([type]) => type === "abort") + const registeredAbortListener = abortAddCall?.[1] + expect(registeredAbortListener).toBeDefined() + expect(firstRemoveSpy).toHaveBeenCalledWith("abort", registeredAbortListener) // Second request starts with a DIFFERENT external signal const secondController = new AbortController() @@ -2255,6 +2355,7 @@ describe("AwsBedrockHandler", () => { expect(secondSendSignal).toBeDefined() expect(secondSendSignal?.aborted).toBe(false) + firstAddSpy.mockRestore() firstRemoveSpy.mockRestore() }) diff --git a/src/api/providers/bedrock.ts b/src/api/providers/bedrock.ts index cc9ab1c9d8..27730aadaf 100644 --- a/src/api/providers/bedrock.ts +++ b/src/api/providers/bedrock.ts @@ -561,12 +561,17 @@ export class AwsBedrockHandler extends BaseProvider implements SingleCompletionH // Create a request-local AbortController with 10 minute timeout. Keeping it // request-local (and detaching the bridge listener in the finally block) means // a completed request can never leave a stale listener on the caller's signal. + // A manual setTimeout (rather than AbortSignal.timeout()) is required here + // because clearTimeout in the finally block needs a cancelable handle — + // AbortSignal.timeout() self-manages its timer and cannot be cleared. const requestController = new AbortController() let timeoutId: NodeJS.Timeout | undefined - // Bridge external abort signal to the request controller using the Bedrock pattern: - // - pre-aborted guard: check if already aborted before adding listener - // - { once: true }: remove listener after first abort to avoid leaks + // Bridge external abort signal to the request controller using the standard + // abort bridge pattern: + // - pre-aborted guard: a listener on an already-aborted signal may never fire, + // so abort the local controller directly in that case + // - { once: true }: the listener auto-removes on first abort event let abortListener: (() => void) | undefined const externalAbortSignal = metadata?.abortSignal if (externalAbortSignal) { @@ -1612,6 +1617,7 @@ Please check: // Check each error type's patterns in order of specificity (most specific first) const errorTypeOrder = [ + "ABORT", // Classify cancellations (user abort or request timeout) before any other pattern "SERVICE_QUOTA_EXCEEDED", // Most specific - check before THROTTLING "MODEL_NOT_READY", "TOO_MANY_TOKENS",