From 599dc1ca67c5999da8408b72514cea70acc07e88 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Sat, 3 Oct 2026 20:52:01 +0200 Subject: [PATCH] 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-` 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 --- .ai/lessons.md | 1 + app/Jobs/DatabaseBackupJob.php | 7 ++-- app/Traits/ExecuteRemoteCommand.php | 4 +-- app/Traits/SshRetryable.php | 19 ++++++++++- tests/Feature/RemoteProcessTimeoutTest.php | 37 ++++++++++++++++++++++ 5 files changed, 60 insertions(+), 8 deletions(-) diff --git a/.ai/lessons.md b/.ai/lessons.md index af4080da3b..948fbcda3f 100644 --- a/.ai/lessons.md +++ b/.ai/lessons.md @@ -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 || true` before `docker run --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`. diff --git a/app/Jobs/DatabaseBackupJob.php b/app/Jobs/DatabaseBackupJob.php index 630e1153fc..44c9d5c7c7 100644 --- a/app/Jobs/DatabaseBackupJob.php +++ b/app/Jobs/DatabaseBackupJob.php @@ -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}"; diff --git a/app/Traits/ExecuteRemoteCommand.php b/app/Traits/ExecuteRemoteCommand.php index 5a4540ea3a..ce3ec8ae3b 100644 --- a/app/Traits/ExecuteRemoteCommand.php +++ b/app/Traits/ExecuteRemoteCommand.php @@ -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()); } } } diff --git a/app/Traits/SshRetryable.php b/app/Traits/SshRetryable.php index 901e957b2f..537965df12 100644 --- a/app/Traits/SshRetryable.php +++ b/app/Traits/SshRetryable.php @@ -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) diff --git a/tests/Feature/RemoteProcessTimeoutTest.php b/tests/Feature/RemoteProcessTimeoutTest.php index 9bc8e12ae4..bdde810ae8 100644 --- a/tests/Feature/RemoteProcessTimeoutTest.php +++ b/tests/Feature/RemoteProcessTimeoutTest.php @@ -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: 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); +});