Skip to content

Fix; job_queue insert only if tag exists in benchmark_request #2254

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

Fix; job_queue insert only if tag exists in benchmark_request #2254

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9e4ee29
Fix; `job_queue` insert only if tag exists in `benchmark_request`
Jamesbarford Sep 19, 2025
75853ce
make master commit parent_sha Nullable
Jamesbarford Sep 22, 2025
d15ef77
fix up test
Jamesbarford Sep 22, 2025
49f9b92
Refactor all instances of a `master` commit to have the `parent_sha` …
Jamesbarford Sep 22, 2025
fbba36c
only post a comparison if a parent_sha exists
Jamesbarford Sep 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions database/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -872,7 +872,7 @@ pub enum BenchmarkRequestType {
/// A Master commit
Master {
sha: String,
parent_sha: String,
parent_sha: Option<String>,
pr: u32,
},
/// A release only has a tag
Expand Down Expand Up @@ -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<Utc>) -> Self {
pub fn create_master(
sha: &str,
parent_sha: Option<&str>,
pr: u32,
commit_date: DateTime<Utc>,
) -> Self {
Self {
commit_type: BenchmarkRequestType::Master {
pr,
sha: sha.to_string(),
parent_sha: parent_sha.to_string(),
parent_sha: parent_sha.map(|it| it.to_string()),
},
commit_date: Some(commit_date),
created_at: Utc::now(),
Expand Down Expand Up @@ -968,8 +973,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,
}
}
Expand Down Expand Up @@ -1036,7 +1041,7 @@ impl BenchmarkRequest {
}

pub fn is_in_progress(&self) -> bool {
matches!(self.status, BenchmarkRequestStatus::InProgress { .. })
matches!(self.status, BenchmarkRequestStatus::InProgress)
}
}

Expand Down
43 changes: 23 additions & 20 deletions database/src/pool.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
))
Expand Down Expand Up @@ -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(),
))
Expand All @@ -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(),
))
Expand All @@ -585,23 +585,26 @@ mod tests {
run_postgres_test(|ctx| async {
let db = ctx.db();

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;

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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -1045,7 +1048,7 @@ mod tests {
RequestBuilder::master(
db,
&format!("sha{}", id),
&format!("sha{}", id - 1),
Some(&format!("sha{}", id - 1)),
id,
)
.await
Expand All @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand Down
28 changes: 25 additions & 3 deletions database/src/pool/postgres.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1597,6 +1597,15 @@ where
&self,
benchmark_request: &BenchmarkRequest,
) -> anyhow::Result<()> {
// 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#"
Expand All @@ -1611,8 +1620,21 @@ 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(),
&benchmark_request.parent_sha(),
Expand Down Expand Up @@ -2290,7 +2312,7 @@ fn row_to_benchmark_request(row: &Row, row_offset: Option<usize>) -> 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,
Expand Down
2 changes: 1 addition & 1 deletion database/src/tests/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion database/src/tests/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
61 changes: 31 additions & 30 deletions site/src/job_queue/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
Expand Down Expand Up @@ -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,
Expand All @@ -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::<Vec<_>>()
.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::<Vec<_>>()
.join(","),
),
};
post_comparison_comment(ctxt, commit, is_master).await?;
}
}
}

Expand Down Expand Up @@ -401,7 +402,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())
}

Expand Down Expand Up @@ -546,16 +547,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;
Expand Down
Loading