Fix quarantine non-atomic operations with two-phase commit
Implements atomic quarantine/restore operations using two-phase commit
pattern to prevent orphaned files when manifest updates fail.
Changes to models.py:
- Add status field to QuarantineEntry ("pending" | "committed")
- Default to "committed" for backward compatibility
Changes to quarantine.py:
- Rewrite quarantine_file() with three phases:
1. Write pending manifest entry BEFORE moving file
2. Move file to quarantine
3. Mark manifest entry as committed
- Rewrite restore_from_quarantine() with same pattern
- Add _recover_pending_entries() for auto-recovery on manifest load
- Update _load_manifest() and _save_manifest() to handle status field
Changes to executor.py:
- Add special handling for quarantine rollback using QuarantineManager
- Fix bug where executor didn't preserve quarantine destination_path
- Return QuarantineManager result directly (includes actual quarantine path)
Testing:
- Fixed pre-existing test_rollback_quarantine_operation
- All 439 tests now pass (was 438 with 1 failure)
Atomicity guarantees:
- If manifest write fails → operation fails, no file moved
- If file move fails → rollback removes pending manifest entry
- If commit fails → auto-recovery fixes on next load
- No orphaned files possible
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit is contained in:
windyboy
co-authored by
Claude Sonnet 4.5
Claude Sonnet 4.5
parent
065195b83b
commit
d6c8852e1e
File diff suppressed because it is too large
Load Diff
+40
-7
@@ -169,15 +169,11 @@ class ExecutionEngine:
|
||||
error_message=None,
|
||||
executed_at=executed_at
|
||||
)
|
||||
result = self._quarantine_manager.quarantine_file(
|
||||
# Quarantine the file and return the result directly
|
||||
# (includes the actual quarantine destination_path for rollback)
|
||||
return self._quarantine_manager.quarantine_file(
|
||||
operation.source_path, operation.reason
|
||||
)
|
||||
return OperationResult(
|
||||
operation=operation,
|
||||
success=result.success,
|
||||
error_message=result.error_message,
|
||||
executed_at=result.executed_at
|
||||
)
|
||||
|
||||
# Handle conflicted operations
|
||||
if operation.has_conflict:
|
||||
@@ -536,6 +532,43 @@ class ExecutionEngine:
|
||||
executed_at=executed_at
|
||||
)
|
||||
|
||||
# Handle quarantine operations using QuarantineManager
|
||||
if operation.operation_type == "quarantine":
|
||||
if not self._quarantine_manager:
|
||||
error_msg = "Cannot rollback quarantine: QuarantineManager not available"
|
||||
log_operation(
|
||||
self.logger,
|
||||
logging.ERROR,
|
||||
error_msg,
|
||||
operation_type="rollback"
|
||||
)
|
||||
return OperationResult(
|
||||
operation=operation,
|
||||
success=False,
|
||||
error_message=error_msg,
|
||||
executed_at=executed_at
|
||||
)
|
||||
|
||||
# Use QuarantineManager to restore the file
|
||||
if operation.destination_path:
|
||||
restore_result = self._quarantine_manager.restore_from_quarantine(
|
||||
operation.destination_path
|
||||
)
|
||||
return OperationResult(
|
||||
operation=operation,
|
||||
success=restore_result.success,
|
||||
error_message=restore_result.error_message,
|
||||
executed_at=executed_at
|
||||
)
|
||||
else:
|
||||
error_msg = "Cannot rollback quarantine: no destination path recorded"
|
||||
return OperationResult(
|
||||
operation=operation,
|
||||
success=False,
|
||||
error_message=error_msg,
|
||||
executed_at=executed_at
|
||||
)
|
||||
|
||||
try:
|
||||
# For move/rename operations, reverse the direction
|
||||
# Original: source -> destination
|
||||
|
||||
+3
-1
@@ -183,7 +183,7 @@ class RollbackLog:
|
||||
@dataclass
|
||||
class QuarantineEntry:
|
||||
"""Represents a single file in quarantine.
|
||||
|
||||
|
||||
Attributes:
|
||||
original_path: Original path of the file before quarantine
|
||||
quarantine_path: Path to the file in quarantine directory
|
||||
@@ -191,6 +191,7 @@ class QuarantineEntry:
|
||||
reason: Optional reason for quarantining the file
|
||||
size_bytes: File size in bytes
|
||||
category: Category of the video ("movie" or "series")
|
||||
status: Operation status ("pending" | "committed") for two-phase commit
|
||||
"""
|
||||
original_path: Path
|
||||
quarantine_path: Path
|
||||
@@ -198,6 +199,7 @@ class QuarantineEntry:
|
||||
reason: Optional[str]
|
||||
size_bytes: int
|
||||
category: str
|
||||
status: str = "committed" # Default for backward compatibility
|
||||
|
||||
|
||||
@dataclass
|
||||
|
||||
+277
-93
@@ -187,54 +187,31 @@ class QuarantineManager:
|
||||
executed_at=executed_at
|
||||
)
|
||||
|
||||
# Move file to quarantine
|
||||
# Two-phase commit for atomic quarantine operation
|
||||
|
||||
# PHASE 1: Write pending manifest entry BEFORE moving file
|
||||
manifest = self._load_manifest(category)
|
||||
pending_entry = QuarantineEntry(
|
||||
original_path=file_path,
|
||||
quarantine_path=quarantine_path,
|
||||
quarantined_at=executed_at,
|
||||
reason=reason,
|
||||
size_bytes=file_size,
|
||||
category=category,
|
||||
status="pending"
|
||||
)
|
||||
manifest.entries.append(pending_entry)
|
||||
|
||||
try:
|
||||
file_path.rename(quarantine_path)
|
||||
|
||||
self._save_manifest(category, manifest)
|
||||
log_operation(
|
||||
self.logger,
|
||||
logging.INFO,
|
||||
f"Successfully quarantined file: {file_path} -> {quarantine_path}",
|
||||
operation_type="quarantine",
|
||||
file_path=file_path
|
||||
logging.DEBUG,
|
||||
f"Phase 1: Wrote pending manifest entry for {file_path}",
|
||||
operation_type="quarantine"
|
||||
)
|
||||
|
||||
# Update manifest
|
||||
try:
|
||||
self._update_manifest(
|
||||
category=category,
|
||||
original_path=file_path,
|
||||
quarantine_path=quarantine_path,
|
||||
quarantined_at=executed_at,
|
||||
reason=reason,
|
||||
size_bytes=file_size
|
||||
)
|
||||
except Exception as e:
|
||||
# Log manifest update failure but don't fail the operation
|
||||
# since the file was already moved successfully
|
||||
log_operation(
|
||||
self.logger,
|
||||
logging.WARNING,
|
||||
f"Failed to update manifest: {str(e)}",
|
||||
operation_type="quarantine",
|
||||
file_path=file_path
|
||||
)
|
||||
|
||||
return OperationResult(
|
||||
operation=FileOperation(
|
||||
operation_type="quarantine",
|
||||
source_path=file_path,
|
||||
destination_path=quarantine_path,
|
||||
reason=reason or "Quarantine",
|
||||
has_conflict=False
|
||||
),
|
||||
success=True,
|
||||
error_message=None,
|
||||
executed_at=executed_at
|
||||
)
|
||||
|
||||
except Exception as e:
|
||||
error_msg = f"Failed to move file to quarantine: {str(e)}"
|
||||
error_msg = f"Failed to write pending manifest entry: {str(e)}"
|
||||
log_operation(
|
||||
self.logger,
|
||||
logging.ERROR,
|
||||
@@ -254,6 +231,91 @@ class QuarantineManager:
|
||||
error_message=error_msg,
|
||||
executed_at=executed_at
|
||||
)
|
||||
|
||||
# PHASE 2: Move file
|
||||
try:
|
||||
file_path.rename(quarantine_path)
|
||||
log_operation(
|
||||
self.logger,
|
||||
logging.INFO,
|
||||
f"Phase 2: Successfully moved file: {file_path} -> {quarantine_path}",
|
||||
operation_type="quarantine",
|
||||
file_path=file_path
|
||||
)
|
||||
except Exception as e:
|
||||
# Rollback: Remove pending entry from manifest
|
||||
error_msg = f"Failed to move file to quarantine: {str(e)}"
|
||||
log_operation(
|
||||
self.logger,
|
||||
logging.ERROR,
|
||||
error_msg,
|
||||
operation_type="quarantine",
|
||||
file_path=file_path
|
||||
)
|
||||
|
||||
try:
|
||||
manifest.entries.remove(pending_entry)
|
||||
self._save_manifest(category, manifest)
|
||||
log_operation(
|
||||
self.logger,
|
||||
logging.INFO,
|
||||
f"Rollback: Removed pending manifest entry for {file_path}",
|
||||
operation_type="quarantine"
|
||||
)
|
||||
except Exception as rollback_error:
|
||||
log_operation(
|
||||
self.logger,
|
||||
logging.ERROR,
|
||||
f"Rollback failed: {str(rollback_error)}",
|
||||
operation_type="quarantine"
|
||||
)
|
||||
|
||||
return OperationResult(
|
||||
operation=FileOperation(
|
||||
operation_type="quarantine",
|
||||
source_path=file_path,
|
||||
destination_path=quarantine_path,
|
||||
reason=reason or "Quarantine",
|
||||
has_conflict=False
|
||||
),
|
||||
success=False,
|
||||
error_message=error_msg,
|
||||
executed_at=executed_at
|
||||
)
|
||||
|
||||
# PHASE 3: Mark as committed
|
||||
try:
|
||||
pending_entry.status = "committed"
|
||||
self._save_manifest(category, manifest)
|
||||
log_operation(
|
||||
self.logger,
|
||||
logging.DEBUG,
|
||||
f"Phase 3: Marked manifest entry as committed for {file_path}",
|
||||
operation_type="quarantine"
|
||||
)
|
||||
except Exception as e:
|
||||
# File was moved but manifest update failed
|
||||
# Auto-recovery will handle this on next load
|
||||
log_operation(
|
||||
self.logger,
|
||||
logging.WARNING,
|
||||
f"Failed to mark entry as committed (auto-recovery will fix): {str(e)}",
|
||||
operation_type="quarantine",
|
||||
file_path=file_path
|
||||
)
|
||||
|
||||
return OperationResult(
|
||||
operation=FileOperation(
|
||||
operation_type="quarantine",
|
||||
source_path=file_path,
|
||||
destination_path=quarantine_path,
|
||||
reason=reason or "Quarantine",
|
||||
has_conflict=False
|
||||
),
|
||||
success=True,
|
||||
error_message=None,
|
||||
executed_at=executed_at
|
||||
)
|
||||
|
||||
def _determine_category(self, file_path: Path) -> str:
|
||||
"""Determine the category of a file based on its path.
|
||||
@@ -363,11 +425,16 @@ class QuarantineManager:
|
||||
quarantined_at=datetime.fromisoformat(entry_data['quarantined_at']),
|
||||
reason=entry_data.get('reason'),
|
||||
size_bytes=entry_data['size_bytes'],
|
||||
category=entry_data['category']
|
||||
category=entry_data['category'],
|
||||
status=entry_data.get('status', 'committed') # Default for backward compatibility
|
||||
)
|
||||
entries.append(entry)
|
||||
|
||||
return QuarantineManifest(entries=entries)
|
||||
|
||||
# Auto-recovery: Clean up pending entries
|
||||
manifest = QuarantineManifest(entries=entries)
|
||||
self._recover_pending_entries(category, manifest)
|
||||
|
||||
return manifest
|
||||
|
||||
except Exception as e:
|
||||
log_operation(
|
||||
@@ -402,7 +469,8 @@ class QuarantineManager:
|
||||
'quarantined_at': entry.quarantined_at.isoformat(),
|
||||
'reason': entry.reason,
|
||||
'size_bytes': entry.size_bytes,
|
||||
'category': entry.category
|
||||
'category': entry.category,
|
||||
'status': entry.status
|
||||
}
|
||||
for entry in manifest.entries
|
||||
]
|
||||
@@ -418,7 +486,73 @@ class QuarantineManager:
|
||||
f"Saved manifest for category '{category}' with {len(manifest.entries)} entries",
|
||||
operation_type="quarantine"
|
||||
)
|
||||
|
||||
|
||||
def _recover_pending_entries(self, category: str, manifest: QuarantineManifest) -> None:
|
||||
"""Auto-recovery: Clean up pending entries from incomplete operations.
|
||||
|
||||
Checks each pending entry:
|
||||
- If file exists in quarantine → mark as committed
|
||||
- If file doesn't exist → remove orphaned entry
|
||||
|
||||
Args:
|
||||
category: Category name
|
||||
manifest: Manifest to recover (modified in place)
|
||||
"""
|
||||
pending_entries = [e for e in manifest.entries if e.status == "pending"]
|
||||
|
||||
if not pending_entries:
|
||||
return
|
||||
|
||||
log_operation(
|
||||
self.logger,
|
||||
logging.INFO,
|
||||
f"Auto-recovery: Found {len(pending_entries)} pending entries for category '{category}'",
|
||||
operation_type="quarantine"
|
||||
)
|
||||
|
||||
recovered = 0
|
||||
removed = 0
|
||||
|
||||
for entry in pending_entries:
|
||||
if entry.quarantine_path.exists():
|
||||
# File exists → mark as committed
|
||||
entry.status = "committed"
|
||||
recovered += 1
|
||||
log_operation(
|
||||
self.logger,
|
||||
logging.INFO,
|
||||
f"Auto-recovery: Marked as committed: {entry.quarantine_path}",
|
||||
operation_type="quarantine"
|
||||
)
|
||||
else:
|
||||
# File doesn't exist → remove orphaned entry
|
||||
manifest.entries.remove(entry)
|
||||
removed += 1
|
||||
log_operation(
|
||||
self.logger,
|
||||
logging.WARNING,
|
||||
f"Auto-recovery: Removed orphaned entry: {entry.original_path}",
|
||||
operation_type="quarantine"
|
||||
)
|
||||
|
||||
if recovered > 0 or removed > 0:
|
||||
# Save recovered manifest
|
||||
try:
|
||||
self._save_manifest(category, manifest)
|
||||
log_operation(
|
||||
self.logger,
|
||||
logging.INFO,
|
||||
f"Auto-recovery: Completed for '{category}' - {recovered} committed, {removed} removed",
|
||||
operation_type="quarantine"
|
||||
)
|
||||
except Exception as e:
|
||||
log_operation(
|
||||
self.logger,
|
||||
logging.ERROR,
|
||||
f"Auto-recovery: Failed to save manifest: {str(e)}",
|
||||
operation_type="quarantine"
|
||||
)
|
||||
|
||||
def _update_manifest(
|
||||
self,
|
||||
category: str,
|
||||
@@ -679,55 +813,20 @@ class QuarantineManager:
|
||||
executed_at=executed_at
|
||||
)
|
||||
|
||||
# Move file back to original location
|
||||
# Two-phase commit for atomic restore operation
|
||||
|
||||
# PHASE 1: Mark entry as pending restoration
|
||||
entry.status = "pending"
|
||||
try:
|
||||
quarantine_path.rename(original_path)
|
||||
|
||||
self._save_manifest(category, manifest)
|
||||
log_operation(
|
||||
self.logger,
|
||||
logging.INFO,
|
||||
f"Successfully restored file: {quarantine_path} -> {original_path}",
|
||||
operation_type="restore",
|
||||
file_path=quarantine_path
|
||||
logging.DEBUG,
|
||||
f"Phase 1: Marked entry as pending restoration for {quarantine_path}",
|
||||
operation_type="restore"
|
||||
)
|
||||
|
||||
# Remove entry from manifest
|
||||
try:
|
||||
manifest.entries.remove(entry)
|
||||
self._save_manifest(category, manifest)
|
||||
|
||||
log_operation(
|
||||
self.logger,
|
||||
logging.INFO,
|
||||
f"Removed entry from manifest for category '{category}'",
|
||||
operation_type="restore"
|
||||
)
|
||||
except Exception as e:
|
||||
# Log manifest update failure but don't fail the operation
|
||||
# since the file was already moved successfully
|
||||
log_operation(
|
||||
self.logger,
|
||||
logging.WARNING,
|
||||
f"Failed to update manifest after restore: {str(e)}",
|
||||
operation_type="restore",
|
||||
file_path=quarantine_path
|
||||
)
|
||||
|
||||
return OperationResult(
|
||||
operation=FileOperation(
|
||||
operation_type="restore",
|
||||
source_path=quarantine_path,
|
||||
destination_path=original_path,
|
||||
reason="Restore",
|
||||
has_conflict=False
|
||||
),
|
||||
success=True,
|
||||
error_message=None,
|
||||
executed_at=executed_at
|
||||
)
|
||||
|
||||
except Exception as e:
|
||||
error_msg = f"Failed to restore file: {str(e)}"
|
||||
error_msg = f"Failed to mark entry as pending: {str(e)}"
|
||||
log_operation(
|
||||
self.logger,
|
||||
logging.ERROR,
|
||||
@@ -747,7 +846,92 @@ class QuarantineManager:
|
||||
error_message=error_msg,
|
||||
executed_at=executed_at
|
||||
)
|
||||
|
||||
|
||||
# PHASE 2: Move file back to original location
|
||||
try:
|
||||
quarantine_path.rename(original_path)
|
||||
log_operation(
|
||||
self.logger,
|
||||
logging.INFO,
|
||||
f"Phase 2: Successfully restored file: {quarantine_path} -> {original_path}",
|
||||
operation_type="restore",
|
||||
file_path=quarantine_path
|
||||
)
|
||||
except Exception as e:
|
||||
# Rollback: Mark entry back as committed
|
||||
error_msg = f"Failed to restore file: {str(e)}"
|
||||
log_operation(
|
||||
self.logger,
|
||||
logging.ERROR,
|
||||
error_msg,
|
||||
operation_type="restore",
|
||||
file_path=quarantine_path
|
||||
)
|
||||
|
||||
try:
|
||||
entry.status = "committed"
|
||||
self._save_manifest(category, manifest)
|
||||
log_operation(
|
||||
self.logger,
|
||||
logging.INFO,
|
||||
f"Rollback: Marked entry back as committed for {quarantine_path}",
|
||||
operation_type="restore"
|
||||
)
|
||||
except Exception as rollback_error:
|
||||
log_operation(
|
||||
self.logger,
|
||||
logging.ERROR,
|
||||
f"Rollback failed: {str(rollback_error)}",
|
||||
operation_type="restore"
|
||||
)
|
||||
|
||||
return OperationResult(
|
||||
operation=FileOperation(
|
||||
operation_type="restore",
|
||||
source_path=quarantine_path,
|
||||
destination_path=original_path,
|
||||
reason="Restore",
|
||||
has_conflict=False
|
||||
),
|
||||
success=False,
|
||||
error_message=error_msg,
|
||||
executed_at=executed_at
|
||||
)
|
||||
|
||||
# PHASE 3: Remove entry from manifest
|
||||
try:
|
||||
manifest.entries.remove(entry)
|
||||
self._save_manifest(category, manifest)
|
||||
log_operation(
|
||||
self.logger,
|
||||
logging.DEBUG,
|
||||
f"Phase 3: Removed entry from manifest for category '{category}'",
|
||||
operation_type="restore"
|
||||
)
|
||||
except Exception as e:
|
||||
# File was restored but manifest update failed
|
||||
# This is not critical since the file is in the right place
|
||||
log_operation(
|
||||
self.logger,
|
||||
logging.WARNING,
|
||||
f"Failed to remove entry from manifest (file was restored): {str(e)}",
|
||||
operation_type="restore",
|
||||
file_path=quarantine_path
|
||||
)
|
||||
|
||||
return OperationResult(
|
||||
operation=FileOperation(
|
||||
operation_type="restore",
|
||||
source_path=quarantine_path,
|
||||
destination_path=original_path,
|
||||
reason="Restore",
|
||||
has_conflict=False
|
||||
),
|
||||
success=True,
|
||||
error_message=None,
|
||||
executed_at=executed_at
|
||||
)
|
||||
|
||||
def _determine_category_from_quarantine(self, quarantine_path: Path) -> Optional[str]:
|
||||
"""Determine the category from a quarantine path.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user