cleveragents/cleveragents-core
4
0
Fork 3
Code Issues 6.0k Pull Requests 115 Actions Packages Projects Releases Wiki Activity

fix(sandbox): make commit_all atomic per specification #1146

Merged
CoreRasurae merged 1 commits from fix/atomic-sandbox-commit into master 2026-03-29 18:13:20 +00:00
Conversation 15 Commits 1 Files Changed 23
+1905 -174
23 changed files with 1905 additions and 174 deletions
Download Patch File Download Diff File Expand all files Collapse all files
CHANGELOG.md
+72
View File
@@ -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
features/consolidated_sandbox.feature
+21
View File
@@ -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 ---
features/git_worktree_sandbox.feature
+10
View File
@@ -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
features/overlay_sandbox.feature
+22
View File
@@ -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
features/sandbox_fs_utils.feature
+53
View File
@@ -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
features/sandbox_manager_coverage.feature
+84 -2
View File
@@ -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
features/steps/git_worktree_sandbox_steps.py
+8
View File
@@ -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:
features/steps/overlay_sandbox_steps.py
+43
View File
@@ -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"
)
features/steps/sandbox_copy_on_write_coverage_steps.py
+3 -4
View File
@@ -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()
features/steps/sandbox_fs_utils_steps.py
+291
View File
@@ -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}"
)
features/steps/sandbox_manager_coverage_steps.py
+324
View File
@@ -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, (
features/steps/transaction_sandbox_coverage_steps.py
+44
View File
@@ -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)
# ---------------------------------------------------------------------------
features/transaction_sandbox_coverage.feature
+9
View File
@@ -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
robot/helper_sandbox_integration.py
+70
View File
@@ -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}")
robot/sandbox_integration.robot
+7
View File
@@ -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
src/cleveragents/infrastructure/sandbox/__init__.py
+2
View File
@@ -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",
src/cleveragents/infrastructure/sandbox/_fs_utils.py
+237
View File
@@ -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))
src/cleveragents/infrastructure/sandbox/copy_on_write.py
+106 -71
View File
@@ -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
src/cleveragents/infrastructure/sandbox/git_worktree.py
+46 -4
View File
@@ -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,
src/cleveragents/infrastructure/sandbox/manager.py
+220 -31
View File
@@ -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,
src/cleveragents/infrastructure/sandbox/overlay.py
+171 -50
View File
@@ -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
src/cleveragents/infrastructure/sandbox/protocol.py
+39 -8
View File
@@ -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: [],
src/cleveragents/infrastructure/sandbox/transaction_sandbox.py
+23 -4
View File
@@ -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: