Ignore PR CI status when GitHub claims that there are no PR CI workflows for the PR

src/bors/approval_queue.rs

@@ -1,8 +1,7 @@
 use crate::BorsContext;
 use crate::bors::approval::{PrCiStatus, get_pr_ci_status};
 use crate::bors::comment::{
-    tentative_approval_pr_ci_not_started, tentative_approval_removed_comment,
-    tentative_approval_timed_out_comment,
+    tentative_approval_removed_comment, tentative_approval_timed_out_comment,
 };
 use crate::bors::event::WorkflowRunCompleted;
 use crate::bors::handlers::unapprove_pr;
@@ -168,30 +167,34 @@ async fn process_tentative_approval(
     match pr_ci_status {
         PrCiStatus::NotStarted => {
             // Either no PR CI workflows have started yet, or GitHub is full of crap, and it claims
-            // that there are no workflows even if there are some. In any case, if it was too long
-            // since the last push, we time out the build.
-            let Some(head_update_time) = repo
-                .client
-                .get_pull_request_head_update_time(&gh_pr)
-                .await?
-            else {
-                tracing::warn!(
-                    "Could not find the last head update for PR #{} at commit {}",
-                    pr.number,
-                    gh_pr.head.sha
-                );
-                return Ok(());
-            };
-            if elapsed_time_since(head_update_time) >= timeout {
-                unapprove_pr(repo, &ctx.db, pr, &PullRequestInfo::from(gh_pr.clone())).await?;
-                repo.client
-                    .post_comment(
-                        pr.number,
-                        tentative_approval_pr_ci_not_started(&gh_pr.head.sha, timeout),
-                        &ctx.db,
-                    )
-                    .await?;
-            }
+            // that there are no workflows even if there are some. Both cases are problematic:
+            //
+            // If no PR CI workflows have started yet, we can hit a race condition where this
+            // background check runs *just* after the PR was approved, and if the PR has a HEAD
+            // commit that was pushed a long time ago, we can immediately time out the PR CI build
+            // by mistake. We could alleviate that by storing the time of the last approval into the
+            // DB, and measuring timeouts against that, rather than against the latest push.
+            // But we could still be timing out by mistake if GitHub returns us wrong data (see
+            // below).
+            //
+            // If some PR CI workflows have started, but GitHub has some issues, it can tell us that
+            // there are no workflows. In this case, we can time out PR CI by mistake. It seems that
+            // this situation is recoverable though, so waiting for GitHub to recover might not be
+            // a bad thing.
+            //
+            // Rather than timing out tentatively approved PRs by mistake (which we used to do
+            // before), we choose the opposite trade-off: we simply don't do anything if GitHub
+            // returns us an empty list of workflows.
+            // This means that if for some reason there are absolutely no workflows started on the
+            // pushed commit, or if GitHub gets stuck and never returns us any workflows for the
+            // given HEAD SHA, the PR could stay tentatively approved forever.
+            // But given that such a PR will be shown on the queue page, which is checked by people
+            // multiple times daily, that would likely be pretty obvious, and people could then
+            // repush a commit to it.
+            //
+            // If this turns out to be problematic, we could start recording the approval date into
+            // the DB, and then time out the tentative approval if no workflows appear e.g. a whole
+            // day after the approval, or something like that.
         }
         PrCiStatus::Pending { started_at } => {
             // A pending workflow has been running for too long
@@ -333,7 +336,7 @@ mod tests {
     }
 
     #[sqlx::test(migrator = "crate::MIGRATOR")]
-    async fn refresh_rejects_tentative_approval_when_ci_does_not_start(pool: sqlx::PgPool) {
+    async fn refresh_keeps_tentative_approval_when_ci_does_not_start(pool: sqlx::PgPool) {
         run_test(pool, async |ctx: &mut BorsTester| {
             ctx.modify_repo((), |repo| repo.default_pr_ci = false);
             ctx.approve(()).await?;
@@ -343,31 +346,13 @@ mod tests {
             })
             .await;
 
-            insta::assert_snapshot!(ctx.get_next_comment_text(()).await?, @":x: Commit pr-1-sha has been unapproved because PR CI has not started after `7200s`.");
-            ctx.pr(()).await.expect_unapproved();
+            // Don't do anything, because it could be a false positive from GitHub
+            ctx.pr(()).await.expect_tentative_approval();
             Ok(())
         })
         .await;
     }
 
-    #[sqlx::test(migrator = "crate::MIGRATOR")]
-    async fn refresh_confirms_tentative_approval_when_ci_is_restarted(pool: sqlx::PgPool) {
-        run_test(pool, async |ctx: &mut BorsTester| {
-            ctx.modify_repo((), |repo| repo.default_pr_ci = false);
-            ctx.approve(()).await?;
-
-            with_mocked_time(Duration::from_secs(8000), async {
-                ctx.refresh_tentative_approvals().await;
-            })
-                .await;
-
-            insta::assert_snapshot!(ctx.get_next_comment_text(()).await?, @":x: Commit pr-1-sha has been unapproved because PR CI has not started after `7200s`.");
-            ctx.pr(()).await.expect_unapproved();
-            Ok(())
-        })
-            .await;
-    }
-
     #[sqlx::test(migrator = "crate::MIGRATOR")]
     async fn refresh_rejects_tentative_approval_when_ci_is_too_long(pool: sqlx::PgPool) {
         run_test(pool, async |ctx: &mut BorsTester| {

src/bors/comment.rs

@@ -393,13 +393,6 @@ pub fn tentative_approval_timed_out_comment(commit_sha: &CommitSha, timeout: Dur
     ))
 }
 
-pub fn tentative_approval_pr_ci_not_started(commit_sha: &CommitSha, timeout: Duration) -> Comment {
-    Comment::new(format!(
-        ":x: Commit {commit_sha} has been unapproved because PR CI has not started after `{}s`.",
-        timeout.as_secs()
-    ))
-}
-
 pub fn tentative_approval_failed_comment(commit_sha: &CommitSha) -> Comment {
     Comment::new(format!(
         ":x: Cannot approve commit {commit_sha}, because CI currently fails on this PR. Use `@bors r+ force` to override the PR CI check."

src/bors/handlers/info.rs

@@ -1,6 +1,7 @@
-use crate::bors::Comment;
 use crate::bors::RepositoryState;
+use crate::bors::approval::{PrCiStatus, get_pr_ci_status};
 use crate::bors::handlers::PullRequestData;
+use crate::bors::{Comment, now};
 use crate::database::{ApprovalStatus, MergeableState};
 use crate::database::{BuildStatus, PgDbClient};
 use std::sync::Arc;
@@ -16,18 +17,55 @@ pub(super) async fn command_info(
 
     // Approval info
     match &pr.db.approval_status {
-        ApprovalStatus::Approved(info) => writeln!(message, "- Approved by: `{}`", info.approver)?,
-        ApprovalStatus::TentativelyApproved(info) => {
-            writeln!(message, "- Tentatively approved by: `{}`", info.approver)?
-        }
+        ApprovalStatus::Approved(info) => writeln!(
+            message,
+            "- Commit {} approved by: `{}`",
+            info.sha, info.approver
+        )?,
+        ApprovalStatus::TentativelyApproved(info) => writeln!(
+            message,
+            "- Commit {} tentatively approved by: `{}`",
+            info.sha, info.approver
+        )?,
         ApprovalStatus::NotApproved => writeln!(message, "- Not Approved")?,
     }
 
+    // PR CI status
+    let pr_ci_status = get_pr_ci_status(&repo, pr.github).await?;
+    write!(message, "- PR CI status: ")?;
+    match pr_ci_status {
+        PrCiStatus::NotStarted => {
+            writeln!(message, "not started")?;
+        }
+        PrCiStatus::Pending { started_at } => {
+            let now = now();
+            writeln!(
+                message,
+                "pending (elapsed: {:.0}s)",
+                now.signed_duration_since(started_at).as_seconds_f64()
+            )?;
+        }
+        PrCiStatus::Success => {
+            writeln!(message, "successful")?;
+        }
+        PrCiStatus::Failed => {
+            writeln!(message, "failed")?;
+        }
+    }
+
     // Priority info
     if let Some(priority) = pr.db.priority {
         writeln!(message, "- Priority: {priority}")?;
-    } else {
-        writeln!(message, "- Priority: unset")?;
+    }
+
+    // Rollup mode
+    if let Some(rollup) = pr.db.rollup {
+        writeln!(message, "- Rollup mode: {rollup}")?;
+    }
+
+    // Note
+    if let Some(note) = pr.db.note() {
+        writeln!(message, "- Note: {note}")?;
     }
 
     // Mergeability state
@@ -42,12 +80,26 @@ pub(super) async fn command_info(
     )?;
 
     // Try build status
-    if let Some(try_build) = &pr.db.try_build
-        && try_build.status == BuildStatus::Pending
+    if let Some(build) = &pr.db.try_build
+        && build.status == BuildStatus::Pending
     {
         writeln!(message, "- Try build is in progress")?;
 
-        if let Ok(urls) = db.get_workflow_urls_for_build(try_build).await {
+        if let Ok(urls) = db.get_workflow_urls_for_build(build).await {
+            message.extend(
+                urls.into_iter()
+                    .map(|url| format!("\t- Workflow URL: {url}")),
+            );
+        }
+    }
+
+    // Unrolled build status
+    if let Some(build) = &pr.db.unrolled_build
+        && build.status == BuildStatus::Pending
+    {
+        writeln!(message, "- Unrolled build is in progress")?;
+
+        if let Ok(urls) = db.get_workflow_urls_for_build(build).await {
             message.extend(
                 urls.into_iter()
                     .map(|url| format!("\t- Workflow URL: {url}")),
@@ -102,7 +154,7 @@ mod tests {
                 @"
             ## Status of PR `1`
             - Not Approved
-            - Priority: unset
+            - PR CI status: successful
             - Mergeable: yes
             "
             );
@@ -121,8 +173,8 @@ mod tests {
                 ctx.get_next_comment_text(()).await?,
                 @"
             ## Status of PR `1`
-            - Approved by: `default-user`
-            - Priority: unset
+            - Commit pr-1-sha approved by: `default-user`
+            - PR CI status: successful
             - Mergeable: yes
             "
             );
@@ -146,8 +198,8 @@ mod tests {
                 ctx.get_next_comment_text(()).await?,
                 @"
             ## Status of PR `1`
-            - Tentatively approved by: `default-user`
-            - Priority: unset
+            - Commit pr-1-sha tentatively approved by: `default-user`
+            - PR CI status: pending (elapsed: 0s)
             - Mergeable: yes
             "
             );
@@ -168,6 +220,7 @@ mod tests {
                 @"
             ## Status of PR `1`
             - Not Approved
+            - PR CI status: successful
             - Priority: 5
             - Mergeable: yes
             "
@@ -189,7 +242,7 @@ mod tests {
                 @"
             ## Status of PR `1`
             - Not Approved
-            - Priority: unset
+            - PR CI status: successful
             - Mergeable: yes
             - Try build is in progress
             "
@@ -216,7 +269,8 @@ mod tests {
                 ctx.get_next_comment_text(()).await?,
                 @"
             ## Status of PR `1`
-            - Approved by: `default-user`
+            - Commit pr-1-sha approved by: `default-user`
+            - PR CI status: successful
             - Priority: 10
             - Mergeable: yes
             - Try build is in progress
@@ -240,8 +294,8 @@ mod tests {
                 ctx.get_next_comment_text(()).await?,
                 @"
             ## Status of PR `1`
-            - Approved by: `default-user`
-            - Priority: unset
+            - Commit pr-1-sha approved by: `default-user`
+            - PR CI status: successful
             - Mergeable: yes
             - Auto build has been cancelled
             "

src/github/api/client.rs

@@ -5,9 +5,7 @@ use chrono::{DateTime, Utc};
 use octocrab::Octocrab;
 use octocrab::models::checks::CheckRun;
 use octocrab::models::pulls::MergeableState;
-use octocrab::models::repos::ActivityType;
 use octocrab::models::{CheckRunId, Repository, RunId, RunnerGroupId, UserId};
-use octocrab::params::Direction;
 use octocrab::params::checks::{CheckRunConclusion, CheckRunStatus};
 use serde::{Deserialize, Serialize};
 use std::fmt::Debug;
@@ -266,50 +264,6 @@ impl GithubRepositoryClient {
         Ok(prs)
     }
 
-    /// Return when the pull request's current head was last updated.
-    pub async fn get_pull_request_head_update_time(
-        &self,
-        pr: &PullRequest,
-    ) -> anyhow::Result<Option<DateTime<Utc>>> {
-        let head_repository = pr.head_repository.as_ref().unwrap_or(&self.repo_name);
-        let head_branch = pr.head.name.clone();
-        let head_sha = pr.head.sha.clone();
-
-        let update_time = perform_retryable(
-            "get_pull_request_head_update_time",
-            RetryMethod::default(),
-            || async {
-                let activities = self
-                    .client
-                    .repos(head_repository.owner(), head_repository.name())
-                    .list_activities()
-                    .direction(Direction::Descending)
-                    .per_page(100)
-                    .git_ref(&head_branch)
-                    .send()
-                    .await?;
-
-                anyhow::Ok(
-                    activities
-                        .items
-                        .into_iter()
-                        .filter(|activity| {
-                            matches!(
-                                activity.activity_type,
-                                ActivityType::Push
-                                    | ActivityType::ForcePush
-                                    | ActivityType::BranchCreation
-                            ) && activity.after == head_sha.as_ref()
-                        })
-                        .map(|activity| activity.timestamp)
-                        .max(),
-                )
-            },
-        )
-        .await?;
-        Ok(update_time)
-    }
-
     pub async fn get_pull_request_commits(
         &self,
         number: PullRequestNumber,
@@ -401,7 +355,7 @@ impl GithubRepositoryClient {
         branch: &str,
         sha: &CommitSha,
         force: ForcePush,
-    ) -> Result<(), crate::github::api::operations::BranchUpdateError> {
+    ) -> Result<(), BranchUpdateError> {
         perform_retryable("set_branch_to_sha", RetryMethod::default(), || async {
             set_branch_to_commit(self, branch.to_string(), sha, force)
                 .await