Refactor computing build duration and add a test

src/bors/build.rs

@@ -1,6 +1,6 @@
 use crate::PgDbClient;
 use crate::bors::{RepositoryState, WorkflowRun};
-use crate::database::{BuildModel, BuildStatus, WorkflowModel};
+use crate::database::{BuildModel, BuildStatus, UpdateBuildParams, WorkflowModel};
 use crate::github::CommitSha;
 use crate::github::api::client::GithubRepositoryClient;
 use octocrab::models::CheckRunId;
@@ -50,7 +50,7 @@ pub async fn cancel_build(
         CancelBuildConclusion::Timeout => BuildStatus::Timeouted,
         CancelBuildConclusion::Cancel => BuildStatus::Cancelled,
     };
-    db.update_build_column(build, status, None)
+    db.update_build(build.id, UpdateBuildParams::default().status(status))
         .await
         .map_err(CancelBuildError::FailedToMarkBuildAsCancelled)?;
 
@@ -156,6 +156,12 @@ pub async fn load_workflow_runs(
                 name: db_run.name,
                 url: db_run.url,
                 status: db_run.status,
+                // This is not really the time the workflow started running, but rather when we
+                // inserted it into the DB. But that hopefully should not matter, as the duration is
+                // `None` and this should never occur anyway :)
+                created_at: db_run.created_at,
+                // We currently do not store workflow duration in the DB
+                duration: None,
             });
         }
     }

src/bors/build_queue.rs

@@ -21,7 +21,9 @@ use crate::bors::merge_queue::MergeQueueSender;
 use crate::bors::{
     BuildKind, FailedWorkflowRun, RepositoryState, elapsed_time_since, hide_tagged_comments,
 };
-use crate::database::{BuildModel, BuildStatus, PullRequestModel, WorkflowStatus};
+use crate::database::{
+    BuildModel, BuildStatus, PullRequestModel, UpdateBuildParams, WorkflowStatus,
+};
 use crate::github::{CommitSha, GithubRepoName, LabelTrigger};
 use crate::{BorsContext, PgDbClient};
 use anyhow::Context;
@@ -95,16 +97,8 @@ pub async fn handle_build_queue_event(
                         // First try to complete builds, and only then timeout then
                         // Because if the bot was offline for some time, we want to first attempt to
                         // actually finish the build, otherwise it might get instantly timeouted.
-                        if !maybe_complete_build(
-                            &repo,
-                            db,
-                            &build,
-                            &pr,
-                            &merge_queue_tx,
-                            None,
-                            None,
-                        )
-                        .await?
+                        if !maybe_complete_build(&repo, db, &build, &pr, &merge_queue_tx, None)
+                            .await?
                         {
                             maybe_timeout_build(&repo, db, &build, &pr, timeout).await?;
                         }
@@ -121,8 +115,11 @@ pub async fn handle_build_queue_event(
                         tracing::warn!(
                             "Detected orphaned pending without a PR, marking it as timeouted: {build:?}"
                         );
-                        db.update_build_column(&build, BuildStatus::Timeouted, None)
-                            .await?;
+                        db.update_build(
+                            build.id,
+                            UpdateBuildParams::default().status(BuildStatus::Timeouted),
+                        )
+                        .await?;
                     }
                     anyhow::Ok(())
                 };
@@ -156,7 +153,6 @@ pub async fn handle_build_queue_event(
                 &pr,
                 &merge_queue_tx,
                 Some(CompletionTrigger { error_context }),
-                event.running_time,
             )
             .await?;
         }
@@ -228,7 +224,6 @@ async fn maybe_complete_build(
     pr: &PullRequestModel,
     merge_queue_tx: &MergeQueueSender,
     completion_trigger: Option<CompletionTrigger>,
-    running_time: Option<chrono::Duration>,
 ) -> anyhow::Result<bool> {
     assert_eq!(
         build.status,
@@ -309,7 +304,24 @@ async fn maybe_complete_build(
         }),
     };
 
-    db.update_build_column(build, status, running_time).await?;
+    let compute_duration = || {
+        // Compute the time when the earliest workflow started, and when the latest workflow ended
+        let start = workflow_runs.iter().map(|run| run.created_at).min()?;
+        let end = workflow_runs
+            .iter()
+            .filter_map(|run| run.duration.map(|d| run.created_at + d))
+            .max()?;
+
+        // The build duration is the difference between those two
+        (end - start).to_std().ok()
+    };
+    db.update_build(
+        build.id,
+        UpdateBuildParams::default()
+            .status(status)
+            .duration(compute_duration()),
+    )
+    .await?;
     if let Some(trigger) = trigger {
         let pr = repo.client.get_pull_request(pr_num).await?;
         handle_label_trigger(repo, &pr, trigger).await?;

src/bors/handlers/trybuild.rs

@@ -15,7 +15,9 @@ use crate::bors::{
     MergeType, RepositoryState, TRY_BRANCH_NAME, bors_commit_author, create_merge_commit_message,
     hide_tagged_comments,
 };
-use crate::database::{BuildModel, BuildStatus, ExclusiveOperationOutcome, PullRequestModel};
+use crate::database::{
+    BuildModel, BuildStatus, ExclusiveOperationOutcome, PullRequestModel, UpdateBuildParams,
+};
 use crate::github::api::client::{CheckRunOutput, GithubRepositoryClient};
 use crate::github::api::operations::ForcePush;
 use crate::github::{CommitSha, GithubUser, PullRequestNumber};
@@ -139,9 +141,10 @@ pub(super) async fn command_try_build(
                         .await
                     {
                         Ok(check_run) => {
-                            db.update_build_check_run_id(
+                            db.update_build(
                                 build_id,
-                                check_run.id.into_inner() as i64,
+                                UpdateBuildParams::default()
+                                    .check_run_id(check_run.id.into_inner() as i64),
                             )
                             .await?;
                         }

src/bors/handlers/workflow.rs

@@ -221,9 +221,10 @@ fn auto_build_cancelled_msg(
 mod tests {
     use std::time::Duration;
 
-    use crate::database::WorkflowStatus;
     use crate::database::operations::get_all_workflows;
-    use crate::tests::{BorsBuilder, BorsTester, GitHub};
+    use crate::database::{PgDuration, WorkflowStatus};
+    use crate::github::CommitSha;
+    use crate::tests::{BorsBuilder, BorsTester, GitHub, default_repo_name};
     use crate::tests::{WorkflowEvent, run_test};
 
     #[sqlx::test]
@@ -442,4 +443,58 @@ min_ci_time = 20
             })
             .await;
     }
+
+    #[sqlx::test]
+    async fn build_success_duration(pool: sqlx::PgPool) {
+        run_test(pool, async |ctx: &mut BorsTester| {
+            ctx.post_comment("@bors try").await?;
+            ctx.expect_comments((), 1).await;
+
+            let w1 = ctx.try_workflow();
+            ctx.modify_workflow(w1, |w| w.set_duration(Duration::from_secs(100)));
+            ctx.workflow_full_success(w1).await?;
+            ctx.expect_comments((), 1).await;
+
+            let build = ctx
+                .db()
+                .find_build(
+                    &default_repo_name(),
+                    ctx.try_branch().name(),
+                    CommitSha(ctx.try_branch().sha()),
+                )
+                .await?
+                .expect("Build not found");
+            assert_eq!(build.duration, Some(PgDuration(Duration::from_secs(100))));
+
+            Ok(())
+        })
+        .await;
+    }
+
+    #[sqlx::test]
+    async fn build_failed_duration(pool: sqlx::PgPool) {
+        run_test(pool, async |ctx: &mut BorsTester| {
+            ctx.post_comment("@bors try").await?;
+            ctx.expect_comments((), 1).await;
+
+            let w1 = ctx.try_workflow();
+            ctx.modify_workflow(w1, |w| w.set_duration(Duration::from_secs(100)));
+            ctx.workflow_full_failure(w1).await?;
+            ctx.expect_comments((), 1).await;
+
+            let build = ctx
+                .db()
+                .find_build(
+                    &default_repo_name(),
+                    ctx.try_branch().name(),
+                    CommitSha(ctx.try_branch().sha()),
+                )
+                .await?
+                .expect("Build not found");
+            assert_eq!(build.duration, Some(PgDuration(Duration::from_secs(100))));
+
+            Ok(())
+        })
+        .await;
+    }
 }

src/bors/merge_queue.rs

@@ -18,7 +18,7 @@ use crate::bors::mergeability_queue::{MergeabilityQueueSender, update_pr_with_kn
 use crate::bors::{AUTO_BRANCH_NAME, PullRequestStatus, RepositoryState};
 use crate::database::{
     ApprovalInfo, BuildModel, BuildStatus, ExclusiveLockProof, ExclusiveOperationOutcome,
-    MergeableState, PullRequestModel, QueueStatus,
+    MergeableState, PullRequestModel, QueueStatus, UpdateBuildParams,
 };
 use crate::github::api::client::CheckRunOutput;
 use crate::github::api::operations::{BranchUpdateError, ForcePush};
@@ -268,7 +268,10 @@ async fn handle_successful_build(
 
         if let Some(error_comment) = error_comment {
             ctx.db
-                .update_build_column(auto_build, BuildStatus::Failure, None)
+                .update_build(
+                    auto_build.id,
+                    UpdateBuildParams::default().status(BuildStatus::Failure),
+                )
                 .await?;
             repo.client
                 .post_comment(pr_num, error_comment, &ctx.db)
@@ -593,7 +596,10 @@ async fn start_auto_build(
 
             if let Err(error) = ctx
                 .db
-                .update_build_check_run_id(build_id, id.into_inner() as i64)
+                .update_build(
+                    build_id,
+                    UpdateBuildParams::default().check_run_id(id.into_inner() as i64),
+                )
                 .await
             {
                 tracing::error!("Failed to update database with build check run id {id}: {error:?}",)

src/bors/mod.rs

@@ -185,6 +185,8 @@ pub struct WorkflowRun {
     pub name: String,
     pub url: String,
     pub status: WorkflowStatus,
+    pub created_at: DateTime<Utc>,
+    pub duration: Option<Duration>,
 }
 
 pub struct FailedWorkflowRun {

src/database/client.rs

@@ -6,21 +6,23 @@ use super::operations::{
     get_workflows_for_build, insert_repo_if_not_exists, record_tagged_bot_comment,
     set_pr_assignees, set_pr_mergeability_state, set_pr_priority, set_pr_rollup, set_pr_status,
     set_stale_mergeability_status_by_base_branch, unapprove_pull_request, undelegate_pull_request,
-    update_build_check_run_id, update_build_column, update_pr_try_build_id, update_workflow_status,
-    upsert_pull_request, upsert_repository,
+    update_build, update_pr_try_build_id, update_workflow_status, upsert_pull_request,
+    upsert_repository,
+};
+use super::{
+    ApprovalInfo, DelegatedPermission, MergeableState, PrimaryKey, RunId, UpdateBuildParams,
+    UpsertPullRequestParams,
 };
-use super::{ApprovalInfo, DelegatedPermission, MergeableState, RunId, UpsertPullRequestParams};
 use crate::bors::comment::CommentTag;
 use crate::bors::{BuildKind, PullRequestStatus, RollupMode};
 use crate::database::operations::update_pr_auto_build_id;
 use crate::database::{
-    BuildModel, BuildStatus, CommentModel, PullRequestModel, RepoModel, TreeState, WorkflowModel,
+    BuildModel, CommentModel, PullRequestModel, RepoModel, TreeState, WorkflowModel,
     WorkflowStatus, WorkflowType,
 };
 use crate::github::PullRequestNumber;
 use crate::github::{CommitSha, GithubRepoName};
 use anyhow::Context;
-use chrono::Duration;
 use itertools::Either;
 use sqlx::PgPool;
 use sqlx::postgres::PgAdvisoryLock;
@@ -257,21 +259,12 @@ impl PgDbClient {
         get_pending_builds(&self.pool, repo).await
     }
 
-    pub async fn update_build_column(
-        &self,
-        build: &BuildModel,
-        status: BuildStatus,
-        duration: Option<Duration>,
-    ) -> anyhow::Result<()> {
-        update_build_column(&self.pool, build.id, status, duration).await
-    }
-
-    pub async fn update_build_check_run_id(
+    pub async fn update_build(
         &self,
-        build_id: i32,
-        check_run_id: i64,
+        build_id: PrimaryKey,
+        params: UpdateBuildParams,
     ) -> anyhow::Result<()> {
-        update_build_check_run_id(&self.pool, build_id, check_run_id).await
+        update_build(&self.pool, build_id, params).await
     }
 
     pub async fn create_workflow(