kumarUjjawal commented on code in PR #26683:
URL: https://github.com/apache/datafusion/pull/26683#discussion_r4489616638
##########
ci/scripts/release_version_labeler.js:
##########
@@ -27,10 +44,21 @@ module.exports = async ({ github, context }) => {
}
}
- await github.rest.issues.addLabels({
- owner: context.repo.owner,
- repo: context.repo.repo,
- issue_number: context.payload.pull_request.number,
- labels: [target_version]
- });
+ // A push can include several merged pull requests. Look the commits up in
+ // parallel, so the retries of commits without one overlap.
+ const pushed = (context.payload.commits || []).map((commit) => commit.id);
+ const shas = pushed.length ? pushed : [context.sha];
+ const found = await Promise.all(shas.map((sha) => mergedPulls({ github,
context, sha })));
Review Comment:
All commits of the push are looked up at the same time with no limit, and
one failed lookup makes Promise.all reject, so no pull request in the push gets
the label.
A push to main can hold many commits, for example a branch merged with a
merge commit or a large merge queue group. Each commit starts its own
listPullRequestsAssociatedWithCommit call at once. A burst of up to 2048 calls
can hit GitHub's secondary rate limit (about 100 requests at the same time).
Then one call fails with 403/429, or one call fails with a normal 5xx.
Promise.all rejects, the job fails before addLabels runs, and no pull request
in that push gets the label. The old pull_request_target flow labeled each pull
request in its own run, so one error never cost other pull requests their
label. Fix: look the commits up in small batches, and catch the error for each
commit (for example with Promise.allSettled) so that the lookups that worked
still lead to labels.
##########
.github/workflows/fork_ci_status.yml:
##########
@@ -31,18 +33,28 @@ on:
permissions:
actions: read
checks: write
+ contents: read
pull-requests: read
concurrency:
- group: ${{ github.workflow }}-${{ github.event.pull_request.number ||
'update' }}
- cancel-in-progress: ${{ github.event_name == 'pull_request_target' }}
+ group: ${{ github.workflow }}-${{ github.event_name == 'workflow_run' &&
format('{0}-{1}', github.event.workflow_run.head_repository.full_name,
github.event.workflow_run.head_branch) || 'update' }}
+ cancel-in-progress: ${{ github.event_name == 'workflow_run' }}
jobs:
bridge:
name: Mirror the fork run
+ if: github.event_name != 'workflow_run' || github.event.workflow_run.event
== 'pull_request'
Review Comment:
The job runs a runner, a checkout and a pulls.list call for every signal
run, even when the payload already shows that the head repository is not a fork
or was deleted.
Failure scenario: Every push to a pull request from an apache/datafusion
branch starts the signal. The bridge job then gets a runner, does the sparse
checkout and calls the API, and only after that notify() says 'Not a fork pull
request to main'. The same happens for a deleted fork (head_repository is
null). Fix: add `github.event.workflow_run.head_repository.fork == true` to the
job-level `if` for the workflow_run event. That skips these runs before any
runner starts, and the release-branch check stays in notify().
##########
.github/workflows/labeler.yml:
##########
@@ -4,36 +4,55 @@
name: Labeler
+# `pull_request_target` is disabled by ASF infra policy. This workflow runs
from
+# the default branch when `pull_request_signal.yml` starts, so it never runs
the
+# code of a pull request. See that file for details.
concurrency:
- group: ${{ github.repository }}-${{ github.head_ref || github.sha }}-${{
github.workflow }}
+ group: ${{ github.repository }}-${{
github.event.workflow_run.head_repository.full_name }}-${{
github.event.workflow_run.head_branch }}-${{ github.workflow }}
Review Comment:
The concurrency group is the head repository and branch with
cancel-in-progress, but the lookup only matches the run's own head SHA. If an
older run is queued after a newer one, it cancels the newer run and then labels
nothing.
Failure scenario: A contributor pushes twice quickly to a fork branch. If
the workflow_run `requested` event for push 1 is queued after the event for
push 2, the push 1 run cancels the push 2 run. pull_requests_of_run.js then
drops the pull request, because pull.head.sha no longer equals run.head_sha.
Neither run labels the new head, and labels stay stale until the next push.
fork_ci_status.yml has the same group shape, but its scheduled sweep fixes the
check later. The labeler has no such recovery. Fix: let the run that lost the
race use the current head (drop the SHA filter for the labeler), or put the SHA
in the group and turn off cancel-in-progress.
##########
ci/scripts/release_version_labeler.js:
##########
@@ -27,10 +44,21 @@ module.exports = async ({ github, context }) => {
}
}
- await github.rest.issues.addLabels({
- owner: context.repo.owner,
- repo: context.repo.repo,
- issue_number: context.payload.pull_request.number,
- labels: [target_version]
- });
+ // A push can include several merged pull requests. Look the commits up in
+ // parallel, so the retries of commits without one overlap.
+ const pushed = (context.payload.commits || []).map((commit) => commit.id);
+ const shas = pushed.length ? pushed : [context.sha];
+ const found = await Promise.all(shas.map((sha) => mergedPulls({ github,
context, sha })));
Review Comment:
Very unlikely but for a push containing more than 100 commits, this launches
enough simultaneous requests to exceed GitHub's secondary limit of 100
concurrent API requests. A rejected lookup rejects the entire `Promise.all`
before any labels are applied, including for successfully resolved PRs. Use
bounded parallelism rather than launching one request per commit; a 101-commit
stub reproduces 101 concurrent requests and zero labels after a rate-limit
rejection.
##########
ci/scripts/pull_requests_of_run.js:
##########
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: Apache-2.0
+// SPDX-FileCopyrightText: See the NOTICE file distributed with this work for
additional information regarding copyright ownership
+// SPDX-FileContributor: Licensed to the Apache Software Foundation (ASF)
under one or more contributor license agreements
+
+// Used by the labeler and fork_ci_status workflows, which run on
`workflow_run`.
+//
+// `workflow_run.pull_requests` is empty for pull requests from forks, so find
+// the open pull requests by head branch and commit. A commit that is no longer
+// a head matches nothing, since a newer run handles the new head. The same
+// branch can target several bases, so pass `base` to select one.
+module.exports = async ({ github, context, base }) => {
+ const run = context.payload.workflow_run;
+ // The head repository is null when the fork was deleted.
+ if (!run.head_repository) {
+ return [];
+ }
+ const { data } = await github.rest.pulls.list({
+ owner: context.repo.owner,
+ repo: context.repo.repo,
+ state: 'open',
+ head: `${run.head_repository.owner.login}:${run.head_branch}`,
+ ...(base && { base })
+ });
+ return data.filter((pull) => pull.head.sha === run.head_sha);
Review Comment:
Prevent stale signals from cancelling the current label update. For rapid
successive pushes, GitHub does not guarantee execution order within a
concurrency group. If the older signal's consumer arrives last, `labeler.yml`
cancels the newer consumer because both share the branch-scoped group. This
filter then discards the older SHA, so neither run updates the labels, and
there is no scheduled label reconciliation. Reconcile the current PR head or
ensure obsolete signals cannot cancel its update.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
