Skip to content

Commit b06f645

Browse files
committed
fix(api): address CodeRabbit review on gateway-b abort handling
1 parent 9429632 commit b06f645

8 files changed

Lines changed: 382 additions & 124 deletions

File tree

‎src/api/providers/__tests__/opencode-go.spec.ts‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -415,6 +415,41 @@ describe("OpencodeGoHandler", () => {
415415
await expect(consumed).rejects.toMatchObject({ name: "AbortError" })
416416
expect(capturedSignal?.aborted).toBe(true)
417417
})
418+
419+
it("detaches the bridged abort listener when the request completes normally", async () => {
420+
// The listener is added with { once: true }, so it only detaches on
421+
// abort. A task-scoped signal spanning many requests must not
422+
// accumulate a listener per request: assert explicit removal after a
423+
// normal (non-aborted) completion.
424+
mockCreate.mockImplementation(async () =>
425+
asyncStreamFrom([
426+
{
427+
choices: [{ delta: { content: "ok" }, index: 0 }],
428+
index: 0,
429+
},
430+
{
431+
choices: [{ delta: {}, index: 0 }],
432+
index: 0,
433+
usage: { prompt_tokens: 2, completion_tokens: 3 },
434+
},
435+
]),
436+
)
437+
438+
const handler = new OpencodeGoHandler(mockOptions)
439+
const controller = new AbortController()
440+
const removeListenerSpy = vi.spyOn(controller.signal, "removeEventListener")
441+
442+
const stream = handler.createMessage(
443+
"sys",
444+
[{ role: "user", content: "hi" }],
445+
makeCreateMessageMetadata({ abortSignal: controller.signal }),
446+
)
447+
448+
const chunks = await collectStream(stream)
449+
expect(chunks).toContainEqual({ type: "text", text: "ok" })
450+
expect(removeListenerSpy).toHaveBeenCalledWith("abort", expect.any(Function))
451+
expect(controller.signal.aborted).toBe(false)
452+
})
418453
})
419454
describe("completePrompt", () => {
420455
it("returns the message content for a non-streaming completion", async () => {
@@ -660,6 +695,20 @@ describe("OpencodeGoHandler", () => {
660695
})
661696
})
662697

698+
it("completePrompt omits the timeout option when timeoutMs is 0 (Anthropic path)", async () => {
699+
// The SDK treats timeout: 0 as an immediate abort, so the "disabled"
700+
// value must never be forwarded — assert the absence of the option.
701+
mockAnthropicCreate.mockResolvedValue({ content: [{ type: "text", text: "response" }] })
702+
const handler = new OpencodeGoHandler(anthropicOptions)
703+
await handler.completePrompt("ping", { timeoutMs: 0 })
704+
const call = mockAnthropicCreate.mock.calls[mockAnthropicCreate.mock.calls.length - 1]
705+
const requestOptions = call[1] as { timeout?: number } | undefined
706+
// When no option is forwarded the provider omits the SDK options
707+
// argument entirely, so absence means: undefined arg OR an arg
708+
// without a timeout key.
709+
expect(Object.keys(requestOptions ?? {})).not.toContain("timeout")
710+
})
711+
663712
it("completePrompt works without options (backward compatible, Anthropic path)", async () => {
664713
mockAnthropicCreate.mockResolvedValue({ content: [{ type: "text", text: "response" }] })
665714
const handler = new OpencodeGoHandler(anthropicOptions)
@@ -735,6 +784,20 @@ describe("OpencodeGoHandler", () => {
735784
)
736785
})
737786

787+
it("completePrompt omits the timeout option when timeoutMs is 0 (OpenAI path)", async () => {
788+
// The OpenAI SDK treats timeout: 0 as an immediate abort, so the
789+
// "disabled" value must never be forwarded — assert the absence of
790+
// the option.
791+
mockCreate.mockResolvedValueOnce({
792+
choices: [{ message: { content: "response" } }],
793+
})
794+
const handler = new OpencodeGoHandler(openaiOptions)
795+
await handler.completePrompt("ping", { timeoutMs: 0 })
796+
const call = mockCreate.mock.calls[mockCreate.mock.calls.length - 1]
797+
const requestOptions = call[1] as { timeout?: number } | undefined
798+
expect(requestOptions).not.toHaveProperty("timeout")
799+
})
800+
738801
it("completePrompt works without options (backward compatible, OpenAI path)", async () => {
739802
mockCreate.mockResolvedValueOnce({
740803
choices: [{ message: { content: "response" } }],

‎src/api/providers/__tests__/unbound.spec.ts‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -239,6 +239,24 @@ describe("UnboundHandler", () => {
239239
)
240240
})
241241

242+
it("completePrompt should omit the timeout option when timeoutMs is 0", async () => {
243+
// The OpenAI SDK treats timeout: 0 as an immediate abort, so the
244+
// "disabled" value must never be forwarded — assert the absence of
245+
// the option (a forwarded timeout: 0 would fail this assertion).
246+
sharedMockCreate.mockResolvedValue({
247+
choices: [{ message: { content: "completed text" } }],
248+
})
249+
250+
const handler = new UnboundHandler({
251+
unboundApiKey: "test-key",
252+
unboundModelId: "openai/gpt-4o",
253+
})
254+
255+
await handler.completePrompt("Write a haiku", { timeoutMs: 0 })
256+
const call = sharedMockCreate.mock.calls[sharedMockCreate.mock.calls.length - 1]
257+
const requestOptions = call[1] as { timeout?: number } | undefined
258+
expect(requestOptions).not.toHaveProperty("timeout")
259+
})
242260
it("completePrompt should work without options (backward compatible)", async () => {
243261
sharedMockCreate.mockResolvedValue({
244262
choices: [{ message: { content: "completed text" } }],
@@ -314,5 +332,38 @@ describe("UnboundHandler", () => {
314332
await expect(consumed).rejects.toMatchObject({ name: "AbortError" })
315333
expect(capturedSignal?.aborted).toBe(true)
316334
})
335+
336+
it("detaches the bridged abort listener when the request completes normally", async () => {
337+
// The listener is added with { once: true }, so it only detaches on
338+
// abort. A task-scoped signal spanning many requests must not
339+
// accumulate a listener per request: assert explicit removal after a
340+
// normal (non-aborted) completion.
341+
sharedMockCreate.mockImplementation(async () =>
342+
asyncStreamFrom([
343+
{ choices: [{ delta: { content: "ok" } }] },
344+
{ choices: [{ delta: {} }], usage: { prompt_tokens: 1, completion_tokens: 1 } },
345+
]),
346+
)
347+
348+
const controller = new AbortController()
349+
const removeListenerSpy = vi.spyOn(controller.signal, "removeEventListener")
350+
351+
const handler = new UnboundHandler({
352+
unboundApiKey: "test-key",
353+
unboundModelId: "openai/gpt-4o",
354+
})
355+
356+
const chunks = await collectStream(
357+
handler.createMessage(
358+
"system",
359+
[{ role: "user", content: "hi" }],
360+
makeCreateMessageMetadata({ abortSignal: controller.signal }),
361+
),
362+
)
363+
364+
expect(chunks).toContainEqual({ type: "text", text: "ok" })
365+
expect(removeListenerSpy).toHaveBeenCalledWith("abort", expect.any(Function))
366+
expect(controller.signal.aborted).toBe(false)
367+
})
317368
})
318369
})

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

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -754,6 +754,29 @@ describe("VercelAiGatewayHandler", () => {
754754
)
755755
})
756756

757+
it("should omit the timeout option when timeoutMs is 0", async () => {
758+
// The OpenAI SDK treats timeout: 0 as an immediate abort, so the
759+
// "disabled" value must never be forwarded — assert the absence of
760+
// the option (a forwarded timeout: 0 would fail this assertion).
761+
const handler = new VercelAiGatewayHandler(mockOptions)
762+
mockCreate.mockImplementation(async () => ({
763+
choices: [
764+
{
765+
message: { role: "assistant", content: "response" },
766+
finish_reason: "stop",
767+
index: 0,
768+
},
769+
],
770+
}))
771+
772+
await handler.completePrompt("test prompt", { timeoutMs: 0 })
773+
const call = mockCreate.mock.calls[mockCreate.mock.calls.length - 1]
774+
const requestOptions = call[1] as { timeout?: number } | undefined
775+
// When no option is forwarded the provider omits the SDK options
776+
// argument entirely, so absence means: undefined arg OR an arg
777+
// without a timeout key.
778+
expect(Object.keys(requestOptions ?? {})).not.toContain("timeout")
779+
})
757780
it("should work without options (backward compatible)", async () => {
758781
const handler = new VercelAiGatewayHandler(mockOptions)
759782
mockCreate.mockImplementation(async () => ({
@@ -824,6 +847,41 @@ describe("VercelAiGatewayHandler", () => {
824847
await expect(consumed).rejects.toMatchObject({ name: "AbortError" })
825848
expect(capturedSignal?.aborted).toBe(true)
826849
})
850+
851+
it("detaches the bridged abort listener when the request completes normally", async () => {
852+
// The listener is added with { once: true }, so it only detaches on
853+
// abort. A task-scoped signal spanning many requests must not
854+
// accumulate a listener per request: assert explicit removal after a
855+
// normal (non-aborted) completion.
856+
mockCreate.mockImplementation(async () =>
857+
asyncStreamFrom([
858+
{
859+
choices: [{ delta: { content: "ok" }, index: 0 }],
860+
index: 0,
861+
},
862+
{
863+
choices: [{ delta: {}, index: 0 }],
864+
index: 0,
865+
usage: { prompt_tokens: 2, completion_tokens: 3 },
866+
},
867+
]),
868+
)
869+
870+
const handler = new VercelAiGatewayHandler(mockOptions)
871+
const controller = new AbortController()
872+
const removeListenerSpy = vi.spyOn(controller.signal, "removeEventListener")
873+
874+
const stream = handler.createMessage(
875+
"test prompt",
876+
[{ role: "user", content: "hello" }],
877+
makeCreateMessageMetadata({ abortSignal: controller.signal }),
878+
)
879+
880+
const chunks = await collectStream(stream)
881+
expect(chunks).toContainEqual({ type: "text", text: "ok" })
882+
expect(removeListenerSpy).toHaveBeenCalledWith("abort", expect.any(Function))
883+
expect(controller.signal.aborted).toBe(false)
884+
})
827885
})
828886

829887
describe("temperature support", () => {

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

Lines changed: 42 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -487,17 +487,19 @@ describe("ZooGatewayHandler", () => {
487487
)
488488
})
489489

490-
it("should pass timeoutMs=0 through to client", async () => {
490+
it("should omit the timeout option when timeoutMs is 0", async () => {
491+
// The OpenAI SDK treats timeout: 0 as an immediate abort, so the
492+
// "disabled" value must never be forwarded — assert the absence of
493+
// the option (a forwarded timeout: 0 would fail this assertion).
491494
const handler = new ZooGatewayHandler(mockOptions)
492495
mockCreate.mockImplementation(async () => ({
493496
choices: [{ message: { role: "assistant", content: "response" } }],
494497
}))
495498

496499
await handler.completePrompt("test prompt", { timeoutMs: 0 })
497-
expect(mockCreate).toHaveBeenCalledWith(
498-
expect.objectContaining({ model: expect.any(String) }),
499-
expect.objectContaining({ timeout: 0 }),
500-
)
500+
const call = mockCreate.mock.calls[mockCreate.mock.calls.length - 1]
501+
const requestOptions = call[1] as { timeout?: number } | undefined
502+
expect(requestOptions).not.toHaveProperty("timeout")
501503
})
502504

503505
it("should work without options (backward compatible)", async () => {
@@ -564,6 +566,41 @@ describe("ZooGatewayHandler", () => {
564566
await expect(consumed).rejects.toMatchObject({ name: "AbortError" })
565567
expect(capturedSignal?.aborted).toBe(true)
566568
})
569+
570+
it("detaches the bridged abort listener when the request completes normally", async () => {
571+
// The listener is added with { once: true }, so it only detaches on
572+
// abort. A task-scoped signal spanning many requests must not
573+
// accumulate a listener per request: assert explicit removal after a
574+
// normal (non-aborted) completion.
575+
mockCreate.mockImplementation(async () =>
576+
asyncStreamFrom([
577+
{
578+
choices: [{ delta: { content: "ok" }, index: 0 }],
579+
index: 0,
580+
},
581+
{
582+
choices: [{ delta: {}, index: 0 }],
583+
index: 0,
584+
usage: { prompt_tokens: 2, completion_tokens: 3 },
585+
},
586+
]),
587+
)
588+
589+
const handler = new ZooGatewayHandler(mockOptions)
590+
const controller = new AbortController()
591+
const removeListenerSpy = vi.spyOn(controller.signal, "removeEventListener")
592+
593+
const stream = handler.createMessage(
594+
"prompt",
595+
[{ role: "user", content: "hello" }],
596+
makeCreateMessageMetadata({ abortSignal: controller.signal }),
597+
)
598+
599+
const chunks = await collectStream(stream)
600+
expect(chunks).toContainEqual({ type: "text", text: "ok" })
601+
expect(removeListenerSpy).toHaveBeenCalledWith("abort", expect.any(Function))
602+
expect(controller.signal.aborted).toBe(false)
603+
})
567604
})
568605
describe("classifyGatewayApiError", () => {
569606
it("returns sign_in on 401", () => {

0 commit comments

Comments
 (0)