Skip to content

Do not throw exception when trying to delete a failed backup - #2458

Merged
Boy132 merged 1 commit into
mainfrom
boy132/fix-failed-backup-delete
Jul 17, 2026
Merged

Do not throw exception when trying to delete a failed backup#2458
Boy132 merged 1 commit into
mainfrom
boy132/fix-failed-backup-delete

Conversation

@Boy132

@Boy132 Boy132 commented Jul 16, 2026

Copy link
Copy Markdown
Member

Closes #2457

@Boy132 Boy132 self-assigned this Jul 16, 2026
@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

DeleteBackupService::handle() now catches adapter deletion exceptions within the transaction. Successful backups still rethrow failures, while unsuccessful backups proceed with local record deletion.

Changes

Backup deletion handling

Layer / File(s) Summary
Conditional adapter deletion
app/Services/Backups/DeleteBackupService.php
Adapter deletion failures are caught and rethrown only for successful backups; failed backup records can be deleted even when adapter cleanup fails.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main behavior change for failed backup deletion.
Description check ✅ Passed The description is brief but directly related to the linked issue and code change.
Linked Issues check ✅ Passed The change lets failed backups be deleted without remote delete errors blocking local record removal, matching #2457.
Out of Scope Changes check ✅ Passed No unrelated or out-of-scope changes are indicated beyond the backup deletion fix.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@app/Services/Backups/DeleteBackupService.php`:
- Around line 38-42: Update the exception handling in DeleteBackupService around
deleteBackup so adapter failures are only swallowed for backups that never
started uploading. Require the backup to lack upload metadata, such as
upload_id, before allowing the row deletion fallback; otherwise rethrow the
exception to preserve cleanup or retry handling.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 0afeed2f-737a-46a1-ac5c-90559e754fb8

📥 Commits

Reviewing files that changed from the base of the PR and between 1f2ff09 and 57b4fc3.

📒 Files selected for processing (1)
  • app/Services/Backups/DeleteBackupService.php

Comment thread app/Services/Backups/DeleteBackupService.php
@Boy132
Boy132 merged commit b9f0eff into main Jul 17, 2026
16 checks passed
@Boy132
Boy132 deleted the boy132/fix-failed-backup-delete branch July 17, 2026 07:42
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 17, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Failed S3 backups cannot be deleted

2 participants