Skip to content

feat: add frontend API timeout and cancellation support - #286

Open
SHAURYAKSHARMA24 wants to merge 1 commit into
openshield-org:devfrom
SHAURYAKSHARMA24:feat/282-api-timeout-cancellation
Open

feat: add frontend API timeout and cancellation support#286
SHAURYAKSHARMA24 wants to merge 1 commit into
openshield-org:devfrom
SHAURYAKSHARMA24:feat/282-api-timeout-cancellation

Conversation

@SHAURYAKSHARMA24

@SHAURYAKSHARMA24 SHAURYAKSHARMA24 commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

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 AbortController with an optional caller-provided AbortSignal, clears timeout handles and caller listeners in finally, and accepts a per-call timeoutMs override (or null to 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

  • New scan rule
  • Remediation playbook
  • Bug fix
  • Dashboard/front-end work
  • API endpoint
  • Documentation
  • Compliance mapping

Rule details (if applicable)

Not applicable.

Testing

  • Tested against a real Azure free trial subscription (not applicable to the request-layer change)
  • Returns correct JSON output
  • All GitHub CI and security checks pass
  • No hardcoded credentials or secrets

Executed from frontend/:

  • node src/utils/api.test.mjs — 11 request lifecycle tests passed
  • node src/utils/aiApi.test.mjs — 9 existing AI settings tests passed
  • npm run test:a11y — passed
  • npm run test:i18n — passed
  • npm run lint — passed with zero warnings
  • npm run build — passed

Focused 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

  • Every commit includes a DCO Signed-off-by trailer (git commit -s; see docs/dco.md)
  • My code follows the rule template in CONTRIBUTING.md (not applicable; no scan rule)
  • I added or updated the matching CLI playbook (not applicable)
  • I added or updated all four compliance framework mappings (not applicable)
  • I have not committed any real Azure credentials
  • My branch name follows the convention: feat/description

Signed-off-by: Shaurya K Sharma <shauryaksharma24@gmail.com>
@SHAURYAKSHARMA24 SHAURYAKSHARMA24 self-assigned this Aug 18, 2026
@SHAURYAKSHARMA24
SHAURYAKSHARMA24 marked this pull request as ready for review August 18, 2026 16:03
@SHAURYAKSHARMA24
SHAURYAKSHARMA24 requested review from parthrohit22 and removed request for vogonPrayas August 22, 2026 09:39

@TFT444 TFT444 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 TFT444 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 parthrohit22 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread frontend/src/utils/api.js
getScan: async (scanId, options = {}) => {
try { return await apiFetch(`/scans/${scanId}`, options); }
catch (err) {
if (!(err instanceof ApiHttpError)) throw err;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread frontend/src/utils/api.js
catch (err) {
if (err instanceof ApiHttpError) {
return { portalSteps: [], cliCommands: [], validationSteps: [], references: [] };
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: add request timeout and cancellation support to the frontend API client

3 participants