Repository navigation
Handle Data API 429/503 with backoff, jitter and a user-visible busy state - #1465
Merged
Merged
Conversation
…9 spec
Revises the original 429 draft against BV-BRC-API/Docs/CLIENT-429.md, which
is normative for all API clients. The draft disagreed with it on four points,
each in the direction of retrying harder than the server permits: 4 retries
vs 3, no jitter, no 503 handling, no 504/400 exclusion. Jitter was the most
consequential omission -- an unjittered Retry-After returns every shed client
at the same instant, reproducing the burst the spec exists to prevent.
Also incorporates spec section 3a (added after the first revision), which
scopes the 3-retry cap to interactive clients and has unattended clients cap
the delay rather than the attempt count. The website is interactive, so the
cap stands here, but retryRequest takes mode as a parameter from the outset.
Key design finding: there is a single interception point for most data-API
traffic. dojo/request resolves through request/default, which honours
has('config-requestProvider') -- registered from the dojoConfig in
views/javascript.ejs. 208 of the p3 imports are dojo/request (vs 10 direct
dojo/request/xhr), so one provider registration covers ~200 widget call
sites without editing any of them.
Prototyped and verified end to end with headless Chrome, in both dev and
production (built-layer) mode:
- provider is invoked; 200 grid rows render; dual .response promise survives
- CDP initiator stacks confirm exactly three transports reach the data API,
and the two the provider does not cover are the two the plan already
carves out: P3JsonRest (dojo/_base/xhr rawXhrPost) and BVBRCClient (fetch)
- the Dojo build leaves has('config-requestProvider') as a runtime lookup
while folding has('host-browser') to a literal, so the hook survives
Corrects two things the draft got wrong about the code: wrapping P3JsonRest
naively breaks results.total on the success path (it reaches into the
Deferred's ioArgs), and bvbrc_client dist/ is a generated browserify bundle
that must not be hand-edited. Documents downloads as unreachable by any
wrapper (form.submit/window.open) and the rows:25000 sites still uncapped
after JIRA-4420, which are all heavy-classified shed candidates.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Implements the retry core and transport interception from
PLAN-http-429-throttle-handling.md sections 1 and 2, per the Data API's
client contract in BV-BRC-API/Docs/CLIENT-429.md.
retryRequest(requestFn, options):
- honours Retry-After; never substitutes a shorter delay
- jitters every wait by up to +30%, so simultaneously-shed clients do not
all return at the same instant and reproduce the burst
- interactive mode (the default, and what the spec assigns the website)
gives up after 3 retries with a truthful busy message; unattended mode
caps the delay at a 300s ceiling and keeps going (spec 3a)
- retries 429 and 503 only; 504 and 400 reject unchanged on the first
attempt, since retrying either is pure waste
- floors every delay at 1s. The API floors Retry-After at 1 for exactly
this reason, but a client must not depend on the server getting it right
-- Retry-After: 0 would otherwise produce the tight loop the spec exists
to prevent. Caught by test, not by inspection.
- publishes /throttle/{active,retrying,resolved,failed} for the banner,
with exactly one closing event per opening one on every exit path
including cancel, so the counter cannot drift and pin the banner open
- cancelling clears the pending timer AND aborts the in-flight request;
a retry fired after cancel is the concurrency escalation the spec bans
throttledRequest is registered as dojoConfig.requestProvider, so it becomes
what every `dojo/request` import resolves to -- 208 of the p3 imports, and
~200 call sites, covered without editing any of them. Only Data API URLs are
retried; workspace/app-service/Shock/user-service traffic falls through to
./xhr untouched.
One subtlety worth keeping: dojo/Deferred freezes its promise
(Deferred.js:305), so assigning .response to it is a silent no-op. The
dual-promise contract (request/util.js:146) is rebuilt by copying the
methods onto a plain wrapper, the way dojo does it internally. Verified live:
hasThen/hasResponse/hasCancel all true, response.status 200, 0 page errors.
22 unit tests; full suite 70 passed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Completes the interactive-client half of PLAN-http-429-throttle-handling.md:
sections 2a and 5, plus the dojoConfig registration that activates section 2.
Shipping these together is deliberate. Testing the provider alone against
injected 429s showed the grid rendering 0 rows: the facet and taxonomy
queries retried and recovered, while the POST that actually fetches the rows
was shed once and never retried, because P3JsonRest uses dojo.rawXhrPost from
legacy dojo/_base/xhr and so bypasses the dojo/request provider entirely.
That is the single outcome the API's client contract calls worst -- a user
who sees zero rows believes their data is missing -- so the provider alone
would have made throttling *less* visible, not more.
P3JsonRest (2a): wraps its rawXhrPost in retryRequest. Two hazards, both on
the success path with no 429 involved, so both would have been silent
regressions:
- results.total used to read the xhr off `results.ioArgs`, a property of
the specific Deferred rawXhrPost returns. retryRequest hands back a
different promise, so that back-channel is gone; the winning attempt's
xhr is captured explicitly instead.
- dojo/Deferred freezes its promise (Deferred.js:305), so `results.total =`
on it is a silent no-op and every grid would lose its row count. Fixed by
delegating first, which is what QueryResults itself does to a promise for
the same reason. then()/cancel() survive because both are closures, and
Pagination.js:419 feature-detects results.cancel.
ThrottleBanner (5): subscribes to the /throttle/* topics and shows a live
countdown while requests back off, then the spec's exhaustion message if they
run out. Counts down rather than merely asserting activity, because a silent
multi-second pause reads as a hang. Fixed-position so showing and hiding
never reflows the page, and pointer-events:none so it cannot block
navigation.
Verified live against injected 429s (Retry-After honoured, jittered, capped):
429 -> 429 -> 200 gaps 3686, 4002 ms POST genome/ (grid rows)
429 -> 429 -> 200 gaps 3348, 4393 ms data/taxon_category/
grid rows rendered: 200
banner: "Retrying in 5 seconds..." counting down to 1, then hidden on
recovery; "The server is currently busy. Please try again in a few
minutes." and stays put when retries are exhausted.
cancel mid-backoff: 0 further requests on the wire, counter balanced.
no throttling at all: banner element never created, results.total
10349515 matching the pager, 0 page errors.
Known gap, unchanged: BVBRCClient (fetch) still throws an unretried 429 --
section 3, next. Downloads via form.submit()/window.open() remain unreachable
by any wrapper (section 4).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit enabled the feature outright by adding requestProvider
to the dojoConfig. That put a rebuilt promise on the path of every
dojo/request call in the app on the strength of testing two pages
(/view/GenomeList/ genomes and features tabs). The retry behaviour is well
covered, but "does routing all dojo/request traffic through a wrapper break
anything that consumes a request promise unusually" is a different question,
and BLAST/Homology, Compare Regions, JBrowse, the heatmaps, workspace
browsing and job submission are all unexercised. This makes that a
deployment decision instead of a code one.
config.js enableThrottleRetry: false (default)
app.js -> applicationOptions -> window.App
p3-web.conf.sample documented
Gated in two places, not one. P3JsonRest calls retryRequest directly --
it uses dojo/_base/xhr and never sees the provider -- so a flag covering
only the dojoConfig registration would leave the grid's own query retrying
while the facets around it did not. That half-enabled state is exactly what
produced the silently-empty grid this branch already had to fix once.
Off means absent, not inert:
- retryRequest returns the caller's own promise object untouched, so
nothing can regress in the promise contract
- the registry matcher itself tests the flag, so a non-matching request
never enters the handler and takes the stock ./xhr path with dojo's own
promise rather than the rebuilt one
Verified live with 429s in flight. Flag off: 4 shed requests, zero retries,
zero throttle topics, no banner -- byte-identical to pre-feature behaviour.
Flag on via p3-web.conf: 429 -> 429 -> 200 on all three retriable requests,
gaps 3375-4552ms, grid renders.
5 new unit tests cover the gate including the App-absent and flag-absent
cases; 27 in the file, 75 in the suite.
Known deviations in the wrapped promise, both believed benign but untested
beyond the two pages: it is not an instanceof dojo/promise/Promise (only
when.js checks, and its non-native branch is the correct path for a foreign
thenable), and it has no .progress() (nothing in dgrid or dojo/store calls
it, and XHR has no progress channel). then/otherwise/always/cancel/response
and error .status are all present and verified.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nconf parses p3-web.conf as JSON (config.js:62), so copying the sample to
p3-web.conf -- what README and CLAUDE.md both tell you to do -- crashed the
server at startup:
Error: Error parsing your configuration file: [...p3-web.conf]:
Expected property name or '}' in JSON at position 4 (line 2 column 3)
at File.loadSync (nconf/lib/nconf/stores/file.js:170:11)
Single-quoted keys and values are not JSON. This has been broken since
e0f8a55 ("cleanup base js", 2018-05-14), which reformatted the file to
JS object-literal style -- valid JavaScript, unparseable by nconf and jq.
Every version before that was correctly double-quoted.
My previous commit made it worse by adding // comments, which are also not
JSON. Those are dropped; the enableThrottleRetry explanation lives in
config.js next to the default, where it does not break the parse.
Verified: jq and JSON.parse both accept it, and the server boots with the
sample copied over p3-web.conf.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
olsonanl
added a commit
that referenced
this pull request
Oct 17, 2026
public/js/bvbrc_js_client was a symlink to node_modules/bvbrc_js_client, installed from github:bv-brc/bvbrc_js_client. Git tracked the symlink and nothing inside it, so the client's source was not editable from this repo at all -- any change would live in node_modules and be overwritten by the next npm install. That blocks adding 429 retry handling to its fetch calls, which is the one remaining uncovered Data API transport (PR #1465 covered dojo/request and P3JsonRest). GenomeAlignment.js:128,146 queries through it with limit:25000, heavy enough to be a prime shed candidate under admission control, and the client discards the status when it throws so the failure cannot even raise the throttle banner. Vendoring rather than patching upstream, because upstream is dormant -- last commit 2021-08-10, and the pinned SHA 3847d9c5 was also HEAD of main. BV-BRC-Web is its only consumer. This copy is now the source of truth; upstream can be resynced later if another consumer appears. No behaviour change. The bundle is byte-identical (sha256 9aed465e7990...), verified both on disk and as served over HTTP, and views/javascript.ejs already loads it by path so nothing in the load path moves. In-browser check after the switch: BVBRCClient global present, p3/DataAPI singleton functional, a live query returning, 200 grid rows, 0 exceptions. Also removes the git+ssh dependency from the install path. One trap worth noting: .gitignore has bare `src/` and `test/` rules (lines 21-22) which are unanchored and so match at any depth. They silently excluded the vendored package's own source -- the first `git add` staged only 7 of 30 files, with dist/ but no src/. Re-included explicitly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
olsonanl
added a commit
that referenced
this pull request
Oct 17, 2026
Closes the last uncovered Data API transport. PR #1465 covered dojo/request (via the requestProvider) and P3JsonRest; this client uses fetch and so saw neither. Its failure mode was the worst of the three: it threw Error(response.statusText), discarding the status, so a shed request could not even reach the throttle banner -- the user got a broken panel with no explanation. GenomeAlignment.js:128,146 queries through it with limit:25000, which classifies as heavy under admission control, making it the most likely thing on the site to be shed and previously the least able to cope. MSA, PhylogeneticTree, MetaCATS, ViralGenomeTree, SeqComparison, Homology, ProteinFamilies and PathwayService use the same path at lighter limits. src/retry.js implements the contract (BV-BRC-API Docs/CLIENT-429.md): honour Retry-After, jitter up to +30%, cap at 3 retries, 429 and 503 only, floor every delay at 1s. All four fetch call sites in src/index.js now route through it. It publishes the same /throttle/* topics as p3/util/retryRequest and respects the same enableThrottleRetry flag, so the banner and the deployment switch cover this transport too. This is a deliberate duplicate of p3/util/retryRequest.js rather than an import: the package is standalone with no AMD loader. Keep the two in step. httpError() replaces the throws that discarded the status. That discard is precisely why this could not be fixed from outside the client, so it is part of the fix rather than a tidy-up. Build note: the dependency range `node-fetch: >=2.6.1` resolved to 3.x, which is ESM-only and unparseable by browserify. The build failed -- and because it pipes through derequire, the failure truncated dist/bvbrc_client.js to zero bytes rather than leaving the old bundle intact. Pinned to ^2.7.0 (CJS) and committed the lockfile so the bundle is reproducible. Same class of problem as the prod Node 22 ESM breakage in CLAUDE.md. Verified live with injected 429s, flag on. All four transports now recover where previously three did: 429 -> 429 -> 200 gaps 3012, 4743 ms POST genome/ (P3JsonRest) 429 -> 429 -> 200 gaps 3120, 4052 ms genome/?select(...) (this client) 429 -> 429 -> 200 gaps 3014, 5460 ms data/taxon_category/ (provider) 429 -> 429 -> 200 gaps 3099, 11139ms genome/?facet(...) (provider) 200 grid rows, banner counts down and hides on recovery, and the `Error: Too Many Requests` exception present in every previous run is gone. 15 mocha tests in the package; 75 jest tests in the host repo still pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
olsonanl
added a commit
to olsonanl/bvbrc_website
that referenced
this pull request
Oct 17, 2026
Brings 182 commits of alpha onto the download-wizard branch, including the 429/503 throttle handling (PRs BV-BRC#1465, BV-BRC#1467) that the download work needs to build on: p3/util/retryRequest.js, the dojo/request throttle provider, ThrottleBanner, and the vendored bvbrc_js_client. Merging before adding the download-side 429 work, so that work is written against the code it will actually ship with rather than a two-month-old base. Three conflicts, all additive on both sides: p3.css auto-merged; both @imports (ThrottleBanner, UnifiedDownload) present. GridContainer.js both sides added AMD dependencies -- alpha's authHeaders (9 uses) and the branch's QueryDescriptor / UnifiedDownloadWizard / SaveSearchDialog (10 uses). Took the union. Verified the dependency array and factory parameter list are both 38 entries and correctly aligned, since a silent off-by-one here would shift every argument. Lint count is identical before and after the merge (6 pre-existing errors), so the resolution added none. CLAUDE.md disjoint documentation sections; kept all three. 75 tests pass. Vendored bvbrc_js_client survived as a real directory rather than reverting to the old node_modules symlink, and the npm dependency stays removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
olsonanl
added a commit
to olsonanl/bvbrc_website
that referenced
this pull request
Oct 18, 2026
Three additions, all from work that shipped in BV-BRC#1465/BV-BRC#1467 and the vendoring in this branch's merge base. **HTTP 429/503 throttling.** What the contract actually requires (honour Retry-After, jitter, 3 retries interactive / 300s ceiling unattended, 429+503 only, floor at 1s) and -- the part that is not guessable from the code -- that there is **no single request funnel**. Three transports need three different treatments, and importing `dojo/request/xhr` directly opts out of all of them. Includes the grep to find callers that have done so; four currently have, two of them in the download wizard. **Two freeze traps when wrapping a dojo promise.** `dojo/Deferred` freezes its promise, so `results.total = ...` is a silent no-op; and `dojo/request` returns a dual promise whose `.response` sub-promise dojo/dgrid reads even though no p3 call site does. Both break the *success* path with no error, so they look nothing like the feature being added. Documented with the fix (`lang.delegate` first) and the advice to test the no-error path first. **Never render a shed request as an empty result set.** Measured: a grid whose query is shed with no banner renders `1 - 1 of 0 results`, indistinguishable from a legitimate empty result. Retry logic without the banner makes throttling less visible, not more. **bvbrc_js_client is vendored.** It was a node_modules symlink, so its source was not editable from this repo at all. Includes the rebuild command and the two traps found vendoring it: node-fetch must stay pinned to v2 (3.x is ESM-only, breaks browserify, and truncates dist/ to zero bytes rather than failing cleanly), and .gitignore's bare `src/`/`test/` rules are unanchored and silently swallowed the vendored source. **An unmerged release PR blocks every later release run.** release.yml derives the next version from alpha's package.json, which only advances on merge, so an open release PR makes every run recompute the same number and die on `! [rejected] ... (fetch first)` -- a message that says nothing about the actual cause. Cost a release cycle on 2026-10-17. Every file:line reference verified against this tree. The one count that drifts with each commit (dojo/request imports) is phrased as approximate with a grep rather than a number that rots. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
Implements the client half of the Data API's
429 Too Many Requestscontract(BV-BRC-API
Docs/CLIENT-429.md).The API currently ships with admission control disabled specifically because this
contract is not implemented in clients. This is what unblocks enabling it.
The governing rule is not "retry on 429" — it is back off by the amount the server asked
for, and give up rather than hammer. A client that retries a shed request badly makes the
overload worse at the moment the system has least capacity to absorb it; retry
amplification of exactly this kind caused the 2026-06-05 coordinator crash.
Ships behind
enableThrottleRetry, defaultfalse. Merging changes nothing until adeployment opts in — see Enabling below.
What's here
util/retryRequest.jsRetry-After, jitter, caps, 429/503 onlyutil/throttledRequest.jsdojo/requesttransport providerwidget/ThrottleBanner.js+ cssstore/P3JsonRest.jsp3-web.conf.samplePer the spec: honours
Retry-Afterand never substitutes a shorter delay; jittersevery wait by up to +30% so simultaneously-shed clients don't all return at the same
instant; caps interactive retries at 3; treats 503 identically and 504/400 as
non-retriable; floors every delay at 1s so
Retry-After: 0can't produce a tight loop.An
unattendedmode (spec §3a — cap the delay at a 300s ceiling, keep going) isimplemented and tested but unused: the spec classifies the website as
interactive. Itexists so downloads can use it later without a rewrite.
One registration covers ~200 call sites
dojo/requestresolves throughrequest/default, which honourshas('config-requestProvider')— set from thedojoConfiginviews/javascript.ejs. 208of the
p3/imports aredojo/request(vs 10 directdojo/request/xhr), so a singleprovider registration covers the widget layer with no call-site edits. Only Data API
URLs are matched; workspace/app-service/Shock/user-service traffic falls through to
./xhruntouched.Two transports bypass it and are handled separately:
P3JsonRest(legacydojo/_base/xhr) is covered here;BVBRCClient(fetch) is not — see Known gaps.Why the banner is not optional
Testing the provider alone against injected 429s rendered a grid with 0 rows: the facet
and taxonomy queries retried and recovered, while the POST that fetches the actual rows was
shed and dropped. That is the single outcome the spec calls worst — "a user who sees zero
rows will believe their data is missing." Exhaustion renders as
1 - 1 of 0 results,indistinguishable from a legitimate empty result.
So
P3JsonRestcoverage and the banner ship together with the provider. Any subset makesthrottling less visible, not more.
Verified live
Against a local server with injected 429s (
Retry-After: 3):"Retrying in 5 seconds…"counting down, hidden on recovery;"The server is currently busy. Please try again in a few minutes."and persists when exhaustedresults.total10349515 matching thepager, 0 page errors
has('config-requestProvider')asa runtime lookup while folding
has('host-browser')to a literal, so the hook survives75 tests pass (27 for
retryRequest).Two silent regressions caught
Both on the success path with no 429 involved — they'd have broken every grid in normal
operation:
results.totalread the xhr offresults.ioArgs, a property of the specific DeferredrawXhrPostreturns.retryRequesthands back a different promise, so thatback-channel vanishes.
dojo/Deferredfreezes its promise (Deferred.js:305), soresults.total = …is asilent no-op. Fixed by delegating first — what
QueryResultsitself does for the samereason.
Enabling
{ "enableThrottleRetry": true }Gated in two places, not one:
P3JsonRestcallsretryRequestdirectly and never sees theprovider, so a flag covering only the registration would leave the grid retrying while its
facets did not — the half-enabled state that produces the empty grid above.
Off means absent, not inert:
retryRequestreturns the caller's own promise objectuntouched, and the registry matcher itself tests the flag, so with the feature off every
request takes the stock
./xhrpath with dojo's own promise rather than the rebuilt one.What reviewers should kick
Retry behaviour is well covered. The open question is different: does routing all
dojo/requesttraffic through a rebuilt promise break anything that consumes a requestpromise unusually?
Live testing covered
/view/GenomeList/genomes + features tabs only. Unexercised:BLAST/Homology, Compare Regions, JBrowse, heatmaps, workspace browsing, job submission,
Copilot. A failure there would likely look like a silently empty panel rather than a console
error.
Two known deviations in the wrapped promise, both believed benign, neither exercised beyond
those two pages:
instanceof dojo/promise/Promise— onlywhen.js:36checks, and its non-nativebranch is the correct path for a foreign thenable
.progress()— nothing in dgrid ordojo/storecalls it, and XHR has no progresschannel
then/otherwise/always/cancel/responseand error.statusare all presentand verified.
Known gaps (deliberate, documented in the plan)
BVBRCClient(fetch) — still throws an unretried 429. Needs a browserify rebuildof the vendored bundle, and the status preserved at the throw site (it currently throws
Error(statusText), discarding the code and headers, so this cannot be fixed fromoutside the client). Next PR.
form.submit()/window.open()hand the response to the browser, so nowrapper can reach them. They are also the most likely to be shed
(
DownloadTooltipDialog.js:106requestslimit(2500000)). Converting to XHR+Blob is thefix and the natural first
unattendedcaller.Drive-by:
p3-web.conf.samplewas not valid JSONnconfparsesp3-web.confas JSON (config.js:62), so copying the sample — what theREADME tells you to do — crashed the server at startup. Single-quoted keys have been there
since
e0f8a55dd(2018-05-14), an autoformat pass that made it valid JavaScript and invalidJSON. Happy to split this out if preferred.
🤖 Generated with Claude Code