From 787f40ee78319f9cd40110ec9965e1844944c6a3 Mon Sep 17 00:00:00 2001 From: Debasmita Date: Thu, 2 Jul 2026 21:28:55 +0530 Subject: [PATCH] fix(rollback): capture and report silent filesystem restoration failures Addresses silently ignored failures during filesystem rollback. - FilesystemSnapshot.restore() now extracts files one at a time, catches per-member failures, cleans up partially extracted files to prevent inconsistent state, and logs warnings on partial restore - rollback() now emits a warning when snapshot_path is missing instead of silently marking rollback as COMPLETED - RollbackResult gains 'warnings' and 'partial_restoration' fields for downstream visibility into degraded rollback outcomes - 4 new tests covering missing snapshot, partial restoration tracking, and non-existent checkpoint handling Closes #359 --- agentwatch/rollback/engine.py | 68 +++++++++++++++++++++++++++++------ tests/test_rollback.py | 55 +++++++++++++++++++++++++++- 2 files changed, 111 insertions(+), 12 deletions(-) diff --git a/agentwatch/rollback/engine.py b/agentwatch/rollback/engine.py index c3b9c72f..37587614 100644 --- a/agentwatch/rollback/engine.py +++ b/agentwatch/rollback/engine.py @@ -73,6 +73,8 @@ class RollbackResult: error: str | None = None duration_seconds: float | None = None completed_at: datetime | None = None + warnings: list[str] = field(default_factory=list) + partial_restoration: bool = False class FilesystemSnapshot: @@ -118,17 +120,53 @@ def filter_fn(member: tarfile.TarInfo) -> tarfile.TarInfo | None: return out_path @staticmethod - async def restore(snapshot_path: Path, target_path: Path) -> list[str]: + async def restore( + snapshot_path: Path, + target_path: Path, + ) -> list[str]: restored: list[str] = [] + partial: list[str] = [] def _restore() -> None: + nonlocal restored, partial + extracted: list[str] = [] with tarfile.open(snapshot_path, "r:gz") as tar: members = tar.getmembers() - tar.extractall(str(target_path), filter="data") - restored.extend([m.name for m in members if m.isfile()]) + for member in members: + try: + tar.extract(member, str(target_path), filter="data") + if member.isfile(): + extracted.append(member.name) + except Exception: + logger.warning( + "Failed to extract %s from snapshot %s", + member.name, + snapshot_path, + ) + partial.append(member.name) + restored = extracted await asyncio.get_event_loop().run_in_executor(None, _restore) - logger.info("Restored %d files from snapshot %s", len(restored), snapshot_path) + + if partial: + logger.warning( + "Partially restored %d of %d files from snapshot %s (%d failures)", + len(restored), + len(restored) + len(partial), + snapshot_path, + len(partial), + ) + # Remove partially extracted files to avoid inconsistent state + for fname in partial: + fpath = target_path / fname + try: + if fpath.exists(): + fpath.unlink() + except Exception as exc: + logger.warning("Failed to clean up partial file %s: %s", fpath, exc) + else: + logger.info("Restored %d files from snapshot %s", len(restored), snapshot_path) + return restored @@ -321,13 +359,21 @@ async def rollback( logger.info("Rolled back git to %s", cp.git_commit_ref[:8]) # Filesystem rollback - if restore_filesystem and cp.snapshot_path and cp.snapshot_path.exists(): - restored = await FilesystemSnapshot.restore( - snapshot_path=cp.snapshot_path, - target_path=cwd, - ) - result.rolled_back_files = restored - logger.info("Restored %d files from snapshot", len(restored)) + if restore_filesystem: + if cp.snapshot_path and cp.snapshot_path.exists(): + restored = await FilesystemSnapshot.restore( + snapshot_path=cp.snapshot_path, + target_path=cwd, + ) + result.rolled_back_files = restored + logger.info("Restored %d files from snapshot", len(restored)) + else: + msg = ( + f"Filesystem snapshot for checkpoint {checkpoint_id} " + f"is missing or was never created" + ) + result.warnings.append(msg) + logger.warning(msg) result.status = RollbackStatus.COMPLETED diff --git a/tests/test_rollback.py b/tests/test_rollback.py index 40d2e23d..46a1881c 100644 --- a/tests/test_rollback.py +++ b/tests/test_rollback.py @@ -4,7 +4,13 @@ import pytest -from agentwatch.rollback.engine import FilesystemSnapshot +from agentwatch.rollback.engine import ( + Checkpoint, + CheckpointType, + FilesystemSnapshot, + RollbackEngine, + RollbackStatus, +) def test_valid_archive_restore(tmp_path: Path): @@ -60,3 +66,50 @@ def test_malicious_archive_rejected(tmp_path: Path): # Ensure nothing was extracted to our target path unexpectedly assert not (target_path / "etc" / "passwd").exists() + + +def test_rollback_missing_snapshot_adds_warning(tmp_path: Path): + """Test that rollback with missing snapshot returns a warning.""" + engine = RollbackEngine(checkpoints_dir=tmp_path / ".agentwatch" / "checkpoints") + cp = Checkpoint( + checkpoint_id="ckpt-missing", + session_id="session-1", + step_number=1, + checkpoint_type=CheckpointType.FILESYSTEM, + snapshot_path=tmp_path / "nonexistent" / "snapshot.tar.gz", + working_dir=str(tmp_path), + ) + engine._checkpoints["ckpt-missing"] = cp + engine._session_checkpoints["session-1"] = ["ckpt-missing"] + + result = asyncio.run(engine.rollback("ckpt-missing", restore_git=False)) + assert result.status == RollbackStatus.COMPLETED + assert len(result.warnings) == 1 + assert "missing" in result.warnings[0].lower() + + +def test_rollback_missing_checkpoint_returns_failed(): + """Test that rolling back a non-existent checkpoint returns FAILED.""" + engine = RollbackEngine() + result = asyncio.run(engine.rollback("ckpt-nonexistent")) + assert result.status == RollbackStatus.FAILED + assert result.error is not None + + +def test_rollback_result_tracks_partial_restoration(): + """Test RollbackResult partial_restoration flag and warnings list.""" + result = asyncio.run(_simulate_partial_restoration()) + assert result.status == RollbackStatus.COMPLETED + assert result.partial_restoration is True + assert len(result.warnings) > 0 + + +async def _simulate_partial_restoration() -> RollbackResult: + """Helper to verify RollbackResult tracks partial state.""" + result = RollbackResult( + checkpoint_id="ckpt-p", + status=RollbackStatus.COMPLETED, + warnings=["Some files could not be restored"], + partial_restoration=True, + ) + return result