Skip to content

[SPARK-60879][INFRA] Speed up build status update workflow - #60098

Closed
nchammas wants to merge 7 commits into
apache:masterfrom
nchammas:build-status-optimization
Closed

nchammas wants to merge 7 commits into
apache:masterfrom
nchammas:build-status-optimization

Conversation

@nchammas

@nchammas nchammas commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Following on to #59829, refactor the scheduled build status updater (update_build_status.yml) completely:

  • Use batched GraphQL requests instead of per-PR REST requests.
  • Only update PR checks that need to be changed. Do a full sync periodically to catch build updates not associated with PR updates (e.g. a job is rerun with no new commit).
  • Clean up the log to only show key metrics and updates made.
  • Move the core logic into its own JavaScript file next to the workflow.

Example of cleaned up log:

https://github.com/apache/spark/pull/60090 1dac863: queued -> in_progress
https://github.com/apache/spark/pull/59954 48a5471: in_progress -> success
726 PRs checked, 36 skipped. Build checks: 7 updated, 2 created, 681 unchanged.

Why are the changes needed?

Currently, an update pass takes about 10 minutes on a 15-minute schedule. That slows down status updates and makes changes to the workflow painful to test. Nearly all of that time goes to per-PR REST reads for PRs that haven't changed in weeks, and to rewriting checks that haven't changed.

Fork runs usually finish within a few hours of a push. Of the latest fork runs on 690 open PRs, 8 were rerun more than 3 days after the push. The full sync picks those up.

Before After
Time per pass ~10 min ~45-60 s (~3-5 min for a full sync)
PRs synced per pass ~725 (all open) ~170
API reads per pass ~1,480 REST 7 GraphQL
Check updates per pass ~685 ~3
Log lines per pass ~3,400 ~10-15
Runner time per day ~16 h ~3 h

Does this PR introduce any user-facing change?

No, not for end-users.

For contributors: Build checks update the same way, just sooner. A fork run rerun more than 3 days after the PR was last updated shows up on the next full sync, which is scheduled every 4 hours, or on the next pass after any PR activity (e.g. a comment).

Contributor guidance now explains enabling Actions, using a non-master branch, and retriggering builds. To keep fork master as an upstream-sync branch, new Build checks for PRs from master provide action_required guidance rather than linking to a build, even if an older workflow ran there; existing run-linked checks continue updating.

How was this patch tested?

Via an agent:

I replayed the old and new scripts read-only against live apache/spark PRs, intercepting writes. The new script made no write the old one didn't. All but one of the updates it skipped were no-ops. The remaining one was a stuck rerun on a PR outside the 3-day window, which the full sync covers. A full-sync replay synced all 727 open PRs.

I also replayed both scripts with existing checks hidden, so every check had to be created. 724 of 725 creates matched, and each new check had its fork run's state. The one difference was a PR pushed a few minutes earlier with no fork run yet, which the new script left for the next pass.

I ran quick and full syncs locally through actions/github-script against live PR data, intercepting writes and simulating responses, including ignored rerun updates. They checked 169 and 729 PRs in 57 seconds and 264 seconds, and logged 7 and 8 lines. All 26 focused tests passed, covering unchanged checks, applied and ignored updates, new checks, PRs from a fork's master, and errors. The test and dry-run harnesses load the JavaScript module directly. Replaying the 3 open PRs from a fork's master with their checks hidden, each got action_required.

Tested on GitHub against my personal fork with a real GITHUB_TOKEN: a full read-only sync checked 728 PRs in 4m30s using 18 GraphQL points. A temporary fork PR verified check creation, updating the same check from queued to cancelled, and zero API writes on an unchanged repeat, with no duplicate checks.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: GitHub Copilot

@HyukjinKwon HyukjinKwon added the ai-review-requested Trigger AI review by Omnigent on Databricks label Nov 4, 2026
spark-test

This comment was marked as resolved.

@spark-test spark-test removed the ai-review-requested Trigger AI review by Omnigent on Databricks label Nov 4, 2026
@nchammas

nchammas commented Nov 5, 2026

Copy link
Copy Markdown
Contributor Author

I've addressed the line items.

Regarding this concern:

Can a GraphQL error on one PR stop the whole sync pass?

It doesn't appear to be the case from looking at how GitHub's endpoint works. If/when we see concrete examples of this in the real world, we can revisit this issue. Until then, I'd rather not add code to handle hypothetical scenarios.

@nchammas nchammas added the ai-review-requested Trigger AI review by Omnigent on Databricks label Nov 5, 2026

@spark-test spark-test left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review summary

The three issues from the previous review are fixed at this head. A fork-master commit read before its run concludes no longer gets a queued check that later syncs to skipped. The action_required guidance now tells contributors on other branches to push a new commit after fixing the cause. The runIdOf comment lists only sources the app-filtered query can return. This review accepts your assessment that GitHub doesn't return per-PR GraphQL errors for this query, and doesn't raise that question again.

The fork-master fix goes further than it needs to. It drops every run on a fork's master, including real builds from commits that predate the fork-master skip in build_main.yml, so those PRs now get the guidance instead of their build result. See the inline comment.

The previous review's question about rerun latency on PRs idle for more than 3 days is still open and follows below.

Findings

1 total: 0 P0, 0 P1, 1 P2, 0 P3.

Non-blocking (P2)

  • Real builds on a fork's master are replaced by the action_required guidance — .github/workflows/update_build_status.js:306 — see inline.

Re-review status

Prior AI findings: 3 no longer raised, 0 still raised; additional unresolved findings in this review: 1.

New attribution: 1 newly introduced, 0 late catch, 0 previously raised, 0 unattributed.

PR description suggestions

  • The "one visible change" sentence leaves out that the action_required guidance text was also rewritten for every PR that gets it: the item about rebasing onto the latest master is replaced by one telling contributors with a PR from a fork's master to open it from another branch, and contributors on other branches are now told to push a new commit after fixing the cause.

Decision challenges

Fork reruns on PRs idle for more than 3 days wait for the 4-hourly full sync

Quick syncs read only PRs updated in the last 3 days, and a fork run changing state doesn't update the PR. So on a PR idle for longer than that, a rerun or late completion in the fork reaches the Build check only at the next full sync, up to about 4 hours later. It can take about 8 hours when GitHub drops that scheduled run, or when the full sync is still pending behind another pass and the next quick sync replaces it, since both schedules share one concurrency group and GitHub keeps at most one pending run per group. The result is still correct, only later.

On the base branch, every 15-minute pass re-read every open PR, so the same change showed up within about 15-25 minutes, but each pass took about 10 minutes and slowed every PR's update. With this change, PRs with recent activity update within about 10 minutes and a pass takes about a minute. By your sample, 8 of the latest fork runs on 690 open PRs were rerun more than 3 days after the push.

This head keeps the 3-day window, the 4-hour full sync, and the shared concurrency group from the previous round. Is about 4 hours, or about 8 in the worst case, the freshness you want for these PRs? A shorter full-sync interval or a longer lookback would cost more runner time, and keeping a pending full sync from being replaced by a quick sync would remove one route to the 8-hour case.

Generated by Omnigent on Databricks

// has nothing to show, and build_main.yml skips every push to a fork's master,
// so a run there has nothing to show even before it concludes.
if (!syncableBuildCheck) {
const run = pr.branch == 'master' || pr.buildRun?.conclusion == 'skipped'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking (P2): This treats every PR opened from a fork's master as having no run to show, without looking at the run's status or conclusion. The comment above assumes build_main.yml skips every push to a fork's master, but that holds only for commits that carry the guard that SPARK-57074 added to build_main.yml on 2026-05-27, because a push runs the workflow file from the pushed commit (SPARK-58459). A new commit on a fork master last synced before then still runs the full build. For that PR, the base branch's updater created a Build check pointing at the run and synced it to the run's result. Here, once the PR is past the 5-minute grace, it gets the action_required guidance instead. Committers see a failure-like status in place of the real pass or fail, and the contributor is told the build does not run on master, which is false for that commit. Fork-master commits that carry the guard still get the intended guidance.

One way to fix it is to decide from the run itself. Treat only a completed skipped run as having nothing to show. While a run on the fork's master hasn't completed, skip the PR on this pass, as the previous review suggested, so a run that will be skipped never becomes a queued check. Then apply the truncated-suites check below to master too, and update the comment above and the workflow header to match.

Verification:

  • Regression: On a PR opened from a fork's master whose head commit's build_main.yml run actually builds, passes made after the 5-minute grace create no action_required check while the run is queued or in progress. Once the run completes, a Build check pointing at that run is created with the run's conclusion.
  • Regression: On a PR opened from a fork's master whose build_main.yml run is queued, in progress, or waiting on one pass and completed as skipped on the next, with the PR last updated more than 5 minutes before the second pass, the first pass creates no Build check and the second creates exactly one action_required check.
  • Behavior: A PR from a non-master fork branch still gets a Build check created in its run's current state, and once the PR was last updated more than 5 minutes ago, a skipped run there still gets the action_required guidance.
  • Behavior: A PR opened from a fork's master with no build_main.yml run in a truncated fork suite list is reported as a failed PR instead of receiving the action_required guidance.

@spark-test spark-test removed the ai-review-requested Trigger AI review by Omnigent on Databricks label Nov 5, 2026
@nchammas

nchammas commented Nov 5, 2026

Copy link
Copy Markdown
Contributor Author

Is about 4 hours, or about 8 in the worst case, the freshness you want for these PRs?

Yes, this is an intentional tradeoff.

@cloud-fan cloud-fan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review summary

The fork-master issue from the previous review is still present at this head. The backfill still drops every run on a fork's master, including real builds from commits that predate the fork-master skip in build_main.yml. The two commits since then reword the code comments and the workflow header but leave the behavior unchanged, so that finding stays open in its inline thread. One correction to the fix suggested there: keep the existing exemption of master from the truncated-suites failure instead of applying that check to master too. Scheduled workflows run on a fork's default-branch head, so a fork-master PR with no build_main.yml run on the page would otherwise fail on every pass, for example when the fork enabled Actions after the push. Thanks for confirming that the up-to-4-hour freshness for idle PRs is an intentional trade-off. This review doesn't raise it again.

This review also raises three new issues. Some PR head commits are apache/spark commits that the repository's own workflows ran on, such as a synced fork master opened against a release branch. Those commits carry more than 10 GitHub Actions suites on the base side, so the PR fails on every pass and never gets a Build check. Separately, four comments describe a narrower action_required rule than the code implements, and the quick and full sync comment ends with a circular sentence.

Findings

4 total: 0 P0, 0 P1, 2 P2, 2 P3.

Non-blocking (P2)

  • PRs whose head commit carries apache/spark's own workflow runs fail on every pass — .github/workflows/update_build_status.js:72 — see inline.
  • Real builds on a fork's master are replaced by the action_required guidance (still raised) — .github/workflows/update_build_status.js:306 — unresolved in existing discussion.

Nit (P3)

  • Comments describe a narrower action_required rule than the code implements — .github/workflows/update_build_status.js:303 — see inline.
  • The quick and full sync comment ends with a circular sentence — .github/workflows/update_build_status.js:44 — see inline.

Re-review status

Prior AI findings: 0 no longer raised, 1 still raised; additional unresolved findings in this review: 3.

New attribution: 1 newly introduced, 2 late catch, 0 previously raised, 0 unattributed.

Existing discussions

PR description suggestions

  • The "one visible change" sentence leaves out that the action_required guidance was rewritten for every PR that gets it. It is retitled "PR build setup requires attention", the item about rebasing onto the latest master is replaced by one saying to use a branch other than master in the fork, and PRs from other branches are also told to push a new commit after fixing the cause. This follows up on an existing discussion.

statusCheckRollup {
commit {
oid
checkSuites(first: 10, filterBy: {appId: $appId}) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking (P2): This connection lists every GitHub Actions check suite in apache/spark on the PR head commit, not only the suites that hold Build checks. The checkName: "Build" filter below applies only to the check runs inside each suite. A commit that only a PR uses has a suite or two. But some PR head commits are apache/spark commits that the repository's own workflows ran on, for example when a PR from a synced fork master is opened against branch-4.x. Every scheduled run while that commit was the master head added a suite to it: this workflow alone adds one every 10 minutes, and 35 other workflows have cron schedules. Past 10 suites, toPR throws (line 154). The PR never gets a Build check, and the step fails on every pass that reads it. The logged remedy, 'raise the limits in prPageQuery', can't help: a day as master head adds about 144 suites from this workflow alone, and first is capped at 100. In a mock run of the module, 10 base-side suites with hasNextPage true produced no write and a failed step. The same PR without the next page got a success Build check. The base branch's updater paginated every check run over REST, so the same PR got a Build check and the step stayed green.

One way to fix it is to add a check-name filter here. CheckSuiteFilter takes a checkName alongside appId, so filterBy: {appId: $appId, checkName: "Build"} should limit the connection to suites that hold a Build check. The truncation failure would then fire only for a real anomaly.

Verification:

  • Regression: A PR whose head commit carries more than 10 GitHub Actions check suites in apache/spark, none or one of which holds a Build check, is synced or gets a Build check backfilled, and is not reported as failed.
  • Behavior: On a read-only full sync against live open PRs, every existing Build check is still found and synced as before the change, and a head commit whose suites holding Build checks exceed the page limit is still reported as failed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed.

// No syncable Build check: none was created yet, only an action_required one
// exists, or the existing one points at a fork run that is gone. Create one
// from the fork's build_main.yml run for the PR branch (see toPR). A skipped run
// has no result to show. New Build checks are only created for non-master fork

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (P3): This comment and three others describe a narrower action_required rule than the code implements. This line and lines 347-348 say new Build checks are created only for non-master fork branches. Yet line 367 creates an action_required Build check for every fork-master PR that has no run-linked check. Line 247 says that check is written 'when no fork run was found', and line 316 says 'Already told there is no run'. Yet line 306 also writes it when the build_main.yml run was skipped. A maintainer reading these comments would conclude that fork-master PRs get no Build check and that action_required means no fork run exists. The workflow header (YAML lines 25-30) already states the rule more precisely.

Consider rewording all four so each names the cases the code beside it handles. Guidance is written when no usable build_main.yml run exists for the PR branch: none is found after the grace window, or the run was skipped. Only run-linked checks need a usable run. Whatever fork-master handling lands, these comments should describe it.

Verification:

  • Inspection: Each comment that describes when an action_required Build check is written, or which PRs get new Build checks, names exactly the cases the adjacent code handles, including skipped runs and the fork-master handling that lands.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed.

// 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 are scheduled at the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (P3): 'Both are scheduled at the schedule in the workflow.' is circular and doesn't say where the quick and full sync schedules are defined. Something like 'Both schedules are defined in update_build_status.yml.' would say what it means.

Verification:

  • Inspection: The quick and full sync comment names the workflow file that holds both cron schedules, and reads as a complete sentence.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed.

@nchammas

nchammas commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

Real builds on a fork's master are replaced by the action_required guidance

This is intentional and I've updated the PR description accordingly. We want to steer contributors away from submitting PRs using their fork's master branch.

@nchammas
nchammas requested a review from cloud-fan November 6, 2026 15:48

@cloud-fan cloud-fan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review summary

Three of the previous review's line items are fixed at this head. The apache/spark-side suite connection now reads only suites that hold a Build check, so apache/spark's own workflow runs on a PR head commit no longer fail the PR. The comments that describe when action_required guidance is written now name the cases the code handles, and the quick and full sync comment now says both schedules are defined in update_build_status.yml.

This review raises one new issue, on the fork side of the same query. The fork-side connection still reads an unfiltered page of 10 GitHub Actions suites on the fork's head commit. A PR whose head commit first sat at the fork's default branch, which is where the new guidance sends contributors who opened a PR from master, can lose its build_main.yml run off that page. Such a PR fails the step on every pass and never gets a Build check, while the base branch's REST lookup by branch found the run.

Conclusions on existing discussions:

  • I accept this fork-master behavior as the author intends: build_main.yml already skips its build on pushes to a fork's master (SPARK-57074), so giving new Build checks on fork-master PRs the setup guidance keeps this workflow consistent with it, at the cost of the build result on PRs from a fork master last synced before that change. (discussion)
  • The fork-master behavior this review questioned is accepted as intended, because build_main.yml already skips its build on pushes to a fork's master (SPARK-57074), and the author confirmed the idle-PR freshness trade-off. (discussion)

Findings

1 total: 0 P0, 0 P1, 1 P2, 0 P3.

Non-blocking (P2)

  • Fork-side 10-suite page can hide the PR branch's build_main.yml run — .github/workflows/update_build_status.js:97 — see inline.

Re-review status

Prior AI findings: 4 no longer raised, 0 still raised; additional unresolved findings in this review: 1.

New attribution: 0 newly introduced, 1 late catch, 0 previously raised, 0 unattributed.

target {
oid
... on Commit {
checkSuites(first: 10, filterBy: {appId: $appId}) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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 the 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.

@cloud-fan

Copy link
Copy Markdown
Contributor

Thanks for confirming. I'm fine with this behavior. build_main.yml already skips its build on every push to a fork's master (SPARK-57074), so giving new Build checks on fork-master PRs the setup guidance keeps this workflow consistent with it. The only cost is the build result on PRs opened from a fork master last synced before that change, and steering contributors to other branches is worth that.

@HyukjinKwon HyukjinKwon added the ai-review-requested Trigger AI review by Omnigent on Databricks label Nov 8, 2026

@spark-test spark-test left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review summary

This review covers the same head commit as the previous review, and that review's one finding still applies. The fork-side query reads only the first 10 GitHub Actions check suites on the fork's head commit. A commit that first sat at the fork's default-branch head can push the PR branch's build_main.yml run past that page. The PR then fails the step on every pass and gets no Build check, and an existing check whose run is past the page stops updating. This review confirms the finding independently and agrees with the REST fallback proposed in that discussion, with two additions. When looking up an existing check's run by ID fails with an error other than not-found, the backfill should not replace that check; otherwise a PR from a fork's master could get action_required guidance next to a check that still syncs. And the toPR description of buildRun needs the same correction as the header comment, because a truncated page can hold only an older run. The other cases traced here behave as intended, including legacy notifier checks, deleted fork runs, skipped runs, and PRs within the 5-minute registration grace. Two decision challenges about trade-offs you have already discussed follow.

Findings

1 total: 0 P0, 0 P1, 1 P2, 0 P3.

Non-blocking (P2)

Re-review status

Prior AI findings: 0 no longer raised, 1 still raised; additional unresolved findings in this review: 0.

New attribution: 0 newly introduced, 0 late catch, 0 previously raised, 0 unattributed.

Prior AI findings still raised

  • Fork-side 10-suite page can hide the run a Build check needs — .github/workflows/update_build_status.js:97

Decision challenges

New Build checks on fork-master PRs show guidance even when a real build ran

Following up on the earlier discussion: the code at this head matches the intent you and cloud-fan described. A new Build check on a PR from a fork's master never links a run (update_build_status.js:315-316), so once the PR is 5 minutes past its last update it gets the 'PR build setup requires attention' guidance. For commits that carry the fork-master skip in build_main.yml, the base branch could only link a skipped run, so the guidance is clearer there. The cost falls only on commits made on a fork master last synced before that skip. Those commits still run the full build, and where the base branch showed its pass or fail, this head shows action_required. Existing run-linked checks on those PRs keep syncing, and the fallback suggested for the fork-side page leaves this behavior unchanged. What remains open is only whether the project accepts losing those results. If it does, no change is needed.

This follows up on existing discussions 1, 2, 3, 4, 5.

Fork run changes on PRs idle over 3 days wait for the 4-hourly full sync

Following up on your confirmation in the earlier discussion: this head keeps the 3-day quick-sync window (update_build_status.js:49-52), the full sync every 4 hours from 02:35 to 22:35 UTC, and one concurrency group for both schedules. A fork run that changes state on a PR idle for more than 3 days, such as a late rerun, therefore reaches its Build check at the next full sync, up to about 4 hours later. It can take about 8 hours if GitHub drops that run, or if the next quick sync replaces it while it waits behind another pass. On the base branch the same change showed up within about 15-25 minutes, but every pass took about 10 minutes and slowed updates for all PRs. Results stay correct, only later, and by your sample 8 of the latest fork runs on 690 open PRs were rerun more than 3 days after the push. This review found nothing that changes the trade-off. What remains open is only whether the project accepts that delay for idle PRs. If it does, no change is needed.

This follows up on existing discussions 1, 2.

Generated by Omnigent on Databricks

@spark-test spark-test removed the ai-review-requested Trigger AI review by Omnigent on Databricks label Nov 8, 2026
@nchammas

nchammas commented Nov 8, 2026

Copy link
Copy Markdown
Contributor Author

This is a very unlikely scenario that, as far as I can tell, we have not hit recently. I've updated the logic, however, to just show the action guidance to push a commit to a non-master branch rather than add the PR to the fail list.

New Build checks on fork-master PRs show guidance even when a real build ran

As intended.

Fork run changes on PRs idle over 3 days wait for the 4-hourly full sync

As intended.

@cloud-fan

Copy link
Copy Markdown
Contributor

@HyukjinKwon does this need to go to both master and 4.x, or master only?

@HyukjinKwon

Copy link
Copy Markdown
Member

master only should work

@nchammas

Copy link
Copy Markdown
Contributor Author

master only, because scheduled workflows only run from the default branch.

@cloud-fan cloud-fan closed this in 341c2ad Nov 10, 2026
@cloud-fan

Copy link
Copy Markdown
Contributor

Merge Summary:

Posted by merge_spark_pr.py

@nchammas
nchammas deleted the build-status-optimization branch November 10, 2026 13:35
@nchammas

Copy link
Copy Markdown
Contributor Author

Post merge update: This is working nicely! Each sync now takes around 1 - 1.5 minutes. The recent runs showing longer times are just the runner waiting to start; the step itself is quick.

There are some minor improvements possible in relation to workflows that are rerun. I will submit a follow-up PR shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants