cloud-fan commented on code in PR #60098: URL: https://github.com/apache/spark/pull/60098#discussion_r4498615039
########## .github/workflows/update_build_status.js: ########## @@ -0,0 +1,401 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +'use strict'; + +module.exports = async ({github, context, core, env = process.env, log = console}) => { + // Open PRs are read with one GraphQL query per page of PRs. For each PR it returns + // the Build checks on the head commit (statusCheckRollup.commit) and the fork's + // workflow runs for that commit (headRef.target), both as GitHub Actions check + // suites: fork runs are Actions runs, and Build checks written with the GITHUB_TOKEN + // land in Actions suites too. On the PR side, only suites containing Build checks + // are relevant; unrelated upstream workflow runs must not fill the page. + // + // A PR rarely has more than a few such suites. A fork's default branch can collect + // thousands from scheduled workflows, but suites are listed oldest first and a push + // creates its runs' suites right away, so they are still among the first. Should a + // page ever not hold everything a PR needs, the PR fails loudly rather than being + // synced from partial data. + // + // GitHub aborts a GraphQL request after 10 seconds and allows 60 seconds of GraphQL + // CPU time per minute. A page of 25 PRs takes about 3 seconds, so each page is + // followed by a pause as long as it took. + // https://docs.github.com/en/graphql/overview/rate-limits-and-query-limits-for-the-graphql-api + const GITHUB_ACTIONS_APP_ID = 15368 + const PR_PAGE_SIZE = 25 + // Name of the mirrored PR check, not the workflow. + const BUILD_CHECK_NAME = 'Build' + + // Quick syncs cover only open PRs updated in the last QUICK_SYNC_LOOKBACK_DAYS. A + // push, comment, review, or label change updates a PR, but a check change does not. + // Fork runs usually finish within hours of a push, and most reruns happen within + // days. A later rerun, or a gap in quick syncs longer than the lookback, is picked + // up by the next full sync, which covers all open PRs. Both schedules are defined + // in update_build_status.yml. + const QUICK_SYNC_LOOKBACK_DAYS = 3 + const fullSync = env.FULL_SYNC == 'true' + const updatedSince = fullSync ? 0 + : Date.now() - QUICK_SYNC_LOOKBACK_DAYS * 24 * 60 * 60 * 1000 + + // A run-linked Build check points at the fork's build_main.yml run for the PR branch. + // Right after a push that run may not be registered yet, so a PR updated in the last + // NEW_RUN_GRACE_MINUTES without a usable run is left for a later pass instead of + // receiving action_required guidance. + const BUILD_WORKFLOW = '.github/workflows/build_main.yml' + const NEW_RUN_GRACE_MINUTES = 5 + const runGraceSince = Date.now() - NEW_RUN_GRACE_MINUTES * 60 * 1000 + + const prPageQuery = ` + query($owner: String!, $repo: String!, $size: Int!, $cursor: String, $appId: Int!) { + repository(owner: $owner, name: $repo) { + pullRequests(states: OPEN, first: $size, after: $cursor, + orderBy: {field: UPDATED_AT, direction: DESC}) { + pageInfo { hasNextPage endCursor } + nodes { + number + updatedAt + headRefOid + statusCheckRollup { + commit { + oid + checkSuites(first: 10, filterBy: { + appId: $appId, checkName: "${BUILD_CHECK_NAME}" + }) { + pageInfo { hasNextPage } + nodes { + checkRuns(first: 10, + filterBy: { + checkName: "${BUILD_CHECK_NAME}", checkType: LATEST + }) { + pageInfo { hasNextPage } + nodes { databaseId status conclusion text } + } + } + } + } + } + headRef { + name + repository { name owner { login } } + target { + oid + ... on Commit { + checkSuites(first: 10, filterBy: {appId: $appId}) { Review Comment: **Non-blocking (P2):** The fork-side suite connection reads only the first 10 GitHub Actions suites on the fork's head commit, with no check-name filter, and `toPR` looks for the PR branch's `build_main.yml` run only on that page. The header comment assumes the PR branch's suites are always among the first 10, but that does not hold when the commit sat at the fork's default-branch head before the PR branch was pushed. The new guidance leads there: a contributor whose PR from `master` gets it pushes the same commit to a new branch and opens a PR from that branch. While the commit was the default-branch head, the fork added suites to it: the skipped `build_main.yml` run from the `master` push, a `test_report.yml` run for every Build completion in the fork (`workflow_run` runs on the default-branch head), and, where the fork runs scheduled workflows, one per scheduled run, which the header says can reach thousands. Once 10 suites precede the branch push, the branch's run is off the page. Reading the code, the backfill then logs 'raise the limits in prPageQuery' and fails the PR on every pass that reads it, and the PR gets no Build check until a new commit is pushed. Raising `first` to its maximum of 100 only moves the threshold, because nothing bounds how many suites the commit collects before the branch push, and GitHub documents no order for this connection. The same window stops syncing an existing Build check whose run is past the page. And when an older run of the same branch and commit is on the page while a newer one is not, `findLast` silently links the new check to the older run. On the base branch, listing the branch's `build_main.yml` runs over REST and matching the head SHA found the run, so these PRs got a linked, synced check and the step stayed green. How fast suites accumulate and in what order GitHub returns them were not verified here. This extends an [existing discussion](https://github.com/apache/spark/pull/60098#discussion_r4489296969). **Recommended change:** When the fork-side suite page is truncated, stop relying on it for the runs it may have cut off. In the sync loop, for any PR, including one from the fork's master, when a check's linked run is not on a truncated page, fetch that run from the fork by its stored run ID over the REST API and sync the check to it; treat a not-found response as a deleted run, so the backfill replaces the check as it does today. In the backfill, for a PR not from the fork's master whose fork page is truncated, list the fork's build_main.yml runs for the PR branch over the REST API, take the newest one for the PR head commit, and use it in place of the page's run, so the existing skipped-run and grace-window rules apply to it, instead of failing the PR or using the newest run on the page; treat a not-found response from that listing as no run, as the base branch did. Fail the PR, with its URL and head commit, only when one of these REST calls fails with any other error. Update th e header comment to describe this fallback instead of saying the PR branch's suites are always among the first. **Why this works:** The GraphQL page stays the only read for nearly every PR. A PR whose fork page reports more suites and lacks what it needs pays one or two REST reads, using the same per-ID and branch-filtered lookups as the base branch, which have no per-commit suite window. Suites the fork adds on its default-branch head can then no longer hide the PR branch's run, or make the backfill pick an older run of the same branch. **Scope:** Add a REST fallback to the build status module for PRs whose fork-side suite page is truncated, and correct the module's header comment. **Compatibility:** PRs whose fork page is not truncated, which is nearly all of them, are still read and synced from the GraphQL page alone. PRs from a fork's master keep their backfill handling; only an existing run-linked check on such a PR whose run is past a truncated page gets the same by-ID lookup as any other check, so it keeps updating as existing run-linked checks on those PRs are meant to. **Risks:** Each pass that reads a PR with a truncated fork page makes one or two extra REST requests. The author's replays found no such open PR today, so the cost is bounded by how many PRs reach that state. **Constraints:** Keep the GraphQL page as the only read for PRs whose fork page is not truncated, so the per-pass cost measured in the PR description is unchanged. Do not change how PRs from a fork's master get new Build checks: they keep getting the action_required guidance, and the backfill looks up no run for them. **Exclusions:** - New Build checks for PRs from a fork's master, which keep the action_required guidance the PR intends even when an older build_main.yml run on the commit ran a real build. **Success:** A PR whose fork head commit carries more than 10 GitHub Actions suites, with the PR branch's build_main.yml run past the first 10, gets a Build check linked to that run on the first pass after the grace window, later passes sync it, and the PR is not reported as failed. An existing Build check whose linked run is past the first 10 fork suites keeps being synced to that run's state, on a PR from any branch, including the fork's master. When a truncated page holds an older build_main.yml run for the PR branch and a newer one exists for the same commit, the new Build check links to the newer run. A REST lookup in the fallback that fails with anything other than not-found fails only that PR for the pass, naming its URL and head commit, and the rest of the pass continues. PRs whose fork page is not truncated make no extra REST requests, and the backfill makes none for PRs from a fork's master. The module's header comment describes the truncation fallback and no longer says the PR branch's suites are always among the first. **Verification:** - **Regression:** Simulate a PR whose fork head commit first collected 10 or more GitHub Actions suites as the fork's default-branch head (a skipped master build_main.yml run, workflow_run and scheduled runs) and then got the PR branch's build_main.yml run, with the page reporting more suites: after the grace window the pass creates exactly one Build check linked to that run in its current state, the step does not fail, and the next pass syncs the check when the run's state changes. - **Regression:** Simulate a PR with an existing run-linked Build check whose run is past a truncated fork page: the pass syncs the check to the run's state without failing the step, and when the run lookup reports the run was not found, the backfill replaces the check as it does for a deleted run. - **Regression:** Simulate a truncated fork page that holds an older build_main.yml run for the PR branch while a newer run for the same commit exists past the page: the new Build check links to the newer run. - **Behavior:** Simulate a REST error other than not-found in the fallback: only that PR is reported as failed, with its URL and head commit, and later PRs on the page and later pages still sync. Simulate a not-found response from the branch listing: the PR is treated as having no run, as on the base branch. - **Compatibility:** For a PR whose fork page is not truncated, the pass makes no extra REST requests, and for a PR from a fork's master the backfill makes none; their Build checks are created, synced, or given guidance exactly as before the change, apart from the by-ID sync of an existing run-linked check whose run is past a truncated page. - **Inspection:** The module's header comment describes the fallback for truncated fork pages and no longer says the PR branch's suites are always among the first. -- 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]
