Post more precise message if a Try is queued multiple times

Jamesbarford · Jamesbarford · commit 4d73090bc4bd · 2025-11-18T09:32:13.000Z
diff --git a/database/src/lib.rs b/database/src/lib.rs
@@ -1039,6 +1039,12 @@ impl BenchmarkRequest {
     }
 }
 
+#[derive(Debug, Clone, PartialEq)]
+pub enum BenchmarkRequestInsertResult {
+    Queued,
+    AlreadyQueued,
+}
+
 pub fn parse_backends(backends: &str) -> Result<Vec<CodegenBackend>, String> {
     backends
         .split(',')
diff --git a/database/src/pool.rs b/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,12 @@ 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::AlreadyQueued);
 
             Ok(ctx)
         })
@@ -566,12 +569,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::AlreadyQueued);
             Ok(ctx)
         })
         .await;
@@ -592,14 +597,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::AlreadyQueued);
 
             Ok(ctx)
         })
diff --git a/database/src/pool/postgres.rs b/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,9 +1633,10 @@ where
     async fn insert_benchmark_request(
         &self,
         benchmark_request: &BenchmarkRequest,
-    ) -> anyhow::Result<()> {
-        self.conn()
-            .execute(
+    ) -> anyhow::Result<BenchmarkRequestInsertResult> {
+        let rows = self
+            .conn()
+            .query(
                 r#"
                 INSERT INTO benchmark_request(
                     tag,
@@ -1648,7 +1649,9 @@ 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
+                RETURNING id;
             "#,
                 &[
                     &benchmark_request.tag(),
@@ -1664,7 +1667,13 @@ where
             )
             .await
             .context("Failed to insert benchmark request")?;
-        Ok(())
+        if rows.is_empty() {
+            // Allows us to handle duplicated cases without the database auto
+            // erroring
+            Ok(BenchmarkRequestInsertResult::AlreadyQueued)
+        } else {
+            Ok(BenchmarkRequestInsertResult::Queued)
+        }
     }
 
     async fn load_benchmark_request_index(&self) -> anyhow::Result<BenchmarkRequestIndex> {
diff --git a/database/src/pool/sqlite.rs b/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!()
     }
 
diff --git a/site/src/job_queue/mod.rs b/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::AlreadyQueued) => {
+                    log::error!(
+                        "Failed to insert master benchmark request, request for PR`#{pr}` already exists",
+                    );
+                }
+                Ok(BenchmarkRequestInsertResult::Queued) => {}
+                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::AlreadyQueued) => {
+                    log::error!(
+                        "Failed to insert release benchmark request, release with tag `{name}` already exists"
+                    );
+                }
+                Ok(BenchmarkRequestInsertResult::Queued) => {}
+                Err(e) => {
+                    log::error!("Failed to insert release benchmark request: {e}");
+                }
             }
         }
     }
diff --git a/site/src/request_handlers/github.rs b/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,19 +76,48 @@ 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(
+    client: &client::Client,
     conn: &dyn database::pool::Connection,
     pr: u32,
     backends: &str,
-) {
+) -> ServerResult<github::Response> {
     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::AlreadyQueued) => {
+            log::error!(
+                "Failed to insert try benchmark request, a request for PR`#{pr}` already exists"
+            );
+            client.post_comment(pr, format!("Failed to insert try benchmark request, a request for PR`#{pr}` already exists")).await;
+        }
+        Ok(BenchmarkRequestInsertResult::Queued) => {
+            client
+                .post_comment(pr, get_awaiting_on_bors_message())
+                .await;
+        }
+        Err(e) => {
+            log::error!("Failed to insert release benchmark request: {e}");
+            client
+                .post_comment(pr, "Something went wrong! This is most likely an internal failure, please message on Zulip".to_string())
+                .await;
+        }
     }
+    return Ok(github::Response);
 }
 
 async fn handle_rust_timer(
@@ -123,7 +152,8 @@ async fn handle_rust_timer(
                 let conn = ctxt.conn().await;
 
                 if should_use_job_queue(issue.number) {
-                    record_try_benchmark_request_without_artifacts(
+                    return record_try_benchmark_request_without_artifacts(
+                        main_client,
                         &*conn,
                         issue.number,
                         cmd.params.backends.unwrap_or(""),
@@ -139,13 +169,7 @@ async fn handle_rust_timer(
                     )
                     .await;
                 }
-                format!(
-                    "Awaiting bors try build completion.
-
-@rustbot label: +S-waiting-on-perf
-
-{COMMENT_MARK_TEMPORARY}"
-                )
+                get_awaiting_on_bors_message()
             }
             Err(error) => {
                 format!("Error occurred while parsing comment: {error}")
@@ -177,7 +201,8 @@ async fn handle_rust_timer(
         let conn = ctxt.conn().await;
         for command in &valid_build_cmds {
             if should_use_job_queue(issue.number) {
-                record_try_benchmark_request_without_artifacts(
+                return record_try_benchmark_request_without_artifacts(
+                    main_client,
                     &*conn,
                     issue.number,
                     command.params.backends.unwrap_or(""),

Original file line number	Diff line number	Diff line change
`@@ -1039,6 +1039,12 @@ impl BenchmarkRequest {`
`1039`	`1039`	`}`
`1040`	`1040`	`}`
`1041`	`1041`
	`1042`	`+#[derive(Debug, Clone, PartialEq)]`
	`1043`	`+pub enum BenchmarkRequestInsertResult {`
	`1044`	`+ Queued,`
	`1045`	`+ AlreadyQueued,`
	`1046`	`+}`
	`1047`	`+`
`1042`	`1048`	`pub fn parse_backends(backends: &str) -> Result<Vec<CodegenBackend>, String> {`
`1043`	`1049`	`backends`
`1044`	`1050`	`.split(',')`