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: