fix: honor timeout when a signal is also passed - #121
Conversation
`getTimeout` only armed the timer when no `signal` was present, and
`betterFetch` handed the caller's signal straight to fetch, so
`{ timeout, signal }` left the request with no deadline at all — it could
hang forever on a dead socket. better-auth's session atom always passes
its own signal, which made `fetchOptions.timeout` a no-op there.
The request now always listens to its own controller. A caller signal is
forwarded onto that controller (`forwardAbortSignal`), preserving its
abort reason, so whichever of the two fires first wins. The listener is
detached in a `finally` once the request settles, so a long-lived signal
reused across requests doesn't accumulate listeners.
Timeout-only behavior is unchanged: it still aborts with the default
`AbortError`, and it is still cleared once the response arrives.
Closes better-auth#120
|
| if (onRequest) { | ||
| const res = await onRequest(context); | ||
| if (typeof res === "object" && res !== null) { | ||
| Object.assign(context, res); |
There was a problem hiding this comment.
Hooks disconnect cancellation controller
When an onRequest hook returns its own signal, Object.assign replaces the internal controller signal before dispatch, but getTimeout still aborts only the internal controller. A hanging fetch then ignores both the configured timeout and the forwarded caller cancellation.
Knowledge Base Used:
There was a problem hiding this comment.
True, but pre-existing: on main the same Object.assign(context, res) already let an onRequest hook replace signal, detaching the timeout the same way. Leaving it out of this PR to keep the diff to the timeout/signal composition; happy to follow up separately if maintainers want hooks prevented from overriding signal.
There was a problem hiding this comment.
That’s fair. This behavior predates the PR, and the change does not introduce a new way for hooks to replace context.signal; it only composes the caller signal with the internal controller before hooks run. Since preventing hooks from overriding cancellation is a separate API/behavior decision, I agree it should be handled separately rather than expanding this fix. Resolving this comment.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
A caller abort or network error rejected `fetch` before `clearTimeout()` ran, leaving the armed deadline holding the event loop until it fired. Clear it in the same `finally` that detaches the forwarded listener. Surfaced by review on better-auth#121. Generated by commit-msg hook Agent: Claude Code Workspace: /tmp/better-fetch-pr Session: 632a37b4-2763-4fa0-b61a-23ea78e273d6 Resume: acr 632a37b4-2763-4fa0-b61a-23ea78e273d6 Claude App: claude://resume?session=632a37b4-2763-4fa0-b61a-23ea78e273d6
Closes #120
What
timeoutis silently ignored whenever the caller also passes asignal, sobetterFetch(url, { timeout: 5000, signal })has no deadline at all and can hang forever on a dead socket.Two things caused it:
getTimeoutonly armed the timer when there was no signal:if (!options?.signal && options?.timeout).betterFetchusedopts.signal ?? controller.signal, so even if the timer had fired, the internal controller was not the signal the request listened to.Why it matters
better-auth's session atom always passes its own
signalto/get-session, which makesfetchOptions: { timeout }oncreateAuthClienta no-op. A hungget-session(laptop sleep, network change) leavesuseSession().isPendingtrue forever, and downstream consumers never reach a signed-out state.retrydoes not help — retry policy is only evaluated after aResponsearrives.How
The request now always listens to its own
controller.signal. A caller-supplied signal is forwarded onto that controller by a newforwardAbortSignalhelper:reason;addEventListener("abort", …, { once: true })and abort the controller with the caller'sreason.The timeout is armed whenever
options.timeoutis set. Whichever fires first wins, and the caller's abort reason is preserved on the way through.forwardAbortSignalreturns a cleanup thatbetterFetchcalls in afinally, so a long-lived signal reused across many requests (exactly the better-auth session-atom case) doesn't accumulate listeners. Thatfinallyis the only reasonfetch.tslooks like a large diff — the body is otherwise untouched, sogit diff -wis one line changed plus the import.Deliberately not using
AbortSignal.any: the package ships areact-nativeentry, and RN runtimes can't be relied on to have it.Behavior preserved
controller.abort(), i.e. the defaultAbortError— this PR does not switch it to aTimeoutErrorDOMException, since that would be a separate behavior change.Tests
Four tests added to
src/test/fetch.test.ts, using acustomFetchImplthat only ever settles by abort:mainand fails withTest timed out in 5000ms;pnpm build && pnpm typecheck && pnpm test— 105 passed (5 files), up from 101.biome checkclean on the touched files.Docs: added a short note plus example to
doc/content/docs/timeout-and-retry.mdx, since the types advertise both options with no documented interaction.