diff --git a/src/file_service.rs b/src/file_service.rs index 641edda..cb6d80e 100644 --- a/src/file_service.rs +++ b/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) -> Option { + 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() { - match fs::remove_file(&main_file).await { - 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); - } - } + if let Some(deleted_path) = safe_delete(&main_file, &mut serious_errors).await { + deleted_files.push(deleted_path); } // 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() { - match fs::remove_file(&thumbnail_path).await { - 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); - } - } + if let Some(deleted_path) = safe_delete(&thumbnail_path, &mut serious_errors).await { + deleted_files.push(deleted_path); } // 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() { - match fs::remove_file(&processed_image_path).await { - 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 let Some(deleted_path) = safe_delete(&processed_image_path, &mut serious_errors).await { + deleted_files.push(deleted_path); } - if !errors.is_empty() { - // Log all deletion results - if !deleted_files.is_empty() { - info!("Successfully deleted {} files for document {}", deleted_files.len(), document.id); - } - error!("Failed to delete some files for document {}: {}", document.id, errors.join("; ")); - return Err(anyhow::anyhow!("Partial file deletion failure: {}", errors.join("; "))); + // Only fail if there were serious errors (not "file not found") + if !serious_errors.is_empty() { + error!("Serious errors occurred while deleting files for document {}: {}", document.id, serious_errors.join("; ")); + return Err(anyhow::anyhow!("File deletion errors: {}", serious_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(()) diff --git a/src/tests/file_service_tests.rs b/src/tests/file_service_tests.rs index 893816f..271a0b3 100644 --- a/src/tests/file_service_tests.rs +++ b/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 - // The other will fail because files are already deleted + // Both calls should complete successfully now that FileService handles concurrent deletions 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 - // delete different files and then can't find the files the other task deleted. - // What matters is that all files get deleted by the end. - if !result1.is_ok() && !result2.is_ok() { - 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 - } + // Both deletion attempts should succeed - the improved FileService handles + // "file not found" errors gracefully as they indicate successful deletion by another task + assert!(result1.is_ok(), "First deletion task should succeed: {:?}", result1); + assert!(result2.is_ok(), "Second deletion task should succeed: {:?}", result2); // Verify files are deleted assert!(!Path::new(&main_path).exists());