Share the query pool, so concurrency actually limits

source/index.ts

@@ -1323,14 +1323,41 @@ 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<number, PromisePool<any>>()
+
+/** Get the pool shared by all queries of this concurrency, creating it if needed. */
+export function getPool(
+	concurrency: number = defaultConcurrency,
+): PromisePool<any> {
+	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.
  * If the user agent is nullish, then it will be set to `"@bevry/github"`
  */
 export async function queryREST<T>(opts: QueryOptions = {}): Promise<T> {
 	// 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<T>(
 	opts: QueryOptions = {},
 ): Promise<T> {
 	// prepare
-	opts.pool ??= new PromisePool(opts.concurrency)
+	opts.pool ??= getPool(opts.concurrency)
 
 	// prepare fetch
 	// https://docs.github.com/en/graphql/overview/explorer

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')