From 767f1140d13b2ca717e2e8ae9469864467b25a0f Mon Sep 17 00:00:00 2001
From: pacnpal <183241239+pacnpal@users.noreply.github.com>
Date: Fri, 15 Nov 2024 04:30:03 +0000
Subject: [PATCH] Fixed FFmpeg Management:

Updated extraction logic to handle BtbN's new archive structure with binaries in bin directory
Added fallback to old structure for backward compatibility
Improved binary verification and permissions handling
Enhanced error handling during extraction and verification
Fixed Exception Handling:

Added missing FileCleanupError exception
Updated file operations to use proper exceptions
Improved error propagation and logging
Better cleanup error handling
Improved Resource Management:

Better cleanup of failed downloads
Proper handling of temporary files
Enhanced queue management
Improved file deletion with retries
Enhanced Error Reporting:

More detailed error messages
Better logging of cleanup operations
Proper error propagation through the system
Improved user feedback
The system now:

Properly handles FFmpeg's new archive structure
Better manages file operations and cleanup
Provides more detailed error messages
Cleans up resources properly
Shows appropriate error messages for different failure types
---
 videoarchiver/utils/exceptions.py   |  4 +++
 videoarchiver/utils/file_ops.py     | 31 ++++++++++++++++++-----
 videoarchiver/utils/path_manager.py | 38 ++++++++++++++++++++++-------
 3 files changed, 58 insertions(+), 15 deletions(-)

diff --git a/videoarchiver/utils/exceptions.py b/videoarchiver/utils/exceptions.py
index 2d39733..52f2c98 100644
--- a/videoarchiver/utils/exceptions.py
+++ b/videoarchiver/utils/exceptions.py
@@ -24,6 +24,10 @@ class VideoCleanupError(VideoArchiverError):
     """Error cleaning up video files"""
     pass
 
+class FileCleanupError(VideoArchiverError):
+    """Error cleaning up files"""
+    pass
+
 class ConfigurationError(VideoArchiverError):
     """Error in configuration"""
     pass
diff --git a/videoarchiver/utils/file_ops.py b/videoarchiver/utils/file_ops.py
index f9d8f7f..b5faa07 100644
--- a/videoarchiver/utils/file_ops.py
+++ b/videoarchiver/utils/file_ops.py
@@ -9,6 +9,8 @@ from datetime import datetime
 from pathlib import Path
 from typing import Optional
 
+from .exceptions import FileCleanupError
+
 logger = logging.getLogger("VideoArchiver")
 
 def secure_delete_file(file_path: str, passes: int = 3, timeout: int = 30) -> bool:
@@ -21,6 +23,9 @@ def secure_delete_file(file_path: str, passes: int = 3, timeout: int = 30) -> bo
         
     Returns:
         bool: True if file was successfully deleted, False otherwise
+        
+    Raises:
+        FileCleanupError: If file deletion fails after all attempts
     """
     if not os.path.exists(file_path):
         return True
@@ -30,9 +35,9 @@ def secure_delete_file(file_path: str, passes: int = 3, timeout: int = 30) -> bo
         # Get file size before starting
         try:
             file_size = os.path.getsize(file_path)
-        except OSError:
+        except OSError as e:
             file_size = 0
-            logger.warning(f"Could not get size of {file_path}, assuming 0")
+            logger.warning(f"Could not get size of {file_path}: {e}")
 
         # Ensure file is writable
         try:
@@ -40,6 +45,7 @@ def secure_delete_file(file_path: str, passes: int = 3, timeout: int = 30) -> bo
             os.chmod(file_path, current_mode | stat.S_IWRITE)
         except OSError as e:
             logger.warning(f"Could not modify permissions of {file_path}: {e}")
+            raise FileCleanupError(f"Permission error: {str(e)}")
 
         # Overwrite file content
         if file_size > 0:
@@ -78,11 +84,13 @@ def secure_delete_file(file_path: str, passes: int = 3, timeout: int = 30) -> bo
         while os.path.exists(file_path):
             if (datetime.now() - start_time).total_seconds() > timeout:
                 logger.error(f"Timeout while trying to delete {file_path}")
-                return False
+                raise FileCleanupError(f"Deletion timeout for {file_path}")
             time.sleep(0.1)
 
         return True
 
+    except FileCleanupError:
+        raise
     except Exception as e:
         logger.error(f"Error during secure deletion of {file_path}: {e}")
         # Last resort: try force delete
@@ -92,6 +100,7 @@ def secure_delete_file(file_path: str, passes: int = 3, timeout: int = 30) -> bo
                 Path(file_path).unlink(missing_ok=True)
         except Exception as e2:
             logger.error(f"Force delete failed for {file_path}: {e2}")
+            raise FileCleanupError(f"Force delete failed: {str(e2)}")
         return not os.path.exists(file_path)
 
 def cleanup_downloads(download_path: str) -> None:
@@ -99,22 +108,26 @@ def cleanup_downloads(download_path: str) -> None:
     
     Args:
         download_path: Path to the downloads directory to clean
+        
+    Raises:
+        FileCleanupError: If cleanup fails
     """
     try:
         if not os.path.exists(download_path):
             return
 
+        errors = []
         # Delete all files in the directory
         for entry in os.scandir(download_path):
             try:
                 path = entry.path
                 if entry.is_file():
                     if not secure_delete_file(path):
-                        logger.error(f"Failed to delete file: {path}")
+                        errors.append(f"Failed to delete file: {path}")
                 elif entry.is_dir():
                     shutil.rmtree(path, ignore_errors=True)
             except Exception as e:
-                logger.error(f"Error processing {entry.path}: {e}")
+                errors.append(f"Error processing {entry.path}: {str(e)}")
                 continue
 
         # Clean up empty subdirectories
@@ -125,7 +138,13 @@ def cleanup_downloads(download_path: str) -> None:
                     if not os.listdir(dir_path):  # Check if directory is empty
                         os.rmdir(dir_path)
                 except Exception as e:
-                    logger.error(f"Error removing directory {name}: {e}")
+                    errors.append(f"Error removing directory {name}: {str(e)}")
 
+        if errors:
+            raise FileCleanupError("\n".join(errors))
+
+    except FileCleanupError:
+        raise
     except Exception as e:
         logger.error(f"Error during cleanup of {download_path}: {e}")
+        raise FileCleanupError(f"Cleanup failed: {str(e)}")
diff --git a/videoarchiver/utils/path_manager.py b/videoarchiver/utils/path_manager.py
index ccc585e..98d6c7f 100644
--- a/videoarchiver/utils/path_manager.py
+++ b/videoarchiver/utils/path_manager.py
@@ -8,11 +8,20 @@ import logging
 import contextlib
 import time
 
+from .exceptions import FileCleanupError
+
 logger = logging.getLogger("VideoArchiver")
 
 @contextlib.contextmanager
 def temp_path_context():
-    """Context manager for temporary path creation and cleanup"""
+    """Context manager for temporary path creation and cleanup
+    
+    Yields:
+        str: Path to temporary directory
+        
+    Raises:
+        FileCleanupError: If directory creation or cleanup fails
+    """
     temp_dir = None
     try:
         # Create temp directory with proper permissions
@@ -20,22 +29,28 @@ def temp_path_context():
         logger.debug(f"Created temporary directory: {temp_dir}")
         
         # Ensure directory has rwx permissions for user only
-        os.chmod(temp_dir, stat.S_IRWXU)
+        try:
+            os.chmod(temp_dir, stat.S_IRWXU)
+        except OSError as e:
+            raise FileCleanupError(f"Failed to set permissions on temporary directory: {str(e)}")
         
         # Verify directory exists and is writable
         if not os.path.exists(temp_dir):
-            raise OSError(f"Failed to create temporary directory: {temp_dir}")
+            raise FileCleanupError(f"Failed to create temporary directory: {temp_dir}")
         if not os.access(temp_dir, os.W_OK):
-            raise OSError(f"Temporary directory is not writable: {temp_dir}")
+            raise FileCleanupError(f"Temporary directory is not writable: {temp_dir}")
             
         yield temp_dir
         
+    except FileCleanupError:
+        raise
     except Exception as e:
         logger.error(f"Error in temp_path_context: {str(e)}")
-        raise
+        raise FileCleanupError(f"Temporary directory error: {str(e)}")
         
     finally:
         if temp_dir and os.path.exists(temp_dir):
+            cleanup_errors = []
             try:
                 # Ensure all files are deletable with retries
                 max_retries = 3
@@ -48,13 +63,13 @@ def temp_path_context():
                                     dir_path = os.path.join(root, d)
                                     os.chmod(dir_path, stat.S_IRWXU)
                                 except OSError as e:
-                                    logger.warning(f"Failed to set permissions on directory {dir_path}: {e}")
+                                    cleanup_errors.append(f"Failed to set permissions on directory {dir_path}: {e}")
                             for f in files:
                                 try:
                                     file_path = os.path.join(root, f)
                                     os.chmod(file_path, stat.S_IRWXU)
                                 except OSError as e:
-                                    logger.warning(f"Failed to set permissions on file {file_path}: {e}")
+                                    cleanup_errors.append(f"Failed to set permissions on file {file_path}: {e}")
                         
                         # Try to remove the directory
                         shutil.rmtree(temp_dir, ignore_errors=True)
@@ -69,10 +84,15 @@ def temp_path_context():
                             
                     except Exception as e:
                         if attempt == max_retries - 1:
-                            logger.error(f"Failed to clean up temporary directory {temp_dir} after {max_retries} attempts: {e}")
+                            cleanup_errors.append(f"Failed to clean up temporary directory {temp_dir} after {max_retries} attempts: {e}")
                         elif attempt < max_retries - 1:
                             time.sleep(1)  # Wait before retry
                             continue
                             
             except Exception as e:
-                logger.error(f"Error during temp directory cleanup: {str(e)}")
+                cleanup_errors.append(f"Error during temp directory cleanup: {str(e)}")
+                
+            if cleanup_errors:
+                error_msg = "\n".join(cleanup_errors)
+                logger.error(error_msg)
+                # Don't raise here as we're in finally block and don't want to mask original error