[Fix] Prevent backups:run crash for backups whose server was deleted

[erhanurgun]

Closes #1206

When a server that has backups is deleted, its Backup rows are left with a dangling server_id (there's no FK on the column). The scheduled backups:run then reads $backup->server->project_id on a null server and crashes on every run.

This makes the runner and the backup-file cleanup resilient to those already-orphaned rows:

• RunBackupCommand: skip backups whose server is gone (whereHas('server')) and eager-load server (also fixes a pre-existing N+1).
• ManageBackupFile::delete(): null-safe when the server is missing — it logs and drops the local record instead of dispatching an SSH job, and skips the socket broadcast. (Server loaded via Server::find() so PHPStan doesn't treat the non-nullable relation as never-null.)

Also removed the unused ManageBackupFile::download() method (dead since the Inertia migration in #593 — its only caller was the old Filament widget, which wasn't re-wired), per review.

Note: cascading a server's backups on delete is now handled by #1198, so this PR doesn't touch that — it only covers rows already orphaned on existing installs, which #1198's cascade doesn't reach.

Tests (SSH::fake + RefreshDatabase):

• RunBackupCommandTest::test_does_not_run_backups_whose_server_is_missing
• BackupTest::test_delete_orphaned_backup_file_hard_deletes_without_dispatching_job

No migration.

Summary by CodeRabbit

• Bug Fixes
  • Backup file deletions are now safely handled when the associated server is missing, hard-deleting the file without triggering remote deletion.
  • Backups scheduled with a missing server are skipped, and no jobs/events are dispatched.
• Tests
  • Added feature coverage for hard-deleting an orphaned backup file without dispatching a delete job.
  • Added unit coverage ensuring the backups:run command does not start backups for missing servers.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• ✅ Review completed - (🔄 Check again to review again)

📝 Walkthrough

Walkthrough

Backup file deletion now handles missing servers by removing orphaned records without dispatching jobs. The scheduled backup command filters out backups without associated servers. Tests cover both behaviours.

Changes

Backup orphan cleanup

Layer / File(s)	Summary
Orphan-safe backup file deletion app/Actions/Backup/ManageBackupFile.php, tests/Feature/BackupTest.php	ManageBackupFile resolves servers by ID, hard-deletes orphaned backup files, and retains the server-backed deletion flow. Feature tests verify orphan deletion without job dispatch.
Backup runner server filtering app/Console/Commands/RunBackupCommand.php, tests/Unit/Commands/RunBackupCommandTest.php	backups:run requires and eager-loads the server relation; tests verify backups with missing servers remain unchanged and dispatch no work.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: saeedvaziry, richardanderson

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The PR skips orphaned backups in backups:run and makes orphaned file deletion safe, matching #1206's crash-prevention goals.
Out of Scope Changes check	✅ Passed	The changes and tests stay focused on orphaned backup handling and do not introduce unrelated scope.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly states the main fix: preventing backups:run crashes when a backup’s server has been deleted.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6dd0eadadc

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[erhanurgun]

Heads up: I noticed #1198 already adds the Server::deleting backup cascade (and reworks backups() into a HasManyThrough), so I trimmed that hunk out of this PR to avoid a conflicting duplicate. This one now only makes backups:run and the backup-file ops resilient to backups that are already orphaned on existing installs, which #1198's future-only cascade doesn't cover (I confirmed it touches neither RunBackupCommand nor ManageBackupFile). Happy to reorder behind #1198, fold the runner guard in, or re-add the cascade here if #1198 stalls, whatever you prefer.

[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 `@tests/Unit/Commands/RunBackupCommandTest.php`:
- Around line 129-138: Update the assertDatabaseHas call in the orphaned-backup
test to include server_id => 999999 alongside the backup ID, ensuring the
persisted fixture is verified as orphaned while preserving the existing
assertions.

🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 9784c4a1-8610-45db-8146-c997c7723fe0

📥 Commits

Reviewing files that changed from the base of the PR and between 90b5392 and 00e8a3c.

📒 Files selected for processing (1)

• tests/Unit/Commands/RunBackupCommandTest.php

[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 `@tests/Unit/Commands/RunBackupCommandTest.php`:
- Around line 132-138: Update the assertDatabaseHas call in the backup orphan
test to include server_id set to 999999 alongside the backup ID, confirming the
fixture persisted the missing server reference while preserving the existing
no-dispatch assertion.

🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 9784c4a1-8610-45db-8146-c997c7723fe0

📥 Commits

Reviewing files that changed from the base of the PR and between 90b5392 and 00e8a3c.

📒 Files selected for processing (1)

• tests/Unit/Commands/RunBackupCommandTest.php

[RichardAnderson]

Hi @erhanurgun - I just merged the approved PR #1198 - and these changes complement it well, this has introduced a conflict on one of the tests.

[erhanurgun]

Thanks @RichardAnderson - glad it fits. I've merged #1198 in and resolved the BackupTest conflict (kept both the orphan-path tests and your global-backup tests). Green on my side again.