fix: run the database dump only once per backup - #4896
Open
hl9020 wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
getBackupCommandinterpolates the generated dump command into the backup script twice: once as a validation run discarded to/dev/null, then a second time piped intorclone rcatfor the actual upload. Every backup therefore dumps the database twice:pg_dumppinning one core at 100% the whole time.getBackupCommand.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:
❌ Error: Backup failedplus the dump's stderr, exits 1❌ Error: Upload to S3 failedplus rclone's stderr, exits 1Additionally, 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
keepLatestNBackupsretention window). Both failure paths now delete the partial object via arclone deletefilecommand 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 currentcanarythe 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 emptyappName, not a parse error.Testing
apps/dokploy/__test__/utils/backups.test.tsasserting 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).EXITtrap in every case.biome checkpasses on both changed files.Related to #4235