Merge pull request #363 from Kobzol/check-suite-refactor

Detect completed builds based only on workflow completion events

Author: Kobzol

docs/design.md

@@ -49,10 +49,13 @@ repository:
 (based on the `timeout` configured for the repository), it will cancel them.
 - Reload user permissions from the Team API.
 - Reload `rust-bors.toml` config for the repository from its main branch.
+- Reload the mergeability status of open PRs from GitHub.
+- Sync the status of PRs between the DB and GitHub.
+- Run the merge queue.
 
 ## Concurrency
 The bot is currently listening for GitHub webhooks concurrently, however it handles all commands serially, to avoid
-race conditions. This limitation is expected to be lifted in the future.
+race conditions. This limitation might be lifted in the future.
 
 ## Try builds
 A try build means that you execute a specific CI job on a PR (without merging the PR), to test if the job passes C
@@ -202,6 +205,13 @@ With [homu](https://github.com/rust-lang/homu) (the old bors implementation), Gi
 to use a "fake" job that marked the whole CI workflow as succeeded or failed, to signal to bors if it should consider
 the workflow to be OK or not.
 
-The new bors uses a different approach. It asks GitHub which [check suites](https://docs.github.com/en/rest/checks/suites)
-are attached to a given commit, and then it waits until all of these check suites complete (or until a timeout is reached).
-Thanks to this approach, there is no need to introduce fake CI jobs.
+The new bors uses a different approach. In an earlier implementation, it used to ask GitHub which [check suites](https://docs.github.com/en/rest/checks/suites) were attached to a given commit, and then it waited until all of these check suites were completed (or until a timeout was reached). However, this was too "powerful" for rust-lang/rust, where we currently have only a single CI workflow per commit, and it was producing certain race conditions, where GitHub would send us a webhook that a check suite was completed, but when we then asked the GitHub API about the status of the check suite, it was still marked as pending.
+
+To make the implementation more robust, it now behaves as follow:
+- We listen for the workflow completed webhook.
+- When it is received, we ask GitHub what are all the workflows attached to the workflow's check suite.
+- If all of them are completed, we mark the build as finished.
+
+Note that we explicitly do not read the "Check suite was completed" webhook, because it can actually be received *before* a webhook that tells us that the last workflow of that check suite was completed. If that happens, we could mark a build as completed without knowing the final conclusion of its workflows. That is not a big problem, but it would mean that we sometimes cannot post the real status of a workflow in the "build completed" bors comment on GitHub. So instead we just listen for the completed workflows.
+
+In any case, with new bors there is no need to introduce fake conclusion CI jobs.

docs/development.md

@@ -55,7 +55,7 @@ One-time setup:
   - Configure its private key.
   - Give it permissions for `Actions` (r/w), `Checks` (r/w), `Commit statuses` (r), `Contents` (r/w), `Issues` (r/w) and
   `Pull requests` (r/w).
-  - Subscribe it to webhook events `Check suite`, `Check run`, `Issue comment`, `Issues`, `Pull request`,
+  - Subscribe it to webhook events `Issue comment`, `Issues`, `Pull request`,
     `Pull request review`, `Pull request review comment` and `Workflow run`.
 - Install your GitHub app on some test repository where you want to test bors.
   - Don't forget to configure `rust-bors.toml` in the root of the repository, and also add some example CI workflows.

src/bors/comment.rs

@@ -196,10 +196,10 @@ fn list_workflows_status(workflows: &[WorkflowModel]) -> String {
                 "- [{}]({}) {}",
                 w.name,
                 w.url,
-                if w.status == WorkflowStatus::Success {
-                    ":white_check_mark:"
-                } else {
-                    ":x:"
+                match w.status {
+                    WorkflowStatus::Success => ":white_check_mark:",
+                    WorkflowStatus::Failure => ":x:",
+                    WorkflowStatus::Pending => ":question:",
                 }
             )
         })

src/bors/event.rs

@@ -1,7 +1,7 @@
 use crate::database::{WorkflowStatus, WorkflowType};
 use crate::github::{CommitSha, GithubRepoName, GithubUser, PullRequest, PullRequestNumber};
 use chrono::Duration;
-use octocrab::models::RunId;
+use octocrab::models::{CheckSuiteId, RunId};
 
 #[derive(Debug)]
 pub enum BorsRepositoryEvent {
@@ -31,12 +31,9 @@ pub enum BorsRepositoryEvent {
     /// when a branch is deleted or when a tag is deleted.
     PushToBranch(PushToBranch),
     /// A workflow run on Github Actions or a check run from external CI system has been started.
-    WorkflowStarted(WorkflowStarted),
+    WorkflowStarted(WorkflowRunStarted),
     /// A workflow run on Github Actions or a check run from external CI system has been completed.
-    WorkflowCompleted(WorkflowCompleted),
-    /// A check suite has been completed, either as a workflow run on Github Actions, or as a
-    /// workflow from some external CI system.
-    CheckSuiteCompleted(CheckSuiteCompleted),
+    WorkflowCompleted(WorkflowRunCompleted),
 }
 
 impl BorsRepositoryEvent {
@@ -56,7 +53,6 @@ impl BorsRepositoryEvent {
             BorsRepositoryEvent::PushToBranch(payload) => &payload.repository,
             BorsRepositoryEvent::WorkflowStarted(workflow) => &workflow.repository,
             BorsRepositoryEvent::WorkflowCompleted(workflow) => &workflow.repository,
-            BorsRepositoryEvent::CheckSuiteCompleted(payload) => &payload.repository,
         }
     }
 }
@@ -166,7 +162,7 @@ pub struct PushToBranch {
 }
 
 #[derive(Debug)]
-pub struct WorkflowStarted {
+pub struct WorkflowRunStarted {
     pub repository: GithubRepoName,
     pub name: String,
     pub branch: String,
@@ -177,18 +173,13 @@ pub struct WorkflowStarted {
 }
 
 #[derive(Debug)]
-pub struct WorkflowCompleted {
+pub struct WorkflowRunCompleted {
     pub repository: GithubRepoName,
     pub branch: String,
     pub commit_sha: CommitSha,
     pub run_id: RunId,
     pub status: WorkflowStatus,
     pub running_time: Option<Duration>,
-}
-
-#[derive(Debug)]
-pub struct CheckSuiteCompleted {
-    pub repository: GithubRepoName,
-    pub branch: String,
-    pub commit_sha: CommitSha,
+    /// Check suite to which this workflow is attached.
+    pub check_suite_id: CheckSuiteId,
 }

src/bors/handlers/info.rs

@@ -60,7 +60,7 @@ pub(super) async fn command_info(
 
 #[cfg(test)]
 mod tests {
-    use crate::tests::mocks::{Workflow, WorkflowEvent, run_test};
+    use crate::tests::mocks::{WorkflowEvent, WorkflowRunData, run_test};
 
     #[sqlx::test]
     async fn info_for_unapproved_pr(pool: sqlx::PgPool) {
@@ -156,7 +156,9 @@ mod tests {
             tester.expect_comments(1).await;
 
             tester
-                .workflow_event(WorkflowEvent::started(Workflow::from(tester.try_branch())))
+                .workflow_event(WorkflowEvent::started(WorkflowRunData::from(
+                    tester.try_branch(),
+                )))
                 .await?;
 
             tester.post_comment("@bors info").await?;

src/bors/handlers/mod.rs

@@ -16,9 +16,7 @@ use crate::bors::handlers::review::{
     command_approve, command_close_tree, command_open_tree, command_unapprove,
 };
 use crate::bors::handlers::trybuild::{TRY_BRANCH_NAME, command_try_build, command_try_cancel};
-use crate::bors::handlers::workflow::{
-    handle_check_suite_completed, handle_workflow_completed, handle_workflow_started,
-};
+use crate::bors::handlers::workflow::{handle_workflow_completed, handle_workflow_started};
 use crate::bors::merge_queue::{AUTO_BRANCH_NAME, MergeQueueSender};
 use crate::bors::{BorsContext, Comment, RepositoryState};
 use crate::database::{DelegatedPermission, PullRequestModel};
@@ -123,15 +121,6 @@ pub async fn handle_bors_repository_event(
                 .instrument(span.clone())
                 .await?;
         }
-        BorsRepositoryEvent::CheckSuiteCompleted(payload) => {
-            let span = tracing::info_span!(
-                "Check suite completed",
-                repo = payload.repository.to_string(),
-            );
-            handle_check_suite_completed(repo, db, payload, &merge_queue_tx)
-                .instrument(span.clone())
-                .await?;
-        }
         BorsRepositoryEvent::PullRequestEdited(payload) => {
             let span =
                 tracing::info_span!("Pull request edited", repo = payload.repository.to_string());

src/bors/handlers/refresh.rs

@@ -304,7 +304,7 @@ timeout = 3600
             .run_test(async |tester| {
                 tester.post_comment("@bors try").await?;
                 tester.expect_comments(1).await;
-                tester.start_workflow(tester.try_branch()).await?;
+                tester.workflow_start(tester.try_branch()).await?;
 
                 with_mocked_time(Duration::from_secs(4000), async {
                     tester.cancel_timed_out_builds().await;