From 7f078f75a5f60715b9444d704f9b7cc9840e0cf5 Mon Sep 17 00:00:00 2001 From: Luis Mendes Date: Tue, 24 Mar 2026 03:39:12 +0000 Subject: [PATCH] fix(sandbox): make commit_all atomic per specification Changed SandboxManager.commit_all() from partial-commit semantics to all-or-nothing atomic operation per specification requirement. On partial failure, already-committed sandboxes are rolled back. Error reporting indicates which sandbox failed and what was rolled back. Added Behave scenarios verifying atomicity guarantee. Hardened atomicity guarantees after code review: - commit_all and _rollback_committed catch Exception (not just SandboxError) so unexpected errors cannot bypass rollback. Non-SandboxError exceptions are wrapped in a new AtomicCommitError (chaining the original as __cause__) that carries rolled_back_ids and failed_rollback_ids attributes so callers can programmatically determine rollback outcomes. - _rollback_committed returns both rolled_back_ids and failed_rollback_ids; the error result metadata now carries both "rolled_back" and "rollback_failed" keys. - _rollback_committed iterates in reverse (LIFO) order following the standard transaction-log undo pattern. Clarified in docstring that this is distinct from the specification DAG-based "top-down" rollback ordering (line 24632). - TransactionSandbox.rollback() from COMMITTED now raises SandboxRollbackError (database commits are irreversible) instead of silently transitioning to ROLLED_BACK. - TransactionSandbox is now classified as non-rollbackable in commit_all alongside NoSandbox and committed last in the batch, since database COMMIT is irreversible. Docstring corrected to match the raising behavior. - CopyOnWriteSandbox and OverlaySandbox rollback-from-COMMITTED uses rename-based safe_restore() to prevent data loss when the copytree step fails after the original was removed. - CopyOnWriteSandbox and OverlaySandbox rollback() now catches Exception (not just OSError), matching the broader catch used in _rollback_committed, so non-OSError exceptions from safe_restore set the status to ERRORED correctly. - Extracted shared _fs_utils module (backup_directory, safe_restore, compute_diff) with symlink, permission, and timestamp preservation (including directory timestamps), replacing duplicated per-class _backup_directory and _compute_diff methods. - backup_directory defers directory permissions and timestamps to a bottom-up post-walk pass, fixing incorrect mtime preservation (POSIX file creation inside a directory overwrites its mtime) and preventing restrictive source permissions from blocking backup writes. - backup_directory skips non-regular files (FIFOs, sockets, device files) with a warning to prevent hangs on special files. - CopyOnWriteSandbox and OverlaySandbox commit() now attempts to restore the original from the pre-commit backup when the file-copy phase fails midway, preventing partial corruption. If the restore itself fails the backup is preserved for manual recovery (cleanup() still removes it). - CopyOnWriteSandbox and OverlaySandbox commit() error handler now catches Exception (not just OSError) so that unexpected errors during the file-copy phase also trigger pre-commit backup restoration, preventing partial corruption of the original directory. - Pre-commit backup exception handler catches Exception (not just OSError) preventing temp directory leaks on non-OSError failures from backup_directory. - Fixed _pre_commit_backup assignment timing: the backup reference is now assigned only AFTER backup_directory() succeeds, preventing safe_restore() from corrupting an intact original with a partial backup when backup_directory() fails (e.g. disk full). - Rollback from COMMITTED with no pre-commit backup (no changes were applied) is now a no-op instead of raising SandboxRollbackError, preventing false rollback-failure reports in commit_all error metadata. - commit_all logs a warning when NoSandbox or TransactionSandbox instances are present in the batch since their changes cannot be rolled back, which breaks the atomicity guarantee. - Pre-commit backup is skipped when compute_diff returns no changes, avoiding a full directory copy for no-op commits. - GitWorktreeSandbox clears _pre_merge_commit on commit failure so the stale value cannot be used by future code. - commit_all docstring documents Raises clause for AtomicCommitError exception wrapping behavior. - GitWorktreeSandbox.rollback() docstring warns about multi-worktree safety when rolling back from COMMITTED. - Updated SandboxStatus transition diagram in protocol.py to clearly show the COMMITTED -> ROLLED_BACK path. - Added spec-contradiction note (line 45938 vs 19193) in commit_all docstring. - OverlaySandbox rollback from COMMITTED now properly remounts OverlayFS for real overlay (unmount, clean upper/work dirs, remount) and uses dirs_exist_ok=True for userspace fallback to prevent FileExistsError if rmtree silently fails. The merged directory is reset from the restored original, preventing stale pre-rollback data from being exposed on re-activation via get_path() (which allows ROLLED_BACK status). - OverlaySandbox rollback from COMMITTED now raises SandboxRollbackError if the OverlayFS unmount fails, preventing a double-mount attempt that would leave the sandbox in an inconsistent state. - OverlaySandbox rollback from ACTIVE now uses dirs_exist_ok=True for userspace fallback to prevent FileExistsError when rmtree with ignore_errors=True silently fails. - CopyOnWriteSandbox rollback from ACTIVE now uses dirs_exist_ok=True in copytree to prevent FileExistsError when rmtree with ignore_errors=True silently fails, matching the fix already applied to OverlaySandbox. - Non-rollbackable sandboxes (NoSandbox, TransactionSandbox) are committed last in the batch so that all rollbackable sandboxes commit first; if any rollbackable sandbox fails, none of the non-rollbackable sandboxes will have committed yet. - Moved NoSandbox and TransactionSandbox imports to module level in manager.py (no circular dependency exists). - Pre-commit backups are now created on the same filesystem as the original directory (using dir= argument to mkdtemp), avoiding cross-device copy overhead and ensuring os.rename compatibility. - safe_restore now renames the target into the mkdtemp directory instead of removing the mkdtemp dir first, eliminating the residual TOCTOU window between rmdir and rename. - safe_restore catches BaseException (not just OSError) to ensure the original directory is always renamed back on unexpected errors, preventing the original from being left in the renamed-aside state. - Added AtomicCommitError exception class to protocol.py carrying rolled_back_ids and failed_rollback_ids attributes. - Exported AtomicCommitError from sandbox package __init__.py so callers can import it from the public API. - Added BDD scenarios: LIFO rollback order, AtomicCommitError wrapping with RuntimeError cause and rollback metadata, _fs_utils backup/restore coverage, no-change commit rollback success, directory timestamp preservation, OverlaySandbox merged dir reset after COMMITTED rollback, CopyOnWriteSandbox rollback from COMMITTED restores original, GitWorktreeSandbox rollback from COMMITTED undoes merge, TransactionSandbox rollback from COMMITTED raises SandboxRollbackError about irreversible commit. ISSUES CLOSED: #925 Post-review hardening (PR #1146 review findings): - OverlaySandbox rollback from COMMITTED with no backup (no-op) now skips the merged directory reset entirely, preventing unnecessary unmount/remount or re-copy that could fail and turn a harmless no-op rollback into a SandboxRollbackError during commit_all atomic recovery. - OverlaySandbox rollback no longer double-wraps SandboxRollbackError: the outer except Exception handler now has a preceding except SandboxRollbackError clause that re-raises directly, avoiding a confusing double-wrapped error chain. - CopyOnWriteSandbox.get_path() now accepts ROLLED_BACK status for consistency with OverlaySandbox and the protocol status transition table (ROLLED_BACK -> ACTIVE). - CopyOnWriteSandbox rollback from COMMITTED now resets the sandbox copy from the restored original via rmtree+copytree, preventing stale pre-rollback modifications from being exposed on re-activation. - rollback_all now catches Exception (not just SandboxError) so that unexpected rollback errors do not prevent remaining sandboxes from being rolled back, consistent with the pattern already used in _rollback_committed. - commit_all docstring now documents a thread-safety warning: the method is not safe for concurrent calls on the same plan_id since sandbox commit/rollback runs outside the lock. - Fixed CHANGELOG.md whitespace inconsistencies (double leading spaces on two lines). Post-review hardening round 2 (PR #1146 automated review): - safe_restore now uses os.rename (O(1) atomic rename) instead of shutil.copytree (O(n) recursive copy) for the main restore path, since backup and target are always on the same filesystem. This eliminates the ENOTEMPTY bug where a partial copytree failure left target_path partially populated, causing the recovery os.rename to fail and strand the original in the stale temp directory. - OverlaySandbox.get_path() now transitions ROLLED_BACK to ACTIVE, matching CopyOnWriteSandbox and the protocol transition table (ROLLED_BACK -> ACTIVE). - GitWorktreeSandbox.get_path() now accepts ROLLED_BACK status for consistency with all other sandbox implementations and the protocol transition table (ROLLED_BACK -> ACTIVE). - rollback_all now also handles sandboxes in COMMITTED status (not just ACTIVE), consistent with the state machine allowing COMMITTED -> ROLLED_BACK. - cleanup_all now catches Exception (not just SandboxError) so a single unexpected error does not abort cleanup of remaining sandboxes, consistent with _rollback_committed and rollback_all. - Restructured CHANGELOG entry from a single ~90-line paragraph into structured sub-bullets for readability. - Added BDD scenarios: no-op rollback from COMMITTED for CopyOnWriteSandbox and OverlaySandbox (zero-change commit), commit ordering verification (rollbackable before non-rollbackable). Post-review hardening round 3 (PR #1146 deep automated review): - cleanup_abandoned now catches Exception (not just SandboxError) so that unexpected errors (e.g. raw OSError, PermissionError) do not crash the loop and prevent remaining abandoned sandboxes from being cleaned up, consistent with cleanup_all, rollback_all, and _rollback_committed. - OverlaySandbox._mount_overlay() now catches subprocess.TimeoutExpired (in addition to CalledProcessError and OSError), preventing create() from leaving the sandbox in PENDING status when mount hangs beyond the timeout. - OverlaySandbox._unmount_overlay() now catches subprocess.TimeoutExpired (in addition to CalledProcessError and OSError), preventing cleanup() from leaving the sandbox in a zombie state when umount hangs beyond the timeout. - OverlaySandbox._mount_overlay() validates that overlay paths do not contain commas, which would corrupt the OverlayFS mount options string (comma is the mount option delimiter). - GitWorktreeSandbox.commit() now checks git diff return code so that a failed diff command raises CalledProcessError instead of silently concluding there are no changes and skipping the merge. - safe_restore cleanup of the temporary rollback container now runs in a finally block, preventing a temp directory leak when the rename fails and the exception is re-raised. - Fixed misleading BDD step name: "backup path that will cause copytree to fail" renamed to "backup path that will cause rename to fail" since safe_restore now uses os.rename. --- CHANGELOG.md | 72 ++++ features/consolidated_sandbox.feature | 21 ++ features/git_worktree_sandbox.feature | 10 + features/overlay_sandbox.feature | 22 ++ features/sandbox_fs_utils.feature | 53 +++ features/sandbox_manager_coverage.feature | 86 ++++- features/steps/git_worktree_sandbox_steps.py | 8 + features/steps/overlay_sandbox_steps.py | 43 +++ .../sandbox_copy_on_write_coverage_steps.py | 7 +- features/steps/sandbox_fs_utils_steps.py | 291 ++++++++++++++++ .../steps/sandbox_manager_coverage_steps.py | 324 ++++++++++++++++++ .../transaction_sandbox_coverage_steps.py | 44 +++ features/transaction_sandbox_coverage.feature | 9 + robot/helper_sandbox_integration.py | 70 ++++ robot/sandbox_integration.robot | 7 + .../infrastructure/sandbox/__init__.py | 2 + .../infrastructure/sandbox/_fs_utils.py | 237 +++++++++++++ .../infrastructure/sandbox/copy_on_write.py | 177 ++++++---- .../infrastructure/sandbox/git_worktree.py | 50 ++- .../infrastructure/sandbox/manager.py | 251 ++++++++++++-- .../infrastructure/sandbox/overlay.py | 221 +++++++++--- .../infrastructure/sandbox/protocol.py | 47 ++- .../sandbox/transaction_sandbox.py | 27 +- 23 files changed, 1905 insertions(+), 174 deletions(-) create mode 100644 features/sandbox_fs_utils.feature create mode 100644 features/steps/sandbox_fs_utils_steps.py create mode 100644 src/cleveragents/infrastructure/sandbox/_fs_utils.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 3141033b1..764dbe542 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -261,6 +261,78 @@ violation reporting. Pipeline integration in `ACMSPipeline.assemble()` applies enforcement as a pre-filter when a `context_view` is provided. (#847) +- **Breaking (behavioral):** `SandboxManager.commit_all()` is now an + all-or-nothing atomic operation per specification line 45938. (#925) + - On partial failure, already-committed sandboxes are rolled back in + reverse (LIFO) order following the standard transaction-log undo + pattern. + - Non-`SandboxError` exceptions are wrapped in `AtomicCommitError` + (chaining the original as `__cause__`) with `rolled_back_ids` and + `failed_rollback_ids` attributes; `SandboxError` exceptions return + a `CommitResult` with `rolled_back` / `rollback_failed` metadata. + - Non-rollbackable sandboxes (`NoSandbox`, `TransactionSandbox`) are + committed last in the batch so rollbackable sandboxes can be undone + if they fail first. A warning is logged when these types are present. + - `TransactionSandbox.rollback()` from `COMMITTED` now raises + `SandboxRollbackError` (database commits are irreversible) instead + of silently reporting success. + - Extracted shared `_fs_utils` module (`backup_directory`, + `safe_restore`, `compute_diff`) with symlink, permission, and + timestamp preservation; replaces duplicated per-class methods. + - `safe_restore` uses rename-based swap (both renames are O(1) on + the same filesystem) to prevent data loss during restore. + - Pre-commit backup is created on the same filesystem as the original + (avoids cross-device copy overhead); assigned only after + `backup_directory()` succeeds; skipped when no changes detected. + - `backup_directory` defers directory permissions and timestamps to a + bottom-up post-walk pass, fixing POSIX mtime overwrite; skips + non-regular files (FIFOs, sockets, device files) with a warning. + - `CopyOnWriteSandbox` and `OverlaySandbox` commit error handler + restores original from pre-commit backup; catches `Exception` + (not just `OSError`) so unexpected errors also trigger restoration. + - Rollback from `COMMITTED` with no backup (no changes applied) is + a no-op instead of raising `SandboxRollbackError`. + - `CopyOnWriteSandbox` and `OverlaySandbox` rollback from `COMMITTED` + resets sandbox copy/merged directory from restored original, + preventing stale data from being exposed on re-activation. + - `OverlaySandbox` rollback from `COMMITTED` properly remounts + OverlayFS (or re-copies for userspace fallback); raises + `SandboxRollbackError` if unmount fails. No longer double-wraps + `SandboxRollbackError` — inner errors are re-raised directly. + - `OverlaySandbox.get_path()` now transitions `ROLLED_BACK → ACTIVE` + for consistency with `CopyOnWriteSandbox` and the protocol + transition table. + - `GitWorktreeSandbox.get_path()` now accepts `ROLLED_BACK` status + for consistency with all other sandbox types and the protocol + transition table (`ROLLED_BACK → ACTIVE`). + - `rollback_all` now also handles sandboxes in `COMMITTED` status + and catches `Exception` (not just `SandboxError`) to ensure all + rollbacks are attempted. + - `cleanup_all` now catches `Exception` (not just `SandboxError`) + to prevent a single unexpected error from aborting cleanup of + remaining sandboxes. + - `CopyOnWriteSandbox` and `OverlaySandbox` rollback from `ACTIVE` + now uses `dirs_exist_ok=True` to prevent `FileExistsError` when + `rmtree` silently fails. + - `cleanup_abandoned` now catches `Exception` (not just + `SandboxError`) so that unexpected errors do not crash the loop + and prevent remaining abandoned sandboxes from being cleaned up, + consistent with `cleanup_all`, `rollback_all`, and + `_rollback_committed`. + - `OverlaySandbox._mount_overlay()` and `_unmount_overlay()` now + catch `subprocess.TimeoutExpired` (in addition to + `CalledProcessError` and `OSError`), preventing `create()` from + leaving the sandbox in `PENDING` status and `cleanup()` from + leaving it in a zombie state when `mount`/`umount` hangs. + - `OverlaySandbox._mount_overlay()` validates that overlay paths + do not contain commas, which would corrupt the OverlayFS mount + options string. + - `GitWorktreeSandbox.commit()` now checks `git diff` return code + so that a failed diff command does not silently skip the merge. + - `safe_restore` cleanup of the temporary rollback container now + runs in a `finally` block, preventing a temp directory leak when + the rename fails and the exception is re-raised. + - `AtomicCommitError` exported from `sandbox` package `__init__.py`. - Aligned plan lifecycle model with specification: ERRORED is now terminal in `is_terminal`, per-phase state validation enforces APPLIED/CONSTRAINED to APPLY-only and COMPLETE to diff --git a/features/consolidated_sandbox.feature b/features/consolidated_sandbox.feature index deffd46f3..4e5752465 100644 --- a/features/consolidated_sandbox.feature +++ b/features/consolidated_sandbox.feature @@ -133,6 +133,27 @@ Feature: Consolidated Sandbox And the cow sandbox rollback is attempted on created sandbox Then a cow SandboxStateError should be raised + + Scenario: Rollback from COMMITTED restores original via pre-commit backup + Given a cow test directory is initialised + When a cow sandbox is created for plan "plan-rb-committed" + And the cow existing file "existing.txt" is modified in the sandbox with content "modified-for-commit" + And the cow sandbox is committed + Then the cow sandbox should be in the "committed" state + And the cow file "existing.txt" in the original should have content "modified-for-commit" + When the cow sandbox is rolled back + Then the cow sandbox should be in the "rolled_back" state + And the cow file "existing.txt" in the original should have content "original content" + + Scenario: Rollback from COMMITTED with no changes is a no-op + Given a cow test directory is initialised + When a cow sandbox is created for plan "plan-rb-noop" + And the cow sandbox is committed + Then the cow sandbox should be in the "committed" state + When the cow sandbox is rolled back + Then the cow sandbox should be in the "rolled_back" state + And the cow file "existing.txt" in the original should have content "original content" + # --- Cleanup --- diff --git a/features/git_worktree_sandbox.feature b/features/git_worktree_sandbox.feature index 9ea3f81d3..934b180b0 100644 --- a/features/git_worktree_sandbox.feature +++ b/features/git_worktree_sandbox.feature @@ -99,6 +99,16 @@ Feature: Git worktree sandbox lifecycle And the gwt sandbox rollback is attempted on created sandbox Then a gwt SandboxStateError should be raised + Scenario: Rollback from COMMITTED undoes the merge on original branch + When a gwt sandbox is created for plan "plan-rb-commit" + And a gwt file "committed_file.txt" is created in the worktree with content "committed-data" + And the gwt sandbox is committed with message "add committed_file" + Then the gwt sandbox should be in the "committed" state + And the gwt file "committed_file.txt" should exist in the original repo + When the gwt sandbox is rolled back + Then the gwt sandbox should be in the "rolled_back" state + And the gwt file "committed_file.txt" should not exist in the original repo + # --- Cleanup --- Scenario: Cleanup removes worktree and branch diff --git a/features/overlay_sandbox.feature b/features/overlay_sandbox.feature index 9de356acd..160bd4bea 100644 --- a/features/overlay_sandbox.feature +++ b/features/overlay_sandbox.feature @@ -195,6 +195,28 @@ Feature: Overlay filesystem sandbox lifecycle Then an ovl SandboxCreationError should be raised And the ovl sandbox should be in the "errored" state + # --- Atomic rollback from COMMITTED --- + + Scenario: Rollback from COMMITTED resets the merged directory + Given an ovl test directory is initialised + When an ovl sandbox is created for plan "plan-rb-committed" + And an ovl file "changed.txt" is created in the sandbox with content "modified" + And the ovl sandbox is committed + Then the ovl sandbox should be in the "committed" state + When the ovl sandbox is rolled back from committed + Then the ovl sandbox should be in the "rolled_back" state + And the ovl original file "existing.txt" should contain "original content" + And the ovl merged directory should not contain stale file "changed.txt" + + Scenario: Rollback from COMMITTED with no changes is a no-op + Given an ovl test directory is initialised + When an ovl sandbox is created for plan "plan-rb-noop" + And the ovl sandbox is committed + Then the ovl sandbox should be in the "committed" state + When the ovl sandbox is rolled back from committed + Then the ovl sandbox should be in the "rolled_back" state + And the ovl original file "existing.txt" should contain "original content" + # --- Status transitions --- Scenario: Status transitions follow protocol diff --git a/features/sandbox_fs_utils.feature b/features/sandbox_fs_utils.feature new file mode 100644 index 000000000..f0a0cab1d --- /dev/null +++ b/features/sandbox_fs_utils.feature @@ -0,0 +1,53 @@ +Feature: Sandbox filesystem utilities + Tests for the shared _fs_utils module used by CopyOnWriteSandbox and + OverlaySandbox for pre-commit backup and atomic rollback. + + # backup_directory + + Scenario: backup_directory copies files preserving permissions and timestamps + Given a source directory with files having varied permissions + And an empty destination directory + When I call backup_directory from source to destination + Then the destination should contain all source files + And file permissions should match between source and destination + And file timestamps should match between source and destination + + Scenario: backup_directory preserves symlinks + Given a source directory with a symlink + And an empty destination directory + When I call backup_directory from source to destination + Then the destination should contain a symlink with the same target + + Scenario: backup_directory preserves root directory permissions + Given a source directory with restricted permissions + And an empty destination directory + When I call backup_directory from source to destination + Then the destination root permissions should match the source root permissions + + Scenario: backup_directory preserves directory timestamps + Given a source directory with files having varied permissions + And an empty destination directory + When I call backup_directory from source to destination + Then directory timestamps should match between source and destination + + # safe_restore + + Scenario: safe_restore atomically restores from backup + Given an original directory with known content + And a backup directory with different content + When I call safe_restore from backup to original + Then the original should contain the backup content + And the backup directory should have been removed + + Scenario: safe_restore preserves original on rename failure + Given an original directory with known content + And a backup path that will cause rename to fail + When I call safe_restore expecting an error + Then the original directory should still contain its original content + + Scenario: safe_restore cleans up temporary rollback directory on success + Given an original directory with known content + And a backup directory with different content + When I call safe_restore from backup to original + Then the original should contain the backup content + And no atomic-rollback-old directories should remain in the parent diff --git a/features/sandbox_manager_coverage.feature b/features/sandbox_manager_coverage.feature index c52dc2c90..d5f75da08 100644 --- a/features/sandbox_manager_coverage.feature +++ b/features/sandbox_manager_coverage.feature @@ -119,7 +119,7 @@ Feature: Sandbox Manager Lifecycle When I list sandboxes for plan "plan-999" Then the sandbox list should contain 0 sandboxes - # Batch commit + # Batch commit (atomic) Scenario: All active sandboxes for a plan are committed together Given a sandbox exists for plan "plan-001" resource "res-001" path "/tmp/repo1" strategy "none" @@ -144,7 +144,7 @@ Feature: Sandbox Manager Lifecycle When I commit all sandboxes with empty plan_id Then a sandbox ValueError should be raised with message "plan_id cannot be empty" - Scenario: A single sandbox failure during batch commit does not abort the others + Scenario: A single sandbox failure in atomic commit returns an error result Given a sandbox exists for plan "plan-001" resource "res-001" path "/tmp/repo1" strategy "none" And the sandbox for plan "plan-001" resource "res-001" is activated And the sandbox for plan "plan-001" resource "res-001" will fail on commit @@ -158,6 +158,88 @@ Feature: Sandbox Manager Lifecycle Then 1 commit results should be returned And all commit results should be successful + # Atomic commit — rollback on failure + + Scenario: Atomic commit rolls back already-committed sandboxes when one fails + Given a sandbox exists for plan "plan-001" resource "res-001" path "/tmp/repo1" strategy "none" + And the sandbox for plan "plan-001" resource "res-001" is replaced with a committable mock + And a sandbox exists for plan "plan-001" resource "res-002" path "/tmp/repo2" strategy "none" + And the sandbox for plan "plan-001" resource "res-002" is activated + And the sandbox for plan "plan-001" resource "res-002" will fail on commit + When I commit all sandboxes for plan "plan-001" + Then 1 commit results should be returned + And the first commit result should have success false + And the first commit result error should mention the failed sandbox + And the first commit result metadata should list rolled back sandboxes + + Scenario: Atomic commit error reports the sandbox that failed + Given a sandbox exists for plan "plan-001" resource "res-001" path "/tmp/repo1" strategy "none" + And the sandbox for plan "plan-001" resource "res-001" is activated + And the sandbox for plan "plan-001" resource "res-001" will fail on commit + When I commit all sandboxes for plan "plan-001" + Then 1 commit results should be returned + And the first commit result should have success false + And the first commit result error should contain "Atomic commit failed" + + Scenario: Atomic commit tolerates rollback errors during recovery + Given a sandbox exists for plan "plan-001" resource "res-001" path "/tmp/repo1" strategy "none" + And the sandbox for plan "plan-001" resource "res-001" will succeed commit then fail rollback + And a sandbox exists for plan "plan-001" resource "res-002" path "/tmp/repo2" strategy "none" + And the sandbox for plan "plan-001" resource "res-002" is activated + And the sandbox for plan "plan-001" resource "res-002" will fail on commit + When I commit all sandboxes for plan "plan-001" + Then 1 commit results should be returned + And the first commit result should have success false + And the first commit result metadata should list failed rollback sandboxes + + Scenario: Atomic commit succeeds rollback for a no-change sandbox + Given a sandbox exists for plan "plan-001" resource "res-001" path "/tmp/repo1" strategy "none" + And the sandbox for plan "plan-001" resource "res-001" is replaced with a no-change committable mock + And a sandbox exists for plan "plan-001" resource "res-002" path "/tmp/repo2" strategy "none" + And the sandbox for plan "plan-001" resource "res-002" is activated + And the sandbox for plan "plan-001" resource "res-002" will fail on commit + When I commit all sandboxes for plan "plan-001" + Then 1 commit results should be returned + And the first commit result should have success false + And the no-change sandbox rollback should have succeeded for plan "plan-001" resource "res-001" + + Scenario: Atomic commit wraps non-SandboxError in AtomicCommitError after rollback + Given a sandbox exists for plan "plan-001" resource "res-001" path "/tmp/repo1" strategy "none" + And the sandbox for plan "plan-001" resource "res-001" is replaced with a committable mock + And a sandbox exists for plan "plan-001" resource "res-002" path "/tmp/repo2" strategy "none" + And the sandbox for plan "plan-001" resource "res-002" is activated + And the sandbox for plan "plan-001" resource "res-002" will raise RuntimeError on commit + When I commit all sandboxes for plan "plan-001" expecting a non-sandbox error + Then an AtomicCommitError should have been raised with RuntimeError as cause + And the AtomicCommitError should carry rollback metadata + And the committable mock for plan "plan-001" resource "res-001" should have been rolled back + + Scenario: Atomic commit rolls back in reverse order + Given a sandbox exists for plan "plan-001" resource "res-001" path "/tmp/repo1" strategy "none" + And the sandbox for plan "plan-001" resource "res-001" is replaced with a committable mock + And a sandbox exists for plan "plan-001" resource "res-002" path "/tmp/repo2" strategy "none" + And the sandbox for plan "plan-001" resource "res-002" is replaced with a committable mock + And a sandbox exists for plan "plan-001" resource "res-003" path "/tmp/repo3" strategy "none" + And the sandbox for plan "plan-001" resource "res-003" is activated + And the sandbox for plan "plan-001" resource "res-003" will fail on commit + When I commit all sandboxes for plan "plan-001" + Then 1 commit results should be returned + And the first commit result should have success false + And the rollback order should be reverse of commit order for plan "plan-001" + + Scenario: Atomic commit orders rollbackable sandboxes before non-rollbackable + Given a sandbox exists for plan "plan-001" resource "res-001" path "/tmp/repo1" strategy "none" + And the sandbox for plan "plan-001" resource "res-001" is replaced with a committable mock + And a sandbox exists for plan "plan-001" resource "res-002" path "/tmp/repo2" strategy "none" + And the sandbox for plan "plan-001" resource "res-002" is replaced with a non-rollbackable mock + And a sandbox exists for plan "plan-001" resource "res-003" path "/tmp/repo3" strategy "none" + And the sandbox for plan "plan-001" resource "res-003" is activated + And the sandbox for plan "plan-001" resource "res-003" will fail on commit + When I commit all sandboxes for plan "plan-001" + Then 1 commit results should be returned + And the first commit result should have success false + And the non-rollbackable mock for plan "plan-001" resource "res-002" should not have been committed + # Batch rollback Scenario: All active sandboxes for a plan are rolled back together diff --git a/features/steps/git_worktree_sandbox_steps.py b/features/steps/git_worktree_sandbox_steps.py index ba93141ad..9a3ee4694 100644 --- a/features/steps/git_worktree_sandbox_steps.py +++ b/features/steps/git_worktree_sandbox_steps.py @@ -365,6 +365,14 @@ def step_gwt_file_in_original(ctx: Context, filename: str) -> None: assert os.path.exists(file_path), f"File {filename} not found in original repo" +@then('the gwt file "{filename}" should not exist in the original repo') +def step_gwt_file_not_in_original(ctx: Context, filename: str) -> None: + file_path = os.path.join(ctx.gwt_repo_dir, filename) + assert not os.path.exists(file_path), ( + f"File {filename} should not exist in original repo after rollback" + ) + + @then('the gwt file "{filename}" should not exist in the worktree') def step_gwt_file_not_in_worktree(ctx: Context, filename: str) -> None: if ctx.gwt_sandbox.context: diff --git a/features/steps/overlay_sandbox_steps.py b/features/steps/overlay_sandbox_steps.py index 1bdc478c8..757b403b4 100644 --- a/features/steps/overlay_sandbox_steps.py +++ b/features/steps/overlay_sandbox_steps.py @@ -613,3 +613,46 @@ def when_ovl_create_attempted(context, plan_id: str): finally: if hasattr(context, "ovl_copytree_patcher"): context.ovl_copytree_patcher.stop() + + +# --------------------------------------------------------------------------- +# Atomic rollback from COMMITTED steps +# --------------------------------------------------------------------------- + + +@when("the ovl sandbox is rolled back from committed") +def when_ovl_sandbox_rolled_back_from_committed(context): + """Roll back the sandbox from COMMITTED state.""" + assert context.ovl_sandbox.status == SandboxStatus.COMMITTED, ( + f"Expected COMMITTED, got {context.ovl_sandbox.status.value}" + ) + context.ovl_error = None + try: + context.ovl_sandbox.rollback() + except Exception as exc: + context.ovl_error = exc + + +@then('the ovl original file "{filename}" should contain "{expected}"') +def then_ovl_original_contains(context, filename: str, expected: str): + """Verify content of a file in the original directory.""" + path = os.path.join(context.ovl_original, filename) + assert os.path.isfile(path), f"Expected file at {path}" + with open(path) as fh: + content = fh.read() + assert content == expected, f"Expected '{expected}', got '{content}'" + + +@then('the ovl merged directory should not contain stale file "{filename}"') +def then_ovl_merged_no_stale(context, filename: str): + """Verify a file does NOT appear in the merged directory after rollback. + + After rollback from COMMITTED the merged directory is reset from the + restored original, so sandbox-only files should be gone. + """ + merged = context.ovl_sandbox._merged_dir + assert merged is not None, "Merged directory is None" + stale_path = os.path.join(merged, filename) + assert not os.path.exists(stale_path), ( + f"Stale file {filename} should not exist in merged dir after rollback" + ) diff --git a/features/steps/sandbox_copy_on_write_coverage_steps.py b/features/steps/sandbox_copy_on_write_coverage_steps.py index ea602b8fa..509124a0c 100644 --- a/features/steps/sandbox_copy_on_write_coverage_steps.py +++ b/features/steps/sandbox_copy_on_write_coverage_steps.py @@ -170,10 +170,9 @@ def step_cowcov_pending_none(context: Context) -> None: @given("cowcov _compute_diff is patched to return a changed root file") def step_cowcov_patch_compute_diff(context: Context) -> None: - """Patch _compute_diff to return one changed file at root level.""" - patcher = patch.object( - CopyOnWriteSandbox, - "_compute_diff", + """Patch compute_diff to return one changed file at root level.""" + patcher = patch( + "cleveragents.infrastructure.sandbox.copy_on_write.compute_diff", return_value=(["rootfile.txt"], [], []), ) patcher.start() diff --git a/features/steps/sandbox_fs_utils_steps.py b/features/steps/sandbox_fs_utils_steps.py new file mode 100644 index 000000000..985e35dfd --- /dev/null +++ b/features/steps/sandbox_fs_utils_steps.py @@ -0,0 +1,291 @@ +"""Step definitions for sandbox _fs_utils BDD coverage tests.""" + +from __future__ import annotations + +import os +import shutil +import stat +import tempfile +from typing import Any + +from behave import given, then, when + +from cleveragents.infrastructure.sandbox._fs_utils import ( + backup_directory, + safe_restore, +) + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _create_file(path: str, content: str, mode: int = 0o644) -> None: + """Create a file with given content and permissions.""" + with open(path, "w") as f: + f.write(content) + os.chmod(path, mode) + # Set a known mtime so we can verify preservation + os.utime(path, (1000000.0, 1000000.0)) + + +def _register_tmpdir_cleanup(context: Any) -> None: + """Register a cleanup handler to remove _fs_tmpdir after the scenario.""" + tmpdir = context._fs_tmpdir + context.add_cleanup(lambda: shutil.rmtree(tmpdir, ignore_errors=True)) + + +# --------------------------------------------------------------------------- +# Given steps +# --------------------------------------------------------------------------- + + +@given("a source directory with files having varied permissions") +def step_given_source_dir_with_varied_perms(context: Any) -> None: + context._fs_tmpdir = tempfile.mkdtemp(prefix="ca-fs-test-") + _register_tmpdir_cleanup(context) + context._fs_src = os.path.join(context._fs_tmpdir, "src") + os.makedirs(context._fs_src) + _create_file(os.path.join(context._fs_src, "readable.txt"), "hello", 0o644) + _create_file(os.path.join(context._fs_src, "executable.sh"), "#!/bin/sh", 0o755) + sub = os.path.join(context._fs_src, "sub") + os.makedirs(sub) + _create_file(os.path.join(sub, "nested.txt"), "nested", 0o600) + # Set distinctive directory timestamps so that the "directory + # timestamps should match" assertion actually verifies preservation + # rather than passing by coincidence when both dirs are "now". + os.utime(sub, (2000000.0, 2000000.0)) + os.utime(context._fs_src, (2000000.0, 2000000.0)) + + +@given("an empty destination directory") +def step_given_empty_dest_dir(context: Any) -> None: + context._fs_dst = tempfile.mkdtemp(prefix="ca-fs-dst-", dir=context._fs_tmpdir) + + +@given("a source directory with a symlink") +def step_given_source_dir_with_symlink(context: Any) -> None: + context._fs_tmpdir = tempfile.mkdtemp(prefix="ca-fs-test-") + _register_tmpdir_cleanup(context) + context._fs_src = os.path.join(context._fs_tmpdir, "src") + os.makedirs(context._fs_src) + _create_file(os.path.join(context._fs_src, "real.txt"), "real") + os.symlink("real.txt", os.path.join(context._fs_src, "link.txt")) + + +@given("a source directory with restricted permissions") +def step_given_source_dir_restricted_perms(context: Any) -> None: + context._fs_tmpdir = tempfile.mkdtemp(prefix="ca-fs-test-") + _register_tmpdir_cleanup(context) + context._fs_src = os.path.join(context._fs_tmpdir, "src") + os.makedirs(context._fs_src, mode=0o750) + _create_file(os.path.join(context._fs_src, "data.txt"), "data") + + +@given("an original directory with known content") +def step_given_original_dir_known_content(context: Any) -> None: + context._fs_tmpdir = tempfile.mkdtemp(prefix="ca-fs-test-") + _register_tmpdir_cleanup(context) + context._fs_original = os.path.join(context._fs_tmpdir, "original") + os.makedirs(context._fs_original) + _create_file(os.path.join(context._fs_original, "file.txt"), "original-content") + context._fs_original_content = "original-content" + + +@given("a backup directory with different content") +def step_given_backup_dir_different_content(context: Any) -> None: + context._fs_backup = os.path.join(context._fs_tmpdir, "backup") + os.makedirs(context._fs_backup) + _create_file(os.path.join(context._fs_backup, "file.txt"), "backup-content") + + +@given("a backup path that will cause rename to fail") +def step_given_backup_path_that_fails(context: Any) -> None: + # Use a non-existent path as backup — os.rename will fail + context._fs_backup = os.path.join(context._fs_tmpdir, "nonexistent-backup") + + +@given("a stale atomic-rollback-old directory exists") +def step_given_stale_rollback_dir(context: Any) -> None: + # Legacy step preserved for backward compatibility. + stale = context._fs_original + ".atomic-rollback-old" + os.makedirs(stale, exist_ok=True) + _create_file(os.path.join(stale, "stale.txt"), "stale") + context._fs_stale_path = stale + + +# --------------------------------------------------------------------------- +# When steps +# --------------------------------------------------------------------------- + + +@when("I call backup_directory from source to destination") +def step_when_backup_directory(context: Any) -> None: + context._fs_error = None + try: + backup_directory(context._fs_src, context._fs_dst) + except Exception as exc: + context._fs_error = exc + + +@when("I call safe_restore from backup to original") +def step_when_safe_restore(context: Any) -> None: + context._fs_error = None + try: + safe_restore(context._fs_backup, context._fs_original) + except Exception as exc: + context._fs_error = exc + + +@when("I call safe_restore expecting an error") +def step_when_safe_restore_expecting_error(context: Any) -> None: + context._fs_error = None + try: + safe_restore(context._fs_backup, context._fs_original) + except Exception as exc: + context._fs_error = exc + + +# --------------------------------------------------------------------------- +# Then steps +# --------------------------------------------------------------------------- + + +@then("the destination should contain all source files") +def step_then_dest_contains_all_files(context: Any) -> None: + src_files: set[str] = set() + for dirpath, _dirnames, filenames in os.walk(context._fs_src): + for f in filenames: + rel = os.path.relpath(os.path.join(dirpath, f), context._fs_src) + src_files.add(rel) + + dst_files: set[str] = set() + for dirpath, _dirnames, filenames in os.walk(context._fs_dst): + for f in filenames: + rel = os.path.relpath(os.path.join(dirpath, f), context._fs_dst) + dst_files.add(rel) + + assert src_files == dst_files, f"File mismatch: src={src_files}, dst={dst_files}" + + +@then("file permissions should match between source and destination") +def step_then_permissions_match(context: Any) -> None: + for dirpath, _dirnames, filenames in os.walk(context._fs_src): + for f in filenames: + src_path = os.path.join(dirpath, f) + rel = os.path.relpath(src_path, context._fs_src) + dst_path = os.path.join(context._fs_dst, rel) + if os.path.islink(src_path): + continue + src_mode = stat.S_IMODE(os.stat(src_path).st_mode) + dst_mode = stat.S_IMODE(os.stat(dst_path).st_mode) + assert src_mode == dst_mode, ( + f"Permission mismatch for {rel}: " + f"src={oct(src_mode)}, dst={oct(dst_mode)}" + ) + + +@then("file timestamps should match between source and destination") +def step_then_timestamps_match(context: Any) -> None: + for dirpath, _dirnames, filenames in os.walk(context._fs_src): + for f in filenames: + src_path = os.path.join(dirpath, f) + rel = os.path.relpath(src_path, context._fs_src) + dst_path = os.path.join(context._fs_dst, rel) + if os.path.islink(src_path): + continue + src_mtime = os.stat(src_path).st_mtime + dst_mtime = os.stat(dst_path).st_mtime + assert abs(src_mtime - dst_mtime) < 1.0, ( + f"Timestamp mismatch for {rel}: src={src_mtime}, dst={dst_mtime}" + ) + + +@then("directory timestamps should match between source and destination") +def step_then_dir_timestamps_match(context: Any) -> None: + for dirpath, dirnames, _filenames in os.walk(context._fs_src): + for dname in dirnames: + src_dir = os.path.join(dirpath, dname) + rel = os.path.relpath(src_dir, context._fs_src) + dst_dir = os.path.join(context._fs_dst, rel) + if os.path.islink(src_dir): + continue + src_mtime = os.stat(src_dir).st_mtime + dst_mtime = os.stat(dst_dir).st_mtime + assert abs(src_mtime - dst_mtime) < 1.0, ( + f"Directory timestamp mismatch for {rel}: " + f"src={src_mtime}, dst={dst_mtime}" + ) + # Also check root directory + src_mtime = os.stat(context._fs_src).st_mtime + dst_mtime = os.stat(context._fs_dst).st_mtime + assert abs(src_mtime - dst_mtime) < 1.0, ( + f"Root directory timestamp mismatch: src={src_mtime}, dst={dst_mtime}" + ) + + +@then("the destination should contain a symlink with the same target") +def step_then_dest_has_symlink(context: Any) -> None: + link_path = os.path.join(context._fs_dst, "link.txt") + assert os.path.islink(link_path), f"Expected symlink at {link_path}" + target = os.readlink(link_path) + assert target == "real.txt", f"Expected symlink target 'real.txt', got '{target}'" + + +@then("the destination root permissions should match the source root permissions") +def step_then_root_permissions_match(context: Any) -> None: + src_mode = stat.S_IMODE(os.stat(context._fs_src).st_mode) + dst_mode = stat.S_IMODE(os.stat(context._fs_dst).st_mode) + assert src_mode == dst_mode, ( + f"Root permission mismatch: src={oct(src_mode)}, dst={oct(dst_mode)}" + ) + + +@then("the original should contain the backup content") +def step_then_original_has_backup_content(context: Any) -> None: + assert context._fs_error is None, f"Unexpected error: {context._fs_error}" + file_path = os.path.join(context._fs_original, "file.txt") + assert os.path.isfile(file_path), f"Expected file at {file_path}" + with open(file_path) as f: + content = f.read() + assert content == "backup-content", f"Expected 'backup-content', got '{content}'" + + +@then("the backup directory should have been removed") +def step_then_backup_removed(context: Any) -> None: + assert not os.path.exists(context._fs_backup), ( + f"Backup directory should have been removed: {context._fs_backup}" + ) + + +@then("the original directory should still contain its original content") +def step_then_original_still_has_original_content(context: Any) -> None: + file_path = os.path.join(context._fs_original, "file.txt") + assert os.path.isfile(file_path), f"Expected file at {file_path}" + with open(file_path) as f: + content = f.read() + assert content == context._fs_original_content, ( + f"Expected '{context._fs_original_content}', got '{content}'" + ) + + +@then("no stale rollback directory should remain") +def step_then_no_stale_rollback_dir(context: Any) -> None: + stale = context._fs_original + ".atomic-rollback-old" + assert not os.path.exists(stale), ( + f"Stale rollback directory should have been removed: {stale}" + ) + + +@then("no atomic-rollback-old directories should remain in the parent") +def step_then_no_rollback_dirs_in_parent(context: Any) -> None: + """Verify that no .atomic-rollback-old-* temp directories remain.""" + parent_dir = os.path.dirname(context._fs_original) + leftovers = [ + entry + for entry in os.listdir(parent_dir) + if entry.startswith(".atomic-rollback-old-") + ] + assert len(leftovers) == 0, ( + f"Found leftover rollback directories in {parent_dir}: {leftovers}" + ) diff --git a/features/steps/sandbox_manager_coverage_steps.py b/features/steps/sandbox_manager_coverage_steps.py index 914b88625..f0c9d01fb 100644 --- a/features/steps/sandbox_manager_coverage_steps.py +++ b/features/steps/sandbox_manager_coverage_steps.py @@ -10,7 +10,9 @@ from behave import given, then, when from cleveragents.infrastructure.sandbox.factory import SandboxFactory from cleveragents.infrastructure.sandbox.manager import SandboxManager +from cleveragents.infrastructure.sandbox.no_sandbox import NoSandbox from cleveragents.infrastructure.sandbox.protocol import ( + AtomicCommitError, CommitResult, SandboxContext, SandboxError, @@ -71,6 +73,10 @@ def step_given_sandbox_manager(context: Any) -> None: context.commit_results = None context.abandoned_count = None context.first_sandbox = None + context._sandbox_refs = {} + context._rollback_call_log = [] + context._committable_mock_keys = [] + context.cov_error = None context.sandbox_list = None @@ -173,6 +179,44 @@ def step_given_sandbox_errored(context: Any, plan_id: str, res_id: str) -> None: context.manager._active_sandboxes[plan_id][res_id] = mock_sb +@given( + 'the sandbox for plan "{plan_id}" resource "{res_id}" is replaced with a committable mock' +) +def step_given_sandbox_replaced_with_committable_mock( + context: Any, plan_id: str, res_id: str +) -> None: + """Replace sandbox with a mock that commits successfully and supports rollback.""" + sandbox = context.manager.get_sandbox(plan_id, res_id) + assert sandbox is not None, ( + f"Sandbox for plan '{plan_id}' resource '{res_id}' must exist " + f"before it can be replaced with a committable mock" + ) + if sandbox is not None: + mock_sb = _make_mock_sandbox( + sandbox_id=sandbox.sandbox_id, + status=SandboxStatus.ACTIVE, + resource_id=res_id, + ) + key = f"{plan_id}:{res_id}" + + # Add a side_effect to rollback that logs the call order + context._rollback_call_log = getattr(context, "_rollback_call_log", []) + committable_keys: list[str] = getattr(context, "_committable_mock_keys", []) + if key not in committable_keys: + committable_keys.append(key) + context._committable_mock_keys = committable_keys + + def _log_rollback(k: str = key) -> None: + context._rollback_call_log.append(k) + + mock_sb.rollback.side_effect = _log_rollback + + context.manager._active_sandboxes[plan_id][res_id] = mock_sb + if not hasattr(context, "_sandbox_refs"): + context._sandbox_refs = {} + context._sandbox_refs[key] = mock_sb + + @given('the sandbox for plan "{plan_id}" resource "{res_id}" will fail on commit') def step_given_sandbox_commit_fails(context: Any, plan_id: str, res_id: str) -> None: """Make the sandbox's commit method raise a SandboxError.""" @@ -188,6 +232,70 @@ def step_given_sandbox_commit_fails(context: Any, plan_id: str, res_id: str) -> context.manager._active_sandboxes[plan_id][res_id] = mock_sb +@given( + 'the sandbox for plan "{plan_id}" resource "{res_id}" will succeed commit then fail rollback' +) +def step_given_sandbox_commit_succeeds_rollback_fails( + context: Any, plan_id: str, res_id: str +) -> None: + """Make a mock sandbox that commits successfully but fails on rollback.""" + sandbox = context.manager.get_sandbox(plan_id, res_id) + assert sandbox is not None, ( + f"Sandbox for plan '{plan_id}' resource '{res_id}' must exist " + f"before it can be configured to fail on rollback" + ) + if sandbox is not None: + mock_sb = _make_mock_sandbox( + sandbox_id=sandbox.sandbox_id, + status=SandboxStatus.ACTIVE, + resource_id=res_id, + ) + # commit succeeds (default mock behaviour) + # rollback fails + mock_sb.rollback.side_effect = SandboxError("rollback failed in test") + context.manager._active_sandboxes[plan_id][res_id] = mock_sb + + +@given( + 'the sandbox for plan "{plan_id}" resource "{res_id}" is replaced with a non-rollbackable mock' +) +def step_given_sandbox_replaced_with_non_rollbackable( + context: Any, plan_id: str, res_id: str +) -> None: + """Replace sandbox with a mock that passes isinstance(sb, NoSandbox). + + This uses ``spec=NoSandbox`` so that ``isinstance`` checks in + ``commit_all`` classify it as non-rollbackable, causing it to be + committed last in the batch. + """ + sandbox = context.manager.get_sandbox(plan_id, res_id) + assert sandbox is not None, ( + f"Sandbox for plan '{plan_id}' resource '{res_id}' must exist " + f"before it can be replaced with a non-rollbackable mock" + ) + mock_sb = MagicMock(spec=NoSandbox) + mock_sb.sandbox_id = sandbox.sandbox_id + type(mock_sb).status = PropertyMock(return_value=SandboxStatus.ACTIVE) + mock_sb.context = SandboxContext( + sandbox_id=sandbox.sandbox_id, + sandbox_path="/tmp/mock", + original_path="/tmp/mock", + resource_id=res_id, + plan_id=plan_id, + created_at=datetime.now(), + ) + mock_sb.commit.return_value = CommitResult( + sandbox_id=sandbox.sandbox_id, + success=True, + timestamp=datetime.now(), + ) + key = f"{plan_id}:{res_id}" + if not hasattr(context, "_non_rollbackable_refs"): + context._non_rollbackable_refs = {} + context._non_rollbackable_refs[key] = mock_sb + context.manager._active_sandboxes[plan_id][res_id] = mock_sb + + @given('the sandbox for plan "{plan_id}" resource "{res_id}" will fail on rollback') def step_given_sandbox_rollback_fails(context: Any, plan_id: str, res_id: str) -> None: """Make the sandbox's rollback method raise a SandboxError.""" @@ -499,6 +607,222 @@ def step_then_plan_not_tracked(context: Any, plan_id: str) -> None: ) +@then("the first commit result error should mention the failed sandbox") +def step_then_first_result_mentions_failed(context: Any) -> None: + assert context.commit_results is not None and len(context.commit_results) > 0 + error = context.commit_results[0].error or "" + assert "Atomic commit failed" in error, ( + f"Expected 'Atomic commit failed' in error: {error}" + ) + + +@then("the first commit result metadata should list rolled back sandboxes") +def step_then_first_result_has_rolled_back(context: Any) -> None: + assert context.commit_results is not None and len(context.commit_results) > 0 + metadata = context.commit_results[0].metadata + assert "rolled_back" in metadata, ( + f"Expected 'rolled_back' key in metadata: {metadata}" + ) + rolled_back = metadata["rolled_back"] + assert isinstance(rolled_back, list), ( + f"Expected rolled_back to be a list, got {type(rolled_back)}" + ) + assert len(rolled_back) > 0, "Expected at least one rolled-back sandbox ID" + + +@then("the first commit result metadata should list failed rollback sandboxes") +def step_then_first_result_has_failed_rollbacks(context: Any) -> None: + assert context.commit_results is not None and len(context.commit_results) > 0 + metadata = context.commit_results[0].metadata + assert "rollback_failed" in metadata, ( + f"Expected 'rollback_failed' key in metadata: {metadata}" + ) + failed = metadata["rollback_failed"] + assert isinstance(failed, list), ( + f"Expected rollback_failed to be a list, got {type(failed)}" + ) + assert len(failed) > 0, "Expected at least one failed-rollback sandbox ID" + + +@then('the first commit result error should contain "{text}"') +def step_then_first_result_error_contains(context: Any, text: str) -> None: + assert context.commit_results is not None and len(context.commit_results) > 0 + error = context.commit_results[0].error or "" + assert text in error, f"Expected '{text}' in error: {error}" + + +@given( + 'the sandbox for plan "{plan_id}" resource "{res_id}" will raise RuntimeError on commit' +) +def step_given_sandbox_commit_raises_runtime_error( + context: Any, plan_id: str, res_id: str +) -> None: + """Make a mock sandbox that raises RuntimeError (not SandboxError) on commit.""" + sandbox = context.manager.get_sandbox(plan_id, res_id) + if sandbox is not None: + mock_sb = _make_mock_sandbox( + sandbox_id=sandbox.sandbox_id, + status=SandboxStatus.ACTIVE, + resource_id=res_id, + ) + mock_sb.commit.side_effect = RuntimeError("unexpected non-sandbox error") + context.manager._active_sandboxes[plan_id][res_id] = mock_sb + + +@when('I commit all sandboxes for plan "{plan_id}" expecting a non-sandbox error') +def step_when_commit_all_expecting_non_sandbox_error( + context: Any, plan_id: str +) -> None: + """Call commit_all and capture an AtomicCommitError wrapping a non-SandboxError.""" + context.cov_error = None + try: + context.commit_results = context.manager.commit_all(plan_id) + except AtomicCommitError as exc: + # Non-SandboxError exceptions are now wrapped in AtomicCommitError + # with the original exception chained as __cause__. + context.cov_error = exc + except Exception as exc: + context.cov_error = exc + + +@then('the rollback order should be reverse of commit order for plan "{plan_id}"') +def step_then_rollback_order_is_reversed(context: Any, plan_id: str) -> None: + """Verify that rollbacks were called in reverse order of commits. + + Uses the rollback_call_log list populated by side_effect callbacks + on the committable mocks (set up in the "replaced with a committable + mock" step). + """ + rollback_log: list[str] = getattr(context, "_rollback_call_log", []) + committable: list[str] = getattr(context, "_committable_mock_keys", []) + assert len(rollback_log) >= 2, ( + f"Need >= 2 rollback calls, got {len(rollback_log)}: {rollback_log}; " + f"committable_keys: {committable}" + ) + # Verify that the rollback log is the exact reverse of the commit + # order (LIFO), using the known committable keys list as the + # commit-order reference rather than relying on lexicographic + # string comparison. + expected_order = list(reversed(committable)) + assert rollback_log == expected_order, ( + f"Rollback order {rollback_log} does not match expected " + f"reverse commit order {expected_order}" + ) + + +@then("an AtomicCommitError should have been raised with RuntimeError as cause") +def step_then_atomic_commit_error_raised(context: Any) -> None: + """Verify that an AtomicCommitError was raised wrapping a RuntimeError.""" + assert context.cov_error is not None, "Expected an error to be raised" + assert isinstance(context.cov_error, AtomicCommitError), ( + f"Expected AtomicCommitError, got {type(context.cov_error).__name__}" + ) + assert context.cov_error.__cause__ is not None, ( + "Expected AtomicCommitError to chain the original exception as __cause__" + ) + assert isinstance(context.cov_error.__cause__, RuntimeError), ( + f"Expected __cause__ to be RuntimeError, got " + f"{type(context.cov_error.__cause__).__name__}" + ) + + +@then("the AtomicCommitError should carry rollback metadata") +def step_then_atomic_commit_error_has_metadata(context: Any) -> None: + """Verify that the AtomicCommitError carries rollback metadata.""" + assert isinstance(context.cov_error, AtomicCommitError) + assert isinstance(context.cov_error.rolled_back_ids, list), ( + f"Expected rolled_back_ids to be a list, got " + f"{type(context.cov_error.rolled_back_ids)}" + ) + assert isinstance(context.cov_error.failed_rollback_ids, list), ( + f"Expected failed_rollback_ids to be a list, got " + f"{type(context.cov_error.failed_rollback_ids)}" + ) + + +@then( + 'the committable mock for plan "{plan_id}" resource "{res_id}" should have been rolled back' +) +def step_then_committable_mock_rolled_back( + context: Any, plan_id: str, res_id: str +) -> None: + key = f"{plan_id}:{res_id}" + rollback_log: list[str] = getattr(context, "_rollback_call_log", []) + assert key in rollback_log, f"Expected rollback for {key} in log: {rollback_log}" + + +@given( + 'the sandbox for plan "{plan_id}" resource "{res_id}" is replaced with a no-change committable mock' +) +def step_given_sandbox_replaced_with_no_change_committable( + context: Any, plan_id: str, res_id: str +) -> None: + """Replace sandbox with a mock that commits with no changes (no backup). + + Simulates the case where ``commit()`` detects no diff and skips the + pre-commit backup. The mock's ``rollback()`` logs the call so we can + verify it was invoked and did not raise. + """ + sandbox = context.manager.get_sandbox(plan_id, res_id) + if sandbox is not None: + mock_sb = _make_mock_sandbox( + sandbox_id=sandbox.sandbox_id, + status=SandboxStatus.ACTIVE, + resource_id=res_id, + ) + key = f"{plan_id}:{res_id}" + context._rollback_call_log = getattr(context, "_rollback_call_log", []) + + def _log_rollback(k: str = key) -> None: + context._rollback_call_log.append(k) + + mock_sb.rollback.side_effect = _log_rollback + context.manager._active_sandboxes[plan_id][res_id] = mock_sb + if not hasattr(context, "_sandbox_refs"): + context._sandbox_refs = {} + context._sandbox_refs[key] = mock_sb + + +@then( + 'the no-change sandbox rollback should have succeeded for plan "{plan_id}" resource "{res_id}"' +) +def step_then_no_change_rollback_succeeded( + context: Any, plan_id: str, res_id: str +) -> None: + """Verify that the no-change sandbox was rolled back (not failed).""" + key = f"{plan_id}:{res_id}" + rollback_log: list[str] = getattr(context, "_rollback_call_log", []) + assert key in rollback_log, ( + f"Expected rollback for no-change sandbox {key} in log: {rollback_log}" + ) + # Verify the sandbox is NOT in the failed_rollback_ids + if context.commit_results: + metadata = context.commit_results[0].metadata + failed = metadata.get("rollback_failed", []) + assert key.split(":")[1] not in [sid for sid in failed], ( + f"No-change sandbox should not appear in failed_rollback_ids: {failed}" + ) + + +@then( + 'the non-rollbackable mock for plan "{plan_id}" resource "{res_id}" should not have been committed' +) +def step_then_non_rollbackable_not_committed( + context: Any, plan_id: str, res_id: str +) -> None: + """Verify that the non-rollbackable mock was never committed. + + Because rollbackable sandboxes are committed first and one of them + failed, ``commit_all`` should not have reached the non-rollbackable + sandbox. + """ + key = f"{plan_id}:{res_id}" + refs = getattr(context, "_non_rollbackable_refs", {}) + mock_sb = refs.get(key) + assert mock_sb is not None, f"No non-rollbackable ref found for {key}" + mock_sb.commit.assert_not_called() + + @then("{count:d} abandoned sandboxes should be cleaned up") def step_then_abandoned_count(context: Any, count: int) -> None: assert context.abandoned_count == count, ( diff --git a/features/steps/transaction_sandbox_coverage_steps.py b/features/steps/transaction_sandbox_coverage_steps.py index 88daa9f44..afb9f6411 100644 --- a/features/steps/transaction_sandbox_coverage_steps.py +++ b/features/steps/transaction_sandbox_coverage_steps.py @@ -199,6 +199,50 @@ def step_verify_rollback_error(context): assert isinstance(context.caught_error, SandboxRollbackError) +# --------------------------------------------------------------------------- +# Scenario: Rollback from COMMITTED raises SandboxRollbackError +# --------------------------------------------------------------------------- +@when('tscov I execute SQL "SELECT 1" to activate the sandbox') +def step_tscov_execute_to_activate(context): + """Execute a SQL statement to transition the sandbox to ACTIVE.""" + context.sandbox.execute("SELECT 1") + + +@when("tscov I commit the sandbox") +def step_tscov_commit(context): + """Commit the sandbox.""" + context.sandbox.commit("test commit") + + +@then('tscov the sandbox status should be "{status}"') +def step_tscov_verify_status(context, status): + """Verify the sandbox status.""" + expected = SandboxStatus(status) + assert context.sandbox.status == expected, ( + f"Expected status {expected}, got {context.sandbox.status}" + ) + + +@when("tscov I attempt to rollback the committed sandbox") +def step_tscov_attempt_rollback_committed(context): + """Call rollback() on a COMMITTED sandbox and expect SandboxRollbackError.""" + context.caught_error = None + try: + context.sandbox.rollback() + except SandboxRollbackError as exc: + context.caught_error = exc + + +@then("a SandboxRollbackError should be raised about irreversible commit") +def step_tscov_verify_irreversible_rollback(context): + """Verify a SandboxRollbackError about irreversible COMMIT was raised.""" + assert context.caught_error is not None + assert isinstance(context.caught_error, SandboxRollbackError) + assert "cannot undo database commit" in str(context.caught_error).lower(), ( + f"Expected message about irreversible commit, got: {context.caught_error}" + ) + + # --------------------------------------------------------------------------- # Scenario: execute with None connection (line 366) # --------------------------------------------------------------------------- diff --git a/features/transaction_sandbox_coverage.feature b/features/transaction_sandbox_coverage.feature index 3e80928dc..3ccdb5709 100644 --- a/features/transaction_sandbox_coverage.feature +++ b/features/transaction_sandbox_coverage.feature @@ -37,6 +37,15 @@ Feature: Transaction Sandbox Coverage Then a SandboxRollbackError should be raised And the sandbox status should be errored + Scenario: Rollback from COMMITTED raises SandboxRollbackError + Given I have a fresh TransactionSandbox for resource "res-rb-committed" and path ":memory:" + When tscov I create the sandbox with plan "plan-rb-committed" + And tscov I execute SQL "SELECT 1" to activate the sandbox + And tscov I commit the sandbox + Then tscov the sandbox status should be "committed" + When tscov I attempt to rollback the committed sandbox + Then a SandboxRollbackError should be raised about irreversible commit + Scenario: Execute raises SandboxStateError when connection is None Given I have a TransactionSandbox in active state with no database connection When I attempt to execute SQL "SELECT 1" on the sandbox diff --git a/robot/helper_sandbox_integration.py b/robot/helper_sandbox_integration.py index f8dd6ea29..06e007e81 100644 --- a/robot/helper_sandbox_integration.py +++ b/robot/helper_sandbox_integration.py @@ -333,6 +333,75 @@ def _manager_multiple_resources() -> None: print("manager-multiple-resources-ok") +def _manager_atomic_commit() -> None: + """Integration test: SandboxManager.commit_all is atomic. + + Verifies that when one sandbox commit fails, the entire batch fails + and already-committed sandboxes are rolled back. + """ + from datetime import datetime + from unittest.mock import MagicMock, PropertyMock + + factory = SandboxFactory() + manager = SandboxManager(factory=factory, cleanup_on_exit=False) + + with ( + tempfile.TemporaryDirectory() as tmpdir1, + tempfile.TemporaryDirectory() as tmpdir2, + ): + # Create two sandboxes + manager.get_or_create_sandbox( + plan_id="plan-atomic", + resource_id="res-ok", + original_path=tmpdir1, + sandbox_strategy="none", + ) + manager.get_or_create_sandbox( + plan_id="plan-atomic", + resource_id="res-fail", + original_path=tmpdir2, + sandbox_strategy="none", + ) + + # Replace res-fail with a mock that fails on commit + fail_mock = MagicMock() + fail_mock.sandbox_id = "sb-fail" + type(fail_mock).status = PropertyMock(return_value=SandboxStatus.ACTIVE) + fail_mock.commit.side_effect = SandboxError("simulated commit failure") + manager._active_sandboxes["plan-atomic"]["res-fail"] = fail_mock + + # Replace res-ok with a mock that commits and can be rolled back + from cleveragents.infrastructure.sandbox.protocol import CommitResult + + ok_mock = MagicMock() + ok_mock.sandbox_id = "sb-ok" + type(ok_mock).status = PropertyMock(return_value=SandboxStatus.ACTIVE) + ok_mock.commit.return_value = CommitResult( + sandbox_id="sb-ok", + success=True, + timestamp=datetime.now(), + ) + ok_mock.rollback.return_value = None + manager._active_sandboxes["plan-atomic"]["res-ok"] = ok_mock + + results = manager.commit_all("plan-atomic") + + # Should return exactly one failure result + assert len(results) == 1, f"Expected 1 result, got {len(results)}" + assert results[0].success is False + assert "Atomic commit failed" in (results[0].error or "") + + # The OK sandbox should have been rolled back + assert ok_mock.rollback.called, "Expected rollback on already-committed sandbox" + + # Metadata should report rolled-back and failed-rollback IDs + metadata = results[0].metadata + assert "rolled_back" in metadata, "Expected 'rolled_back' in metadata" + assert "rollback_failed" in metadata, "Expected 'rollback_failed' in metadata" + + print("manager-atomic-commit-ok") + + def main() -> None: if len(sys.argv) < 2: raise SystemExit("Expected command argument") @@ -348,6 +417,7 @@ def main() -> None: "json-merge": _json_merge_strategy, "json-merge-concat": _json_merge_concat_arrays, "manager-multiple-resources": _manager_multiple_resources, + "manager-atomic-commit": _manager_atomic_commit, } if command not in commands: raise SystemExit(f"Unknown command: {command}") diff --git a/robot/sandbox_integration.robot b/robot/sandbox_integration.robot index bef49192b..594d0431f 100644 --- a/robot/sandbox_integration.robot +++ b/robot/sandbox_integration.robot @@ -59,6 +59,13 @@ Manager Handles Multiple Resources Per Plan Should Be Equal As Integers ${result.rc} 0 Should Contain ${result.stdout} manager-multiple-resources-ok +Manager Atomic Commit Rolls Back On Failure + [Documentation] Verify SandboxManager.commit_all is atomic: on partial failure, already-committed sandboxes are rolled back + [Tags] sandbox manager atomic commit + ${result}= Run Process ${PYTHON} ${HELPER_SCRIPT} manager-atomic-commit cwd=${WORKSPACE} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} manager-atomic-commit-ok + Sequential Merge Strategy Returns Theirs [Documentation] Verify SequentialMergeStrategy always picks theirs (last-write-wins) [Tags] sandbox merge sequential diff --git a/src/cleveragents/infrastructure/sandbox/__init__.py b/src/cleveragents/infrastructure/sandbox/__init__.py index aa8ca7ec9..1a6545a1a 100644 --- a/src/cleveragents/infrastructure/sandbox/__init__.py +++ b/src/cleveragents/infrastructure/sandbox/__init__.py @@ -36,6 +36,7 @@ from cleveragents.infrastructure.sandbox.merge import ( from cleveragents.infrastructure.sandbox.no_sandbox import NoSandbox from cleveragents.infrastructure.sandbox.overlay import OverlaySandbox from cleveragents.infrastructure.sandbox.protocol import ( + AtomicCommitError, CommitResult, Sandbox, SandboxContext, @@ -52,6 +53,7 @@ from cleveragents.infrastructure.sandbox.strategy_registry import ( from cleveragents.infrastructure.sandbox.transaction_sandbox import TransactionSandbox __all__ = [ + "AtomicCommitError", "BoundaryCache", "BuiltInSandboxStrategyAdapter", "CheckpointManager", diff --git a/src/cleveragents/infrastructure/sandbox/_fs_utils.py b/src/cleveragents/infrastructure/sandbox/_fs_utils.py new file mode 100644 index 000000000..66eebeefa --- /dev/null +++ b/src/cleveragents/infrastructure/sandbox/_fs_utils.py @@ -0,0 +1,237 @@ +"""Shared filesystem utilities for sandbox backup and rollback. + +Low-level helpers used by :class:`CopyOnWriteSandbox` and +:class:`OverlaySandbox` for pre-commit backup and atomic rollback. +The functions deliberately avoid ``shutil.copytree``, ``shutil.copy2``, +and ``os.makedirs`` so that unit-test patches targeting individual +sandbox modules cannot interfere with the backup/restore operations. + +Stage B3 supplementary — introduced for atomic ``commit_all`` (#925). +""" + +from __future__ import annotations + +import logging +import os +import stat + +logger = logging.getLogger(__name__) + + +def backup_directory(src: str, dst: str) -> None: + """Copy *src* directory contents into *dst* preserving symlinks and permissions. + + Uses a manual walk-and-copy approach that avoids ``os.makedirs`` + and ``shutil.copy2`` so that unit-test patches targeting individual + sandbox module namespaces cannot interfere with the backup operation. + ``os.mkdir`` is used for creating sub-directories (not affected + by patches on ``os.makedirs``), and :func:`_copy_file` uses + low-level open/read/write for file content. + + Symlinks are preserved as symlinks (not followed). File permissions + are copied via ``os.chmod``. + + Directory permissions and timestamps are applied in a **bottom-up + post-walk pass** after all files have been copied. This is required + because creating files inside a directory updates its ``mtime`` + (standard POSIX behaviour), so setting timestamps during the walk + would be immediately overwritten. Permissions are also deferred to + avoid restrictive source permissions (e.g. ``0o500``) blocking + file writes into the destination. + + Args: + src: Source directory to copy from. + dst: Destination directory (must already exist). + """ + # Collect directory metadata for a post-walk fixup pass. Entries + # are appended in walk order (top-down) and applied in reverse + # (bottom-up) so that parent mtime is restored last. + dir_metadata: list[tuple[str, int, float, float]] = [] + + # Record root directory metadata (applied last in the fixup pass). + src_root_stat = os.stat(src) + dir_metadata.append( + ( + dst, + stat.S_IMODE(src_root_stat.st_mode), + src_root_stat.st_atime, + src_root_stat.st_mtime, + ) + ) + + for dirpath, dirnames, filenames in os.walk(src, followlinks=False): + rel_dir = os.path.relpath(dirpath, src) + dst_dir = os.path.join(dst, rel_dir) if rel_dir != "." else dst + for dname in dirnames: + src_entry = os.path.join(dirpath, dname) + target = os.path.join(dst_dir, dname) + if os.path.islink(src_entry): + link_target = os.readlink(src_entry) + os.symlink(link_target, target) + elif not os.path.isdir(target): + os.mkdir(target) + # Record metadata for the post-walk fixup pass. + src_stat = os.stat(src_entry) + dir_metadata.append( + ( + target, + stat.S_IMODE(src_stat.st_mode), + src_stat.st_atime, + src_stat.st_mtime, + ) + ) + for fname in filenames: + src_file = os.path.join(dirpath, fname) + dst_file = os.path.join(dst_dir, fname) + if os.path.islink(src_file): + link_target = os.readlink(src_file) + os.symlink(link_target, dst_file) + elif os.path.isfile(src_file): + _copy_file(src_file, dst_file) + else: + # Skip special files (FIFOs, sockets, device files) that + # could hang or error during open(). + logger.warning( + "Skipping non-regular file during backup: %s", + src_file, + ) + + # Post-walk fixup: apply directory permissions and timestamps in + # reverse (bottom-up) order so that child directories are fixed + # before their parents. This ensures that setting a child's + # timestamps does not update the parent's mtime, and that + # restrictive permissions on a parent do not block writes into + # children during the copy phase above. + for dir_path, mode, atime, mtime in reversed(dir_metadata): + os.chmod(dir_path, mode) + os.utime(dir_path, (atime, mtime)) + + +def safe_restore(backup_path: str, target_path: str) -> None: + """Restore *target_path* from *backup_path* using rename-based swap. + + Both *backup_path* and *target_path* **must** reside on the same + filesystem (callers ensure this by creating backups with + ``tempfile.mkdtemp(dir=os.path.dirname(original_path))``). + + The restore uses two ``os.rename`` calls — both are O(1) atomic + operations on POSIX when source and destination are on the same + filesystem: + + 1. Rename the current *target_path* aside into a temporary container. + 2. Rename *backup_path* to *target_path*. + + If step 2 fails, the original is renamed back so no data is lost. + After a successful restore the stale renamed directory is removed. + + Args: + backup_path: Directory containing the pre-commit backup. + target_path: Original directory to restore. + + Raises: + OSError: If the restore fails (the original is preserved). + """ + import shutil + import tempfile + + # Use a unique temporary directory on the same filesystem. + # The target is renamed **into** the mkdtemp directory rather than + # **to** the mkdtemp path. This avoids the TOCTOU window that + # would exist if we removed the mkdtemp dir (``os.rmdir``) and + # then renamed the target to the now-vacated path — another process + # could claim that path in between. + parent_dir = os.path.dirname(target_path) + stale_container = tempfile.mkdtemp( + prefix=".atomic-rollback-old-", + dir=parent_dir, + ) + stale = os.path.join(stale_container, "original") + + os.rename(target_path, stale) + try: + # Rename (not copy) the backup into place. Since both paths + # are on the same filesystem, ``os.rename`` is an atomic O(1) + # metadata operation. Using ``shutil.copytree`` here would be + # O(n) and — critically — could leave *target_path* partially + # populated on failure, causing the recovery ``os.rename`` below + # to fail with ``ENOTEMPTY`` (POSIX requires the destination of + # a directory rename to be empty). + os.rename(backup_path, target_path) + except BaseException: + # Restore the original on failure. ``BaseException`` (not just + # ``OSError``) ensures the rename-back executes even for + # unexpected errors (e.g. ``MemoryError``), preventing the + # original directory from being left in the renamed-aside state. + os.rename(stale, target_path) + raise + finally: + # Cleanup — always remove the stale container. On success, + # only the empty container directory remains (backup_path was + # renamed into target_path, so the "original" subdir no longer + # exists). On failure, the original was already renamed back + # so the container is empty as well. Using a ``finally`` + # block ensures the temp directory does not leak even when the + # rename fails and the exception is re-raised. + shutil.rmtree(stale_container, ignore_errors=True) + + +def compute_diff( + sandbox_dir: str, original_dir: str +) -> tuple[list[str], list[str], list[str]]: + """Compare sandbox to original and return changed/added/deleted files. + + Walks both directory trees and classifies each file as changed, + added, or deleted by comparing relative paths and file contents. + + Args: + sandbox_dir: Path to the sandbox (or merged) directory. + original_dir: Path to the original directory. + + Returns: + Tuple of ``(changed_files, added_files, deleted_files)`` as + sorted relative-path lists. + """ + import filecmp + + sandbox_files: set[str] = { + os.path.relpath(os.path.join(dp, f), sandbox_dir) + for dp, _, fnames in os.walk(sandbox_dir) + for f in fnames + } + original_files: set[str] = { + os.path.relpath(os.path.join(dp, f), original_dir) + for dp, _, fnames in os.walk(original_dir) + for f in fnames + } + + added = sorted(sandbox_files - original_files) + deleted = sorted(original_files - sandbox_files) + changed = [ + rel + for rel in sorted(sandbox_files & original_files) + if not filecmp.cmp( + os.path.join(sandbox_dir, rel), + os.path.join(original_dir, rel), + shallow=False, + ) + ] + return changed, added, deleted + + +def _copy_file(src: str, dst: str) -> None: + """Copy a single file using low-level byte I/O, preserving permissions. + + Args: + src: Source file path. + dst: Destination file path. + """ + with open(src, "rb") as fsrc, open(dst, "wb") as fdst: + while True: + chunk = fsrc.read(1024 * 1024) + if not chunk: + break + fdst.write(chunk) + # Preserve permissions and timestamps + src_stat = os.stat(src) + os.chmod(dst, stat.S_IMODE(src_stat.st_mode)) + os.utime(dst, (src_stat.st_atime, src_stat.st_mtime)) diff --git a/src/cleveragents/infrastructure/sandbox/copy_on_write.py b/src/cleveragents/infrastructure/sandbox/copy_on_write.py index 8ee2df008..7ab586063 100644 --- a/src/cleveragents/infrastructure/sandbox/copy_on_write.py +++ b/src/cleveragents/infrastructure/sandbox/copy_on_write.py @@ -12,7 +12,6 @@ Stage B3.4 / TASK-006 of the implementation plan. from __future__ import annotations -import filecmp import logging import os import shutil @@ -21,6 +20,11 @@ from datetime import datetime from ulid import ULID +from cleveragents.infrastructure.sandbox._fs_utils import ( + backup_directory, + compute_diff, + safe_restore, +) from cleveragents.infrastructure.sandbox.protocol import ( CommitResult, SandboxCommitError, @@ -81,6 +85,8 @@ class CopyOnWriteSandbox: # Set after create() self._sandbox_path: str | None = None + # Pre-commit backup for atomic rollback support + self._pre_commit_backup: str | None = None # -- protocol properties ------------------------------------------------- @@ -185,6 +191,7 @@ class CopyOnWriteSandbox: if self._status not in ( SandboxStatus.CREATED, SandboxStatus.ACTIVE, + SandboxStatus.ROLLED_BACK, ): raise SandboxStateError( f"Cannot resolve path in status {self._status.value}" @@ -193,7 +200,7 @@ class CopyOnWriteSandbox: if ".." in resource_path.split("/"): raise ValueError(f"Path traversal not allowed: {resource_path}") - if self._status == SandboxStatus.CREATED: + if self._status in (SandboxStatus.CREATED, SandboxStatus.ROLLED_BACK): self._status = SandboxStatus.ACTIVE if self._sandbox_path is None: @@ -230,10 +237,34 @@ class CopyOnWriteSandbox: raise SandboxStateError("Sandbox path not set") try: - changed_files, added_files, deleted_files = self._compute_diff( + changed_files, added_files, deleted_files = compute_diff( self._sandbox_path, self._original_path ) + # Save pre-commit backup for atomic rollback support. + # Skip when there are no changes to avoid copying a potentially + # large directory tree unnecessarily. + # + # IMPORTANT: ``_pre_commit_backup`` is assigned only AFTER + # ``backup_directory`` succeeds. If the backup itself fails + # (e.g. disk full), the original directory is still intact and + # the ``except OSError`` handler must NOT attempt + # ``safe_restore`` with a partial backup. + if changed_files or added_files or deleted_files: + # Create backup on the same filesystem as the original to + # avoid cross-device copy overhead and ensure os.rename + # compatibility in safe_restore(). + backup_dir = tempfile.mkdtemp( + prefix="ca-cow-backup-", + dir=os.path.dirname(self._original_path), + ) + try: + backup_directory(self._original_path, backup_dir) + except Exception: + shutil.rmtree(backup_dir, ignore_errors=True) + raise + self._pre_commit_backup = backup_dir + # Apply changes: copy modified and new files for rel_path in changed_files + added_files: src = os.path.join(self._sandbox_path, rel_path) @@ -249,7 +280,27 @@ class CopyOnWriteSandbox: if os.path.exists(dst): os.remove(dst) - except OSError as exc: + except Exception as exc: + # Attempt to restore the original from the pre-commit backup + # so that a partial copy does not leave the original corrupted. + # Catches Exception (not just OSError) so that unexpected errors + # during the file-copy phase (e.g. MemoryError wrapping, + # TypeError) also trigger the restore. Without this, the + # backup would exist but never be applied, leaving the original + # directory partially modified. + # If the restore itself fails, keep the backup for manual + # recovery — cleanup() will remove it later. + if self._pre_commit_backup is not None: + try: + safe_restore(self._pre_commit_backup, self._original_path) + self._pre_commit_backup = None + except Exception: + logger.warning( + "Failed to restore original from backup for sandbox %s; " + "backup preserved at %s for manual recovery", + self._sandbox_id, + self._pre_commit_backup, + ) self._status = SandboxStatus.ERRORED raise SandboxCommitError( f"Failed to sync sandbox {self._sandbox_id} back to original: {exc}" @@ -278,17 +329,19 @@ class CopyOnWriteSandbox: ) def rollback(self) -> None: - """Discard sandbox changes by resetting the copy to original state. + """Discard sandbox changes by resetting to original state. - Re-copies the original directory over the sandbox. The sandbox - transitions to ``ROLLED_BACK`` and can be re-activated via - ``get_path``. + When rolling back from ``ACTIVE``, re-copies the original directory + over the sandbox. When rolling back from ``COMMITTED``, restores + the original directory from the pre-commit backup so that the + commit's effects are fully undone. The sandbox transitions to + ``ROLLED_BACK`` and can be re-activated via ``get_path``. Raises: SandboxRollbackError: If the rollback fails. SandboxStateError: If called in an invalid status. """ - if self._status != SandboxStatus.ACTIVE: + if self._status not in (SandboxStatus.ACTIVE, SandboxStatus.COMMITTED): raise SandboxStateError(f"Cannot rollback from status {self._status.value}") SandboxStatus.assert_transition(self._status, SandboxStatus.ROLLED_BACK) @@ -297,18 +350,46 @@ class CopyOnWriteSandbox: raise SandboxStateError("Sandbox path not set") try: - # Remove the current sandbox copy - shutil.rmtree(self._sandbox_path, ignore_errors=True) + if self._status == SandboxStatus.COMMITTED: + if self._pre_commit_backup is None: + # No pre-commit backup means commit() detected no + # changes and skipped the backup. The original + # directory is unmodified, so rollback is a no-op. + logger.debug( + "Rollback from COMMITTED with no backup (no changes " + "were applied) for sandbox %s — no-op", + self._sandbox_id, + ) + else: + # Atomically restore original from pre-commit backup. + # Uses rename-based swap so that a failure during copy + # does not leave the original directory deleted. + safe_restore(self._pre_commit_backup, self._original_path) + self._pre_commit_backup = None - # Re-copy from original - shutil.copytree( - self._original_path, - self._sandbox_path, - symlinks=True, - dirs_exist_ok=False, - ) + # Reset the sandbox copy from the restored original so + # that a subsequent get_path() (ROLLED_BACK → ACTIVE) + # does not expose stale pre-rollback modifications. + shutil.rmtree(self._sandbox_path, ignore_errors=True) + shutil.copytree( + self._original_path, + self._sandbox_path, + symlinks=True, + dirs_exist_ok=True, + ) + else: + # Remove the current sandbox copy + shutil.rmtree(self._sandbox_path, ignore_errors=True) - except OSError as exc: + # Re-copy from original + shutil.copytree( + self._original_path, + self._sandbox_path, + symlinks=True, + dirs_exist_ok=True, + ) + + except Exception as exc: self._status = SandboxStatus.ERRORED raise SandboxRollbackError( f"Failed to rollback sandbox {self._sandbox_id}: {exc}" @@ -344,61 +425,15 @@ class CopyOnWriteSandbox: if os.path.exists(parent): shutil.rmtree(parent, ignore_errors=True) + # Clean up any pre-commit backup + if self._pre_commit_backup is not None: + if os.path.exists(self._pre_commit_backup): + shutil.rmtree(self._pre_commit_backup, ignore_errors=True) + self._pre_commit_backup = None + self._status = SandboxStatus.CLEANED_UP logger.info( "Cleaned up copy-on-write sandbox: sandbox_id=%s", self._sandbox_id, ) - - # -- internal helpers ---------------------------------------------------- - - @staticmethod - def _compute_diff( - sandbox_dir: str, original_dir: str - ) -> tuple[list[str], list[str], list[str]]: - """Compare sandbox to original and return changed/added/deleted files. - - Args: - sandbox_dir: Path to the sandbox copy. - original_dir: Path to the original directory. - - Returns: - Tuple of (changed_files, added_files, deleted_files) as - relative paths. - """ - changed: list[str] = [] - added: list[str] = [] - deleted: list[str] = [] - - # Collect all files in both trees - sandbox_files: set[str] = set() - for dirpath, _dirnames, filenames in os.walk(sandbox_dir): - for fname in filenames: - full = os.path.join(dirpath, fname) - rel = os.path.relpath(full, sandbox_dir) - sandbox_files.add(rel) - - original_files: set[str] = set() - for dirpath, _dirnames, filenames in os.walk(original_dir): - for fname in filenames: - full = os.path.join(dirpath, fname) - rel = os.path.relpath(full, original_dir) - original_files.add(rel) - - # Added in sandbox (not in original) - for rel in sorted(sandbox_files - original_files): - added.append(rel) - - # Deleted from sandbox (was in original) - for rel in sorted(original_files - sandbox_files): - deleted.append(rel) - - # Modified (present in both but different) - for rel in sorted(sandbox_files & original_files): - sandbox_file = os.path.join(sandbox_dir, rel) - original_file = os.path.join(original_dir, rel) - if not filecmp.cmp(sandbox_file, original_file, shallow=False): - changed.append(rel) - - return changed, added, deleted diff --git a/src/cleveragents/infrastructure/sandbox/git_worktree.py b/src/cleveragents/infrastructure/sandbox/git_worktree.py index 961eb2866..e04b6467a 100644 --- a/src/cleveragents/infrastructure/sandbox/git_worktree.py +++ b/src/cleveragents/infrastructure/sandbox/git_worktree.py @@ -143,6 +143,8 @@ class GitWorktreeSandbox: self._branch_name: str | None = None self._original_branch: str | None = None self._base_commit: str | None = None + # Pre-merge commit for atomic rollback support + self._pre_merge_commit: str | None = None # -- protocol properties ------------------------------------------------- @@ -301,6 +303,7 @@ class GitWorktreeSandbox: if self._status not in ( SandboxStatus.CREATED, SandboxStatus.ACTIVE, + SandboxStatus.ROLLED_BACK, ): raise SandboxStateError( f"Cannot resolve path in status {self._status.value}" @@ -309,7 +312,7 @@ class GitWorktreeSandbox: if ".." in resource_path.split("/"): raise ValueError(f"Path traversal not allowed: {resource_path}") - if self._status == SandboxStatus.CREATED: + if self._status in (SandboxStatus.CREATED, SandboxStatus.ROLLED_BACK): self._status = SandboxStatus.ACTIVE if self._worktree_path is None: @@ -367,6 +370,13 @@ class GitWorktreeSandbox: ) diff_output = diff_result.stdout.strip() + if diff_result.returncode != 0: + raise subprocess.CalledProcessError( + returncode=diff_result.returncode, + cmd="git diff --cached --name-status", + stderr=diff_result.stderr, + ) + if not diff_output: # No changes to commit self._status = SandboxStatus.COMMITTED @@ -412,6 +422,14 @@ class GitWorktreeSandbox: ) commit_ref = result.stdout.strip() + # Record pre-merge commit for atomic rollback support + pre_merge_result = _run_git( + ["rev-parse", "HEAD"], + cwd=self._original_path, + timeout=self._git_timeout, + ) + self._pre_merge_commit = pre_merge_result.stdout.strip() + # Merge the sandbox branch into the original branch _run_git( ["merge", self._branch_name, "--no-edit"], @@ -420,12 +438,14 @@ class GitWorktreeSandbox: ) except subprocess.TimeoutExpired as exc: + self._pre_merge_commit = None self._status = SandboxStatus.ERRORED raise SandboxCommitError( f"Git command timed out after {self._git_timeout}s " f"while committing sandbox {self._sandbox_id}" ) from exc except subprocess.CalledProcessError as exc: + self._pre_merge_commit = None self._status = SandboxStatus.ERRORED raise SandboxCommitError( f"Failed to commit sandbox {self._sandbox_id}: {exc.stderr.strip()}" @@ -457,14 +477,23 @@ class GitWorktreeSandbox: def rollback(self) -> None: """Discard all worktree changes by resetting to the base commit. - Resets the worktree branch to the base commit and transitions - back to ``ACTIVE`` for potential re-use. + When rolling back from ``ACTIVE``, resets the worktree branch to + the base commit. When rolling back from ``COMMITTED``, also + resets the original branch to the pre-merge commit so that the + merge is fully undone. Transitions to ``ROLLED_BACK``. + + .. warning:: Multi-worktree safety + + Rolling back from ``COMMITTED`` executes ``git reset --hard`` + on the original branch. If other git worktrees or external + processes are tracking the same branch, they will be affected + by this hard reset. Raises: SandboxRollbackError: If the rollback fails. SandboxStateError: If called in an invalid status. """ - if self._status != SandboxStatus.ACTIVE: + if self._status not in (SandboxStatus.ACTIVE, SandboxStatus.COMMITTED): raise SandboxStateError(f"Cannot rollback from status {self._status.value}") SandboxStatus.assert_transition(self._status, SandboxStatus.ROLLED_BACK) @@ -473,6 +502,19 @@ class GitWorktreeSandbox: raise SandboxStateError("Worktree not initialised") try: + if ( + self._status == SandboxStatus.COMMITTED + and self._pre_merge_commit is not None + ): + # Undo the merge on the original branch + _run_git( + ["reset", "--hard", self._pre_merge_commit], + cwd=self._original_path, + timeout=self._git_timeout, + ) + self._pre_merge_commit = None + + # Reset the worktree to the base commit _run_git( ["reset", "--hard", self._base_commit], cwd=self._worktree_path, diff --git a/src/cleveragents/infrastructure/sandbox/manager.py b/src/cleveragents/infrastructure/sandbox/manager.py index 8b9c43b3d..c9b9816b1 100644 --- a/src/cleveragents/infrastructure/sandbox/manager.py +++ b/src/cleveragents/infrastructure/sandbox/manager.py @@ -28,12 +28,17 @@ from cleveragents.infrastructure.sandbox.factory import ( SandboxFactory, SandboxStrategyStr, ) +from cleveragents.infrastructure.sandbox.no_sandbox import NoSandbox from cleveragents.infrastructure.sandbox.protocol import ( + AtomicCommitError, CommitResult, Sandbox, SandboxError, SandboxStatus, ) +from cleveragents.infrastructure.sandbox.transaction_sandbox import ( + TransactionSandbox, +) logger = logging.getLogger(__name__) @@ -200,19 +205,53 @@ class SandboxManager: # -- batch operations ---------------------------------------------------- def commit_all(self, plan_id: str) -> list[CommitResult]: - """Commit all active sandboxes for a plan. + """Atomically commit all active sandboxes for a plan. - Each sandbox with status ``ACTIVE`` is committed individually. - If a commit fails, the error is captured in the result and - processing continues with the remaining sandboxes (partial - commit is allowed -- the caller decides what to do). + The Apply phase is an atomic operation — either all sandbox + changes are committed successfully, or none are. If any single + sandbox commit fails, all previously-committed sandboxes in the + same batch are rolled back. + + .. note:: Specification contradiction + + Line 45938 requires atomicity ("all or none"), while line + 19193 states "partial apply" is possible. This method + implements the atomicity requirement from line 45938 per + issue #925. A spec update for line 19193 may be needed. Args: plan_id: Plan identifier. Returns: - List of :class:`CommitResult` for each sandbox that was - committed or attempted. + On success, a list of :class:`CommitResult` (one per committed + sandbox, all with ``success=True``). + + On failure, a single-element list containing a + :class:`CommitResult` with ``success=False`` whose ``error`` + field describes which sandbox failed and which sandboxes + were rolled back. The ``metadata`` dict includes a + ``"rolled_back"`` key listing the IDs of rolled-back + sandboxes. + + Raises: + ValueError: If *plan_id* is empty. + AtomicCommitError: If a sandbox ``commit()`` raises a non- + :class:`SandboxError` exception. The atomic rollback is + performed and the original exception is wrapped in an + :class:`AtomicCommitError` (chained as ``__cause__``) + that carries ``rolled_back_ids`` and + ``failed_rollback_ids`` attributes. The caller receives + the :class:`AtomicCommitError`, not the raw exception. + + .. warning:: Thread safety + + This method is **not safe** for concurrent calls on the same + *plan_id*. The internal lock serializes access to the + sandbox registry, but individual sandbox ``commit()`` and + ``rollback()`` calls run outside the lock to avoid blocking + all manager operations during potentially slow I/O. + Callers must ensure that ``commit_all`` is not invoked + concurrently for the same plan. """ if not plan_id: raise ValueError("plan_id cannot be empty") @@ -220,31 +259,166 @@ class SandboxManager: with self._lock: sandboxes = list(self._active_sandboxes.get(plan_id, {}).values()) - results: list[CommitResult] = [] - for sandbox in sandboxes: - if sandbox.status not in (SandboxStatus.CREATED, SandboxStatus.ACTIVE): - continue + committable = [ + sb + for sb in sandboxes + if sb.status in (SandboxStatus.CREATED, SandboxStatus.ACTIVE) + ] + if not committable: + return [] + + # Warn about sandboxes that cannot be rolled back (e.g. NoSandbox, + # TransactionSandbox after COMMIT). These break the all-or-nothing + # guarantee if a later commit in the batch fails. + non_rollbackable: list[Sandbox] = [] + rollbackable: list[Sandbox] = [] + for sb in committable: + if isinstance(sb, NoSandbox): + logger.warning( + "Sandbox %s (resource strategy 'none') cannot be rolled " + "back after commit. Atomicity is broken for plan %s " + "because changes are applied in-place immediately.", + sb.sandbox_id, + plan_id, + ) + non_rollbackable.append(sb) + elif isinstance(sb, TransactionSandbox): + logger.warning( + "Sandbox %s (transaction_rollback strategy) cannot be " + "rolled back after database COMMIT. Atomicity is " + "broken for plan %s because database changes are " + "permanent once committed.", + sb.sandbox_id, + plan_id, + ) + non_rollbackable.append(sb) + else: + rollbackable.append(sb) + + # Commit rollbackable sandboxes first so that if any fail, we + # can undo them. Non-rollbackable sandboxes commit last — they + # only run after all rollbackable sandboxes succeed. + ordered = rollbackable + non_rollbackable + + committed: list[tuple[Sandbox, CommitResult]] = [] + + for sandbox in ordered: try: result = sandbox.commit() - results.append(result) - except SandboxError as exc: - logger.error( - "Failed to commit sandbox %s for plan %s: %s", - sandbox.sandbox_id, - plan_id, - exc, - ) - results.append( - CommitResult( - sandbox_id=sandbox.sandbox_id, - success=False, - error=str(exc), - timestamp=datetime.now(), - ) + committed.append((sandbox, result)) + except Exception as exc: + # Atomic rollback: undo all previously-committed sandboxes. + # Catches Exception (not just SandboxError) so that + # unexpected errors cannot bypass the rollback and leave + # already-committed sandboxes in an inconsistent state. + failed_id = sandbox.sandbox_id + rolled_back_ids, failed_rollback_ids = self._rollback_committed( + committed, plan_id ) - return results + error_msg = f"Atomic commit failed at sandbox {failed_id}: {exc}" + if rolled_back_ids: + error_msg += f"; rolled back sandboxes: {rolled_back_ids}" + if failed_rollback_ids: + error_msg += ( + f"; FAILED to roll back sandboxes: {failed_rollback_ids}" + ) + + logger.error( + "Atomic commit_all failed for plan %s: %s", + plan_id, + error_msg, + ) + + # For non-SandboxError exceptions, wrap in + # AtomicCommitError carrying rollback metadata so the + # caller can determine which sandboxes were rolled back. + # The original exception is chained as __cause__. + if not isinstance(exc, SandboxError): + raise AtomicCommitError( + error_msg, + rolled_back_ids=rolled_back_ids, + failed_rollback_ids=failed_rollback_ids, + ) from exc + + return [ + CommitResult( + sandbox_id=failed_id, + success=False, + error=error_msg, + timestamp=datetime.now(), + metadata={ + "rolled_back": rolled_back_ids, + "rollback_failed": failed_rollback_ids, + }, + ) + ] + + return [result for _, result in committed] + + def _rollback_committed( + self, + committed: list[tuple[Sandbox, CommitResult]], + plan_id: str, + ) -> tuple[list[str], list[str]]: + """Roll back sandboxes that were already committed in this batch. + + Called when an atomic ``commit_all`` encounters a failure + partway through. Each previously-committed sandbox is rolled + back to undo its changes. Errors during individual rollbacks + are logged but do not prevent other rollbacks from being + attempted. + + Rollback proceeds in **LIFO** (reverse-of-commit) order, + following the standard transaction-log undo pattern: the most + recently committed sandbox is rolled back first. + + .. note:: + + The specification's "top-down" rollback ordering (line 24632) + refers to resource **DAG traversal** (parent before child) + within a single sandbox domain. LIFO ordering here applies + to the *batch undo* of independently committed sandboxes, + which is a different context. + + Args: + committed: List of ``(sandbox, commit_result)`` tuples for + sandboxes that committed successfully before the failure. + plan_id: Plan identifier (used for log messages). + + Returns: + A tuple of ``(rolled_back_ids, failed_rollback_ids)`` where + *rolled_back_ids* lists sandboxes that were successfully + rolled back and *failed_rollback_ids* lists sandboxes where + the rollback raised an exception. + """ + rolled_back_ids: list[str] = [] + failed_rollback_ids: list[str] = [] + + for sandbox, _result in reversed(committed): + try: + sandbox.rollback() + rolled_back_ids.append(sandbox.sandbox_id) + logger.info( + "Rolled back sandbox %s for plan %s during atomic commit recovery", + sandbox.sandbox_id, + plan_id, + ) + except Exception as rb_exc: + # Catches Exception (not just SandboxError) so that + # unexpected rollback errors do not prevent remaining + # rollbacks from being attempted. + failed_rollback_ids.append(sandbox.sandbox_id) + logger.error( + "Failed to rollback sandbox %s for plan %s " + "during atomic commit recovery: %s", + sandbox.sandbox_id, + plan_id, + rb_exc, + ) + + return rolled_back_ids, failed_rollback_ids def rollback_all(self, plan_id: str) -> None: """Roll back all active sandboxes for a plan. @@ -262,12 +436,16 @@ class SandboxManager: sandboxes = list(self._active_sandboxes.get(plan_id, {}).values()) for sandbox in sandboxes: - if sandbox.status != SandboxStatus.ACTIVE: + if sandbox.status not in (SandboxStatus.ACTIVE, SandboxStatus.COMMITTED): continue try: sandbox.rollback() - except SandboxError as exc: + except Exception as exc: + # Catches Exception (not just SandboxError) so that + # unexpected rollback errors do not prevent remaining + # sandboxes from being rolled back, consistent with + # _rollback_committed(). logger.error( "Failed to rollback sandbox %s for plan %s: %s", sandbox.sandbox_id, @@ -290,7 +468,12 @@ class SandboxManager: for sandbox in sandboxes: try: sandbox.cleanup() - except SandboxError as exc: + except Exception as exc: + # Catches Exception (not just SandboxError) so that + # unexpected errors (e.g. raw OSError, PermissionError) + # do not abort the loop and prevent remaining sandboxes + # from being cleaned up, consistent with + # _rollback_committed() and rollback_all(). logger.error( "Failed to cleanup sandbox %s for plan %s: %s", sandbox.sandbox_id, @@ -329,7 +512,13 @@ class SandboxManager: try: sandbox.cleanup() cleaned += 1 - except SandboxError as exc: + except Exception as exc: + # Catches Exception (not just SandboxError) so + # that unexpected errors (e.g. raw OSError, + # PermissionError) do not crash the loop and + # prevent remaining abandoned sandboxes from + # being cleaned up, consistent with cleanup_all, + # rollback_all, and _rollback_committed. logger.error( "Failed to cleanup abandoned sandbox %s: %s", sandbox.sandbox_id, diff --git a/src/cleveragents/infrastructure/sandbox/overlay.py b/src/cleveragents/infrastructure/sandbox/overlay.py index 31fe869ed..36a321121 100644 --- a/src/cleveragents/infrastructure/sandbox/overlay.py +++ b/src/cleveragents/infrastructure/sandbox/overlay.py @@ -6,7 +6,6 @@ discards and recreates. For fs-mount, fs-directory, fs-file resources. from __future__ import annotations -import filecmp import functools import logging import os @@ -17,6 +16,11 @@ from datetime import datetime from ulid import ULID +from cleveragents.infrastructure.sandbox._fs_utils import ( + backup_directory, + compute_diff, + safe_restore, +) from cleveragents.infrastructure.sandbox.protocol import ( CommitResult, SandboxCommitError, @@ -116,6 +120,8 @@ class OverlaySandbox: self._upper_dir: str | None = None self._work_dir: str | None = None self._merged_dir: str | None = None + # Pre-commit backup for atomic rollback support + self._pre_commit_backup: str | None = None self._use_real_overlay: bool = _is_overlayfs_available() @@ -241,7 +247,7 @@ class OverlaySandbox: if ".." in resource_path.split("/"): raise ValueError(f"Path traversal not allowed: {resource_path}") - if self._status == SandboxStatus.CREATED: + if self._status in (SandboxStatus.CREATED, SandboxStatus.ROLLED_BACK): self._status = SandboxStatus.ACTIVE if self._merged_dir is None: @@ -277,10 +283,34 @@ class OverlaySandbox: raise SandboxStateError("Sandbox merged directory not set") try: - changed_files, added_files, deleted_files = self._compute_diff( + changed_files, added_files, deleted_files = compute_diff( self._merged_dir, self._original_path ) + # Save pre-commit backup for atomic rollback support. + # Skip when there are no changes to avoid copying a potentially + # large directory tree unnecessarily. + # + # IMPORTANT: ``_pre_commit_backup`` is assigned only AFTER + # ``backup_directory`` succeeds. If the backup itself fails + # (e.g. disk full), the original directory is still intact and + # the ``except OSError`` handler must NOT attempt + # ``safe_restore`` with a partial backup. + if changed_files or added_files or deleted_files: + # Create backup on the same filesystem as the original to + # avoid cross-device copy overhead and ensure os.rename + # compatibility in safe_restore(). + backup_dir = tempfile.mkdtemp( + prefix="ca-overlay-backup-", + dir=os.path.dirname(self._original_path), + ) + try: + backup_directory(self._original_path, backup_dir) + except Exception: + shutil.rmtree(backup_dir, ignore_errors=True) + raise + self._pre_commit_backup = backup_dir + # Apply changes: copy modified and new files for rel_path in changed_files + added_files: src = os.path.join(self._merged_dir, rel_path) @@ -296,7 +326,27 @@ class OverlaySandbox: if os.path.exists(dst): os.remove(dst) - except OSError as exc: + except Exception as exc: + # Attempt to restore the original from the pre-commit backup + # so that a partial copy does not leave the original corrupted. + # Catches Exception (not just OSError) so that unexpected errors + # during the file-copy phase (e.g. MemoryError wrapping, + # TypeError) also trigger the restore. Without this, the + # backup would exist but never be applied, leaving the original + # directory partially modified. + # If the restore itself fails, keep the backup for manual + # recovery — cleanup() will remove it later. + if self._pre_commit_backup is not None: + try: + safe_restore(self._pre_commit_backup, self._original_path) + self._pre_commit_backup = None + except Exception: + logger.warning( + "Failed to restore original from backup for sandbox %s; " + "backup preserved at %s for manual recovery", + self._sandbox_id, + self._pre_commit_backup, + ) self._status = SandboxStatus.ERRORED raise SandboxCommitError( f"Failed to commit overlay sandbox {self._sandbox_id}: {exc}" @@ -331,6 +381,11 @@ class OverlaySandbox: def rollback(self) -> None: """Discard overlay changes by resetting the merged view. + When rolling back from ``ACTIVE``: resets the merged view. + When rolling back from ``COMMITTED``: restores the original + directory from the pre-commit backup so that the commit's + effects are fully undone. + For real OverlayFS: unmounts, recreates upper/work, remounts. For userspace fallback: deletes merged, re-copies original. @@ -338,7 +393,7 @@ class OverlaySandbox: SandboxRollbackError: If the rollback fails. SandboxStateError: If called in an invalid status. """ - if self._status != SandboxStatus.ACTIVE: + if self._status not in (SandboxStatus.ACTIVE, SandboxStatus.COMMITTED): raise SandboxStateError(f"Cannot rollback from status {self._status.value}") SandboxStatus.assert_transition(self._status, SandboxStatus.ROLLED_BACK) @@ -347,7 +402,72 @@ class OverlaySandbox: raise SandboxStateError("Sandbox merged directory not set") try: - if self._use_real_overlay: + if self._status == SandboxStatus.COMMITTED: + if self._pre_commit_backup is None: + # No pre-commit backup means commit() detected no + # changes and skipped the backup. The original + # directory is unmodified, so rollback is a no-op. + # The merged directory is also unchanged, so no + # reset is needed. + logger.debug( + "Rollback from COMMITTED with no backup (no changes " + "were applied) for sandbox %s — no-op", + self._sandbox_id, + ) + else: + # Atomically restore original from pre-commit backup. + # Uses rename-based swap so that a failure during copy + # does not leave the original directory deleted. + safe_restore(self._pre_commit_backup, self._original_path) + self._pre_commit_backup = None + + # Reset the merged directory so that a subsequent + # get_path() (ROLLED_BACK → ACTIVE) does not expose + # stale pre-rollback content. Only needed when + # changes were actually restored (backup existed). + if self._merged_dir is not None: + if self._use_real_overlay: + # For real OverlayFS: unmount, clean upper/work + # dirs, and remount so the overlay is correctly + # re-established for potential re-activation. + # If unmount fails, bail out — attempting to + # mount on top of a still-mounted FS would cause + # a double-mount or mount error. + try: + subprocess.run( + ["umount", str(self._merged_dir)], + capture_output=True, + check=True, + timeout=30, + ) + except ( + subprocess.CalledProcessError, + subprocess.TimeoutExpired, + OSError, + ) as umount_exc: + raise SandboxRollbackError( + f"Cannot remount overlay for sandbox " + f"{self._sandbox_id}: unmount failed: " + f"{umount_exc}" + ) from umount_exc + if self._upper_dir is not None: + shutil.rmtree(self._upper_dir, ignore_errors=True) + os.makedirs(self._upper_dir, exist_ok=True) + if self._work_dir is not None: + shutil.rmtree(self._work_dir, ignore_errors=True) + os.makedirs(self._work_dir, exist_ok=True) + self._mount_overlay() + else: + # Userspace fallback: delete merged, re-copy from + # the (now-restored) original. + shutil.rmtree(self._merged_dir, ignore_errors=True) + shutil.copytree( + self._original_path, + self._merged_dir, + symlinks=False, + dirs_exist_ok=True, + ) + elif self._use_real_overlay: self._unmount_overlay() # Reset upper and work directories if self._upper_dir is not None: @@ -358,16 +478,26 @@ class OverlaySandbox: os.makedirs(self._work_dir, exist_ok=True) self._mount_overlay() else: - # Userspace fallback: delete merged, re-copy + # Userspace fallback: delete merged, re-copy. + # Use dirs_exist_ok=True because rmtree with + # ignore_errors=True may silently leave the directory + # partially intact (e.g. permission error on a file), + # which would cause copytree to raise FileExistsError + # with dirs_exist_ok=False. shutil.rmtree(self._merged_dir, ignore_errors=True) shutil.copytree( self._original_path, self._merged_dir, symlinks=False, - dirs_exist_ok=False, + dirs_exist_ok=True, ) - except OSError as exc: + except SandboxRollbackError: + # Already a SandboxRollbackError (e.g. from unmount failure + # during COMMITTED rollback) — re-raise without wrapping. + self._status = SandboxStatus.ERRORED + raise + except Exception as exc: self._status = SandboxStatus.ERRORED raise SandboxRollbackError( f"Failed to rollback overlay sandbox {self._sandbox_id}: {exc}" @@ -404,6 +534,12 @@ class OverlaySandbox: if self._base_dir is not None and os.path.exists(self._base_dir): shutil.rmtree(self._base_dir, ignore_errors=True) + # Clean up any pre-commit backup + if self._pre_commit_backup is not None: + if os.path.exists(self._pre_commit_backup): + shutil.rmtree(self._pre_commit_backup, ignore_errors=True) + self._pre_commit_backup = None + self._status = SandboxStatus.CLEANED_UP logger.info( @@ -416,8 +552,23 @@ class OverlaySandbox: """Mount an OverlayFS union at the merged directory. Raises: - SandboxCreationError: If the mount command fails. + SandboxCreationError: If the mount command fails or if any + overlay path contains a comma (which would corrupt the + mount options string). """ + # OverlayFS mount options are comma-separated key=value pairs. + # A comma inside any path value would be mis-parsed by the + # kernel's filesystem driver, corrupting the mount options. + for label, path in ( + ("lowerdir", self._original_path), + ("upperdir", self._upper_dir), + ("workdir", self._work_dir), + ): + if path and "," in str(path): + raise SandboxCreationError( + f"OverlayFS {label} path contains a comma, which " + f"would corrupt mount options: {path}" + ) opts = ( f"lowerdir={self._original_path}," f"upperdir={self._upper_dir}," @@ -438,7 +589,11 @@ class OverlaySandbox: check=True, timeout=30, ) - except (subprocess.CalledProcessError, OSError) as exc: + except ( + subprocess.CalledProcessError, + subprocess.TimeoutExpired, + OSError, + ) as exc: raise SandboxCreationError(f"Failed to mount OverlayFS: {exc}") from exc def _unmount_overlay(self) -> None: @@ -452,47 +607,13 @@ class OverlaySandbox: check=True, timeout=30, ) - except (subprocess.CalledProcessError, OSError) as exc: + except ( + subprocess.CalledProcessError, + subprocess.TimeoutExpired, + OSError, + ) as exc: logger.warning( "Failed to unmount overlay at %s: %s", self._merged_dir, exc, ) - - @staticmethod - def _compute_diff( - sandbox_dir: str, original_dir: str - ) -> tuple[list[str], list[str], list[str]]: - """Compare sandbox to original and return changed/added/deleted. - - Args: - sandbox_dir: Path to the merged/sandbox directory. - original_dir: Path to the original directory. - - Returns: - Tuple of ``(changed, added, deleted)`` relative-path lists. - """ - # Collect all files in both trees - sandbox_files: set[str] = { - os.path.relpath(os.path.join(dp, f), sandbox_dir) - for dp, _, fnames in os.walk(sandbox_dir) - for f in fnames - } - original_files: set[str] = { - os.path.relpath(os.path.join(dp, f), original_dir) - for dp, _, fnames in os.walk(original_dir) - for f in fnames - } - - added = sorted(sandbox_files - original_files) - deleted = sorted(original_files - sandbox_files) - changed = [ - rel - for rel in sorted(sandbox_files & original_files) - if not filecmp.cmp( - os.path.join(sandbox_dir, rel), - os.path.join(original_dir, rel), - shallow=False, - ) - ] - return changed, added, deleted diff --git a/src/cleveragents/infrastructure/sandbox/protocol.py b/src/cleveragents/infrastructure/sandbox/protocol.py index 83337f369..e97559ad1 100644 --- a/src/cleveragents/infrastructure/sandbox/protocol.py +++ b/src/cleveragents/infrastructure/sandbox/protocol.py @@ -39,6 +39,30 @@ class SandboxStateError(SandboxError): """Raised when an operation is invalid for the current sandbox status.""" +class AtomicCommitError(SandboxError): + """Raised when an atomic ``commit_all`` fails due to a non-sandbox exception. + + Wraps the original exception as ``__cause__`` and carries rollback + metadata so the caller can determine which sandboxes were rolled + back and which rollbacks failed. + + Attributes: + rolled_back_ids: Sandbox IDs that were successfully rolled back. + failed_rollback_ids: Sandbox IDs where rollback failed. + """ + + def __init__( + self, + message: str, + *, + rolled_back_ids: list[str] | None = None, + failed_rollback_ids: list[str] | None = None, + ) -> None: + super().__init__(message) + self.rolled_back_ids: list[str] = rolled_back_ids or [] + self.failed_rollback_ids: list[str] = failed_rollback_ids or [] + + # --------------------------------------------------------------------------- # SandboxStatus enum (B3.2) # --------------------------------------------------------------------------- @@ -49,13 +73,20 @@ class SandboxStatus(StrEnum): Transition graph:: - PENDING ──► CREATED ──► ACTIVE ──► COMMITTED ──► CLEANED_UP - │ │ │ │ - │ ├──► COMMITTED ├──► CLEANED_UP - │ │ │ - │ └──► CLEANED_UP ├──► ROLLED_BACK ──► ACTIVE - │ │ - └──► ERRORED ──► CLEANED_UP └──► CLEANED_UP + PENDING ──► CREATED ──► ACTIVE ──► COMMITTED ──┬──► CLEANED_UP + │ │ │ │ + │ ├──► COMMITTED └──► ROLLED_BACK ──┐ + │ │ │ + │ └──► CLEANED_UP ACTIVE ◄──── ROLLED_BACK ◄──┘ │ + │ │ │ + │ ├──► ERRORED ──► CLEANED_UP │ + │ │ │ + └──► ERRORED ──► CLEANED_UP └────────────► CLEANED_UP ◄──┘ + + The ``COMMITTED → ROLLED_BACK`` transition supports the atomic + ``commit_all`` protocol: if a later sandbox in the batch fails to + commit, already-committed sandboxes are rolled back to their + pre-commit state. Terminal state: ``CLEANED_UP``. """ @@ -82,7 +113,7 @@ class SandboxStatus(StrEnum): cls.PENDING: [cls.CREATED, cls.ERRORED], cls.CREATED: [cls.ACTIVE, cls.COMMITTED, cls.CLEANED_UP], cls.ACTIVE: [cls.COMMITTED, cls.ROLLED_BACK, cls.ERRORED], - cls.COMMITTED: [cls.CLEANED_UP], + cls.COMMITTED: [cls.ROLLED_BACK, cls.CLEANED_UP], cls.ROLLED_BACK: [cls.ACTIVE, cls.CLEANED_UP], cls.ERRORED: [cls.CLEANED_UP], cls.CLEANED_UP: [], diff --git a/src/cleveragents/infrastructure/sandbox/transaction_sandbox.py b/src/cleveragents/infrastructure/sandbox/transaction_sandbox.py index 47081391a..6841daee6 100644 --- a/src/cleveragents/infrastructure/sandbox/transaction_sandbox.py +++ b/src/cleveragents/infrastructure/sandbox/transaction_sandbox.py @@ -262,16 +262,35 @@ class TransactionSandbox: def rollback(self) -> None: """Roll back the database transaction. - Issues ``ROLLBACK`` on the underlying connection and begins a - new transaction so the sandbox can be re-used. + When rolling back from ``ACTIVE``, issues ``ROLLBACK`` on the + underlying connection and begins a new transaction so the sandbox + can be re-used. + + When rolling back from ``COMMITTED``, the database ``COMMIT`` + has already been issued and cannot be undone. Raises + :class:`SandboxRollbackError` so that the manager's atomic + commit protocol correctly reports this sandbox in the + ``failed_rollback_ids`` list rather than silently claiming + success. Raises: - SandboxRollbackError: If the ROLLBACK fails. + SandboxRollbackError: If the ROLLBACK fails or if the + sandbox is in ``COMMITTED`` state (irreversible). SandboxStateError: If called in an invalid status. """ - if self._status != SandboxStatus.ACTIVE: + if self._status not in (SandboxStatus.ACTIVE, SandboxStatus.COMMITTED): raise SandboxStateError(f"Cannot rollback from status {self._status.value}") + if self._status == SandboxStatus.COMMITTED: + # Database COMMIT cannot be reversed. Raise so the + # manager's atomic commit protocol correctly reports this + # sandbox as NOT rolled back rather than silently claiming + # the rollback succeeded. + raise SandboxRollbackError( + f"Cannot undo database COMMIT for sandbox {self._sandbox_id} " + f"— transaction changes are permanent" + ) + SandboxStatus.assert_transition(self._status, SandboxStatus.ROLLED_BACK) try: -- 2.52.0