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

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.

Also, add tests to verify the new behavior.

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,16 @@ 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(),
         };
     }