Add CI checks caching #11698
Add CI checks caching #11698
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
krlvi
force-pushed
the
cache-ci-checks-db-migration
branch
3 times, most recently
from
daf94cf to
08620a9
Compare
crates/but-db/src/ci_checks.rs
Outdated
| for check in checks { | ||
| diesel::insert_into(all_checks) | ||
| .values(&check) | ||
| .execute(conn)?; | ||
| } |
There was a problem hiding this comment.
Using individual INSERT statements in a loop can be very slow for references with many CI checks. Consider using diesel's batch insert capability by passing the entire checks vector to a single insert_into(...).values(&checks) call, which would be much more efficient.
| for check in checks { | |
| diesel::insert_into(all_checks) | |
| .values(&check) | |
| .execute(conn)?; | |
| } | |
| if !checks.is_empty() { | |
| diesel::insert_into(all_checks) | |
| .values(&checks) | |
| .execute(conn)?; | |
| } |
| if let Some(last_sync) = cached.first().map(|c| c.last_sync_at) { | ||
| let age = chrono::Local::now().naive_local() - last_sync; | ||
| if !cached.is_empty() && age.num_seconds() as u64 <= max_age_seconds { |
There was a problem hiding this comment.
The age calculation uses the last_sync_at field from the first check, assuming all checks in the list have the same sync time. If checks could have different sync times (e.g., from partial cache updates), this logic may incorrectly invalidate or use stale data. Consider checking the minimum or maximum last_sync_at across all cached checks, or storing a single timestamp per reference.
| if let Some(last_sync) = cached.first().map(|c| c.last_sync_at) { | |
| let age = chrono::Local::now().naive_local() - last_sync; | |
| if !cached.is_empty() && age.num_seconds() as u64 <= max_age_seconds { | |
| if let Some(last_sync) = cached.iter().map(|c| c.last_sync_at).min() { | |
| let age = chrono::Local::now().naive_local() - last_sync; | |
| if age.num_seconds() as u64 <= max_age_seconds { |
| for branch in &details.branch_details { | ||
| current_refs.insert(branch.name.to_string()); | ||
|
|
||
| // Only fetch CI checks for branches with PRs | ||
| if branch.pr_number.is_some() { | ||
| // Fetch CI checks with NoCache to force refresh | ||
| let _ = list_ci_checks( | ||
| &mut ctx, | ||
| branch.name.to_string(), | ||
| Some(but_forge::CacheConfig::NoCache), | ||
| ); | ||
| // Ignore errors for individual branches to ensure we process all branches | ||
| } | ||
| } |
There was a problem hiding this comment.
The function collects all branch names into current_refs including those without PRs, but only fetches CI checks for branches with PRs. This means branches without PRs could prevent cache cleanup of their reference even when they no longer have CI checks. Consider only adding branches to current_refs when they actually have PRs and CI checks were fetched for them.
| let db_checks: Vec<but_db::CiCheck> = checks | ||
| .iter() | ||
| .map(|c| c.clone().try_into()) |
There was a problem hiding this comment.
Cloning each check during the mapping operation is unnecessary and impacts performance. Consider using .map(|c| c.try_into()) and changing the signature of TryFrom implementation to consume the value, or use references throughout if the original values need to be preserved.
| if value.struct_version != CiCheck::struct_version() { | ||
| return Err(anyhow::Error::msg(format!( | ||
| "Incompatible CiCheck struct version: expected {}, found {}", | ||
| CiCheck::struct_version(), | ||
| value.struct_version | ||
| ))); | ||
| } |
There was a problem hiding this comment.
The version check returns an error if struct versions don't match, but this could lead to data loss on version upgrades. Consider implementing migration logic or gracefully handling old cached data by either ignoring incompatible entries or attempting to migrate them to the new format.
| let reference = reference.to_string(); | ||
| let reference_for_checks = reference.clone(); |
There was a problem hiding this comment.
The reference is cloned twice unnecessarily. Line 58 converts to String and then line 59 immediately clones it. Consider removing the intermediate clone and using reference.to_string() directly on line 59, or storing it in a single variable that gets moved into the closure.
crates/but-forge/src/ci.rs
Outdated
| .map(|check| { | ||
| let mut ci_check = CiCheck::from(check); | ||
| ci_check.reference = reference_for_checks.clone(); |
There was a problem hiding this comment.
The reference string is cloned for every check in the iterator. Since all checks for a reference share the same reference value, consider using Arc<String> or moving the reference assignment outside the map by constructing checks with a shared reference, to avoid repeated allocations.
| let _ = list_ci_checks( | ||
| &mut ctx, | ||
| branch.name.to_string(), | ||
| Some(but_forge::CacheConfig::NoCache), | ||
| ); | ||
| // Ignore errors for individual branches to ensure we process all branches |
There was a problem hiding this comment.
Errors from individual branch CI check fetches are silently ignored. While the comment indicates this is intentional, consider logging these errors at debug or trace level so that failures can be diagnosed when investigating cache warming issues.
| crate::db::cache_ci_checks(db, reference, &checks).ok(); | ||
| checks | ||
| } | ||
| crate::CacheConfig::NoCache => { | ||
| let checks = | ||
| ci_checks_for_ref(preferred_forge_user, forge_repo_info, storage, reference)?; | ||
| crate::db::cache_ci_checks(db, reference, &checks).ok(); |
There was a problem hiding this comment.
Errors from cache operations are silently ignored with .ok(). While this prevents cache failures from breaking the main flow, it means that caching failures go unnoticed. Consider logging these errors at debug level to help diagnose caching issues in production.
krlvi
force-pushed
the
cache-ci-checks-db-migration
branch
from
0583c60 to
a7b9973
Compare
krlvi
force-pushed
the
cache-ci-checks-db-migration
branch
from
a7b9973 to
07f7a5c
Compare
No description provided.