From d298d4a6f6b835eac62471c010ddfa98bbd1c881 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Sat, 5 Sep 2026 22:44:24 +0800 Subject: [PATCH 01/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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" } }] }) From 1975c270789d2e91dc44e5604942db8148bbcd16 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sun, 8 Nov 2026 06:33:21 +0800 Subject: [PATCH 18/26] test(requesty): pin timeout normalisation, timeout-covered lookup and the three cancel paths Three review asks on this head, all in the spec so no production behaviour moved. createOptions timeout: an it.each now asserts what the SDK is actually handed for two values that whole numbers cannot distinguish - 1500.5 must be ceiled to 1501 and 3e9 must be clamped to 2147483647. Measured: with toSdkTimeoutMs replaced by a pass-through of options.timeoutMs both cases fail (2 failed) and the received value is 1500.5, which is the sentence that turns the coverage ask into a proof: dropping the conversion is observable. Model lookup under timeoutMs: a new case runs completePrompt with { timeoutMs: 20 } and no caller signal at all against a getModels that never settles, so the only cancellation source is the timeout this method configures; it asserts the provider AbortError and that create was never reached. The control that drives the lookup race from bridgedCallerSignal instead was attempted twice and the vitest process was killed both times, so that half is reported as unmeasured rather than claimed. Detach lifecycle: the resolve test and the failure test are folded into one it.each with a third case that aborts mid-flight. The fold asserts nothing new about detaching for the two original outcomes; the new ground is that dispose also runs on the cancellation exit, which neither original pinned. The comment that said it mirrored the Requesty test inside the Requesty spec no longer claims a sibling: git grep for withSettleGuard/notifyCreateStarted across src/api/providers/__tests__ returns matches only in this file (28), so there was no other provider test to name and the comparison was dropped rather than re-pointed. Verified: tsc -p tsconfig.json --noEmit clean; spec 50 passed (was 45 - the fold nets one extra case and the timeout it.each adds two, the lookup case one); prettier --check clean; eslint over the touched file clean after formatting; pre-commit hook run by hand before the commit. src/eslint-suppressions.json untouched, no changeset, no CHANGELOG. --- src/api/providers/__tests__/requesty.spec.ts | 171 ++++++++++++++----- 1 file changed, 126 insertions(+), 45 deletions(-) diff --git a/src/api/providers/__tests__/requesty.spec.ts b/src/api/providers/__tests__/requesty.spec.ts index 6ec0e28757..80ef6ff6cb 100644 --- a/src/api/providers/__tests__/requesty.spec.ts +++ b/src/api/providers/__tests__/requesty.spec.ts @@ -726,6 +726,28 @@ describe("RequestyHandler", () => { await expect(handler.completePrompt("test prompt")).rejects.toThrow("Unexpected error") }) + it.each([ + // Ceiled up: 1500.5 ms is a usable timeout and toSdkTimeoutMs rounds it to 1501. + // Without the conversion the SDK would receive 1500.5 and the assertion below fails. + { input: 1500.5, expected: 1501 }, + // Clamped down: 3e9 exceeds the platform timer ceiling, so it is capped at MAX. + // Without the conversion the SDK would receive 3e9 and the assertion below fails. + { input: 3e9, expected: 2147483647 }, + ] as const)( + "normalises a fractional or oversized timeoutMs to the SDK timeout value $input -> $expected", + async ({ input, expected }) => { + // The wiring under test is toSdkTimeoutMs at the createOptions construction: whole + // numbers alone cannot tell the conversion from a pass-through, so both a value that + // must be ceiled and a value that must be clamped are asserted on what the SDK is + // actually handed. + const handler = new RequestyHandler(mockOptions) + mockCreate.mockResolvedValueOnce({ choices: [{ message: { content: "response" } }] }) + await handler.completePrompt("test prompt", { timeoutMs: input }) + const options = mockCreate.mock.calls.at(-1)?.[1] as { timeout?: number } | undefined + expect(options?.timeout).toBe(expected) + }, + ) + it.each([ ["an external signal", { abortSignal: new AbortController().signal }], ["a per-request timeout", { timeoutMs: 5_000 }], @@ -881,6 +903,25 @@ describe("RequestyHandler", () => { expect(mockCreate).not.toHaveBeenCalled() }) + it("applies timeoutMs to model lookup with no caller signal at all", async () => { + // The comment above completePrompt claims timeoutMs covers model lookup too. With no + // caller signal the only cancellation source is the timeout this method configures, so + // the lookup race must be driven by that merged signal: a never-settling getModels has + // to be ended by the 20ms deadline rather than hanging. Red if the lookup race is + // driven by the bridged caller signal instead (undefined here), which leaves the + // lookup awaited forever and the settle guard fires first. + const handler = new RequestyHandler(mockOptions) + const { getModels } = await import("../fetchers/modelCache") + vitest.mocked(getModels).mockImplementationOnce(() => new Promise(() => {})) + const promise = handler.completePrompt("test prompt", { timeoutMs: 20 }) + void promise.catch(() => {}) + 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") @@ -911,7 +952,7 @@ describe("RequestyHandler", () => { const handler = new RequestyHandler(mockOptions) const controller = new AbortController() - // Deterministic synchronization (mirrors the Requesty test): the mock notifies the + // Deterministic synchronization: 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 @@ -1140,50 +1181,90 @@ describe("RequestyHandler", () => { 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.each([ + { + outcome: "completes", + expectRejection: undefined as string | undefined, + barrier: undefined as Promise | undefined, + start: (controller: AbortController) => { + mockCreate.mockResolvedValueOnce({ choices: [{ message: { content: "response" } }] }) + return new RequestyHandler(mockOptions).completePrompt("test prompt", { + abortSignal: controller.signal, + }) + }, + }, + { + outcome: "fails", + expectRejection: "API Error", + barrier: undefined as Promise | undefined, + start: (controller: AbortController) => { + mockCreate.mockRejectedValueOnce(new Error("API Error")) + return new RequestyHandler(mockOptions).completePrompt("test prompt", { + abortSignal: controller.signal, + }) + }, + }, + { + // Aborting mid-flight is the third exit path through finally: the SDK-shaped mock + // rejects once the signal aborts, so dispose() must detach on that path too. + outcome: "aborts mid-flight", + expectRejection: "aborted", + start: (controller: AbortController) => { + let notifyStarted!: () => void + const barrier = new Promise((resolve) => { + notifyStarted = resolve + }) + mockCreate.mockImplementationOnce(async (_params: unknown, opts?: { signal?: AbortSignal }) => { + notifyStarted() + await new Promise((resolve) => + opts?.signal?.addEventListener("abort", () => resolve(), { once: true }), + ) + const abortError = new Error("The user aborted a request") + abortError.name = "AbortError" + throw abortError + }) + return { + pending: new RequestyHandler(mockOptions).completePrompt("test prompt", { + abortSignal: controller.signal, + }), + barrier, + } + }, + }, + ] as const)( + "detaches the abort listener from the caller's signal when the completion $outcome", + async (item) => { + // The bridge registers its listener on the caller's long-lived signal, so every exit + // path - resolve, provider error and abort - must detach it: otherwise one listener plus + // one per-request controller is retained for every request on a reused signal. The + // abort case adds nothing the two originals asserted about detaching; it pins that the + // finally dispose also runs when the request ends by cancellation rather than by settling. + const controller = new AbortController() + const addSpy = vitest.spyOn(controller.signal, "addEventListener") + const removeSpy = vitest.spyOn(controller.signal, "removeEventListener") + const started = item.start(controller) + const pending = "pending" in started ? started.pending : started + const barrier = "pending" in started ? started.barrier : item.barrier + void pending.catch(() => {}) + try { + if (barrier) { + await withSettleGuard(barrier) + controller.abort() + } + if (item.expectRejection) { + await expect(pending).rejects.toThrow(item.expectRejection) + } else { + await pending + } + 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 From bd60384a27dce94180ef59defd2ff2db04b97860 Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Mon, 9 Nov 2026 00:31:20 +0000 Subject: [PATCH 19/26] test(lint): flag bare abort substring in toThrow assertions --- packages/config-eslint/api-provider-hygiene.js | 8 ++++++++ .../config-eslint/api-provider-hygiene.test.js | 16 ++++++++++++++++ src/api/providers/__tests__/requesty.spec.ts | 2 +- .../__tests__/ask-queued-message-drain.spec.ts | 2 +- src/eslint.config.mjs | 10 ++++++++-- 5 files changed, 34 insertions(+), 4 deletions(-) diff --git a/packages/config-eslint/api-provider-hygiene.js b/packages/config-eslint/api-provider-hygiene.js index a021c298c6..de21c35e72 100644 --- a/packages/config-eslint/api-provider-hygiene.js +++ b/packages/config-eslint/api-provider-hygiene.js @@ -63,6 +63,13 @@ export const apiProviderHygieneConfig = { }, } +// Shared by every spec lane, not only src/api: an abort assertion must not pass on a raw SDK error. +export const abortSubstringSelector = { + selector: "CallExpression[callee.property.name='toThrow'][arguments.0.value=/^[Aa]bort(ed)?$/]", + message: + "A bare 'abort' substring also matches a raw SDK error. Assert the AbortError name and the full provider message.", +} + // Spec files under src/api. A loop of expects in one test stops at the first failing case and hides the rest. export const apiProviderTestSelectors = [ { @@ -70,6 +77,7 @@ export const apiProviderTestSelectors = [ message: "Use it.each for repeated cases. A loop of expects in one test stops at the first failing case and does not name it.", }, + abortSubstringSelector, ] // A new @ts-ignore in a provider spec hides a private-member access. Use bracket notation or a typed double. diff --git a/packages/config-eslint/api-provider-hygiene.test.js b/packages/config-eslint/api-provider-hygiene.test.js index 62f197fa6b..735d045e03 100644 --- a/packages/config-eslint/api-provider-hygiene.test.js +++ b/packages/config-eslint/api-provider-hygiene.test.js @@ -173,6 +173,22 @@ describe("api provider hygiene: spec loops", () => { assert.deepEqual(lintSpec(code), []) }) } + + for (const code of ['await expect(p).rejects.toThrow("aborted")', "expect(() => f()).toThrow('abort')"]) { + it(`flags a bare abort substring: ${code}`, () => { + assert.equal(lintSpec(code).length, 1) + }) + } + + for (const code of [ + 'await expect(p).rejects.toThrow("The Requesty request was aborted")', + 'await expect(p).rejects.toMatchObject({ name: "AbortError" })', + 'await expect(p).rejects.toThrow("API Error")', + ]) { + it(`allows: ${code}`, () => { + assert.deepEqual(lintSpec(code), []) + }) + } }) describe("api provider hygiene: hand-built AbortError", () => { diff --git a/src/api/providers/__tests__/requesty.spec.ts b/src/api/providers/__tests__/requesty.spec.ts index 80ef6ff6cb..a94e80a4e0 100644 --- a/src/api/providers/__tests__/requesty.spec.ts +++ b/src/api/providers/__tests__/requesty.spec.ts @@ -1208,7 +1208,7 @@ describe("RequestyHandler", () => { // Aborting mid-flight is the third exit path through finally: the SDK-shaped mock // rejects once the signal aborts, so dispose() must detach on that path too. outcome: "aborts mid-flight", - expectRejection: "aborted", + expectRejection: "The Requesty request was aborted", start: (controller: AbortController) => { let notifyStarted!: () => void const barrier = new Promise((resolve) => { diff --git a/src/core/task/__tests__/ask-queued-message-drain.spec.ts b/src/core/task/__tests__/ask-queued-message-drain.spec.ts index bb74531d45..e9fc356a81 100644 --- a/src/core/task/__tests__/ask-queued-message-drain.spec.ts +++ b/src/core/task/__tests__/ask-queued-message-drain.spec.ts @@ -349,7 +349,7 @@ describe("Task.ask queued message drain", () => { access.abort = true finishAddingAsk() - await expect(ask).rejects.toThrow("aborted") + await expect(ask).rejects.toThrow(/#ask\] task \S+ aborted$/) expect(task.messageQueueService.messages).toHaveLength(1) expect(task.messageQueueService.claimNextMessage()?.text).toBe("Persist me later") }) diff --git a/src/eslint.config.mjs b/src/eslint.config.mjs index f82e677029..36cb3e8ec7 100644 --- a/src/eslint.config.mjs +++ b/src/eslint.config.mjs @@ -1,5 +1,6 @@ import { config } from "@roo-code/config-eslint/base" import { + abortSubstringSelector, apiProviderHygieneConfig, apiProviderTestSelectors, apiSpecTsIgnoreConfig, @@ -123,7 +124,7 @@ export default [ "**/services/__tests__/workflow-self-listen.test.ts", ], rules: { - "no-restricted-syntax": ["error", ...testPathTraversalSelectors], + "no-restricted-syntax": ["error", ...testPathTraversalSelectors, abortSubstringSelector], }, }, { @@ -144,7 +145,12 @@ export default [ "integrations/terminal/**/__tests__/**/*.ts", ], rules: { - "no-restricted-syntax": ["error", ...testPathTraversalSelectors, testResetHelperSelector], + "no-restricted-syntax": [ + "error", + ...testPathTraversalSelectors, + abortSubstringSelector, + testResetHelperSelector, + ], }, }, { From deee63a3a0031bcbea26f0b9a27afca0aa20d774 Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Mon, 9 Nov 2026 04:11:44 +0000 Subject: [PATCH 20/26] fix(eslint): tighten abort assertion guards and test lane coverage --- .../config-eslint/api-provider-hygiene.js | 3 +- .../api-provider-hygiene.test.js | 52 ++++++++++++++++++- .../task/__tests__/ask-abort-race.spec.ts | 20 +++++-- 3 files changed, 68 insertions(+), 7 deletions(-) diff --git a/packages/config-eslint/api-provider-hygiene.js b/packages/config-eslint/api-provider-hygiene.js index de21c35e72..0fac2440a9 100644 --- a/packages/config-eslint/api-provider-hygiene.js +++ b/packages/config-eslint/api-provider-hygiene.js @@ -65,7 +65,8 @@ export const apiProviderHygieneConfig = { // Shared by every spec lane, not only src/api: an abort assertion must not pass on a raw SDK error. export const abortSubstringSelector = { - selector: "CallExpression[callee.property.name='toThrow'][arguments.0.value=/^[Aa]bort(ed)?$/]", + selector: + "CallExpression[callee.property.name=/^toThrow(Error)?$/]:not([callee.object.property.name='not']):matches([arguments.0.value=/^[Aa]bort(ed)?$/], [arguments.0.regex.pattern=/^[Aa]bort(ed)?$/])", message: "A bare 'abort' substring also matches a raw SDK error. Assert the AbortError name and the full provider message.", } diff --git a/packages/config-eslint/api-provider-hygiene.test.js b/packages/config-eslint/api-provider-hygiene.test.js index 735d045e03..dfc4630ed7 100644 --- a/packages/config-eslint/api-provider-hygiene.test.js +++ b/packages/config-eslint/api-provider-hygiene.test.js @@ -1,7 +1,8 @@ import assert from "node:assert/strict" import { describe, it } from "node:test" +import { fileURLToPath, URL } from "node:url" -import { Linter } from "eslint" +import { ESLint, Linter } from "eslint" import typescriptEslint from "typescript-eslint" import { apiProviderHygieneConfig, apiProviderTestSelectors, apiSpecTsIgnoreConfig } from "./api-provider-hygiene.js" @@ -174,7 +175,13 @@ describe("api provider hygiene: spec loops", () => { }) } - for (const code of ['await expect(p).rejects.toThrow("aborted")', "expect(() => f()).toThrow('abort')"]) { + for (const code of [ + 'await expect(p).rejects.toThrow("aborted")', + "expect(() => f()).toThrow('abort')", + 'await expect(p).rejects.toThrowError("aborted")', + "expect(() => f()).toThrow(/abort/i)", + "await expect(p).rejects.toThrowError(/aborted/i)", + ]) { it(`flags a bare abort substring: ${code}`, () => { assert.equal(lintSpec(code).length, 1) }) @@ -184,6 +191,11 @@ describe("api provider hygiene: spec loops", () => { 'await expect(p).rejects.toThrow("The Requesty request was aborted")', 'await expect(p).rejects.toMatchObject({ name: "AbortError" })', 'await expect(p).rejects.toThrow("API Error")', + 'expect(() => f()).not.toThrow("aborted")', + 'await expect(p).rejects.not.toThrow("aborted")', + "expect(() => f()).not.toThrowError(/abort/i)", + "await expect(p).rejects.not.toThrowError(/aborted/i)", + "await expect(p).rejects.toThrow(/^The Requesty request was aborted$/)", ]) { it(`allows: ${code}`, () => { assert.deepEqual(lintSpec(code), []) @@ -261,3 +273,39 @@ describe("api provider hygiene: @ts-ignore in specs", () => { assert.deepEqual(lintSpecFile("// @ts-ignore x\nhandler.client", "services/__tests__/example.spec.ts"), []) }) }) + +// Resolve each lane through the extension's real config before exercising its syntax rules. +// Isolating syntax rules avoids requiring fixture files in the TypeScript project. +describe("abort assertions in the extension ESLint config", () => { + const srcRoot = fileURLToPath(new URL("../../src/", import.meta.url)) + const eslint = new ESLint({ cwd: srcRoot }) + + for (const filename of ["core/task/__tests__/example.spec.ts", "core/config/__tests__/example.spec.ts"]) { + it(`checks rejected and accepted assertions in ${filename}`, async () => { + const resolved = await eslint.calculateConfigForFile(filename) + const syntaxConfig = [ + { + files: ["**/*.ts"], + languageOptions: { parser: typescriptEslint.parser }, + rules: { "no-restricted-syntax": resolved.rules["no-restricted-syntax"] }, + }, + ] + for (const [code, expectedCount] of [ + ['await expect(p).rejects.toThrow("aborted")', 1], + ["await expect(p).rejects.toThrowError(/abort/i)", 1], + ['await expect(p).rejects.not.toThrow("aborted")', 0], + [ + 'await expect(p).rejects.toMatchObject({ name: "AbortError", message: "The Requesty request was aborted" })', + 0, + ], + ]) { + const messages = linter.verify(code, syntaxConfig, { filename }) + assert.equal(messages.length, expectedCount, code) + if (expectedCount) { + assert.equal(messages[0].ruleId, "no-restricted-syntax") + assert.match(messages[0].message, /bare 'abort' substring/) + } + } + }) + } +}) diff --git a/src/core/task/__tests__/ask-abort-race.spec.ts b/src/core/task/__tests__/ask-abort-race.spec.ts index aa5dbe4a82..5796be79b5 100644 --- a/src/core/task/__tests__/ask-abort-race.spec.ts +++ b/src/core/task/__tests__/ask-abort-race.spec.ts @@ -47,7 +47,10 @@ describe("Task.ask abort during the auto-approval await", () => { task["abort"] = true gate.release() - await expect(askPromise).rejects.toThrow(/aborted/) + await expect(askPromise).rejects.toMatchObject({ + name: "Error", + message: `[RooCode#ask] task ${task.taskId}.${task.instanceId} aborted`, + }) expect(task["addToClineMessages"]).not.toHaveBeenCalled() expect(task["lastMessageTs"]).toBeUndefined() }) @@ -65,7 +68,10 @@ describe("Task.ask abort during the auto-approval await", () => { task["abort"] = true gate.release() - await expect(askPromise).rejects.toThrow(/aborted/) + await expect(askPromise).rejects.toMatchObject({ + name: "Error", + message: `[RooCode#ask] task ${task.taskId}.${task.instanceId} aborted`, + }) expect(task["addToClineMessages"]).not.toHaveBeenCalled() expect(task["lastMessageTs"]).toBeUndefined() }) @@ -86,7 +92,10 @@ describe("Task.ask abort during the auto-approval await", () => { task["abort"] = true gate.release() - await expect(askPromise).rejects.toThrow(/aborted/) + await expect(askPromise).rejects.toMatchObject({ + name: "Error", + message: `[RooCode#ask] task ${task.taskId}.${task.instanceId} aborted`, + }) expect(task["clineMessages"]).toEqual([original]) expect(task["saveClineMessages"]).not.toHaveBeenCalled() expect(task["updateClineMessage"]).not.toHaveBeenCalled() @@ -105,7 +114,10 @@ describe("Task.ask abort during the auto-approval await", () => { task["abort"] = true gate.release() - await expect(askPromise).rejects.toThrow(/aborted/) + await expect(askPromise).rejects.toMatchObject({ + name: "Error", + message: `[RooCode#ask] task ${task.taskId}.${task.instanceId} aborted`, + }) expect(task["addToClineMessages"]).not.toHaveBeenCalled() expect(task.messageQueueService.claimNextMessage()?.text).toBe("queued feedback") }) From d7800611e2811b833fe56f98818f92f8988cb4a7 Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Mon, 9 Nov 2026 05:17:37 +0000 Subject: [PATCH 21/26] fix(requesty): pass the request signal to model lookup, widen abort lint --- .../config-eslint/api-provider-hygiene.js | 2 +- .../api-provider-hygiene.test.js | 4 ++ src/api/providers/__tests__/requesty.spec.ts | 65 +++++++++++++++++++ src/api/providers/requesty.ts | 7 +- 4 files changed, 75 insertions(+), 3 deletions(-) diff --git a/packages/config-eslint/api-provider-hygiene.js b/packages/config-eslint/api-provider-hygiene.js index 0fac2440a9..9da19a49c1 100644 --- a/packages/config-eslint/api-provider-hygiene.js +++ b/packages/config-eslint/api-provider-hygiene.js @@ -66,7 +66,7 @@ export const apiProviderHygieneConfig = { // Shared by every spec lane, not only src/api: an abort assertion must not pass on a raw SDK error. export const abortSubstringSelector = { selector: - "CallExpression[callee.property.name=/^toThrow(Error)?$/]:not([callee.object.property.name='not']):matches([arguments.0.value=/^[Aa]bort(ed)?$/], [arguments.0.regex.pattern=/^[Aa]bort(ed)?$/])", + "CallExpression[callee.property.name=/^toThrow(Error)?$/]:not([callee.object.property.name='not']):matches([arguments.0.value=/^[$^]?(the )?(request )?(was )?abort(ed)?[.$]?$/i], [arguments.0.regex.pattern=/^[$^]?(the )?(request )?(was )?abort(ed)?[.$]?$/i])", message: "A bare 'abort' substring also matches a raw SDK error. Assert the AbortError name and the full provider message.", } diff --git a/packages/config-eslint/api-provider-hygiene.test.js b/packages/config-eslint/api-provider-hygiene.test.js index dfc4630ed7..a1ed8fac15 100644 --- a/packages/config-eslint/api-provider-hygiene.test.js +++ b/packages/config-eslint/api-provider-hygiene.test.js @@ -181,6 +181,10 @@ describe("api provider hygiene: spec loops", () => { 'await expect(p).rejects.toThrowError("aborted")', "expect(() => f()).toThrow(/abort/i)", "await expect(p).rejects.toThrowError(/aborted/i)", + 'await expect(p).rejects.toThrow("was aborted")', + 'await expect(p).rejects.toThrow("Request was aborted.")', + 'await expect(p).rejects.toThrow("The request was aborted")', + "await expect(p).rejects.toThrow(/was aborted/)", ]) { it(`flags a bare abort substring: ${code}`, () => { assert.equal(lintSpec(code).length, 1) diff --git a/src/api/providers/__tests__/requesty.spec.ts b/src/api/providers/__tests__/requesty.spec.ts index a94e80a4e0..951f337445 100644 --- a/src/api/providers/__tests__/requesty.spec.ts +++ b/src/api/providers/__tests__/requesty.spec.ts @@ -903,6 +903,71 @@ describe("RequestyHandler", () => { expect(mockCreate).not.toHaveBeenCalled() }) + it("forwards the request signal to the model lookup", async () => { + const handler = new RequestyHandler(mockOptions) + const controller = new AbortController() + const { getModels } = await import("../fetchers/modelCache") + let lookupSignal: AbortSignal | undefined + let notifyLookupStarted!: () => void + const lookupStarted = new Promise((resolve) => { + notifyLookupStarted = resolve + }) + vitest.mocked(getModels).mockImplementationOnce((options) => { + lookupSignal = options.signal + notifyLookupStarted() + return new Promise(() => {}) + }) + + const promise = handler.completePrompt("test prompt", { abortSignal: controller.signal }) + void promise.catch(() => {}) + await withSettleGuard(lookupStarted) + expect(lookupSignal).toBeDefined() + expect(lookupSignal?.aborted).toBe(false) + controller.abort() + + await expect(withSettleGuard(promise)).rejects.toMatchObject({ name: "AbortError" }) + expect(lookupSignal?.aborted).toBe(true) + }) + + it("does not leak an unhandled rejection when the lookup rejects after abort", async () => { + const handler = new RequestyHandler(mockOptions) + const controller = new AbortController() + const { getModels } = await import("../fetchers/modelCache") + let notifyLookupStarted!: () => void + const lookupStarted = new Promise((resolve) => { + notifyLookupStarted = resolve + }) + let rejectLookup!: (error: Error) => void + vitest.mocked(getModels).mockImplementationOnce(() => { + notifyLookupStarted() + return new Promise((_resolve, reject) => { + rejectLookup = reject + }) + }) + + const unhandled: unknown[] = [] + const onUnhandledRejection = (reason: unknown) => unhandled.push(reason) + process.on("unhandledRejection", onUnhandledRejection) + try { + const promise = handler.completePrompt("test prompt", { abortSignal: controller.signal }) + void promise.catch(() => {}) + await withSettleGuard(lookupStarted) + controller.abort() + await expect(withSettleGuard(promise)).rejects.toMatchObject({ + name: "AbortError", + message: "The Requesty request was aborted", + }) + + // The lookup fails after the request already settled as aborted. + rejectLookup(new Error("late lookup failure")) + // Node reports an unhandled rejection only after the microtask queue drains. + await new Promise((resolve) => setTimeout(resolve, 0)) + expect(unhandled).toHaveLength(0) + } finally { + process.off("unhandledRejection", onUnhandledRejection) + } + }) + it("applies timeoutMs to model lookup with no caller signal at all", async () => { // The comment above completePrompt claims timeoutMs covers model lookup too. With no // caller signal the only cancellation source is the timeout this method configures, so diff --git a/src/api/providers/requesty.ts b/src/api/providers/requesty.ts index 36ecf8ba74..b69afb18f0 100644 --- a/src/api/providers/requesty.ts +++ b/src/api/providers/requesty.ts @@ -104,7 +104,7 @@ export class RequestyHandler extends BaseProvider implements SingleCompletionHan return getRequestyModels(options.baseUrl, options.apiKey, ...fetchOpts) } - public async fetchModel() { + public async fetchModel(signal?: AbortSignal) { // Pass the API key so the catalog request is authenticated and the cache entry is // key-scoped. Requesty enforces per-key model allowlists, so a keyless (URL-only) // entry would let one key's catalog be served to another key on the same server. @@ -113,6 +113,9 @@ export class RequestyHandler extends BaseProvider implements SingleCompletionHan provider: providerIdentifiers.requesty, apiKey: this.options.requestyApiKey, baseUrl: this.baseURL, + // Per-waiter: modelCache detaches this caller and aborts the shared fetch only + // when the last waiter leaves. + ...(signal && { signal }), }, this, ) @@ -281,7 +284,7 @@ export class RequestyHandler extends BaseProvider implements SingleCompletionHan let modelData: Awaited> try { modelData = requestAbortSignal - ? await rejectOnAbort(this.fetchModel(), requestAbortSignal, this.providerName) + ? await rejectOnAbort(this.fetchModel(requestAbortSignal), requestAbortSignal, this.providerName) : await this.fetchModel() } catch (error) { if (isRequestAborted(error, requestAbortSignal)) { From a326612c198fbb16b1c2669be83ac99ecda30474 Mon Sep 17 00:00:00 2001 From: f0-1528 Date: Mon, 9 Nov 2026 16:55:21 +0800 Subject: [PATCH 22/26] test(opencode-go,requesty): pin the abort assertions the new lint rule exposed The compile arm failed on three lint errors after main was absorbed (merge commit 801a672fe, 602 files, no conflicts). Two of them sit in src/api/providers/__tests__/opencode-go.spec.ts, a file this pull request never touched: the branch adds abortSubstringSelector to the spec lint configs, and main's already-merged assertions then fail that new rule on the merge ref. A head-only lint run reports the same two rows one line earlier, at 1134 and 1485, so the failure is the merge-tree class a head-only run cannot see. In the merged tree eslint reports exactly the arm's three rows - 1135:10, 1486:10 and 964:36 - and exits 1. opencode-go.spec.ts:1137 - the double is now one object shared by the two listeners that race to reject the next() promise, and the test asserts rejects.toBe(abortDouble). The stream path propagates the SDK-level rejection unchanged, so identity is the strongest true statement; asserting an AbortError name and a provider message there would assert a contract this handler does not have. opencode-go.spec.ts:1495 - the Responses completion path wraps every failure (opencode-go.ts:439 throws "Opencode Go completion error: "), so the row pins name "Error" and that whole message instead of a bare substring that a raw SDK error would match too. requesty.spec.ts:966 - the fixed sleep becomes the shared flushMacrotasks() helper (src/test-utils/timers.ts): a rejection that ends up handled has no observable condition to poll, which is what that helper is for, and setTimeout is what the rule bans in specs. Controls in this repo, on the merged tree: eslint over src exits 1 with those three errors before the change and 0 after; the two spec files report 177 passed after it; and mutating the two pinned details - asserting a fresh but equal error object, and dropping the provider prefix from the message - fails exactly those two tests ("expected Error: request aborted to be Error: request aborted", and the mismatch on { name: 'Error', message: 'Opencode Go completion error: request aborted' }) and nothing else. Test-only: no implementation file is touched, eslint-suppressions counts are unchanged for every entry (opencode-go.spec.ts stays 3). --- .../providers/__tests__/opencode-go.spec.ts | 25 +++++++++++++++---- src/api/providers/__tests__/requesty.spec.ts | 7 ++++-- 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/src/api/providers/__tests__/opencode-go.spec.ts b/src/api/providers/__tests__/opencode-go.spec.ts index b96cf2acce..6ee2df0f87 100644 --- a/src/api/providers/__tests__/opencode-go.spec.ts +++ b/src/api/providers/__tests__/opencode-go.spec.ts @@ -1100,13 +1100,19 @@ describe("OpencodeGoHandler", () => { it("stops an in-flight Responses iterator when its abort signal rejects the read", async () => { const controller = new AbortController() + // The iterator's own listener and the request-level listener race to reject the same + // promise, so the double is one object and the assertion below pins which rejection + // propagated. opencode-go hands the caller's signal straight to the SDK and never + // normalises it into a provider AbortError, so there is no provider message to assert + // on this path: identity is the strongest statement that is true here. + const abortDouble = new Error("request aborted") let rejectNext: ((reason?: unknown) => void) | undefined const iterator = { next: vitest.fn().mockImplementation( () => new Promise((_resolve, reject) => { rejectNext = reject - controller.signal.addEventListener("abort", () => reject(new Error("request aborted")), { + controller.signal.addEventListener("abort", () => reject(abortDouble), { once: true, }) }), @@ -1117,7 +1123,7 @@ describe("OpencodeGoHandler", () => { }, } mockResponsesCreate.mockImplementation(async (_body: unknown, options: { signal?: AbortSignal }) => { - options.signal?.addEventListener("abort", () => rejectNext?.(new Error("request aborted")), { + options.signal?.addEventListener("abort", () => rejectNext?.(abortDouble), { once: true, }) return iterator @@ -1132,7 +1138,7 @@ describe("OpencodeGoHandler", () => { await vitest.waitFor(() => expect(mockResponsesCreate).toHaveBeenCalled()) controller.abort() - await expect(nextPromise).rejects.toThrow("request aborted") + await expect(nextPromise).rejects.toBe(abortDouble) expect(iterator.return).toHaveBeenCalled() }) @@ -1473,8 +1479,14 @@ describe("OpencodeGoHandler", () => { it("rejects non-streaming Responses completion when the abort signal fires", async () => { const controller = new AbortController() + // One double object here too. The Responses completion path wraps every failure + // (`opencode-go.ts:439` throws `Opencode Go completion error: `), so the + // abort surfaces as a new Error carrying the double's text behind the provider prefix, + // not as the double and not as an AbortError. The assertion below pins that whole + // message; a bare "abort" substring would also match a raw SDK error. + const abortDouble = new Error("request aborted") const request = new Promise((_resolve, reject) => { - controller.signal.addEventListener("abort", () => reject(new Error("request aborted")), { once: true }) + controller.signal.addEventListener("abort", () => reject(abortDouble), { once: true }) }) mockResponsesCreate.mockReturnValue(request) const handler = new OpencodeGoHandler(lunaOptions) @@ -1483,7 +1495,10 @@ describe("OpencodeGoHandler", () => { await vitest.waitFor(() => expect(mockResponsesCreate).toHaveBeenCalled()) controller.abort() - await expect(completion).rejects.toThrow("request aborted") + await expect(completion).rejects.toMatchObject({ + name: "Error", + message: `Opencode Go completion error: ${abortDouble.message}`, + }) }) it("completePrompt calls responses.create and returns output_text", async () => { diff --git a/src/api/providers/__tests__/requesty.spec.ts b/src/api/providers/__tests__/requesty.spec.ts index 951f337445..0603f395a6 100644 --- a/src/api/providers/__tests__/requesty.spec.ts +++ b/src/api/providers/__tests__/requesty.spec.ts @@ -20,6 +20,7 @@ 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" +import { flushMacrotasks } from "../../../test-utils/timers" const mockCreate = vitest.fn() @@ -960,8 +961,10 @@ describe("RequestyHandler", () => { // The lookup fails after the request already settled as aborted. rejectLookup(new Error("late lookup failure")) - // Node reports an unhandled rejection only after the microtask queue drains. - await new Promise((resolve) => setTimeout(resolve, 0)) + // Node reports an unhandled rejection only after the queue drains, and a rejection + // that ends up handled has no observable state to poll, so drain the queue through + // the shared helper instead of sleeping. + await flushMacrotasks() expect(unhandled).toHaveLength(0) } finally { process.off("unhandledRejection", onUnhandledRejection) From 177fb9513bc8e173f4c91101f33ec821e47f7ad9 Mon Sep 17 00:00:00 2001 From: f0-1528 Date: Mon, 9 Nov 2026 19:04:48 +0800 Subject: [PATCH 23/26] fix(config-eslint): flag an escaped dot or anchor in a bare abort assertion The row on this pull request's own rule says a regex with an escaped dot slips past abortSubstringSelector: /^Request was aborted\.$/ is not flagged while the same message written as a string is. Measured against the rule as it stood, through the extension's real config (calculateConfigForFile on core/task/__tests__/example.spec.ts, then linter.verify): MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit toThrow(/^aborted$/) 1 (already flagged) toThrow(/^Request was aborted\.$/) 0 <- the hole toThrowError(/request aborted\.$/i) 0 <- the hole toThrow(/^request aborted\$/) 0 <- same hole, escaped anchor toThrow(/^Request was aborted because the peer …\.$/) 0 (a full message stays allowed) The tail [.$]?$ cannot consume the backslash that an escaped dot or anchor leaves in the pattern text, so the match dies one character short. The tail becomes [\\.$]*$ in both branches of the selector (the string-value branch and the regex-pattern branch), which consumes \\., \$, . or $ before the end and nothing else. Red before, green after, in packages/config-eslint/api-provider-hygiene.test.js: the four new rows fail with 'expected: 1' against the old tail (# tests 89, # fail 2 - the loop runs for two lane paths) and pass against the new one (# tests 89, # pass 89, # fail 0). After the change the probe reports 13 of 13 cases as wanted, including toThrow(/aborted\./) -> 1, rejects.not.toThrow(/^Request was aborted\.$/) -> 0 and the long-message cases -> 0. No spec in the repository is newly caught: eslint over src (with main merged at a326612c1) still exits 0 with --max-warnings=0, root pnpm run lint is 12/12, and pnpm lint in packages/config-eslint exits 0. No eslint-suppression entry changes (pnpm exec eslint --prune-suppressions on both files leaves src/eslint-suppressions.json byte-identical). Rule source and its test are the only files touched. --- packages/config-eslint/api-provider-hygiene.js | 5 ++++- packages/config-eslint/api-provider-hygiene.test.js | 7 +++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/packages/config-eslint/api-provider-hygiene.js b/packages/config-eslint/api-provider-hygiene.js index 041a0b2a24..f04dccc4d5 100644 --- a/packages/config-eslint/api-provider-hygiene.js +++ b/packages/config-eslint/api-provider-hygiene.js @@ -66,7 +66,10 @@ export const apiProviderHygieneConfig = { // Shared by every spec lane, not only src/api: an abort assertion must not pass on a raw SDK error. export const abortSubstringSelector = { selector: - "CallExpression[callee.property.name=/^toThrow(Error)?$/]:not([callee.object.property.name='not']):matches([arguments.0.value=/^[$^]?(the )?(request )?(was )?abort(ed)?[.$]?$/i], [arguments.0.regex.pattern=/^[$^]?(the )?(request )?(was )?abort(ed)?[.$]?$/i])", + // The escaped-dot and escaped-anchor forms keep a backslash in the pattern text, so the tail + // has to allow one before the dot or anchor; without it /^Request was aborted\\.$/ slips + // through the rule while the same message written as a string is flagged. + "CallExpression[callee.property.name=/^toThrow(Error)?$/]:not([callee.object.property.name='not']):matches([arguments.0.value=/^[$^]?(the )?(request )?(was )?abort(ed)?[\\\\.$]*$/i], [arguments.0.regex.pattern=/^[$^]?(the )?(request )?(was )?abort(ed)?[\\\\.$]*$/i])", message: "A bare 'abort' substring also matches a raw SDK error. Assert the AbortError name and the full provider message.", } diff --git a/packages/config-eslint/api-provider-hygiene.test.js b/packages/config-eslint/api-provider-hygiene.test.js index a1ed8fac15..9e8fa61886 100644 --- a/packages/config-eslint/api-provider-hygiene.test.js +++ b/packages/config-eslint/api-provider-hygiene.test.js @@ -297,7 +297,14 @@ describe("abort assertions in the extension ESLint config", () => { for (const [code, expectedCount] of [ ['await expect(p).rejects.toThrow("aborted")', 1], ["await expect(p).rejects.toThrowError(/abort/i)", 1], + // An escaped dot in the source text is still a bare abort substring: the pattern + // keeps a backslash, so the matcher must allow it before the trailing dot or anchor. + ["await expect(p).rejects.toThrow(/^aborted$/)", 1], + ["await expect(p).rejects.toThrow(/^Request was aborted\\.$/)", 1], + ["await expect(p).rejects.toThrowError(/request aborted\\.$/i)", 1], + ["await expect(p).rejects.toThrow(/^request aborted\\$/)", 1], ['await expect(p).rejects.not.toThrow("aborted")', 0], + ["await expect(p).rejects.toThrow(/^Request was aborted because the peer closed the stream\\.$/)", 0], [ 'await expect(p).rejects.toMatchObject({ name: "AbortError", message: "The Requesty request was aborted" })', 0, From df3e889ba42f92eaeadd5a5a6596f5ac67265b40 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Mon, 9 Nov 2026 19:39:32 +0800 Subject: [PATCH 24/26] config-eslint,api: close the escaped-group hole and reach the ignored workflow tests abortSubstringSelector consumed escaped dots and anchors but stopped at an inline non-capturing group, so toThrow(/^Request was aborted\.$/) and toThrow(/abort(?:ed)?/i) fell through the rule while the same message written as a string was flagged. The pattern branch now consumes a (?:ed|ion|al)? group verbatim, and the four workflow tests that are exempt from the path-traversal selectors get the abort rule back instead of losing the whole block. Assertions tightened where they were blind: Requesty's normalised completion message instead of a substring that the unwrapped SDK error also satisfies, the model-lookup rejection by object identity, the ask abort message by task and instance id, and a case where only the per-request timeout may abort while the caller signal stays live. Controls, each run and reverted: rule suite 90/90 with the new tail, 89/90 with the old one and with the workflow mapping removed; requesty spec fails on the exact message when handleOpenAIError is bypassed at both catch sites, fails on Object.is when the lookup assertion takes a copy, and times out in the settle guard at timeoutMs 100_000; the ask assertion fails against foreign task ids. eslint over src is 0 findings before and after, the repo-wide abort-rule count stays 11, the four workflow files score 0 under the selector alone, and the mutation selection is 46 executable lines against main tip / 0 packages against head^1 (cap 500). --- .../config-eslint/api-provider-hygiene.js | 7 ++- .../api-provider-hygiene.test.js | 17 ++++++- src/api/providers/__tests__/requesty.spec.ts | 47 ++++++++++++++++++- .../ask-queued-message-drain.spec.ts | 13 ++++- src/eslint.config.mjs | 14 ++++++ 5 files changed, 91 insertions(+), 7 deletions(-) diff --git a/packages/config-eslint/api-provider-hygiene.js b/packages/config-eslint/api-provider-hygiene.js index f04dccc4d5..597b62a1b0 100644 --- a/packages/config-eslint/api-provider-hygiene.js +++ b/packages/config-eslint/api-provider-hygiene.js @@ -68,8 +68,11 @@ export const abortSubstringSelector = { selector: // The escaped-dot and escaped-anchor forms keep a backslash in the pattern text, so the tail // has to allow one before the dot or anchor; without it /^Request was aborted\\.$/ slips - // through the rule while the same message written as a string is flagged. - "CallExpression[callee.property.name=/^toThrow(Error)?$/]:not([callee.object.property.name='not']):matches([arguments.0.value=/^[$^]?(the )?(request )?(was )?abort(ed)?[\\\\.$]*$/i], [arguments.0.regex.pattern=/^[$^]?(the )?(request )?(was )?abort(ed)?[\\\\.$]*$/i])", + // through the rule while the same message written as a string is flagged. The pattern + // branch also has to consume a non-capturing group verbatim, because an inline + // (?:ed)? is regex source, not a message character, and an anchored match that stops + // short of it leaves /abort(?:ed)?/i unflagged. + "CallExpression[callee.property.name=/^toThrow(Error)?$/]:not([callee.object.property.name='not']):matches([arguments.0.value=/^[$^]?(the )?(request )?(was )?abort(ed)?[\\\\.$]*$/i], [arguments.0.regex.pattern=/^[$^]?(the )?(request )?(was )?abort(ed)?(?:\\(\\?:(?:ed|ion|al)\\)\\?)?[\\\\.$]*$/i])", message: "A bare 'abort' substring also matches a raw SDK error. Assert the AbortError name and the full provider message.", } diff --git a/packages/config-eslint/api-provider-hygiene.test.js b/packages/config-eslint/api-provider-hygiene.test.js index 9e8fa61886..e86d02b36a 100644 --- a/packages/config-eslint/api-provider-hygiene.test.js +++ b/packages/config-eslint/api-provider-hygiene.test.js @@ -284,7 +284,13 @@ describe("abort assertions in the extension ESLint config", () => { const srcRoot = fileURLToPath(new URL("../../src/", import.meta.url)) const eslint = new ESLint({ cwd: srcRoot }) - for (const filename of ["core/task/__tests__/example.spec.ts", "core/config/__tests__/example.spec.ts"]) { + for (const filename of [ + "core/task/__tests__/example.spec.ts", + "core/config/__tests__/example.spec.ts", + // The workflow tests are exempt from the path-traversal selectors, not from the abort + // rule; listing one proves the lane that re-applies it actually resolves for them. + "services/__tests__/pr-review-state-workflow.test.ts", + ]) { it(`checks rejected and accepted assertions in ${filename}`, async () => { const resolved = await eslint.calculateConfigForFile(filename) const syntaxConfig = [ @@ -303,6 +309,15 @@ describe("abort assertions in the extension ESLint config", () => { ["await expect(p).rejects.toThrow(/^Request was aborted\\.$/)", 1], ["await expect(p).rejects.toThrowError(/request aborted\\.$/i)", 1], ["await expect(p).rejects.toThrow(/^request aborted\\$/)", 1], + // An inline non-capturing group is regex source, not part of the asserted + // message: an anchored match that stops short of it lets a bare abort through. + ["await expect(p).rejects.toThrow(/abort(?:ed)?/i)", 1], + ["await expect(p).rejects.toThrowError(/^request aborted(?:ed)?$/i)", 1], + ["await expect(p).rejects.toThrow(/cancel(?:ed)?/i)", 0], + [ + "await expect(p).rejects.toThrow(/^The Requesty request was aborted(?:ed)? because the peer closed the stream\\.$/)", + 0, + ], ['await expect(p).rejects.not.toThrow("aborted")', 0], ["await expect(p).rejects.toThrow(/^Request was aborted because the peer closed the stream\\.$/)", 0], [ diff --git a/src/api/providers/__tests__/requesty.spec.ts b/src/api/providers/__tests__/requesty.spec.ts index 0603f395a6..d4dfe2c566 100644 --- a/src/api/providers/__tests__/requesty.spec.ts +++ b/src/api/providers/__tests__/requesty.spec.ts @@ -764,7 +764,10 @@ describe("RequestyHandler", () => { expect(error).toBeInstanceOf(Error) expect((error as Error).name).not.toBe("AbortError") - expect((error as Error).message).toContain("API Error") + // Assert the normalised message, not a substring: "API Error" is also the message of + // the unwrapped SDK error, so a catch that skipped handleOpenAIError would pass a + // contains-check while the provider contract was gone. + expect((error as Error).message).toBe("Requesty completion error: API Error") }, ) @@ -998,7 +1001,9 @@ describe("RequestyHandler", () => { return Promise.reject(lookupError) }) - await expect(handler.completePrompt("test prompt")).rejects.toThrow("lookup failed") + // The handler rethrows this exact object, and the fixture owns it: pin the identity rather + // than the message text, which a rebuilt copy of the same failure would also satisfy. + await expect(handler.completePrompt("test prompt")).rejects.toBe(lookupError) }) it("normalizes raw AbortError lookup failures to the provider AbortError", async () => { @@ -1207,6 +1212,44 @@ describe("RequestyHandler", () => { ) }) + it("lets the per-request timeout abort the request while the caller signal stays live", async () => { + // The caller signal is live and never aborted here, so only the timer may end the request. + // The combined-options test above aborts through the controller and cannot see a merged + // signal that drops the timeout whenever a caller signal exists; this one hangs if it does. + 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: 25 }) + void promise.catch(() => {}) + await withSettleGuard(createStarted) + + await expect(withSettleGuard(promise)).rejects.toMatchObject({ name: "AbortError" }) + expect(controller.signal.aborted).toBe(false) + expect(requestSignal).toBeDefined() + expect(requestSignal).not.toBe(controller.signal) + expect(requestSignal?.aborted).toBe(true) + }) + 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 diff --git a/src/core/task/__tests__/ask-queued-message-drain.spec.ts b/src/core/task/__tests__/ask-queued-message-drain.spec.ts index 5004ab327b..6c4edd23ee 100644 --- a/src/core/task/__tests__/ask-queued-message-drain.spec.ts +++ b/src/core/task/__tests__/ask-queued-message-drain.spec.ts @@ -15,7 +15,12 @@ const getQueueTaskTestAccess = (task: Task) => task as unknown as QueueTaskTestA describe("Task.ask queued message drain", () => { function createTask(provider?: { getState: () => Promise> }) { - const task = Object.create(Task.prototype) as Task + // Distinct task and instance ids, so an abort assertion can name this task instead of + // accepting whatever the fixture left unset. + const task = Object.create(Task.prototype, { + taskId: { value: "task-ask-drain", enumerable: true }, + instanceId: { value: "instance-ask-drain", enumerable: true }, + }) as Task setAbortLatch(task, false) ;(task as any).clineMessages = [] ;(task as any).askResponse = undefined @@ -349,7 +354,11 @@ describe("Task.ask queued message drain", () => { setAbortLatch(task, true) finishAddingAsk() - await expect(ask).rejects.toThrow(/#ask\] task \S+ aborted$/) + // The abort must name this task and this instance: a regex that accepts any id would also + // pass for another task's abort or a different prefix. + const rejection = await ask.catch((e: unknown) => e) + expect((rejection as Error).message).toBe(`[ZooCode#ask] task ${task.taskId}.${task.instanceId} aborted`) + expect((rejection as Error).message).toBe("[ZooCode#ask] task task-ask-drain.instance-ask-drain aborted") expect(task.messageQueueService.messages).toHaveLength(1) expect(task.messageQueueService.claimNextMessage()?.text).toBe("Persist me later") }) diff --git a/src/eslint.config.mjs b/src/eslint.config.mjs index a3853bd822..d850e6cf07 100644 --- a/src/eslint.config.mjs +++ b/src/eslint.config.mjs @@ -144,6 +144,20 @@ export default [ "no-restricted-syntax": ["error", ...testPathTraversalSelectors, abortSubstringSelector], }, }, + { + // The four workflow tests ignored above are exempt from the path-traversal selectors + // because they read root-level config. That exception was never about abort + // assertions, so re-apply the abort rule for exactly those files. + files: [ + "**/services/__tests__/pr-review-state-workflow.test.ts", + "**/services/__tests__/stale-pr-clock-workflow.test.ts", + "**/services/__tests__/fork-review-followup-workflow.test.ts", + "**/services/__tests__/workflow-self-listen.test.ts", + ], + rules: { + "no-restricted-syntax": ["error", abortSubstringSelector], + }, + }, { // A later block replaces no-restricted-syntax for the same files, so repeat the traversal selectors. files: ["api/**/__tests__/**/*.spec.ts"], From 729fad6e35855b3870bd60ef56b4c082ac1f8f2f Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Mon, 9 Nov 2026 20:11:32 +0800 Subject: [PATCH 25/26] test(api): prove the Requesty lookup timeout aborts with a live caller signal The bot row asked for the pending model-lookup case with both options present. The existing lookup-deadline case supplies no caller signal, so nothing proved the deadline still ends a lookup when a caller signal is live and that the caller signal is not the thing that changed. New case asserts the provider AbortError, controller.signal.aborted false, and that no completion was issued. Control: dropping the merged timeout whenever a caller signal exists turns this case and the completion-timeout case red (2 failed / 52 passed) while the no-caller-signal lookup case stays green; flipping the caller-signal assertion gives expected false to be true // Object.is equality. --- src/api/providers/__tests__/requesty.spec.ts | 23 ++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/api/providers/__tests__/requesty.spec.ts b/src/api/providers/__tests__/requesty.spec.ts index d4dfe2c566..a6db1cf0ff 100644 --- a/src/api/providers/__tests__/requesty.spec.ts +++ b/src/api/providers/__tests__/requesty.spec.ts @@ -993,6 +993,29 @@ describe("RequestyHandler", () => { expect(mockCreate).not.toHaveBeenCalled() }) + it("applies timeoutMs to model lookup while a live caller signal stays untouched", async () => { + // The case above proves the deadline can end a pending lookup with no caller in the picture. + // This one proves it still does when a caller signal *is* present: the lookup must die on the + // merged request signal, not on the caller's, and the caller's own signal must still read as + // live afterwards. Red if the timeout is dropped whenever a caller signal exists (the lookup + // then hangs and the settle guard fires), which no other case here distinguishes. + const handler = new RequestyHandler(mockOptions) + const controller = new AbortController() + const { getModels } = await import("../fetchers/modelCache") + vitest.mocked(getModels).mockImplementationOnce(() => new Promise(() => {})) + const promise = handler.completePrompt("test prompt", { + abortSignal: controller.signal, + timeoutMs: 20, + }) + void promise.catch(() => {}) + await expect(withSettleGuard(promise)).rejects.toMatchObject({ + name: "AbortError", + message: "The Requesty request was aborted", + }) + expect(controller.signal.aborted, "the timeout must not surface through the caller signal").toBe(false) + expect(mockCreate, "the lookup must not fall through into a completion").not.toHaveBeenCalled() + }) + it("rethrows non-abort model lookup failures from completePrompt", async () => { const handler = new RequestyHandler(mockOptions) const { getModels } = await import("../fetchers/modelCache") From 622bb324593f4623fd0671f439626423c99b31cc Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Mon, 9 Nov 2026 23:08:28 +0000 Subject: [PATCH 26/26] fix: address CodeRabbit abort assertion findings --- packages/config-eslint/api-provider-hygiene.js | 10 +++------- packages/config-eslint/api-provider-hygiene.test.js | 7 +++++++ src/api/providers/__tests__/requesty.spec.ts | 3 +++ .../task/__tests__/ask-queued-message-drain.spec.ts | 9 ++++++--- 4 files changed, 19 insertions(+), 10 deletions(-) diff --git a/packages/config-eslint/api-provider-hygiene.js b/packages/config-eslint/api-provider-hygiene.js index 597b62a1b0..f340694834 100644 --- a/packages/config-eslint/api-provider-hygiene.js +++ b/packages/config-eslint/api-provider-hygiene.js @@ -66,13 +66,9 @@ export const apiProviderHygieneConfig = { // Shared by every spec lane, not only src/api: an abort assertion must not pass on a raw SDK error. export const abortSubstringSelector = { selector: - // The escaped-dot and escaped-anchor forms keep a backslash in the pattern text, so the tail - // has to allow one before the dot or anchor; without it /^Request was aborted\\.$/ slips - // through the rule while the same message written as a string is flagged. The pattern - // branch also has to consume a non-capturing group verbatim, because an inline - // (?:ed)? is regex source, not a message character, and an anchored match that stops - // short of it leaves /abort(?:ed)?/i unflagged. - "CallExpression[callee.property.name=/^toThrow(Error)?$/]:not([callee.object.property.name='not']):matches([arguments.0.value=/^[$^]?(the )?(request )?(was )?abort(ed)?[\\\\.$]*$/i], [arguments.0.regex.pattern=/^[$^]?(the )?(request )?(was )?abort(ed)?(?:\\(\\?:(?:ed|ion|al)\\)\\?)?[\\\\.$]*$/i])", + // Strings contain literal punctuation; only regex source can contain anchors, wildcards, + // optional suffix groups, or word boundaries around the generic abort wording. + "CallExpression[callee.property.name=/^toThrow(Error)?$/]:not([callee.object.property.name='not']):matches([arguments.0.value=/^(the )?(request )?(was )?abort(ed)?[.]*$/i], [arguments.0.regex.pattern=/^[$^]?(?:\\.\\*)?(the )?(request )?(was )?abort(ed)?(?:\\(\\?:(?:ed|ion|al)\\)\\??)?(?:\\\\b)?(?:\\.\\*)?[\\\\.$]*$/i])", message: "A bare 'abort' substring also matches a raw SDK error. Assert the AbortError name and the full provider message.", } diff --git a/packages/config-eslint/api-provider-hygiene.test.js b/packages/config-eslint/api-provider-hygiene.test.js index e86d02b36a..310500d62f 100644 --- a/packages/config-eslint/api-provider-hygiene.test.js +++ b/packages/config-eslint/api-provider-hygiene.test.js @@ -306,6 +306,13 @@ describe("abort assertions in the extension ESLint config", () => { // An escaped dot in the source text is still a bare abort substring: the pattern // keeps a backslash, so the matcher must allow it before the trailing dot or anchor. ["await expect(p).rejects.toThrow(/^aborted$/)", 1], + ["await expect(p).rejects.toThrow(/.*aborted.*/i)", 1], + ["await expect(p).rejects.toThrowError(/abort(?:ed)/i)", 1], + ["await expect(p).rejects.toThrow(/aborted\\b/i)", 1], + ['await expect(p).rejects.toThrow("abort$")', 0], + ['await expect(p).rejects.toThrow("^abort")', 0], + ["await expect(p).rejects.toThrow(/.*The Requesty request was aborted.*/i)", 0], + ["await expect(p).rejects.toThrow(/The Requesty request was aborted\\b/i)", 0], ["await expect(p).rejects.toThrow(/^Request was aborted\\.$/)", 1], ["await expect(p).rejects.toThrowError(/request aborted\\.$/i)", 1], ["await expect(p).rejects.toThrow(/^request aborted\\$/)", 1], diff --git a/src/api/providers/__tests__/requesty.spec.ts b/src/api/providers/__tests__/requesty.spec.ts index a6db1cf0ff..31a245570e 100644 --- a/src/api/providers/__tests__/requesty.spec.ts +++ b/src/api/providers/__tests__/requesty.spec.ts @@ -1012,6 +1012,9 @@ describe("RequestyHandler", () => { name: "AbortError", message: "The Requesty request was aborted", }) + const lookupSignal = vitest.mocked(getModels).mock.calls.at(-1)?.[0].signal + expect(lookupSignal).toBeInstanceOf(AbortSignal) + expect(lookupSignal?.aborted, "the timeout must cancel the model lookup").toBe(true) expect(controller.signal.aborted, "the timeout must not surface through the caller signal").toBe(false) expect(mockCreate, "the lookup must not fall through into a completion").not.toHaveBeenCalled() }) diff --git a/src/core/task/__tests__/ask-queued-message-drain.spec.ts b/src/core/task/__tests__/ask-queued-message-drain.spec.ts index 6c4edd23ee..16fbb1c93e 100644 --- a/src/core/task/__tests__/ask-queued-message-drain.spec.ts +++ b/src/core/task/__tests__/ask-queued-message-drain.spec.ts @@ -356,9 +356,12 @@ describe("Task.ask queued message drain", () => { // The abort must name this task and this instance: a regex that accepts any id would also // pass for another task's abort or a different prefix. - const rejection = await ask.catch((e: unknown) => e) - expect((rejection as Error).message).toBe(`[ZooCode#ask] task ${task.taskId}.${task.instanceId} aborted`) - expect((rejection as Error).message).toBe("[ZooCode#ask] task task-ask-drain.instance-ask-drain aborted") + await expect(ask).rejects.toMatchObject({ + message: `[ZooCode#ask] task ${task.taskId}.${task.instanceId} aborted`, + }) + await expect(ask).rejects.toMatchObject({ + message: "[ZooCode#ask] task task-ask-drain.instance-ask-drain aborted", + }) expect(task.messageQueueService.messages).toHaveLength(1) expect(task.messageQueueService.claimNextMessage()?.text).toBe("Persist me later") })