Merge pull request #2255 from Jamesbarford/fix/parent-request-job-queuing

Fix; Skip adding a `parent_sha` if the `parent_sha` has not been benchmarked ever

Author: Kobzol

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)
     }
 }
 

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

@@ -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);
+                            }
+                        }
                     }
                 }
             }