From 73cf9f4a9da432b57a808d6a513a1b99c9a79e23 Mon Sep 17 00:00:00 2001 From: addyCooks Date: Wed, 19 Aug 2026 21:29:18 +0530 Subject: [PATCH] fix(tools): keep the forget-memory timeout when a caller passes a signal #1451 bounded `DELETE /v4/memories` with a 30s abort, but the two signals were selected between with `??`: signal: options?.signal ?? AbortSignal.timeout(FETCH_TIMEOUT_MS) so they were mutually exclusive. Any caller who passed a cancellation signal silently dropped the timeout and the request went unbounded again - exactly the hang #1451 set out to remove - and there was no way to ask for both through the API. No production call site passes options today (ai-sdk.ts and openai/tools.ts both omit it), so this was latent. Composes the two with `AbortSignal.any` instead, so a caller signal cancels the request and the 30s ceiling still applies. The timeout signal is built once and reused for the bare case. Verified against a server that never responds: before this change the caller-signal path hangs indefinitely; after it, it rejects with TimeoutError at the deadline, an early caller abort still wins with AbortError, and an already-aborted signal rejects immediately. tool-operations.test.ts replaces the case that asserted the old select-one behaviour with five: the caller signal is composed rather than substituted and its abort reason reaches the request, the timeout still fires while the caller signal stays open, an already-aborted caller signal is forwarded, the bare path still gets the 30s timeout signal itself, and an aborted fetch surfaces to the caller. Each case was checked against mutants of the fix - dropping the timeout from AbortSignal.any, composing with a signal that never fires, reverting to ??, and shortening the timeout - and every mutant fails at least one of them. --- packages/tools/src/shared/forget-memory.ts | 5 +- packages/tools/src/tool-operations.test.ts | 89 +++++++++++++++++++--- 2 files changed, 81 insertions(+), 13 deletions(-) diff --git a/packages/tools/src/shared/forget-memory.ts b/packages/tools/src/shared/forget-memory.ts index 8691c92ac..31b712d44 100644 --- a/packages/tools/src/shared/forget-memory.ts +++ b/packages/tools/src/shared/forget-memory.ts @@ -26,6 +26,7 @@ export async function forgetMemoryRequest( baseUrl: string = DEFAULT_BASE_URL, options?: ForgetMemoryRequestOptions, ): Promise { + const timeoutSignal = AbortSignal.timeout(FETCH_TIMEOUT_MS) const response = await fetch(`${baseUrl}/v4/memories`, { method: "DELETE", headers: { @@ -33,7 +34,9 @@ export async function forgetMemoryRequest( Authorization: `Bearer ${apiKey}`, }, body: JSON.stringify(params), - signal: options?.signal ?? AbortSignal.timeout(FETCH_TIMEOUT_MS), + signal: options?.signal + ? AbortSignal.any([options.signal, timeoutSignal]) + : timeoutSignal, }) if (!response.ok) { diff --git a/packages/tools/src/tool-operations.test.ts b/packages/tools/src/tool-operations.test.ts index 136a19bea..37dadee9e 100644 --- a/packages/tools/src/tool-operations.test.ts +++ b/packages/tools/src/tool-operations.test.ts @@ -1,4 +1,4 @@ -import { beforeEach, describe, expect, it, vi } from "vitest" +import { beforeEach, describe, expect, it, onTestFinished, vi } from "vitest" // Mock the Supermemory SDK (same pattern as claude-memory.test.ts) so tool // executions can be verified deterministically without network access. @@ -88,6 +88,20 @@ describe("memoryForget", () => { return fetchMock } + function forget(options?: { signal?: AbortSignal }) { + return forgetMemoryRequest( + API_KEY, + { containerTag: "user_1", id: "mem_1" }, + undefined, + options, + ) + } + + function signalOf(fetchMock: ReturnType) { + const [, init] = fetchMock.mock.calls[0] as [string, RequestInit] + return init.signal as AbortSignal + } + it("issues DELETE /v4/memories with the forget payload", async () => { const fetchMock = stubFetch() @@ -112,27 +126,78 @@ describe("memoryForget", () => { expect(init.signal).toBeInstanceOf(AbortSignal) }) - it("uses a caller-provided signal instead of creating a timeout", async () => { + it("composes a caller-provided signal with the timeout", async () => { const fetchMock = stubFetch() const controller = new AbortController() - await forgetMemoryRequest( - API_KEY, - { containerTag: "user_1", id: "mem_1" }, - undefined, - { signal: controller.signal }, + await forget({ signal: controller.signal }) + + const signal = signalOf(fetchMock) + expect(signal).not.toBe(controller.signal) + expect(signal.aborted).toBe(false) + + const reason = new Error("caller cancelled") + controller.abort(reason) + expect(signal.aborted).toBe(true) + expect(signal.reason).toBe(reason) + }) + + it("still times out while a caller-provided signal stays open", async () => { + const realTimeout = AbortSignal.timeout.bind(AbortSignal) + const timeout = vi + .spyOn(AbortSignal, "timeout") + .mockImplementation(() => realTimeout(5)) + onTestFinished(() => timeout.mockRestore()) + const fetchMock = stubFetch() + const controller = new AbortController() + + await forget({ signal: controller.signal }) + + expect(timeout).toHaveBeenCalledWith(30_000) + const signal = signalOf(fetchMock) + await vi.waitFor(() => expect(signal.aborted).toBe(true)) + expect((signal.reason as Error).name).toBe("TimeoutError") + expect(controller.signal.aborted).toBe(false) + }) + + it("forwards an already-aborted caller signal", async () => { + const fetchMock = stubFetch() + const reason = new Error("cancelled before dispatch") + + await forget({ signal: AbortSignal.abort(reason) }) + + expect(signalOf(fetchMock).aborted).toBe(true) + expect(signalOf(fetchMock).reason).toBe(reason) + }) + + it("uses the bare timeout when options carry no signal", async () => { + const timeout = vi.spyOn(AbortSignal, "timeout") + onTestFinished(() => timeout.mockRestore()) + const fetchMock = stubFetch() + + await forget({}) + + expect(timeout).toHaveBeenCalledWith(30_000) + expect(signalOf(fetchMock)).toBe(timeout.mock.results[0]?.value) + }) + + it("surfaces an aborted request to the caller", async () => { + vi.stubGlobal( + "fetch", + vi + .fn() + .mockRejectedValue( + new DOMException("The operation timed out", "TimeoutError"), + ), ) - const [, init] = fetchMock.mock.calls[0] as [string, RequestInit] - expect(init.signal).toBe(controller.signal) + await expect(forget()).rejects.toThrow("The operation timed out") }) it("throws a descriptive error on non-2xx responses", async () => { stubFetch(new Response("nope", { status: 401, statusText: "Unauthorized" })) - await expect( - forgetMemoryRequest(API_KEY, { containerTag: "user_1", id: "mem_1" }), - ).rejects.toThrow(/401/) + await expect(forget()).rejects.toThrow(/401/) }) it("ai-sdk tool forgets by content through the endpoint", async () => {