Merge pull request #433 from Kobzol/fix-tree-state-finish-bug

Correctly finish successful auto builds when the tree is closed

Author: Kobzol

.sqlx/query-b1fe02180d44f3cdc04ca0be76163606a84a5ed390079e4786e471b2e8850e49.json renamed to .sqlx/query-92d160378de8ed75192f25e393d1eceee06cd95b22cc6efe813e5d1c9812ee14.json

@@ -137,13 +137,17 @@ Make sure to also run `cargo sqlx migrate run` to apply the migrations to the da
     The goal is to check that we have a test database with production-like data, so that we can test that applying migrations will not produce errors on a non-empty database.
     - If it doesn't make sense to add any data to the migration (e.g. if the migration only adds an index), put `-- Empty to satisfy migration tests` into the file.
 
-### Generate `.sqlx` directory
+### Regenerate `.sqlx` directory
 ```console
+$ rm -rf .sqlx
 $ cargo sqlx prepare -- --all-targets
+$ git add .sqlx
 ```
 
 After that, you should commit the changes to the `.sqlx` directory.
 
+Make sure to remove the `.sqlx` directory before running the `prepare` command, to ensure that leftover queries are not do not remain committed to the repository.
+
 ## Updating commands
 When modifying commands, make sure to update both:
 

docs/development.md

@@ -1045,4 +1045,59 @@ auto_build_failed = ["+foo", "+bar", "-baz"]
         })
         .await;
     }
+
+    #[sqlx::test]
+    async fn finish_auto_build_while_tree_is_closed_1(pool: sqlx::PgPool) {
+        run_test(pool, async |tester: &mut BorsTester| {
+            // Start an auto build with the default priority (0)
+            tester.approve(()).await?;
+            tester.start_auto_build(()).await?;
+
+            // Now close the tree for priority below 100
+            tester.post_comment("@bors treeclosed=100").await?;
+            tester.expect_comments((), 1).await;
+
+            // Then finish the auto build AFTER the tree has been closed and then
+            // run the merge queue
+            tester.finish_auto_build(()).await?;
+
+            // And ensure that the PR was indeed merged
+            tester
+                .get_pr_copy(())
+                .await
+                .expect_status(PullRequestStatus::Merged)
+                .expect_auto_build(|b| b.status == BuildStatus::Success);
+            Ok(())
+        })
+        .await;
+    }
+
+    #[sqlx::test]
+    async fn finish_auto_build_while_tree_is_closed_2(pool: sqlx::PgPool) {
+        run_test(pool, async |tester: &mut BorsTester| {
+            // Start an auto build with the default priority (0)
+            tester.approve(()).await?;
+            tester.start_auto_build(()).await?;
+
+            // Finish the auto build BEFORE the tree has been closed, then close the tree,
+            // and only then run the merge queue
+            tester
+                .workflow_full_success(tester.auto_branch().await)
+                .await?;
+
+            tester.post_comment("@bors treeclosed=100").await?;
+            tester.expect_comments((), 1).await;
+            tester.process_merge_queue().await;
+            tester.expect_comments((), 1).await;
+
+            // And ensure that the PR was indeed merged
+            tester
+                .get_pr_copy(())
+                .await
+                .expect_status(PullRequestStatus::Merged)
+                .expect_auto_build(|b| b.status == BuildStatus::Success);
+            Ok(())
+        })
+        .await;
+    }
 }

src/bors/merge_queue.rs

@@ -970,7 +970,7 @@ pub(crate) async fn update_build_check_run_id(
 /// Fetches pull requests eligible for merge:
 /// - Only approved PRs that are open and mergeable
 /// - Includes only PRs with pending or successful auto builds
-/// - Excludes PRs that do not meet the tree closure priority threshold (if tree closed)
+/// - Excludes non-pending PRs that do not meet the tree closure priority threshold (if tree closed)
 pub(crate) async fn get_merge_queue_prs(
     executor: impl PgExecutor<'_>,
     repo: &GithubRepoName,
@@ -1007,10 +1007,13 @@ pub(crate) async fn get_merge_queue_prs(
               AND pr.status = 'open'
               AND pr.approved_by IS NOT NULL
               AND pr.mergeable_state = 'mergeable'
-              -- Include only PRs with pending or successful auto builds
-              AND (auto_build.status IS NULL OR auto_build.status IN ('pending', 'success'))
-              -- Tree closure check (if tree_priority is set)
-              AND ($2::int IS NULL OR pr.priority >= $2)
+              AND
+                -- We ALWAYS need to return pending and successful PRs, regardless of tree state
+                auto_build.status IN ('pending', 'success') OR (
+                    -- For PRs without a build status, we check if they pass the tree state
+                    -- priority check, if the tree is closed
+                    auto_build.status IS NULL AND ($2::int IS NULL OR pr.priority >= $2)
+                )
             "#,
             repo as &GithubRepoName,
             tree_priority