UAT: `SandboxManager.commit_all()` atomicity guarantee broken for `transaction_rollback` and `none` sandbox strategies — spec requires all-or-nothing Apply #4442

New issue

Open

opened 2026-04-08 12:41:23 +00:00 by HAL9000 · 0 comments

HAL9000 commented

2026-04-08 12:41:23 +00:00

Owner

Metadata

Milestone: (none — backlog)
Parent Epic: (sandbox and checkpoint safety)

Bug Report

What Was Tested

The SandboxManager.commit_all() method was analyzed against the spec's sandbox isolation requirements.

Expected Behavior (from spec)

Per docs/specification.md (line ~46204):

Atomic apply: The Apply phase is an atomic operation — either all sandbox changes are committed to the real resources, or none are. For git-based resources, this uses git merge. For database resources, this uses transaction commit.

The spec also defines sandbox strategies:

git_worktree: Git worktree isolation — supports rollback
transaction_rollback: Database transaction isolation — commit on apply; rollback on cancel/error
none: No isolation — requires require_sandbox: false

Actual Behavior

In src/cleveragents/infrastructure/sandbox/manager.py, the commit_all() method explicitly warns that atomicity is broken for two strategies:

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)

And for NoSandbox:

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)

The spec says transaction_rollback should support rollback on cancel/error. The implementation acknowledges that TransactionSandbox cannot be rolled back after a database COMMIT, which means:

If a plan has both a git_worktree sandbox (for code) and a transaction_rollback sandbox (for database), and the git commit succeeds but the database commit fails, the git changes cannot be rolled back — the atomicity guarantee is violated.
If the database commit succeeds but a subsequent git commit fails, the database changes are permanent and cannot be rolled back.

Code Locations

SandboxManager.commit_all(): src/cleveragents/infrastructure/sandbox/manager.py (lines ~200-290)
TransactionSandbox: src/cleveragents/infrastructure/sandbox/transaction_sandbox.py
NoSandbox: src/cleveragents/infrastructure/sandbox/no_sandbox.py

Impact

This affects plans that use database resources with transaction_rollback strategy alongside file resources with git_worktree strategy. The spec's atomicity guarantee cannot be upheld in these mixed-strategy scenarios.

Possible Fix

Options:

Two-phase commit protocol: Use a prepare/commit pattern where all sandboxes prepare first, then all commit. If any prepare fails, abort all.
Ordering constraint: Always commit transaction_rollback sandboxes last, so that if they fail, git sandboxes can be rolled back.
Documentation: If true atomicity is not achievable, document this limitation clearly in the spec and surface it as a warning to users when mixed strategies are used.

The code already implements option 2 partially (rollbackable sandboxes commit first), but the warning message acknowledges this doesn't fully solve the problem.

Automated by CleverAgents Bot
Supervisor: UAT Testing | Agent: uat-tester

## Metadata - **Milestone**: *(none — backlog)* - **Parent Epic**: *(sandbox and checkpoint safety)* ## Bug Report ### What Was Tested The `SandboxManager.commit_all()` method was analyzed against the spec's sandbox isolation requirements. ### Expected Behavior (from spec) Per `docs/specification.md` (line ~46204): > **Atomic apply**: The Apply phase is an atomic operation — either all sandbox changes are committed to the real resources, or none are. For git-based resources, this uses git merge. For database resources, this uses transaction commit. The spec also defines sandbox strategies: - `git_worktree`: Git worktree isolation — supports rollback - `transaction_rollback`: Database transaction isolation — commit on apply; **rollback on cancel/error** - `none`: No isolation — requires `require_sandbox: false` ### Actual Behavior In `src/cleveragents/infrastructure/sandbox/manager.py`, the `commit_all()` method explicitly warns that atomicity is broken for two strategies: ```python 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) ``` And for `NoSandbox`: ```python 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) ``` **The spec says `transaction_rollback` should support rollback on cancel/error.** The implementation acknowledges that `TransactionSandbox` cannot be rolled back after a database COMMIT, which means: 1. If a plan has both a `git_worktree` sandbox (for code) and a `transaction_rollback` sandbox (for database), and the git commit succeeds but the database commit fails, the git changes cannot be rolled back — the atomicity guarantee is violated. 2. If the database commit succeeds but a subsequent git commit fails, the database changes are permanent and cannot be rolled back. ### Code Locations - `SandboxManager.commit_all()`: `src/cleveragents/infrastructure/sandbox/manager.py` (lines ~200-290) - `TransactionSandbox`: `src/cleveragents/infrastructure/sandbox/transaction_sandbox.py` - `NoSandbox`: `src/cleveragents/infrastructure/sandbox/no_sandbox.py` ### Impact This affects plans that use database resources with `transaction_rollback` strategy alongside file resources with `git_worktree` strategy. The spec's atomicity guarantee cannot be upheld in these mixed-strategy scenarios. ### Possible Fix Options: 1. **Two-phase commit protocol**: Use a prepare/commit pattern where all sandboxes prepare first, then all commit. If any prepare fails, abort all. 2. **Ordering constraint**: Always commit `transaction_rollback` sandboxes last, so that if they fail, git sandboxes can be rolled back. 3. **Documentation**: If true atomicity is not achievable, document this limitation clearly in the spec and surface it as a warning to users when mixed strategies are used. The code already implements option 2 partially (rollbackable sandboxes commit first), but the warning message acknowledges this doesn't fully solve the problem. --- **Automated by CleverAgents Bot** Supervisor: UAT Testing | Agent: uat-tester

HAL9000 added this to the v3.5.0 milestone

2026-04-08 17:42:26 +00:00

HAL9000 added the