diff --git a/src/api/providers/__tests__/requesty.spec.ts b/src/api/providers/__tests__/requesty.spec.ts index f256e5b1c1..4a3faeb34a 100644 --- a/src/api/providers/__tests__/requesty.spec.ts +++ b/src/api/providers/__tests__/requesty.spec.ts @@ -9,12 +9,15 @@ 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" import { makeApiHandlerOptions } from "../../../test-utils/api" import { asyncStreamFrom, collectStream } from "../../../test-utils/stream" import { clearAllMocks } from "../../../test-utils/reset" +import { withSettleGuard } from "../../../test-utils/settle-guard" const mockCreate = vitest.fn() @@ -613,6 +616,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 +633,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 +655,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 +677,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 +699,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 +724,279 @@ 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" } }] }) + + // 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 () => { + 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", + }) + expect(mockCreate).not.toHaveBeenCalled() + }) + + 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..2249ca21b2 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(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 || "" } } diff --git a/src/api/providers/utils/__tests__/abort-signal.spec.ts b/src/api/providers/utils/__tests__/abort-signal.spec.ts index 1692f71e63..04d657b564 100644 --- a/src/api/providers/utils/__tests__/abort-signal.spec.ts +++ b/src/api/providers/utils/__tests__/abort-signal.spec.ts @@ -3,8 +3,97 @@ import { isRequestAborted, mergeAbortSignalAndTimeout, mergeAbortSignals, + rejectOnAbort, throwIfAborted, } from "../abort-signal" +import { withSettleGuard } from "../../../../test-utils/settle-guard" + +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", + message: "The TestProvider request was aborted", + }) + }) + + 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 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") + + // 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") + + await expect( + withSettleGuard(rejectOnAbort(Promise.reject(lookupError), controller.signal, "TestProvider")), + ).rejects.toBe(lookupError) + + // 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() + }) +}) describe("abort-signal utilities", () => { describe("mergeAbortSignalAndTimeout", () => { 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) + }, + ) + }) +} 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) + }, + ) + }) +}