From 033f78d90770a076362c5ec3a971854869521bde Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Tue, 29 Sep 2026 23:23:02 +0200 Subject: [PATCH] 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 --- app/Jobs/DatabaseBackupJob.php | 20 ++-- .../DatabaseImportCommandBuilder.php | 4 +- bootstrap/helpers/databases.php | 10 ++ tests/Feature/DatabaseDumpExitStatusTest.php | 111 ++++++++++++++++++ tests/Feature/Jobs/DatabaseBackupJobTest.php | 5 +- tests/Feature/StandaloneSqliteTest.php | 2 +- 6 files changed, 137 insertions(+), 15 deletions(-) create mode 100644 tests/Feature/DatabaseDumpExitStatusTest.php diff --git a/app/Jobs/DatabaseBackupJob.php b/app/Jobs/DatabaseBackupJob.php index bc28f4a968..890e14462e 100644 --- a/app/Jobs/DatabaseBackupJob.php +++ b/app/Jobs/DatabaseBackupJob.php @@ -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); } /** diff --git a/app/Support/DatabaseImport/DatabaseImportCommandBuilder.php b/app/Support/DatabaseImport/DatabaseImportCommandBuilder.php index f93c51cc66..7d598b46e3 100644 --- a/app/Support/DatabaseImport/DatabaseImportCommandBuilder.php +++ b/app/Support/DatabaseImport/DatabaseImportCommandBuilder.php @@ -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 << "\$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; } diff --git a/bootstrap/helpers/databases.php b/bootstrap/helpers/databases.php index 359ad4481f..a4407c064a 100644 --- a/bootstrap/helpers/databases.php +++ b/bootstrap/helpers/databases.php @@ -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)) { diff --git a/tests/Feature/DatabaseDumpExitStatusTest.php b/tests/Feature/DatabaseDumpExitStatusTest.php new file mode 100644 index 0000000000..e614db4250 --- /dev/null +++ b/tests/Feature/DatabaseDumpExitStatusTest.php @@ -0,0 +1,111 @@ +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'/"); +}); diff --git a/tests/Feature/Jobs/DatabaseBackupJobTest.php b/tests/Feature/Jobs/DatabaseBackupJobTest.php index 3ff9a2cf28..a19958608f 100644 --- a/tests/Feature/Jobs/DatabaseBackupJobTest.php +++ b/tests/Feature/Jobs/DatabaseBackupJobTest.php @@ -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') diff --git a/tests/Feature/StandaloneSqliteTest.php b/tests/Feature/StandaloneSqliteTest.php index d813ed175d..f3a794d5fa 100644 --- a/tests/Feature/StandaloneSqliteTest.php +++ b/tests/Feature/StandaloneSqliteTest.php @@ -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 () {