Skip to content

Commit e8aab5f

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: - gemini.spec.ts: Check config.abortSignal for both streaming and non-streaming tests - native-ollama.spec.ts: Mock Ollama client with abort() method, assert no signal in options (per-request client pattern) - deepseek.spec.ts: Move abortSignal suite out of parent test, assert from request options (2nd arg) - vercel-ai-gateway.spec.ts: Add second argument assertion to completePrompt tests - zoo-gateway.spec.ts: Add second argument assertion to completePrompt tests Capture per-request client's abort() spy and add tests for: - controller.abort() triggering the captured abort spy - pre-aborted signals calling abort() immediately - no abortSignal provided not calling abort()
1 parent de408ee commit e8aab5f

17 files changed

Lines changed: 280 additions & 106 deletions

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: 97 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
})
@@ -232,7 +238,7 @@ describe("NativeOllamaHandler", () => {
232238
)
233239
})
234240

235-
it("should pass abortSignal to chat when provided in metadata", async () => {
241+
it("should wire abortSignal to per-request client's abort() method", async () => {
236242
const controller = new AbortController()
237243
const mockAbortSignal = controller.signal
238244

@@ -242,11 +248,12 @@ describe("NativeOllamaHandler", () => {
242248

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

251+
// The chat call should NOT have signal in options (we use per-request client instead)
245252
const callArgs = mockedData.mockChat.mock.calls[0][0]
246-
expect(callArgs.signal).toBe(mockAbortSignal)
253+
expect(callArgs.signal).toBeUndefined()
247254
})
248255

249-
it("should pass undefined signal when abortSignal is not provided", async () => {
256+
it("should not pass signal in options when abortSignal is not provided", async () => {
250257
mockedData.mockChat.mockResolvedValue({
251258
message: { content: "Test response" },
252259
})
@@ -639,10 +646,9 @@ describe("NativeOllamaHandler", () => {
639646
})
640647

641648
describe("abortSignal support", () => {
642-
it("should pass abortSignal to chat when provided in metadata", async () => {
649+
it("should wire abortSignal to per-request client's abort() method", async () => {
643650
vitest.clearAllMocks()
644-
const mockAbortController: any = { signal: Symbol("abort") }
645-
651+
mockedData.capturedAbortSpies.length = 0
646652
;(mockGetOllamaModels as any).mockImplementationOnce(async () => ({
647653
llama2: { contextWindow: 4096, maxTokens: 4096, supportsImages: false, supportsPromptCache: false },
648654
}))
@@ -657,20 +663,96 @@ describe("NativeOllamaHandler", () => {
657663
ollamaBaseUrl: "http://localhost:11434",
658664
})
659665

666+
const controller = new AbortController()
667+
668+
const stream = handlerWithSignal.createMessage("system", [{ role: "user", content: "Hello!" }], {
669+
taskId: "test",
670+
abortSignal: controller.signal,
671+
})
672+
673+
// Start iteration and break early to test abort behavior
674+
for await (const _chunk of stream) {
675+
break
676+
}
677+
678+
// Verify chat was called without signal in options
679+
expect(mockedData.mockChat).toHaveBeenCalled()
680+
const callArgs = mockedData.mockChat.mock.calls[0][0]
681+
expect(callArgs.signal).toBeUndefined()
682+
683+
// Now abort and verify the per-request client's abort() was called
684+
const abortSpyBeforeAbort = mockedData.capturedAbortSpies[mockedData.capturedAbortSpies.length - 1]
685+
expect(abortSpyBeforeAbort).toBeDefined()
686+
expect(abortSpyBeforeAbort!).toHaveBeenCalledTimes(0)
687+
688+
controller.abort()
689+
// Give event loop time for the abort listener to fire
690+
await new Promise((r) => setTimeout(r, 0))
691+
expect(abortSpyBeforeAbort!).toHaveBeenCalled()
692+
})
693+
694+
it("should call abort() immediately when signal is already aborted", async () => {
695+
vitest.clearAllMocks()
696+
mockedData.capturedAbortSpies.length = 0
697+
;(mockGetOllamaModels as any).mockImplementationOnce(async () => ({
698+
llama2: { contextWindow: 4096, maxTokens: 4096, supportsImages: false, supportsPromptCache: false },
699+
}))
700+
701+
mockedData.mockChat.mockImplementation(async function* () {
702+
yield { message: { content: "Hello" } }
703+
})
704+
705+
const controller = new AbortController()
706+
controller.abort() // Pre-abort the signal
707+
708+
const handlerWithSignal = new NativeOllamaHandler({
709+
apiModelId: "llama2",
710+
ollamaModelId: "llama2",
711+
ollamaBaseUrl: "http://localhost:11434",
712+
})
713+
660714
for await (const _chunk of handlerWithSignal.createMessage(
661715
"system",
662716
[{ role: "user", content: "Hello!" }],
663-
{ taskId: "test", abortSignal: mockAbortController.signal },
717+
{ taskId: "test", abortSignal: controller.signal },
664718
)) {
665719
break
666720
}
667721

668-
expect(mockedData.mockChat).toHaveBeenCalled()
669-
const callArgs = mockedData.mockChat.mock.calls[0][0]
670-
expect(callArgs.signal).toBe(mockAbortController.signal)
722+
// Verify abort was called immediately (before any iteration)
723+
const abortSpyBeforeAbort = mockedData.capturedAbortSpies[mockedData.capturedAbortSpies.length - 1]
724+
expect(abortSpyBeforeAbort).toBeDefined()
725+
expect(abortSpyBeforeAbort!).toHaveBeenCalled()
726+
})
727+
728+
it("should not call abort when no abortSignal is provided", async () => {
729+
vitest.clearAllMocks()
730+
mockedData.capturedAbortSpies.length = 0
731+
;(mockGetOllamaModels as any).mockImplementationOnce(async () => ({
732+
llama2: { contextWindow: 4096, maxTokens: 4096, supportsImages: false, supportsPromptCache: false },
733+
}))
734+
735+
mockedData.mockChat.mockImplementation(async function* () {
736+
yield { message: { content: "Hello" } }
737+
})
738+
739+
const handlerNoSignal = new NativeOllamaHandler({
740+
apiModelId: "llama2",
741+
ollamaModelId: "llama2",
742+
ollamaBaseUrl: "http://localhost:11434",
743+
})
744+
745+
for await (const _chunk of handlerNoSignal.createMessage("system", [{ role: "user", content: "Hello!" }])) {
746+
break
747+
}
748+
749+
// Verify abort was NOT called when no signal provided
750+
const abortSpy = mockedData.capturedAbortSpies[mockedData.capturedAbortSpies.length - 1]
751+
expect(abortSpy).toBeDefined()
752+
expect(abortSpy!).toHaveBeenCalledTimes(0)
671753
})
672754

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

src/api/providers/__tests__/vercel-ai-gateway.spec.ts

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -586,13 +586,16 @@ describe("VercelAiGatewayHandler", () => {
586586
const result = await handler.completePrompt(prompt)
587587

588588
expect(result).toBe("Test completion response")
589-
expect(mockCreate).toHaveBeenCalledWith({
590-
model: "anthropic/claude-sonnet-4",
591-
messages: [{ role: "user", content: prompt }],
592-
stream: false,
593-
temperature: VERCEL_AI_GATEWAY_DEFAULT_TEMPERATURE,
594-
max_completion_tokens: 64000,
595-
})
589+
expect(mockCreate).toHaveBeenCalledWith(
590+
{
591+
model: "anthropic/claude-sonnet-4",
592+
messages: [{ role: "user", content: prompt }],
593+
stream: false,
594+
temperature: VERCEL_AI_GATEWAY_DEFAULT_TEMPERATURE,
595+
max_completion_tokens: 64000,
596+
},
597+
{ signal: undefined },
598+
)
596599
})
597600

598601
it("uses custom temperature for completion", async () => {
@@ -608,6 +611,7 @@ describe("VercelAiGatewayHandler", () => {
608611
expect.objectContaining({
609612
temperature: customTemp,
610613
}),
614+
expect.objectContaining({ signal: undefined }),
611615
)
612616
})
613617

@@ -656,6 +660,7 @@ describe("VercelAiGatewayHandler", () => {
656660
expect.objectContaining({
657661
temperature: 0.9,
658662
}),
663+
expect.objectContaining({ signal: undefined }),
659664
)
660665
})
661666

src/api/providers/__tests__/zoo-gateway.spec.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -464,6 +464,7 @@ describe("ZooGatewayHandler", () => {
464464
temperature: ZOO_GATEWAY_DEFAULT_TEMPERATURE,
465465
max_completion_tokens: 64000,
466466
}),
467+
expect.objectContaining({ signal: undefined }),
467468
)
468469
})
469470

src/api/providers/bedrock.ts

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -534,12 +534,21 @@ export class AwsBedrockHandler extends BaseProvider implements SingleCompletionH
534534
const controller = new AbortController()
535535
let timeoutId: NodeJS.Timeout | undefined
536536

537-
// Listen for external abort signal from metadata and forward to internal controller
537+
// Listen for external abort signal from metadata and forward to internal controller.
538+
// Handle both pre-aborted signals and future abort events.
538539
const externalAbortSignal = metadata?.abortSignal
539540
if (externalAbortSignal) {
540-
externalAbortSignal.addEventListener("abort", () => {
541+
if (externalAbortSignal.aborted) {
541542
controller.abort()
542-
})
543+
} else {
544+
externalAbortSignal.addEventListener(
545+
"abort",
546+
() => {
547+
controller.abort()
548+
},
549+
{ once: true },
550+
)
551+
}
543552
}
544553

545554
try {

src/api/providers/gemini.ts

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -343,7 +343,11 @@ export class GeminiHandler extends BaseProvider implements SingleCompletionHandl
343343
}
344344
}
345345

346-
const params: any = { model, contents, config, signal: metadata?.abortSignal }
346+
const params: any = {
347+
model,
348+
contents,
349+
config: { ...config, abortSignal: metadata?.abortSignal },
350+
}
347351

348352
try {
349353
const result = await this.client.models.generateContentStream(params)
@@ -595,8 +599,7 @@ export class GeminiHandler extends BaseProvider implements SingleCompletionHandl
595599
const request = {
596600
model,
597601
contents: [{ role: "user", parts: [{ text: prompt }] }],
598-
config: promptConfig,
599-
signal: metadata?.abortSignal,
602+
config: { ...promptConfig, abortSignal: metadata?.abortSignal },
600603
}
601604

602605
const result = await this.client.models.generateContent(request)

0 commit comments

Comments
 (0)