consistency: warn & continue when errors happen while fixing inconsis…

…tencies
rust-lang · Feb 4, 2023 · 2eda218 · 2eda218
1 parent a56e1ee
commit 2eda218
Show file tree

Hide file tree

Showing 4 changed files with 66 additions and 49 deletions.
diff --git a/Cargo.toml b/Cargo.toml
@@ -97,6 +97,7 @@ percent-encoding = "2.2.0"
 
 # NOTE: if you change this, also double-check that the comment in `queue_builder::remove_tempdirs` is still accurate.
 tempfile = "3.1.0"
+fn-error-context = "0.2.0"
 
 # Templating
 tera = { version = "1.5.0", features = ["builtins"] }
@@ -122,7 +123,6 @@ kuchiki = "0.8"
 rand = "0.8"
 mockito = "0.31.0"
 test-case = "2.0.0"
-fn-error-context = "0.2.0"
 aws-smithy-client = { version = "0.53.0", features = ["test-util"]}
 aws-smithy-http = "0.53.0"
 indoc = "1.0.7"

diff --git a/src/build_queue.rs b/src/build_queue.rs
@@ -6,6 +6,7 @@ use crate::storage::Storage;
 use crate::utils::{get_config, get_crate_priority, report_error, set_config, ConfigName};
 use crate::{Config, Index, Metrics, RustwideBuilder};
 use anyhow::Context;
+use fn_error_context::context;
 
 use tracing::{debug, error, info, warn};
 
@@ -69,6 +70,7 @@ impl BuildQueue {
         Ok(())
     }
 
+    #[context("error trying to add {name}-{version} to build queue")]
     pub fn add_crate(
         &self,
         name: &str,
@@ -342,15 +344,16 @@ impl BuildQueue {
             let yanked = change.yanked();
             let unyanked = change.unyanked();
             if let Some(release) = yanked.or(unyanked) {
-                // FIXME:  https://github.com/rust-lang/docs.rs/issues/1934
-                // we sometimes have inconsistencies between our yank-state and
-                // the crates.io yank state, and we don't know why yet.
-                self.set_yanked(
+                // FIXME: delay yanks of crates that have not yet finished building
+                // https://github.com/rust-lang/docs.rs/issues/1934
+                if let Err(err) = self.set_yanked(
                     &mut conn,
                     release.name.as_str(),
                     release.version.as_str(),
                     yanked.is_some(),
-                );
+                ) {
+                    report_error(&err);
+                }
 
                 if let Err(err) =
                     cdn::queue_crate_invalidation(&mut *conn, &self.config, &release.name)
@@ -372,48 +375,48 @@ impl BuildQueue {
         Ok(crates_added)
     }
 
-    pub fn set_yanked(&self, conn: &mut postgres::Client, name: &str, version: &str, yanked: bool) {
+    #[context("error trying to set {name}-{version} to yanked: {yanked}")]
+    pub fn set_yanked(
+        &self,
+        conn: &mut postgres::Client,
+        name: &str,
+        version: &str,
+        yanked: bool,
+    ) -> Result<()> {
         let activity = if yanked { "yanked" } else { "unyanked" };
 
-        match conn
-            .execute(
-                "UPDATE releases
-                     SET yanked = $3
-                 FROM crates
-                 WHERE crates.id = releases.crate_id
-                     AND name = $1
-                     AND version = $2
-                ",
-                &[&name, &version, &yanked],
-            )
-            .with_context(|| format!("error while setting {}-{} to {}", name, version, activity))
-        {
-            Ok(rows) => {
-                if rows != 1 {
-                    match self
-                        .has_build_queued(name, version)
-                        .context("error trying to fetch build queue")
-                    {
-                        Ok(false) => {
-                            // the rustwide builder will fetch the current yank state from
-                            // crates.io, so and missed update here will be fixed after the
-                            // build is finished.
-                            error!(
-                                "tried to yank or unyank non-existing release: {} {}",
-                                name, version
-                            );
-                        }
-                        Ok(true) => {}
-                        Err(err) => {
-                            report_error(&err);
-                        }
-                    }
-                } else {
-                    debug!("{}-{} {}", name, version, activity);
+        let rows = conn.execute(
+            "UPDATE releases
+             SET yanked = $3
+             FROM crates
+             WHERE crates.id = releases.crate_id
+                 AND name = $1
+                 AND version = $2",
+            &[&name, &version, &yanked],
+        )?;
+        if rows != 1 {
+            match self
+                .has_build_queued(name, version)
+                .context("error trying to fetch build queue")
+            {
+                Ok(false) => {
+                    // the rustwide builder will fetch the current yank state from
+                    // crates.io, so and missed update here will be fixed after the
+                    // build is finished.
+                    error!(
+                        "tried to yank or unyank non-existing release: {} {}",
+                        name, version
+                    );
+                }
+                Ok(true) => {}
+                Err(err) => {
+                    report_error(&err);
                 }
             }
-            Err(err) => report_error(&err),
+        } else {
+            debug!("{}-{} {}", name, version, activity);
         }
+        Ok(())
     }
 
     /// Builds the top package from the queue. Returns whether there was a package in the queue.

diff --git a/src/db/delete.rs b/src/db/delete.rs
@@ -2,6 +2,7 @@ use crate::error::Result;
 use crate::storage::{rustdoc_archive_path, source_archive_path, Storage};
 use crate::{Config, Context};
 use anyhow::Context as _;
+use fn_error_context::context;
 use postgres::Client;
 use std::fs;
 
@@ -16,6 +17,7 @@ enum CrateDeletionError {
     MissingCrate(String),
 }
 
+#[context("error trying to delete crate {name} from database")]
 pub fn delete_crate(
     conn: &mut Client,
     storage: &Storage,
@@ -52,6 +54,7 @@ pub fn delete_crate(
     Ok(())
 }
 
+#[context("error trying to delete release {name}-{version} from database")]
 pub fn delete_version(ctx: &dyn Context, name: &str, version: &str) -> Result<()> {
     let conn = &mut ctx.pool()?.get()?;
     let storage = ctx.storage()?;

diff --git a/src/utils/consistency/mod.rs b/src/utils/consistency/mod.rs
@@ -1,7 +1,7 @@
 use crate::{build_queue, db::delete, Context};
 use anyhow::{Context as _, Result};
 use itertools::Itertools;
-use tracing::info;
+use tracing::{info, warn};
 
 mod data;
 mod db;
@@ -92,33 +92,44 @@ where
         match difference {
             diff::Difference::CrateNotInIndex(name) => {
                 if !dry_run {
-                    delete::delete_crate(&mut conn, &storage, &config, name)?;
+                    if let Err(err) = delete::delete_crate(&mut conn, &storage, &config, name) {
+                        warn!("{:?}", err);
+                    }
                 }
                 result.crates_deleted += 1;
             }
             diff::Difference::CrateNotInDb(name, versions) => {
                 for version in versions {
                     if !dry_run {
-                        build_queue.add_crate(name, version, BUILD_PRIORITY, None)?;
+                        if let Err(err) = build_queue.add_crate(name, version, BUILD_PRIORITY, None)
+                        {
+                            warn!("{:?}", err);
+                        }
                     }
                     result.builds_queued += 1;
                 }
             }
             diff::Difference::ReleaseNotInIndex(name, version) => {
                 if !dry_run {
-                    delete::delete_version(ctx, name, version)?;
+                    if let Err(err) = delete::delete_version(ctx, name, version) {
+                        warn!("{:?}", err);
+                    }
                 }
                 result.releases_deleted += 1;
             }
             diff::Difference::ReleaseNotInDb(name, version) => {
                 if !dry_run {
-                    build_queue.add_crate(name, version, BUILD_PRIORITY, None)?;
+                    if let Err(err) = build_queue.add_crate(name, version, BUILD_PRIORITY, None) {
+                        warn!("{:?}", err);
+                    }
                 }
                 result.builds_queued += 1;
             }
             diff::Difference::ReleaseYank(name, version, yanked) => {
                 if !dry_run {
-                    build_queue::set_yanked(&mut conn, name, version, *yanked);
+                    if let Err(err) = build_queue::set_yanked(&mut conn, name, version, *yanked) {
+                        warn!("{:?}", err);
+                    }
                 }
                 result.yanks_corrected += 1;
             }