From 7185c443bd7187b85f153f63d6db275eddbab544 Mon Sep 17 00:00:00 2001 From: Doug Donohoe Date: Wed, 2 Sep 2026 09:23:35 -0400 Subject: [PATCH] Share the query pool, so concurrency actually limits `queryREST` and `queryGraphQL` resolve their pool lazily: opts.pool ??= new PromisePool(opts.concurrency) But every caller reaches them through a spread. `getGitHubRepository`, for example, calls `queryREST({ ...opts, pathname })`, and there are over twenty such sites. The assignment therefore lands on a throwaway copy of the options, so each request builds a pool of its own and nothing limits the batch. The `concurrency` option silently limits nothing. Keying pools by concurrency at module level fixes every one of those call sites at once, since they all funnel through these two functions. An explicitly supplied `pool` still wins, and a concurrency of 0 still means unlimited. Peak in-flight requests through `getGitHubRepositories` over 430 slugs, with a stubbed fetch: before after no options 430 100 { concurrency: 10 } 430 10 { concurrency: 0 } 430 430 Note the behavior change in the first row. Previously an unspecified concurrency produced `new PromisePool(undefined)`, which is unlimited. It now defaults to 100, which is the ceiling GitHub documents for concurrent requests, shared across the REST and GraphQL APIs. Callers wanting the old behavior can pass `concurrency: 0`. Adds a `pool` suite covering this. The behavioral test drives `queryREST` against a stubbed fetch, so it needs neither the network nor credentials, and it fails against the unfixed build. --- source/index.ts | 31 ++++++++++++++++++-- source/test.ts | 76 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+), 2 deletions(-) diff --git a/source/index.ts b/source/index.ts index 7bcbb0d..a5730fa 100644 --- a/source/index.ts +++ b/source/index.ts @@ -1323,6 +1323,33 @@ export function getCredentialedURL( return url } +/** + * The concurrency used when {@link QueryOptions.concurrency} is not specified. + * GitHub allows no more than 100 concurrent requests, shared across its REST and GraphQL APIs. + * https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api#about-secondary-rate-limits + */ +export const defaultConcurrency = 100 + +/** + * The pools shared by all queries, keyed by their concurrency. + * Shared because query options are spread into a new object on their way to + * {@link queryREST} and {@link queryGraphQL}, so a pool created against those + * options would only ever apply to the single request that created it. + */ +const pools = new Map>() + +/** Get the pool shared by all queries of this concurrency, creating it if needed. */ +export function getPool( + concurrency: number = defaultConcurrency, +): PromisePool { + let pool = pools.get(concurrency) + if (!pool) { + pool = new PromisePool(concurrency) + pools.set(concurrency, pool) + } + return pool +} + /** * Fetches a GitHub REST API response with authentication, parsing, waiting, pooling, paging. * If the credentials property is nullish, then the environment variables are attempted. @@ -1330,7 +1357,7 @@ export function getCredentialedURL( */ export async function queryREST(opts: QueryOptions = {}): Promise { // defaults - opts.pool ??= new PromisePool(opts.concurrency) + opts.pool ??= getPool(opts.concurrency) const searchParams = new URLSearchParams() applySearchParams(searchParams, opts.searchParams) if (opts.page != null || opts.pages != null || opts.size != null) { @@ -1438,7 +1465,7 @@ export async function queryGraphQL( opts: QueryOptions = {}, ): Promise { // prepare - opts.pool ??= new PromisePool(opts.concurrency) + opts.pool ??= getPool(opts.concurrency) // prepare fetch // https://docs.github.com/en/graphql/overview/explorer diff --git a/source/test.ts b/source/test.ts index 08eb1bf..364dc6b 100644 --- a/source/test.ts +++ b/source/test.ts @@ -26,6 +26,9 @@ import getGitHubLatestCommit, { getGitHubRepositoriesFromSearch, getGitHubSlugFromUrl, hasCredentials, + getPool, + defaultConcurrency, + QueryOptions, } from './index.js' type Errback = (error?: Error) => void @@ -243,6 +246,79 @@ kava.suite('@bevry/github-api', function (suite, test) { suite('redact', function (suite, test) { testFixtures(redactFixtures, redactSearchParams, test) }) + suite('pool', function (suite, test) { + test('limits concurrent requests', function (done: Errback) { + // Drives the public path with a stubbed fetch, so it needs no network + // and no credentials. Before the pool was shared, this peaked at the + // full request count rather than the requested concurrency. + const originalFetch = globalThis.fetch + const concurrency = 3 + const requests = 30 + let inflight = 0 + let peak = 0 + globalThis.fetch = async function () { + inflight++ + peak = Math.max(peak, inflight) + await new Promise((resolve) => setTimeout(resolve, 15)) + inflight-- + return { + ok: true, + status: 200, + statusText: 'OK', + json: async () => ({ ok: true }), + } + } as any + const credentials: GitHubCredentials = { GITHUB_ACCESS_TOKEN: 'stub' } + Promise.all( + Array.from({ length: requests }, () => + queryREST({ pathname: 'rate_limit', concurrency, credentials }), + ), + ) + .finally(() => { + globalThis.fetch = originalFetch + }) + .then(() => { + equal( + peak <= concurrency, + true, + `peak of ${peak} concurrent requests was within the concurrency of ${concurrency}`, + ) + done() + }) + .catch(done) + }) + test('shares one pool across spread options', function () { + // The bug this guards against: queryREST and queryGraphQL receive + // `{ ...opts }` from their callers, so a pool constructed from those + // options applied only to the single request that constructed it, and + // the concurrency option silently limited nothing. + const opts: QueryOptions = { concurrency: 5 } + const a: QueryOptions = { ...opts } + const b: QueryOptions = { ...opts } + a.pool ??= getPool(a.concurrency) + b.pool ??= getPool(b.concurrency) + equal(a.pool === b.pool, true, 'spread options resolved to the same pool') + }) + test('separates pools by concurrency', function () { + equal( + getPool(5) === getPool(7), + false, + 'a different concurrency resolved to a different pool', + ) + }) + test('defaults to defaultConcurrency', function () { + equal( + getPool() === getPool(defaultConcurrency), + true, + 'an unspecified concurrency resolved to the default pool', + ) + equal( + getPool().concurrency, + defaultConcurrency, + 'the default pool limits to the documented concurrency', + ) + }) + }) suite('api', function (suite, test) { if (!hasCredentials()) { console.warn('unable to test API, as github credentials not set')