Merge pull request #2331 from Jamesbarford/fix/try-builds-non-ready-state

Post more precise message if a Try is queued multiple times

Author: Kobzol

database/src/lib.rs

@@ -1039,6 +1039,16 @@ impl BenchmarkRequest {
     }
 }
 
+/// Result of inserting into the database
+#[derive(Debug, Clone, PartialEq)]
+pub enum BenchmarkRequestInsertResult {
+    /// The request was inserted into the database and is a unique instance
+    Inserted,
+    /// The request was not inserted into the database as something else already
+    /// existed that clashed with the unique clause
+    NothingInserted,
+}
+
 pub fn parse_backends(backends: &str) -> Result<Vec<CodegenBackend>, String> {
     backends
         .split(',')

database/src/pool.rs

@@ -1,9 +1,9 @@
 use crate::selector::CompileTestCase;
 use crate::{
     ArtifactCollection, ArtifactId, ArtifactIdNumber, BenchmarkJob, BenchmarkJobConclusion,
-    BenchmarkJobKind, BenchmarkRequest, BenchmarkRequestIndex, BenchmarkRequestStatus,
-    BenchmarkRequestWithErrors, BenchmarkSet, CodegenBackend, CollectorConfig, CompileBenchmark,
-    PendingBenchmarkRequests, Target,
+    BenchmarkJobKind, BenchmarkRequest, BenchmarkRequestIndex, BenchmarkRequestInsertResult,
+    BenchmarkRequestStatus, BenchmarkRequestWithErrors, BenchmarkSet, CodegenBackend,
+    CollectorConfig, CompileBenchmark, PendingBenchmarkRequests, Target,
 };
 use crate::{CollectionId, Index, Profile, QueuedCommit, Scenario, Step};
 use chrono::{DateTime, Utc};
@@ -207,7 +207,7 @@ pub trait Connection: Send + Sync {
     async fn insert_benchmark_request(
         &self,
         benchmark_request: &BenchmarkRequest,
-    ) -> anyhow::Result<()>;
+    ) -> anyhow::Result<BenchmarkRequestInsertResult>;
 
     /// Load all known benchmark request SHAs and all completed benchmark requests.
     async fn load_benchmark_request_index(&self) -> anyhow::Result<BenchmarkRequestIndex>;
@@ -538,9 +538,15 @@ mod tests {
             .await
             .unwrap();
 
-            db.insert_benchmark_request(&BenchmarkRequest::create_release("a-sha-1", Utc::now()))
-                .await
-                .expect_err("it was possible to insert a second commit with the same SHA");
+            let result = db
+                .insert_benchmark_request(&BenchmarkRequest::create_release("a-sha-1", Utc::now()))
+                .await;
+
+            assert!(result.is_ok());
+            assert_eq!(
+                result.unwrap(),
+                BenchmarkRequestInsertResult::NothingInserted
+            );
 
             Ok(ctx)
         })
@@ -566,12 +572,14 @@ mod tests {
             // This should be fine, because the previous request was already completed
             ctx.insert_try_request(42).await;
             // But this should fail, as we can't have two queued requests at once
-            db.insert_benchmark_request(&BenchmarkRequest::create_try_without_artifacts(
-                42, "", "",
-            ))
-            .await
-            .expect_err("It was possible to record two try benchmark requests without artifacts");
+            let result = db
+                .insert_benchmark_request(&BenchmarkRequest::create_try_without_artifacts(
+                    42, "", "",
+                ))
+                .await
+                .unwrap();
 
+            assert_eq!(result, BenchmarkRequestInsertResult::NothingInserted);
             Ok(ctx)
         })
         .await;
@@ -592,14 +600,17 @@ mod tests {
             .await
             .unwrap();
 
-            db.insert_benchmark_request(&BenchmarkRequest::create_master(
-                "a-sha-2",
-                "parent-sha-2",
-                42,
-                Utc::now(),
-            ))
-            .await
-            .expect_err("it was possible to insert a second master commit on the same PR");
+            let result = db
+                .insert_benchmark_request(&BenchmarkRequest::create_master(
+                    "a-sha-2",
+                    "parent-sha-2",
+                    42,
+                    Utc::now(),
+                ))
+                .await
+                .unwrap();
+
+            assert_eq!(result, BenchmarkRequestInsertResult::NothingInserted);
 
             Ok(ctx)
         })

database/src/pool/postgres.rs

@@ -5,10 +5,10 @@ use crate::selector::CompileTestCase;
 use crate::{
     ArtifactCollection, ArtifactId, ArtifactIdNumber, Benchmark, BenchmarkJob,
     BenchmarkJobConclusion, BenchmarkJobKind, BenchmarkJobStatus, BenchmarkRequest,
-    BenchmarkRequestIndex, BenchmarkRequestStatus, BenchmarkRequestType,
-    BenchmarkRequestWithErrors, BenchmarkSet, CodegenBackend, CollectionId, CollectorConfig,
-    Commit, CommitType, CompileBenchmark, Date, Index, PendingBenchmarkRequests, Profile,
-    QueuedCommit, Scenario, Target, BENCHMARK_JOB_STATUS_FAILURE_STR,
+    BenchmarkRequestIndex, BenchmarkRequestInsertResult, BenchmarkRequestStatus,
+    BenchmarkRequestType, BenchmarkRequestWithErrors, BenchmarkSet, CodegenBackend, CollectionId,
+    CollectorConfig, Commit, CommitType, CompileBenchmark, Date, Index, PendingBenchmarkRequests,
+    Profile, QueuedCommit, Scenario, Target, BENCHMARK_JOB_STATUS_FAILURE_STR,
     BENCHMARK_JOB_STATUS_IN_PROGRESS_STR, BENCHMARK_JOB_STATUS_QUEUED_STR,
     BENCHMARK_JOB_STATUS_SUCCESS_STR, BENCHMARK_REQUEST_MASTER_STR, BENCHMARK_REQUEST_RELEASE_STR,
     BENCHMARK_REQUEST_STATUS_ARTIFACTS_READY_STR, BENCHMARK_REQUEST_STATUS_COMPLETED_STR,
@@ -1633,8 +1633,9 @@ where
     async fn insert_benchmark_request(
         &self,
         benchmark_request: &BenchmarkRequest,
-    ) -> anyhow::Result<()> {
-        self.conn()
+    ) -> anyhow::Result<BenchmarkRequestInsertResult> {
+        let row_insert_count = self
+            .conn()
             .execute(
                 r#"
                 INSERT INTO benchmark_request(
@@ -1648,7 +1649,8 @@ where
                     profiles,
                     commit_date
                 )
-                VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9);
+                VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9)
+                ON CONFLICT DO NOTHING;
             "#,
                 &[
                     &benchmark_request.tag(),
@@ -1664,7 +1666,13 @@ where
             )
             .await
             .context("Failed to insert benchmark request")?;
-        Ok(())
+        if row_insert_count == 0 {
+            // Allows us to handle duplicated cases without the database auto
+            // erroring
+            Ok(BenchmarkRequestInsertResult::NothingInserted)
+        } else {
+            Ok(BenchmarkRequestInsertResult::Inserted)
+        }
     }
 
     async fn load_benchmark_request_index(&self) -> anyhow::Result<BenchmarkRequestIndex> {

database/src/pool/sqlite.rs

@@ -4,9 +4,10 @@ use crate::pool::{
 use crate::selector::CompileTestCase;
 use crate::{
     ArtifactCollection, ArtifactId, Benchmark, BenchmarkJob, BenchmarkJobConclusion,
-    BenchmarkJobKind, BenchmarkRequest, BenchmarkRequestIndex, BenchmarkRequestStatus,
-    BenchmarkRequestWithErrors, BenchmarkSet, CodegenBackend, CollectionId, CollectorConfig,
-    Commit, CommitType, CompileBenchmark, Date, PendingBenchmarkRequests, Profile, Target,
+    BenchmarkJobKind, BenchmarkRequest, BenchmarkRequestIndex, BenchmarkRequestInsertResult,
+    BenchmarkRequestStatus, BenchmarkRequestWithErrors, BenchmarkSet, CodegenBackend, CollectionId,
+    CollectorConfig, Commit, CommitType, CompileBenchmark, Date, PendingBenchmarkRequests, Profile,
+    Target,
 };
 use crate::{ArtifactIdNumber, Index, QueuedCommit};
 use chrono::{DateTime, TimeZone, Utc};
@@ -1318,7 +1319,7 @@ impl Connection for SqliteConnection {
     async fn insert_benchmark_request(
         &self,
         _benchmark_request: &BenchmarkRequest,
-    ) -> anyhow::Result<()> {
+    ) -> anyhow::Result<BenchmarkRequestInsertResult> {
         no_queue_implementation_abort!()
     }
 

site/src/job_queue/mod.rs

@@ -8,9 +8,9 @@ use chrono::Utc;
 use collector::benchmark_set::benchmark_set_count;
 use database::pool::{JobEnqueueResult, Transaction};
 use database::{
-    BenchmarkJobKind, BenchmarkRequest, BenchmarkRequestIndex, BenchmarkRequestStatus,
-    BenchmarkRequestType, CodegenBackend, Connection, Date, PendingBenchmarkRequests, Profile,
-    QueuedCommit, Target,
+    BenchmarkJobKind, BenchmarkRequest, BenchmarkRequestIndex, BenchmarkRequestInsertResult,
+    BenchmarkRequestStatus, BenchmarkRequestType, CodegenBackend, Connection, Date,
+    PendingBenchmarkRequests, Profile, QueuedCommit, Target,
 };
 use parking_lot::RwLock;
 use std::sync::Arc;
@@ -29,7 +29,6 @@ pub fn should_use_job_queue(_pr: u32) -> bool {
 
 /// Store the latest master commits or do nothing if all of them are
 /// already in the database.
-/// Returns `true` if at least one benchmark request was inserted.
 async fn create_benchmark_request_master_commits(
     ctxt: &SiteCtxt,
     conn: &dyn database::pool::Connection,
@@ -56,8 +55,16 @@ async fn create_benchmark_request_master_commits(
             );
             log::info!("Inserting master benchmark request {benchmark:?}");
 
-            if let Err(error) = conn.insert_benchmark_request(&benchmark).await {
-                log::error!("Failed to insert master benchmark request: {error:?}");
+            match conn.insert_benchmark_request(&benchmark).await {
+                Ok(BenchmarkRequestInsertResult::NothingInserted) => {
+                    log::error!(
+                        "Failed to insert master benchmark request, request for PR`#{pr}` already exists",
+                    );
+                }
+                Ok(BenchmarkRequestInsertResult::Inserted) => {}
+                Err(e) => {
+                    log::error!("Failed to insert master benchmark request: {e:?}");
+                }
             }
         }
     }
@@ -87,8 +94,16 @@ async fn create_benchmark_request_releases(
             let release_request = BenchmarkRequest::create_release(&name, commit_date);
             log::info!("Inserting release benchmark request {release_request:?}");
 
-            if let Err(error) = conn.insert_benchmark_request(&release_request).await {
-                log::error!("Failed to insert release benchmark request: {error}");
+            match conn.insert_benchmark_request(&release_request).await {
+                Ok(BenchmarkRequestInsertResult::NothingInserted) => {
+                    log::error!(
+                        "Failed to insert release benchmark request, release with tag `{name}` already exists"
+                    );
+                }
+                Ok(BenchmarkRequestInsertResult::Inserted) => {}
+                Err(e) => {
+                    log::error!("Failed to insert release benchmark request: {e}");
+                }
             }
         }
     }

site/src/request_handlers/github.rs

@@ -6,7 +6,7 @@ use crate::github::{
 use crate::job_queue::should_use_job_queue;
 use crate::load::SiteCtxt;
 
-use database::{parse_backends, BenchmarkRequest, CodegenBackend};
+use database::{parse_backends, BenchmarkRequest, BenchmarkRequestInsertResult, CodegenBackend};
 use hashbrown::HashMap;
 use std::sync::Arc;
 
@@ -76,18 +76,40 @@ async fn handle_issue(
     Ok(github::Response)
 }
 
+fn get_awaiting_on_bors_message() -> String {
+    format!(
+        "Awaiting bors try build completion.
+
+@rustbot label: +S-waiting-on-perf
+
+{COMMENT_MARK_TEMPORARY}"
+    )
+}
+
 /// The try request does not have a `sha` or a `parent_sha` but we need to keep a record
 /// of this commit existing. The DB ensures that there is only one non-completed
 /// try benchmark request per `pr`.
 async fn record_try_benchmark_request_without_artifacts(
     conn: &dyn database::pool::Connection,
     pr: u32,
     backends: &str,
-) {
+) -> String {
     let try_request = BenchmarkRequest::create_try_without_artifacts(pr, backends, "");
     log::info!("Inserting try benchmark request {try_request:?}");
-    if let Err(e) = conn.insert_benchmark_request(&try_request).await {
-        log::error!("Failed to insert try benchmark request: {}", e);
+
+    match conn.insert_benchmark_request(&try_request).await {
+        Ok(BenchmarkRequestInsertResult::NothingInserted) => {
+            log::info!(
+                "Failed to insert try benchmark request, a request for PR`#{pr}` already exists"
+            );
+            "This pull request was already queued before and is awaiting a try build to finish."
+                .to_string()
+        }
+        Ok(BenchmarkRequestInsertResult::Inserted) => get_awaiting_on_bors_message(),
+        Err(e) => {
+            log::error!("Failed to insert release benchmark request: {e}");
+            "Something went wrong! This is most likely an internal failure, please let us know on [Zulip](https://rust-lang.zulipchat.com/#narrow/channel/242791-t-infra)".to_string()
+        }
     }
 }
 
@@ -128,7 +150,7 @@ async fn handle_rust_timer(
                         issue.number,
                         cmd.params.backends.unwrap_or(""),
                     )
-                    .await;
+                    .await
                 } else {
                     conn.queue_pr(
                         issue.number,
@@ -138,14 +160,8 @@ async fn handle_rust_timer(
                         cmd.params.backends,
                     )
                     .await;
+                    get_awaiting_on_bors_message()
                 }
-                format!(
-                    "Awaiting bors try build completion.
-
-@rustbot label: +S-waiting-on-perf
-
-{COMMENT_MARK_TEMPORARY}"
-                )
             }
             Err(error) => {
                 format!("Error occurred while parsing comment: {error}")