feat(dif): Fail debug-files upload when file is too big (#2331)

Previously, we only logged an easy-to-miss warning when we had to skip
uploading a debug file because it was too big. Now, we instead fail the
entire upload loudly, to ensure users do not miss this important
information.

Resolves #2313

src/utils/dif_upload/error.rs

@@ -1,5 +1,6 @@
 //! Error types for the dif_upload module.
 
+use anyhow::Result;
 use indicatif::HumanBytes;
 use thiserror::Error;
 
@@ -21,13 +22,45 @@ pub enum ValidationError {
 }
 
 /// Handles a DIF validation error by logging it to console
-/// at the appropriate log level.
-pub fn handle(dif_name: &str, error: &ValidationError) {
-    let message = format!("Skipping {}: {}", dif_name, error);
+/// at the appropriate log level. Or, if the error should stop
+/// the upload, it will return an error, that can be propagated
+/// to the caller.
+pub fn handle(dif_name: &str, error: &ValidationError) -> Result<()> {
+    let message = format!("{}: {}", dif_name, error);
     match error {
         ValidationError::InvalidFormat
         | ValidationError::InvalidFeatures
-        | ValidationError::InvalidDebugId => log::debug!("{message}"),
-        ValidationError::TooLarge { .. } => log::warn!("{message}"),
+        | ValidationError::InvalidDebugId => log::debug!("Skipping {message}"),
+        ValidationError::TooLarge { .. } => {
+            anyhow::bail!("Upload failed due to error in debug file {message}")
+        }
+    }
+
+    Ok(())
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    use rstest::rstest;
+
+    #[rstest]
+    #[case(ValidationError::InvalidFormat)]
+    #[case(ValidationError::InvalidFeatures)]
+    #[case(ValidationError::InvalidDebugId)]
+    fn test_handle_should_not_error(#[case] error: ValidationError) {
+        let result = handle("test", &error);
+        assert!(result.is_ok());
+    }
+
+    #[test]
+    fn test_handle_should_error() {
+        let error = ValidationError::TooLarge {
+            size: 1000,
+            max_size: 100,
+        };
+        let result = handle("test", &error);
+        assert!(result.is_err());
     }
 }

src/utils/dif_upload/mod.rs

@@ -658,14 +658,14 @@ fn search_difs(options: &DifUpload) -> Result<Vec<DifMatch<'static>>> {
 
             if Archive::peek(&buffer) != FileFormat::Unknown {
                 let mut difs =
-                    collect_object_dif(source, name, buffer, options, &mut age_overrides);
+                    collect_object_dif(source, name, buffer, options, &mut age_overrides)?;
                 collected.append(difs.as_mut());
             } else if BcSymbolMap::test(&buffer) {
-                if let Some(dif) = collect_auxdif(name, buffer, options, AuxDifKind::BcSymbolMap) {
+                if let Some(dif) = collect_auxdif(name, buffer, options, AuxDifKind::BcSymbolMap)? {
                     collected.push(dif);
                 }
             } else if buffer.starts_with(b"<?xml") {
-                if let Some(dif) = collect_auxdif(name, buffer, options, AuxDifKind::UuidMap) {
+                if let Some(dif) = collect_auxdif(name, buffer, options, AuxDifKind::UuidMap)? {
                     collected.push(dif);
                 }
             };
@@ -731,7 +731,7 @@ fn collect_auxdif<'a>(
     buffer: ByteView<'static>,
     options: &DifUpload,
     kind: AuxDifKind,
-) -> Option<DifMatch<'a>> {
+) -> Result<Option<DifMatch<'a>>> {
     let file_stem = Path::new(&name)
         .file_stem()
         .map(|stem| stem.to_string_lossy())
@@ -748,7 +748,7 @@ fn collect_auxdif<'a>(
                     name = name
                 );
             }
-            return None;
+            return Ok(None);
         }
     };
     let dif_result = match kind {
@@ -764,17 +764,17 @@ fn collect_auxdif<'a>(
                 name = name,
                 err = err
             );
-            return None;
+            return Ok(None);
         }
     };
 
     // Skip this file if we don't want to process it.
     if let Err(e) = options.validate_dif(&dif) {
-        error::handle(&name, &e);
-        return None;
+        error::handle(&name, &e)?;
+        return Ok(None);
     }
 
-    Some(dif)
+    Ok(Some(dif))
 }
 
 /// Processes and [`DifSource`] which is expected to be an object file.
@@ -784,7 +784,7 @@ fn collect_object_dif<'a>(
     buffer: ByteView<'static>,
     options: &DifUpload,
     age_overrides: &mut BTreeMap<Uuid, u32>,
-) -> Vec<DifMatch<'a>> {
+) -> Result<Vec<DifMatch<'a>>> {
     let mut collected = Vec::with_capacity(2);
 
     // Try to parse a potential object file. If this is not possible,
@@ -799,15 +799,15 @@ fn collect_object_dif<'a>(
         format == FileFormat::Pe && options.valid_format(DifFormat::Object(FileFormat::Pdb));
 
     if !should_override_age && !options.valid_format(DifFormat::Object(format)) {
-        return collected;
+        return Ok(collected);
     }
 
     debug!("trying to parse dif {}", name);
     let archive = match Archive::parse(&buffer) {
         Ok(archive) => archive,
         Err(e) => {
             warn!("Skipping invalid debug file {}: {}", name, e);
-            return collected;
+            return Ok(collected);
         }
     };
 
@@ -840,7 +840,7 @@ fn collect_object_dif<'a>(
         if let Object::Pe(pe) = &object {
             if let Ok(Some(ppdb_dif)) = extract_embedded_ppdb(pe, name.as_str()) {
                 if let Err(e) = options.validate_dif(&ppdb_dif) {
-                    error::handle(&ppdb_dif.name, &e);
+                    error::handle(&ppdb_dif.name, &e)?;
                 } else {
                     collected.push(ppdb_dif);
                 }
@@ -884,14 +884,14 @@ fn collect_object_dif<'a>(
 
         // Skip this file if we don't want to process it.
         if let Err(e) = options.validate_dif(&dif) {
-            error::handle(&name, &e);
+            error::handle(&name, &e)?;
             continue;
         }
 
         collected.push(dif);
     }
 
-    collected
+    Ok(collected)
 }
 
 /// Resolves BCSymbolMaps and replaces hidden symbols in a `DifMatch` using

tests/integration/_responses/debug_files/get-chunk-upload-small-max-size.json

@@ -0,0 +1,11 @@
+{
+  "url": "organizations/wat-org/chunk-upload/",
+  "chunkSize": 8388608,
+  "chunksPerRequest": 64,
+  "maxFileSize": 2048,
+  "maxRequestSize": 33554432,
+  "concurrency": 8,
+  "hashAlgorithm": "sha1",
+  "compression": ["gzip"],
+  "accept": ["debug_files", "release_files", "pdbs", "portablepdbs", "sources", "bcsymbolmaps"]
+}

tests/integration/debug_files/upload.rs

@@ -731,3 +731,19 @@ fn chunk_upload_multiple_files_small_chunks_only_some() {
         "Uploaded chunks differ from the expected chunks"
     );
 }
+
+#[test]
+fn test_dif_too_big() {
+    TestManager::new()
+        .mock_endpoint(
+            MockEndpointBuilder::new("GET", "/api/0/organizations/wat-org/chunk-upload/")
+                .with_response_file("debug_files/get-chunk-upload-small-max-size.json"),
+        )
+        .assert_cmd(vec![
+            "debug-files",
+            "upload",
+            "tests/integration/_fixtures/SrcGenSampleApp.pdb",
+        ])
+        .with_default_token()
+        .run_and_assert(AssertCommand::Failure);
+}

tests/integration/test_utils/test_manager.rs

@@ -185,6 +185,8 @@ pub struct AssertCmdTestManager {
 pub enum AssertCommand {
     /// Assert that the command succeeds (i.e. returns a `0` exit code).
     Success,
+    /// Assert that the command fails (i.e. returns a non-zero exit code).
+    Failure,
 }
 
 impl AssertCmdTestManager {
@@ -219,6 +221,7 @@ impl AssertCmdTestManager {
 
         match assert {
             AssertCommand::Success => command_result.success(),
+            AssertCommand::Failure => command_result.failure(),
         };
     }