Skip to content

fix(api): enforce idempotent retries and classified failure handling - #436

Merged
Jagadeeshftw merged 2 commits into
AnchorNet-Org:mainfrom
Flames4fun:fix/433-api-client-resilience
Aug 29, 2026
Merged

fix(api): enforce idempotent retries and classified failure handling#436
Jagadeeshftw merged 2 commits into
AnchorNet-Org:mainfrom
Flames4fun:fix/433-api-client-resilience

Conversation

@Flames4fun

@Flames4fun Flames4fun commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

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, AbortSignal propagation 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 DOMException values, and timeout/signal cleanup happened before response-body consumption completed.

api.test.ts already 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

  1. Tests/messages only: smallest change, but it preserves incorrect 5xx, network, timeout, and abort behavior.
  2. Policy in each resource client: precise per endpoint, but duplicates transport, timing, cancellation, and parsing logic and invites drift.
  3. Central policy with explicit semantic idempotency (selected): GET/HEAD are safe by default, read-only POST /quote opts 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

429 can be temporary, but correct rate-limit recovery requires Retry-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.

503 remains retryable because transient service unavailability is part of the existing resilience policy and retries are restricted to idempotent operations. If a 503 includes Retry-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 is 3 * 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, and unknown. ApiRequestError adds retryability, attempts, status/code, request ID, and cause. toast.ts owns safe copy; ToastProvider.notifyError composes it with errorReporter, 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

  • Response bodies outlived timeout/cancellation after headers.
  • Idempotent timeouts were not retried; 501/505 were.
  • Network retries bypassed jitter and lacked typed metadata.
  • Abort recognition rejected compatible non-DOMException errors.
  • Invalid timeout inputs broke the documented budget.
  • The first UI integration converted the abort mapper's null back into "Quote failed."; an adversarial regression exposed this, and QuoteForm now 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 - passed
  • npm test -- src/lib/api.test.ts - passed (98 tests)
  • Focused taxonomy/UI run - passed (5 files, 159 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 remain
  • npm run build - passed
  • git diff --check - passed
  • Changed executable lines - 98.84% covered (255/258); focused coverage: 99.19% lines/statements, 96.92% branches, 100% functions
  • Manual UI - anchor/settlement network failures, resource 404, and quote exhaustion rendered distinct states

TypeScript baseline comparison

After npm ci, the same npx tsc --noEmit command was executed in a clean detached worktree at the PR base and on this branch:

Checkout Commit Diagnostics
PR base feb9b67836d25327ccbb8db8d2c61cb7fe24b6c5 9
PR branch a266ff41b3f2a75d670a2110fd60ec6072f95cb1 9
Delta introduced by this PR - 0

The diagnostic text was identical: four incomplete useAsync mocks in unchanged MetricsBar.test.tsx and five Element/HTMLElement mismatches in unchanged SettlementTable.test.tsx. The PR adds no TypeScript diagnostic, next build passes, and the unrelated baseline remains untouched.

Scope

This PR does not alter jitter timing, modify useAsync or its tests, add e2e infrastructure, or change backend idempotency behavior.

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.
@Jagadeeshftw
Jagadeeshftw merged commit 866bd3d into AnchorNet-Org:main Aug 29, 2026
Jagadeeshftw added a commit that referenced this pull request Aug 29, 2026
* Fix CI typecheck blindspot

Closes #425

* test: use semantic settlement table queries (#439)

Co-authored-by: Jagadeeshftw <92681651+Jagadeeshftw@users.noreply.github.com>

* feat(a11y): add jest-axe and jsx-a11y with baseline for SettlementTable and MetricsBar (#438)

* fix(test): type MetricsBar's useAsync mock against the real hook shape (Closes #429) (#437)

MetricsBar.test.tsx mocked useAsync with only { state, refresh }, omitting
reload and mutate, so tsc reported TS2345 at all four call sites and the
component was verified against a contract the real hook never returns.

Replace the hand-written mocks with a typed mockUseAsync factory whose
return type is ReturnType<typeof useAsync>; a future change to the hook's
shape now breaks compilation instead of silently testing a fabricated
interface. No as any, @ts-expect-error or Partial cast used.

MetricsBar itself only consumes state and refresh (reload/mutate are
intentionally unused: the manual refresh must stay silent, which is
refresh's contract), so no behavioural gap was found. Repo-wide tsc error
count drops from 9 to 5; the remaining 5 are the out-of-scope
SettlementTable.test.tsx errors.

Generated with Codebuff 🤖

Co-authored-by: Codebuff <noreply@codebuff.com>

* fix(api): enforce idempotent retries and classified failure handling (#436)

* fix(api): enforce bounded idempotent retries

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.

* fix(ui): integrate API error taxonomy

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.

* test: add tests for lib/wallet.ts (#435)

Closes #<n>.

### Coverage Inventory
* **wallet.ts exports**: `saveAccount`, `loadAccount`, `clearAccount`, `truncateAddress`, `mockAddress`, `STORAGE_KEY`.
* **useWallet.test.ts**: Covers *none* of the above. It only tests the `useWallet` hook to ensure it throws when used outside a `WalletProvider`.

### The Defect (Mocking Strategy & Incorrect Assumptions)
The issue description assumed that `wallet.ts` implements a real wallet integration with error paths like "Provider absent", "User rejection", and "Chain mismatch". However, as documented in `wallet.ts`, this module is purely a mock / stand-in that stores a deterministic fake address in `localStorage`.

Since the module does not integrate with any real wallet provider (like Freighter), these assumed error paths and listeners **do not exist** in `wallet.ts`. I have reported these "untested paths" as `.todo()` in the test suite to formally acknowledge them as defects (i.e. the promised feature doesn't exist).

I have written tests for the actual exposed methods, verifying:
- Successful saving, loading, and clearing of accounts from `localStorage`
- The `loadAccount` error paths (invalid JSON, missing address, regex validation failure)
- Address truncation formatting
- Deterministic seed generation in `mockAddress`

---------

Co-authored-by: daveedAJ <davidadegoke055@gmail.com>
Co-authored-by: Jagadeeshftw <92681651+Jagadeeshftw@users.noreply.github.com>
Co-authored-by: Opulence Chuks <162402876+Opulencechuks@users.noreply.github.com>
Co-authored-by: Annie <168873935+AnnieIj@users.noreply.github.com>
Co-authored-by: Codebuff <noreply@codebuff.com>
Co-authored-by: Luis Carlos Fuentes De Avila <125478683+Flames4fun@users.noreply.github.com>
Co-authored-by: ugoocreates-pixel <ugoocreates@gmail.com>
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.

Audit src/lib/api.ts's retry and abort behaviour — establish which failure modes are classified and which surface as generic errors

2 participants