Share the query pool, so concurrency actually limits

[dougdonohoe]

Requested by @balupton in bevry/staticsitegenerators#497:

brilliant work uncovering the github-api concurrency bug. If we can get an upstream PR for that, and for its own rate-limit retries, that would be amazing!

This is the first of those two. The rate-limit retries are #280, which this deliberately does not touch.

The bug

queryREST and queryGraphQL resolve their pool lazily:

opts.pool ??= new PromisePool(opts.concurrency)

But every caller reaches them through a spread — getGitHubRepository calls queryREST({ ...opts, pathname }), and there are over twenty such sites. The assignment lands on a throwaway copy of the options, so each request constructs a pool of its own and nothing limits the batch.

The visible symptom is that concurrency silently limits nothing. In staticsitegenerators, getGitHubRepositories over 430 slugs issued all 430 requests simultaneously and tripped GitHub's secondary rate limit, and passing { concurrency: 10 } changed nothing.

The change

Keying pools by concurrency at module level fixes every call site at once, since they all funnel through those two functions. An explicitly supplied pool still wins, and concurrency: 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

Behavior change, flagged deliberately

That first row is a change for existing consumers. Previously an unspecified concurrency produced new PromisePool(undefined), and native-promise-pool treats 0 as unlimited — so the default was no limit at all. It now defaults to 100, which is the ceiling GitHub documents:

Make too many concurrent requests. No more than 100 concurrent requests are allowed. This limit is shared across the REST API and GraphQL API.

Callers who want the previous behavior can pass concurrency: 0. Happy to drop defaultConcurrency back to unlimited and fix only the sharing if you would rather not change the default in a patch — the two parts are independent.

Tests

Adds a pool suite. The behavioral test drives queryREST against a stubbed fetch, so it needs neither the network nor credentials and runs in CI regardless of secrets. Verified it fails against the unfixed build:

@bevry/github-api ➞ pool ➞ limits concurrent requests ✘
FAILURE: 32/33 tests ran successfully; 1 failed, 0 incomplete, 1 errors

The other three cover the registry contract: spread options resolve to the same pool, different concurrencies resolve to different pools, and an unspecified concurrency resolves to the documented default.

tsc, eslint and prettier are clean.

One unrelated thing worth knowing

Running npm run our:compile on a machine without deno silently rewrites package.json, removing the deno edition entry and its keywords — make-deno-edition --attempt reports necessary entry file [source/index.ts] failed and drops it rather than failing the build. It is easy for a contributor to commit that by accident and delete the deno edition from the published package. I have kept it out of this PR.

[dougdonohoe]

@balupton - have you had a chance to look at this?

[balupton]

I eyeballed it last week. Planning to clone it out in the next 24 hours, and test it out. Next window to work on PRs will be mid next week.

[balupton]

Will have to be next Wednesday. This week is too busy with upcoming travel. Sorry for the delay

[balupton]

integrating now

[balupton]

Strange. I tried to merge this via the CLI and it didn't work properly. Anyway, it is merged and you are credited. Release incoming.

My workflow was gh pr checkout 297, then I did some work on the branch, then I tried all sorts of ways to update this PR with the new commit, but nothing did it, so then I did a merge commit where I mentioned the PR, and that pushed that, and it still didn't merge it. I guess github doesn't care about the git history and only about merging via their web interface...