diff --git a/database/src/lib.rs b/database/src/lib.rs index 5d0c142e1..c1e04a0c8 100644 --- a/database/src/lib.rs +++ b/database/src/lib.rs @@ -1036,7 +1036,7 @@ impl BenchmarkRequest { } pub fn is_in_progress(&self) -> bool { - matches!(self.status, BenchmarkRequestStatus::InProgress { .. }) + matches!(self.status, BenchmarkRequestStatus::InProgress) } } diff --git a/database/src/pool.rs b/database/src/pool.rs index 7a6cac627..2eeea2979 100644 --- a/database/src/pool.rs +++ b/database/src/pool.rs @@ -234,6 +234,18 @@ pub trait Connection: Send + Sync { benchmark_set: u32, ) -> anyhow::Result; + /// Add a benchmark job which is explicitly using a `parent_sha` we split + /// this out to improve our error handling. A `parent_sha` may not have + /// an associated request in the `benchmarek` + async fn enqueue_parent_benchmark_job( + &self, + parent_sha: &str, + target: Target, + backend: CodegenBackend, + profile: Profile, + benchmark_set: u32, + ) -> (bool, anyhow::Result); + /// Returns a set of compile-time benchmark test cases that were already computed for the /// given artifact. /// Note that for efficiency reasons, the function only checks if we have at least a single @@ -1187,4 +1199,26 @@ mod tests { }) .await; } + + #[tokio::test] + async fn enqueue_parent_benchmark_job() { + run_postgres_test(|ctx| async { + let db = ctx.db(); + + let (violates_foreign_key, _) = db + .enqueue_parent_benchmark_job( + "sha-0", + Target::X86_64UnknownLinuxGnu, + CodegenBackend::Llvm, + Profile::Debug, + 0, + ) + .await; + + assert!(violates_foreign_key); + + Ok(ctx) + }) + .await; + } } diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs index 8fa0988eb..347f2b30b 100644 --- a/database/src/pool/postgres.rs +++ b/database/src/pool/postgres.rs @@ -22,6 +22,7 @@ use std::str::FromStr; use std::sync::Arc; use std::time::Duration; use tokio::sync::Mutex; +use tokio_postgres::error::SqlState; use tokio_postgres::Statement; use tokio_postgres::{GenericClient, Row}; @@ -1738,6 +1739,64 @@ where }) } + async fn enqueue_parent_benchmark_job( + &self, + parent_sha: &str, + target: Target, + backend: CodegenBackend, + profile: Profile, + benchmark_set: u32, + ) -> (bool, anyhow::Result) { + let row_result = self + .conn() + .query_one( + r#" + INSERT INTO job_queue( + request_tag, + target, + backend, + profile, + benchmark_set, + status + ) + VALUES ($1, $2, $3, $4, $5, $6) + ON CONFLICT DO NOTHING + RETURNING job_queue.id + "#, + &[ + &parent_sha, + &target, + &backend, + &profile, + &(benchmark_set as i32), + &BENCHMARK_JOB_STATUS_QUEUED_STR, + ], + ) + .await; + + match row_result { + Ok(row) => (false, Ok(row.get::<_, i32>(0) as u32)), + Err(e) => { + if let Some(db_err) = e.as_db_error() { + if db_err.code() == &SqlState::FOREIGN_KEY_VIOLATION { + let constraint = db_err.constraint().unwrap_or("benchmark_tag constraint"); + let detail = db_err.detail().unwrap_or(""); + return ( + true, + Err(anyhow::anyhow!( + "Foreign key violation on {} for request_tag='{}'. {}", + constraint, + parent_sha, + detail + )), + ); + } + } + (false, Err(e.into())) + } + } + } + async fn enqueue_benchmark_job( &self, request_tag: &str, diff --git a/database/src/pool/sqlite.rs b/database/src/pool/sqlite.rs index 74cb63111..f331cb886 100644 --- a/database/src/pool/sqlite.rs +++ b/database/src/pool/sqlite.rs @@ -1339,6 +1339,17 @@ impl Connection for SqliteConnection { no_queue_implementation_abort!() } + async fn enqueue_parent_benchmark_job( + &self, + _parent_sha: &str, + _target: Target, + _backend: CodegenBackend, + _profile: Profile, + _benchmark_set: u32, + ) -> (bool, anyhow::Result) { + no_queue_implementation_abort!() + } + async fn get_compile_test_cases_with_measurements( &self, artifact_row_id: &ArtifactIdNumber, diff --git a/site/src/job_queue/mod.rs b/site/src/job_queue/mod.rs index 6c23783dd..c63bfbe79 100644 --- a/site/src/job_queue/mod.rs +++ b/site/src/job_queue/mod.rs @@ -216,6 +216,8 @@ pub async fn enqueue_benchmark_request( let backends = benchmark_request.backends()?; let profiles = benchmark_request.profiles()?; + // Prevent the error from spamming the logs + let mut has_emitted_parent_sha_error = false; // Target x benchmark_set x backend x profile -> BenchmarkJob for target in Target::all() { @@ -237,15 +239,30 @@ pub async fn enqueue_benchmark_request( // but was already benchmarked then the collector will ignore // it as it will see it already has results. if let Some(parent_sha) = benchmark_request.parent_sha() { - tx.conn() - .enqueue_benchmark_job( + let (is_foreign_key_violation, result) = tx + .conn() + .enqueue_parent_benchmark_job( parent_sha, target, backend, profile, benchmark_set as u32, ) - .await?; + .await; + + // At some point in time the parent_sha may not refer + // to a `benchmark_request` and we want to be able to + // see that error. + if let Err(e) = result { + if is_foreign_key_violation && !has_emitted_parent_sha_error { + log::error!("Failed to create job for parent sha {e:?}"); + has_emitted_parent_sha_error = true; + } else if has_emitted_parent_sha_error && is_foreign_key_violation { + continue; + } else { + return Err(e); + } + } } } }