Synchorize pr state periodically #276

geetanshjuneja · 2025-04-27T13:11:44Z

Closes #266.

Kobzol

Thank you! Left a few comments. I'll take a look at refactoring the refresh handlers, to see if it makes sense.

Kobzol · 2025-04-30T07:44:18Z

src/github/api/client.rs

@@ -319,6 +320,27 @@ impl GithubRepositoryClient {
        run_ids.map(|workflow_id| self.get_workflow_url(workflow_id))
    }

+    pub async fn fetch_prs(&self) -> anyhow::Result<Vec<PullRequest>> {


This will download all PRs in the repo, which is more than 80 thousand PRs on rust-lang/rust. We should only download the open/draft (if it's possible to filter for both in one API call) PRs.

Then when we find a PR that is not amongst the open ones, we assume that it is closed (in theory it could also be merged, but bors does the merging, so it should not miss that event).

It could also happen that open and close webhook of a pr is lost and therefore this pr has no trace of it in db. Also while refreshing pr state we will only download open/draft prs, so still there is no trace of it in the db.

That's fine. If we miss some PR that was opened and then closed while bors wasn't running, it doesn't really matter, we don't care about closed PRs. We are only interested in open PRs waiting to be merged.
These situations should be quite rare anyway.

I need some help in implementation of this logic. So we have two vectors nonclosed_gh_pull_request and nonclosed_db_pull_requests.
For prs that are in nonclosed_gh_pull_request and not in nonclosed_db_pull_requests. I have to upsert prs in the db.
For prs that are in nonclosed_db_pull_request and not in nonclosed_gh_pull_requests. I have to mark their state close in db.

I was trying to implement this using BTreeset and use set difference. Is this fine or can I do better.

You implemented the logic well, thank you! :)

Kobzol · 2025-04-30T07:52:23Z

src/bin/bors.rs

@@ -18,6 +18,8 @@ use tracing_subscriber::filter::EnvFilter;

 /// How often should the bot check DB state, e.g. for handling timeouts.
 const PERIODIC_REFRESH: Duration = Duration::from_secs(120);
+/// How often should the bot synchronize PR state.
+const PR_STATE_PERIODIC_REFRESH: Duration = Duration::from_secs(60 * 60);


I'm not sure if we need to separate this from the normal periodic refresh, because PRs are also per repository. So it's weird to have a separate event for it.

That being said, it should have a different update frequency than the refresh of configs and permissions. We could have a single outside trigger for the refresh logic, that would "tick" e.g. every minute, and the refresh code could then remember the most recent timestamps for the individual refresh logic handlers. But this would be a bit annoying in the tests, as we'd need to override the period between refreshes, rather than just triggering the refresh with a mocked event. Or maybe we could do something like Event::RefreshPrs { force: bool } to enable us to force the refresh in tests.

I will split the refresh handlers into individual events in a separate PR, and then we'll rebase this one on top of that.

Kobzol · 2025-04-30T07:52:42Z

src/tests/mocks/bors.rs

@@ -447,22 +472,25 @@ impl BorsTester {
        &mut self,
        repo_name: GithubRepoName,
        pr_number: u64,
+        send_webhook: bool,


Why should this ever be false?

Will fix test refresh_pr_with_status_closed in which a pr's status is closed in gh but not in db. So false here is req for that case.

Then let's just modify the mock GH state in the test directly, to avoid having to pass true/false everywhere.

Can you elaborate more on this? Which specific struct in test you are talking about?

I mean to either copy-paste the logic for creating a PR in the mock GH database into the test, or creating a separate function that will only create the PR on GH, but not send the webhook. It's quite hard to read the false, false etc. parameters to open_pr in tests.

But sometimes in tests I need only open webhook to work.

let pr = tester.open_pr(default_repo_name(), false).await?; tester.block_webhooks(async move { tester .close_pr(default_repo_name(), pr.number.0) .await }).await?;

But this would require BorsTester to be cloned.

Oh, right, this was a bit tricky until recently. Luckily, we now finally have async closures! :) I pushed an implementation of with_blocked_webhooks.

Thanks for the help. I didn't knew about async closures and now will look into it.

Kobzol · 2025-04-30T07:53:30Z

src/bors/handlers/refresh.rs

+    let prs = repo.client.fetch_prs().await?;
+    let pr_to_status = db.get_all_pull_request_status(repo_name).await?;
+
+    for pr in prs {


As noted in my other comment, and in the original issue, we cannot simply download all PRs from GitHub, there's too many. We only have to fetch the open ones and synchronize smartly based on that.

Kobzol · 2025-04-30T11:43:44Z

I split the handlers in #278.

Kobzol · 2025-05-05T07:02:31Z

src/bors/handlers/refresh.rs

+    }
+    // PRs that are closed in GitHub but not in the DB. In theory PR could also be merged
+    // but bors does the merging so it should not happen.
+    for pr_num in nonclosed_db_prs_num.keys() {


There is a potential race condition here, where if the PR just got merged on GH in between the GH API and DB calls, we would mark it as closed even though it was merged. But let's not deal with that now.

If the PR just got merged b/w GH API and DB calls, then GH would send a webhook which will be processed after it changing the status to merged.

I was thinking something like this:
Bors merges a PR. Before setting its state to merged, this refresh handler runs and loads the PR as open from GH. Then bors sets the local state as merged, but it gets overwritten by this refresh handler. This is possible because the refresh handler can run concurrently with command handlers.

That being said, this is incredibly unlikely to happen, and even if we did, the state should be correctly synchronized during the next refresh, or during the next comment sent on the PR, so it should be fine.

Kobzol · 2025-05-05T07:02:49Z

src/bors/handlers/refresh.rs

@@ -139,6 +208,15 @@ mod tests {
        .await;
    }

+    #[sqlx::test]
+    async fn refresh_without_out_of_sync_prs(pool: sqlx::PgPool) {


refresh_pr_state

Kobzol · 2025-05-05T07:03:51Z

src/tests/mocks/bors.rs

@@ -447,22 +472,25 @@ impl BorsTester {
        &mut self,
        repo_name: GithubRepoName,
        pr_number: u64,
+        send_webhook: bool,


I mean to either copy-paste the logic for creating a PR in the mock GH database into the test, or creating a separate function that will only create the PR on GH, but not send the webhook. It's quite hard to read the false, false etc. parameters to open_pr in tests.

Kobzol · 2025-05-05T07:04:55Z

src/tests/mocks/bors.rs

@@ -447,22 +472,25 @@ impl BorsTester {
        &mut self,
        repo_name: GithubRepoName,
        pr_number: u64,
+        send_webhook: bool,


Actually, I think that we could solve this in a more general way, by creating something like this:

tester.block_webhooks(async move { // While this future runs, no webhooks will be sent });

Kobzol · 2025-05-05T07:05:18Z

src/github/api/client.rs

@@ -319,6 +320,27 @@ impl GithubRepositoryClient {
        run_ids.map(|workflow_id| self.get_workflow_url(workflow_id))
    }

+    pub async fn fetch_prs(&self) -> anyhow::Result<Vec<PullRequest>> {


You implemented the logic well, thank you! :)

Kobzol

Thanks! Left one nit, once you remove the send_webhook parameter, please squash the commits and we can merge this.

Kobzol · 2025-05-05T09:40:55Z

src/bors/handlers/pr_events.rs

@@ -466,7 +466,9 @@ mod tests {
                    pr.pr_status == PullRequestStatus::Open
                })
                .await?;
-            tester.merge_pr(default_repo_name(), pr.number.0).await?;
+            tester
+                .merge_pr(default_repo_name(), pr.number.0, true)


I missed this function, let's also remove send_webhook: bool, from its parameters, now that we have a better way of blocking webhooks.

Kobzol · 2025-05-05T09:46:19Z

src/bors/handlers/refresh.rs

+    }
+    // PRs that are closed in GitHub but not in the DB. In theory PR could also be merged
+    // but bors does the merging so it should not happen.
+    for pr_num in nonclosed_db_prs_num.keys() {


I was thinking something like this:
Bors merges a PR. Before setting its state to merged, this refresh handler runs and loads the PR as open from GH. Then bors sets the local state as merged, but it gets overwritten by this refresh handler. This is possible because the refresh handler can run concurrently with command handlers.

That being said, this is incredibly unlikely to happen, and even if we did, the state should be correctly synchronized during the next refresh, or during the next comment sent on the PR, so it should be fine.

cargo sqlx prepare Add `with_blocked_webhooks` remove send_webhook flag

Kobzol

Thank you! :)

geetanshjuneja closed this Apr 27, 2025

geetanshjuneja reopened this Apr 27, 2025

Kobzol reviewed Apr 30, 2025

View reviewed changes

geetanshjuneja force-pushed the sync branch from 2dba623 to 4eeba1d Compare May 4, 2025 04:31

geetanshjuneja requested a review from Kobzol May 4, 2025 05:41

Kobzol reviewed May 5, 2025

View reviewed changes

geetanshjuneja force-pushed the sync branch 2 times, most recently from 27e8902 to 97350ba Compare May 5, 2025 11:35

Synchorize pr state periodically

4185fb9

cargo sqlx prepare Add `with_blocked_webhooks` remove send_webhook flag

geetanshjuneja force-pushed the sync branch from 97350ba to 4185fb9 Compare May 5, 2025 11:39

Kobzol approved these changes May 5, 2025

View reviewed changes

Kobzol added this pull request to the merge queue May 5, 2025

Merged via the queue into rust-lang:main with commit 48c30e3 May 5, 2025
2 checks passed

Synchorize pr state periodically #276

Synchorize pr state periodically #276

Uh oh!

Conversation

geetanshjuneja commented Apr 27, 2025

Uh oh!

Kobzol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geetanshjuneja May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geetanshjuneja May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geetanshjuneja May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kobzol May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geetanshjuneja May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geetanshjuneja May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kobzol commented Apr 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geetanshjuneja May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kobzol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geetanshjuneja May 2, 2025 •

edited

Loading

geetanshjuneja May 2, 2025 •

edited

Loading

geetanshjuneja May 2, 2025 •

edited

Loading

Kobzol May 5, 2025 •

edited

Loading

geetanshjuneja May 5, 2025 •

edited

Loading

geetanshjuneja May 5, 2025 •

edited

Loading

geetanshjuneja May 5, 2025 •

edited

Loading