From f068b34e5aa30f9a1b2b43f841c8d72d7c0d8d1b Mon Sep 17 00:00:00 2001 From: James Barford-Evans Date: Tue, 23 Sep 2025 09:51:19 +0100 Subject: [PATCH 1/2] Fix; Skip adding a `parent_sha` if the `parent_sha` has not been benchmarked ever --- database/src/lib.rs | 2 +- site/src/job_queue/mod.rs | 33 ++++++++++++++++++++++----------- 2 files changed, 23 insertions(+), 12 deletions(-) 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/site/src/job_queue/mod.rs b/site/src/job_queue/mod.rs index 6c23783dd..27d3ce9f8 100644 --- a/site/src/job_queue/mod.rs +++ b/site/src/job_queue/mod.rs @@ -205,6 +205,7 @@ pub async fn build_queue( pub async fn enqueue_benchmark_request( conn: &mut dyn database::pool::Connection, benchmark_request: &BenchmarkRequest, + index: &BenchmarkRequestIndex, ) -> anyhow::Result<()> { let mut tx = conn.transaction().await; @@ -216,6 +217,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 +240,22 @@ 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( - parent_sha, - target, - backend, - profile, - benchmark_set as u32, - ) - .await?; + if !has_emitted_parent_sha_error && !index.contains_tag(parent_sha) { + log::error!("Parent tag; {parent_sha} does not exist in request benchmarks. Skipping"); + has_emitted_parent_sha_error = true; + } + + if !has_emitted_parent_sha_error { + tx.conn() + .enqueue_benchmark_job( + parent_sha, + target, + backend, + profile, + benchmark_set as u32, + ) + .await?; + } } } } @@ -267,6 +277,7 @@ pub async fn enqueue_benchmark_request( /// Returns benchmark requests that were completed. async fn process_benchmark_requests( conn: &mut dyn database::pool::Connection, + index: &BenchmarkRequestIndex, ) -> anyhow::Result> { let queue = build_queue(conn).await?; @@ -282,7 +293,7 @@ async fn process_benchmark_requests( break; } BenchmarkRequestStatus::ArtifactsReady => { - enqueue_benchmark_request(conn, &request).await?; + enqueue_benchmark_request(conn, &request, index).await?; break; } BenchmarkRequestStatus::WaitingForArtifacts @@ -306,7 +317,7 @@ async fn cron_enqueue_jobs(ctxt: &SiteCtxt) -> anyhow::Result<()> { // Put the releases into the `benchmark_requests` queue requests_inserted |= create_benchmark_request_releases(&*conn, &index).await?; // Enqueue waiting requests and try to complete in-progress ones - let completed_reqs = process_benchmark_requests(&mut *conn).await?; + let completed_reqs = process_benchmark_requests(&mut *conn, &index).await?; // If some change happened, reload the benchmark request index if requests_inserted { From 3eb5edb40be6ebd83b6e6226fae8a93d5b436d09 Mon Sep 17 00:00:00 2001 From: James Barford-Evans Date: Tue, 23 Sep 2025 11:01:14 +0100 Subject: [PATCH 2/2] Different approach for catching foreign key violation --- database/src/pool.rs | 34 ++++++++++++++++++++ database/src/pool/postgres.rs | 59 +++++++++++++++++++++++++++++++++++ database/src/pool/sqlite.rs | 11 +++++++ site/src/job_queue/mod.rs | 44 +++++++++++++++----------- 4 files changed, 129 insertions(+), 19 deletions(-) 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 27d3ce9f8..c63bfbe79 100644 --- a/site/src/job_queue/mod.rs +++ b/site/src/job_queue/mod.rs @@ -205,7 +205,6 @@ pub async fn build_queue( pub async fn enqueue_benchmark_request( conn: &mut dyn database::pool::Connection, benchmark_request: &BenchmarkRequest, - index: &BenchmarkRequestIndex, ) -> anyhow::Result<()> { let mut tx = conn.transaction().await; @@ -240,21 +239,29 @@ 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() { - if !has_emitted_parent_sha_error && !index.contains_tag(parent_sha) { - log::error!("Parent tag; {parent_sha} does not exist in request benchmarks. Skipping"); - has_emitted_parent_sha_error = true; - } - - if !has_emitted_parent_sha_error { - tx.conn() - .enqueue_benchmark_job( - parent_sha, - target, - backend, - profile, - benchmark_set as u32, - ) - .await?; + let (is_foreign_key_violation, result) = tx + .conn() + .enqueue_parent_benchmark_job( + parent_sha, + target, + backend, + profile, + benchmark_set as u32, + ) + .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); + } } } } @@ -277,7 +284,6 @@ pub async fn enqueue_benchmark_request( /// Returns benchmark requests that were completed. async fn process_benchmark_requests( conn: &mut dyn database::pool::Connection, - index: &BenchmarkRequestIndex, ) -> anyhow::Result> { let queue = build_queue(conn).await?; @@ -293,7 +299,7 @@ async fn process_benchmark_requests( break; } BenchmarkRequestStatus::ArtifactsReady => { - enqueue_benchmark_request(conn, &request, index).await?; + enqueue_benchmark_request(conn, &request).await?; break; } BenchmarkRequestStatus::WaitingForArtifacts @@ -317,7 +323,7 @@ async fn cron_enqueue_jobs(ctxt: &SiteCtxt) -> anyhow::Result<()> { // Put the releases into the `benchmark_requests` queue requests_inserted |= create_benchmark_request_releases(&*conn, &index).await?; // Enqueue waiting requests and try to complete in-progress ones - let completed_reqs = process_benchmark_requests(&mut *conn, &index).await?; + let completed_reqs = process_benchmark_requests(&mut *conn).await?; // If some change happened, reload the benchmark request index if requests_inserted {