fix(backup): fail compressed dumps when the dump command fails

A plain pipe into the compressor only reported the compressor's exit
status, so a failed dump still produced a small, valid archive. Add
pipeToFileKeepingExitStatus() to propagate the source's exit status in
POSIX sh and use it for all compressed dump commands.

- Delete the partial local backup file on any failed backup, not only
  for ClickHouse, since untracked files are never removed by retention
- Wait up to 10s for locks when running VACUUM INTO for SQLite backups
- Refuse SQLite restores unless the file has a SQLite header, so an
  empty backup no longer wipes the database
- Add tests for dump exit status handling and SQLite restore validation
This commit is contained in:
Andras Bacsai
2026-09-29 23:23:02 +02:00
parent 2ac5969d90
commit 033f78d907
6 changed files with 137 additions and 15 deletions
+9 -11
View File
@@ -427,10 +427,8 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue
throw new \Exception('Local backup file is empty or was not created');
}
} catch (Throwable $e) {
// Local backup failed
if ($this->database instanceof StandaloneClickhouse) {
deleteBackupsLocally($this->backup_location, $this->server);
}
// Local backup failed: the partial file is not tracked, so retention would never remove it.
deleteBackupsLocally($this->backup_location, $this->server);
if ($this->backup_log) {
$this->backup_log->update([
'status' => 'failed',
@@ -658,7 +656,7 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue
$escapedBackupLocation = escapeshellarg($this->backup_location);
if ($this->backup->dump_all) {
$backupCommand .= " {$escapedContainerName} pg_dumpall --username $escapedUsername";
$backupCommand = $this->buildCompressedDumpCommand($backupCommand).' > '.$escapedBackupLocation;
$backupCommand = $this->buildCompressedDumpCommand($backupCommand, $escapedBackupLocation);
} else {
// Validate and escape database name to prevent command injection
validateShellSafePath($database, 'database name');
@@ -687,7 +685,7 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue
$escapedBackupLocation = escapeshellarg($this->backup_location);
if ($this->backup->dump_all) {
$dumpCommand = "docker exec {$escapedContainerName} mysqldump -u root -p$escapedPassword --all-databases --single-transaction --quick --lock-tables=false";
$commands[] = $this->buildCompressedDumpCommand($dumpCommand).' > '.$escapedBackupLocation;
$commands[] = $this->buildCompressedDumpCommand($dumpCommand, $escapedBackupLocation);
} else {
// Validate and escape database name to prevent command injection
validateShellSafePath($database, 'database name');
@@ -714,7 +712,7 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue
$escapedBackupLocation = escapeshellarg($this->backup_location);
if ($this->backup->dump_all) {
$dumpCommand = "docker exec {$escapedContainerName} mariadb-dump -u root -p$escapedPassword --all-databases --single-transaction --quick --lock-tables=false";
$commands[] = $this->buildCompressedDumpCommand($dumpCommand).' > '.$escapedBackupLocation;
$commands[] = $this->buildCompressedDumpCommand($dumpCommand, $escapedBackupLocation);
} else {
// Validate and escape database name to prevent command injection
validateShellSafePath($database, 'database name');
@@ -765,9 +763,9 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue
throw new \Exception("Invalid database file name: {$database}");
}
$commands[] = 'mkdir -p '.escapeshellarg($this->backup_dir);
$script = 'f=$(mktemp) && sqlite3 -readonly '.escapeshellarg(StandaloneSqlite::DATA_DIRECTORY.'/'.$database).' "VACUUM INTO \'$f\'" && cat "$f"; s=$?; rm -f "$f"; exit $s';
$script = 'f=$(mktemp) && sqlite3 -readonly '.escapeshellarg(StandaloneSqlite::DATA_DIRECTORY.'/'.$database).' \'.timeout 10000\' "VACUUM INTO \'$f\'" && cat "$f"; s=$?; rm -f "$f"; exit $s';
$dumpCommand = 'docker exec '.escapeshellarg($this->container_name).' sh -c '.escapeshellarg($script);
$commands[] = $this->buildCompressedDumpCommand($dumpCommand).' > '.escapeshellarg($this->backup_location);
$commands[] = $this->buildCompressedDumpCommand($dumpCommand, escapeshellarg($this->backup_location));
$this->backup_output = instant_remote_process($this->writeBackupFileAsRoot($commands), $this->server, true, false, $this->timeout, disableMultiplexing: true);
$this->backup_output = trim($this->backup_output);
if ($this->backup_output === '') {
@@ -900,13 +898,13 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue
);
}
private function buildCompressedDumpCommand(string $dumpCommand): string
private function buildCompressedDumpCommand(string $dumpCommand, string $escapedBackupLocation): string
{
$cpuPercentage = BackupCompression::cpuPercentage($this->server->settings->backup_compression_cpu_percentage);
$compressorCommand = BackupCompression::compressorCommand($cpuPercentage);
$script = "compressor=\$({$compressorCommand}); exec \$compressor";
return $dumpCommand.' | docker run --rm -i '.escapeshellarg($this->getFullImageName()).' sh -c '.escapeshellarg($script);
return pipeToFileKeepingExitStatus($dumpCommand, 'docker run --rm -i '.escapeshellarg($this->getFullImageName()).' sh -c '.escapeshellarg($script), $escapedBackupLocation);
}
/**
@@ -277,7 +277,8 @@ SH;
/**
* SQLite restores a plain or gzip-compressed database file into the first database
* file with .restore, which replaces its contents.
* file with .restore, which replaces its contents. SQLite reads an empty file (for
* example from a failed dump) as an empty database, so only real database files are restored.
*/
private function sqlite(string $file): string
{
@@ -285,6 +286,7 @@ SH;
return <<<SH
stream > "\$backup.db" || fail 'The backup cannot be read. Nothing was changed.'
[ "\$(head -c 15 "\$backup.db")" = 'SQLite format 3' ] || { rm -f "\$backup.db"; fail 'The backup is not a SQLite database. Nothing was changed.'; }
sqlite3 -bail {$file} '.timeout 10000' ".restore \$backup.db"; status=\$?; rm -f "\$backup.db"; exit \$status
SH;
}
+10
View File
@@ -199,6 +199,16 @@ function create_standalone_sqlite($environment_id, StandaloneDocker|SwarmDocker
return $database;
}
/**
* Shell line that pipes $source into $sink and writes the output to $escapedFile. It fails when
* either command fails: a plain pipe only reports the exit status of $sink, so a failed dump
* would still leave a small, valid archive. POSIX sh (dash, BusyBox ash); no pipefail needed.
*/
function pipeToFileKeepingExitStatus(string $source, string $sink, string $escapedFile): string
{
return 'status=$( { { '.$source.'; echo $? >&3; } | '.$sink.' > '.$escapedFile.'; } 3>&1 ) && [ "$status" -eq 0 ]';
}
function deleteBackupsLocally(string|array|null $filenames, Server $server, bool $throwError = false): void
{
if (empty($filenames)) {
@@ -0,0 +1,111 @@
<?php
use App\Jobs\DatabaseBackupJob;
use App\Models\Environment;
use App\Models\InstanceSettings;
use App\Models\PrivateKey;
use App\Models\Project;
use App\Models\ScheduledDatabaseBackup;
use App\Models\Server;
use App\Models\StandaloneDocker;
use App\Models\StandaloneSqlite;
use App\Models\Team;
use App\Support\DatabaseImport\DatabaseImportCommandBuilder;
use Illuminate\Foundation\Testing\RefreshDatabase;
use Illuminate\Support\Facades\Notification;
use Illuminate\Support\Facades\Process;
uses(RefreshDatabase::class);
beforeEach(function () {
$this->workDirectory = sys_get_temp_dir().'/coolify-dump-exit-status-'.uniqid();
mkdir($this->workDirectory.'/bin', 0700, true);
});
afterEach(function () {
Process::run(['rm', '-rf', $this->workDirectory]);
});
it('fails a compressed dump when the dump or the compressor fails', function (string $source, string $sink, bool $succeeds, ?string $content) {
$file = $this->workDirectory.'/backup.gz';
$result = Process::run(['sh', '-c', pipeToFileKeepingExitStatus($source, $sink, escapeshellarg($file))]);
expect($result->successful())->toBe($succeeds);
if ($content !== null) {
expect(gzdecode(file_get_contents($file)))->toBe($content);
}
})->with([
'dump fails after partial output' => ['sh -c "echo partial; exit 1"', 'gzip', false, null],
'dump cannot open the database' => ['sh -c "exit 1"', 'gzip', false, null],
'compressor fails' => ['echo data', 'false', false, null],
'dump and compressor succeed' => ['echo data', 'gzip', true, "data\n"],
]);
it('restores a SQLite backup only when it is a SQLite database file', function (string $backupContent, bool $restores) {
$backup = $this->workDirectory.'/restore';
file_put_contents($backup, gzencode($backupContent));
$marker = $this->workDirectory.'/sqlite3-called';
file_put_contents($this->workDirectory.'/bin/sqlite3', "#!/bin/sh\ntouch ".escapeshellarg($marker)."\n");
chmod($this->workDirectory.'/bin/sqlite3', 0755);
$database = Mockery::mock(StandaloneSqlite::class);
$database->shouldReceive('getMorphClass')->andReturn(StandaloneSqlite::class);
$database->shouldReceive('databaseFilePath')->andReturn($this->workDirectory.'/app.db');
$script = app(DatabaseImportCommandBuilder::class)->buildRestoreCommand($database, $backup, false);
$result = Process::env(['PATH' => $this->workDirectory.'/bin:'.getenv('PATH')])->run(['sh', '-c', $script]);
expect($result->successful())->toBe($restores)
->and(file_exists($marker))->toBe($restores)
->and(file_exists($backup.'.db'))->toBeFalse();
if (! $restores) {
expect($result->errorOutput())->toContain('The backup is not a SQLite database. Nothing was changed.');
}
})->with([
'empty backup from a failed dump' => ['', false],
'SQL text instead of a database file' => ['CREATE TABLE t (x TEXT);', false],
'SQLite database file' => ["SQLite format 3\0".str_repeat("\0", 84), true],
]);
it('records a failed SQLite dump as failed and removes the partial backup file', function () {
InstanceSettings::forceCreate(['id' => 0]);
Notification::fake();
$team = Team::factory()->create();
$server = Server::factory()->create(['team_id' => $team->id]);
$server->update(['private_key_id' => PrivateKey::factory()->create(['team_id' => $team->id])->id]);
$server->settings()->update(['is_reachable' => true, 'is_usable' => true]);
$project = Project::factory()->create(['team_id' => $team->id]);
$environment = Environment::factory()->create(['project_id' => $project->id]);
$database = create_standalone_sqlite($environment->id, StandaloneDocker::where('server_id', $server->id)->firstOrFail(), [
'sqlite_databases' => 'app.db',
'status' => 'running:healthy',
]);
$backup = ScheduledDatabaseBackup::create([
'frequency' => '0 0 * * *',
'save_s3' => false,
'database_type' => StandaloneSqlite::class,
'database_id' => $database->id,
'team_id' => $team->id,
'databases_to_backup' => 'app.db',
]);
$commands = collect();
Process::fake(function ($process) use ($commands) {
$command = is_array($process->command) ? implode(' ', $process->command) : $process->command;
$commands->push($command);
return str_contains($command, 'VACUUM INTO')
? Process::result(errorOutput: 'Error: unable to open database "/var/lib/sqlite/app.db"', exitCode: 1)
: Process::result(output: '');
});
try {
(new DatabaseBackupJob($backup))->handle();
} catch (Throwable) {
// The job reports the failure after it records the execution.
}
$execution = $backup->executions()->latest('id')->firstOrFail();
expect($execution->status)->toBe('failed')
->and($execution->filename)->toBeNull()
->and($commands->implode("\n"))->toMatch("/rm -f '[^']*sqlite-backup-app\\.db-\\d+\\.gz'/");
});
+3 -2
View File
@@ -346,10 +346,11 @@ test('database dump compression uses the helper image and shared CPU setting', f
$command = (new ReflectionClass($job))
->getMethod('buildCompressedDumpCommand')
->invoke($job, 'docker exec database pg_dumpall');
->invoke($job, 'docker exec database pg_dumpall', "'/backups/pg-dump-all.gz'");
expect($command)
->toStartWith('docker exec database pg_dumpall | docker run --rm -i')
->toStartWith('status=$( { { docker exec database pg_dumpall; echo $? >&3; } | docker run --rm -i')
->toContain("> '/backups/pg-dump-all.gz'; } 3>&1 ) && [ \"\$status\" -eq 0 ]")
->toContain('coolify-helper')
->toContain('command -v pigz')
->toContain('pigz -3 -p')
+1 -1
View File
@@ -83,7 +83,7 @@ it('restores a gzipped backup with .restore into the first database file', funct
expect($command)
->toStartWith("backup='/tmp/restore_1'\n")
->toContain('stream() { if is_gzip; then gunzip -c "$backup"; else cat "$backup"; fi; }')
->toEndWith("stream > \"\$backup.db\" || fail 'The backup cannot be read. Nothing was changed.'\nsqlite3 -bail '/var/lib/sqlite/app.db' '.timeout 10000' \".restore \$backup.db\"; status=\$?; rm -f \"\$backup.db\"; exit \$status");
->toEndWith("stream > \"\$backup.db\" || fail 'The backup cannot be read. Nothing was changed.'\n[ \"\$(head -c 15 \"\$backup.db\")\" = 'SQLite format 3' ] || { rm -f \"\$backup.db\"; fail 'The backup is not a SQLite database. Nothing was changed.'; }\nsqlite3 -bail '/var/lib/sqlite/app.db' '.timeout 10000' \".restore \$backup.db\"; status=\$?; rm -f \"\$backup.db\"; exit \$status");
});
it('normalises the file list when saved from the general page', function () {