Skip to content

fix(volume-backups): cancel the scheduled job when a volume backup is deleted - #5454

Open
Dev-next-gen wants to merge 2 commits into
Dokploy:canaryfrom
Dev-next-gen:fix/volume-backup-delete-cancel-job
Open

Dev-next-gen wants to merge 2 commits into
Dokploy:canaryfrom
Dev-next-gen:fix/volume-backup-delete-cancel-job

Conversation

@Dev-next-gen

@Dev-next-gen Dev-next-gen commented Sep 13, 2026

Copy link
Copy Markdown

What is this PR about?

While comparing how the different routers tear down their cron jobs, I noticed that volumeBackups.delete only calls removeVolumeBackup, which deletes the database row and nothing else. backup.remove and schedule.delete also cancel the job they registered, but the volume backup path never does: the node-schedule job (self-hosted) or the BullMQ repeatable job (cloud) created in volumeBackups.create keeps firing on its cron and each run fails in findVolumeBackupById with "Volume backup not found", until the process or the jobs service restarts.

The fix cancels the job right after the row is deleted, the same way schedule.delete does: removeJob with the stored cron expression in cloud, removeVolumeBackupJob otherwise.

I added __test__/backups/volume-backup-delete.test.ts, which calls the router through createCaller with the server functions mocked. On canary both cases fail (removeVolumeBackupJob and removeJob are called 0 times); with the change they pass. pnpm run typecheck in apps/dokploy is clean, and the full vitest suite passes except deploy/application.real.test.ts, which needs nixpacks and real Docker builds on the machine and is unrelated to this change.

Checklist

Before submitting this PR, please make sure that:

  • You created a dedicated branch based on the canary branch.
  • You have read the suggestions in the CONTRIBUTING.md file https://github.com/Dokploy/dokploy/blob/canary/CONTRIBUTING.md#pull-request
  • You have tested this PR in your local instance. If you have not tested it yet, please do so before submitting. This helps avoid wasting maintainers' time reviewing code that has not been verified by you.

To be upfront about that last box: I verified the change with the unit test above and the typecheck, not by deleting a volume backup in a running Dokploy instance.

Issues related (if applicable)

None that I could find.

Screenshots (if applicable)

AI tools used

RetriggerConfidence Score: 5/5

The PR appears safe to merge, with no outstanding findings or new changes since the previous review.

Summary

Fixes volume-backup deletion so its scheduled work is also canceled after the database record is removed.

  • Cancels the local node-schedule job in self-hosted deployments.
  • Removes the BullMQ repeatable job in cloud deployments using the stored cron expression.
  • Adds router tests covering both deployment modes.

Reviews (2) · Last reviewed commit: "fix(volume-backups): cancel the schedule..."

… deleted

The delete procedure only removed the database row. The node-schedule job
(self-hosted) or the BullMQ repeatable job (cloud) registered at creation
kept firing on its cron and failed each time with "Volume backup not found".
Cancel it the same way backup.remove and schedule.delete already do.
Comment thread apps/dokploy/server/api/routers/volume-backups.ts Outdated
Comment thread apps/dokploy/server/api/routers/volume-backups.ts Outdated
@narcisonunez

narcisonunez commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator

1. volume-backups.ts:178 — DB row deleted before job cancellation, no rollback
removeVolumeBackup runs before removeJob/removeVolumeBackupJob. If job cancellation fails (e.g. JOBS_URL request fails in cloud mode), the mutation errors but the DB row is already gone — leaving an orphaned job that keeps firing and failing with "Volume backup not found," the exact bug this PR is meant to fix. Cancel the job first, then delete the row.

2. volume-backups.ts:179 — unconditional job-removal call even when never enabled
create only schedules a job when enabled: true, but delete now always tries to cancel a job regardless, causing an unnecessary round-trip to JOBS_URL for backups that were never enabled.

3. volume-backups.ts:179-186 — triplicated cancellation logic
The IS_CLOUD ? removeJob : removeVolumeBackupJob branch with the same payload is now duplicated in create (~114-124), update (~216-237), and delete (~179-186). Extract to a shared helper (e.g. cancelVolumeBackupJob(vb)) to avoid the same class of bug recurring at future deletion call sites.

Cancellation now runs before removeVolumeBackup, so a cancellation that
fails leaves the record in place and the mutation can be retried instead
of committing the deletion and leaving an orphan job behind.

The local branch awaits removeVolumeBackupJob, delete skips cancellation
for a backup that was never enabled, and the IS_CLOUD branch shared by
delete and update moves into a cancelVolumeBackupJob helper.
@Dev-next-gen

Copy link
Copy Markdown
Author

All four are right, and the second one I had missed: removeVolumeBackupJob is async (packages/server/src/utils/volume-backups/utils.ts:72), and update was calling it unawaited too, in both of its non-cloud branches.

Pushed as a148c0c. Cancellation now runs before removeVolumeBackup, it is awaited in both modes, delete only cancels when vb.enabled (mirroring create, which only schedules when enabled), and the IS_CLOUD branch became a single cancelVolumeBackupJob helper used by delete and by update's cancel paths. I renamed the test file to volume-backup-jobs.test.ts since it now covers both. Run against a63c018 with the new tests in place, the five delete tests fail and the three update tests pass, which is the split I wanted: the update assertions pin the behaviour the helper had to preserve. With the change, 8/8 pass, tsc --noEmit in apps/dokploy is clean, biome check reports no diff on either file, and the full vitest run is 1037 passed / 4 failed, the four being __test__/deploy/application.real.test.ts, which needs nixpacks and a swarm-enabled Docker on the machine.

The one judgement call I would rather hand back to you is that vb.enabled guard: it trusts the model the file already assumes, so a row that was never enabled has no job to cancel. If you would rather delete always attempt the cancellation as a cleanup for rows whose flag drifted from the live schedule, say so and I will drop the guard.

Found by a defect-hunting pipeline I build and run (Dev-next-gen), using Claude Code with Anthropic's Claude Opus 5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants