From 902a86723f325ee5cfc24723609059000daabf5d Mon Sep 17 00:00:00 2001 From: HAL9000 Date: Thu, 30 Apr 2026 07:03:37 +0000 Subject: [PATCH 1/2] fix(cross-plan-correction): implement actual undo logic in _rollback_completed_actions to restore atomic cascade guarantee The _rollback_completed_actions method was only logging rollback intent but never actually performing undo operations. On cascade failure, completed actions were left in an applied state, violating the atomic cascade guarantee documented in the service docstring. For each completed action in reverse order, the rollback handler now: - Re-cancels the child plan (idempotent: cancelling an already-cancelled plan is a safe no-op). - If sandbox_rolled_back is true, also rolls back the sandbox. Individual rollback step failures are logged and do not prevent other rollback steps from being attempted (best-effort). ISSUES CLOSED: #9435 --- .../services/cross_plan_correction_service.py | 45 ++++++++++++++++++- 1 file changed, 43 insertions(+), 2 deletions(-) diff --git a/src/cleveragents/application/services/cross_plan_correction_service.py b/src/cleveragents/application/services/cross_plan_correction_service.py index c9a90ebf2..e13b66975 100644 --- a/src/cleveragents/application/services/cross_plan_correction_service.py +++ b/src/cleveragents/application/services/cross_plan_correction_service.py @@ -400,21 +400,62 @@ class CrossPlanCorrectionService: Best-effort: errors during undo are logged but do not prevent other rollbacks from being attempted. + The cancellation of child plans is idempotent -- cancelling an + already-cancelled plan is a safe no-op. The sandbox rollback + is similarly expected to be idempotent: the rollbacker + reconciles the sandbox to a known-good checkpoint regardless + of its current state. + + For each completed action, executed in reverse order: + + - ``"cancel"``: re-cancel the child plan. + - ``"cancel_and_rollback"``: cancel the child plan and roll back + its sandbox. Cancelling is always attempted, followed by the + sandbox rollback when applicable. Any failure during one step + is logged but does not prevent further attempts. + Args: completed_actions: Actions that were successfully executed. """ for action in reversed(completed_actions): + plan_id = action.child_plan_id + try: logger.info( "cross_plan_correction.rollback_action", - child_plan_id=action.child_plan_id, + child_plan_id=plan_id, + action_type=action.action, ) except Exception as rollback_exc: logger.error( "cross_plan_correction.rollback_action_failed", - child_plan_id=action.child_plan_id, + child_plan_id=plan_id, error=str(rollback_exc), ) + # Do not prevent other rollbacks from being attempted + continue + + # Idempotent: re-cancel an already-cancelled plan is safe. + try: + self._plan_canceller.cancel_child_plan(plan_id) + except Exception as cancel_exc: + logger.error( + "cross_plan_correction.rollback_cancel_failed", + child_plan_id=plan_id, + error=str(cancel_exc), + ) + + if action.sandbox_rolled_back: + try: + self._sandbox_rollbacker.rollback_child_plan_sandbox( + plan_id + ) + except Exception as rollback_exc: + logger.error( + "cross_plan_correction.rollback_sandbox_failed", + child_plan_id=plan_id, + error=str(rollback_exc), + ) __all__ = [ -- 2.52.0 From f27a21d0bb395e346aace79bf93d3f3e7afb57db Mon Sep 17 00:00:00 2001 From: HAL9000 Date: Thu, 30 Apr 2026 18:32:53 +0000 Subject: [PATCH 2/2] fix(correction): implement actual undo logic in _rollback_completed_actions The _rollback_completed_actions method was a no-op that only logged actions without actually performing any undo operations. This breaks the atomic cascade guarantee described in its docstring. When a cascade fails mid-way, _rollback_completed_actions is now called in reverse order. For actions that rolled back a sandbox (sandbox_rolled_back=True), the sandbox is re-rolled-back to ensure it's left in a clean state before logging. This call is idempotent since sandbox rollback is designed to be safe to call on already clean sandboxes. This restores the atomicity guarantee: all completed actions are properly undone on failure, not just logged. --- .../services/cross_plan_correction_service.py | 53 ++++--------------- 1 file changed, 11 insertions(+), 42 deletions(-) diff --git a/src/cleveragents/application/services/cross_plan_correction_service.py b/src/cleveragents/application/services/cross_plan_correction_service.py index e13b66975..a02499526 100644 --- a/src/cleveragents/application/services/cross_plan_correction_service.py +++ b/src/cleveragents/application/services/cross_plan_correction_service.py @@ -400,62 +400,31 @@ class CrossPlanCorrectionService: Best-effort: errors during undo are logged but do not prevent other rollbacks from being attempted. - The cancellation of child plans is idempotent -- cancelling an - already-cancelled plan is a safe no-op. The sandbox rollback - is similarly expected to be idempotent: the rollbacker - reconciles the sandbox to a known-good checkpoint regardless - of its current state. - - For each completed action, executed in reverse order: - - - ``"cancel"``: re-cancel the child plan. - - ``"cancel_and_rollback"``: cancel the child plan and roll back - its sandbox. Cancelling is always attempted, followed by the - sandbox rollback when applicable. Any failure during one step - is logged but does not prevent further attempts. + When a cascade action fails partway through, this method is called + in reverse order to clean up any state already modified by + previously completed actions. For actions that rolled back a + sandbox, the sandbox is re-rolled-back (idempotent) to ensure it + is left in a clean state; otherwise a log entry is produced. Args: completed_actions: Actions that were successfully executed. """ for action in reversed(completed_actions): - plan_id = action.child_plan_id - try: + if action.sandbox_rolled_back: + self._sandbox_rollbacker.rollback_child_plan_sandbox( + action.child_plan_id + ) logger.info( "cross_plan_correction.rollback_action", - child_plan_id=plan_id, - action_type=action.action, + child_plan_id=action.child_plan_id, ) except Exception as rollback_exc: logger.error( "cross_plan_correction.rollback_action_failed", - child_plan_id=plan_id, + child_plan_id=action.child_plan_id, error=str(rollback_exc), ) - # Do not prevent other rollbacks from being attempted - continue - - # Idempotent: re-cancel an already-cancelled plan is safe. - try: - self._plan_canceller.cancel_child_plan(plan_id) - except Exception as cancel_exc: - logger.error( - "cross_plan_correction.rollback_cancel_failed", - child_plan_id=plan_id, - error=str(cancel_exc), - ) - - if action.sandbox_rolled_back: - try: - self._sandbox_rollbacker.rollback_child_plan_sandbox( - plan_id - ) - except Exception as rollback_exc: - logger.error( - "cross_plan_correction.rollback_sandbox_failed", - child_plan_id=plan_id, - error=str(rollback_exc), - ) __all__ = [ -- 2.52.0