fix(react-query): stop retrying polls the server deterministically rejects - #1601
Open
dawsontoth wants to merge 1 commit into
Open
fix(react-query): stop retrying polls the server deterministically rejects#1601dawsontoth wants to merge 1 commit into
dawsontoth wants to merge 1 commit into
Conversation
…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>
Contributor
There was a problem hiding this comment.
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.
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
kriszyp
approved these changes
Aug 7, 2026
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.
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
retrypredicate treated it as transient and spent the full budget on it.This broadens the retry predicate from 403-only to 403 + 400 and adds
isDeterministicRejectionas the shared predicate.retryUnlessForbidden→retryUnlessRejected(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:
retryDelaygetOrganizationRoleInfo,getChallengeCertificates,getListUsers,getListRoles,getSSHKeygetStatus10_000state.error, the global error toast, and the UI on the first request instead of ~30s laterWhat this deliberately does not do
pollUnlessForbiddenis 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.getChallengeCertificatespolls 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:
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
isDeterministicRejectionunit tests: covers 400/403, asserts 401/404/409/429/5xx/non-HTTP are excluded.retryUnlessRejectedunit tests: never retries 400 or 403; unchangedretry: 3budget for 5xx; still retries 401.QueryClientdriving the realgetStatusQueryOptions: a 400 surfaces on the first request (isError, status 400, one call) rather than after three retries, and the poll timer keeps running.tsc -b,oxlint,dprintclean.🤖 Generated with Claude Code