Different approach for catching foreign key violation

Author: Jamesbarford

database/src/pool.rs

@@ -234,6 +234,18 @@ pub trait Connection: Send + Sync {
         benchmark_set: u32,
     ) -> anyhow::Result<u32>;
 
+    /// 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<u32>);
+
     /// 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;
+    }
 }

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<u32>) {
+        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,

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<u32>) {
+        no_queue_implementation_abort!()
+    }
+
     async fn get_compile_test_cases_with_measurements(
         &self,
         artifact_row_id: &ArtifactIdNumber,

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<Vec<BenchmarkRequest>> {
     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 {