Skip to content

[CI] Stop infinite reviewer reassignment, batch state commits, clean up stale state - #40096

Open
Abacn wants to merge 2 commits into
apache:masterfrom
Abacn:pr-bot-ttl
Open

[CI] Stop infinite reviewer reassignment, batch state commits, clean up stale state#40096
Abacn wants to merge 2 commits into
apache:masterfrom
Abacn:pr-bot-ttl

Conversation

@Abacn

@Abacn Abacn commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

…ean up stale state

  • Stop infinite reviewer reassignment loops: label with "awaiting triage" if a PR has both "reassigned-reviewers" and "Next Action: Reviewers", and review started >60 days ago.

  • Track initial reviewer assignment timestamp (reviewersAssignedAt) in persistent PR state, falling back to PR creation time for legacy PRs.

  • Skip reviewer assignment for PRs labeled "awaiting triage" across new PR processing and daily reminder workflows.

  • Batch persistent state updates into a single commit and push per workflow execution instead of committing on every file write.

  • Incrementally prune closed PR state files (oldest 100 per daily run) from the pr-bot-state branch.

  • Document core PR bot logic in scripts/ci/pr-bot/README.md.

Please add a meaningful description for your change here


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests
Go tests

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

…ean up stale state

* Stop infinite reviewer reassignment loops: label with "awaiting triage" if a PR has both "reassigned-reviewers" and "Next Action: Reviewers", and review started >60 days ago.

* Track initial reviewer assignment timestamp (reviewersAssignedAt) in persistent PR state, falling back to PR creation time for legacy PRs.

* Skip reviewer assignment for PRs labeled "awaiting triage" across new PR processing and daily reminder workflows.

* Batch persistent state updates into a single commit and push per workflow execution instead of committing on every file write.

* Incrementally prune closed PR state files (oldest 100 per daily run) from the pr-bot-state branch.

* Document core PR bot logic in scripts/ci/pr-bot/README.md.
@Abacn Abacn changed the title [CI] Stop infinite reviewer reassignment, batch state commits, and cl… [CI] Stop infinite reviewer reassignment, batch state commits, clean up stale state Sep 10, 2026
@Abacn
Abacn marked this pull request as ready for review September 10, 2026 18:42
@Abacn

Abacn commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

R: @kennknowles @damccorm

@github-actions

Copy link
Copy Markdown
Contributor

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment assign set of reviewers

@damccorm

Copy link
Copy Markdown
Contributor

Batch persistent state updates into a single commit and push per workflow execution instead of committing on every file write.

I think I didn't do this because it increases the chances of conflicts and the impact of them (e.g. if multiple files should've been written to)

@damccorm

Copy link
Copy Markdown
Contributor

Other than that, this set of changes SGTM. With that said, I think Kenn is trying to reimagine some of this in #39980

@damccorm damccorm 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.

Functionally, this LGTM as well

@damccorm

Copy link
Copy Markdown
Contributor

So I think my only feedback is that I think we should keep the previous commit semantics (AFAIK they haven't caused any issues, but let me know if that's wrong)

@Abacn

Abacn commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Batch persistent state updates into a single commit and push per workflow execution instead of committing on every file write.

I think I didn't do this because it increases the chances of conflicts and the impact of them (e.g. if multiple files should've been written to)

I did this because I realize we commit multiple times during a single run of the workflow, e.g. in processNewPrs.ts, a single PR with 2 labels triggers 3 separate commits (writePrState + 2 $\times$ writeReviewersForLabelState) seconds apart. We now record a timestamp on PR creation and will make it worse.

To avoid merge conflict we can add the 3 pr-bot workflow into a concurrency_group (pr-bot-prs-needing-attention.yml, pr-bot-pr-updates.yml, pr-bot-new-prs.yml). Checking running history all of them are running for 1-2 minutes. If number of PR comments is less than "~50" per hour it's managable.

@damccorm

Copy link
Copy Markdown
Contributor

I did this because I realize we commit multiple times during a single run of the workflow, e.g. in processNewPrs.ts, a single PR with 2 labels triggers 3 separate commits (writePrState + 2 × writeReviewersForLabelState) seconds apart. We now record a timestamp on PR creation and will make it worse.

Does this actually cause problems though? 2 (or 10) small commits are not really meaningfully different than a single large commit here.

To avoid merge conflict we can add the 3 pr-bot workflow into a concurrency_group (pr-bot-prs-needing-attention.yml, pr-bot-pr-updates.yml, pr-bot-new-prs.yml). Checking running history all of them are running for 1-2 minutes. If number of PR comments is less than "~50" per hour it's managable.

This seems less desirable to me. In particular, there are some issues with slow pr-bot-pr-updates. As an example, consider:

  1. pr-bot-new-prs gets triggered but queued
  2. I comment R: foo and mark my PR ready for review (not draft)

Today, there is a small chance of the PR getting assigned before the R: comment is processed (you'd need pr-bot-new-prs to have pathological timing). If there is backlog, the odds get much worse with this change.

I think this is ok if there is a problem we're solving, but I don't think we're actually seeing poor behavior.

@Abacn

Abacn commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

How about this (latest commit): the commit remains per file, however it now batches state changes for single PR (in findPrsNeedingAttention, processNewPrs, pr-updates' assignToNextReviewer and assignReviewerSet) so the number of commit is reduced by 3x while conflict remains unlikely (because they remains single PR)

@damccorm

Copy link
Copy Markdown
Contributor

Stepping back for a moment - what problem are you trying to solve? Is having many commits a problem?

The only downside I'm aware of is that too many commits (at huge scale) can slow down git operations. But currently checking out the branch takes ~1 second, so I don't think we're in danger of having problems there.

I guess each commit induces some latency, but these workflows are running in ~1 minute (with the time dominated by other GitHub API calls).

@Abacn

Abacn commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

Yes there is a concern of this change make more commits and worsen the time cost over time. Checked an example run: https://github.com/apache/beam/actions/runs/34609044939/job/103294594744

Fri, 11 Sep 2026 14:15:55 GMT /usr/bin/git fetch origin pr-bot-state
Fri, 11 Sep 2026 14:16:29 GMT From https://github.com/apache/beam
Fri, 11 Sep 2026 14:16:29 GMT * branch                  pr-bot-state -> FETCH_HEAD
Fri, 11 Sep 2026 14:16:29 GMT * [new branch]            pr-bot-state -> origin/pr-bot-state
Fri, 11 Sep 2026 14:16:37 GMT /usr/bin/git checkout pr-bot-state
Fri, 11 Sep 2026 14:16:39 GMT Switched to a new branch 'pr-bot-state'

checkout itself takes 2 s (39 - 37) but fetch the branch pr-bot-state currently takes 42 s (16:37 - 15:55).

https://github.com/apache/beam/tree/pr-bot-state says "This branch is 52776 commits ahead of and 13373 commits behind master."

fetch and checkout onto pr-bot-state already takes half of the time of a typical run( while the initialization that checkout onto master takes 11 s)

@damccorm

Copy link
Copy Markdown
Contributor

Oh that's true, I was looking at the wrong log line as the base. With that said, I think it is actually because of the fetch depth. The initial checkout does fetch-depth=1 by default (https://github.com/actions/checkout/blob/f548e57e544e1ff5a4c46bf1e1b8685f8e4a348a/action.yml#L74)

On a fresh machine, I tried:

  • git clone https://github.com/apache/beam --depth=1
  • time git fetch origin pr-bot-state (took 1m10s)

Then I tried:

  • git clone https://github.com/apache/beam --depth=1
  • time git fetch origin pr-bot-state --depth=10 (took 5s) - --depth=1 is even faster

So I think if we want to drive down fetch times, we should just reduce the fetch depth.

Since we don't have real locking semantics, I'm interested in keeping the bot state as close as possible to the real state.

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.

2 participants