fix: space out concurrent NCBI E-utilities requests - #238
Closed
Shu-Min (Allen) Kao (shkao) wants to merge 3 commits into
Closed
fix: space out concurrent NCBI E-utilities requests#238Shu-Min (Allen) Kao (shkao) wants to merge 3 commits into
Shu-Min (Allen) Kao (shkao) wants to merge 3 commits into
Conversation
NCBI allows 3 E-utilities requests/sec/IP, 10 with an API key. Pi runs sibling tool calls from one assistant message concurrently, so a single research turn fires a dozen PubMed requests at once and most come back 429. Nothing limited how fast they went out. Add a small in-process gate that spaces request starts, and forward NCBI_API_KEY, which ncbiIdentityParams() never sent even though scrubEndpoint() below it already redacted an api_key param. Measured live, twelve concurrent searches through the registered tool: 0/12 on main, 12/12 with spacing, both anonymously and with a key. Retry is deliberately not part of this. NCBI sends no Retry-After, so a rate-limited burst retries in lockstep, and with spacing in place the result is 12/12 either way. The PMC ID Converter is not sent the key: it is a separate service with no documented api_key support, and attaching one would also pace it at the keyed rate.
|
Shu-Min (Allen) Kao (@shkao) is attempting to deploy a commit to the Companion Team on Vercel. A member of the Team first needs to authorize it. |
…ctual starts Two defects found by review, both with repros. send() cleared the abort timer as soon as fetch resolved, which is when headers arrive, so the body read ran unprotected. A stalled body hung the tool call forever where main aborted it after the request timeout. send() now takes the reader and awaits it inside the try, which also closes the same pre-existing hole in fetchText. The gate reserved absolute wake times upfront. One long tick left every reservation overdue and the whole burst then started at once: measured 0ms gaps after a 700ms event-loop block. Each waiter now measures from the previous request's actual start. Tests were passing with the gate disabled. Mutating ANONYMOUS_MIN_GAP_MS to 0 or dropping the ID Converter host went undetected. Cover the anonymous rate, the ID Converter host, loop-block resilience, and the body-read timeout, and add scripts/ncbi-burst-check.mjs so the burst is reproducible.
The budget is per-IP, so gating PubMed alone left the reported failure in place: a turn mixing pubmed with clinvar or geo still put 21 requests into one rolling second against a ceiling of 3. Route the specialty, variants, and omics-archive fetch helpers through the same gate. They carry no API key, and the interval is read from the outgoing URL, so they pace at the anonymous rate rather than the keyed one. Non-NCBI hosts still return immediately, so their other backends are unaffected. Add NCBI_MIN_REQUEST_GAP_MS to override the interval, for shared or institutional IPs that need more room. The timeout test asserted only that a stalled request had not resolved, which the regression also satisfies by hanging, so it passed with the bug reintroduced. Give it a real budget through a test seam and assert the rejection, racing a deadline so a regression fails the case instead of wedging the run. Live, twelve concurrent calls split between pubmed and clinvar: 12/12 with no rate limiting.
This was referenced Aug 21, 2026
Contributor
|
Thank you for the measured reproduction and the three review-driven fixes. I ported the contributor-authored commits into maintainer PR #239 because this fork PR's Actions attempts ran zero jobs. PR #239 preserved your author attribution, added the public configuration docs, and merged as The maintainer run 32444448914 is now terminal and green across the release-candidate package gate, both Windows installer hosts, and all six Linux/macOS/Windows Node consumers. Issue #237 closed through that merge. Closing this source PR as superseded by #239. |
Contributor
Author
|
Thank you! That was fast! ⚡️ Glad to contribute to this great repo. |
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
2 participants
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
Space out NCBI E-utilities request starts so one research turn stops exceeding the per-IP rate limit.
Fixes #237.
Trade-off up front: without an API key a 12-search turn now takes about 11.9s instead of returning mostly-failed in under a second. With a key it is 3.1s, because the interval can be 125ms instead of 500ms.
NCBI_MIN_REQUEST_GAP_MSoverrides it.Root cause
NCBI allows 3 E-utilities requests/sec/IP, 10 with an API key. Pi runs sibling tool calls from one assistant message concurrently, so a single turn fires a dozen searches at once. Each search-mode call makes two requests, esearch then esummary, so twelve calls is twenty-four requests against a limit of three per second. Nothing paced them.
The budget is per-IP, and four modules use it:
pubmed,specialty,variants,omics-archives, all dispatched from the same tool. All four now share one queue. Gating only PubMed measurably did not fix it: a mixed pubmed/clinvar turn still put 21 requests into one rolling second.ncbiIdentityParams()also never sentNCBI_API_KEY, whilescrubEndpoint()immediately below it already redacted anapi_keyparam that no code path set.Reproduce
NCBI_API_KEYmainatcbe293bMixed sources, 6 pubmed plus 6 clinvar: 12/12, no rate limiting.
Retry is deliberately not here. I tried it first: NCBI sends no
Retry-After, so a rate-limited burst waits one interval and retries in lockstep, reaching only 3/12 anonymously. With pacing it is 12/12 either way.Why not just use europepmc
It is a good workaround and I have suggested it in #237, but it changes which corpus answers a question. PubMed remains the default for several modes, and ClinVar and GEO have no Europe PMC equivalent at all, so they would still exceed the limit.
Notes
pubmedsends one today.api_keysupport.feynmanruns on one machine can still exceed the per-IP limit.Validation
Node
24.15.0, branch onmainatcbe293b.npm run build,npm run typecheck,npm run architecture:check,git diff --checkall clean.Tests:
802pass,1fail. The failure ispi-settings.test.ts"seeds OpenCode Go Kimi", which fails identically on pristinemain. I ran the runner directly becausenpm test'spatch-embedded-pi.mjsprestep fails here onmaintoo; if that is unexpected on your side I can open a separate issue.10 tests in
tests/science-database-pubmed-rate-limit.test.ts. I verified each guards something by reintroducing the bug it covers and confirming it fails: zeroing either interval, dropping the ID Converter host, reverting the gate to absolute timers, un-gating the variants module, and restoring the earlyclearTimeouteach fail at least one case.