fix(deletion): properly handle concurrent deletion requests

src/file_service.rs

@@ -432,68 +432,63 @@ impl FileService {
 
     pub async fn delete_document_files(&self, document: &Document) -> Result<()> {
         let mut deleted_files = Vec::new();
-        let mut errors = Vec::new();
+        let mut serious_errors = Vec::new();
+
+        // Helper function to safely delete a file, handling concurrent deletion scenarios
+        async fn safe_delete(path: &Path, serious_errors: &mut Vec<String>) -> Option<String> {
+            match fs::remove_file(path).await {
+                Ok(_) => {
+                    info!("Deleted file: {}", path.display());
+                    Some(path.to_string_lossy().to_string())
+                }
+                Err(e) => {
+                    match e.kind() {
+                        std::io::ErrorKind::NotFound => {
+                            // File already deleted (possibly by concurrent request) - this is fine
+                            info!("File already deleted: {}", path.display());
+                            None
+                        }
+                        _ => {
+                            // Other errors (permissions, I/O errors, etc.) are serious
+                            warn!("Failed to delete file {}: {}", path.display(), e);
+                            serious_errors.push(format!("Failed to delete file {}: {}", path.display(), e));
+                            None
+                        }
+                    }
+                }
+            }
+        }
 
         // Delete main document file
         let main_file = Path::new(&document.file_path);
-        if main_file.exists() {
+        if let Some(deleted_path) = safe_delete(&main_file, &mut serious_errors).await {
-            match fs::remove_file(&main_file).await {
+            deleted_files.push(deleted_path);
-                Ok(_) => {
-                    deleted_files.push(main_file.to_string_lossy().to_string());
-                    info!("Deleted document file: {}", document.file_path);
-                }
-                Err(e) => {
-                    errors.push(format!("Failed to delete document file {}: {}", document.file_path, e));
-                    warn!("Failed to delete document file {}: {}", document.file_path, e);
-                }
-            }
         }
 
         // Delete thumbnail if it exists
         let thumbnail_filename = format!("{}_thumb.jpg", document.id);
         let thumbnail_path = self.get_thumbnails_path().join(&thumbnail_filename);
-        if thumbnail_path.exists() {
+        if let Some(deleted_path) = safe_delete(&thumbnail_path, &mut serious_errors).await {
-            match fs::remove_file(&thumbnail_path).await {
+            deleted_files.push(deleted_path);
-                Ok(_) => {
-                    deleted_files.push(thumbnail_path.to_string_lossy().to_string());
-                    info!("Deleted thumbnail: {}", thumbnail_path.display());
-                }
-                Err(e) => {
-                    errors.push(format!("Failed to delete thumbnail {}: {}", thumbnail_path.display(), e));
-                    warn!("Failed to delete thumbnail {}: {}", thumbnail_path.display(), e);
-                }
-            }
         }
 
         // Delete processed image if it exists
         let processed_image_filename = format!("{}_processed.png", document.id);
         let processed_image_path = self.get_processed_images_path().join(&processed_image_filename);
-        if processed_image_path.exists() {
+        if let Some(deleted_path) = safe_delete(&processed_image_path, &mut serious_errors).await {
-            match fs::remove_file(&processed_image_path).await {
+            deleted_files.push(deleted_path);
-                Ok(_) => {
-                    deleted_files.push(processed_image_path.to_string_lossy().to_string());
-                    info!("Deleted processed image: {}", processed_image_path.display());
-                }
-                Err(e) => {
-                    errors.push(format!("Failed to delete processed image {}: {}", processed_image_path.display(), e));
-                    warn!("Failed to delete processed image {}: {}", processed_image_path.display(), e);
-                }
-            }
         }
 
-        if !errors.is_empty() {
+        // Only fail if there were serious errors (not "file not found")
-            // Log all deletion results
+        if !serious_errors.is_empty() {
-            if !deleted_files.is_empty() {
+            error!("Serious errors occurred while deleting files for document {}: {}", document.id, serious_errors.join("; "));
-                info!("Successfully deleted {} files for document {}", deleted_files.len(), document.id);
+            return Err(anyhow::anyhow!("File deletion errors: {}", serious_errors.join("; ")));
-            }
-            error!("Failed to delete some files for document {}: {}", document.id, errors.join("; "));
-            return Err(anyhow::anyhow!("Partial file deletion failure: {}", errors.join("; ")));
         }
 
         if deleted_files.is_empty() {
-            warn!("No files found to delete for document {}", document.id);
+            info!("No files needed deletion for document {} (all files already removed)", document.id);
         } else {
-            info!("Successfully deleted all {} files for document {}", deleted_files.len(), document.id);
+            info!("Successfully deleted {} files for document {}", deleted_files.len(), document.id);
         }
 
         Ok(())

src/tests/file_service_tests.rs

@@ -514,20 +514,14 @@ mod file_deletion_tests {
             service_clone.delete_document_files(&document_clone).await
         });
         
-        // Both calls should complete, but only one will successfully delete files
+        // Both calls should complete successfully now that FileService handles concurrent deletions
-        // The other will fail because files are already deleted
         let result1 = task1.await.expect("Task 1 should complete");
         let result2 = task2.await.expect("Task 2 should complete");
         
-        // In concurrent scenarios, both tasks may partially fail because they
+        // Both deletion attempts should succeed - the improved FileService handles 
-        // delete different files and then can't find the files the other task deleted.
+        // "file not found" errors gracefully as they indicate successful deletion by another task
-        // What matters is that all files get deleted by the end.
+        assert!(result1.is_ok(), "First deletion task should succeed: {:?}", result1);
-        if !result1.is_ok() && !result2.is_ok() {
+        assert!(result2.is_ok(), "Second deletion task should succeed: {:?}", result2);
-            println!("Both deletion attempts failed (expected in concurrent scenario):");
-            println!("Result 1: {:?}", result1);
-            println!("Result 2: {:?}", result2);
-            // This is okay as long as all files are actually deleted
-        }
         
         // Verify files are deleted
         assert!(!Path::new(&main_path).exists());