Repository navigation
fix(ai): retry middleware honors abortSignal and skips AbortError - #5342
serhiizghama wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the retry middleware to support AbortSignal, enabling the cancellation of both active requests and backoff delays. It introduces an abortableDelay helper and an isAbortError check, along with tests covering various abort scenarios. Feedback was provided regarding a potential ReferenceError in the abortableDelay implementation if the timeout executes synchronously, recommending that the abort listener be defined before the timer starts to ensure robustness.
| return new Promise((resolve, reject) => { | ||
| const timer = __setTimeout(() => { | ||
| signal?.removeEventListener('abort', onAbort); | ||
| resolve(); | ||
| }, ms); | ||
| const onAbort = () => { | ||
| clearTimeout(timer); | ||
| reject(signal?.reason ?? new Error('Aborted')); | ||
| }; | ||
| if (signal) { | ||
| if (signal.aborted) { | ||
| clearTimeout(timer); | ||
| reject(signal.reason ?? new Error('Aborted')); | ||
| return; | ||
| } | ||
| signal.addEventListener('abort', onAbort, { once: true }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
If __setTimeout is synchronous (as used in some internal tests), onAbort is accessed before it is initialized, leading to a ReferenceError. Reordering the logic to define onAbort and add the listener before starting the timer ensures the code is robust against synchronous execution.
return new Promise((resolve, reject) => {
let timer: NodeJS.Timeout | undefined;
const onAbort = () => {
if (timer !== undefined) clearTimeout(timer);
reject(signal?.reason ?? new Error('Aborted'));
};
signal?.addEventListener('abort', onAbort, { once: true });
timer = __setTimeout(() => {
signal?.removeEventListener('abort', onAbort);
resolve();
}, ms);
});References
- Avoid making changes for minor optimizations if the existing code is functionally correct.
|
I'm fixing this as part of a bigger change to the retry middleware (in the new |
Problem
The `retry` model middleware retries any non-`GenkitError`, including `AbortError`. When a caller passes `abortSignal` to `generateStream`/`generate` and aborts mid-stream, the middleware intercepts the resulting `AbortError` and sleeps through exponential backoff before retrying the model call — completely defeating the abort. The backoff sleep itself is also not signal-aware, so even if a later attempt detects the abort, the user still waits out the full delay.
Fixes #5331.
Solution
The retry function's public return type changes from `ModelMiddleware` to `ModelMiddlewareWithOptions`. Both are valid `ModelMiddlewareArgument` entries, so usage via `use: [retry()]` is unaffected.
Testing
Three new cases in `tests/model/middleware_test.ts`:
All 38 retry/fallback/system-prompt/context tests in the suite pass:
```
pnpm test:single tests/model/middleware_test.ts
tests 38 / pass 38 / fail 0
```
`pnpm check` (tsc) is clean.