From 51acc56dcb07fa099efc7f8ec921e4e2c67f3732 Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Thu, 6 Nov 2025 16:07:31 +0100 Subject: [PATCH 1/4] ci/github-script/merge: ignore PRs with >= 100 files We use the files endpoint to get a list of all *names* of files touched in the PR - but this endpoint will also actually download the files / their diff, too. That's pointless and actually takes quite some time for huge treewides. We're just putting in a stop-gap for now, so that we're not burning more than 1 API requests on this and don't spend so much time on it either. A limit of 99 files will be more than enough for quite some time - we will only need to raise this when we're able to represent package sets in by-name properly and have "package set maintainers", who are not committers. --- ci/github-script/merge.js | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/ci/github-script/merge.js b/ci/github-script/merge.js index 688a7928fe97..674d8153a7e9 100644 --- a/ci/github-script/merge.js +++ b/ci/github-script/merge.js @@ -128,11 +128,20 @@ async function handleMerge({ (await getTeamMembers('nixpkgs-committers')).map(({ id }) => id), ) - const files = await github.paginate(github.rest.pulls.listFiles, { - ...context.repo, - pull_number, - per_page: 100, - }) + const files = ( + await github.rest.pulls.listFiles({ + ...context.repo, + pull_number, + per_page: 100, + }) + ).data + + // Early exit to prevent treewides from using up a lot of API requests (and time!) to list + // all the files in the pull request. For now, the merge-bot will not work when 100 or more + // files are touched in a PR - which should be more than fine. + // TODO: Find a more efficient way of downloading all the *names* of the touched files, + // including an early exit when the first non-by-name file is found. + if (files.length >= 100) return false // Only look through comments *after* the latest (force) push. const lastPush = events.findLastIndex( From 9efe926863c5d4e087057fe610ef9eb73fa942a7 Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Thu, 6 Nov 2025 16:11:01 +0100 Subject: [PATCH 2/4] ci/github-script/reviewers: exit early for treewides When hitting a treewide, we would previously find the username for each user and then check all of them for collaborator status - only to then realize that this results in more than 15 reviewers and exit. We can put a simple stop-gap in, even before de-duplicating the combined lists of maintainers and owners as safe guard. We could still hit huge numbers of code owners, but in practice we don't nearly as many as maintainers, so this will be sufficient for now. --- ci/github-script/reviewers.js | 46 ++++++++++++++++++++++------------- 1 file changed, 29 insertions(+), 17 deletions(-) diff --git a/ci/github-script/reviewers.js b/ci/github-script/reviewers.js index c5f79c11d992..eefa87e36d65 100644 --- a/ci/github-script/reviewers.js +++ b/ci/github-script/reviewers.js @@ -13,6 +13,34 @@ async function handleReviewers({ }) { const pull_number = pull_request.number + const requested_reviewers = new Set( + pull_request.requested_reviewers.map(({ login }) => login), + ) + log( + 'reviewers - requested_reviewers', + Array.from(requested_reviewers).join(', '), + ) + + const existing_reviewers = new Set( + reviews.map(({ user }) => user?.login).filter(Boolean), + ) + log( + 'reviewers - existing_reviewers', + Array.from(existing_reviewers).join(', '), + ) + + // Early sanity check, before we start making any API requests. The list of maintainers + // does not have duplicates so the only user to filter out from this list would be the + // PR author. Therefore, we check for a limit of 15+1, where 15 is the limit we check + // further down again. + // This is to protect against huge treewides consuming all our API requests for no + // reason. + if (maintainers.length > 16) { + core.warning('Too many potential reviewers, skipping review requests.') + // Return a boolean on whether the "needs: reviewers" label should be set. + return existing_reviewers.size === 0 && requested_reviewers.size === 0 + } + const users = new Set([ ...(await Promise.all( maintainers.map(async (id) => (await getUser(id)).login), @@ -64,24 +92,8 @@ async function handleReviewers({ ).filter(Boolean) log('reviewers - reviewers', reviewers.join(', ')) - const requested_reviewers = new Set( - pull_request.requested_reviewers.map(({ login }) => login), - ) - log( - 'reviewers - requested_reviewers', - Array.from(requested_reviewers).join(', '), - ) - - const existing_reviewers = new Set( - reviews.map(({ user }) => user?.login).filter(Boolean), - ) - log( - 'reviewers - existing_reviewers', - Array.from(existing_reviewers).join(', '), - ) - if (reviewers.length > 15) { - log( + core.warning( `Too many reviewers (${reviewers.join(', ')}), skipping review requests.`, ) // Return a boolean on whether the "needs: reviewers" label should be set. From 17199e5ff6d970c47338c22b07e5e39e305f2a8b Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Thu, 6 Nov 2025 16:11:36 +0100 Subject: [PATCH 3/4] ci/github-script/reviewers: add TODO about future optimization We still use a few too many API requests by checking team members for collaborator status - we can improve on that in the future. --- ci/github-script/reviewers.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ci/github-script/reviewers.js b/ci/github-script/reviewers.js index eefa87e36d65..07a77a5fec0d 100644 --- a/ci/github-script/reviewers.js +++ b/ci/github-script/reviewers.js @@ -75,6 +75,9 @@ async function handleReviewers({ const reviewers = ( await Promise.all( Array.from(new_reviewers, async (username) => { + // TODO: Restructure this file to only do the collaborator check for those users + // who were not already part of a team. Being a member of a team makes them + // collaborators by definition. try { await github.rest.repos.checkCollaborator({ ...context.repo, From cd7f83638e0e96f667ce904a3f0363967f654cef Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Thu, 6 Nov 2025 16:16:30 +0100 Subject: [PATCH 4/4] ci/github-script/bot: limit concurrency in PR runs This lead to reaching secondary API limits in a treewide recently, so we better limit it to where we actually need it. --- ci/github-script/bot.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/ci/github-script/bot.js b/ci/github-script/bot.js index c66a91583140..34abe96941fb 100644 --- a/ci/github-script/bot.js +++ b/ci/github-script/bot.js @@ -576,7 +576,10 @@ module.exports = async ({ github, context, core, dry }) => { // Controls level of parallelism. Applies to both the number of concurrent requests // as well as the number of concurrent workers going through the list of PRs. - const maxConcurrent = 20 + // We'll only boost concurrency when we're running many PRs in parallel on a schedule, + // but not for single PRs. This avoids things going wild, when we accidentally make + // too many API requests on treewides. + const maxConcurrent = context.eventName === 'pull_request' ? 1 : 20 await withRateLimit({ github, core, maxConcurrent }, async (stats) => { if (context.payload.pull_request) {