Skip to content

Repository navigation

fix(worker): add p-limit to GitHub API calls to avoid overwhelming the node process (or the API rate limits) - #591

Merged
brendan-kellam merged 1 commit into
sourcebot-dev:mainfrom
brianphillips:github-parallel-requests-limit
Oct 31, 2025
Merged

fix(worker): add p-limit to GitHub API calls to avoid overwhelming the node process (or the API rate limits)#591
brendan-kellam merged 1 commit into
sourcebot-dev:mainfrom
brianphillips:github-parallel-requests-limit

Conversation

@brianphillips

@brianphillips brianphillips commented Oct 31, 2025

Copy link
Copy Markdown
Contributor
  • Add a shared p-limit guard to the GitHub fetch helpers so user/org/repo lookups use capped parallelism instead of unbounded Promise.allSettled.
  • Introduce a central MAX_CONCURRENT_GITHUB_QUERIES constant (set to 5) with explanatory comment to keep token usage and rate-limit pressure predictable. This setting could be pulled from the configuration file but that is (IMO) not likely something that will need to be tuned.

@coderabbitai

coderabbitai Bot commented Oct 31, 2025

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@brendan-kellam
brendan-kellam requested review from brendan-kellam and removed request for brendan-kellam October 31, 2025 21:03
@brendan-kellam brendan-kellam self-assigned this Oct 31, 2025
@brendan-kellam

brendan-kellam commented Oct 31, 2025

Copy link
Copy Markdown
Contributor

Hey @brianphillips thanks for the PR - could you describe the errors you were running into prior to implementing this? Were you hitting rate limits? How many orgs / repos / users were you syncing?

@brianphillips

brianphillips commented Oct 31, 2025

Copy link
Copy Markdown
Contributor Author

Hey @brianphillips thanks for the PR - could you describe the errors you were running into prior to implementing this? Were you hitting rate limits? How many orgs / repos / users were you syncing?

Perhaps not a typical use case but I had >1700 repositories in the config and all of them were getting kicked off simultaneously :) The first sign of trouble is a bunch of these warnings (not critical, but a sign that too many things are being done at once):

[backend] | (node:161) MaxListenersExceededWarning: Possible EventTarget memory leak detected. 1555 abort listeners added to [AbortSignal]. MaxListeners is 1500. Use events.setMaxListeners() to increase limit

After that, things stall for a bit and then it seems that all of them fail with an error like this:

[backend] | 2025-10-31T21:21:45.138Z error: [github] Failed to fetch repository my-org/my-repo. Connect Timeout Error (attempted address: github.my-corp.net:443, timeout: 10000ms)
[backend] | HttpError: Connect Timeout Error (attempted address: github.my-corp.net:443, timeout: 10000ms)
[backend] |     at fetchWrapper (file:///app/node_modules/@octokit/core/node_modules/@octokit/request/dist-bundle/index.js:76:26)
[backend] |     at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
[backend] |     at async fetchWithRetry (file:///app/packages/backend/dist/utils.js:69:20)
[backend] |     at async measure (file:///app/packages/backend/dist/utils.js:9:18)
[backend] |     at async file:///app/packages/backend/dist/github.js:250:50
[backend] |     at async Promise.allSettled (index 1688)
[backend] |     at async getRepos (file:///app/packages/backend/dist/github.js:245:21)
[backend] |     at async getGitHubReposFromConfig (file:///app/packages/backend/dist/github.js:91:37)
[backend] |     at async compileGithubConfig (file:///app/packages/backend/dist/repoCompileUtils.js:17:31)
[backend] |     at async file:///app/packages/backend/dist/connectionManager.js:147:28

@brianphillips
brianphillips force-pushed the github-parallel-requests-limit branch from 3f589b1 to ebc7a72 Compare October 31, 2025 21:32
@brendan-kellam brendan-kellam mentioned this pull request Oct 31, 2025
Closed
@brianphillips
add p-limit to GitHub API calls to avoid overwhelming the node proces…
be92d26 
…s (or the API rate limits)

If there's a high number of `repos` in the config, it is not productive
to try and run all the API requests in parallel.
@brianphillips
brianphillips force-pushed the github-parallel-requests-limit branch from ebc7a72 to be92d26 Compare October 31, 2025 21:34
@brendan-kellam brendan-kellam changed the title add p-limit to GitHub API calls to avoid overwhelming the node process (or the API rate limits) fix(worker): add p-limit to GitHub API calls to avoid overwhelming the node process (or the API rate limits) Oct 31, 2025
@brendan-kellam

brendan-kellam commented Oct 31, 2025

Copy link
Copy Markdown
Contributor

@brianphillips gotcha makes sense - 1700 individual entries in the config is definitely on the edge-case side (hence we have not hit this before). This approach of using p-limit is clean (and applicable to other areas were we are seeing unbounded promises causing problems, #590).

This change is applicable to the other code hosts where we are doing concurrent api calls, but I'd say we can put it into a separate PR
image

@brendan-kellam
brendan-kellam merged commit 58456d6 into sourcebot-dev:main Oct 31, 2025
5 checks passed
@brianphillips
brianphillips deleted the github-parallel-requests-limit branch October 31, 2025 21:50
@github-actions github-actions Bot mentioned this pull request Oct 31, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brendan-kellam brendan-kellam brendan-kellam approved these changes

Assignees

@brendan-kellam brendan-kellam

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@brianphillips @brendan-kellam