From d298d4a6f6b835eac62471c010ddfa98bbd1c881 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Sat, 5 Sep 2026 22:44:24 +0800 Subject: [PATCH 01/17] feat(api): abort signal support for requesty (completePrompt + shared helpers) --- src/api/providers/__tests__/requesty.spec.ts | 351 ++++++++++++++++-- src/api/providers/requesty.ts | 48 ++- .../utils/__tests__/abort-signal.spec.ts | 96 +++++ src/api/providers/utils/abort-signal.ts | 32 ++ 4 files changed, 501 insertions(+), 26 deletions(-) diff --git a/src/api/providers/__tests__/requesty.spec.ts b/src/api/providers/__tests__/requesty.spec.ts index f256e5b1c1..bbad8dbd94 100644 --- a/src/api/providers/__tests__/requesty.spec.ts +++ b/src/api/providers/__tests__/requesty.spec.ts @@ -9,6 +9,8 @@ const MOCK_TIMEOUT_MS = 300_000 import { Anthropic } from "@anthropic-ai/sdk" import OpenAI from "openai" +import type { ModelRecord } from "@roo-code/types" + import { RequestyHandler } from "../requesty" import { Package } from "../../../shared/package" import { ApiHandlerCreateMessageMetadata } from "../../index" @@ -16,6 +18,33 @@ import { makeApiHandlerOptions } from "../../../test-utils/api" import { asyncStreamFrom, collectStream } from "../../../test-utils/stream" import { clearAllMocks } from "../../../test-utils/reset" +/** + * Stryker guard: fails fast if `promise` does not settle within `ms`. + * + * Stryker's per-mutant cutoff (timeoutMS 5s x timeoutFactor 1.5 ~= 7.5s) is shorter + * than vitest's testTimeout (20s). A mutant that removes a settle call (or an abort + * listener) leaves an awaited promise pending forever; without this guard the test + * would outlive the cutoff and the mutant would be reported as Timeout (inconclusive). + * Settling the guard at 500ms turns those mutants into fast failures (KILLED). + */ +function withSettleGuard(promise: Promise, ms = 500): Promise { + return new Promise((resolve, reject) => { + const timer = setTimeout(() => { + reject(new Error(`settle guard timed out after ${ms}ms`)) + }, ms) + void promise.then( + (value) => { + clearTimeout(timer) + resolve(value) + }, + (error) => { + clearTimeout(timer) + reject(error) + }, + ) + }) +} + const mockCreate = vitest.fn() vitest.mock("openai", () => { @@ -613,6 +642,13 @@ describe("RequestyHandler", () => { }) describe("completePrompt", () => { + // The createMessage tests leave behind a persistent stream mock plus queued + // one-shot implementations; reset so each completePrompt test starts from a clean + // mock (its own mockSetup below is authoritative). + beforeEach(() => { + mockCreate.mockReset() + }) + it("returns correct response", async () => { const handler = new RequestyHandler(mockOptions) const mockResponse = { choices: [{ message: { content: "test completion" } }] } @@ -623,12 +659,15 @@ describe("RequestyHandler", () => { expect(result).toBe("test completion") - expect(mockCreate).toHaveBeenCalledWith({ - model: mockOptions.requestyModelId, - max_tokens: 8192, - messages: [{ role: "system", content: "test prompt" }], - temperature: 0, - }) + expect(mockCreate).toHaveBeenCalledWith( + { + model: mockOptions.requestyModelId, + max_tokens: 8192, + messages: [{ role: "system", content: "test prompt" }], + temperature: 0, + }, + {}, + ) }) it("omits temperature for Claude Fable 5 in completePrompt", async () => { @@ -642,12 +681,15 @@ describe("RequestyHandler", () => { await handler.completePrompt("test prompt") - expect(mockCreate).toHaveBeenCalledWith({ - model: "anthropic/claude-fable-5", - max_tokens: 8192, - messages: [{ role: "system", content: "test prompt" }], - temperature: undefined, - }) + expect(mockCreate).toHaveBeenCalledWith( + { + model: "anthropic/claude-fable-5", + max_tokens: 8192, + messages: [{ role: "system", content: "test prompt" }], + temperature: undefined, + }, + {}, + ) }) it("omits temperature for Claude Sonnet 5 in completePrompt", async () => { @@ -661,12 +703,15 @@ describe("RequestyHandler", () => { await handler.completePrompt("test prompt") - expect(mockCreate).toHaveBeenCalledWith({ - model: "anthropic/claude-sonnet-5", - max_tokens: 8192, - messages: [{ role: "system", content: "test prompt" }], - temperature: undefined, - }) + expect(mockCreate).toHaveBeenCalledWith( + { + model: "anthropic/claude-sonnet-5", + max_tokens: 8192, + messages: [{ role: "system", content: "test prompt" }], + temperature: undefined, + }, + {}, + ) }) it("omits temperature for Claude Opus 5 in completePrompt", async () => { @@ -680,12 +725,15 @@ describe("RequestyHandler", () => { await handler.completePrompt("test prompt") - expect(mockCreate).toHaveBeenCalledWith({ - model: "anthropic/claude-opus-5", - max_tokens: 8192, - messages: [{ role: "system", content: "test prompt" }], - temperature: undefined, - }) + expect(mockCreate).toHaveBeenCalledWith( + { + model: "anthropic/claude-opus-5", + max_tokens: 8192, + messages: [{ role: "system", content: "test prompt" }], + temperature: undefined, + }, + {}, + ) }) it("handles API errors", async () => { @@ -702,5 +750,260 @@ describe("RequestyHandler", () => { await expect(handler.completePrompt("test prompt")).rejects.toThrow("Unexpected error") }) + it("should pass abort signal through to client", async () => { + const handler = new RequestyHandler(mockOptions) + const controller = new AbortController() + mockCreate.mockResolvedValueOnce({ choices: [{ message: { content: "response" } }] }) + + await handler.completePrompt("test prompt", { abortSignal: controller.signal }) + expect(mockCreate).toHaveBeenCalledWith(expect.objectContaining({ model: expect.any(String) }), { + signal: controller.signal, + }) + }) + + it("should pass timeout through to client", async () => { + const handler = new RequestyHandler(mockOptions) + mockCreate.mockResolvedValueOnce({ choices: [{ message: { content: "response" } }] }) + + await handler.completePrompt("test prompt", { timeoutMs: 5000 }) + expect(mockCreate).toHaveBeenCalledWith( + expect.objectContaining({ model: expect.any(String) }), + expect.objectContaining({ + timeout: 5000, + }), + ) + }) + + it("should work without options (backward compatible)", async () => { + const handler = new RequestyHandler(mockOptions) + mockCreate.mockResolvedValueOnce({ choices: [{ message: { content: "response" } }] }) + + const result = await handler.completePrompt("test prompt") + expect(result).toBe("response") + }) + + it("rejects with AbortError when the signal is pre-aborted", async () => { + const handler = new RequestyHandler(mockOptions) + mockCreate.mockResolvedValueOnce({ choices: [{ message: { content: "response" } }] }) + + const controller = new AbortController() + controller.abort() + + await expect( + handler.completePrompt("test prompt", { abortSignal: controller.signal }), + ).rejects.toMatchObject({ + name: "AbortError", + message: "This operation was aborted", + }) + }) + + it("rejects with AbortError when the signal aborts during model lookup", async () => { + const handler = new RequestyHandler(mockOptions) + const controller = new AbortController() + + // Model discovery is deferred and never settles: with rejectOnAbort racing the + // lookup, the abort must end the request before the lookup resolves. + let notifyLookupStarted!: () => void + const lookupStarted = new Promise((resolve) => { + notifyLookupStarted = resolve + }) + const deferredModelLookup = new Promise(() => {}) + const { getModels } = await import("../fetchers/modelCache") + vitest.mocked(getModels).mockImplementationOnce(() => { + notifyLookupStarted() + return deferredModelLookup + }) + + const promise = handler.completePrompt("test prompt", { abortSignal: controller.signal }) + // Defensive: the fast-fail path may reject before the barrier below settles, + // which would otherwise surface as an unhandled rejection. + void promise.catch(() => {}) + await withSettleGuard(lookupStarted) + controller.abort() + + await expect(withSettleGuard(promise)).rejects.toMatchObject({ + name: "AbortError", + message: "The Requesty request was aborted", + }) + expect(mockCreate).not.toHaveBeenCalled() + }) + + it("rethrows non-abort model lookup failures from completePrompt", async () => { + const handler = new RequestyHandler(mockOptions) + const { getModels } = await import("../fetchers/modelCache") + const lookupError = new Error("lookup failed") + vitest.mocked(getModels).mockImplementationOnce(() => { + return Promise.reject(lookupError) + }) + + await expect(handler.completePrompt("test prompt")).rejects.toThrow("lookup failed") + }) + + it("normalizes raw AbortError lookup failures to the provider AbortError", async () => { + const handler = new RequestyHandler(mockOptions) + const { getModels } = await import("../fetchers/modelCache") + const rawAbort = new Error("The user aborted a request") + rawAbort.name = "AbortError" + vitest.mocked(getModels).mockImplementationOnce(() => { + return Promise.reject(rawAbort) + }) + + await expect(handler.completePrompt("test prompt")).rejects.toMatchObject({ + name: "AbortError", + message: "The Requesty request was aborted", + }) + }) + + it("rejects with AbortError when aborted mid-flight", async () => { + const handler = new RequestyHandler(mockOptions) + const controller = new AbortController() + + // Deterministic synchronization (mirrors the Requesty test): the mock notifies the + // test when the request actually starts, so the abort lands mid-flight (after model + // lookup) instead of winning the race at model discovery on a slow runner. + let notifyCreateStarted!: () => void + const createStarted = new Promise((resolve) => { + notifyCreateStarted = resolve + }) + mockCreate.mockImplementationOnce(async (_params: unknown, options?: { signal?: AbortSignal }) => { + notifyCreateStarted() + // Emulate the OpenAI SDK: the in-flight request rejects when the signal aborts. + await new Promise((resolve) => { + if (options?.signal?.aborted) { + resolve() + } else { + options?.signal?.addEventListener("abort", () => resolve(), { once: true }) + } + }) + const abortError = new Error("The user aborted a request") + abortError.name = "AbortError" + throw abortError + }) + + const promise = handler.completePrompt("test prompt", { abortSignal: controller.signal }) + // Defensive: the fast-fail path may reject before the barrier below settles, + // which would otherwise surface as an unhandled rejection. + void promise.catch(() => {}) + // Abort only once create() has actually started (after model lookup). + await withSettleGuard(createStarted) + controller.abort() + + await expect(withSettleGuard(promise)).rejects.toMatchObject({ + name: "AbortError", + message: "The Requesty request was aborted", + }) + }) + it("rejects with AbortError when only a timeout is provided and it elapses", async () => { + const handler = new RequestyHandler(mockOptions) + mockCreate.mockImplementationOnce(async (_params: unknown, options?: { signal?: AbortSignal }) => { + // Emulate the OpenAI SDK: the in-flight request rejects when the signal times out. + await new Promise((resolve) => { + if (options?.signal?.aborted) { + resolve() + } else { + options?.signal?.addEventListener("abort", () => resolve(), { once: true }) + } + }) + const timeoutError = new Error("TimeoutError: Request timed out.") + timeoutError.name = "TimeoutError" + throw timeoutError + }) + + await expect(handler.completePrompt("test prompt", { timeoutMs: 50 })).rejects.toMatchObject({ + name: "AbortError", + }) + }) + + it("does not return a late result when the response resolves after abort", async () => { + const handler = new RequestyHandler(mockOptions) + const controller = new AbortController() + + // Emulate the OpenAI SDK: the pending request resolves once the signal aborts, + // i.e. after the caller has already cancelled. + let notifyCreateStarted!: () => void + const createStarted = new Promise((resolve) => { + notifyCreateStarted = resolve + }) + mockCreate.mockImplementationOnce(async (_params: unknown, options?: { signal?: AbortSignal }) => { + notifyCreateStarted() + await new Promise((resolve) => { + if (options?.signal?.aborted) { + resolve() + } else { + options?.signal?.addEventListener("abort", () => resolve(), { once: true }) + } + }) + return { choices: [{ message: { content: "late" } }] } + }) + + const promise = handler.completePrompt("test prompt", { abortSignal: controller.signal }) + // Defensive: the fast-fail path may reject before the barrier below settles, + // which would otherwise surface as an unhandled rejection. + void promise.catch(() => {}) + // Abort while the request is in flight; the resolved response is late and must + // be discarded instead of returned. + await withSettleGuard(createStarted) + controller.abort() + + await expect(withSettleGuard(promise)).rejects.toMatchObject({ + name: "AbortError", + message: "The Requesty request was aborted", + }) + }) + + it("does not forward a non-positive timeout to the client", async () => { + const handler = new RequestyHandler(mockOptions) + mockCreate.mockResolvedValueOnce({ choices: [{ message: { content: "response" } }] }) + + await handler.completePrompt("test prompt", { timeoutMs: 0 }) + expect(mockCreate).toHaveBeenCalledWith(expect.objectContaining({ model: expect.any(String) }), {}) + }) + + it("rejects with AbortError when both an abort signal and a timeout are provided", async () => { + const handler = new RequestyHandler(mockOptions) + const controller = new AbortController() + + let notifyCreateStarted!: () => void + const createStarted = new Promise((resolve) => { + notifyCreateStarted = resolve + }) + let requestSignal: AbortSignal | undefined + mockCreate.mockImplementationOnce(async (_params: unknown, options?: { signal?: AbortSignal }) => { + notifyCreateStarted() + requestSignal = options?.signal + await new Promise((resolve) => { + if (options?.signal?.aborted) { + resolve() + } else { + options?.signal?.addEventListener("abort", () => resolve(), { once: true }) + } + }) + const abortError = new Error("The user aborted a request") + abortError.name = "AbortError" + throw abortError + }) + + const promise = handler.completePrompt("test prompt", { + abortSignal: controller.signal, + timeoutMs: 100_000, + }) + // Defensive: the fast-fail path may reject before the barrier below settles, + // which would otherwise surface as an unhandled rejection. + void promise.catch(() => {}) + // Abort only once create() has actually started (after model lookup). + await withSettleGuard(createStarted) + controller.abort() + + await expect(withSettleGuard(promise)).rejects.toMatchObject({ name: "AbortError" }) + // The SDK received a merged signal (not the caller's signal) plus the timeout. + expect(requestSignal).toBeDefined() + expect(requestSignal).not.toBe(controller.signal) + expect(mockCreate).toHaveBeenCalledWith( + expect.objectContaining({ model: expect.any(String) }), + expect.objectContaining({ + timeout: 100_000, + }), + ) + }) }) }) diff --git a/src/api/providers/requesty.ts b/src/api/providers/requesty.ts index 2c1092d303..f77beba9cd 100644 --- a/src/api/providers/requesty.ts +++ b/src/api/providers/requesty.ts @@ -23,6 +23,13 @@ import { BaseProvider } from "./base-provider" import type { SingleCompletionHandler, ApiHandlerCreateMessageMetadata, CompletePromptOptions } from "../index" import { toRequestyServiceUrl } from "../../shared/utils/requesty" import { handleOpenAIError } from "./utils/error-handler" +import { + createAbortError, + isRequestAborted, + mergeAbortSignalAndTimeout, + rejectOnAbort, + throwIfAborted, +} from "./utils/abort-signal" import { applyRouterToolPreferences } from "./utils/router-tool-preferences" import { extractReasoningFromDelta } from "./utils/extract-reasoning" @@ -211,7 +218,25 @@ export class RequestyHandler extends BaseProvider implements SingleCompletionHan } async completePrompt(prompt: string, options?: CompletePromptOptions): Promise { - const { id: model, maxTokens: max_tokens, temperature } = await this.fetchModel() + // Establish the cancellation scope before model lookup: a pre-aborted call, or + // one aborted while model metadata is loading, must reject promptly instead of + // waiting for the lookup to settle. The configured timeoutMs covers the lookup + // as well. + const requestAbortSignal = mergeAbortSignalAndTimeout(options?.abortSignal, options?.timeoutMs) + throwIfAborted(requestAbortSignal) + + let modelData: Awaited> + try { + modelData = requestAbortSignal + ? await rejectOnAbort(this.fetchModel(), requestAbortSignal, this.providerName) + : await this.fetchModel() + } catch (error) { + if (isRequestAborted(error, requestAbortSignal)) { + throw createAbortError(this.providerName) + } + throw error + } + const { id: model, maxTokens: max_tokens, temperature } = modelData const openAiMessages: OpenAI.Chat.ChatCompletionMessageParam[] = [{ role: "system", content: prompt }] @@ -222,12 +247,31 @@ export class RequestyHandler extends BaseProvider implements SingleCompletionHan temperature: temperature, } + // The merged abort signal (established before model lookup, above) is forwarded to the + // SDK so both abort and timeout reject with a DOM-standard AbortError in the catch + // below. The client-level timeout remains the default safety net; 0 is never passed + // to the SDK timeout. + const createOptions: OpenAI.RequestOptions = { + ...(requestAbortSignal && { signal: requestAbortSignal }), + ...(typeof options?.timeoutMs === "number" && options.timeoutMs > 0 && { timeout: options.timeoutMs }), + } + let response: OpenAI.Chat.ChatCompletion try { - response = await this.client.chat.completions.create(completionParams) + response = await this.client.chat.completions.create(completionParams, createOptions) } catch (error) { + // Aborted requests are user-initiated: surface them as AbortError (this also covers + // timeouts, which abort the same signal) instead of a completion error. + if (requestAbortSignal?.aborted) { + throw createAbortError("Requesty") + } throw handleOpenAIError(error, this.providerName) } + + if (requestAbortSignal?.aborted) { + // The response resolved after the request was aborted: do not return the late result. + throw createAbortError("Requesty") + } return response.choices[0]?.message.content || "" } } diff --git a/src/api/providers/utils/__tests__/abort-signal.spec.ts b/src/api/providers/utils/__tests__/abort-signal.spec.ts index 1692f71e63..c95b800d8c 100644 --- a/src/api/providers/utils/__tests__/abort-signal.spec.ts +++ b/src/api/providers/utils/__tests__/abort-signal.spec.ts @@ -3,9 +3,105 @@ import { isRequestAborted, mergeAbortSignalAndTimeout, mergeAbortSignals, + rejectOnAbort, throwIfAborted, } from "../abort-signal" +/** + * Stryker guard: fails fast if `promise` does not settle within `ms`. + * + * Stryker's per-mutant cutoff (timeoutMS 5s x timeoutFactor 1.5 ~= 7.5s) is shorter + * than vitest's testTimeout (20s). A mutant that removes a settle call (or an abort + * listener) leaves an awaited promise pending forever; without this guard the test + * would outlive the cutoff and the mutant would be reported as Timeout (inconclusive). + * Settling the guard at 500ms turns those mutants into fast failures (KILLED). + */ +function withSettleGuard(promise: Promise, ms = 500): Promise { + return new Promise((resolve, reject) => { + const timer = setTimeout(() => { + reject(new Error(`settle guard timed out after ${ms}ms`)) + }, ms) + void promise.then( + (value) => { + clearTimeout(timer) + resolve(value) + }, + (error) => { + clearTimeout(timer) + reject(error) + }, + ) + }) +} + +describe("rejectOnAbort", () => { + it("resolves with the pending value when it settles before the signal aborts", async () => { + const controller = new AbortController() + + await expect( + withSettleGuard(rejectOnAbort(Promise.resolve("done"), controller.signal, "TestProvider")), + ).resolves.toBe("done") + expect(controller.signal.aborted).toBe(false) + }) + + it("rejects with the provider abort error when the signal aborts first", async () => { + const controller = new AbortController() + // Never settles: the race must end purely via the abort. + const pending = new Promise(() => {}) + const race = rejectOnAbort(pending, controller.signal, "TestProvider") + controller.abort() + + await expect(withSettleGuard(race)).rejects.toMatchObject({ + name: "AbortError", + message: "The TestProvider request was aborted", + }) + }) + + it("rejects immediately when the signal is already aborted", async () => { + const controller = new AbortController() + controller.abort() + const pending = new Promise(() => {}) + + await expect(withSettleGuard(rejectOnAbort(pending, controller.signal, "TestProvider"))).rejects.toMatchObject({ + name: "AbortError", + }) + }) + + it("propagates the pending rejection when the signal stays active", async () => { + const controller = new AbortController() + const boom = new Error("lookup failed") + + await expect( + withSettleGuard(rejectOnAbort(Promise.reject(boom), controller.signal, "TestProvider")), + ).rejects.toBe(boom) + }) + + it("detaches the abort listener once the pending settles", async () => { + const controller = new AbortController() + const removeSpy = vi.spyOn(controller.signal, "removeEventListener") + + await expect( + withSettleGuard(rejectOnAbort(Promise.resolve("done"), controller.signal, "TestProvider")), + ).resolves.toBe("done") + + expect(removeSpy).toHaveBeenCalledWith("abort", expect.any(Function)) + removeSpy.mockRestore() + }) + + it("detaches the abort listener when the pending rejects", async () => { + const controller = new AbortController() + const removeSpy = vi.spyOn(controller.signal, "removeEventListener") + const lookupError = new Error("lookup failed") + + await expect( + withSettleGuard(rejectOnAbort(Promise.reject(lookupError), controller.signal, "TestProvider")), + ).rejects.toBe(lookupError) + + expect(removeSpy).toHaveBeenCalledWith("abort", expect.any(Function)) + removeSpy.mockRestore() + }) +}) + describe("abort-signal utilities", () => { describe("mergeAbortSignalAndTimeout", () => { it("returns undefined when no signal or positive timeout is provided", () => { diff --git a/src/api/providers/utils/abort-signal.ts b/src/api/providers/utils/abort-signal.ts index 26f57c3e9a..bd7d579b00 100644 --- a/src/api/providers/utils/abort-signal.ts +++ b/src/api/providers/utils/abort-signal.ts @@ -93,3 +93,35 @@ export function createAbortError(providerName: string): Error { abortError.name = "AbortError" return abortError } + +/** + * Await `pending` but reject with the provider's abort error when `signal` + * aborts first. For async phases that have no native signal support (model + * discovery) yet must still settle promptly on cancellation. The underlying + * promise keeps running (its settlement is ignored) — cancellation is + * cooperative at this boundary. + * + * The abort listener is detached once `pending` settles (success or + * failure), so repeated calls on one signal do not accumulate listeners. + */ +export function rejectOnAbort(pending: Promise, signal: AbortSignal, providerName: string): Promise { + if (signal.aborted) { + return Promise.reject(createAbortError(providerName)) + } + + return new Promise((resolve, reject) => { + const onAbort = () => reject(createAbortError(providerName)) + // Stryker disable next-line ObjectLiteral,BooleanLiteral: a signal fires its abort event exactly once and the settle handler removes this listener, so the once flag is unobservable + signal.addEventListener("abort", onAbort, { once: true }) + void pending.then( + (value) => { + signal.removeEventListener("abort", onAbort) + resolve(value) + }, + (error) => { + signal.removeEventListener("abort", onAbort) + reject(error) + }, + ) + }) +} From ebd5404df6c5b415e2507211c74d1aa810a4513e Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Sun, 6 Sep 2026 01:04:58 +0800 Subject: [PATCH 02/17] test(api): strengthen requesty abort assertions and share settle guard helper --- src/api/providers/__tests__/requesty.spec.ts | 61 ++++++++----------- src/api/providers/requesty.ts | 4 +- .../utils/__tests__/abort-signal.spec.ts | 51 +++++++--------- src/test-utils/settle-guard.ts | 26 ++++++++ 4 files changed, 77 insertions(+), 65 deletions(-) create mode 100644 src/test-utils/settle-guard.ts diff --git a/src/api/providers/__tests__/requesty.spec.ts b/src/api/providers/__tests__/requesty.spec.ts index bbad8dbd94..4a3faeb34a 100644 --- a/src/api/providers/__tests__/requesty.spec.ts +++ b/src/api/providers/__tests__/requesty.spec.ts @@ -17,33 +17,7 @@ import { ApiHandlerCreateMessageMetadata } from "../../index" import { makeApiHandlerOptions } from "../../../test-utils/api" import { asyncStreamFrom, collectStream } from "../../../test-utils/stream" import { clearAllMocks } from "../../../test-utils/reset" - -/** - * Stryker guard: fails fast if `promise` does not settle within `ms`. - * - * Stryker's per-mutant cutoff (timeoutMS 5s x timeoutFactor 1.5 ~= 7.5s) is shorter - * than vitest's testTimeout (20s). A mutant that removes a settle call (or an abort - * listener) leaves an awaited promise pending forever; without this guard the test - * would outlive the cutoff and the mutant would be reported as Timeout (inconclusive). - * Settling the guard at 500ms turns those mutants into fast failures (KILLED). - */ -function withSettleGuard(promise: Promise, ms = 500): Promise { - return new Promise((resolve, reject) => { - const timer = setTimeout(() => { - reject(new Error(`settle guard timed out after ${ms}ms`)) - }, ms) - void promise.then( - (value) => { - clearTimeout(timer) - resolve(value) - }, - (error) => { - clearTimeout(timer) - reject(error) - }, - ) - }) -} +import { withSettleGuard } from "../../../test-utils/settle-guard" const mockCreate = vitest.fn() @@ -765,13 +739,31 @@ describe("RequestyHandler", () => { const handler = new RequestyHandler(mockOptions) mockCreate.mockResolvedValueOnce({ choices: [{ message: { content: "response" } }] }) - await handler.completePrompt("test prompt", { timeoutMs: 5000 }) - expect(mockCreate).toHaveBeenCalledWith( - expect.objectContaining({ model: expect.any(String) }), - expect.objectContaining({ - timeout: 5000, - }), - ) + // Capture the exact timeout signal instance the provider creates so the + // test can assert identity, not just type, for the signal it forwards. + const timeoutSignalSpy = vitest.spyOn(AbortSignal, "timeout") + try { + await handler.completePrompt("test prompt", { timeoutMs: 5000 }) + // Assert the factory argument and call count too: the identity check + // below would also pass for a signal created with the wrong duration. + expect(timeoutSignalSpy).toHaveBeenCalledTimes(1) + expect(timeoutSignalSpy).toHaveBeenCalledWith(5000) + expect(mockCreate).toHaveBeenCalledWith( + expect.objectContaining({ model: expect.any(String) }), + expect.objectContaining({ timeout: 5000 }), + ) + // Without a caller signal the merged signal is exactly the + // AbortSignal.timeout instance; the SDK relies on it to reject with a + // DOM-standard AbortError when the timeout fires. + const clientOptions = mockCreate.mock.calls[0]?.[1] as { signal?: AbortSignal } | undefined + const expectedSignal = timeoutSignalSpy.mock.results[0]?.value + expect(expectedSignal).toBeInstanceOf(AbortSignal) + expect(clientOptions?.signal).toBe(expectedSignal) + } finally { + // Restore on every exit path: a failed assertion above must not leave + // the static spy installed for the remaining tests in this file. + timeoutSignalSpy.mockRestore() + } }) it("should work without options (backward compatible)", async () => { @@ -795,6 +787,7 @@ describe("RequestyHandler", () => { name: "AbortError", message: "This operation was aborted", }) + expect(mockCreate).not.toHaveBeenCalled() }) it("rejects with AbortError when the signal aborts during model lookup", async () => { diff --git a/src/api/providers/requesty.ts b/src/api/providers/requesty.ts index f77beba9cd..2249ca21b2 100644 --- a/src/api/providers/requesty.ts +++ b/src/api/providers/requesty.ts @@ -263,14 +263,14 @@ export class RequestyHandler extends BaseProvider implements SingleCompletionHan // Aborted requests are user-initiated: surface them as AbortError (this also covers // timeouts, which abort the same signal) instead of a completion error. if (requestAbortSignal?.aborted) { - throw createAbortError("Requesty") + throw createAbortError(this.providerName) } throw handleOpenAIError(error, this.providerName) } if (requestAbortSignal?.aborted) { // The response resolved after the request was aborted: do not return the late result. - throw createAbortError("Requesty") + throw createAbortError(this.providerName) } return response.choices[0]?.message.content || "" } diff --git a/src/api/providers/utils/__tests__/abort-signal.spec.ts b/src/api/providers/utils/__tests__/abort-signal.spec.ts index c95b800d8c..04d657b564 100644 --- a/src/api/providers/utils/__tests__/abort-signal.spec.ts +++ b/src/api/providers/utils/__tests__/abort-signal.spec.ts @@ -6,33 +6,7 @@ import { rejectOnAbort, throwIfAborted, } from "../abort-signal" - -/** - * Stryker guard: fails fast if `promise` does not settle within `ms`. - * - * Stryker's per-mutant cutoff (timeoutMS 5s x timeoutFactor 1.5 ~= 7.5s) is shorter - * than vitest's testTimeout (20s). A mutant that removes a settle call (or an abort - * listener) leaves an awaited promise pending forever; without this guard the test - * would outlive the cutoff and the mutant would be reported as Timeout (inconclusive). - * Settling the guard at 500ms turns those mutants into fast failures (KILLED). - */ -function withSettleGuard(promise: Promise, ms = 500): Promise { - return new Promise((resolve, reject) => { - const timer = setTimeout(() => { - reject(new Error(`settle guard timed out after ${ms}ms`)) - }, ms) - void promise.then( - (value) => { - clearTimeout(timer) - resolve(value) - }, - (error) => { - clearTimeout(timer) - reject(error) - }, - ) - }) -} +import { withSettleGuard } from "../../../../test-utils/settle-guard" describe("rejectOnAbort", () => { it("resolves with the pending value when it settles before the signal aborts", async () => { @@ -64,6 +38,7 @@ describe("rejectOnAbort", () => { await expect(withSettleGuard(rejectOnAbort(pending, controller.signal, "TestProvider"))).rejects.toMatchObject({ name: "AbortError", + message: "The TestProvider request was aborted", }) }) @@ -78,18 +53,28 @@ describe("rejectOnAbort", () => { it("detaches the abort listener once the pending settles", async () => { const controller = new AbortController() + const addSpy = vi.spyOn(controller.signal, "addEventListener") const removeSpy = vi.spyOn(controller.signal, "removeEventListener") await expect( withSettleGuard(rejectOnAbort(Promise.resolve("done"), controller.signal, "TestProvider")), ).resolves.toBe("done") - expect(removeSpy).toHaveBeenCalledWith("abort", expect.any(Function)) + // The settle path must remove the exact listener that was registered, not just any + // function: removing a different reference would leave the original abort listener + // attached to the signal. First require the registration to have happened at all, + // so a missing registration cannot silently degrade to an undefined comparison. + expect(addSpy).toHaveBeenCalledTimes(1) + const registeredListener = addSpy.mock.calls[0]?.[1] as EventListener | undefined + expect(typeof registeredListener).toBe("function") + expect(removeSpy).toHaveBeenCalledWith("abort", registeredListener) + addSpy.mockRestore() removeSpy.mockRestore() }) it("detaches the abort listener when the pending rejects", async () => { const controller = new AbortController() + const addSpy = vi.spyOn(controller.signal, "addEventListener") const removeSpy = vi.spyOn(controller.signal, "removeEventListener") const lookupError = new Error("lookup failed") @@ -97,7 +82,15 @@ describe("rejectOnAbort", () => { withSettleGuard(rejectOnAbort(Promise.reject(lookupError), controller.signal, "TestProvider")), ).rejects.toBe(lookupError) - expect(removeSpy).toHaveBeenCalledWith("abort", expect.any(Function)) + // The settle path must remove the exact listener that was registered, not just any + // function: removing a different reference would leave the original abort listener + // attached to the signal. First require the registration to have happened at all, + // so a missing registration cannot silently degrade to an undefined comparison. + expect(addSpy).toHaveBeenCalledTimes(1) + const registeredListener = addSpy.mock.calls[0]?.[1] as EventListener | undefined + expect(typeof registeredListener).toBe("function") + expect(removeSpy).toHaveBeenCalledWith("abort", registeredListener) + addSpy.mockRestore() removeSpy.mockRestore() }) }) diff --git a/src/test-utils/settle-guard.ts b/src/test-utils/settle-guard.ts new file mode 100644 index 0000000000..1efe5286c8 --- /dev/null +++ b/src/test-utils/settle-guard.ts @@ -0,0 +1,26 @@ +/** + * Stryker guard: fails fast if `promise` does not settle within `ms`. + * + * Stryker's per-mutant cutoff (timeoutMS 5s x timeoutFactor 1.5 ~= 7.5s) is shorter + * than vitest's testTimeout (20s). A mutant that removes a settle call (or an abort + * listener) leaves an awaited promise pending forever; without this guard the test + * would outlive the cutoff and the mutant would be reported as Timeout (inconclusive). + * Settling the guard at 500ms turns those mutants into fast failures (KILLED). + */ +export function withSettleGuard(promise: Promise, ms = 500): Promise { + return new Promise((resolve, reject) => { + const timer = setTimeout(() => { + reject(new Error(`settle guard timed out after ${ms}ms`)) + }, ms) + void promise.then( + (value) => { + clearTimeout(timer) + resolve(value) + }, + (error) => { + clearTimeout(timer) + reject(error) + }, + ) + }) +} From 2d915cb15d2d5ffe2c1cdde4188247e15f841eba Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Mon, 7 Sep 2026 06:04:38 +0800 Subject: [PATCH 03/17] test(api): assert pre-aborted Requesty completePrompt skips model lookup (CodeRabbit finding) --- src/api/providers/__tests__/requesty.spec.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/api/providers/__tests__/requesty.spec.ts b/src/api/providers/__tests__/requesty.spec.ts index 4a3faeb34a..de7a4df778 100644 --- a/src/api/providers/__tests__/requesty.spec.ts +++ b/src/api/providers/__tests__/requesty.spec.ts @@ -788,6 +788,10 @@ describe("RequestyHandler", () => { message: "This operation was aborted", }) expect(mockCreate).not.toHaveBeenCalled() + // Pre-flight cancellation must skip model lookup entirely: the + // getModels mock must remain uncalled, not just the SDK create call. + const { getModels } = await import("../fetchers/modelCache") + expect(vitest.mocked(getModels)).not.toHaveBeenCalled() }) it("rejects with AbortError when the signal aborts during model lookup", async () => { From 58b4ef12fbc214e899948f1f48bf39f5b33a38fe Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Fri, 11 Sep 2026 13:13:15 +0800 Subject: [PATCH 04/17] chore: re-trigger CodeRabbit review From 84baa462fa42d34a2749a39f8c31114f807306a4 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sat, 3 Oct 2026 03:08:13 +0800 Subject: [PATCH 05/17] chore: re-trigger CodeRabbit re-review and label reconcile From 137e071dee455d2dc2edf4dbd921a175f0c52b76 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sat, 3 Oct 2026 05:58:11 +0800 Subject: [PATCH 06/17] chore: trigger CodeRabbit re-review (findings already addressed) From 1ad08bc73d5b8e316f7aa75b61803c421a13e65a Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sat, 3 Oct 2026 07:35:52 +0800 Subject: [PATCH 07/17] chore: trigger CodeRabbit re-review / label reconcile From 2d12c0110859167d83cef442bc12d04383997c3f Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sat, 3 Oct 2026 08:12:00 +0800 Subject: [PATCH 08/17] chore: trigger CodeRabbit re-review at current head (all findings already addressed; stale review commit needs a fresh pass) From a0173d875f2e7b7f3e72621c7b00b727b583ce90 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sat, 3 Oct 2026 09:11:59 +0800 Subject: [PATCH 09/17] fix(abort): consume the pending rejection when the signal is already aborted CodeRabbit finding at 2d12c01108: rejectOnAbort returned early on an already-aborted signal without attaching a handler to pending, so a later rejection of the in-flight work surfaced as an unhandled rejection. The early return now consumes it; the test asserts no unhandled rejection is reported. --- .../utils/__tests__/abort-signal.spec.ts | 24 +++++++++++++++++++ src/api/providers/utils/abort-signal.ts | 5 ++++ 2 files changed, 29 insertions(+) diff --git a/src/api/providers/utils/__tests__/abort-signal.spec.ts b/src/api/providers/utils/__tests__/abort-signal.spec.ts index 04d657b564..c65841f546 100644 --- a/src/api/providers/utils/__tests__/abort-signal.spec.ts +++ b/src/api/providers/utils/__tests__/abort-signal.spec.ts @@ -42,6 +42,30 @@ describe("rejectOnAbort", () => { }) }) + it("consumes a later pending rejection when the signal is already aborted", async () => { + const unhandled: unknown[] = [] + const onUnhandled = (reason: unknown) => unhandled.push(reason) + process.on("unhandledRejection", onUnhandled) + try { + const controller = new AbortController() + controller.abort() + // Rejects after the early return: only a handler attached inside + // rejectOnAbort can consume it. + const pending = Promise.reject(new Error("late lookup failure")) + + await expect(rejectOnAbort(pending, controller.signal, "TestProvider")).rejects.toMatchObject({ + name: "AbortError", + message: "The TestProvider request was aborted", + }) + + // Let the pending rejection settle and be reported. + await new Promise((resolve) => setImmediate(resolve)) + expect(unhandled).toHaveLength(0) + } finally { + process.off("unhandledRejection", onUnhandled) + } + }) + it("propagates the pending rejection when the signal stays active", async () => { const controller = new AbortController() const boom = new Error("lookup failed") diff --git a/src/api/providers/utils/abort-signal.ts b/src/api/providers/utils/abort-signal.ts index bd7d579b00..6d95f5b625 100644 --- a/src/api/providers/utils/abort-signal.ts +++ b/src/api/providers/utils/abort-signal.ts @@ -106,6 +106,11 @@ export function createAbortError(providerName: string): Error { */ export function rejectOnAbort(pending: Promise, signal: AbortSignal, providerName: string): Promise { if (signal.aborted) { + // The pending work keeps running past this early return, so its later + // rejection has to be consumed here: without a handler it surfaces as an + // unhandled rejection. The settlement is intentionally ignored at this + // boundary (cancellation is cooperative). + void pending.catch(() => {}) return Promise.reject(createAbortError(providerName)) } From 145a93d6486826d82fec0c82f09b0c87b6b1ae2e Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sun, 4 Oct 2026 14:26:56 +0800 Subject: [PATCH 10/17] Re-trigger review at the current head Every review thread on this PR is resolved and CI is green at this head; the review decision still points at an older commit. This empty commit re-runs the review so the decision and the label reflect the current head. From 2c25eaed8050f017234c764e1cf4563a8f9ef0ed Mon Sep 17 00:00:00 2001 From: dsh-agent Date: Thu, 22 Oct 2026 02:23:18 +0800 Subject: [PATCH 11/17] test(requesty): pin the provider abort message in the timeout test The name assertion alone only proves that a TimeoutError did not reach handleOpenAIError; it does not pin the complete abort contract. Assert the provider-specific message the sibling cancellation tests use. --- src/api/providers/__tests__/requesty.spec.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/api/providers/__tests__/requesty.spec.ts b/src/api/providers/__tests__/requesty.spec.ts index 7ece79400c..3b83924ff2 100644 --- a/src/api/providers/__tests__/requesty.spec.ts +++ b/src/api/providers/__tests__/requesty.spec.ts @@ -908,6 +908,9 @@ describe("RequestyHandler", () => { await expect(handler.completePrompt("test prompt", { timeoutMs: 50 })).rejects.toMatchObject({ name: "AbortError", + // Pin the complete contract: the timeout must be normalized to the + // provider abort message, not merely to a generic AbortError name. + message: "The Requesty request was aborted", }) }) From efb9ac8399264b90369fc6d1315133293260ccb4 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Wed, 28 Oct 2026 01:28:42 +0800 Subject: [PATCH 12/17] test(requesty): pin ordinary SDK failures with a live signal/timeout Review follow-up: an ordinary API error while a live external signal or per-request timeout is configured must stay a provider failure, not an AbortError. requesty.spec 36 passed. --- src/api/providers/__tests__/requesty.spec.ts | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/api/providers/__tests__/requesty.spec.ts b/src/api/providers/__tests__/requesty.spec.ts index 3b83924ff2..822eb0d786 100644 --- a/src/api/providers/__tests__/requesty.spec.ts +++ b/src/api/providers/__tests__/requesty.spec.ts @@ -724,6 +724,25 @@ describe("RequestyHandler", () => { await expect(handler.completePrompt("test prompt")).rejects.toThrow("Unexpected error") }) + it.each([ + ["an external signal", { abortSignal: new AbortController().signal }], + ["a per-request timeout", { timeoutMs: 5_000 }], + ])( + "does not classify an ordinary SDK failure as an abort when %s is configured", + async (_label, options: { abortSignal?: AbortSignal; timeoutMs?: number }) => { + // The catch must key on the MERGED SIGNAL BEING ABORTED, not on the signal merely + // being configured: a real API failure with a live signal is a provider error. + const handler = new RequestyHandler(mockOptions) + mockCreate.mockRejectedValue(new Error("API Error")) + + const error = await handler.completePrompt("test prompt", options).catch((e: unknown) => e) + + expect(error).toBeInstanceOf(Error) + expect((error as Error).name).not.toBe("AbortError") + expect((error as Error).message).toContain("API Error") + }, + ) + it("should pass abort signal through to client", async () => { const handler = new RequestyHandler(mockOptions) const controller = new AbortController() From 16f92dbd6e4639cf1a7c1b57d30debc3794129b7 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Wed, 28 Oct 2026 01:33:03 +0800 Subject: [PATCH 13/17] fix(requesty): forward a validated SDK timeout via the shared helpers Lint follow-up: the per-request timeout is validated with hasUsableTimeout and rounded/capped with toSdkTimeoutMs instead of forwarding the raw value (SDKs treat 0 as an immediate abort and reject fractional/non-finite values). Added the two helpers to the branch abort-signal utils. requesty + abort-signal specs 69 passed; eslint 0; tsc clean. --- src/api/providers/requesty.ts | 4 +++- src/api/providers/utils/abort-signal.ts | 13 +++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/api/providers/requesty.ts b/src/api/providers/requesty.ts index 586ba74a08..9a35e68a8f 100644 --- a/src/api/providers/requesty.ts +++ b/src/api/providers/requesty.ts @@ -30,6 +30,8 @@ import type { import { toRequestyServiceUrl } from "../../shared/utils/requesty" import { handleOpenAIError } from "./utils/error-handler" import { + hasUsableTimeout, + toSdkTimeoutMs, createAbortError, isRequestAborted, mergeAbortSignalAndTimeout, @@ -268,7 +270,7 @@ export class RequestyHandler extends BaseProvider implements SingleCompletionHan // to the SDK timeout. const createOptions: OpenAI.RequestOptions = { ...(requestAbortSignal && { signal: requestAbortSignal }), - ...(typeof options?.timeoutMs === "number" && options.timeoutMs > 0 && { timeout: options.timeoutMs }), + ...(hasUsableTimeout(options?.timeoutMs) && { timeout: toSdkTimeoutMs(options.timeoutMs) }), } let response: OpenAI.Chat.ChatCompletion diff --git a/src/api/providers/utils/abort-signal.ts b/src/api/providers/utils/abort-signal.ts index cbb7a13aaf..c3d1c3f9a6 100644 --- a/src/api/providers/utils/abort-signal.ts +++ b/src/api/providers/utils/abort-signal.ts @@ -5,6 +5,19 @@ * native AbortSignal.timeout() API, which self-manages its timer — callers do * not need to (and cannot) clear it manually. */ +/** A timeout is usable when it is finite and positive (SDKs treat 0 as immediate abort). */ +export function hasUsableTimeout(timeoutMs?: number): timeoutMs is number { + return typeof timeoutMs === "number" && Number.isFinite(timeoutMs) && timeoutMs > 0 +} + +/** Maximum delay a Node timer (and therefore an SDK timeout option) accepts. */ +export const MAX_TIMEOUT_MS = 2_147_483_647 + +/** Convert a validated timeout to the value forwarded to an SDK: round up fractions, cap at the max delay. */ +export function toSdkTimeoutMs(timeoutMs: number): number { + return Math.min(Math.max(1, Math.ceil(timeoutMs)), MAX_TIMEOUT_MS) +} + export function mergeAbortSignalAndTimeout(externalSignal?: AbortSignal, timeoutMs?: number): AbortSignal | undefined { const hasTimeout = typeof timeoutMs === "number" && timeoutMs > 0 From 7f61c0284f509ed18414a7c18f1a27df34fee824 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Wed, 28 Oct 2026 01:37:16 +0800 Subject: [PATCH 14/17] chore: drop the duplicated timeout helpers after the upstream merge upstream/main already provides hasUsableTimeout/toSdkTimeoutMs/MAX_TIMEOUT_MS, so the branch-local copies are removed; requesty keeps using the shared helpers. requesty + abort-signal specs 80 passed. --- src/api/providers/utils/abort-signal.ts | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/src/api/providers/utils/abort-signal.ts b/src/api/providers/utils/abort-signal.ts index 09863f263c..c26bce4f6f 100644 --- a/src/api/providers/utils/abort-signal.ts +++ b/src/api/providers/utils/abort-signal.ts @@ -31,19 +31,6 @@ export function toSdkTimeoutMs(timeoutMs: number): number { * native AbortSignal.timeout() API, which self-manages its timer — callers do * not need to (and cannot) clear it manually. */ -/** A timeout is usable when it is finite and positive (SDKs treat 0 as immediate abort). */ -export function hasUsableTimeout(timeoutMs?: number): timeoutMs is number { - return typeof timeoutMs === "number" && Number.isFinite(timeoutMs) && timeoutMs > 0 -} - -/** Maximum delay a Node timer (and therefore an SDK timeout option) accepts. */ -export const MAX_TIMEOUT_MS = 2_147_483_647 - -/** Convert a validated timeout to the value forwarded to an SDK: round up fractions, cap at the max delay. */ -export function toSdkTimeoutMs(timeoutMs: number): number { - return Math.min(Math.max(1, Math.ceil(timeoutMs)), MAX_TIMEOUT_MS) -} - export function mergeAbortSignalAndTimeout(externalSignal?: AbortSignal, timeoutMs?: number): AbortSignal | undefined { // Node's AbortSignal.timeout() throws ERR_OUT_OF_RANGE for non-integer // delays (Infinity) and clamps delays above 2^31-1 ms to 1 ms, which would From 444c26ce910c5bed35479e970476560f1b01682f Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sun, 1 Nov 2026 22:03:51 +0800 Subject: [PATCH 15/17] fix(merge): restore the upstream hunks dropped by the main merge restore the upstream requesty.spec imports (providerIdentifiers, requestyDefaultModelInfo, getModels) alongside the branch abort tests --- src/api/providers/__tests__/requesty.spec.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/api/providers/__tests__/requesty.spec.ts b/src/api/providers/__tests__/requesty.spec.ts index 6022503ed4..3d5bd65d78 100644 --- a/src/api/providers/__tests__/requesty.spec.ts +++ b/src/api/providers/__tests__/requesty.spec.ts @@ -9,8 +9,10 @@ const MOCK_TIMEOUT_MS = 300_000 import { Anthropic } from "@anthropic-ai/sdk" import OpenAI from "openai" +import { providerIdentifiers, requestyDefaultModelInfo } from "@roo-code/types" import type { ModelRecord } from "@roo-code/types" +import { getModels } from "../fetchers/modelCache" import { RequestyHandler } from "../requesty" import { Package } from "../../../shared/package" import { ApiHandlerCreateMessageMetadata } from "../../index" From 311f6b7b071aa68a7850dc3653a9b6e2c37cb464 Mon Sep 17 00:00:00 2001 From: eason liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 5 Nov 2026 02:43:46 +0800 Subject: [PATCH 16/17] fix(requesty): detach the SDK abort signal and race the completion Two cancellation gaps in completePrompt: 1. The SDK signal was not detachable. mergeAbortSignalAndTimeout() returns the caller's signal unchanged when timeoutMs is not usable, and openai@5.23.2 attaches its abort listener to whichever signal it receives and leaves it attached after the request completes, so reusing one caller signal retained one listener and one per-request controller per completed request. The caller signal is now bridged with bridgeExternalAbortSignal, the SDK receives the per-request signal, and dispose() runs in a finally covering model lookup and completion alike - completion, failure, abort, and late result. 2. The completion was not raced against the cancellation scope. openai@5.23.2 does not observe the signal inside retryRequest: it sleeps for the backoff (or a server retry-after-ms, up to about a minute) before issuing the next attempt, so awaiting the SDK call alone could hold completePrompt open long after the caller cancelled. The create() call now goes through rejectOnAbort, the same shape the model lookup above already uses. Tests (requesty.spec.ts, completePrompt): - settles on abort while the SDK is sleeping between retries instead of waiting for the completion - detaches the abort listener from the caller's signal when the completion finishes - detaches the abort listener from the caller's signal when the completion fails - does not leave the SDK's abort listener on the caller's signal after a completed request (the mock emulates the SDK leaving its listener attached) "should pass abort signal through to client" now asserts the contract the bridge provides - the SDK receives a per-request signal that tracks the caller's abort - instead of identity with the caller's own signal. Negative controls (each restored byte-identically afterwards): - dropping the race fails exactly the retry-sleep test: "expected Error: settle guard timed out after 500ms to match object { name: 'AbortError', ... (1) }". - dropping the bridge fails the pass-through test ("expected AbortSignal { aborted: false } not to be AbortSignal { aborted: false }") and the leak test ("expected true to be false"). - keeping the bridge but emptying the finally fails the two detach tests ("expected \"removeEventListener\" to be called with arguments: [ 'abort', [Function onExternalAbort] ]") plus the leak test. - requesty.spec.ts 43/43 green; src tsc --noEmit, eslint --max-warnings=0 and prettier --check clean; no new suppression, no new "as any", no Stryker directive. --- src/api/providers/__tests__/requesty.spec.ts | 157 ++++++++++++++++++- src/api/providers/requesty.ts | 116 ++++++++------ 2 files changed, 224 insertions(+), 49 deletions(-) diff --git a/src/api/providers/__tests__/requesty.spec.ts b/src/api/providers/__tests__/requesty.spec.ts index 3d5bd65d78..6c1d401b6a 100644 --- a/src/api/providers/__tests__/requesty.spec.ts +++ b/src/api/providers/__tests__/requesty.spec.ts @@ -748,12 +748,47 @@ describe("RequestyHandler", () => { it("should pass abort signal through to client", async () => { const handler = new RequestyHandler(mockOptions) const controller = new AbortController() - mockCreate.mockResolvedValueOnce({ choices: [{ message: { content: "response" } }] }) - await handler.completePrompt("test prompt", { abortSignal: controller.signal }) - expect(mockCreate).toHaveBeenCalledWith(expect.objectContaining({ model: expect.any(String) }), { - signal: controller.signal, + let notifyCreateStarted!: () => void + const createStarted = new Promise((resolve) => { + notifyCreateStarted = resolve + }) + let requestSignal: AbortSignal | undefined + mockCreate.mockImplementationOnce(async (_params: unknown, options?: { signal?: AbortSignal }) => { + notifyCreateStarted() + requestSignal = options?.signal + // Emulate the OpenAI SDK: the in-flight request settles through its signal. + await new Promise((resolve) => { + if (options?.signal?.aborted) { + resolve() + } else { + options?.signal?.addEventListener("abort", () => resolve(), { once: true }) + } + }) + return { choices: [{ message: { content: "response" } }] } }) + + const promise = handler.completePrompt("test prompt", { abortSignal: controller.signal }) + void promise.catch(() => {}) + await withSettleGuard(createStarted) + + expect(requestSignal).toBeDefined() + // The SDK receives the bridged per-request signal rather than the caller's own: the SDK + // leaves its abort listener attached after a completed request, so that listener has to + // land on a signal that dies with the request. What the contract requires is that the + // signal tracks the caller, which the mock above proves: it settles only through the + // signal it was handed. + expect(requestSignal).not.toBe(controller.signal) + expect(requestSignal?.aborted).toBe(false) + + controller.abort() + // The late response must be discarded, and the bridged signal must have aborted with the + // caller's. + await expect(withSettleGuard(promise)).rejects.toMatchObject({ + name: "AbortError", + message: "The Requesty request was aborted", + }) + expect(requestSignal?.aborted).toBe(true) }) it("should pass timeout through to client", async () => { @@ -1026,6 +1061,120 @@ describe("RequestyHandler", () => { }), ) }) + + it("settles on abort while the SDK is sleeping between retries instead of waiting for the completion", async () => { + // openai@5.23.2 runs "await sleep(timeoutMillis)" inside retryRequest before issuing the + // next attempt, and that sleep does not observe the request signal: awaiting the SDK call + // alone keeps completePrompt pending for the rest of the backoff (a server-provided + // retry-after-ms may stretch it to about a minute). The race against the cancellation + // scope must settle the caller at the abort instead, exactly as the model lookup above + // already does. + const handler = new RequestyHandler(mockOptions) + const controller = new AbortController() + + let notifyCreateStarted!: () => void + const createStarted = new Promise((resolve) => { + notifyCreateStarted = resolve + }) + let releaseBackoff!: () => void + const backoffSleep = new Promise((resolve) => { + releaseBackoff = resolve + }) + let completionSettled = false + mockCreate.mockImplementationOnce(async () => { + notifyCreateStarted() + await backoffSleep + completionSettled = true + return { choices: [{ message: { content: "late" } }] } + }) + + const promise = handler.completePrompt("test prompt", { abortSignal: controller.signal }) + // Defensive: the race may reject before the barrier below settles, which would otherwise + // surface as an unhandled rejection. + void promise.catch(() => {}) + await withSettleGuard(createStarted) + controller.abort() + + await expect(withSettleGuard(promise)).rejects.toMatchObject({ + name: "AbortError", + message: "The Requesty request was aborted", + }) + // The caller settled while the SDK was still inside its retry sleep. + expect(completionSettled).toBe(false) + releaseBackoff() + }) + + it("detaches the abort listener from the caller's signal when the completion finishes", async () => { + // The bridge registers its listener on the caller's long-lived signal, so completion must + // detach it: otherwise one listener plus one per-request controller is retained for every + // completed request on a reused signal. + const handler = new RequestyHandler(mockOptions) + const controller = new AbortController() + const addSpy = vitest.spyOn(controller.signal, "addEventListener") + const removeSpy = vitest.spyOn(controller.signal, "removeEventListener") + mockCreate.mockResolvedValueOnce({ choices: [{ message: { content: "response" } }] }) + + try { + await handler.completePrompt("test prompt", { abortSignal: controller.signal }) + + const registered = addSpy.mock.calls.find(([type]) => type === "abort")?.[1] + expect(registered).toBeDefined() + expect(removeSpy).toHaveBeenCalledWith("abort", registered) + } finally { + addSpy.mockRestore() + removeSpy.mockRestore() + } + }) + + it("detaches the abort listener from the caller's signal when the completion fails", async () => { + // Failure is an exit path too: dispose() runs in finally, so a provider error must not + // leave the listener behind either. + const handler = new RequestyHandler(mockOptions) + const controller = new AbortController() + const addSpy = vitest.spyOn(controller.signal, "addEventListener") + const removeSpy = vitest.spyOn(controller.signal, "removeEventListener") + mockCreate.mockRejectedValueOnce(new Error("API Error")) + + try { + await expect(handler.completePrompt("test prompt", { abortSignal: controller.signal })).rejects.toThrow( + "API Error", + ) + + const registered = addSpy.mock.calls.find(([type]) => type === "abort")?.[1] + expect(registered).toBeDefined() + expect(removeSpy).toHaveBeenCalledWith("abort", registered) + } finally { + addSpy.mockRestore() + removeSpy.mockRestore() + } + }) + + it("does not leave the SDK's abort listener on the caller's signal after a completed request", async () => { + // Emulate openai@5.23.2: its request path attaches an abort listener to the signal it is + // handed and leaves it attached once the request completes. Reusing one caller signal + // therefore retains that listener - and the per-request state it closes over - for every + // completed request, unless the provider hands the SDK a signal that dies with the request. + const handler = new RequestyHandler(mockOptions) + const controller = new AbortController() + let leakedListenerFired = false + mockCreate.mockImplementationOnce(async (_params: unknown, options?: { signal?: AbortSignal }) => { + options?.signal?.addEventListener( + "abort", + () => { + leakedListenerFired = true + }, + { once: true }, + ) + return { choices: [{ message: { content: "response" } }] } + }) + + await handler.completePrompt("test prompt", { abortSignal: controller.signal }) + + // Nothing the request created may still be listening on the caller's signal: aborting it + // now must not reach the listener the SDK installed during the request. + controller.abort() + expect(leakedListenerFired).toBe(false) + }) }) describe("model catalog", () => { diff --git a/src/api/providers/requesty.ts b/src/api/providers/requesty.ts index c3de4609fc..36ecf8ba74 100644 --- a/src/api/providers/requesty.ts +++ b/src/api/providers/requesty.ts @@ -32,6 +32,7 @@ import { handleOpenAIError } from "./utils/error-handler" import { hasUsableTimeout, toSdkTimeoutMs, + bridgeExternalAbortSignal, createAbortError, isRequestAborted, mergeAbortSignalAndTimeout, @@ -259,60 +260,85 @@ export class RequestyHandler extends BaseProvider implements SingleCompletionHan } async completePrompt(prompt: string, options?: CompletePromptOptions): Promise { - // Establish the cancellation scope before model lookup: a pre-aborted call, or - // one aborted while model metadata is loading, must reject promptly instead of - // waiting for the lookup to settle. The configured timeoutMs covers the lookup - // as well. - const requestAbortSignal = mergeAbortSignalAndTimeout(options?.abortSignal, options?.timeoutMs) - throwIfAborted(requestAbortSignal) - - let modelData: Awaited> + // Bridge the caller's signal before anything else: the OpenAI SDK attaches its abort + // listener to whichever signal it receives and leaves it attached once the request + // completes, and with no usable timeout `mergeAbortSignalAndTimeout` would hand it the + // caller's own long-lived signal - one retained listener and controller per completed + // request. The bridge gives the SDK a per-request signal and returns the handle that + // detaches it, which the `finally` below must run on every exit path. + // + // Establishing the cancellation scope here also covers model lookup: a pre-aborted call, + // or one aborted while model metadata is loading, must reject promptly instead of waiting + // for the lookup to settle. The configured timeoutMs covers the lookup as well. + const { signal: bridgedCallerSignal, dispose: removeExternalAbortListener } = bridgeExternalAbortSignal( + options?.abortSignal, + ) + const requestAbortSignal = mergeAbortSignalAndTimeout(bridgedCallerSignal, options?.timeoutMs) + try { - modelData = requestAbortSignal - ? await rejectOnAbort(this.fetchModel(), requestAbortSignal, this.providerName) - : await this.fetchModel() - } catch (error) { - if (isRequestAborted(error, requestAbortSignal)) { - throw createAbortError(this.providerName) + throwIfAborted(requestAbortSignal) + + let modelData: Awaited> + try { + modelData = requestAbortSignal + ? await rejectOnAbort(this.fetchModel(), requestAbortSignal, this.providerName) + : await this.fetchModel() + } catch (error) { + if (isRequestAborted(error, requestAbortSignal)) { + throw createAbortError(this.providerName) + } + throw error } - throw error - } - const { id: model, maxTokens: max_tokens, temperature } = modelData + const { id: model, maxTokens: max_tokens, temperature } = modelData - const openAiMessages: OpenAI.Chat.ChatCompletionMessageParam[] = [{ role: "system", content: prompt }] + const openAiMessages: OpenAI.Chat.ChatCompletionMessageParam[] = [{ role: "system", content: prompt }] - const completionParams: RequestyChatCompletionParams = { - model, - max_tokens, - messages: openAiMessages, - temperature: temperature, - } + const completionParams: RequestyChatCompletionParams = { + model, + max_tokens, + messages: openAiMessages, + temperature: temperature, + } - // The merged abort signal (established before model lookup, above) is forwarded to the - // SDK so both abort and timeout reject with a DOM-standard AbortError in the catch - // below. The client-level timeout remains the default safety net; 0 is never passed - // to the SDK timeout. - const createOptions: OpenAI.RequestOptions = { - ...(requestAbortSignal && { signal: requestAbortSignal }), - ...(hasUsableTimeout(options?.timeoutMs) && { timeout: toSdkTimeoutMs(options.timeoutMs) }), - } + // The merged abort signal (established before model lookup, above) is forwarded to the + // SDK so both abort and timeout reject with a DOM-standard AbortError in the catch + // below. The client-level timeout remains the default safety net; 0 is never passed + // to the SDK timeout. + const createOptions: OpenAI.RequestOptions = { + ...(requestAbortSignal && { signal: requestAbortSignal }), + ...(hasUsableTimeout(options?.timeoutMs) && { timeout: toSdkTimeoutMs(options.timeoutMs) }), + } + + let response: OpenAI.Chat.ChatCompletion + try { + const pendingCompletion = this.client.chat.completions.create(completionParams, createOptions) + // Race the completion against the cancellation scope as the model lookup above does: + // openai@5.23.2 does not observe the signal inside `retryRequest`, which sleeps for + // the backoff (or a server-provided `retry-after-ms`, up to about a minute) before + // issuing the next attempt. Awaiting the SDK call alone can therefore hold + // completePrompt open long after the caller cancelled; the race settles at once and a + // rejection the SDK wins first is still normalized by the catch below. + response = requestAbortSignal + ? await rejectOnAbort(pendingCompletion, requestAbortSignal, this.providerName) + : await pendingCompletion + } catch (error) { + // Aborted requests are user-initiated: surface them as AbortError (this also covers + // timeouts, which abort the same signal) instead of a completion error. + if (requestAbortSignal?.aborted) { + throw createAbortError(this.providerName) + } + throw handleOpenAIError(error, this.providerName) + } - let response: OpenAI.Chat.ChatCompletion - try { - response = await this.client.chat.completions.create(completionParams, createOptions) - } catch (error) { - // Aborted requests are user-initiated: surface them as AbortError (this also covers - // timeouts, which abort the same signal) instead of a completion error. if (requestAbortSignal?.aborted) { + // The response resolved after the request was aborted: do not return the late result. throw createAbortError(this.providerName) } - throw handleOpenAIError(error, this.providerName) - } - - if (requestAbortSignal?.aborted) { - // The response resolved after the request was aborted: do not return the late result. - throw createAbortError(this.providerName) + return response.choices[0]?.message.content || "" + } finally { + // Detach on every exit path - completion, failure, abort, or a late result. The listener + // sits on the caller's long-lived signal and would outlive this request otherwise. + removeExternalAbortListener() } - return response.choices[0]?.message.content || "" } } From 530033ca45fcf78ac77c9ec95b3fa28527be62b2 Mon Sep 17 00:00:00 2001 From: eason liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 5 Nov 2026 02:52:35 +0800 Subject: [PATCH 17/17] test(requesty): pin the late-completion discard and the empty-content fallback Rerunning the unit mutation preflight after the completePrompt fix left three mutants on lines the rewrite touches, none of them reachable by the previous tests: - ConditionalExpression at requesty.ts:333 (the post-completion abort check, replaced by false) survived because the new race settles at the abort, so no test still reached that check. "discards a completion that resolves in the same turn the caller aborts" makes the SDK win the race outright - it returns an already-resolved response and aborts from a handler queued one step ahead of the provider's continuation - and asserts the value is still discarded. Control: deleting the check fails exactly that test with "promise resolved \"'late'\" instead of rejecting". - StringLiteral at requesty.ts:337 (the || "" fallback) was NoCoverage. - OptionalChaining at requesty.ts:337 (choices[0]?.message) survived. "returns an empty string when the completion carries an empty message content / no choices at all" covers both: the string mutant fails both cases with "expected 'Stryker was here!' to be ''", the optional-chain mutant fails the no-choices case with "Cannot read properties of undefined (reading 'message')". requesty.spec.ts 46/46 green; eslint --max-warnings=0, prettier --check and src tsc --noEmit clean; no suppression and no Stryker directive added. --- src/api/providers/__tests__/requesty.spec.ts | 36 ++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/src/api/providers/__tests__/requesty.spec.ts b/src/api/providers/__tests__/requesty.spec.ts index 6c1d401b6a..6ec0e28757 100644 --- a/src/api/providers/__tests__/requesty.spec.ts +++ b/src/api/providers/__tests__/requesty.spec.ts @@ -1007,6 +1007,42 @@ describe("RequestyHandler", () => { }) }) + it("discards a completion that resolves in the same turn the caller aborts", async () => { + // The race above settles at the abort, so the post-completion check is reached only when the + // SDK wins the race outright: the response promise resolves, the abort lands before the + // provider continues with the value, and the value must still be discarded. Aborting from a + // handler on the response promise - queued one step ahead of the provider's race handler - + // pins that interleaving without timing it. + const handler = new RequestyHandler(mockOptions) + const controller = new AbortController() + mockCreate.mockImplementationOnce(() => { + const response = Promise.resolve({ choices: [{ message: { content: "late" } }] }) + void response.then(() => { + queueMicrotask(() => controller.abort()) + }) + return response + }) + + await expect( + handler.completePrompt("test prompt", { abortSignal: controller.signal }), + ).rejects.toMatchObject({ + name: "AbortError", + message: "The Requesty request was aborted", + }) + }) + + it.each([ + ["an empty message content", { choices: [{ message: { content: "" } }] }], + ["no choices at all", { choices: [] }], + ])("returns an empty string when the completion carries %s", async (_label: string, response: unknown) => { + // The fallback is what makes completePrompt return "" rather than an empty value or a + // TypeError, and callers render the result directly. + const handler = new RequestyHandler(mockOptions) + mockCreate.mockResolvedValueOnce(response) + + await expect(handler.completePrompt("test prompt")).resolves.toBe("") + }) + it("does not forward a non-positive timeout to the client", async () => { const handler = new RequestyHandler(mockOptions) mockCreate.mockResolvedValueOnce({ choices: [{ message: { content: "response" } }] })