Skip to content

Retry 429/503 in bvbrc_js_client (vendored) - #1467

Merged
olsonanl merged 2 commits into
BV-BRC:alphafrom
olsonanl:feature/client-429-bvbrc-client
Oct 17, 2026
Merged

olsonanl merged 2 commits into
BV-BRC:alphafrom
olsonanl:feature/client-429-bvbrc-client

Conversation

@olsonanl

Copy link
Copy Markdown
Contributor

Summary

Closes the last uncovered Data API transport for
CLIENT-429.md.
#1465 covered dojo/request (via the requestProvider) and P3JsonRest; bvbrc_js_client
uses fetch and saw neither.

Its failure mode was the worst of the three. The client threw
Error(response.statusText), discarding the status, so a shed request couldn't even
reach the throttle banner — the user got a broken panel and no explanation. That discard is
also precisely why this couldn't be fixed from outside the client, so httpError() is part
of the fix rather than a tidy-up.

Still gated behind enableThrottleRetry (default false). Same flag, same
/throttle/* topics, same banner.

Why this matters now

GenomeAlignment.js:128,146 queries through this client with limit: 25000 — heavy enough
to be shed 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.

Two commits, reviewable separately

7ecacade6 — vendor the client, zero behaviour change.
public/js/bvbrc_js_client was a symlink into node_modules; git tracked the symlink
and nothing inside it. The source lived in github:bv-brc/bvbrc_js_client, pinned at
3847d9c5 — which is also HEAD of main, untouched since 2021-08-10, with no other
consumer in the org. So the client simply wasn't editable from this repo.

Bundle is byte-identical (sha256 9aed465e…), verified on disk and as served over
HTTP. views/javascript.ejs already loads it by path, so nothing in the load path moves.
Also drops a git+ssh:// dependency from the install path.

This copy is now the source of truth; upstream can be resynced later if another consumer
appears.

0ee5f8671 — the actual 429 handling.
src/retry.js: honour Retry-After, jitter up to +30%, cap at 3 retries, 429/503 only,
floor every delay at 1s. All four fetch call sites route through it.

Verified live

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)  <- was failing
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.

Two traps found on the way

The build was broken, and failed destructively. node-fetch: ">=2.6.1" resolved to
3.x, which is ESM-only and unparseable by browserify. Because the build pipes through
derequire, the failure truncated dist/bvbrc_client.js to zero bytes rather than
leaving the previous bundle intact. Pinned to ^2.7.0 (CJS) and committed the lockfile so
the bundle is reproducible. Same class as the prod Node 22 ESM breakage in CLAUDE.md.

.gitignore nearly swallowed the vendored source. The bare src/ and test/ rules
(lines 21–22) are unanchored and match at any depth — the first git add staged 7 of 30
files, with dist/ but no src/. Re-included explicitly. Worth knowing before vendoring
anything else.

What reviewers should kick

src/retry.js is a deliberate duplicate of p3/util/retryRequest.js. The package is
standalone with no AMD loader, so it can't import it. Two implementations of the same
contract will drift, and I'd rather that be an explicit decision than something discovered
later. Noted in both files. If you'd prefer a different structure — build-time inlining, or
collapsing the client into a thin p3/ shim — say so.

Also unexercised beyond /view/GenomeList/: the pages that actually use this client
(Genome Alignment, MSA, trees) weren't driven under throttling, only the shared
GenomeList.js:49 call. The retry path is identical, but the consuming code is not.

Remaining gap

Downloads (§4) — form.submit() / window.open() hand the response to the browser, so
no wrapper can reach them, and they're heavy (DownloadTooltipDialog.js:106 requests
limit(2500000)). Converting to XHR+Blob is the fix and the natural first unattended
caller. Not in this PR.

🤖 Generated with Claude Code

olsonanl and others added 2 commits October 16, 2026 20:05
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 BV-BRC#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>
Closes the last uncovered Data API transport. PR BV-BRC#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
olsonanl merged commit 443ce9b into BV-BRC:alpha Oct 17, 2026
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>
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.

1 participant