fix(tools): keep the forget-memory timeout when a caller passes a signal - #1595
Open
Agnik47 wants to merge 1 commit into
Open
fix(tools): keep the forget-memory timeout when a caller passes a signal#1595Agnik47 wants to merge 1 commit into
Agnik47 wants to merge 1 commit into
Conversation
`forgetMemoryRequest` combined the caller's signal and the 30s abort with `??`, making them mutually exclusive. Passing a cancellation signal removed the timeout, so a hung `DELETE /v4/memories` could wedge the tool call again — the exact condition supermemoryai#1451 set out to remove. There was also no way for a caller to ask for both cancellation and a timeout. Compose the two with `AbortSignal.any` instead of choosing between them. `AbortSignal.any` is available in Node 20.3+, Bun and workerd. No production call site passes `options` today (`ai-sdk.ts` and `openai/tools.ts` both omit it), so this was latent rather than live. The existing test asserted the buggy behaviour (`init.signal` being the caller's own signal), so it is replaced by two tests that pin the composed semantics: aborting the caller aborts the request, and the timeout leg still aborts the request on its own. Both fail against the previous implementation. Fixes supermemoryai#1549
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1549.
The problem
packages/tools/src/shared/forget-memory.tscombined the caller's signal and the 30s abort with??, which makes them mutually exclusive:Pass a cancellation signal and the request becomes unbounded again — exactly the condition #1451 set out to remove. A caller who wants both cancellation and a timeout had no way to express it.
Latent rather than live today: neither production call site (
ai-sdk.ts,openai/tools.ts) passesoptions. It becomes a real hang the first time someone wires up cancellation.The fix
Compose the two signals instead of choosing between them:
AbortSignal.anyis available in Node 20.3+, Bun and workerd. Rootpackage.jsondeclares"node": ">=20", so in theory 20.0–20.2 are below the floor for this API; every maintained 20.x release is well past 20.3. Happy to guard it if you'd rather not rely on that.Tests
The existing case — "uses a caller-provided signal instead of creating a timeout" — asserted the buggy behaviour (
expect(init.signal).toBe(controller.signal)), so it could not survive the fix. It is replaced by two tests that pin the composed semantics:cancels through a caller-provided signal— the request signal is a composite rather than the caller's own, and aborting the caller still aborts the request.keeps the timeout when a caller-provided signal is present—AbortSignal.timeoutis still called with30_000, and firing only the timeout leg aborts the request.Both new tests fail against the previous implementation (verified by reverting the source and keeping the tests):
Verification
vitest run src/tool-operations.test.ts— 15/15 pass (was 14).packages/toolssuite — 91 passed / 30 skipped. The two failing files (src/tools.test.ts,test/claude-memory.test.ts) fail identically on a cleanorigin/maincheckout; they are the pre-existing collection errors tracked in@supermemory/toolsfailscheck-types: 31 errors intest/, including an import of a file that no longer exists #1545, untouched by this change.biome checkclean on both files;tsc --noEmitreports nothing on either file.Unrelated, but noticed nearby
apps/mcp/src/server/client/index.ts:339(getDocuments) has the identicaloptions?.signal ?? AbortSignal.timeout(...)shape. Out of scope here — say the word and I'll open a separate issue or PR for it.