Do not throw exception when trying to delete a failed backup

[Boy132]

Closes #2457

[coderabbitai]

📝 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

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

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n## Files\n'
git ls-files | rg 'app/Services/Backups/DeleteBackupService.php|app/Models/Backup.php|app/Services/Backups|app/Models'

printf '\n## Outline: DeleteBackupService\n'
ast-grep outline app/Services/Backups/DeleteBackupService.php --view expanded || true

printf '\n## Outline: Backup model\n'
ast-grep outline app/Models/Backup.php --view expanded || true

printf '\n## Relevant text search\n'
rg -n "is_successful|upload_id|deleteBackup|multipart|S3BackupSchema|successful|upload" app/Services/Backups app/Models -S

Repository: pelican-dev/panel

Length of output: 4782

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n## DeleteBackupService\n'
cat -n app/Services/Backups/DeleteBackupService.php

printf '\n## InitiateBackupService (relevant section)\n'
sed -n '1,180p' app/Services/Backups/InitiateBackupService.php | cat -n

printf '\n## Backup model (relevant section)\n'
sed -n '1,180p' app/Models/Backup.php | cat -n

printf '\n## Upload ID usage across repo\n'
rg -n "upload_id|multipart|completeMultipart|abortMultipart|is_successful|completed_at" app -S

printf '\n## Backup adapter/schema classes\n'
fd -a "BackupAdapterService.php|S3BackupSchema.php|*BackupSchema.php" app

Repository: pelican-dev/panel

Length of output: 19578

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n## S3BackupSchema::deleteBackup and multipart flow\n'
sed -n '1,260p' app/Extensions/BackupAdapter/Schemas/S3BackupSchema.php | cat -n

printf '\n## BackupRemoteUploadController\n'
sed -n '1,220p' app/Http/Controllers/Api/Remote/Backups/BackupRemoteUploadController.php | cat -n

printf '\n## BackupStatusController\n'
sed -n '1,160p' app/Http/Controllers/Api/Remote/Backups/BackupStatusController.php | cat -n

printf '\n## PruneOrphanedBackupsCommand\n'
sed -n '1,160p' app/Console/Commands/Maintenance/PruneOrphanedBackupsCommand.php | cat -n

printf '\n## BackupQueryBuilder\n'
sed -n '1,120p' app/Eloquent/BackupQueryBuilder.php | cat -n

Repository: pelican-dev/panel

Length of output: 19646

---

Do not swallow adapter failures for in-progress backups.
is_successful === false also covers backups that already have upload_id set, so deleting the row after a failed deleteBackup() can leave multipart uploads or objects orphaned. Restrict this fallback to records that never started uploading, or keep cleanup metadata for a retry.

🤖 Prompt for 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.

In `@app/Services/Backups/DeleteBackupService.php` around lines 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.