diff --git a/src/build_queue.rs b/src/build_queue.rs index 506acee9b..6bf9600d8 100644 --- a/src/build_queue.rs +++ b/src/build_queue.rs @@ -1,10 +1,10 @@ -use crate::cdn; use crate::db::{delete_crate, delete_version, update_latest_version_id, Pool}; use crate::docbuilder::PackageKind; use crate::error::Result; use crate::storage::Storage; use crate::utils::{get_config, get_crate_priority, report_error, retry, set_config, ConfigName}; use crate::Context; +use crate::{cdn, BuildPackageSummary}; use crate::{Config, Index, InstanceMetrics, RustwideBuilder}; use anyhow::Context as _; use fn_error_context::context; @@ -168,7 +168,10 @@ impl BuildQueue { .is_some()) } - fn process_next_crate(&self, f: impl FnOnce(&QueuedCrate) -> Result<()>) -> Result<()> { + fn process_next_crate( + &self, + f: impl FnOnce(&QueuedCrate) -> Result, + ) -> Result<()> { let mut conn = self.db.get()?; let mut transaction = conn.transaction()?; @@ -204,14 +207,11 @@ impl BuildQueue { None => return Ok(()), }; - let res = self.metrics.build_time.observe_closure_duration(|| { - f(&to_process).with_context(|| { - format!( - "Failed to build package {}-{} from queue", - to_process.name, to_process.version - ) - }) - }); + let res = self + .metrics + .build_time + .observe_closure_duration(|| f(&to_process)); + self.metrics.total_builds.inc(); if let Err(err) = cdn::queue_crate_invalidation(&mut transaction, &self.config, &to_process.name) @@ -219,29 +219,44 @@ impl BuildQueue { report_error(&err); } - match res { - Ok(()) => { - transaction.execute("DELETE FROM queue WHERE id = $1;", &[&to_process.id])?; - } - Err(e) => { - // Increase attempt count - let attempt: i32 = transaction - .query_one( - "UPDATE queue + let mut increase_attempt_count = || -> Result<()> { + let attempt: i32 = transaction + .query_one( + "UPDATE queue SET attempt = attempt + 1, last_attempt = NOW() WHERE id = $1 RETURNING attempt;", - &[&to_process.id], - )? - .get(0); + &[&to_process.id], + )? + .get(0); - if attempt >= self.max_attempts { - self.metrics.failed_builds.inc(); - } + if attempt >= self.max_attempts { + self.metrics.failed_builds.inc(); + } + Ok(()) + }; - report_error(&e); + match res { + Ok(BuildPackageSummary { + should_reattempt: false, + successful: _, + }) => { + transaction.execute("DELETE FROM queue WHERE id = $1;", &[&to_process.id])?; + } + Ok(BuildPackageSummary { + should_reattempt: true, + successful: _, + }) => { + increase_attempt_count()?; + } + Err(e) => { + increase_attempt_count()?; + report_error(&e.context(format!( + "Failed to build package {}-{} from queue", + to_process.name, to_process.version + ))) } } @@ -521,8 +536,7 @@ impl BuildQueue { return Err(err); } - builder.build_package(&krate.name, &krate.version, kind)?; - Ok(()) + builder.build_package(&krate.name, &krate.version, kind) })?; Ok(processed) @@ -644,7 +658,7 @@ mod tests { queue.process_next_crate(|krate| { assert_eq!(krate.name, "krate"); handled = true; - Ok(()) + Ok(BuildPackageSummary::default()) })?; assert!(handled); @@ -680,7 +694,7 @@ mod tests { let assert_next = |name| -> Result<()> { queue.process_next_crate(|krate| { assert_eq!(name, krate.name); - Ok(()) + Ok(BuildPackageSummary::default()) })?; Ok(()) }; @@ -720,7 +734,7 @@ mod tests { let mut called = false; queue.process_next_crate(|_| { called = true; - Ok(()) + Ok(BuildPackageSummary::default()) })?; assert!(!called, "there were still items in the queue"); @@ -755,7 +769,7 @@ mod tests { queue.process_next_crate(|krate| { assert_eq!("will_succeed", krate.name); - Ok(()) + Ok(BuildPackageSummary::default()) })?; let queued_invalidations = cdn::queued_or_active_crate_invalidations(&mut *conn)?; @@ -793,7 +807,7 @@ mod tests { queue.process_next_crate(|krate| { assert_eq!("foo", krate.name); - Ok(()) + Ok(BuildPackageSummary::default()) })?; assert_eq!(queue.pending_count()?, 1); @@ -816,7 +830,7 @@ mod tests { queue.process_next_crate(|krate| { assert_eq!("bar", krate.name); - Ok(()) + Ok(BuildPackageSummary::default()) })?; assert_eq!(queue.prioritized_count()?, 1); @@ -841,7 +855,7 @@ mod tests { ); while queue.pending_count()? > 0 { - queue.process_next_crate(|_| Ok(()))?; + queue.process_next_crate(|_| Ok(BuildPackageSummary::default()))?; } assert!(queue.pending_count_by_priority()?.is_empty()); @@ -850,7 +864,44 @@ mod tests { } #[test] - fn test_failed_count() { + fn test_failed_count_for_reattempts() { + const MAX_ATTEMPTS: u16 = 3; + crate::test::wrapper(|env| { + env.override_config(|config| { + config.build_attempts = MAX_ATTEMPTS; + config.delay_between_build_attempts = Duration::ZERO; + }); + let queue = env.build_queue(); + + assert_eq!(queue.failed_count()?, 0); + queue.add_crate("foo", "1.0.0", -100, None)?; + assert_eq!(queue.failed_count()?, 0); + queue.add_crate("bar", "1.0.0", 0, None)?; + + for _ in 0..MAX_ATTEMPTS { + assert_eq!(queue.failed_count()?, 0); + queue.process_next_crate(|krate| { + assert_eq!("foo", krate.name); + Ok(BuildPackageSummary { + should_reattempt: true, + ..Default::default() + }) + })?; + } + assert_eq!(queue.failed_count()?, 1); + + queue.process_next_crate(|krate| { + assert_eq!("bar", krate.name); + Ok(BuildPackageSummary::default()) + })?; + assert_eq!(queue.failed_count()?, 1); + + Ok(()) + }); + } + + #[test] + fn test_failed_count_after_error() { const MAX_ATTEMPTS: u16 = 3; crate::test::wrapper(|env| { env.override_config(|config| { @@ -875,7 +926,7 @@ mod tests { queue.process_next_crate(|krate| { assert_eq!("bar", krate.name); - Ok(()) + Ok(BuildPackageSummary::default()) })?; assert_eq!(queue.failed_count()?, 1); diff --git a/src/docbuilder/mod.rs b/src/docbuilder/mod.rs index 9328c2203..80764d9ec 100644 --- a/src/docbuilder/mod.rs +++ b/src/docbuilder/mod.rs @@ -3,4 +3,4 @@ mod rustwide_builder; pub(crate) use self::limits::Limits; pub(crate) use self::rustwide_builder::DocCoverage; -pub use self::rustwide_builder::{PackageKind, RustwideBuilder}; +pub use self::rustwide_builder::{BuildPackageSummary, PackageKind, RustwideBuilder}; diff --git a/src/docbuilder/rustwide_builder.rs b/src/docbuilder/rustwide_builder.rs index 0b7629767..21b2509b6 100644 --- a/src/docbuilder/rustwide_builder.rs +++ b/src/docbuilder/rustwide_builder.rs @@ -331,7 +331,7 @@ impl RustwideBuilder { Ok(()) } - pub fn build_local_package(&mut self, path: &Path) -> Result { + pub fn build_local_package(&mut self, path: &Path) -> Result { let metadata = CargoMetadata::load_from_rustwide(&self.workspace, &self.toolchain, path) .map_err(|err| { err.context(format!("failed to load local package {}", path.display())) @@ -346,7 +346,7 @@ impl RustwideBuilder { name: &str, version: &str, kind: PackageKind<'_>, - ) -> Result { + ) -> Result { let build_id = self.runtime.block_on(async { let mut conn = self.db.get_async().await?; let crate_id = initialize_crate(&mut conn, name).await?; @@ -356,16 +356,23 @@ impl RustwideBuilder { })?; match self.build_package_inner(name, version, kind, build_id) { - Ok(successful) => Ok(successful), + Ok(successful) => Ok(BuildPackageSummary { + successful, + should_reattempt: false, + }), Err(err) => self.runtime.block_on(async { // NOTE: this might hide some errors from us, while only surfacing them in the build // result. // At some point we might introduce a special error type which additionally reports // to sentry. let mut conn = self.db.get_async().await?; + update_build_with_error(&mut conn, build_id, Some(&format!("{:?}", err))).await?; - Ok(false) + Ok(BuildPackageSummary { + successful: false, + should_reattempt: true, + }) }), } } @@ -964,6 +971,22 @@ pub(crate) struct BuildResult { pub(crate) successful: bool, } +#[derive(Debug)] +pub struct BuildPackageSummary { + pub successful: bool, + pub should_reattempt: bool, +} + +#[cfg(test)] +impl Default for BuildPackageSummary { + fn default() -> Self { + Self { + successful: true, + should_reattempt: false, + } + } +} + #[cfg(test)] mod tests { use super::*; @@ -1017,7 +1040,11 @@ mod tests { let mut builder = RustwideBuilder::init(env).unwrap(); builder.update_toolchain()?; - assert!(builder.build_package(crate_, version, PackageKind::CratesIo)?); + assert!( + builder + .build_package(crate_, version, PackageKind::CratesIo)? + .successful + ); // check release record in the db (default and other targets) let mut conn = env.db().conn(); @@ -1163,7 +1190,11 @@ mod tests { let mut builder = RustwideBuilder::init(env).unwrap(); builder.update_toolchain()?; - assert!(!builder.build_package(crate_, version, PackageKind::CratesIo)?); + assert!( + !builder + .build_package(crate_, version, PackageKind::CratesIo)? + .successful + ); // check release record in the db (default and other targets) let mut conn = env.db().conn(); @@ -1211,7 +1242,11 @@ mod tests { let version = "1.0.26"; let mut builder = RustwideBuilder::init(env).unwrap(); builder.update_toolchain()?; - assert!(builder.build_package(crate_, version, PackageKind::CratesIo)?); + assert!( + builder + .build_package(crate_, version, PackageKind::CratesIo)? + .successful + ); let storage = env.storage(); @@ -1238,7 +1273,11 @@ mod tests { if builder.toolchain.as_ci().is_some() { return Ok(()); } - assert!(builder.build_package(crate_, version, PackageKind::CratesIo)?); + assert!( + builder + .build_package(crate_, version, PackageKind::CratesIo)? + .successful + ); let storage = env.storage(); @@ -1288,7 +1327,11 @@ mod tests { let version = "0.1.2"; let mut builder = RustwideBuilder::init(env).unwrap(); builder.update_toolchain()?; - assert!(builder.build_package(crate_, version, PackageKind::CratesIo)?); + assert!( + builder + .build_package(crate_, version, PackageKind::CratesIo)? + .successful + ); Ok(()) }); @@ -1311,7 +1354,11 @@ mod tests { let version = "0.2.0"; let mut builder = RustwideBuilder::init(env).unwrap(); builder.update_toolchain()?; - assert!(builder.build_package(crate_, version, PackageKind::CratesIo)?); + assert!( + builder + .build_package(crate_, version, PackageKind::CratesIo)? + .successful + ); Ok(()) }); @@ -1325,7 +1372,11 @@ mod tests { let version = "1.0.33"; let mut builder = RustwideBuilder::init(env).unwrap(); builder.update_toolchain()?; - assert!(builder.build_package(crate_, version, PackageKind::CratesIo)?); + assert!( + builder + .build_package(crate_, version, PackageKind::CratesIo)? + .successful + ); Ok(()) }); } @@ -1344,7 +1395,11 @@ mod tests { builder.update_toolchain()?; // `Result` is `Ok`, but the build-result is `false` - assert!(!builder.build_package(crate_, version, PackageKind::CratesIo)?); + assert!( + !builder + .build_package(crate_, version, PackageKind::CratesIo)? + .successful + ); // source archice exists let source_archive = source_archive_path(crate_, version); @@ -1370,7 +1425,10 @@ mod tests { builder.update_toolchain()?; // `Result` is `Ok`, but the build-result is `false` - assert!(!builder.build_package(crate_, version, PackageKind::CratesIo)?); + let summary = builder.build_package(crate_, version, PackageKind::CratesIo)?; + + assert!(!summary.successful); + assert!(summary.should_reattempt); let row = env.runtime().block_on(async { let mut conn = env.async_db().await.async_conn().await; @@ -1409,7 +1467,11 @@ mod tests { let version = "1.0.152"; let mut builder = RustwideBuilder::init(env).unwrap(); builder.update_toolchain()?; - assert!(builder.build_package(crate_, version, PackageKind::CratesIo)?); + assert!( + builder + .build_package(crate_, version, PackageKind::CratesIo)? + .successful + ); let mut conn = env.db().conn(); let features: Vec = conn @@ -1436,7 +1498,11 @@ mod tests { let version = "0.1.1"; let mut builder = RustwideBuilder::init(env).unwrap(); builder.update_toolchain()?; - assert!(builder.build_package(crate_, version, PackageKind::CratesIo)?); + assert!( + builder + .build_package(crate_, version, PackageKind::CratesIo)? + .successful + ); let mut conn = env.db().conn(); let features: Vec = conn @@ -1461,7 +1527,11 @@ mod tests { wrapper(|env| { let mut builder = RustwideBuilder::init(env)?; builder.update_toolchain()?; - assert!(builder.build_local_package(Path::new("tests/crates/build-std"))?); + assert!( + builder + .build_local_package(Path::new("tests/crates/build-std"))? + .successful + ); Ok(()) }) } @@ -1473,7 +1543,11 @@ mod tests { let mut builder = RustwideBuilder::init(env)?; builder.update_toolchain()?; builder.reinitialize_workspace_if_interval_passed(env)?; - assert!(builder.build_local_package(Path::new("tests/crates/build-std"))?); + assert!( + builder + .build_local_package(Path::new("tests/crates/build-std"))? + .successful + ); Ok(()) }) } @@ -1489,10 +1563,18 @@ mod tests { }); let mut builder = RustwideBuilder::init(env)?; builder.update_toolchain()?; - assert!(builder.build_local_package(Path::new("tests/crates/build-std"))?); + assert!( + builder + .build_local_package(Path::new("tests/crates/build-std"))? + .successful + ); sleep(Duration::from_secs(1)); builder.reinitialize_workspace_if_interval_passed(env)?; - assert!(builder.build_local_package(Path::new("tests/crates/build-std"))?); + assert!( + builder + .build_local_package(Path::new("tests/crates/build-std"))? + .successful + ); Ok(()) }) } @@ -1509,11 +1591,11 @@ mod tests { // new builder should detect the existing rustc version from the previous builder // (simulating running `update-toolchain` and `build crate` in separate invocations) let mut builder = RustwideBuilder::init(env)?; - assert!(builder.build_package( - DUMMY_CRATE_NAME, - DUMMY_CRATE_VERSION, - PackageKind::CratesIo - )?); + assert!( + builder + .build_package(DUMMY_CRATE_NAME, DUMMY_CRATE_VERSION, PackageKind::CratesIo)? + .successful + ); assert_eq!(old_version, builder.rustc_version()?); Ok(()) diff --git a/src/lib.rs b/src/lib.rs index bbeb2a3a6..8fe4904f1 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -6,7 +6,7 @@ pub use self::build_queue::BuildQueue; pub use self::config::Config; pub use self::context::Context; pub use self::docbuilder::PackageKind; -pub use self::docbuilder::RustwideBuilder; +pub use self::docbuilder::{BuildPackageSummary, RustwideBuilder}; pub use self::index::Index; pub use self::metrics::{InstanceMetrics, ServiceMetrics}; pub use self::registry_api::RegistryApi;