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