diff --git a/src/api/providers/utils/__tests__/abort-signal.spec.ts b/src/api/providers/utils/__tests__/abort-signal.spec.ts index 1692f71e63..8a9a63bb94 100644 --- a/src/api/providers/utils/__tests__/abort-signal.spec.ts +++ b/src/api/providers/utils/__tests__/abort-signal.spec.ts @@ -3,8 +3,196 @@ import { isRequestAborted, mergeAbortSignalAndTimeout, mergeAbortSignals, - throwIfAborted, + rejectOnAbort, + resolveModelWithAbort, } from "../abort-signal" +import { withSettleGuard } from "../../../../test-utils/settle-guard" + +describe("rejectOnAbort", () => { + it("resolves with the pending value when it settles before the signal aborts", async () => { + const controller = new AbortController() + + await expect( + withSettleGuard(rejectOnAbort(Promise.resolve("done"), controller.signal, "TestProvider")), + ).resolves.toBe("done") + expect(controller.signal.aborted).toBe(false) + }) + + it("rejects with the provider abort error when the signal aborts first", async () => { + const controller = new AbortController() + // Never settles: the race must end purely via the abort. + const pending = new Promise(() => {}) + const race = rejectOnAbort(pending, controller.signal, "TestProvider") + controller.abort() + + await expect(withSettleGuard(race)).rejects.toMatchObject({ + name: "AbortError", + message: "The TestProvider request was aborted", + }) + }) + + it("rejects immediately when the signal is already aborted", async () => { + const controller = new AbortController() + controller.abort() + const pending = new Promise(() => {}) + + await expect(withSettleGuard(rejectOnAbort(pending, controller.signal, "TestProvider"))).rejects.toMatchObject({ + name: "AbortError", + message: "The TestProvider request was aborted", + }) + }) + + it("propagates the pending rejection when the signal stays active", async () => { + const controller = new AbortController() + const boom = new Error("lookup failed") + + await expect( + withSettleGuard(rejectOnAbort(Promise.reject(boom), controller.signal, "TestProvider")), + ).rejects.toBe(boom) + }) + + it("detaches the abort listener once the pending settles", async () => { + const controller = new AbortController() + const addSpy = vi.spyOn(controller.signal, "addEventListener") + const removeSpy = vi.spyOn(controller.signal, "removeEventListener") + + await expect( + withSettleGuard(rejectOnAbort(Promise.resolve("done"), controller.signal, "TestProvider")), + ).resolves.toBe("done") + + // The settle path must remove the exact listener that was registered, not just any + // function: removing a different reference would leave the original abort listener + // attached to the signal. First require the registration to have happened at all, + // so a missing registration cannot silently degrade to an undefined comparison. + expect(addSpy).toHaveBeenCalledTimes(1) + const registeredListener = addSpy.mock.calls[0]?.[1] as EventListener | undefined + expect(typeof registeredListener).toBe("function") + expect(removeSpy).toHaveBeenCalledWith("abort", registeredListener) + addSpy.mockRestore() + removeSpy.mockRestore() + }) + + it("detaches the abort listener when the pending rejects", async () => { + const controller = new AbortController() + const addSpy = vi.spyOn(controller.signal, "addEventListener") + const removeSpy = vi.spyOn(controller.signal, "removeEventListener") + const lookupError = new Error("lookup failed") + + await expect( + withSettleGuard(rejectOnAbort(Promise.reject(lookupError), controller.signal, "TestProvider")), + ).rejects.toBe(lookupError) + + // The settle path must remove the exact listener that was registered, not just any + // function: removing a different reference would leave the original abort listener + // attached to the signal. First require the registration to have happened at all, + // so a missing registration cannot silently degrade to an undefined comparison. + expect(addSpy).toHaveBeenCalledTimes(1) + const registeredListener = addSpy.mock.calls[0]?.[1] as EventListener | undefined + expect(typeof registeredListener).toBe("function") + expect(removeSpy).toHaveBeenCalledWith("abort", registeredListener) + addSpy.mockRestore() + removeSpy.mockRestore() + }) +}) + +describe("resolveModelWithAbort", () => { + it("fast-fails with the provider abort error when the signal is already aborted", async () => { + const controller = new AbortController() + controller.abort() + let lookupRan = false + + await expect( + withSettleGuard( + resolveModelWithAbort( + async () => { + lookupRan = true + return "model" + }, + controller.signal, + "TestProvider", + ), + ), + ).rejects.toMatchObject({ + name: "AbortError", + message: "The TestProvider request was aborted", + }) + // The entry guard must run before the lookup starts at all. + expect(lookupRan).toBe(false) + }) + + it("resolves the model when no abort signal is present", async () => { + await expect(resolveModelWithAbort(async () => "model", undefined, "TestProvider")).resolves.toBe("model") + }) + + it("propagates a raw resolution failure unchanged when no abort signal is present", async () => { + const boom = new Error("lookup failed") + + await expect( + resolveModelWithAbort( + async () => { + throw boom + }, + undefined, + "TestProvider", + ), + ).rejects.toBe(boom) + }) + + it("rejects with the provider abort error when the signal aborts while resolution is pending", async () => { + const controller = new AbortController() + let release: (value: string) => void = () => {} + const pending = new Promise((resolve) => { + release = resolve + }) + const race = withSettleGuard(resolveModelWithAbort(async () => pending, controller.signal, "TestProvider")) + + setTimeout(() => controller.abort(), 10) + + await expect(race).rejects.toMatchObject({ + name: "AbortError", + message: "The TestProvider request was aborted", + }) + // A late resolution after the abort won the race must not surface. + release("late model") + }) + + it("normalizes an abort-flavored resolution failure to the provider abort error", async () => { + const controller = new AbortController() + const abortFlavored = new Error("The operation was aborted.") + abortFlavored.name = "AbortError" + + const error = await resolveModelWithAbort( + async () => { + throw abortFlavored + }, + controller.signal, + "TestProvider", + ).catch((caught: unknown) => caught) + + expect(error).not.toBe(abortFlavored) + expect(error).toMatchObject({ + name: "AbortError", + message: "The TestProvider request was aborted", + }) + }) + + it("propagates non-abort resolution failures unchanged when a signal is present", async () => { + const controller = new AbortController() + const boom = new Error("lookup failed") + + await expect( + withSettleGuard( + resolveModelWithAbort( + async () => { + throw boom + }, + controller.signal, + "TestProvider", + ), + ), + ).rejects.toBe(boom) + }) +}) describe("abort-signal utilities", () => { describe("mergeAbortSignalAndTimeout", () => { @@ -106,34 +294,6 @@ describe("abort-signal utilities", () => { }) }) - describe("throwIfAborted", () => { - it("does not throw when signal is undefined", () => { - expect(() => throwIfAborted()).not.toThrow() - }) - - it("does not throw when signal is not aborted", () => { - const controller = new AbortController() - - expect(() => throwIfAborted(controller.signal)).not.toThrow() - }) - - it("throws an AbortError when signal is already aborted", () => { - const controller = new AbortController() - controller.abort() - - let caught: unknown - try { - throwIfAborted(controller.signal) - } catch (error) { - caught = error - } - - expect(caught).toBeInstanceOf(Error) - expect((caught as Error).name).toBe("AbortError") - expect((caught as Error).message).toBe("This operation was aborted") - }) - }) - describe("isRequestAborted", () => { it("returns true when the caller signal is aborted", () => { const controller = new AbortController() @@ -167,6 +327,16 @@ describe("abort-signal utilities", () => { const controller = new AbortController() expect(isRequestAborted(new Error("boom"), controller.signal)).toBe(false) }) + + it("requires an Error instance for the name and message checks", () => { + // A plain object that merely looks like an abort must not be + // classified as one: the instanceof guard keeps such failures + // propagating unchanged so callers can inspect the real shape. + const fakeAbort = { name: "AbortError", message: "Request was aborted." } + expect(isRequestAborted(fakeAbort)).toBe(false) + expect(isRequestAborted(Object.assign(Object.create(null), { name: "APIUserAbortError" }))).toBe(false) + expect(isRequestAborted("Request was aborted.")).toBe(false) + }) }) describe("createAbortError", () => { diff --git a/src/api/providers/utils/abort-signal.ts b/src/api/providers/utils/abort-signal.ts index 26f57c3e9a..e95226d2a3 100644 --- a/src/api/providers/utils/abort-signal.ts +++ b/src/api/providers/utils/abort-signal.ts @@ -36,23 +36,6 @@ export function mergeAbortSignals(primarySignal: AbortSignal, secondarySignal?: return AbortSignal.any([primarySignal, secondarySignal]) } -/** - * Throw an AbortError if the given signal is already aborted. - * - * Use as a fast-fail guard at the top of request-building code paths so - * callers receive a consistent `name === "AbortError"` when the operation - * was cancelled before it started, without building or issuing the request. - */ -export function throwIfAborted(signal?: AbortSignal): void { - if (!signal?.aborted) { - return - } - - const abortError = new Error("This operation was aborted") - abortError.name = "AbortError" - throw abortError -} - /** * Request options this series passes to the OpenAI SDK call. The SDK's * `RequestOptions` declares `signal` as `AbortSignal | null | undefined`, @@ -66,19 +49,20 @@ export type OpenAiRequestOptions = { /** * Whether a failure indicates an aborted request: the caller's signal fired, - * the SDK raised a native abort error, or the error carries the OpenAI SDK - * abort error message (exactly "Request was aborted."). The message check - * is an exact match on purpose: a substring match would misclassify - * unrelated errors that merely mention aborting. + * an `Error` carries a native abort error name (`AbortError`, + * `APIUserAbortError`), or an `Error` carries the OpenAI SDK abort error + * message (exactly "Request was aborted."). + * + * The name and message checks require an `Error` instance on purpose: a + * plain object that merely looks like an abort must propagate unchanged so + * callers can inspect the real failure shape. The message check is an exact + * match on purpose: a substring match would misclassify unrelated errors + * that merely mention aborting. */ export function isRequestAborted(error: unknown, signal?: AbortSignal): boolean { - const candidate = error as { name?: string; message?: string } - return ( - Boolean(signal?.aborted) || - candidate?.name === "AbortError" || - candidate?.name === "APIUserAbortError" || - candidate?.message === "Request was aborted." - ) + const hasAbortName = error instanceof Error && (error.name === "AbortError" || error.name === "APIUserAbortError") + const hasSdkAbortMessage = error instanceof Error && error.message === "Request was aborted." + return Boolean(signal?.aborted) || hasAbortName || hasSdkAbortMessage } /** @@ -93,3 +77,70 @@ 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) + }, + ) + }) +} + +/** + * Resolve a provider's model metadata inside a cancellation scope. + * + * A pre-aborted signal rejects immediately with the standardized AbortError + * before any lookup starts; a signal that fires while the lookup is pending + * settles via {@link rejectOnAbort} instead of waiting for the catalog to + * resolve. Abort failures from the lookup itself are normalized to the + * provider AbortError; any other resolution failure propagates unchanged. + * + * Providers pass their own resolution step, so the entry guard, the race, and + * the normalization logic exist once and are exercised through every + * provider's spec. + */ +export async function resolveModelWithAbort( + fetchModel: () => Promise, + abortSignal: AbortSignal | undefined, + providerName: string, +): Promise { + if (abortSignal?.aborted) { + throw createAbortError(providerName) + } + + try { + if (abortSignal) { + return await rejectOnAbort(fetchModel(), abortSignal, providerName) + } + return await fetchModel() + } catch (error) { + if (isRequestAborted(error, abortSignal)) { + throw createAbortError(providerName) + } + throw error + } +} diff --git a/src/test-utils/settle-guard.ts b/src/test-utils/settle-guard.ts new file mode 100644 index 0000000000..1efe5286c8 --- /dev/null +++ b/src/test-utils/settle-guard.ts @@ -0,0 +1,26 @@ +/** + * Stryker guard: fails fast if `promise` does not settle within `ms`. + * + * Stryker's per-mutant cutoff (timeoutMS 5s x timeoutFactor 1.5 ~= 7.5s) is shorter + * than vitest's testTimeout (20s). A mutant that removes a settle call (or an abort + * listener) leaves an awaited promise pending forever; without this guard the test + * would outlive the cutoff and the mutant would be reported as Timeout (inconclusive). + * Settling the guard at 500ms turns those mutants into fast failures (KILLED). + */ +export function withSettleGuard(promise: Promise, ms = 500): Promise { + return new Promise((resolve, reject) => { + const timer = setTimeout(() => { + reject(new Error(`settle guard timed out after ${ms}ms`)) + }, ms) + void promise.then( + (value) => { + clearTimeout(timer) + resolve(value) + }, + (error) => { + clearTimeout(timer) + reject(error) + }, + ) + }) +}