Merge pull request #392 from Kobzol/build-cancel

Handle build cancellation errors properly

Author: Kobzol

src/bors/comment.rs

@@ -86,7 +86,7 @@ pub fn no_try_build_in_progress_comment() -> Comment {
     Comment::new(":exclamation: There is currently no try build in progress.".to_string())
 }
 
-pub fn unclean_try_build_cancelled_comment() -> Comment {
+pub fn try_build_cancelled_with_failed_workflow_cancel_comment() -> Comment {
     Comment::new(
         "Try build was cancelled. It was not possible to cancel some workflows.".to_string(),
     )

src/bors/handlers/info.rs

@@ -182,7 +182,7 @@ mod tests {
             - Priority: 10
             - Mergeable: yes
             - Try build is in progress
-            	- Workflow URL: https://github.com/workflows/Workflow1/1
+            	- Workflow URL: https://github.com/rust-lang/borstest/actions/runs/1
             "
             );
             Ok(())

src/bors/handlers/pr_events.rs

@@ -4,14 +4,13 @@ use crate::bors::event::{
     PullRequestMerged, PullRequestOpened, PullRequestPushed, PullRequestReadyForReview,
     PullRequestReopened, PullRequestUnassigned, PushToBranch,
 };
-use crate::bors::handlers::trybuild::cancel_build_workflows;
 use crate::bors::handlers::unapprove_pr;
+use crate::bors::handlers::workflow::{AutoBuildCancelReason, maybe_cancel_auto_build};
 use crate::bors::mergeable_queue::MergeableQueueSender;
 use crate::bors::{Comment, PullRequestStatus, RepositoryState};
-use crate::database::{BuildStatus, MergeableState};
+use crate::database::MergeableState;
 use crate::github::{CommitSha, PullRequestNumber};
 use crate::utils::text::pluralize;
-use octocrab::params::checks::CheckRunConclusion;
 use std::sync::Arc;
 
 pub(super) async fn handle_pull_request_edited(
@@ -55,44 +54,13 @@ pub(super) async fn handle_push_to_pull_request(
 
     mergeable_queue.enqueue(repo_state.repository().clone(), pr_number);
 
-    let mut auto_build_cancel_message: Option<String> = None;
-    if let Some(auto_build) = &pr_model.auto_build {
-        if auto_build.status == BuildStatus::Pending {
-            tracing::info!("Cancelling auto build for PR {pr_number} due to push");
-
-            match cancel_build_workflows(
-                &repo_state.client,
-                db.as_ref(),
-                auto_build,
-                CheckRunConclusion::Cancelled,
-            )
-            .await
-            {
-                Ok(workflow_ids) => {
-                    tracing::info!("Auto build cancelled");
-
-                    let workflow_urls = repo_state
-                        .client
-                        .get_workflow_urls(workflow_ids.into_iter());
-                    auto_build_cancel_message = Some(cancelled_workflows_message(workflow_urls));
-                }
-                Err(error) => {
-                    tracing::error!(
-                        "Could not cancel workflows for SHA {}: {error:?}",
-                        auto_build.commit_sha
-                    );
-
-                    auto_build_cancel_message = Some(unclean_auto_build_cancelled_message());
-                }
-            };
-        }
-
-        tracing::info!(
-            "Deleting auto build {} for PR {pr_number} due to push",
-            auto_build.id
-        );
-        db.delete_auto_build(&pr_model).await?;
-    }
+    let auto_build_cancel_message = maybe_cancel_auto_build(
+        &repo_state.client,
+        &db,
+        &pr_model,
+        AutoBuildCancelReason::PushToPR,
+    )
+    .await?;
 
     if !pr_model.is_approved() {
         return Ok(());
@@ -314,21 +282,6 @@ PR will need to be re-approved."#,
         .await
 }
 
-fn cancelled_workflows_message(workflow_urls: impl Iterator<Item = String>) -> String {
-    let mut comment =
-        r#"Auto build cancelled due to push to branch. Cancelled workflows:"#.to_string();
-    for url in workflow_urls {
-        comment += format!("\n- {}", url).as_str();
-    }
-
-    comment
-}
-
-fn unclean_auto_build_cancelled_message() -> String {
-    "Auto build was cancelled due to push to branch. It was not possible to cancel some workflows."
-        .to_string()
-}
-
 #[cfg(test)]
 mod tests {
     use octocrab::params::checks::{CheckRunConclusion, CheckRunStatus};
@@ -758,32 +711,6 @@ mod tests {
         .await;
     }
 
-    #[sqlx::test]
-    async fn delete_completed_auto_build_on_push(pool: sqlx::PgPool) {
-        BorsBuilder::new(pool)
-            .github(gh_state_with_merge_queue())
-            .run_test(async |tester| {
-                tester.post_comment("@bors r+").await?;
-                tester.expect_comments(1).await;
-                tester.process_merge_queue().await;
-                tester.expect_comments(1).await;
-                tester
-                    .wait_for_default_pr(|pr| pr.auto_build.is_some())
-                    .await?;
-                tester.workflow_full_success(tester.auto_branch()).await?;
-                tester.expect_comments(1).await;
-                tester
-                    .push_to_pr(default_repo_name(), default_pr_number())
-                    .await?;
-                tester.expect_comments(1).await;
-                tester
-                    .wait_for_default_pr(|pr| pr.auto_build.is_none())
-                    .await?;
-                Ok(())
-            })
-            .await;
-    }
-
     #[sqlx::test]
     async fn cancel_pending_auto_build_on_push_comment(pool: sqlx::PgPool) {
         BorsBuilder::new(pool)
@@ -804,7 +731,8 @@ mod tests {
                 :warning: A new commit `pr-1-commit-1` was pushed to the branch, the
                 PR will need to be re-approved.
 
-                Auto build cancelled due to push to branch. Cancelled workflows:
+                Auto build cancelled due to push. Cancelled workflows:
+
                 - https://github.com/rust-lang/borstest/actions/runs/1
                 ");
                 Ok(())
@@ -834,7 +762,7 @@ mod tests {
                 :warning: A new commit `pr-1-commit-1` was pushed to the branch, the
                 PR will need to be re-approved.
 
-                Auto build was cancelled due to push to branch. It was not possible to cancel some workflows.
+                Auto build cancelled due to push. It was not possible to cancel some workflows.
                 ");
                 Ok(())
             })

src/bors/handlers/refresh.rs

@@ -9,8 +9,9 @@ use std::collections::BTreeMap;
 use crate::bors::PullRequestStatus;
 use crate::bors::RepositoryState;
 use crate::bors::comment::build_timed_out_comment;
-use crate::bors::handlers::trybuild::cancel_build_workflows;
+use crate::bors::handlers::workflow::{CancelBuildError, cancel_build};
 use crate::bors::mergeable_queue::MergeableQueueSender;
+use crate::database::{BuildModel, BuildStatus};
 use crate::{PgDbClient, TeamApiClient};
 
 /// Go through pending builds and figure out if we need to do something about them:
@@ -20,33 +21,61 @@ pub async fn refresh_pending_builds(
     db: &PgDbClient,
 ) -> anyhow::Result<()> {
     let running_builds = db.get_pending_builds(repo.repository()).await?;
-    tracing::info!("Found {} running build(s)", running_builds.len());
+    tracing::info!("Found {} pending build(s)", running_builds.len());
 
     let timeout = repo.config.load().timeout;
     for build in running_builds {
-        if elapsed_time(build.created_at) >= timeout {
-            if let Some(pr) = db.find_pr_by_build(&build).await? {
-                tracing::info!("Cancelling build {build:?}");
-                if let Err(error) =
-                    cancel_build_workflows(&repo.client, db, &build, CheckRunConclusion::TimedOut)
-                        .await
-                {
+        if let Err(error) = refresh_build(&repo, db, &build, timeout).await {
+            tracing::error!("Could not refresh pending build {build:?}: {error:?}");
+        }
+    }
+    Ok(())
+}
+
+async fn refresh_build(
+    repo: &RepositoryState,
+    db: &PgDbClient,
+    build: &BuildModel,
+    timeout: Duration,
+) -> anyhow::Result<()> {
+    if elapsed_time(build.created_at) >= timeout {
+        if let Some(pr) = db.find_pr_by_build(build).await? {
+            tracing::info!("Cancelling build {build:?}");
+            match cancel_build(&repo.client, db, build, CheckRunConclusion::TimedOut).await {
+                Ok(_) => {}
+                Err(
+                    CancelBuildError::FailedToMarkBuildAsCancelled(error)
+                    | CancelBuildError::FailedToCancelWorkflows(error),
+                ) => {
                     tracing::error!(
                         "Could not cancel workflows for SHA {}: {error:?}",
                         build.commit_sha
                     );
                 }
+            }
 
-                if let Err(error) = repo
-                    .client
-                    .post_comment(pr.number, build_timed_out_comment(timeout))
-                    .await
-                {
-                    tracing::error!("Could not send comment to PR {}: {error:?}", pr.number);
-                }
-            } else {
-                tracing::warn!("No PR found for build {build:?}");
+            if let Err(error) = repo
+                .client
+                .post_comment(pr.number, build_timed_out_comment(timeout))
+                .await
+            {
+                tracing::error!("Could not send comment to PR {}: {error:?}", pr.number);
             }
+        } else {
+            // This is an orphaned build. It should never be created, unless we have some bug or
+            // unexpected race condition in bors.
+            // When we do encounter such a build, we can mark it as timeouted, as it is no longer
+            // relevant.
+            // Note that we could write an explicit query for finding these orphaned builds,
+            // but that could be quite expensive. Instead we piggyback on the existing logic
+            // for timed out builds; if a build is still pending and has no PR attached, then
+            // there likely won't be any additional event that could mark it as finished.
+            // So eventually all such builds will arrive here
+            tracing::warn!(
+                "Detected orphaned pending without a PR, marking it as time outed: {build:?}"
+            );
+            db.update_build_status(build, BuildStatus::Timeouted)
+                .await?;
         }
     }
     Ok(())

src/bors/handlers/review.rs

@@ -1,7 +1,5 @@
 use std::sync::Arc;
 
-use octocrab::params::checks::CheckRunConclusion;
-
 use crate::bors::RepositoryState;
 use crate::bors::command::RollupMode;
 use crate::bors::command::{Approver, CommandPrefix};
@@ -10,14 +8,14 @@ use crate::bors::comment::{
     approved_comment, delegate_comment, delegate_try_builds_comment, unapprove_non_open_pr_comment,
 };
 use crate::bors::handlers::labels::handle_label_trigger;
-use crate::bors::handlers::trybuild::cancel_build_workflows;
+use crate::bors::handlers::workflow::{AutoBuildCancelReason, maybe_cancel_auto_build};
 use crate::bors::handlers::{PullRequestData, deny_request};
 use crate::bors::handlers::{has_permission, unapprove_pr};
 use crate::bors::merge_queue::MergeQueueSender;
 use crate::bors::{Comment, PullRequestStatus};
+use crate::database::ApprovalInfo;
 use crate::database::DelegatedPermission;
 use crate::database::TreeState;
-use crate::database::{ApprovalInfo, BuildStatus};
 use crate::github::LabelTrigger;
 use crate::github::{GithubUser, PullRequestNumber};
 use crate::permissions::PermissionType;
@@ -137,41 +135,13 @@ pub(super) async fn command_unapprove(
         return Ok(());
     }
 
-    let mut auto_build_cancel_message: Option<String> = None;
-    if let Some(auto_build) = &pr.db.auto_build {
-        if auto_build.status != BuildStatus::Pending {
-            return Ok(());
-        }
-
-        tracing::info!("Cancelling auto build for PR {pr_num} due to push");
-
-        match cancel_build_workflows(
-            &repo_state.client,
-            db.as_ref(),
-            auto_build,
-            CheckRunConclusion::Cancelled,
-        )
-        .await
-        {
-            Ok(workflow_ids) => {
-                tracing::info!("Auto build cancelled");
-
-                let workflow_urls = repo_state
-                    .client
-                    .get_workflow_urls(workflow_ids.into_iter());
-                auto_build_cancel_message = Some(auto_build_cancelled_message(workflow_urls).await);
-            }
-            Err(error) => {
-                tracing::error!(
-                    "Could not cancel workflows for SHA {}: {error:?}",
-                    auto_build.commit_sha
-                );
-
-                auto_build_cancel_message = Some(unclean_auto_build_cancelled_message().await);
-            }
-        };
-    }
-
+    let auto_build_cancel_message = maybe_cancel_auto_build(
+        &repo_state.client,
+        &db,
+        &pr.db,
+        AutoBuildCancelReason::Unapproval,
+    )
+    .await?;
     unapprove_pr(&repo_state, &db, &pr.db).await?;
     notify_of_unapproval(&repo_state, pr, auto_build_cancel_message).await?;
 
@@ -392,20 +362,6 @@ async fn notify_of_delegation(
     repo.client.post_comment(pr_number, comment).await
 }
 
-async fn auto_build_cancelled_message(workflow_urls: impl Iterator<Item = String>) -> String {
-    let mut comment = r#"Auto build cancelled due to unapproval. Cancelled workflows:"#.to_string();
-    for url in workflow_urls {
-        comment += format!("\n- {}", url).as_str();
-    }
-
-    comment
-}
-
-async fn unclean_auto_build_cancelled_message() -> String {
-    "Auto build was cancelled due to unapproval. It was not possible to cancel some workflows."
-        .to_string()
-}
-
 #[cfg(test)]
 mod tests {
     use octocrab::params::checks::{CheckRunConclusion, CheckRunStatus};
@@ -1386,6 +1342,7 @@ merge_queue_enabled = true
                 Commit pr-1-sha has been unapproved.
 
                 Auto build cancelled due to unapproval. Cancelled workflows:
+
                 - https://github.com/rust-lang/borstest/actions/runs/1
                 ");
                 Ok(())
@@ -1415,7 +1372,7 @@ merge_queue_enabled = true
                 insta::assert_snapshot!(tester.get_comment().await?, @r"
                 Commit pr-1-sha has been unapproved.
 
-                Auto build was cancelled due to unapproval. It was not possible to cancel some workflows.
+                Auto build cancelled due to unapproval. It was not possible to cancel some workflows.
                 ");
                 Ok(())
             })