mirror of
https://github.com/coollabsio/coolify.git
synced 2026-11-06 13:10:37 -04:00
fix(ssh): do not replay remote commands that failed on the server
The SSH retry matched connection words in any error output. When `mc alias set` in a backup upload could not reach S3, its "connection refused" output caused a retry of the full batch, and the second `docker run --name backup-of-<uuid>` failed with a name conflict that hid the real error. - Retry only SSH transport failures: exit code 255, or errors without an exit code. - Keep the remote exit code on deployment command failures. - Remove a leftover backup helper container in the same upload batch. Closes #12059
This commit is contained in:
@@ -8,6 +8,7 @@
|
||||
- Several dev instances can run from one checkout (see `./scripts/dev urls`). After you add a migration, run it on each running instance of that checkout, or those instances fail on the new code.
|
||||
- The checkout is shared with other sessions. When the full suite fails, rerun each failing file alone and compare with a clean `git archive HEAD` copy before you connect a failure to your change.
|
||||
- `sshd` keeps the login shell of an open multiplexed SSH connection. Call `SshMultiplexingHelper::removeMuxFile()` before a live test of SSH or login-shell behavior.
|
||||
- SSH retries replay the full command batch. Only exit code 255 is an SSH transport failure; other exit codes come from the remote command, so do not retry them. Keep batches safe to run twice (for example `docker rm -f <name> || true` before `docker run --name <name>`).
|
||||
- Redirect browser test output to a file (`> /tmp/x.log 2>&1`); piping it (`| tail`) hangs because the Playwright server keeps the pipe open.
|
||||
- Call `visit()` directly in each `tests/v4/Browser` test body; Pest does not mark a test that only uses helper-wrapped `visit()` as a browser test, so it fails with `sendText() on null`.
|
||||
|
||||
|
||||
@@ -847,11 +847,8 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue
|
||||
|
||||
$fullImageName = $this->getFullImageName();
|
||||
|
||||
$containerExists = instant_remote_process(["docker ps -a -q -f name=backup-of-{$this->backup_log_uuid}"], $this->server, false, false, null, disableMultiplexing: true);
|
||||
if (filled($containerExists)) {
|
||||
instant_remote_process(["docker rm -f backup-of-{$this->backup_log_uuid}"], $this->server, false, false, null, disableMultiplexing: true);
|
||||
}
|
||||
|
||||
// Remove a leftover helper in the same batch, so a replayed batch does not fail with a name conflict.
|
||||
$commands[] = "docker rm -f backup-of-{$this->backup_log_uuid} >/dev/null 2>&1 || true";
|
||||
$mount = escapeshellarg($this->backupMountSource().':'.$this->backup_location.':ro');
|
||||
$commands[] = "docker run -d --network {$safeNetwork} --name backup-of-{$this->backup_log_uuid} --rm -v {$mount} {$fullImageName}";
|
||||
|
||||
|
||||
@@ -125,7 +125,7 @@ trait ExecuteRemoteCommand
|
||||
$lastError = $e;
|
||||
$errorMessage = $e->getMessage();
|
||||
// Only retry if it's an SSH connection error and we haven't exhausted retries
|
||||
if ($this->isRetryableSshError($errorMessage) && $attempt < $maxRetries - 1) {
|
||||
if ($this->isRetryableSshFailure($e) && $attempt < $maxRetries - 1) {
|
||||
$attempt++;
|
||||
$delay = $this->calculateRetryDelay($attempt - 1);
|
||||
|
||||
@@ -243,7 +243,7 @@ trait ExecuteRemoteCommand
|
||||
if (empty($error)) {
|
||||
$error = $process_result->output() ?: 'Command failed with no error output';
|
||||
}
|
||||
throw new DeploymentException($this->commandFailureMessage((string) $command, (int) $process_result->exitCode(), (string) $error, $skip_command_log));
|
||||
throw new DeploymentException($this->commandFailureMessage((string) $command, (int) $process_result->exitCode(), (string) $error, $skip_command_log), (int) $process_result->exitCode());
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -53,6 +53,23 @@ trait SshRetryable
|
||||
return false;
|
||||
}
|
||||
|
||||
/**
|
||||
* Check if a failure came from the SSH connection and not from the remote command.
|
||||
*
|
||||
* SSH exits with 255 for its own errors. Any other non-zero exit code comes from the
|
||||
* remote command, whose output can contain words such as "Connection refused" (for
|
||||
* example a failed S3 upload). Retrying would replay commands that already ran.
|
||||
*/
|
||||
protected function isRetryableSshFailure(\Throwable $exception): bool
|
||||
{
|
||||
$exitCode = $exception->getCode();
|
||||
if (is_int($exitCode) && $exitCode > 0 && $exitCode !== 255) {
|
||||
return false;
|
||||
}
|
||||
|
||||
return $this->isRetryableSshError($exception->getMessage());
|
||||
}
|
||||
|
||||
/**
|
||||
* Calculate delay for exponential backoff
|
||||
*/
|
||||
@@ -92,7 +109,7 @@ trait SshRetryable
|
||||
$lastErrorMessage = $e->getMessage();
|
||||
|
||||
// Check if it's retryable and not the last attempt
|
||||
if ($this->isRetryableSshError($lastErrorMessage) && $attempt < $maxRetries - 1) {
|
||||
if ($this->isRetryableSshFailure($e) && $attempt < $maxRetries - 1) {
|
||||
$delay = $this->calculateRetryDelay($attempt);
|
||||
|
||||
// Add deployment log if available (for ExecuteRemoteCommand trait)
|
||||
|
||||
@@ -101,3 +101,40 @@ it('uses the configured default timeout in instant_remote_process when no timeou
|
||||
|
||||
Process::assertRan(fn ($process) => str_starts_with($process->command, 'timeout 1234 ssh '));
|
||||
});
|
||||
|
||||
it('does not replay a command batch when a remote command fails with connection text in its output', function () {
|
||||
config([
|
||||
'constants.ssh.mux_enabled' => false,
|
||||
'constants.ssh.max_retries' => 3,
|
||||
'constants.ssh.retry_base_delay' => 0,
|
||||
]);
|
||||
$server = makeTimeoutTestServer();
|
||||
|
||||
Process::fake([
|
||||
'*' => Process::result(errorOutput: 'mc: <ERROR> Unable to initialize new alias. dial tcp 10.0.0.5:9000: connect: connection refused', exitCode: 1),
|
||||
]);
|
||||
|
||||
expect(fn () => instant_remote_process(['docker run -d --name backup-of-x minio/mc', 'docker exec backup-of-x mc alias set temporary http://s3 k s'], $server, disableMultiplexing: true))
|
||||
->toThrow(RuntimeException::class, 'connection refused');
|
||||
|
||||
Process::assertRanTimes(fn ($process) => str_contains($process->command, 'ssh '), 1);
|
||||
});
|
||||
|
||||
it('retries a command batch when the ssh connection itself fails', function () {
|
||||
config([
|
||||
'constants.ssh.mux_enabled' => false,
|
||||
'constants.ssh.max_retries' => 3,
|
||||
'constants.ssh.retry_base_delay' => 0,
|
||||
]);
|
||||
$server = makeTimeoutTestServer();
|
||||
|
||||
Process::fake([
|
||||
'*' => Process::sequence()
|
||||
->push(Process::result(errorOutput: 'kex_exchange_identification: read: Connection reset by peer', exitCode: 255))
|
||||
->push(Process::result(output: 'ok')),
|
||||
]);
|
||||
|
||||
expect(instant_remote_process(['echo ok'], $server, disableMultiplexing: true))->toBe('ok');
|
||||
|
||||
Process::assertRanTimes(fn ($process) => str_contains($process->command, 'ssh '), 2);
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user