diff --git a/database/src/lib.rs b/database/src/lib.rs index 9c0d74030..053e941c2 100644 --- a/database/src/lib.rs +++ b/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, String> { backends .split(',') diff --git a/database/src/pool.rs b/database/src/pool.rs index a366f72c8..a3b2d7b7e 100644 --- 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; /// Load all known benchmark request SHAs and all completed benchmark requests. async fn load_benchmark_request_index(&self) -> anyhow::Result; @@ -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) }) diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs index 499a10e21..173d763c7 100644 --- 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,8 +1633,9 @@ where async fn insert_benchmark_request( &self, benchmark_request: &BenchmarkRequest, - ) -> anyhow::Result<()> { - self.conn() + ) -> anyhow::Result { + 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 { diff --git a/database/src/pool/sqlite.rs b/database/src/pool/sqlite.rs index 803eb02c9..f61079da2 100644 --- 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 { no_queue_implementation_abort!() } diff --git a/site/src/job_queue/mod.rs b/site/src/job_queue/mod.rs index 1849b7e3d..a61bddebe 100644 --- 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::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}"); + } } } } diff --git a/site/src/request_handlers/github.rs b/site/src/request_handlers/github.rs index 905e90b77..122218f82 100644 --- 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,6 +76,16 @@ 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`. @@ -83,11 +93,23 @@ 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}")