Skip to content

Commit 1975c27

Browse files
easonLiangWorldedtecheasonLiangWorldedtech
committed
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.
1 parent 530033c commit 1975c27

1 file changed

Lines changed: 126 additions & 45 deletions

File tree

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

Lines changed: 126 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -726,6 +726,28 @@ describe("RequestyHandler", () => {
726726

727727
await expect(handler.completePrompt("test prompt")).rejects.toThrow("Unexpected error")
728728
})
729+
it.each([
730+
// Ceiled up: 1500.5 ms is a usable timeout and toSdkTimeoutMs rounds it to 1501.
731+
// Without the conversion the SDK would receive 1500.5 and the assertion below fails.
732+
{ input: 1500.5, expected: 1501 },
733+
// Clamped down: 3e9 exceeds the platform timer ceiling, so it is capped at MAX.
734+
// Without the conversion the SDK would receive 3e9 and the assertion below fails.
735+
{ input: 3e9, expected: 2147483647 },
736+
] as const)(
737+
"normalises a fractional or oversized timeoutMs to the SDK timeout value $input -> $expected",
738+
async ({ input, expected }) => {
739+
// The wiring under test is toSdkTimeoutMs at the createOptions construction: whole
740+
// numbers alone cannot tell the conversion from a pass-through, so both a value that
741+
// must be ceiled and a value that must be clamped are asserted on what the SDK is
742+
// actually handed.
743+
const handler = new RequestyHandler(mockOptions)
744+
mockCreate.mockResolvedValueOnce({ choices: [{ message: { content: "response" } }] })
745+
await handler.completePrompt("test prompt", { timeoutMs: input })
746+
const options = mockCreate.mock.calls.at(-1)?.[1] as { timeout?: number } | undefined
747+
expect(options?.timeout).toBe(expected)
748+
},
749+
)
750+
729751
it.each([
730752
["an external signal", { abortSignal: new AbortController().signal }],
731753
["a per-request timeout", { timeoutMs: 5_000 }],
@@ -881,6 +903,25 @@ describe("RequestyHandler", () => {
881903
expect(mockCreate).not.toHaveBeenCalled()
882904
})
883905

906+
it("applies timeoutMs to model lookup with no caller signal at all", async () => {
907+
// The comment above completePrompt claims timeoutMs covers model lookup too. With no
908+
// caller signal the only cancellation source is the timeout this method configures, so
909+
// the lookup race must be driven by that merged signal: a never-settling getModels has
910+
// to be ended by the 20ms deadline rather than hanging. Red if the lookup race is
911+
// driven by the bridged caller signal instead (undefined here), which leaves the
912+
// lookup awaited forever and the settle guard fires first.
913+
const handler = new RequestyHandler(mockOptions)
914+
const { getModels } = await import("../fetchers/modelCache")
915+
vitest.mocked(getModels).mockImplementationOnce(() => new Promise<ModelRecord>(() => {}))
916+
const promise = handler.completePrompt("test prompt", { timeoutMs: 20 })
917+
void promise.catch(() => {})
918+
await expect(withSettleGuard(promise)).rejects.toMatchObject({
919+
name: "AbortError",
920+
message: "The Requesty request was aborted",
921+
})
922+
expect(mockCreate).not.toHaveBeenCalled()
923+
})
924+
884925
it("rethrows non-abort model lookup failures from completePrompt", async () => {
885926
const handler = new RequestyHandler(mockOptions)
886927
const { getModels } = await import("../fetchers/modelCache")
@@ -911,7 +952,7 @@ describe("RequestyHandler", () => {
911952
const handler = new RequestyHandler(mockOptions)
912953
const controller = new AbortController()
913954

914-
// Deterministic synchronization (mirrors the Requesty test): the mock notifies the
955+
// Deterministic synchronization: the mock notifies the
915956
// test when the request actually starts, so the abort lands mid-flight (after model
916957
// lookup) instead of winning the race at model discovery on a slow runner.
917958
let notifyCreateStarted!: () => void
@@ -1140,50 +1181,90 @@ describe("RequestyHandler", () => {
11401181
releaseBackoff()
11411182
})
11421183

1143-
it("detaches the abort listener from the caller's signal when the completion finishes", async () => {
1144-
// The bridge registers its listener on the caller's long-lived signal, so completion must
1145-
// detach it: otherwise one listener plus one per-request controller is retained for every
1146-
// completed request on a reused signal.
1147-
const handler = new RequestyHandler(mockOptions)
1148-
const controller = new AbortController()
1149-
const addSpy = vitest.spyOn(controller.signal, "addEventListener")
1150-
const removeSpy = vitest.spyOn(controller.signal, "removeEventListener")
1151-
mockCreate.mockResolvedValueOnce({ choices: [{ message: { content: "response" } }] })
1152-
1153-
try {
1154-
await handler.completePrompt("test prompt", { abortSignal: controller.signal })
1155-
1156-
const registered = addSpy.mock.calls.find(([type]) => type === "abort")?.[1]
1157-
expect(registered).toBeDefined()
1158-
expect(removeSpy).toHaveBeenCalledWith("abort", registered)
1159-
} finally {
1160-
addSpy.mockRestore()
1161-
removeSpy.mockRestore()
1162-
}
1163-
})
1164-
1165-
it("detaches the abort listener from the caller's signal when the completion fails", async () => {
1166-
// Failure is an exit path too: dispose() runs in finally, so a provider error must not
1167-
// leave the listener behind either.
1168-
const handler = new RequestyHandler(mockOptions)
1169-
const controller = new AbortController()
1170-
const addSpy = vitest.spyOn(controller.signal, "addEventListener")
1171-
const removeSpy = vitest.spyOn(controller.signal, "removeEventListener")
1172-
mockCreate.mockRejectedValueOnce(new Error("API Error"))
1173-
1174-
try {
1175-
await expect(handler.completePrompt("test prompt", { abortSignal: controller.signal })).rejects.toThrow(
1176-
"API Error",
1177-
)
1178-
1179-
const registered = addSpy.mock.calls.find(([type]) => type === "abort")?.[1]
1180-
expect(registered).toBeDefined()
1181-
expect(removeSpy).toHaveBeenCalledWith("abort", registered)
1182-
} finally {
1183-
addSpy.mockRestore()
1184-
removeSpy.mockRestore()
1185-
}
1186-
})
1184+
it.each([
1185+
{
1186+
outcome: "completes",
1187+
expectRejection: undefined as string | undefined,
1188+
barrier: undefined as Promise<void> | undefined,
1189+
start: (controller: AbortController) => {
1190+
mockCreate.mockResolvedValueOnce({ choices: [{ message: { content: "response" } }] })
1191+
return new RequestyHandler(mockOptions).completePrompt("test prompt", {
1192+
abortSignal: controller.signal,
1193+
})
1194+
},
1195+
},
1196+
{
1197+
outcome: "fails",
1198+
expectRejection: "API Error",
1199+
barrier: undefined as Promise<void> | undefined,
1200+
start: (controller: AbortController) => {
1201+
mockCreate.mockRejectedValueOnce(new Error("API Error"))
1202+
return new RequestyHandler(mockOptions).completePrompt("test prompt", {
1203+
abortSignal: controller.signal,
1204+
})
1205+
},
1206+
},
1207+
{
1208+
// Aborting mid-flight is the third exit path through finally: the SDK-shaped mock
1209+
// rejects once the signal aborts, so dispose() must detach on that path too.
1210+
outcome: "aborts mid-flight",
1211+
expectRejection: "aborted",
1212+
start: (controller: AbortController) => {
1213+
let notifyStarted!: () => void
1214+
const barrier = new Promise<void>((resolve) => {
1215+
notifyStarted = resolve
1216+
})
1217+
mockCreate.mockImplementationOnce(async (_params: unknown, opts?: { signal?: AbortSignal }) => {
1218+
notifyStarted()
1219+
await new Promise<void>((resolve) =>
1220+
opts?.signal?.addEventListener("abort", () => resolve(), { once: true }),
1221+
)
1222+
const abortError = new Error("The user aborted a request")
1223+
abortError.name = "AbortError"
1224+
throw abortError
1225+
})
1226+
return {
1227+
pending: new RequestyHandler(mockOptions).completePrompt("test prompt", {
1228+
abortSignal: controller.signal,
1229+
}),
1230+
barrier,
1231+
}
1232+
},
1233+
},
1234+
] as const)(
1235+
"detaches the abort listener from the caller's signal when the completion $outcome",
1236+
async (item) => {
1237+
// The bridge registers its listener on the caller's long-lived signal, so every exit
1238+
// path - resolve, provider error and abort - must detach it: otherwise one listener plus
1239+
// one per-request controller is retained for every request on a reused signal. The
1240+
// abort case adds nothing the two originals asserted about detaching; it pins that the
1241+
// finally dispose also runs when the request ends by cancellation rather than by settling.
1242+
const controller = new AbortController()
1243+
const addSpy = vitest.spyOn(controller.signal, "addEventListener")
1244+
const removeSpy = vitest.spyOn(controller.signal, "removeEventListener")
1245+
const started = item.start(controller)
1246+
const pending = "pending" in started ? started.pending : started
1247+
const barrier = "pending" in started ? started.barrier : item.barrier
1248+
void pending.catch(() => {})
1249+
try {
1250+
if (barrier) {
1251+
await withSettleGuard(barrier)
1252+
controller.abort()
1253+
}
1254+
if (item.expectRejection) {
1255+
await expect(pending).rejects.toThrow(item.expectRejection)
1256+
} else {
1257+
await pending
1258+
}
1259+
const registered = addSpy.mock.calls.find(([type]) => type === "abort")?.[1]
1260+
expect(registered).toBeDefined()
1261+
expect(removeSpy).toHaveBeenCalledWith("abort", registered)
1262+
} finally {
1263+
addSpy.mockRestore()
1264+
removeSpy.mockRestore()
1265+
}
1266+
},
1267+
)
11871268

11881269
it("does not leave the SDK's abort listener on the caller's signal after a completed request", async () => {
11891270
// Emulate [email protected]: its request path attaches an abort listener to the signal it is

0 commit comments

Comments
 (0)