From 1cb9f001f8a1287a5f1bd560bc181e0be8c899c4 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Tue, 18 Aug 2026 23:38:38 +0200 Subject: [PATCH] fix(ssh): preserve sessions when recycling mux connections (#11377) --- app/Helpers/SshMultiplexingHelper.php | 100 ++++++---- .../CleanupStaleMultiplexedConnections.php | 30 +-- config/constants.php | 1 - tests/Feature/SshMultiplexingLockTest.php | 177 +++++++++++++++++- 4 files changed, 246 insertions(+), 62 deletions(-) diff --git a/app/Helpers/SshMultiplexingHelper.php b/app/Helpers/SshMultiplexingHelper.php index 1a9b688f40..cbb18945e2 100644 --- a/app/Helpers/SshMultiplexingHelper.php +++ b/app/Helpers/SshMultiplexingHelper.php @@ -87,15 +87,48 @@ class SshMultiplexingHelper return false; } - self::storeConnectionMetadata($server); - return true; } public static function removeMuxFile(Server $server): void { - Process::run(self::muxControlCommand($server, 'exit')); - self::clearConnectionMetadata($server); + $checkProcess = Process::run(self::muxControlCommand($server, 'check')); + $pid = preg_match('/pid=(\d+)/', $checkProcess->output().$checkProcess->errorOutput(), $matches) + ? $matches[1] + : null; + + if ($pid !== null) { + self::markMuxProcessAsRetiring($pid, self::muxSocket($server)); + } + + $stopProcess = Process::run(self::muxControlCommand($server, 'stop')); + + if ($pid !== null && ! $stopProcess->successful()) { + self::unmarkMuxProcessAsRetiring($pid, self::muxSocket($server)); + } + } + + public static function markMuxProcessAsRetiring(string $pid, string $muxSocket, ?string $processStartTime = null): void + { + $processStartTime ??= self::processStartTime($pid); + Cache::forever(self::muxProcessRetirementKey($pid, $muxSocket, $processStartTime), true); + } + + public static function isMuxProcessRetiring(string $pid, string $muxSocket, ?string $processStartTime = null): bool + { + $processStartTime ??= self::processStartTime($pid); + $key = self::muxProcessRetirementKey($pid, $muxSocket, $processStartTime); + if (! Cache::has($key)) { + return false; + } + + return true; + } + + public static function unmarkMuxProcessAsRetiring(string $pid, string $muxSocket, ?string $processStartTime = null): void + { + $processStartTime ??= self::processStartTime($pid); + Cache::forget(self::muxProcessRetirementKey($pid, $muxSocket, $processStartTime)); } public static function generateScpCommand(Server $server, string $source, string $dest): string @@ -242,25 +275,6 @@ class SshMultiplexingHelper return $process->exitCode() === 0 && str_contains($process->output(), 'health_check_ok'); } - public static function isConnectionExpired(Server $server): bool - { - $connectionAge = self::getConnectionAge($server); - $maxAge = config('constants.ssh.mux_max_age'); - - return $connectionAge !== null && $connectionAge > $maxAge; - } - - public static function getConnectionAge(Server $server): ?int - { - $connectionTime = Cache::get("ssh_mux_connection_time_{$server->uuid}"); - - if ($connectionTime === null) { - return null; - } - - return time() - $connectionTime; - } - public static function refreshMultiplexedConnection(Server $server): bool { self::removeMuxFile($server); @@ -273,6 +287,28 @@ class SshMultiplexingHelper return 'ssh_mux_lock_'.(gethostname() ?: 'unknown').'_'.$server->uuid; } + private static function muxProcessRetirementKey(string $pid, string $muxSocket, ?string $processStartTime): string + { + return 'ssh_mux_retiring_'.hash('sha256', self::processScope().'|'.$pid.'|'.$processStartTime.'|'.$muxSocket); + } + + private static function processScope(): string + { + return (gethostname() ?: 'unknown').'|'.(@readlink('/proc/self/ns/pid') ?: 'unknown'); + } + + private static function processStartTime(string $pid): ?string + { + $stat = @file_get_contents("/proc/{$pid}/stat"); + if ($stat === false || ! preg_match('/^\d+ \(.*\) (.*)$/', trim($stat), $matches)) { + return null; + } + + $fields = preg_split('/\s+/', $matches[1]); + + return $fields[19] ?? null; + } + private static function masterConnectionExists(Server $server): bool { return Process::run(self::muxControlCommand($server, 'check'))->exitCode() === 0; @@ -284,14 +320,6 @@ class SshMultiplexingHelper return false; } - if (self::getConnectionAge($server) === null) { - self::storeConnectionMetadata($server); - } - - if (self::isConnectionExpired($server)) { - return false; - } - if (config('constants.ssh.mux_health_check_enabled') && ! self::isConnectionHealthy($server)) { return false; } @@ -382,14 +410,4 @@ class SshMultiplexingHelper return $options.'-p '.escapeshellarg((string) $server->port).' '; } - - private static function storeConnectionMetadata(Server $server): void - { - Cache::put("ssh_mux_connection_time_{$server->uuid}", time(), config('constants.ssh.mux_persist_time') + 300); - } - - private static function clearConnectionMetadata(Server $server): void - { - Cache::forget("ssh_mux_connection_time_{$server->uuid}"); - } } diff --git a/app/Jobs/CleanupStaleMultiplexedConnections.php b/app/Jobs/CleanupStaleMultiplexedConnections.php index 0d3029c668..69e151d24f 100644 --- a/app/Jobs/CleanupStaleMultiplexedConnections.php +++ b/app/Jobs/CleanupStaleMultiplexedConnections.php @@ -2,8 +2,8 @@ namespace App\Jobs; +use App\Helpers\SshMultiplexingHelper; use App\Models\Server; -use Carbon\Carbon; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Bus\Dispatchable; @@ -51,7 +51,9 @@ class CleanupStaleMultiplexedConnections implements ShouldQueue continue; } - if ($process['etimes'] >= $minAge && ! file_exists($pathMatch[1])) { + if ($process['etimes'] >= $minAge + && ! file_exists($pathMatch[1]) + && ! SshMultiplexingHelper::isMuxProcessRetiring($process['pid'], $pathMatch[1])) { $this->reapOrphan('ssh', $process); } } @@ -169,14 +171,6 @@ class CleanupStaleMultiplexedConnections implements ShouldQueue if ($checkProcess->exitCode() !== 0) { $this->removeMultiplexFile($muxFile, 'connection_check_failed'); - } else { - $muxContent = Storage::disk('ssh-mux')->get($muxFile); - $establishedAt = Carbon::parse(substr($muxContent, 37)); - $expirationTime = $establishedAt->addSeconds(config('constants.ssh.mux_persist_time')); - - if (Carbon::now()->isAfter($expirationTime)) { - $this->removeMultiplexFile($muxFile, 'expired'); - } } } } @@ -216,8 +210,20 @@ class CleanupStaleMultiplexedConnections implements ShouldQueue } $muxSocket = "/var/www/html/storage/app/ssh/mux/{$muxFile}"; - $closeCommand = "ssh -O exit -o ControlPath={$muxSocket} localhost 2>/dev/null"; - Process::run($closeCommand); + $checkProcess = Process::run("ssh -O check -o ControlPath={$muxSocket} localhost"); + $pid = preg_match('/pid=(\d+)/', $checkProcess->output().$checkProcess->errorOutput(), $matches) + ? $matches[1] + : null; + + if ($pid !== null) { + SshMultiplexingHelper::markMuxProcessAsRetiring($pid, $muxSocket); + } + + $closeCommand = "ssh -O stop -o ControlPath={$muxSocket} localhost 2>/dev/null"; + $stopProcess = Process::run($closeCommand); + if ($pid !== null && ! $stopProcess->successful()) { + SshMultiplexingHelper::unmarkMuxProcessAsRetiring($pid, $muxSocket); + } Storage::disk('ssh-mux')->delete($muxFile); Log::info('Removed stale mux file', [ diff --git a/config/constants.php b/config/constants.php index e0aefe8e2b..61d5c7502c 100644 --- a/config/constants.php +++ b/config/constants.php @@ -71,7 +71,6 @@ return [ 'mux_persist_time' => env('SSH_MUX_PERSIST_TIME', 3600), 'mux_health_check_enabled' => env('SSH_MUX_HEALTH_CHECK_ENABLED', true), 'mux_health_check_timeout' => env('SSH_MUX_HEALTH_CHECK_TIMEOUT', 5), - 'mux_max_age' => env('SSH_MUX_MAX_AGE', 1800), // 30 minutes 'mux_lock_ttl' => env('SSH_MUX_LOCK_TTL', 30), // lock auto-release, seconds 'mux_lock_timeout' => env('SSH_MUX_LOCK_TIMEOUT', 10), // max wait for lock, seconds 'mux_orphan_min_age' => env('SSH_MUX_ORPHAN_MIN_AGE', 600), // min process age before reaping orphans, seconds diff --git a/tests/Feature/SshMultiplexingLockTest.php b/tests/Feature/SshMultiplexingLockTest.php index f06e50c1a9..45e150dfab 100644 --- a/tests/Feature/SshMultiplexingLockTest.php +++ b/tests/Feature/SshMultiplexingLockTest.php @@ -87,25 +87,22 @@ it('reuses an existing healthy master without spawning a new one', function () { Process::assertNotRan(fn ($process) => str_contains($process->command, 'ssh -fN')); }); -it('refreshes an expired master before reuse', function () { +it('reuses a healthy master regardless of its absolute age', function () { config([ 'constants.ssh.mux_enabled' => true, 'constants.ssh.mux_health_check_enabled' => false, - 'constants.ssh.mux_max_age' => 10, ]); $server = makeMuxServer(); - Cache::put("ssh_mux_connection_time_{$server->uuid}", time() - 30, 3600); + Cache::put("ssh_mux_connection_time_{$server->uuid}", time() - 7200, 10800); Process::fake([ '*-O check*' => Process::result(exitCode: 0), - '*-O exit*' => Process::result(exitCode: 0), - '*-fN *' => Process::result(exitCode: 0), ]); expect(SshMultiplexingHelper::ensureMultiplexedConnection($server))->toBeTrue(); - Process::assertRan(fn ($process) => str_contains($process->command, 'ssh -O exit')); - Process::assertRan(fn ($process) => str_contains($process->command, 'ssh -fN ')); + Process::assertNotRan(fn ($process) => str_contains($process->command, 'ssh -O stop')); + Process::assertNotRan(fn ($process) => str_contains($process->command, 'ssh -fN ')); }); it('does not spawn a master when the per-server lock is already held', function () { @@ -241,6 +238,101 @@ it('kills only old orphaned ssh masters whose control socket no longer exists', File::delete($liveSocket); }); +it('does not reap an ssh master that is intentionally retiring', function () { + config(['constants.ssh.mux_orphan_reap_enabled' => true]); + $muxDir = storage_path('app/ssh/mux'); + $retiringSocket = $muxDir.'/mux_retiring_'.uniqid(); + SshMultiplexingHelper::markMuxProcessAsRetiring('222', $retiringSocket); + + Process::fake([ + 'ps*' => Process::result(output: "222 1 5000 ssh -fN -o ControlMaster=auto -o ControlPath={$retiringSocket} root@1.2.3.4\n"), + 'kill*' => Process::result(exitCode: 0), + ]); + + $job = new CleanupStaleMultiplexedConnections; + $method = new ReflectionMethod($job, 'cleanupOrphanedSshProcesses'); + $method->setAccessible(true); + $method->invoke($job); + + Process::assertNotRan(fn ($process) => str_contains($process->command, 'kill')); +}); + +it('does not treat a reused pid as retiring', function () { + $socket = storage_path('app/ssh/mux/mux_original'); + + SshMultiplexingHelper::markMuxProcessAsRetiring('222', $socket, '1000'); + + expect(SshMultiplexingHelper::isMuxProcessRetiring('222', $socket, '1000'))->toBeTrue() + ->and(SshMultiplexingHelper::isMuxProcessRetiring('222', $socket, '2000'))->toBeFalse(); +}); + +it('scopes retirement markers to the current host and pid namespace', function () { + $method = new ReflectionMethod(SshMultiplexingHelper::class, 'processScope'); + $method->setAccessible(true); + + expect($method->invoke(null)) + ->toBeString() + ->toStartWith((gethostname() ?: 'unknown').'|'); +}); + +it('keeps a retirement marker for long-running ssh sessions', function () { + $socket = storage_path('app/ssh/mux/mux_retired'); + + SshMultiplexingHelper::markMuxProcessAsRetiring('222', $socket); + $this->travel((int) config('constants.ssh.mux_persist_time') * 2 + 1)->seconds(); + + expect(SshMultiplexingHelper::isMuxProcessRetiring('222', $socket))->toBeTrue(); +}); + +it('reads the process start time used to distinguish pid reuse', function () { + $method = new ReflectionMethod(SshMultiplexingHelper::class, 'processStartTime'); + $method->setAccessible(true); + + expect($method->invoke(null, (string) getmypid()))->toMatch('/^\d+$/'); +}); + +it('marks a successfully stopped mux process as retiring', function () { + $server = makeMuxServer(); + Process::fake([ + '*-O check*' => Process::result(output: 'Master running (pid=222)', exitCode: 0), + '*-O stop*' => Process::result(exitCode: 0), + ]); + + SshMultiplexingHelper::removeMuxFile($server); + + expect(SshMultiplexingHelper::isMuxProcessRetiring('222', "/var/www/html/storage/app/ssh/mux/mux_{$server->uuid}"))->toBeTrue(); +}); + +it('marks a mux process as retiring before stopping it', function () { + $server = makeMuxServer(); + Process::fake([ + '*-O check*' => Process::result(output: 'Master running (pid=555)', exitCode: 0), + '*-O stop*' => function () use ($server) { + expect(SshMultiplexingHelper::isMuxProcessRetiring('555', "/var/www/html/storage/app/ssh/mux/mux_{$server->uuid}"))->toBeTrue(); + + return Process::result(exitCode: 0); + }, + ]); + + SshMultiplexingHelper::removeMuxFile($server); +}); + +it('does not mark a mux process as retiring when stop fails', function () { + $server = makeMuxServer(); + Process::fake([ + '*-O check*' => Process::result(output: 'Master running (pid=444)', exitCode: 0), + '*-O stop*' => function () use ($server) { + expect(SshMultiplexingHelper::isMuxProcessRetiring('444', "/var/www/html/storage/app/ssh/mux/mux_{$server->uuid}"))->toBeTrue(); + + return Process::result(exitCode: 1); + }, + ]); + + SshMultiplexingHelper::removeMuxFile($server); + + expect(SshMultiplexingHelper::isMuxProcessRetiring('444', "/var/www/html/storage/app/ssh/mux/mux_{$server->uuid}"))->toBeFalse(); +}); + it('kills only old orphaned cloudflared proxies whose parent ssh is gone', function () { config(['constants.ssh.mux_orphan_reap_enabled' => true]); @@ -294,7 +386,10 @@ it('removes mux files for non-existent servers when reaping is enabled', functio Storage::fake('ssh-mux'); $file = 'mux_ghost'.uniqid(); Storage::disk('ssh-mux')->put($file, 'x'); - Process::fake(); + Process::fake([ + '*-O check*' => Process::result(errorOutput: 'Master running (pid=333)', exitCode: 0), + '*-O stop*' => Process::result(exitCode: 0), + ]); $job = new CleanupStaleMultiplexedConnections; $method = new ReflectionMethod($job, 'cleanupNonExistentServerConnections'); @@ -302,6 +397,72 @@ it('removes mux files for non-existent servers when reaping is enabled', functio $method->invoke($job); expect(Storage::disk('ssh-mux')->exists($file))->toBeFalse(); + expect(SshMultiplexingHelper::isMuxProcessRetiring('333', "/var/www/html/storage/app/ssh/mux/{$file}"))->toBeTrue(); + Process::assertRan(fn ($process) => str_contains($process->command, 'ssh -O stop')); + Process::assertNotRan(fn ($process) => str_contains($process->command, 'ssh -O exit')); +}); + +it('marks a stale mux process as retiring before stopping it', function () { + config(['constants.ssh.mux_orphan_reap_enabled' => true]); + Storage::fake('ssh-mux'); + $file = 'mux_ghost'.uniqid(); + Storage::disk('ssh-mux')->put($file, 'x'); + Process::fake([ + '*-O check*' => Process::result(output: 'Master running (pid=666)', exitCode: 0), + '*-O stop*' => function () use ($file) { + expect(SshMultiplexingHelper::isMuxProcessRetiring('666', "/var/www/html/storage/app/ssh/mux/{$file}"))->toBeTrue(); + + return Process::result(exitCode: 0); + }, + ]); + + $job = new CleanupStaleMultiplexedConnections; + $method = new ReflectionMethod($job, 'cleanupNonExistentServerConnections'); + $method->setAccessible(true); + $method->invoke($job); +}); + +it('removes a stale mux retirement marker when stopping fails', function () { + config(['constants.ssh.mux_orphan_reap_enabled' => true]); + Storage::fake('ssh-mux'); + $file = 'mux_ghost'.uniqid(); + $muxSocket = "/var/www/html/storage/app/ssh/mux/{$file}"; + Storage::disk('ssh-mux')->put($file, 'x'); + Process::fake([ + '*-O check*' => Process::result(output: 'Master running (pid=777)', exitCode: 0), + '*-O stop*' => function () use ($muxSocket) { + expect(SshMultiplexingHelper::isMuxProcessRetiring('777', $muxSocket))->toBeTrue(); + + return Process::result(exitCode: 1); + }, + ]); + + $job = new CleanupStaleMultiplexedConnections; + $method = new ReflectionMethod($job, 'cleanupNonExistentServerConnections'); + $method->setAccessible(true); + $method->invoke($job); + + expect(SshMultiplexingHelper::isMuxProcessRetiring('777', $muxSocket))->toBeFalse(); +}); + +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'); + $server = makeMuxServer(); + $file = "mux_{$server->uuid}"; + Storage::disk('ssh-mux')->put($file, str_repeat('x', 37).now()->subHours(2)->toIso8601String()); + + Process::fake([ + '*-O check*' => Process::result(exitCode: 0), + ]); + + $job = new CleanupStaleMultiplexedConnections; + $method = new ReflectionMethod($job, 'cleanupStaleConnections'); + $method->setAccessible(true); + $method->invoke($job); + + expect(Storage::disk('ssh-mux')->exists($file))->toBeTrue(); + Process::assertNotRan(fn ($process) => str_contains($process->command, 'ssh -O stop')); }); it('keeps mux files for non-existent servers in dry-run mode', function () {