[1/n] [downloader] dedupe helpers, fix error-message bugs

Cargo.toml

@@ -433,6 +433,9 @@ declare_interior_mutable_const = "warn"
 # We'd like to warn on lossy casts in the future, but lossless casts are the
 # easiest ones to convert over.
 cast_lossless = "warn"
+# This lint catches format-style placeholders in string literals outside format
+# macros, e.g. `.context("failed to read {path}")`.
+literal_string_with_formatting_args = "warn"
 
 [workspace.dependencies]
 anyhow = "1.0"

dev-tools/downloader/src/lib.rs

@@ -363,7 +363,10 @@ async fn get_values_from_file<const N: usize>(
         }
     }
     if !keys.is_empty() {
-        bail!("Could not find keys: {:?}", keys.keys().collect::<Vec<_>>(),);
+        bail!(
+            "Could not find keys {:?} in {path}",
+            keys.keys().collect::<Vec<_>>(),
+        );
     }
     Ok(values)
 }
@@ -449,30 +452,21 @@ async fn unpack_gzip(
     task.await?
 }
 
-async fn clickhouse_confirm_binary_works(binary: &Utf8Path) -> Result<()> {
-    let mut cmd = Command::new(binary);
-    cmd.args(["server", "--version"]);
-
-    let output =
-        cmd.output().await.context(format!("Failed to run {binary}"))?;
-    if !output.status.success() {
-        let stderr =
-            String::from_utf8(output.stderr).unwrap_or_else(|_| String::new());
-        bail!("{binary} failed: {} (stderr: {stderr})", output.status);
-    }
-    Ok(())
-}
-
-async fn cockroach_confirm_binary_works(binary: &Utf8Path) -> Result<()> {
+async fn confirm_binary_works(binary: &Utf8Path, args: &[&str]) -> Result<()> {
     let mut cmd = Command::new(binary);
-    cmd.arg("version");
+    cmd.args(args);
 
-    let output =
-        cmd.output().await.context(format!("Failed to run {binary}"))?;
+    let output = cmd
+        .output()
+        .await
+        .with_context(|| format!("Failed to run {binary}"))?;
     if !output.status.success() {
-        let stderr =
-            String::from_utf8(output.stderr).unwrap_or_else(|_| String::new());
-        bail!("{binary} failed: {} (stderr: {stderr})", output.status);
+        let stdout = String::from_utf8_lossy(&output.stdout);
+        let stderr = String::from_utf8_lossy(&output.stderr);
+        bail!(
+            "{binary} {args:?} failed: {} (stdout: {stdout}, stderr: {stderr})",
+            output.status
+        );
     }
     Ok(())
 }
@@ -566,6 +560,18 @@ async fn download_file_and_verify(
 }
 
 impl Downloader<'_> {
+    async fn read_version_file(&self, filename: &str) -> Result<String> {
+        let path = self.versions_dir.join(filename);
+        let contents = tokio::fs::read_to_string(&path)
+            .await
+            .with_context(|| format!("Failed to read version from {path}"))?;
+        let version = contents.trim();
+        if version.is_empty() {
+            bail!("Version file {path} should not be empty");
+        }
+        Ok(version.to_string())
+    }
+
     async fn download_cargo_hack(&self) -> Result<()> {
         let os = os_name()?;
         let arch = arch()?;
@@ -580,11 +586,7 @@ impl Downloader<'_> {
         )
         .await?;
 
-        let versions_path = self.versions_dir.join("cargo_hack_version");
-        let version = tokio::fs::read_to_string(&versions_path)
-            .await
-            .context("Failed to read version from {versions_path}")?;
-        let version = version.trim();
+        let version = self.read_version_file("cargo_hack_version").await?;
 
         let (platform, supported_arch) = match (os, arch) {
             (Os::Illumos, Arch::X86_64) => ("unknown-illumos", "x86_64"),
@@ -632,11 +634,7 @@ impl Downloader<'_> {
         )
         .await?;
 
-        let versions_path = self.versions_dir.join("clickhouse_version");
-        let version = tokio::fs::read_to_string(&versions_path)
-            .await
-            .context("Failed to read version from {versions_path}")?;
-        let version = version.trim();
+        let version = self.read_version_file("clickhouse_version").await?;
 
         const S3_BUCKET: &'static str =
             "https://oxide-clickhouse-build.s3.us-west-2.amazonaws.com";
@@ -668,7 +666,8 @@ impl Downloader<'_> {
         let clickhouse_binary = destination_dir.join("clickhouse");
 
         info!(self.log, "Checking that binary works");
-        clickhouse_confirm_binary_works(&clickhouse_binary).await?;
+        confirm_binary_works(&clickhouse_binary, &["server", "--version"])
+            .await?;
 
         Ok(())
     }
@@ -725,10 +724,12 @@ impl Downloader<'_> {
         let binary_dir = destination_dir.join("bin");
         tokio::fs::create_dir_all(&binary_dir).await?;
         let src = tarball_path.with_file_name("cockroach").join("cockroach");
-        tokio::fs::copy(src, &cockroach_binary).await?;
+        tokio::fs::copy(&src, &cockroach_binary).await.with_context(|| {
+            format!("Failed to copy {src} to {cockroach_binary}")
+        })?;
 
         info!(self.log, "Checking that binary works");
-        cockroach_confirm_binary_works(&cockroach_binary).await?;
+        confirm_binary_works(&cockroach_binary, &["version"]).await?;
 
         Ok(())
     }

dev-tools/omdb/src/bin/omdb/db.rs

@@ -4452,6 +4452,9 @@ async fn cmd_db_region_replacement_status(
                         .await?;
 
                 if let Some(repair_progress) = maybe_repair_progress {
+                    // {wide_bar:.green}, etc. are indicatif template
+                    // placeholders, not Rust format args.
+                    #[expect(clippy::literal_string_with_formatting_args)]
                     let bar = ProgressBar::with_draw_target(
                         Some(repair_progress.total_items as u64),
                         ProgressDrawTarget::stdout(),
@@ -4552,6 +4555,9 @@ async fn cmd_db_region_replacement_info(
                     .await?;
 
             if let Some(repair_progress) = maybe_repair_progress {
+                // {wide_bar:.green}, etc. are indicatif template
+                // placeholders, not Rust format args.
+                #[expect(clippy::literal_string_with_formatting_args)]
                 let bar = ProgressBar::with_draw_target(
                     Some(repair_progress.total_items as u64),
                     ProgressDrawTarget::stdout(),

dev-tools/repo-depot-standalone/src/main.rs

@@ -98,8 +98,9 @@ impl RepoDepotStandalone {
                 .load_zip_path(repo_path.clone(), &log)
                 .await
                 .with_context(|| format!("load {:?}", repo_path))?;
-            ctx.load_repo(repo)
-                .context("loading artifacts from repository at {repo_path}")?;
+            ctx.load_repo(repo).with_context(|| {
+                format!("loading artifacts from repository at {repo_path}")
+            })?;
             info!(&log, "loaded Omicron TUF repository"; "path" => %repo_path);
         }
 

flake.nix

@@ -103,7 +103,8 @@
             prefix = "${upperName}=\"";
           in
           trivial.pipe shas [
-            (lists.findFirst (strings.hasPrefix prefix) "")
+            (lists.findFirst (strings.hasPrefix prefix)
+              (throw "findSha: no '${upperName}' entry found in the provided checksums -- add it to the corresponding tools/*_checksums file"))
             (strings.removePrefix prefix)
             (strings.removeSuffix "\"")
           ]);

nexus/db-queries/src/db/datastore/rack.rs

@@ -297,9 +297,9 @@ impl DataStore {
             .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?;
         match subnet {
             Some(subnet) => Ok(subnet),
-            None => Err(Error::internal_error(
-                "DB Error(bug): returned a null subnet for {rack_id}",
-            )),
+            None => Err(Error::internal_error(&format!(
+                "DB Error(bug): returned a null subnet for {rack_id}"
+            ))),
         }
     }
 

nexus/db-queries/src/db/datastore/sled.rs

@@ -2847,10 +2847,10 @@ pub(in crate::db::datastore) mod test {
             all_groups: &AllGroups,
         ) {
             for our_group_name in &self.groups {
-                let (affinity, group_id) = all_groups
-                    .id_by_name
-                    .get(our_group_name)
-                    .expect("Group not found: {our_group_name}");
+                let (affinity, group_id) =
+                    all_groups.id_by_name.get(our_group_name).unwrap_or_else(
+                        || panic!("Group not found: {our_group_name}"),
+                    );
                 match *affinity {
                     Affinity::Positive => {
                         add_instance_to_affinity_group(

nexus/db-queries/src/db/queries/vpc_subnet.rs

@@ -398,8 +398,10 @@ mod test {
             other_ipv6_block,
         );
         let err = db_datastore.vpc_create_subnet_raw(new_row).await.expect_err(
-            "Should not be able to insert VPC Subnet with \
-                overlapping IPv4 range {overlapping_ipv4_block_longer}",
+            &format!(
+                "Should not be able to insert VPC Subnet with \
+                overlapping IPv4 range {overlapping_ipv4_block_longer}"
+            ),
         );
         assert_eq!(
             err,
@@ -416,8 +418,10 @@ mod test {
             other_ipv6_block,
         );
         let err = db_datastore.vpc_create_subnet_raw(new_row).await.expect_err(
-            "Should not be able to insert VPC Subnet with \
-                overlapping IPv4 range {overlapping_ipv4_block_shorter}",
+            &format!(
+                "Should not be able to insert VPC Subnet with \
+                overlapping IPv4 range {overlapping_ipv4_block_shorter}"
+            ),
         );
         assert_eq!(
             err,
@@ -434,8 +438,10 @@ mod test {
             overlapping_ipv6_block_longer,
         );
         let err = db_datastore.vpc_create_subnet_raw(new_row).await.expect_err(
-            "Should not be able to insert VPC Subnet with \
-                overlapping IPv6 range {overlapping_ipv6_block_longer}",
+            &format!(
+                "Should not be able to insert VPC Subnet with \
+                overlapping IPv6 range {overlapping_ipv6_block_longer}"
+            ),
         );
         assert_eq!(
             err,
@@ -452,8 +458,10 @@ mod test {
             overlapping_ipv6_block_shorter,
         );
         let err = db_datastore.vpc_create_subnet_raw(new_row).await.expect_err(
-            "Should not be able to insert VPC Subnet with \
-                overlapping IPv6 range {overlapping_ipv6_block_shorter}",
+            &format!(
+                "Should not be able to insert VPC Subnet with \
+                overlapping IPv6 range {overlapping_ipv6_block_shorter}"
+            ),
         );
         assert_eq!(
             err,

nexus/reconfigurator/planning/tests/integration_tests/planner.rs

@@ -4260,7 +4260,7 @@ fn test_update_boundary_ntp() {
             let config = &sled
                 .last_reconciliation
                 .as_ref()
-                .expect("Sled missing ledger? {sled:?}")
+                .unwrap_or_else(|| panic!("Sled missing ledger? {sled:?}"))
                 .last_reconciled_config;
 
             let Some(zone_id) =

nexus/src/app/sled.rs

@@ -371,9 +371,9 @@ impl super::Nexus {
                     || existing_disk.serial != request.serial
                     || existing_disk.model != request.model
                 {
-                    return Err(Error::internal_error(
-                        "Invalid Physical Disk update (was: {existing_disk:?}, asking for {request:?})",
-                    ));
+                    return Err(Error::internal_error(&format!(
+                        "Invalid Physical Disk update (was: {existing_disk:?}, asking for {request:?})"
+                    )));
                 }
                 return Ok(());
             }

nexus/test-utils/src/starter.rs

@@ -1008,15 +1008,22 @@ impl<'a, N: NexusServer> ControlPlaneStarter<'a, N> {
                     measurements: BTreeSet::new(),
                 })
                 .await
-                .expect("Failed to configure sled agent {sled_id} with zones");
+                .unwrap_or_else(|err| {
+                    panic!(
+                        "Failed to configure sled agent {sled_id} \
+                         with zones: {err:?}"
+                    )
+                });
 
             client
                 .write_network_bootstore_config(&early_network_config)
                 .await
-                .expect(
-                    "Failed to write early networking config \
-                     to bootstore on sled {sled_id}",
-                );
+                .unwrap_or_else(|err| {
+                    panic!(
+                        "Failed to write early networking config \
+                         to bootstore on sled {sled_id}: {err:?}"
+                    )
+                });
         }
     }
 

oximeter/oxql-types/src/point.rs

@@ -726,9 +726,9 @@ impl Points {
                     let mut new = Vec::with_capacity(doubles.len());
                     for maybe_double in doubles.iter().copied() {
                         if let Some(d) = maybe_double {
-                            let as_int = d
-                                .to_i64()
-                                .context("Cannot cast double {d} to i64")?;
+                            let as_int = d.to_i64().with_context(|| {
+                                format!("Cannot cast double {d} to i64")
+                            })?;
                             new.push(Some(as_int));
                         } else {
                             new.push(None);
@@ -753,9 +753,9 @@ impl Points {
                     let mut new = Vec::with_capacity(strings.len());
                     for maybe_str in strings.iter() {
                         if let Some(s) = maybe_str {
-                            let as_int = s
-                                .parse()
-                                .context("Cannot cast string '{s}' to i64")?;
+                            let as_int = s.parse().with_context(|| {
+                                format!("Cannot cast string '{s}' to i64")
+                            })?;
                             new.push(Some(as_int));
                         } else {
                             new.push(None);
@@ -769,9 +769,12 @@ impl Points {
                     let mut new = Vec::with_capacity(ints.len());
                     for maybe_int in ints.iter().copied() {
                         if let Some(int) = maybe_int {
-                            let as_double = int.to_f64().context(
-                                "Cannot cast integer {int} as double",
-                            )?;
+                            let as_double =
+                                int.to_f64().with_context(|| {
+                                    format!(
+                                        "Cannot cast integer {int} as double"
+                                    )
+                                })?;
                             new.push(Some(as_double));
                         } else {
                             new.push(None);
@@ -801,9 +804,9 @@ impl Points {
                     let mut new = Vec::with_capacity(strings.len());
                     for maybe_str in strings.iter() {
                         if let Some(s) = maybe_str {
-                            let as_double = s
-                                .parse()
-                                .context("Cannot cast string '{s}' to f64")?;
+                            let as_double = s.parse().with_context(|| {
+                                format!("Cannot cast string '{s}' to f64")
+                            })?;
                             new.push(Some(as_double));
                         } else {
                             new.push(None);

sled-agent/config-reconciler/src/debug_collector/file_archiver/filesystem.rs

@@ -67,7 +67,7 @@ impl FileLister for FilesystemLister {
                     return vec![];
                 } else {
                     return vec![Err(
-                        anyhow!(error).context("readdir {path:?}")
+                        anyhow!(error).context(format!("readdir {path:?}"))
                     )];
                 }
             }