feat: add frontend API timeout and cancellation support - #286
feat: add frontend API timeout and cancellation support#286SHAURYAKSHARMA24 wants to merge 1 commit into
Conversation
Signed-off-by: Shaurya K Sharma <shauryaksharma24@gmail.com>
TFT444
left a comment
There was a problem hiding this comment.
Clean, scoped fix with no unrelated changes pulled in. CVE and Dependabot alert are properly referenced and the updated lockfile looks correct. Good to go. Approving.
@ritiksah141 @parthrohit22 please have look into that thanks
TFT444
left a comment
There was a problem hiding this comment.
Went through the abort logic carefully. The abortSource preference ordering handles the caller-vs-timeout race correctly and the inFlight guard prevents stale state on rapid re-renders. Tests are focused and meaningful. Approving.
parthrohit22
left a comment
There was a problem hiding this comment.
The core apiFetch timeout/abort implementation is genuinely well-built: clearTimeout and the caller-signal listener removal both run unconditionally in a finally covering every exit path (success, HTTP error, network error, timeout, cancellation, JSON-parse error), the timeout-vs-caller-abort race is handled with a first-writer-wins guard so simultaneous timeout+cancel doesn't produce ambiguous error typing, and the test suite genuinely simulates a hanging request via a fake-timer harness rather than just asserting on mocked returns (it also asserts zero pending timers and zero leaked listeners after completion, which is the part most implementations skip). All 11 tests in api.test.mjs pass, and every existing caller of the touched wrapper functions still works with the new optional options param.
But alongside the new apiFetch, three existing wrapper functions (getCVESummary, getPlaybook, getScan) had their catch blocks narrowed from "catch anything, fall back gracefully" to "only catch ApiHttpError, rethrow everything else" - and two of those have real call sites that were never updated for the new thrown error types, which is a functional regression in exactly the scenario (long-running operation, imperfect network) this PR is meant to make more robust. Left inline comments on the two consequential ones.
Suggested fix direction: either broaden the catch back to cover transient failures (e.g. ApiHttpError | ApiNetworkError, letting only ApiCancellationError propagate since that's genuinely caller-initiated) or update the Header.jsx poll loop and DetailedScan.jsx's selectFinding to handle the new error types explicitly. Also worth noting for later: no component actually passes an AbortController/signal into any api.* call yet, so while apiFetch now correctly composes a caller signal with its internal timeout, nothing aborts in-flight requests on unmount/re-trigger today - the race condition the PR title references isn't fixed at any call site yet, just made possible. Not blocking, just flagging so it isn't mistaken for done.
| getScan: async (scanId, options = {}) => { | ||
| try { return await apiFetch(`/scans/${scanId}`, options); } | ||
| catch (err) { | ||
| if (!(err instanceof ApiHttpError)) throw err; |
There was a problem hiding this comment.
This used to be a bare catch { ... } that fell back to listing scans on any failure. Now it only falls back on ApiHttpError and rethrows everything else - including the new ApiTimeoutError/ApiNetworkError.
Header.jsx's executeScan polls this in a loop (for (let i = 0; i < 75; i++) { ...; const scan = await api.getScan(scanId); ... }, ~5 minutes at 4s intervals) with a single try/catch around the whole loop. A single transient network blip or timeout on any one poll now throws straight out of the loop and ends polling entirely - the user gets a "Scan failed" toast even though the backend scan is still running to completion. Before this PR there was no timeout and any transient error fell back to /scans and let the loop continue.
Given getScan doesn't override timeoutMs, it also now inherits the new default 30s timeout per call, so this isn't just a network-blip edge case - a single slow response during the 5-minute poll is enough to trigger it.
| catch (err) { | ||
| if (err instanceof ApiHttpError) { | ||
| return { portalSteps: [], cliCommands: [], validationSteps: [], references: [] }; | ||
| } |
There was a problem hiding this comment.
Same narrowing as getScan below. DetailedScan.jsx's selectFinding calls await api.getPlaybook(f.id) with no try/catch, on both mount and click, so a timeout/network error here becomes an unhandled promise rejection with no fallback UI - actually worse than the pre-PR behavior (which returned the empty-arrays fallback) for exactly the failure modes this PR targets.
What does this PR do?
Adds a shared 30-second default timeout and caller cancellation support to the frontend API client without introducing automatic retries.
The request layer now composes an internal
AbortControllerwith an optional caller-providedAbortSignal, clears timeout handles and caller listeners infinally, and accepts a per-calltimeoutMsoverride (ornullto let the caller manage cancellation alone). Public API wrapper methods forward these request options, including scan operations.Timeouts, caller cancellation, HTTP responses, and network failures are represented by distinct exported error classes and codes. Existing optional-data fallbacks still handle HTTP failures, while transport cancellation and timeout errors remain observable to callers. The scan lookup fallback no longer launches a second request after cancellation or timeout.
No automatic retry policy was added. In particular, state-changing requests such as scan triggers are never silently repeated.
Type of change
Rule details (if applicable)
Not applicable.
Testing
Executed from
frontend/:node src/utils/api.test.mjs— 11 request lifecycle tests passednode src/utils/aiApi.test.mjs— 9 existing AI settings tests passednpm run test:a11y— passednpm run test:i18n— passednpm run lint— passed with zero warningsnpm run build— passedFocused coverage includes success, non-2xx errors, network failures, response parsing, timeouts, active and already-aborted caller signals, listener and timer cleanup, operation-specific timeout overrides, completed requests, and explicit timeout disablement.
Related issue
Closes #282
Checklist
Signed-off-bytrailer (git commit -s; seedocs/dco.md)