From 9e4ee29d0a87788dbed142357dd656767d4ca2ca Mon Sep 17 00:00:00 2001 From: James Barford-Evans Date: Fri, 19 Sep 2025 08:31:27 +0100 Subject: [PATCH 1/5] Fix; `job_queue` insert only if tag exists in `benchmark_request` --- database/src/pool/postgres.rs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs index 8fa0988eb..5c3cde692 100644 --- a/database/src/pool/postgres.rs +++ b/database/src/pool/postgres.rs @@ -1597,6 +1597,22 @@ where &self, benchmark_request: &BenchmarkRequest, ) -> anyhow::Result<()> { + // @TODO delete this when the new system has been running. This code + // prevents a foreign key violation in the job_queue table. This can + // happen as the parent_sha may not be in the range of initial commits + // we load into the database when the job queue first starts and inserts + // it's first batch of requests. + let row = self + .conn() + .query_one( + "SELECT tag FROM benchmark_request WHERE tag = $1", + &[&benchmark_request.parent_sha()], + ) + .await + .context("Failed to check parent sha")?; + + let parent_sha = row.get::<_, Option>(0).map(|parent_sha| parent_sha); + self.conn() .execute( r#" @@ -1615,7 +1631,7 @@ where "#, &[ &benchmark_request.tag(), - &benchmark_request.parent_sha(), + &parent_sha, &benchmark_request.pr().map(|it| it as i32), &benchmark_request.commit_type, &benchmark_request.status.as_str(), From 75853ceb52198844ba123f4af71e88d388f4251b Mon Sep 17 00:00:00 2001 From: James Barford-Evans Date: Mon, 22 Sep 2025 11:06:01 +0100 Subject: [PATCH 2/5] make master commit parent_sha Nullable --- database/src/lib.rs | 8 ++++---- database/src/pool/postgres.rs | 10 ++++++---- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/database/src/lib.rs b/database/src/lib.rs index 5d0c142e1..a0a033f54 100644 --- a/database/src/lib.rs +++ b/database/src/lib.rs @@ -872,7 +872,7 @@ pub enum BenchmarkRequestType { /// A Master commit Master { sha: String, - parent_sha: String, + parent_sha: Option, pr: u32, }, /// A release only has a tag @@ -937,7 +937,7 @@ impl BenchmarkRequest { commit_type: BenchmarkRequestType::Master { pr, sha: sha.to_string(), - parent_sha: parent_sha.to_string(), + parent_sha: Some(parent_sha.to_string()), }, commit_date: Some(commit_date), created_at: Utc::now(), @@ -968,8 +968,8 @@ impl BenchmarkRequest { pub fn parent_sha(&self) -> Option<&str> { match &self.commit_type { - BenchmarkRequestType::Try { parent_sha, .. } => parent_sha.as_deref(), - BenchmarkRequestType::Master { parent_sha, .. } => Some(parent_sha), + BenchmarkRequestType::Try { parent_sha, .. } + | BenchmarkRequestType::Master { parent_sha, .. } => parent_sha.as_deref(), BenchmarkRequestType::Release { .. } => None, } } diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs index 5c3cde692..53ba75abf 100644 --- a/database/src/pool/postgres.rs +++ b/database/src/pool/postgres.rs @@ -1602,16 +1602,18 @@ where // happen as the parent_sha may not be in the range of initial commits // we load into the database when the job queue first starts and inserts // it's first batch of requests. - let row = self + let rows = self .conn() - .query_one( + .query( "SELECT tag FROM benchmark_request WHERE tag = $1", &[&benchmark_request.parent_sha()], ) .await .context("Failed to check parent sha")?; - let parent_sha = row.get::<_, Option>(0).map(|parent_sha| parent_sha); + let parent_sha = rows + .first() + .map(|row| row.get::<_, Option>(0).map(|parent_sha| parent_sha)); self.conn() .execute( @@ -2306,7 +2308,7 @@ fn row_to_benchmark_request(row: &Row, row_offset: Option) -> BenchmarkRe BENCHMARK_REQUEST_MASTER_STR => BenchmarkRequest { commit_type: BenchmarkRequestType::Master { sha: tag.expect("Master commit in the DB without a SHA"), - parent_sha: parent_sha.expect("Master commit in the DB without a parent SHA"), + parent_sha, pr: pr.expect("Master commit in the DB without a PR"), }, commit_date, From d15ef77551d9cb7264d54e2f7faf0601ce282dd9 Mon Sep 17 00:00:00 2001 From: James Barford-Evans Date: Mon, 22 Sep 2025 11:45:49 +0100 Subject: [PATCH 3/5] fix up test --- database/src/pool.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/database/src/pool.rs b/database/src/pool.rs index 7a6cac627..d59b8e536 100644 --- a/database/src/pool.rs +++ b/database/src/pool.rs @@ -585,6 +585,10 @@ mod tests { run_postgres_test(|ctx| async { let db = ctx.db(); + // Releases don't need parent shas + ctx.insert_release_request("parent-sha-1").await; + ctx.insert_release_request("parent-sha-2").await; + // ArtifactsReady let req_a = ctx.insert_master_request("sha-1", "parent-sha-1", 42).await; // ArtifactsReady @@ -597,11 +601,7 @@ mod tests { ctx.insert_release_request("1.79.0").await; ctx.complete_request("1.79.0").await; - ctx.insert_master_request("parent-sha-1", "grandparent-sha-0", 100) - .await; ctx.complete_request("parent-sha-1").await; - ctx.insert_master_request("parent-sha-2", "grandparent-sha-1", 101) - .await; ctx.complete_request("parent-sha-2").await; db.update_benchmark_request_status("sha-2", BenchmarkRequestStatus::InProgress) From 49f9b92532be6f486c965a69e1eb43f04820d803 Mon Sep 17 00:00:00 2001 From: James Barford-Evans Date: Mon, 22 Sep 2025 14:57:13 +0100 Subject: [PATCH 4/5] Refactor all instances of a `master` commit to have the `parent_sha` as optional --- database/src/lib.rs | 11 ++++++--- database/src/pool.rs | 41 ++++++++++++++++--------------- database/src/pool/postgres.rs | 46 +++++++++++++++++++---------------- database/src/tests/builder.rs | 2 +- database/src/tests/mod.rs | 2 +- site/src/job_queue/mod.rs | 18 +++++++------- 6 files changed, 66 insertions(+), 54 deletions(-) diff --git a/database/src/lib.rs b/database/src/lib.rs index a0a033f54..f2084fe18 100644 --- a/database/src/lib.rs +++ b/database/src/lib.rs @@ -932,12 +932,17 @@ impl BenchmarkRequest { } /// Create a master benchmark request that is in the `ArtifactsReady` status. - pub fn create_master(sha: &str, parent_sha: &str, pr: u32, commit_date: DateTime) -> Self { + pub fn create_master( + sha: &str, + parent_sha: Option<&str>, + pr: u32, + commit_date: DateTime, + ) -> Self { Self { commit_type: BenchmarkRequestType::Master { pr, sha: sha.to_string(), - parent_sha: Some(parent_sha.to_string()), + parent_sha: parent_sha.map(|it| it.to_string()), }, commit_date: Some(commit_date), created_at: Utc::now(), @@ -1036,7 +1041,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 d59b8e536..12101af28 100644 --- a/database/src/pool.rs +++ b/database/src/pool.rs @@ -505,7 +505,7 @@ mod tests { db.insert_benchmark_request(&BenchmarkRequest::create_master( "a-sha-1", - "parent-sha-1", + Some("parent-sha-1"), 42, Utc::now(), )) @@ -559,7 +559,7 @@ mod tests { db.insert_benchmark_request(&BenchmarkRequest::create_master( "a-sha-1", - "parent-sha-1", + Some("parent-sha-1"), 42, Utc::now(), )) @@ -568,7 +568,7 @@ mod tests { db.insert_benchmark_request(&BenchmarkRequest::create_master( "a-sha-2", - "parent-sha-2", + Some("parent-sha-2"), 42, Utc::now(), )) @@ -585,18 +585,21 @@ mod tests { run_postgres_test(|ctx| async { let db = ctx.db(); - // Releases don't need parent shas - ctx.insert_release_request("parent-sha-1").await; - ctx.insert_release_request("parent-sha-2").await; + ctx.insert_master_request("parent-sha-1", None, 68).await; + ctx.insert_master_request("parent-sha-2", None, 419).await; // ArtifactsReady - let req_a = ctx.insert_master_request("sha-1", "parent-sha-1", 42).await; + let req_a = ctx + .insert_master_request("sha-1", Some("parent-sha-1"), 42) + .await; // ArtifactsReady let req_b = ctx.insert_release_request("1.80.0").await; // WaitingForArtifacts ctx.insert_try_request(50).await; // InProgress - let req_d = ctx.insert_master_request("sha-2", "parent-sha-2", 51).await; + let req_d = ctx + .insert_master_request("sha-2", Some("parent-sha-2"), 51) + .await; // Completed ctx.insert_release_request("1.79.0").await; @@ -678,7 +681,7 @@ mod tests { let time = chrono::DateTime::from_str("2021-09-01T00:00:00.000Z").unwrap(); let benchmark_request = - BenchmarkRequest::create_master("sha-1", "parent-sha-1", 42, time); + BenchmarkRequest::create_master("sha-1", Some("parent-sha-1"), 42, time); // Insert the request so we don't violate the foreign key db.insert_benchmark_request(&benchmark_request) @@ -833,7 +836,7 @@ mod tests { .unwrap(); let benchmark_request = - BenchmarkRequest::create_master("sha-1", "parent-sha-1", 42, time); + BenchmarkRequest::create_master("sha-1", Some("parent-sha-1"), 42, time); // Insert the request so we don't violate the foreign key db.insert_benchmark_request(&benchmark_request) @@ -902,7 +905,7 @@ mod tests { assert!(insert_result.is_ok()); let benchmark_request = - BenchmarkRequest::create_master("sha-1", "parent-sha-1", 42, time); + BenchmarkRequest::create_master("sha-1", Some("parent-sha-1"), 42, time); db.insert_benchmark_request(&benchmark_request) .await .unwrap(); @@ -1045,7 +1048,7 @@ mod tests { RequestBuilder::master( db, &format!("sha{}", id), - &format!("sha{}", id - 1), + Some(&format!("sha{}", id - 1)), id, ) .await @@ -1057,7 +1060,7 @@ mod tests { } // Create an additional non-completed request - ctx.insert_master_request("foo", "bar", 1000).await; + ctx.insert_master_request("foo", Some("bar"), 1000).await; // Request 1 will have artifact with errors let aid1 = ctx.upsert_master_artifact("sha1").await; @@ -1112,10 +1115,10 @@ mod tests { let collector = ctx.add_collector(Default::default()).await; // Artifacts ready request, should be ignored - RequestBuilder::master(db, "foo", "bar", 1000).await; + RequestBuilder::master(db, "foo", Some("bar"), 1000).await; // Create a completed parent with jobs - let completed = RequestBuilder::master(db, "sha4-parent", "sha0", 1001) + let completed = RequestBuilder::master(db, "sha4-parent", Some("sha0"), 1001) .await .add_jobs( db, @@ -1126,13 +1129,13 @@ mod tests { .await; // In progress request without a parent - let req1 = RequestBuilder::master(db, "sha1", "sha0", 1) + let req1 = RequestBuilder::master(db, "sha1", Some("sha0"), 1) .await .set_in_progress(db) .await; // In progress request with a parent that has no jobs - let req2 = RequestBuilder::master(db, "sha2", "sha1", 2) + let req2 = RequestBuilder::master(db, "sha2", Some("sha1"), 2) .await .add_jobs( db, @@ -1143,7 +1146,7 @@ mod tests { .await; // In progress request with a parent that has jobs - let req3 = RequestBuilder::master(db, "sha3", "sha2", 3) + let req3 = RequestBuilder::master(db, "sha3", Some("sha2"), 3) .await .add_jobs( db, @@ -1154,7 +1157,7 @@ mod tests { .await; // In progress request with a parent that has jobs, but is completed - let req4 = RequestBuilder::master(db, "sha4", completed.tag(), 4) + let req4 = RequestBuilder::master(db, "sha4", Some(completed.tag()), 4) .await .add_jobs( db, diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs index 53ba75abf..be0dcb28b 100644 --- a/database/src/pool/postgres.rs +++ b/database/src/pool/postgres.rs @@ -1597,24 +1597,15 @@ where &self, benchmark_request: &BenchmarkRequest, ) -> anyhow::Result<()> { - // @TODO delete this when the new system has been running. This code - // prevents a foreign key violation in the job_queue table. This can - // happen as the parent_sha may not be in the range of initial commits - // we load into the database when the job queue first starts and inserts - // it's first batch of requests. - let rows = self - .conn() - .query( - "SELECT tag FROM benchmark_request WHERE tag = $1", - &[&benchmark_request.parent_sha()], - ) - .await - .context("Failed to check parent sha")?; - - let parent_sha = rows - .first() - .map(|row| row.get::<_, Option>(0).map(|parent_sha| parent_sha)); - + // We sometimes mark the `parent_sha` as NULL if the `parent_sha` does + // not exist as a `benchmark_request`. This can occur when we start the + // system and previous commits do not exist in the database. + // + // We thought about doing this at the point of job creation however + // that would then end up swallowing some possibly errors when + // inserting. Namely a job should not be created if a tag does not + // exist in the `benchmark_request` table. Thus it's living here for + // the time being. self.conn() .execute( r#" @@ -1629,11 +1620,24 @@ where profiles, commit_date ) - VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9); - "#, + VALUES ( + $1, + CASE + WHEN EXISTS (SELECT 1 FROM benchmark_request WHERE tag = $2) + THEN $2 + ELSE NULL + END, + $3, + $4, + $5, + $6, + $7, + $8, + $9 + );"#, &[ &benchmark_request.tag(), - &parent_sha, + &benchmark_request.parent_sha(), &benchmark_request.pr().map(|it| it as i32), &benchmark_request.commit_type, &benchmark_request.status.as_str(), diff --git a/database/src/tests/builder.rs b/database/src/tests/builder.rs index 48679e657..7daba4f9f 100644 --- a/database/src/tests/builder.rs +++ b/database/src/tests/builder.rs @@ -11,7 +11,7 @@ pub struct RequestBuilder { } impl RequestBuilder { - pub async fn master(db: &dyn Connection, tag: &str, parent: &str, pr: u32) -> Self { + pub async fn master(db: &dyn Connection, tag: &str, parent: Option<&str>, pr: u32) -> Self { let request = BenchmarkRequest::create_master(tag, parent, pr, Utc::now()); db.insert_benchmark_request(&request).await.unwrap(); Self { diff --git a/database/src/tests/mod.rs b/database/src/tests/mod.rs index 9d88a60ce..dff4880b6 100644 --- a/database/src/tests/mod.rs +++ b/database/src/tests/mod.rs @@ -131,7 +131,7 @@ impl TestContext { pub async fn insert_master_request( &self, sha: &str, - parent: &str, + parent: Option<&str>, pr: u32, ) -> BenchmarkRequest { let req = BenchmarkRequest::create_master(sha, parent, pr, Utc::now()); diff --git a/site/src/job_queue/mod.rs b/site/src/job_queue/mod.rs index 6c23783dd..b50bede74 100644 --- a/site/src/job_queue/mod.rs +++ b/site/src/job_queue/mod.rs @@ -47,7 +47,7 @@ async fn create_benchmark_request_master_commits( let pr = master_commit.pr.unwrap_or(0); let benchmark = BenchmarkRequest::create_master( &master_commit.sha, - &master_commit.parent_sha, + Some(&master_commit.parent_sha), pr, master_commit.time, ); @@ -401,7 +401,7 @@ mod tests { CodegenBackend, Profile, Target, }; - fn create_master(sha: &str, parent: &str, pr: u32) -> BenchmarkRequest { + fn create_master(sha: &str, parent: Option<&str>, pr: u32) -> BenchmarkRequest { BenchmarkRequest::create_master(sha, parent, pr, Utc::now()) } @@ -546,16 +546,16 @@ mod tests { * +----------------+ **/ let requests = vec![ - create_master("bar", "parent1", 10), - create_master("345", "parent2", 11), - create_master("aaa", "parent3", 12), - create_master("rrr", "parent4", 13), - create_master("123", "bar", 14), - create_master("foo", "345", 15), + create_master("bar", Some("parent1"), 10), + create_master("345", Some("parent2"), 11), + create_master("aaa", Some("parent3"), 12), + create_master("rrr", Some("parent4"), 13), + create_master("123", Some("bar"), 14), + create_master("foo", Some("345"), 15), create_try(16), create_release("v1.2.3"), create_try(17), - create_master("mmm", "aaa", 18), + create_master("mmm", Some("aaa"), 18), ]; db_insert_requests(db, &requests).await; From fbba36cf679177a7c93ea9526ed544232cf7e5e3 Mon Sep 17 00:00:00 2001 From: James Barford-Evans Date: Mon, 22 Sep 2025 15:52:31 +0100 Subject: [PATCH 5/5] only post a comparison if a parent_sha exists --- site/src/job_queue/mod.rs | 43 ++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/site/src/job_queue/mod.rs b/site/src/job_queue/mod.rs index b50bede74..5d9dcfbf0 100644 --- a/site/src/job_queue/mod.rs +++ b/site/src/job_queue/mod.rs @@ -334,9 +334,7 @@ async fn cron_enqueue_jobs(ctxt: &SiteCtxt) -> anyhow::Result<()> { false, *pr, sha.clone().expect("Completed try commit without a SHA"), - parent_sha - .clone() - .expect("Completed try commit without a parent SHA"), + parent_sha.clone(), ), BenchmarkRequestType::Master { pr, @@ -345,24 +343,27 @@ async fn cron_enqueue_jobs(ctxt: &SiteCtxt) -> anyhow::Result<()> { } => (true, *pr, sha.clone(), parent_sha.clone()), BenchmarkRequestType::Release { .. } => continue, }; - let commit = QueuedCommit { - pr, - sha, - parent_sha, - include: None, - exclude: None, - runs: None, - commit_date: request.commit_date().map(Date), - backends: Some( - request - .backends()? - .into_iter() - .map(|b| b.as_str()) - .collect::>() - .join(","), - ), - }; - post_comparison_comment(ctxt, commit, is_master).await?; + + if let Some(parent_sha) = parent_sha { + let commit = QueuedCommit { + pr, + sha, + parent_sha, + include: None, + exclude: None, + runs: None, + commit_date: request.commit_date().map(Date), + backends: Some( + request + .backends()? + .into_iter() + .map(|b| b.as_str()) + .collect::>() + .join(","), + ), + }; + post_comparison_comment(ctxt, commit, is_master).await?; + } } }