fix(web-search): stop leaking abort listeners in custom provider retry (#611)
`fetchWithRetry` created a fresh `AbortController` per attempt and did:
signal?.addEventListener('abort', () => controller.abort(), { once: true })
The listener was never removed. Consequences:
- On retry, a second listener was attached to the caller's signal,
each closing over a different controller.
- After a successful fetch, the listener remained on the caller's
signal indefinitely, referencing a controller whose work was done.
For a long-lived caller signal this is a slow leak.
- The `{ once: true }` only helps if the signal actually fires — on
non-aborted signals the listener stays attached forever.
Replace the manual controller + timer + listener dance with
`AbortSignal.any([signal, AbortSignal.timeout(ms)])`, which the
codebase already uses elsewhere (see src/services/mcp/xaa.ts). This:
- has no user-code listener to leak,
- gives each attempt a fresh independent timeout,
- cleanly distinguishes caller-initiated abort from timeout via
`signal.aborted` vs `timeoutSignal.aborted` before rewriting the
error as "Custom search timed out after Ns".
Also resets `lastStatus` per attempt so a 5xx on attempt 0 can't leak
into attempt 1's retry decision, and collapses the two redundant
retry branches (`lastStatus >= 500` and `lastStatus === undefined`)
into one.
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -406,20 +406,18 @@ async function fetchWithRetry(url: string, init: RequestInit, signal?: AbortSign
|
|||||||
let lastStatus: number | undefined
|
let lastStatus: number | undefined
|
||||||
|
|
||||||
for (let attempt = 0; attempt < 2; attempt++) {
|
for (let attempt = 0; attempt < 2; attempt++) {
|
||||||
// Create a timeout that races with the external signal
|
// Compose timeout with caller signal via AbortSignal.any so each attempt
|
||||||
const controller = new AbortController()
|
// has a fresh timeout and we don't leak an abort listener on `signal`
|
||||||
const timer = setTimeout(() => controller.abort(), timeoutMs)
|
// (the previous implementation added one per attempt and never removed
|
||||||
|
// it, and the listener kept a reference to a stale AbortController).
|
||||||
// If the external signal is already aborted, forward it
|
const timeoutSignal = AbortSignal.timeout(timeoutMs)
|
||||||
if (signal?.aborted) {
|
const combined = signal
|
||||||
controller.abort()
|
? AbortSignal.any([signal, timeoutSignal])
|
||||||
} else {
|
: timeoutSignal
|
||||||
signal?.addEventListener('abort', () => controller.abort(), { once: true })
|
|
||||||
}
|
|
||||||
|
|
||||||
|
lastStatus = undefined
|
||||||
try {
|
try {
|
||||||
const res = await fetch(url, { ...init, signal: controller.signal })
|
const res = await fetch(url, { ...init, signal: combined })
|
||||||
clearTimeout(timer)
|
|
||||||
|
|
||||||
if (!res.ok) {
|
if (!res.ok) {
|
||||||
lastStatus = res.status
|
lastStatus = res.status
|
||||||
@@ -427,27 +425,21 @@ async function fetchWithRetry(url: string, init: RequestInit, signal?: AbortSign
|
|||||||
}
|
}
|
||||||
return await res.json()
|
return await res.json()
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
clearTimeout(timer)
|
|
||||||
lastErr = err instanceof Error ? err : new Error(String(err))
|
lastErr = err instanceof Error ? err : new Error(String(err))
|
||||||
|
|
||||||
// AbortError from timeout
|
// Caller-initiated abort wins — propagate without retry or rewrite.
|
||||||
if (lastErr.name === 'AbortError' && !signal?.aborted) {
|
if (signal?.aborted) throw lastErr
|
||||||
|
|
||||||
|
// Timeout (TimeoutError on Bun/Node, or AbortError with timeoutSignal aborted).
|
||||||
|
if (timeoutSignal.aborted) {
|
||||||
throw new Error(`Custom search timed out after ${timeoutSec}s`)
|
throw new Error(`Custom search timed out after ${timeoutSec}s`)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Retry on 5xx or network errors only
|
// Retry once on 5xx or network errors; do not retry 4xx.
|
||||||
if (attempt === 0) {
|
if (attempt === 0 && (lastStatus === undefined || lastStatus >= 500)) {
|
||||||
if (lastStatus !== undefined && lastStatus >= 500) {
|
|
||||||
await new Promise(r => setTimeout(r, 500))
|
await new Promise(r => setTimeout(r, 500))
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
if (lastStatus === undefined) {
|
|
||||||
// Network error — retry
|
|
||||||
await new Promise(r => setTimeout(r, 500))
|
|
||||||
continue
|
|
||||||
}
|
|
||||||
// 4xx — don't retry
|
|
||||||
}
|
|
||||||
throw lastErr
|
throw lastErr
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user