Skip to content

Commit 9bd5d6f

Browse files
committed
fix(openrouter): address CodeRabbit review findings
- abort the per-request controller in createMessage's finally so an abandoned generator (early break / downstream error) cancels the in-flight stream - reject pre-aborted completePrompt with the canonical createAbortError message instead of throwIfAborted's generic text (drop the now-unused throwIfAborted import and its Stryker directive) - extract the duplicated settlesWithin helper into shared src/test-utils/promise.ts and import it from both spec files - assert the registered abort listener by identity instead of expect.any(Function) - add a test that verifies abandoning the generator aborts the request signal
1 parent 470c453 commit 9bd5d6f

4 files changed

Lines changed: 72 additions & 54 deletions

File tree

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

Lines changed: 41 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import { makeApiHandlerOptions, makeCreateMessageMetadata } from "../../../test-
2525
import { asyncStreamFrom, collectStream } from "../../../test-utils/stream"
2626
import { collectStreamAndParseToolCalls } from "../../../test-utils/native-tool-call-stream"
2727
import { clearAllMocks } from "../../../test-utils/reset"
28+
import { settlesWithin } from "../../../test-utils/promise"
2829

2930
vitest.mock("openai")
3031
vitest.mock("delay", () => ({
@@ -109,27 +110,6 @@ vitest.mock("../fetchers/modelCache", () => ({
109110
}),
110111
}))
111112

112-
/**
113-
* Fail fast when an awaited operation never settles. Mutations that break abort
114-
* propagation would otherwise hang the test until Stryker's per-mutant timeout,
115-
* marking the mutant "Timeout" instead of "Killed".
116-
*/
117-
function settlesWithin<T>(promise: Promise<T>, ms: number): Promise<T> {
118-
return new Promise<T>((resolve, reject) => {
119-
const timer = setTimeout(() => reject(new Error("operation did not settle within " + ms + "ms")), ms)
120-
void promise.then(
121-
(value) => {
122-
clearTimeout(timer)
123-
resolve(value)
124-
},
125-
(error) => {
126-
clearTimeout(timer)
127-
reject(error)
128-
},
129-
)
130-
})
131-
}
132-
133113
const ABORT_SETTLE_MS = 150
134114

135115
describe("OpenRouterHandler", () => {
@@ -859,6 +839,45 @@ describe("OpenRouterHandler", () => {
859839
})
860840
expect(chunks).toContainEqual({ type: "text", text: "first" })
861841
})
842+
it("cancels the in-flight stream when the consumer abandons the generator", async () => {
843+
const handler = new OpenRouterHandler(mockOptions)
844+
845+
// Emulate the OpenAI SDK: the first chunk arrives, then the response body
846+
// stalls until the request signal aborts — no further chunk arrives on its own.
847+
let requestSignal: AbortSignal | undefined
848+
const mockCreate = vitest
849+
.fn()
850+
.mockImplementation(async (_params: unknown, options?: { signal?: AbortSignal }) => {
851+
requestSignal = options?.signal
852+
return (async function* () {
853+
yield { id: "1", choices: [{ delta: { content: "first" } }] }
854+
await new Promise<void>((resolve) => {
855+
expect(requestSignal).toBeDefined()
856+
if (requestSignal!.aborted) {
857+
resolve()
858+
} else {
859+
requestSignal!.addEventListener("abort", () => resolve(), { once: true })
860+
}
861+
})
862+
yield { id: "2", choices: [{ delta: { content: "second" } }] }
863+
})()
864+
})
865+
// The auto-mocked OpenAI client is injected via a structural type to avoid `any` casts.
866+
const client = handler["client"] as unknown as { chat: { completions: { create: typeof mockCreate } } }
867+
client.chat = { completions: { create: mockCreate } }
868+
869+
const generator = handler.createMessage("test", [{ role: "user" as const, content: "hi" }])
870+
871+
const first = await settlesWithin(generator.next(), ABORT_SETTLE_MS)
872+
expect(first.value).toEqual({ type: "text", text: "first" })
873+
expect(requestSignal?.aborted).toBe(false)
874+
875+
// Abandon the generator mid-stream: the finally block must abort the per-request
876+
// controller so the in-flight stream is cancelled instead of lingering until
877+
// the client-level timeout.
878+
await settlesWithin(generator.return(undefined), ABORT_SETTLE_MS)
879+
expect(requestSignal?.aborted).toBe(true)
880+
})
862881
it("does not emit buffered chunks after a mid-stream abort (iterator keeps delivering)", async () => {
863882
const handler = new OpenRouterHandler(mockOptions)
864883
const controller = new AbortController()
@@ -2501,7 +2520,7 @@ describe("OpenRouterHandler", () => {
25012520
handler.completePrompt("test prompt", { abortSignal: controller.signal }),
25022521
).rejects.toMatchObject({
25032522
name: "AbortError",
2504-
message: "This operation was aborted",
2523+
message: "The OpenRouter request was aborted",
25052524
})
25062525
// The pre-abort guard rejects before any model lookup beyond the constructor's own.
25072526
const { getModels } = await import("../fetchers/modelCache")

‎src/api/providers/openrouter.ts‎

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -36,13 +36,7 @@ import { DEFAULT_HEADERS, NOT_PROVIDED } from "./constants"
3636
import { BaseProvider } from "./base-provider"
3737
import type { ApiHandlerCreateMessageMetadata, CompletePromptOptions, SingleCompletionHandler } from "../index"
3838
import { handleOpenAIError } from "./utils/error-handler"
39-
import {
40-
createAbortError,
41-
isRequestAborted,
42-
mergeAbortSignalAndTimeout,
43-
rejectOnAbort,
44-
throwIfAborted,
45-
} from "./utils/abort-signal"
39+
import { createAbortError, isRequestAborted, mergeAbortSignalAndTimeout, rejectOnAbort } from "./utils/abort-signal"
4640
import { generateImageWithProvider, ImageGenerationResult } from "./utils/image-generation"
4741
import { applyRouterToolPreferences } from "./utils/router-tool-preferences"
4842

@@ -628,6 +622,10 @@ export class OpenRouterHandler extends BaseProvider implements SingleCompletionH
628622
}
629623
} finally {
630624
removeExternalAbortListener?.()
625+
// Cancel the in-flight request when the consumer abandons the generator
626+
// (early break / downstream error). No-op once the stream has completed
627+
// or the controller is already aborted.
628+
controller.abort()
631629
}
632630
}
633631

@@ -678,9 +676,8 @@ export class OpenRouterHandler extends BaseProvider implements SingleCompletionH
678676
// waiting for the lookup to settle. The configured timeoutMs covers the lookup
679677
// as well.
680678
const requestAbortSignal = mergeAbortSignalAndTimeout(options?.abortSignal, options?.timeoutMs)
681-
// Stryker disable next-line ConditionalExpression: without signal or timeout the merged signal is undefined and throwIfAborted(undefined) is a no-op, so forcing the branch is unobservable
682-
if (requestAbortSignal) {
683-
throwIfAborted(requestAbortSignal)
679+
if (requestAbortSignal?.aborted) {
680+
throw createAbortError(this.providerName)
684681
}
685682

686683
let model: Awaited<ReturnType<OpenRouterHandler["fetchModel"]>>

‎src/api/providers/utils/__tests__/abort-signal.spec.ts‎

Lines changed: 4 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -6,27 +6,7 @@ import {
66
rejectOnAbort,
77
throwIfAborted,
88
} from "../abort-signal"
9-
10-
/**
11-
* Fail fast when a promise never settles. Mutations that remove the settle,
12-
* reject, or abort wiring would otherwise hang the test until Stryker's
13-
* per-mutant timeout, marking the mutant "Timeout" instead of "Killed".
14-
*/
15-
function settlesWithin<T>(promise: Promise<T>, ms: number): Promise<T> {
16-
return new Promise<T>((resolve, reject) => {
17-
const timer = setTimeout(() => reject(new Error("promise did not settle within " + ms + "ms")), ms)
18-
void promise.then(
19-
(value) => {
20-
clearTimeout(timer)
21-
resolve(value)
22-
},
23-
(error) => {
24-
clearTimeout(timer)
25-
reject(error)
26-
},
27-
)
28-
})
29-
}
9+
import { settlesWithin } from "../../../../test-utils/promise"
3010

3111
const SETTLE_MS = 200
3212

@@ -116,7 +96,9 @@ describe("rejectOnAbort", () => {
11696
controller.abort()
11797

11898
await expect(settlesWithin(race, SETTLE_MS)).rejects.toMatchObject({ name: "AbortError" })
119-
expect(addSpy).toHaveBeenCalledWith("abort", expect.any(Function), { once: true })
99+
const registeredListener = addSpy.mock.calls[0]?.[1]
100+
expect(registeredListener).toBeTypeOf("function")
101+
expect(addSpy).toHaveBeenCalledWith("abort", registeredListener, { once: true })
120102
addSpy.mockRestore()
121103
})
122104
})

‎src/test-utils/promise.ts‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
/**
2+
* Fail fast when a promise never settles. Mutations that break settle or
3+
* reject wiring would otherwise hang the test until Stryker's per-mutant
4+
* timeout, marking the mutant "Timeout" instead of "Killed".
5+
*/
6+
export function settlesWithin<T>(promise: Promise<T>, ms: number): Promise<T> {
7+
return new Promise<T>((resolve, reject) => {
8+
const timer = setTimeout(() => reject(new Error(`operation did not settle within ${ms}ms`)), ms)
9+
void promise.then(
10+
(value) => {
11+
clearTimeout(timer)
12+
resolve(value)
13+
},
14+
(error) => {
15+
clearTimeout(timer)
16+
reject(error)
17+
},
18+
)
19+
})
20+
}

0 commit comments

Comments
 (0)