Add rollback restoration for failed deployments#794
Merged
Conversation
Reviewer's GuideThis PR enhances the DeploymentAutomator to support automatic rollback by tracking and restoring from the most recent backup when a deployment fails. Sequence diagram for deployment with automatic rollback restorationsequenceDiagram
participant User
participant DeploymentAutomator
User->>DeploymentAutomator: deploy(environment, mode)
DeploymentAutomator->>DeploymentAutomator: _create_backup()
DeploymentAutomator->>DeploymentAutomator: last_backup = backup_name
DeploymentAutomator->>DeploymentAutomator: deployment_plan["backup_name"] = last_backup
alt Deployment fails
DeploymentAutomator->>DeploymentAutomator: _rollback_deployment(deployment_plan)
alt last_backup exists
DeploymentAutomator->>DeploymentAutomator: deployment_plan["rollback_backup"] = last_backup
DeploymentAutomator->>DeploymentAutomator: _restore_backup(last_backup)
else No backup available
DeploymentAutomator->>DeploymentAutomator: print("No backup available for rollback")
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements automatic backup restoration for failed deployments by tracking the most recent backup and adding restoration capabilities to the rollback process.
- Tracks the most recent backup in the
DeploymentAutomatorclass instance - Adds a
_restore_backuphelper method to restore workspace from backups - Records the backup name in deployment plans and performs automatic restoration during rollbacks
| self.deployment_dir = self.workspace_root / "09-deployment" | ||
| self.config_dir = self.workspace_root / "10-config" | ||
| self.backup_dir = self.workspace_root / "backups" | ||
| self.last_backup: Optional[str] = None |
There was a problem hiding this comment.
The last_backup attribute lacks documentation. Consider adding a docstring or inline comment explaining its purpose and when it gets updated.
Suggested change
| self.last_backup: Optional[str] = None | |
| self.last_backup: Optional[str] = None # Stores the name of the most recent backup created during deployment. Updated by the `_create_backup` method. |
| src = backup_path / rel_path | ||
| dest = self.workspace_root / rel_path | ||
|
|
||
| if dest.exists(): |
There was a problem hiding this comment.
The restoration process removes existing files/directories without validation. This could lead to unintended data loss if the backup contains malicious paths or if there's a path traversal vulnerability. Consider validating that rel_path doesn't contain directory traversal sequences like '../'.
Comment on lines
+554
to
+556
| if src.is_file(): | ||
| shutil.copy2(src, dest) | ||
| else: |
There was a problem hiding this comment.
The code attempts to copy files without ensuring the parent directories exist. If the destination directory structure doesn't exist, shutil.copy2 will fail. Consider using dest.parent.mkdir(parents=True, exist_ok=True) before copying.
Suggested change
| if src.is_file(): | |
| shutil.copy2(src, dest) | |
| else: | |
| if src.is_file(): | |
| dest.parent.mkdir(parents=True, exist_ok=True) | |
| shutil.copy2(src, dest) | |
| else: | |
| dest.parent.mkdir(parents=True, exist_ok=True) |
Comment on lines
+571
to
+573
| if self.last_backup: | ||
| deployment_plan["rollback_backup"] = self.last_backup | ||
| self._restore_backup(self.last_backup) |
There was a problem hiding this comment.
The rollback logic uses self.last_backup but also checks for deployment_plan.get('backup_name') would be more reliable since it's explicitly tied to the current deployment. Consider using the backup name from the deployment plan instead of the instance variable.
Suggested change
| if self.last_backup: | |
| deployment_plan["rollback_backup"] = self.last_backup | |
| self._restore_backup(self.last_backup) | |
| backup_name = deployment_plan.get("backup_name") | |
| if backup_name: | |
| deployment_plan["rollback_backup"] = backup_name | |
| self._restore_backup(backup_name) |
There was a problem hiding this comment.
Hey @Bryan-Roe - I've reviewed your changes - here's some feedback:
- Use the backup_name stored in the deployment_plan to drive the restore in _rollback_deployment rather than relying on a transient self.last_backup attribute, so rollbacks remain consistent even if the automator instance is recreated.
- Wrap the file operations in _restore_backup with try/except and emit errors or abort on failure to avoid partial restores leaving the workspace in an inconsistent state.
- After setting deployment_plan["rollback_backup"], call _save_deployment_record to persist the rollback backup choice in the deployment history.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Use the backup_name stored in the deployment_plan to drive the restore in _rollback_deployment rather than relying on a transient self.last_backup attribute, so rollbacks remain consistent even if the automator instance is recreated.
- Wrap the file operations in _restore_backup with try/except and emit errors or abort on failure to avoid partial restores leaving the workspace in an inconsistent state.
- After setting deployment_plan["rollback_backup"], call _save_deployment_record to persist the rollback backup choice in the deployment history.
## Individual Comments
### Comment 1
<location> `02-ai-workspace/scripts/deployment_automator.py:528` </location>
<code_context>
+ def _restore_backup(self, backup_name: str):
</code_context>
<issue_to_address>
Restoration logic may overwrite files and directories without confirmation.
Unconditionally deleting files or directories risks data loss. Please add a prompt, skip option, or backup for overwritten files, or at minimum log replacements.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+528
to
+537
| def _restore_backup(self, backup_name: str): | ||
| """Restore workspace from a specific backup.""" | ||
| backup_path = self.backup_dir / backup_name | ||
| if not backup_path.exists(): | ||
| print(f"Backup not found: {backup_name}") | ||
| return | ||
|
|
||
| manifest_file = backup_path / "manifest.json" | ||
| if not manifest_file.exists(): | ||
| print("Backup manifest missing, cannot restore") |
There was a problem hiding this comment.
suggestion (bug_risk): Restoration logic may overwrite files and directories without confirmation.
Unconditionally deleting files or directories risks data loss. Please add a prompt, skip option, or backup for overwritten files, or at minimum log replacements.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
DeploymentAutomator_restore_backuphelper to copy backup contents back into the workspace_rollback_deploymentTesting
pytest test_direct_ai.py::test_ai_processing -s(fails: async framework plugin missing)https://chatgpt.com/codex/tasks/task_e_6886a75b21f08322b293e242277abf2c
Summary by Sourcery
Enable automated backup tracking and restore failed deployments by recording backups and applying them during rollback.
New Features: