fix(ssh): bound mux control checks and replace wedged masters (#12241)

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

app/Helpers/SshMultiplexingHelper.php

@@ -5,7 +5,10 @@ namespace App\Helpers;
 use App\Models\PrivateKey;
 use App\Models\Server;
 use Illuminate\Contracts\Cache\LockTimeoutException;
+use Illuminate\Contracts\Process\ProcessResult;
+use Illuminate\Process\Exceptions\ProcessTimedOutException;
 use Illuminate\Support\Facades\Cache;
+use Illuminate\Support\Facades\File;
 use Illuminate\Support\Facades\Hash;
 use Illuminate\Support\Facades\Log;
 use Illuminate\Support\Facades\Process;
@@ -42,10 +45,17 @@ class SshMultiplexingHelper
                     return true;
                 }
 
-                if (self::masterConnectionExists($server)) {
+                $check = self::runMuxControlCommand($server, 'check');
+                if ($check?->successful()) {
                     return self::refreshMultiplexedConnection($server);
                 }
 
+                // A wedged master accepts on its socket but never answers, and a new
+                // `ssh -fN` would hang on it too. Remove the socket so a fresh master can bind.
+                if ($check === null) {
+                    File::delete(self::muxSocket($server));
+                }
+
                 return self::establishNewMultiplexedConnection($server);
             });
         } catch (LockTimeoutException) {
@@ -104,8 +114,8 @@ class SshMultiplexingHelper
 
     public static function removeMuxFile(Server $server): void
     {
-        $checkProcess = Process::run(self::muxControlCommand($server, 'check'));
-        $pid = preg_match('/pid=(\d+)/', $checkProcess->output().$checkProcess->errorOutput(), $matches)
+        $checkProcess = self::runMuxControlCommand($server, 'check');
+        $pid = $checkProcess && preg_match('/pid=(\d+)/', $checkProcess->output().$checkProcess->errorOutput(), $matches)
             ? $matches[1]
             : null;
 
@@ -113,11 +123,19 @@ class SshMultiplexingHelper
             self::markMuxProcessAsRetiring($pid, self::muxSocket($server));
         }
 
-        $stopProcess = Process::run(self::muxControlCommand($server, 'stop'));
+        $stopProcess = self::runMuxControlCommand($server, 'stop');
 
-        if ($pid !== null && ! $stopProcess->successful()) {
+        if ($stopProcess?->successful()) {
+            return;
+        }
+
+        if ($pid !== null) {
             self::unmarkMuxProcessAsRetiring($pid, self::muxSocket($server));
         }
+
+        // A master that does not stop must not stay reusable under the old user, IP, or port.
+        // Without its socket it is an orphan, and the cleanup job reaps it.
+        File::delete(self::muxSocket($server));
     }
 
     public static function markMuxProcessAsRetiring(string $pid, string $muxSocket, ?string $processStartTime = null): void
@@ -300,17 +318,17 @@ class SshMultiplexingHelper
     {
         $sshConfig = self::serverSshConfiguration($server);
         $muxSocket = $sshConfig['muxFilename'];
-        $healthCheckTimeout = config('constants.ssh.mux_health_check_timeout');
-
-        $healthCommand = "timeout $healthCheckTimeout ssh -o ControlMaster=auto -o ControlPath=".escapeshellarg($muxSocket).' ';
+        $healthCommand = 'ssh -o ControlMaster=auto -o ControlPath='.escapeshellarg($muxSocket).' ';
         if (data_get($server, 'settings.is_cloudflare_tunnel')) {
             $healthCommand .= '-o ProxyCommand="cloudflared access ssh --hostname %h" ';
         }
         $healthCommand .= self::escapedUserAtHost($server)." 'echo \"health_check_ok\"'";
 
-        $process = Process::run($healthCommand);
+        // The health check goes over the network, so a slow link gets at least the server's connection timeout.
+        $timeout = max((int) config('constants.ssh.mux_health_check_timeout'), self::getConnectionTimeout($server));
+        $process = self::runWithTimeout($healthCommand, $timeout);
 
-        return $process->exitCode() === 0 && str_contains($process->output(), 'health_check_ok');
+        return $process?->exitCode() === 0 && str_contains($process->output(), 'health_check_ok');
     }
 
     public static function refreshMultiplexedConnection(Server $server): bool
@@ -349,7 +367,36 @@ class SshMultiplexingHelper
 
     private static function masterConnectionExists(Server $server): bool
     {
-        return Process::run(self::muxControlCommand($server, 'check'))->exitCode() === 0;
+        return self::runMuxControlCommand($server, 'check')?->successful() === true;
+    }
+
+    /**
+     * Run `ssh -O <operation>` against a mux socket file. Returns null when the master does not answer in time.
+     */
+    public static function runMuxSocketCommand(string $muxSocket, string $operation): ?ProcessResult
+    {
+        return self::runWithTimeout(
+            "ssh -O {$operation} -o ControlPath=".escapeshellarg($muxSocket).' localhost',
+            (int) config('constants.ssh.mux_health_check_timeout'),
+        );
+    }
+
+    private static function runMuxControlCommand(Server $server, string $operation): ?ProcessResult
+    {
+        return self::runWithTimeout(self::muxControlCommand($server, $operation), (int) config('constants.ssh.mux_health_check_timeout'));
+    }
+
+    /**
+     * Use a PHP-side timeout, not the `timeout` binary: BusyBox `timeout` in the Coolify image
+     * kills ssh with SIGTERM, and Symfony turns that into an exception. Returns null on timeout.
+     */
+    private static function runWithTimeout(string $command, int $timeout): ?ProcessResult
+    {
+        try {
+            return Process::timeout($timeout)->run($command);
+        } catch (ProcessTimedOutException) {
+            return null;
+        }
     }
 
     private static function connectionIsReusable(Server $server): bool

app/Jobs/CleanupStaleMultiplexedConnections.php

@@ -81,9 +81,9 @@ class CleanupStaleMultiplexedConnections implements ShouldQueue
             return null;
         }
 
-        $check = Process::run('timeout 10 ssh -O check -o ControlPath='.escapeshellarg($muxSocket).' localhost');
+        $check = SshMultiplexingHelper::runMuxSocketCommand($muxSocket, 'check');
 
-        return preg_match('/pid=(\d+)/', $check->output().$check->errorOutput(), $matches)
+        return $check && preg_match('/pid=(\d+)/', $check->output().$check->errorOutput(), $matches)
             ? $matches[1]
             : false;
     }
@@ -227,10 +227,7 @@ class CleanupStaleMultiplexedConnections implements ShouldQueue
             }
 
             $muxSocket = "/var/www/html/storage/app/ssh/mux/{$muxFile}";
-            $checkCommand = 'ssh -O check -o ControlPath='.escapeshellarg($muxSocket).' '.escapeshellarg($server->user).'@'.escapeshellarg($server->ip).' 2>/dev/null';
-            $checkProcess = Process::run($checkCommand);
-
-            if ($checkProcess->exitCode() !== 0) {
+            if (! SshMultiplexingHelper::runMuxSocketCommand($muxSocket, 'check')?->successful()) {
                 $this->removeMultiplexFile($muxFile, 'connection_check_failed');
             }
         }
@@ -272,8 +269,8 @@ class CleanupStaleMultiplexedConnections implements ShouldQueue
         }
 
         $muxSocket = "/var/www/html/storage/app/ssh/mux/{$muxFile}";
-        $checkProcess = Process::run('ssh -O check -o ControlPath='.escapeshellarg($muxSocket).' localhost');
-        $pid = preg_match('/pid=(\d+)/', $checkProcess->output().$checkProcess->errorOutput(), $matches)
+        $checkProcess = SshMultiplexingHelper::runMuxSocketCommand($muxSocket, 'check');
+        $pid = $checkProcess && preg_match('/pid=(\d+)/', $checkProcess->output().$checkProcess->errorOutput(), $matches)
             ? $matches[1]
             : null;
 
@@ -281,9 +278,8 @@ class CleanupStaleMultiplexedConnections implements ShouldQueue
             SshMultiplexingHelper::markMuxProcessAsRetiring($pid, $muxSocket);
         }
 
-        $closeCommand = 'ssh -O stop -o ControlPath='.escapeshellarg($muxSocket).' localhost 2>/dev/null';
-        $stopProcess = Process::run($closeCommand);
-        if ($pid !== null && ! $stopProcess->successful()) {
+        $stopProcess = SshMultiplexingHelper::runMuxSocketCommand($muxSocket, 'stop');
+        if ($pid !== null && ! $stopProcess?->successful()) {
             SshMultiplexingHelper::unmarkMuxProcessAsRetiring($pid, $muxSocket);
         }
         Storage::disk('ssh-mux')->delete($muxFile);

tests/Feature/SshMultiplexingLockTest.php

@@ -6,10 +6,14 @@ use App\Models\PrivateKey;
 use App\Models\Server;
 use App\Models\User;
 use Illuminate\Foundation\Testing\RefreshDatabase;
+use Illuminate\Process\Exceptions\ProcessTimedOutException;
+use Illuminate\Process\ProcessResult;
 use Illuminate\Support\Facades\Cache;
 use Illuminate\Support\Facades\File;
 use Illuminate\Support\Facades\Process;
 use Illuminate\Support\Facades\Storage;
+use Symfony\Component\Process\Exception\ProcessTimedOutException as SymfonyTimeoutException;
+use Symfony\Component\Process\Process as SymfonyProcess;
 
 /**
  * Tests for the explicit per-server mux lock that prevents concurrent workers
@@ -55,6 +59,25 @@ function makeMuxServer(): Server
     return $server;
 }
 
+/**
+ * A fake process that times out. Laravel does not record a fake that throws, so the
+ * timeout of each call goes into $timeouts.
+ *
+ * @param  array<int, int|float|null>  $timeouts
+ */
+function muxProcessTimeout(array &$timeouts = []): Closure
+{
+    return function ($pendingProcess) use (&$timeouts) {
+        $timeouts[] = $pendingProcess->timeout;
+        $process = new SymfonyProcess(['ssh']);
+
+        throw new ProcessTimedOutException(
+            new SymfonyTimeoutException($process, SymfonyTimeoutException::TYPE_GENERAL),
+            new ProcessResult($process),
+        );
+    };
+}
+
 it('establishes a master with ssh -fN and never the orphan-prone ssh -fNM', function () {
     config(['constants.ssh.mux_enabled' => true]);
     $server = makeMuxServer();
@@ -105,6 +128,113 @@ it('reuses a healthy master regardless of its absolute age', function () {
     Process::assertNotRan(fn ($process) => str_contains($process->command, 'ssh -fN '));
 });
 
+it('bounds mux check commands so a wedged master cannot block callers', function () {
+    config([
+        'constants.ssh.mux_enabled' => true,
+        'constants.ssh.mux_health_check_timeout' => 5,
+    ]);
+    $server = makeMuxServer();
+
+    Process::fake([
+        '*-O check*' => Process::result(exitCode: 1),
+        '*-fN *' => Process::result(exitCode: 0),
+    ]);
+
+    SshMultiplexingHelper::ensureMultiplexedConnection($server);
+
+    Process::assertRan(fn ($process) => str_contains($process->command, '-O check'));
+    Process::assertNotRan(fn ($process) => str_contains($process->command, '-O check') && $process->timeout !== 5);
+});
+
+it('replaces a wedged master whose control check times out', function () {
+    config([
+        'constants.ssh.mux_enabled' => true,
+        'constants.ssh.mux_health_check_timeout' => 5,
+    ]);
+    $server = makeMuxServer();
+    $muxSocket = "/var/www/html/storage/app/ssh/mux/mux_{$server->uuid}";
+
+    File::partialMock()->shouldReceive('delete')->once()->with($muxSocket)->andReturnTrue();
+    $timeouts = [];
+    Process::fake([
+        '*-O check*' => muxProcessTimeout($timeouts),
+        '*-fN *' => Process::result(exitCode: 0),
+    ]);
+
+    expect(SshMultiplexingHelper::ensureMultiplexedConnection($server))->toBeTrue()
+        ->and($timeouts)->each->toBe(5);
+
+    Process::assertRan(fn ($process) => str_contains($process->command, 'ssh -fN '));
+    Process::assertNotRan(fn ($process) => str_contains($process->command, '-O stop'));
+});
+
+it('does not delete the control socket when the master is simply absent', function () {
+    config(['constants.ssh.mux_enabled' => true]);
+    $server = makeMuxServer();
+
+    File::partialMock()->shouldNotReceive('delete');
+    Process::fake([
+        '*-O check*' => Process::result(exitCode: 255),
+        '*-fN *' => Process::result(exitCode: 0),
+    ]);
+
+    expect(SshMultiplexingHelper::ensureMultiplexedConnection($server))->toBeTrue();
+});
+
+it('bounds the control commands used to stop a mux master', function () {
+    config(['constants.ssh.mux_health_check_timeout' => 5]);
+    $server = makeMuxServer();
+    Process::fake([
+        '*-O check*' => Process::result(output: 'Master running (pid=222)', exitCode: 0),
+        '*-O stop*' => Process::result(exitCode: 0),
+    ]);
+
+    SshMultiplexingHelper::removeMuxFile($server);
+
+    Process::assertRan(fn ($process) => str_contains($process->command, 'ssh -O check') && $process->timeout === 5);
+    Process::assertRan(fn ($process) => str_contains($process->command, 'ssh -O stop') && $process->timeout === 5);
+});
+
+it('replaces a master whose health check times out', function () {
+    config([
+        'constants.ssh.mux_enabled' => true,
+        'constants.ssh.mux_health_check_enabled' => true,
+    ]);
+    $server = makeMuxServer();
+
+    Process::fake([
+        '*-O check*' => Process::result(output: 'Master running (pid=222)', exitCode: 0),
+        '*health_check_ok*' => muxProcessTimeout(),
+        '*-O stop*' => Process::result(exitCode: 0),
+        '*-fN *' => Process::result(exitCode: 0),
+    ]);
+
+    expect(SshMultiplexingHelper::ensureMultiplexedConnection($server))->toBeTrue();
+
+    Process::assertRan(fn ($process) => str_contains($process->command, 'ssh -O stop'));
+    Process::assertRan(fn ($process) => str_contains($process->command, 'ssh -fN '));
+});
+
+it('gives the mux health check at least the server connection timeout', function () {
+    config([
+        'constants.ssh.mux_enabled' => true,
+        'constants.ssh.mux_health_check_enabled' => true,
+        'constants.ssh.mux_health_check_timeout' => 5,
+    ]);
+    $server = makeMuxServer();
+    $server->settings->update(['connection_timeout' => 30]);
+
+    Process::fake([
+        '*-O check*' => Process::result(exitCode: 0),
+        '*health_check_ok*' => Process::result(output: 'health_check_ok', exitCode: 0),
+    ]);
+
+    expect(SshMultiplexingHelper::ensureMultiplexedConnection($server->fresh()))->toBeTrue();
+
+    Process::assertRan(fn ($process) => str_contains($process->command, 'health_check_ok') && $process->timeout === 30);
+    Process::assertNotRan(fn ($process) => str_contains($process->command, '-O check') && $process->timeout !== 5);
+});
+
 it('does not spawn a master when the per-server lock is already held', function () {
     config([
         'constants.ssh.mux_enabled' => true,
@@ -418,6 +548,33 @@ it('does not mark a mux process as retiring when stop fails', function () {
     expect(SshMultiplexingHelper::isMuxProcessRetiring('444', "/var/www/html/storage/app/ssh/mux/mux_{$server->uuid}"))->toBeFalse();
 });
 
+it('removes the control socket when the mux master does not stop', function (bool $stopTimesOut) {
+    $server = makeMuxServer();
+    File::partialMock()->shouldReceive('delete')->once()->with("/var/www/html/storage/app/ssh/mux/mux_{$server->uuid}");
+    Process::fake([
+        '*-O check*' => Process::result(output: 'Master running (pid=888)', exitCode: 0),
+        '*-O stop*' => $stopTimesOut ? muxProcessTimeout() : Process::result(exitCode: 255),
+    ]);
+
+    SshMultiplexingHelper::removeMuxFile($server);
+
+    expect(SshMultiplexingHelper::isMuxProcessRetiring('888', "/var/www/html/storage/app/ssh/mux/mux_{$server->uuid}"))->toBeFalse();
+})->with([
+    'stop fails' => false,
+    'stop times out' => true,
+]);
+
+it('keeps the control socket untouched when the mux master stops', function () {
+    $server = makeMuxServer();
+    File::partialMock()->shouldNotReceive('delete');
+    Process::fake([
+        '*-O check*' => Process::result(output: 'Master running (pid=999)', exitCode: 0),
+        '*-O stop*' => Process::result(exitCode: 0),
+    ]);
+
+    SshMultiplexingHelper::removeMuxFile($server);
+});
+
 it('kills only old cloudflared proxies whose stdin pipe no live ssh holds', function () {
     config(['constants.ssh.mux_orphan_reap_enabled' => true]);
 
@@ -544,6 +701,58 @@ it('removes a stale mux retirement marker when stopping fails', function () {
     expect(SshMultiplexingHelper::isMuxProcessRetiring('777', $muxSocket))->toBeFalse();
 });
 
+it('removes a mux connection whose control check times out during cleanup', function () {
+    config([
+        'constants.ssh.mux_orphan_reap_enabled' => true,
+        'constants.ssh.mux_health_check_timeout' => 5,
+    ]);
+    Storage::fake('ssh-mux');
+    $server = makeMuxServer();
+    $file = "mux_{$server->uuid}";
+    Storage::disk('ssh-mux')->put($file, 'x');
+
+    $timeouts = [];
+    Process::fake([
+        '*-O check*' => muxProcessTimeout($timeouts),
+        '*-O stop*' => muxProcessTimeout($timeouts),
+    ]);
+
+    $job = new CleanupStaleMultiplexedConnections;
+    $method = new ReflectionMethod($job, 'cleanupStaleConnections');
+    $method->invoke($job);
+
+    expect(Storage::disk('ssh-mux')->exists($file))->toBeFalse()
+        ->and($timeouts)->toBe([5, 5, 5]);
+    Process::assertNotRan(fn ($process) => str_contains($process->command, $server->ip));
+});
+
+it('does not reap ssh processes when the socket owner check times out', function () {
+    config([
+        'constants.ssh.mux_orphan_reap_enabled' => true,
+        'constants.ssh.mux_health_check_timeout' => 5,
+    ]);
+    $muxDir = storage_path('app/ssh/mux');
+    File::ensureDirectoryExists($muxDir);
+    $socket = $muxDir.'/mux_wedged_'.uniqid();
+    File::put($socket, 'x');
+
+    $timeouts = [];
+    Process::fake([
+        'ps*' => Process::result(output: "111 1 5000 ssh -fN -o ControlMaster=auto -o ControlPath={$socket} root@1.2.3.4\n"),
+        '*ssh -O check*' => muxProcessTimeout($timeouts),
+        'kill*' => Process::result(exitCode: 0),
+    ]);
+
+    $job = new CleanupStaleMultiplexedConnections;
+    $method = new ReflectionMethod($job, 'cleanupOrphanedSshProcesses');
+    $method->invoke($job);
+
+    expect($timeouts)->toBe([5]);
+    Process::assertNotRan(fn ($process) => str_contains($process->command, 'kill'));
+
+    File::delete($socket);
+});
+
 it('does not remove a healthy mux connection based on its absolute age', function () {
     config(['constants.ssh.mux_orphan_reap_enabled' => true]);
     Storage::fake('ssh-mux');