Skip to content

fix: run the database dump only once per backup - #4896

Open
hl9020 wants to merge 1 commit into
Dokploy:canaryfrom
hl9020:fix/backup-double-dump
Open

fix: run the database dump only once per backup#4896
hl9020 wants to merge 1 commit into
Dokploy:canaryfrom
hl9020:fix/backup-double-dump

Conversation

@hl9020

@hl9020 hl9020 commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Problem

getBackupCommand interpolates the generated dump command into the backup script twice: once as a validation run discarded to /dev/null, then a second time piped into rclone rcat for the actual upload. Every backup therefore dumps the database twice:

  • Doubles backup runtime and database load. Real-world measurement: an 11 GB Postgres database takes ~24 minutes instead of ~12, with pg_dump pinning one core at 100% the whole time.
  • Affects all database types (postgres, mysql, mariadb, mongo, libsql) and compose backups, since they all go through getBackupCommand.
  • The dump that gets "validated" is not the dump that gets uploaded, so the validation says nothing about the file actually stored in S3.

Fix

Run the dump once and stream it directly into rclone. The point of the old validation run — distinguishing "Backup failed" from "Upload to S3 failed" in the deployment log — is preserved: the dump's exit code and stderr are captured through temp files inside the combined pipeline, so the failure paths stay separate:

  • dump fails → logs ❌ Error: Backup failed plus the dump's stderr, exits 1
  • rclone fails → logs ❌ Error: Upload to S3 failed plus rclone's stderr, exits 1
  • if both fail, the dump failure takes precedence

Additionally, a failed streamed run can leave a truncated object behind in the bucket (already possible today whenever the second dump or the connection failed mid-stream — the truncated object then counted against the keepLatestNBackups retention window). Both failure paths now delete the partial object via a rclone deletefile command derived from the rcat command, so failed runs no longer pollute the bucket or evict good backups from retention.

Notes regarding #4235

The double dump is mentioned there as a secondary finding — this PR removes it.

Regarding the reported missing semicolon after CONTAINER_ID=$(...): on current canary the generated script contains real newlines, so this does not parse as a syntax error (the error output in the issue shows the script collapsed to a single line, which looks like a display artifact of the error message). I still added the ; for consistency with the rest of the script and as defense in depth. Worth noting for triage: the container filter in the issue's error output shows an empty app name (com.docker.swarm.service.name=), so the reporter's actual failure is most likely "container not found" due to an empty appName, not a parse error.

Testing

  • Added unit tests in apps/dokploy/__test__/utils/backups.test.ts asserting that the dump command appears exactly once in the generated script, that it is piped directly into the rclone command, that both error messages remain present, and that the cleanup command is derived correctly. All 13 tests in the file pass (pnpm server:build + vitest run).
  • Verified the new bash logic with a harness exercising all four paths (success, dump failure, upload failure, both failing): the dump runs exactly once, the correct error message and stderr are logged per side, the partial-object cleanup runs on both failure paths, and the temp files are removed via the EXIT trap in every case.
  • biome check passes on both changed files.
  • Not run: an end-to-end backup against a live Docker/S3 setup — the change is confined to the generated script, whose semantics are covered by the harness above.

Related to #4235

@hl9020
hl9020 requested a review from Siumauricio as a code owner July 22, 2026 20:49
@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. bug Something isn't working labels Jul 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant