Skip to content

fix(react-query): stop retrying polls the server deterministically rejects - #1601

Open
dawsontoth wants to merge 1 commit into
stagefrom
claude/rum-stop-retrying-400-polls
Open

fix(react-query): stop retrying polls the server deterministically rejects#1601
dawsontoth wants to merge 1 commit into
stagefrom
claude/rum-stop-retrying-400-polls

Conversation

@dawsontoth

Copy link
Copy Markdown
Contributor

Summary

A 400 is a validator/parser rejection of the request itself — an immediate retry sends identical bytes and can only be rejected identically. React Query's retry predicate treated it as transient and spent the full budget on it.

This broadens the retry predicate from 403-only to 403 + 400 and adds isDeterministicRejection as the shared predicate. retryUnlessForbiddenretryUnlessRejected (the name would otherwise lie); 6 call sites updated mechanically.

Deliberately narrower than "all 4xx": a 401 is resolved by the auth layer re-authenticating, and 404/409 can reflect a resource that is still being created.

What actually changes, per caller

The benefit is not uniform, and the docstring now says so rather than claiming a blanket 4× win:

Caller retryDelay Effect
getOrganizationRoleInfo, getChallengeCertificates, getListUsers, getListRoles, getSSHKey default backoff (1s/2s/4s) a 400ing tick spent 4 requests inside ~7s → 1
getStatus pinned 10_000 request rate unchanged (retries were already interval-spaced). What changes is latency to visibility: the 400 reaches state.error, the global error toast, and the UI on the first request instead of ~30s later

What this deliberately does not do

pollUnlessForbidden is untouched — a 400 still does not halt the poll timer. Halting would freeze a poll whose 400 came from state that is still settling (a certificate challenge mid-provision, an argument derived from a not-yet-loaded resource) until the user remounts or refocuses the tab. getChallengeCertificates polls at 5s during ACME provisioning and is exactly the case that would regress.

Whether a sustained 400 should stop the timer needs a per-call-site judgment and is left open in #1569.

Motivation (Datadog RUM, 2026-08-07)

Surfaced by the automated daily RUM review. Direct-to-instance operations API returned 400 twenty-six times in 24h against a ~1–3/day baseline:

Window 400s Sessions
24h → now 26 3
48h → 24h 2 1
7d → 48h 6 4
14d → 7d 14 6

One page re-issued the same rejected operation every ~60s for ~30 minutes, and a burst of 16 landed in 55s. Fast, uniform rejections (median ~122ms) — a validator saying no, not work failing partway. Full write-up in the companion issue.

Testing

  • isDeterministicRejection unit tests: covers 400/403, asserts 401/404/409/429/5xx/non-HTTP are excluded.
  • retryUnlessRejected unit tests: never retries 400 or 403; unchanged retry: 3 budget for 5xx; still retries 401.
  • Integration test against a real QueryClient driving the real getStatusQueryOptions: a 400 surfaces on the first request (isError, status 400, one call) rather than after three retries, and the poll timer keeps running.
  • Verified the new tests are not vacuous — reverting the predicate to 403-only fails 3 of them, including the integration test.
  • Full suite green: 278 files / 2131 tests. tsc -b, oxlint, dprint clean.

🤖 Generated with Claude Code

…jects

A 400 is a validator/parser rejection of the request itself, so an immediate
retry sends identical bytes and can only be rejected identically. `retry`
treated it as transient and spent the full budget on it.

Broadens the retry predicate from 403-only to 403 + 400 (`retryUnlessRejected`,
renamed from `retryUnlessForbidden`; `isDeterministicRejection` is the new
predicate). Deliberately narrower than "all 4xx" — a 401 is resolved by the auth
layer re-authenticating, and 404/409 can reflect a resource still being created.

Two effects, depending on the caller:

- The five callers on React Query's default exponential backoff (1s/2s/4s) spent
  4 requests inside ~7s on every 400ing tick. Those become 1.
- `getStatus` pins `retryDelay: 10_000`, so its request *rate* was already one
  per interval and nothing is saved there. What changes is latency to
  visibility: the 400 reaches `state.error`, the global error handler, and the UI
  on the first request instead of ~30s later.

`pollUnlessForbidden` is unchanged — a 400 still does NOT halt the poll timer.
Halting would freeze a poll whose 400 came from state that is still settling
(a certificate challenge mid-provision, an argument derived from a not-yet-loaded
resource) until the user remounts or refocuses the tab. Whether a *sustained*
400 should stop the timer is left open in #1569.

Motivated by RUM 2026-08-07: 26 first-party 400s in 24h against a ~1-3/day
baseline, with one page re-issuing the same rejected operation every ~60s for
half an hour.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@dawsontoth
dawsontoth requested a review from a team as a code owner August 7, 2026 07:17

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request renames retryUnlessForbidden to retryUnlessRejected and introduces isDeterministicRejection to immediately halt retries on both 400 and 403 errors. This prevents retry amplification for deterministic client/server rejections and ensures errors are surfaced immediately. All affected queries and test suites have been updated to reflect this change. I have no feedback to provide.

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 54.28% 6431 / 11847
🔵 Statements 54.91% 6926 / 12612
🔵 Functions 45.89% 1561 / 3401
🔵 Branches 47.96% 4423 / 9221
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
src/features/cluster/domains/queries/getChallengeCertificates.ts 0% 0% 0% 0% 16-43
src/features/organization/queries/getOrganizationQuery.ts 0% 0% 0% 0% 11-28
src/features/organization/queries/getOrganizationRoleInfo.ts 0% 100% 0% 0% 18-28
src/features/organization/queries/getOrganizationRoles.ts 0% 100% 0% 0% 7-14
src/integrations/api/instance/auth/getListRoles.ts 0% 100% 0% 0% 7-20
src/integrations/api/instance/auth/getListUsers.ts 0% 100% 0% 0% 7-21
src/integrations/api/instance/ssh/getSSHKey.ts 0% 100% 0% 0% 17-27
src/integrations/api/instance/status/getStatus.ts 100% 100% 100% 100%
src/react-query/pollUnlessForbidden.ts 100% 100% 100% 100%
Generated in workflow #1675 for commit f8e4b6d by the Vitest Coverage Report Action

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.

2 participants