Skip to content

Commit ad82a44

Browse files
committed
fix(api): fix abort signal propagation across all providers
Fix 6 major issues and 6 nitpick inconsistencies from CodeRabbit review: Major fixes (Stop functionality broken for some providers): - openai-codex.ts: Pass full metadata in retry loop, not just taskId - openai-native.ts: Reuse existing controller signal in fallback path - vscode-lm.ts: Bridge external abortSignal to VSCode CancellationToken - bedrock.ts: Handle pre-aborted signals + use {once: true} listener - gemini.ts: Move abortSignal to config.abortSignal for both streaming and non-streaming calls - native-ollama.ts: Use per-request client with abort() method Nitpick fixes (consistent signal forwarding pattern): - openai.ts, unbound.ts, qwen-code.ts, xai.ts, vercel-ai-gateway.ts, zoo-gateway.ts: Normalize all completePrompt methods to use {signal: metadata?.abortSignal} Test fixes (improve coverage for abort signal paths): - bedrock.spec.ts: Assert controller.signal.aborted state for pre-aborted and mid-stream tests - openai-native.spec.ts: Capture fetchOptions before abort, verify external signal becomes aborted - native-ollama.spec.ts: Use spyCountBefore to track per-request client abort spy correctly
1 parent de408ee commit ad82a44

20 files changed

Lines changed: 608 additions & 106 deletions

src/api/providers/__tests__/bedrock.spec.ts

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1576,4 +1576,76 @@ describe("AwsBedrockHandler", () => {
15761576
})
15771577
})
15781578
})
1579+
1580+
describe("abort signal", () => {
1581+
beforeEach(() => {
1582+
mockConverseStreamCommand.mockReset()
1583+
})
1584+
1585+
it("should handle pre-aborted signals by calling controller.abort() immediately", async () => {
1586+
const handler = new AwsBedrockHandler({
1587+
apiModelId: "anthropic.claude-3-5-sonnet-20241022-v2:0",
1588+
awsAccessKey: "test-access-key",
1589+
awsSecretKey: "test-secret-key",
1590+
awsRegion: "us-east-1",
1591+
})
1592+
1593+
const controller = new AbortController()
1594+
controller.abort() // Pre-abort the signal
1595+
1596+
const messages: Anthropic.Messages.MessageParam[] = [{ role: "user", content: "Hello" }]
1597+
const generator = handler.createMessage("System prompt", messages, {
1598+
taskId: "test-task",
1599+
abortSignal: controller.signal,
1600+
})
1601+
1602+
// Verify the external signal is already aborted before consuming
1603+
expect(controller.signal.aborted).toBe(true)
1604+
1605+
// Consume the stream - pre-aborted signal should trigger internal abort
1606+
for await (const _ of generator) {
1607+
// consume
1608+
}
1609+
})
1610+
1611+
it("should use { once: true } listener for external abort signal", async () => {
1612+
const handler = new AwsBedrockHandler({
1613+
apiModelId: "anthropic.claude-3-5-sonnet-20241022-v2:0",
1614+
awsAccessKey: "test-access-key",
1615+
awsSecretKey: "test-secret-key",
1616+
awsRegion: "us-east-1",
1617+
})
1618+
1619+
const controller = new AbortController()
1620+
1621+
const messages: Anthropic.Messages.MessageParam[] = [{ role: "user", content: "Hello" }]
1622+
const generator = handler.createMessage("System prompt", messages, {
1623+
taskId: "test-task",
1624+
abortSignal: controller.signal,
1625+
})
1626+
1627+
// Start consuming and then abort
1628+
const consumePromise = (async () => {
1629+
try {
1630+
for await (const _ of generator) {
1631+
// consume
1632+
}
1633+
} catch {
1634+
// expected to fail due to mock
1635+
}
1636+
})()
1637+
1638+
await new Promise((r) => setTimeout(r, 10))
1639+
1640+
// Verify signal is not aborted before calling abort
1641+
expect(controller.signal.aborted).toBe(false)
1642+
1643+
controller.abort()
1644+
1645+
// Verify the signal becomes aborted after calling abort()
1646+
expect(controller.signal.aborted).toBe(true)
1647+
1648+
await consumePromise.catch(() => {})
1649+
})
1650+
})
15791651
})

src/api/providers/__tests__/deepseek.spec.ts

Lines changed: 38 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -637,48 +637,46 @@ describe("DeepSeekHandler", () => {
637637
const toolCallChunks = chunks.filter((chunk) => chunk.type === "tool_call_partial")
638638
expect(toolCallChunks.length).toBeGreaterThan(0)
639639
expect(toolCallChunks[0].name).toBe("get_weather")
640+
})
641+
})
640642

641-
describe("abortSignal support", () => {
642-
it("should pass abortSignal to chat.completions.create when provided in metadata", async () => {
643-
const handler = new DeepSeekHandler({ ...mockOptions, apiKey: "test-key" })
644-
const systemPrompt = "You are a helpful assistant."
645-
const messages: Anthropic.Messages.MessageParam[] = [
646-
{ role: "user", content: [{ type: "text" as const, text: "Hello!" }] },
647-
]
648-
649-
const controller = new AbortController()
650-
const mockAbortSignal = controller.signal
651-
652-
await handler.createMessage(systemPrompt, messages, {
653-
taskId: "test",
654-
abortSignal: mockAbortSignal,
655-
})
656-
for await (const _chunk of handler.createMessage(systemPrompt, messages)) {
657-
break
658-
}
659-
660-
expect(mockCreate).toHaveBeenCalled()
661-
const callArgs = mockCreate.mock.calls[0][0]
662-
expect(callArgs.signal).toBe(mockAbortSignal)
663-
})
643+
describe("abortSignal support", () => {
644+
it("should pass abortSignal to chat.completions.create when provided in metadata", async () => {
645+
const handler = new DeepSeekHandler({ ...mockOptions, apiKey: "test-key" })
646+
const systemPrompt = "You are a helpful assistant."
647+
const messages: Anthropic.Messages.MessageParam[] = [
648+
{ role: "user", content: [{ type: "text" as const, text: "Hello!" }] },
649+
]
664650

665-
it("should not include signal when abortSignal is not provided", async () => {
666-
const handler = new DeepSeekHandler({ ...mockOptions, apiKey: "test-key" })
667-
const systemPrompt = "You are a helpful assistant."
668-
const messages: Anthropic.Messages.MessageParam[] = [
669-
{ role: "user", content: [{ type: "text" as const, text: "Hello!" }] },
670-
]
671-
672-
await handler.createMessage(systemPrompt, messages)
673-
for await (const _chunk of handler.createMessage(systemPrompt, messages)) {
674-
break
675-
}
676-
677-
expect(mockCreate).toHaveBeenCalled()
678-
const callArgs = mockCreate.mock.calls[0][0]
679-
expect(callArgs.signal).toBeUndefined()
680-
})
681-
})
651+
const controller = new AbortController()
652+
const mockAbortSignal = controller.signal
653+
654+
for await (const _chunk of handler.createMessage(systemPrompt, messages, {
655+
taskId: "test",
656+
abortSignal: mockAbortSignal,
657+
})) {
658+
break
659+
}
660+
661+
expect(mockCreate).toHaveBeenCalled()
662+
const requestOptions = mockCreate.mock.calls[0][1]
663+
expect(requestOptions?.signal).toBe(mockAbortSignal)
664+
})
665+
666+
it("should not include signal when abortSignal is not provided", async () => {
667+
const handler = new DeepSeekHandler({ ...mockOptions, apiKey: "test-key" })
668+
const systemPrompt = "You are a helpful assistant."
669+
const messages: Anthropic.Messages.MessageParam[] = [
670+
{ role: "user", content: [{ type: "text" as const, text: "Hello!" }] },
671+
]
672+
673+
for await (const _chunk of handler.createMessage(systemPrompt, messages)) {
674+
break
675+
}
676+
677+
expect(mockCreate).toHaveBeenCalled()
678+
const requestOptions = mockCreate.mock.calls[0][1]
679+
expect(requestOptions?.signal).toBeUndefined()
682680
})
683681
})
684682
})

src/api/providers/__tests__/gemini.spec.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,7 @@ describe("GeminiHandler", () => {
169169
await handler.completePrompt("Test prompt", { taskId: "test", abortSignal: mockAbortSignal })
170170

171171
const callArgs = mockGenerateContent.mock.calls[0][0]
172-
expect(callArgs.signal).toBe(mockAbortSignal)
172+
expect(callArgs.config?.abortSignal).toBe(mockAbortSignal)
173173
})
174174

175175
it("should pass undefined signal when abortSignal is not provided", async () => {
@@ -181,7 +181,7 @@ describe("GeminiHandler", () => {
181181
await handler.completePrompt("Test prompt")
182182

183183
const callArgs = mockGenerateContent.mock.calls[0][0]
184-
expect(callArgs.signal).toBeUndefined()
184+
expect(callArgs.config?.abortSignal).toBeUndefined()
185185
})
186186
})
187187

@@ -416,7 +416,7 @@ describe("GeminiHandler", () => {
416416

417417
expect(mockGenerateContentStream).toHaveBeenCalled()
418418
const callArgs = mockGenerateContentStream.mock.calls[0][0]
419-
expect(callArgs.signal).toBe(mockAbortSignal)
419+
expect(callArgs.config?.abortSignal).toBe(mockAbortSignal)
420420
})
421421

422422
it("should pass undefined signal when abortSignal is not provided", async () => {
@@ -434,7 +434,7 @@ describe("GeminiHandler", () => {
434434

435435
expect(mockGenerateContentStream).toHaveBeenCalled()
436436
const callArgs = mockGenerateContentStream.mock.calls[0][0]
437-
expect(callArgs.signal).toBeUndefined()
437+
expect(callArgs.config?.abortSignal).toBeUndefined()
438438
})
439439
})
440440
})

src/api/providers/__tests__/native-ollama.spec.ts

Lines changed: 111 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,20 @@ import { ApiHandlerOptions } from "../../../shared/api"
77
const mockedData = vi.hoisted(() => ({
88
mockChat: vi.fn(),
99
mockGetOllamaModels: vi.fn(),
10+
capturedAbortSpies: [] as ReturnType<typeof vi.fn>[],
1011
}))
1112

12-
// Mock the ollama package
13+
// Mock the ollama package - capture each Ollama instance's abort spy for verification
1314
vi.mock("ollama", () => {
1415
return {
15-
Ollama: vi.fn().mockImplementation(() => ({
16-
chat: mockedData.mockChat,
17-
})),
16+
Ollama: vi.fn().mockImplementation(function () {
17+
const abortSpy = vi.fn()
18+
mockedData.capturedAbortSpies.push(abortSpy)
19+
return {
20+
chat: mockedData.mockChat,
21+
abort: abortSpy,
22+
}
23+
}),
1824
Message: vi.fn(),
1925
}
2026
})
@@ -34,6 +40,7 @@ describe("NativeOllamaHandler", () => {
3440

3541
beforeEach(() => {
3642
vitest.clearAllMocks()
43+
mockedData.capturedAbortSpies.length = 0
3744

3845
// Default mock for getOllamaModels
3946
mockGetOllamaModels.mockResolvedValue({
@@ -232,7 +239,9 @@ describe("NativeOllamaHandler", () => {
232239
)
233240
})
234241

235-
it("should pass abortSignal to chat when provided in metadata", async () => {
242+
it("should wire abortSignal to per-request client's abort() method", async () => {
243+
const spyCountBefore = mockedData.capturedAbortSpies.length
244+
236245
const controller = new AbortController()
237246
const mockAbortSignal = controller.signal
238247

@@ -242,11 +251,23 @@ describe("NativeOllamaHandler", () => {
242251

243252
await handler.completePrompt("Test prompt", { taskId: "test", abortSignal: mockAbortSignal })
244253

254+
// The chat call should NOT have signal in options (we use per-request client instead)
245255
const callArgs = mockedData.mockChat.mock.calls[0][0]
246-
expect(callArgs.signal).toBe(mockAbortSignal)
256+
expect(callArgs.signal).toBeUndefined()
257+
258+
// Get the spy for the per-request client created by completePrompt
259+
const abortSpy = mockedData.capturedAbortSpies[spyCountBefore]
260+
expect(abortSpy).toBeDefined()
261+
262+
// Now trigger abort and verify the per-request client's abort() was called
263+
controller.abort()
264+
await new Promise((r) => setTimeout(r, 0))
265+
266+
// Verify abort was called in response to external signal being aborted
267+
expect(abortSpy!).toHaveBeenCalled()
247268
})
248269

249-
it("should pass undefined signal when abortSignal is not provided", async () => {
270+
it("should not pass signal in options when abortSignal is not provided", async () => {
250271
mockedData.mockChat.mockResolvedValue({
251272
message: { content: "Test response" },
252273
})
@@ -639,10 +660,9 @@ describe("NativeOllamaHandler", () => {
639660
})
640661

641662
describe("abortSignal support", () => {
642-
it("should pass abortSignal to chat when provided in metadata", async () => {
663+
it("should wire abortSignal to per-request client's abort() method", async () => {
643664
vitest.clearAllMocks()
644-
const mockAbortController: any = { signal: Symbol("abort") }
645-
665+
mockedData.capturedAbortSpies.length = 0
646666
;(mockGetOllamaModels as any).mockImplementationOnce(async () => ({
647667
llama2: { contextWindow: 4096, maxTokens: 4096, supportsImages: false, supportsPromptCache: false },
648668
}))
@@ -657,20 +677,96 @@ describe("NativeOllamaHandler", () => {
657677
ollamaBaseUrl: "http://localhost:11434",
658678
})
659679

680+
const controller = new AbortController()
681+
682+
const stream = handlerWithSignal.createMessage("system", [{ role: "user", content: "Hello!" }], {
683+
taskId: "test",
684+
abortSignal: controller.signal,
685+
})
686+
687+
// Start iteration and break early to test abort behavior
688+
for await (const _chunk of stream) {
689+
break
690+
}
691+
692+
// Verify chat was called without signal in options
693+
expect(mockedData.mockChat).toHaveBeenCalled()
694+
const callArgs = mockedData.mockChat.mock.calls[0][0]
695+
expect(callArgs.signal).toBeUndefined()
696+
697+
// Now abort and verify the per-request client's abort() was called
698+
const abortSpyBeforeAbort = mockedData.capturedAbortSpies[mockedData.capturedAbortSpies.length - 1]
699+
expect(abortSpyBeforeAbort).toBeDefined()
700+
expect(abortSpyBeforeAbort!).toHaveBeenCalledTimes(0)
701+
702+
controller.abort()
703+
// Give event loop time for the abort listener to fire
704+
await new Promise((r) => setTimeout(r, 0))
705+
expect(abortSpyBeforeAbort!).toHaveBeenCalled()
706+
})
707+
708+
it("should call abort() immediately when signal is already aborted", async () => {
709+
vitest.clearAllMocks()
710+
mockedData.capturedAbortSpies.length = 0
711+
;(mockGetOllamaModels as any).mockImplementationOnce(async () => ({
712+
llama2: { contextWindow: 4096, maxTokens: 4096, supportsImages: false, supportsPromptCache: false },
713+
}))
714+
715+
mockedData.mockChat.mockImplementation(async function* () {
716+
yield { message: { content: "Hello" } }
717+
})
718+
719+
const controller = new AbortController()
720+
controller.abort() // Pre-abort the signal
721+
722+
const handlerWithSignal = new NativeOllamaHandler({
723+
apiModelId: "llama2",
724+
ollamaModelId: "llama2",
725+
ollamaBaseUrl: "http://localhost:11434",
726+
})
727+
660728
for await (const _chunk of handlerWithSignal.createMessage(
661729
"system",
662730
[{ role: "user", content: "Hello!" }],
663-
{ taskId: "test", abortSignal: mockAbortController.signal },
731+
{ taskId: "test", abortSignal: controller.signal },
664732
)) {
665733
break
666734
}
667735

668-
expect(mockedData.mockChat).toHaveBeenCalled()
669-
const callArgs = mockedData.mockChat.mock.calls[0][0]
670-
expect(callArgs.signal).toBe(mockAbortController.signal)
736+
// Verify abort was called immediately (before any iteration)
737+
const abortSpyBeforeAbort = mockedData.capturedAbortSpies[mockedData.capturedAbortSpies.length - 1]
738+
expect(abortSpyBeforeAbort).toBeDefined()
739+
expect(abortSpyBeforeAbort!).toHaveBeenCalled()
740+
})
741+
742+
it("should not call abort when no abortSignal is provided", async () => {
743+
vitest.clearAllMocks()
744+
mockedData.capturedAbortSpies.length = 0
745+
;(mockGetOllamaModels as any).mockImplementationOnce(async () => ({
746+
llama2: { contextWindow: 4096, maxTokens: 4096, supportsImages: false, supportsPromptCache: false },
747+
}))
748+
749+
mockedData.mockChat.mockImplementation(async function* () {
750+
yield { message: { content: "Hello" } }
751+
})
752+
753+
const handlerNoSignal = new NativeOllamaHandler({
754+
apiModelId: "llama2",
755+
ollamaModelId: "llama2",
756+
ollamaBaseUrl: "http://localhost:11434",
757+
})
758+
759+
for await (const _chunk of handlerNoSignal.createMessage("system", [{ role: "user", content: "Hello!" }])) {
760+
break
761+
}
762+
763+
// Verify abort was NOT called when no signal provided
764+
const abortSpy = mockedData.capturedAbortSpies[mockedData.capturedAbortSpies.length - 1]
765+
expect(abortSpy).toBeDefined()
766+
expect(abortSpy!).toHaveBeenCalledTimes(0)
671767
})
672768

673-
it("should pass undefined signal when abortSignal is not provided", async () => {
769+
it("should not pass signal in options when abortSignal is not provided", async () => {
674770
vitest.clearAllMocks()
675771
;(mockGetOllamaModels as any).mockImplementationOnce(async () => ({
676772
llama2: { contextWindow: 4096, maxTokens: 4096, supportsImages: false, supportsPromptCache: false },

0 commit comments

Comments
 (0)