Repository navigation
fix(sdk): honor caller abort signals in retry.fetch - #5127
umeraamir69 wants to merge 1 commit into
Conversation
Stop treating caller AbortErrors as timeouts, listen to Request signals, and reject immediately when the signal is already aborted.
|
|
Hi @umeraamir69, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (callerSignal) { | ||
| callerSignal.removeEventListener("abort", onCallerAbort); | ||
| } |
There was a problem hiding this comment.
🔴 Response body cancellation stops working
When callers abort after retry.fetch returns, the response body continues downloading instead of rejecting. fetch resolves at headers, before body consumption finishes; onCallerAbort no longer forwards cancellation.
Learn more
The caller's signal controls both the initial fetch and subsequent response-body consumption. Fetch resolves as soon as response headers arrive, so this cleanup runs while the response body can still be streaming. Removing the forwarding listener disconnects the caller from the internal abort controller. The base implementation kept forwarding cancellation after returning the response. Cancellation also stops reaching body reads performed by getRetryStrategyForResponse.
Example: A server sends headers and one chunk immediately, then streams the rest. A caller awaits retry.fetch(url, { signal: controller.signal }), calls controller.abort(), and awaits response.text(). The base rejects with AbortError; this change reads the remaining body successfully.
Recommended fix: Keep caller cancellation connected for the response body's lifetime without restoring permanently retained listeners. Consider native signal composition where supported by the SDK's runtime requirements, or arrange cleanup when the body finishes or is cancelled. Preserve per-attempt timeout cleanup and verify cancellation after headers with a real streaming HTTP server.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (init?.signal) { | ||
| return init.signal; | ||
| } |
There was a problem hiding this comment.
🟡 Null signal override rejects valid requests
With an aborted Request and init.signal: null, resolveCallerAbortSignal inherits the aborted signal and rejects. Native fetch clears the inherited signal, allowing the request.
| if (init?.signal) { | |
| return init.signal; | |
| } | |
| if (init?.signal !== undefined) { | |
| return init.signal ?? undefined; | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| vi.mock("./wait.js", () => ({ | ||
| wait: { | ||
| until: vi.fn(async () => undefined), | ||
| }, | ||
| })); |
There was a problem hiding this comment.
🟡 Abort tests use prohibited mocks
The new abort tests use vi.mock, vi.fn, and fake globalThis.fetch implementations. These violate the explicit testing rules against mocks, stubs, and fakes.
Learn more
The repository forbids mocking dependencies, including replacing functions with fake implementations. The new tests replace the durable wait dependency and the global fetch implementation rather than exercising real HTTP cancellation. The same violation occurs in all three new abort tests.
Example: The mid-request test installs a fake fetch that rejects when its supplied signal fires. It never exercises a real fetch response or its streaming body.
Recommended fix: Replace the fake fetch implementations with a local HTTP test server and exercise real abort signals. Configure short retry delays explicitly where required, and use the real wait implementation rather than a module mock. Keep server shutdown and connection cleanup in test teardown.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (e instanceof FetchErrorWithSpan && e.originalError instanceof Error) { | ||
| if (e.originalError.name === "AbortError") { | ||
| // Caller-initiated aborts must not be retried as timeouts. | ||
| if (callerSignal?.aborted) { | ||
| e.span.end(); | ||
| throw e.originalError; |
There was a problem hiding this comment.
🔍 Custom abort reasons remain wrapped
With AbortSignal.timeout, retry.fetch still rejects with FetchErrorWithSpan, not the signal's TimeoutError. The catch unwraps only AbortError; custom errors remain wrapped. This limitation predates the PR.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Caution Review failedThe pull request is closed.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
Summary
init.signalorRequestinputAbortErroras a timeoutTest plan
init.signal/Requestsignal@trigger.dev/sdk