fix: remove redundant backup command execution that runs database dump twice

[kuishou68]

Summary

Fixes a bug where the database dump command is executed twice per backup operation, wasting resources and potentially uploading a different dump than the one that was "verified".

Problem

In packages/server/src/utils/backups/utils.ts, the getBackupCommand function generates a shell script that runs ${backupCommand} (the database dump) twice:

# First execution — output discarded (just a test run)
BACKUP_OUTPUT=$(${backupCommand} 2>&1 >/dev/null) || {
    echo "[$(date)] ❌ Error: Backup failed" >> ${logPath};
    exit 1;
}

# Second execution — this is the actual upload
UPLOAD_OUTPUT=$(${backupCommand} | ${rcloneCommand} 2>&1 >/dev/null) || {
    echo "[$(date)] ❌ Error: Upload to S3 failed" >> ${logPath};
    exit 1;
}

This means every backup:

1. Dumps the entire database and discards the output
2. Dumps the database again and uploads it to S3

The two dumps may capture different database states. The "verified" dump is never what gets uploaded.

Fix

Removed the redundant first dump block. The backup command now runs exactly once and streams directly into rclone. Error handling is preserved in the upload pipeline.

Files Changed

• packages/server/src/utils/backups/utils.ts — removed the redundant BACKUP_OUTPUT test execution block

Closes #4222

Greptile Summary

This PR fixes a real bug where the database dump command was executed twice per backup — once as a silent "test run" (output discarded) and again piped into rclone for upload — meaning the uploaded dump could reflect a different database state than the one that "passed" verification. The fix is correct: removing the first block lets the dump run once and stream directly into rclone, preserving pipefail error propagation.

One minor follow-up worth considering: after the change, backupCommand's stderr is not redirected into UPLOAD_OUTPUT, so if the dump itself fails the log will show "Upload to S3 failed" with no useful context from the database tool. Merging backupCommand's stderr into the captured stream (2>&1 | ${rcloneCommand}) would restore that diagnostic value.

Confidence Score: 5/5

• Safe to merge — the core fix is correct and the only remaining finding is a P2 observability improvement.
• The change removes a clear double-execution bug. No correctness, data-integrity, or security issues remain. The one comment is a P2 style/observability suggestion about capturing dump stderr, which does not block merge.
• No files require special attention.

Comments Outside Diff (1)

1. packages/server/src/utils/backups/utils.ts, line 286-289 (link)

   Misleading error message and lost stderr when the dump itself fails

   With the redundant block removed, if backupCommand fails (e.g. DB connection refused), the user still sees "❌ Error: Upload to S3 failed" and UPLOAD_OUTPUT will only contain rclone's stderr (likely a broken-pipe message), not the actual database error. backupCommand's stderr is not redirected by 2>&1 >/dev/null here — it escapes to the process's stderr and never reaches the log file.

   Consider splitting the check to preserve diagnostic clarity:

   # Run the backup and pipe directly to rclone
   ${backupCommand} | RCLONE_EXIT=${PIPESTATUS[1]}; BACKUP_EXIT=${PIPESTATUS[0]}

   Or more portably, keep a single pipeline but improve error attribution:

   UPLOAD_OUTPUT=$(${backupCommand} 2>&1 | ${rcloneCommand} 2>&1 >/dev/null) || {
       echo "[$(date)] ❌ Error: Backup or upload failed" >> ${logPath};
       echo "Error: $UPLOAD_OUTPUT" >> ${logPath};
       exit 1;
   }

   Redirecting backupCommand's stderr into the pipeline (2>&1) would capture dump errors in UPLOAD_OUTPUT and make the log actionable when diagnosing failures.

_{Reviews (1): Last reviewed commit: "fix: remove redundant backup command exe..." | Re-trigger Greptile}

_{(4/5) You can add custom instructions or style guidelines for the agent here!}