fix(api): enforce idempotent retries and classified failure handling - #436
Open
Flames4fun wants to merge 2 commits into
Open
fix(api): enforce idempotent retries and classified failure handling#436Flames4fun wants to merge 2 commits into
Flames4fun wants to merge 2 commits into
Conversation
Retry only explicitly idempotent operations for transient failures, preserve abort propagation through response consumption, and expose deterministic attempt and elapsed-time bounds. Cover the status allowlist, jittered backoff, timeout handling, abort races, and non-idempotent exclusions with fake-timer tests.
Route classified API failures through the existing reporter and toast infrastructure, suppress deliberate aborts, and preserve actionable inline states. Document the pre-change audit, defects, retry rationale, elapsed ceiling, coverage, and baseline constraints.
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR implements bounded, idempotency-aware retries and a stable error taxonomy in the shared API client. Errors reach components through the existing
errorReporter/toast path,AbortSignalpropagation is preserved, and deliberate cancellation never becomes user-visible.Closes #433.
Pre-change audit
Previously, GET/HEAD retried every 5xx and network rejection, with two retries after the initial attempt. Other methods, timeouts, malformed responses, 4xx responses, and caller aborts were not retried. HTTP failures used equal jitter, while network failures used fixed 500/1000 ms delays. Abort recognition was limited to fetch-path
DOMExceptionvalues, and timeout/signal cleanup happened before response-body consumption completed.api.test.tsalready covered 400/404, 503 success/exhaustion, GET network failures, POST exclusion, caller abort, timeout classification, abort during backoff, malformed JSON, and jitter. Missing coverage included the status allowlist, body timeout/abort, timeout retries, semantic quote idempotency, the elapsed budget, and UI integration.Alternatives evaluated
POST /quoteopts in, and mutations remain single-attempt. It keeps one auditable policy without assuming HTTP method alone defines safety.The selected design fixes the defects without changing the jitter algorithm, modifying
useAsync, adding e2e infrastructure, or trusting process-local backend deduplication.Retry and security policy
Idempotent operations retry only network failures, client timeouts, and HTTP 408/500/502/503/504. All other responses stop immediately, including 400, 404, 409, 422, 429, 501, and 505.
Why 429 remains non-retryable
429can be temporary, but correct rate-limit recovery requiresRetry-After-aware scheduling. Reusing the client's sub-two-second jitter could send another request before the server permits it and amplify throttling. This change therefore excludes 429 instead of introducing a second scheduling policy without an API contract.503remains retryable because transient service unavailability is part of the existing resilience policy and retries are restricted to idempotent operations. If a 503 includesRetry-After, this client does not yet consume that hint; server-directed scheduling for either status is a separate follow-up. See RFC 6585 section 4 and RFC 9110 section 10.2.3.The allowlist is semantic: 408 permits repeating an incomplete request; 501/505 describe unsupported capability/protocol conditions that immediate retries cannot resolve.
Anchor registration/deactivation and settlement open/execute/cancel remain non-retryable. A lost response therefore cannot trigger a client retry of an operation already applied on another replica, where the process-local idempotency cache offers no protection. Quotes may retry because they do not mutate state.
Retry bound
The count, base values, and equal-jitter algorithm are unchanged: two retries, three fetches maximum, and delays in
[500, 1000)then[1000, 2000)ms. The maximum configured retry budget is3 * perAttemptTimeout + 3000 ms: less than 33 seconds of configured request/backoff time with the default 10-second timeout. It excludes event-loop scheduling delay and is not a hard wall-clock guarantee. Timeout inputs are validated against the browser timer range.Error taxonomy and UI flow
Components can branch on
aborted,timeout,network,not_found,invalid_response,client,server, andunknown.ApiRequestErroradds retryability, attempts, status/code, request ID, and cause.toast.tsowns safe copy;ToastProvider.notifyErrorcomposes it witherrorReporter, reports exhausted operational failures, and suppresses aborts. Route 404s, mutation toasts, and inline quote errors remain distinct; no parallel mechanism was added.Defects found and hardening applied
DOMExceptionerrors.nullback into"Quote failed."; an adversarial regression exposed this, andQuoteFormnow returns to idle without rendering an error.Adversarial coverage also includes abort/timeout races, stalled error bodies, malformed envelopes, JSON/text parity, final metadata, all excluded statuses, and non-idempotent mutation failures.
Verification
npm ci- passednpm test -- src/lib/api.test.ts- passed (98 tests)npm test && npm test && npm test- passed three consecutive runs (61 files, 592 tests each)npm run lint- passed with 0 errors; 9 pre-existing warnings remainnpm run build- passedgit diff --check- passedTypeScript baseline comparison
After
npm ci, the samenpx tsc --noEmitcommand was executed in a clean detached worktree at the PR base and on this branch:feb9b67836d25327ccbb8db8d2c61cb7fe24b6c5a266ff41b3f2a75d670a2110fd60ec6072f95cb1The diagnostic text was identical: four incomplete
useAsyncmocks in unchangedMetricsBar.test.tsxand fiveElement/HTMLElementmismatches in unchangedSettlementTable.test.tsx. The PR adds no TypeScript diagnostic,next buildpasses, and the unrelated baseline remains untouched.Scope
This PR does not alter jitter timing, modify
useAsyncor its tests, add e2e infrastructure, or change backend idempotency behavior.