From 697087f2eba75b25ae57ee32c4a86337efd125ed Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 26 Aug 2026 07:53:20 -0700 Subject: [PATCH] fix(desktop): SIGKILL-escalate the owned SSH backend when it survives the graceful quit wait (#91668 remainder) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #95085 quit teardown kills the owned serve --isolated before the SSH tunnel closes, but a backend mid-turn (in-flight LLM call, live MCP children) can ride out SIGTERM past cleanupStale's 5s graceful wait. The old code then gave up (threw, kept the lockfile) and before-quit's 6s race closed SSH anyway — reparenting the still-running serve to pid 1: the reported leak, now specific to quit-during-active-turn. Escalate to kill -9 with a confirmed-exit wait; only an unkillable pid (D-state, permissions) still throws and preserves the lock record so the next connect's reap pass retries. --- .../desktop/electron/remote-lifecycle.test.ts | 49 +++++++++++++++++++ apps/desktop/electron/remote-lifecycle.ts | 26 ++++++++-- 2 files changed, 70 insertions(+), 5 deletions(-) diff --git a/apps/desktop/electron/remote-lifecycle.test.ts b/apps/desktop/electron/remote-lifecycle.test.ts index bfe7d89891..832be8836a 100644 --- a/apps/desktop/electron/remote-lifecycle.test.ts +++ b/apps/desktop/electron/remote-lifecycle.test.ts @@ -1365,3 +1365,52 @@ test('remote SSH ownership capability requires both secure bootstrap flags', asy const unsupported = fakeSsh([[/serve --help/, 'NO\n']]) assert.equal(await remoteSupportsSshOwnership(unsupported, '/x/hermes'), false) }) + +test('cleanupStale escalates to SIGKILL when the backend survives the graceful wait (#91668 quit-during-active-turn)', async () => { + // A serve mid-turn (in-flight LLM call, live MCP children) can ride out + // SIGTERM well past the 5s graceful wait. Before-quit races the whole + // teardown against 6s and then closes SSH — so a give-up here reparents + // the still-running backend to pid 1: exactly the #91668 leak. The + // graceful-wait failure must escalate to SIGKILL and still drop the lock. + const ssh = fakeSsh([ + [/print\("OWNED"/, 'OWNED\n'], + [(cmd: string) => /kill 9 &&/.test(cmd), new Error('exit 1: pid alive after graceful wait')] + ]) + + await cleanupStale(ssh, OWNERSHIP_ID, { + pid: 9, + spawnNonce: SPAWN_NONCE, + hermesPath: '/x/hermes', + logPath: spawnLogPath(OWNERSHIP_ID, SPAWN_NONCE) + }) + + assert.ok( + ssh.calls.some(c => /kill -9 9\b/.test(c)), + 'must escalate to SIGKILL after the graceful wait fails' + ) + assert.ok( + ssh.calls.some(c => /rm -f .*backend\.lock\.json/.test(c)), + 'lockfile must still be dropped after the forced kill' + ) +}) + +test('cleanupStale keeps the lockfile when even SIGKILL cannot confirm the pid died', async () => { + const ssh = fakeSsh([ + [/print\("OWNED"/, 'OWNED\n'], + [(cmd: string) => /kill 9 &&/.test(cmd), new Error('exit 1: pid alive after graceful wait')], + [(cmd: string) => /kill -9 9\b/.test(cmd), new Error('exit 1: unkillable (D-state)')] + ]) + + await assert.rejects( + cleanupStale(ssh, OWNERSHIP_ID, { + pid: 9, + spawnNonce: SPAWN_NONCE, + hermesPath: '/x/hermes', + logPath: spawnLogPath(OWNERSHIP_ID, SPAWN_NONCE) + }), + /Could not terminate/ + ) + + // The record must survive so the next connect's reap pass retries. + assert.ok(!ssh.calls.some(c => /rm -f .*backend\.lock\.json/.test(c))) +}) diff --git a/apps/desktop/electron/remote-lifecycle.ts b/apps/desktop/electron/remote-lifecycle.ts index fbc4b5cf25..b671de2b3d 100644 --- a/apps/desktop/electron/remote-lifecycle.ts +++ b/apps/desktop/electron/remote-lifecycle.ts @@ -506,11 +506,27 @@ async function cleanupStale(ssh, ownershipId, lock, pidAlive = true) { ).trim() void result - } catch (cause) { - const error: any = new Error('Could not terminate the stale SSH backend.') - error.kind = 'transient-transport-error' - error.cause = cause - throw error + } catch { + // A backend mid-turn (in-flight LLM call, live MCP children) can ride + // out SIGTERM past the 5s graceful wait — and before-quit races this + // whole teardown against 6s before closing SSH, so giving up here + // reparents the still-running serve to pid 1: the #91668 leak, now on + // the quit-during-active-turn path. Escalate to SIGKILL and require a + // confirmed exit before treating the record as reclaimed. + try { + await ssh.exec( + `kill -9 ${Number(lock.pid)} 2>/dev/null; ` + + `i=0; while kill -0 ${Number(lock.pid)} 2>/dev/null; do ` + + `i=$((i+1)); [ "$i" -ge 20 ] && exit 1; sleep 0.1; done` + ) + } catch (cause) { + // Even SIGKILL could not confirm death (D-state, permissions). Keep + // the lockfile so the next connect's reap pass retries. + const error: any = new Error('Could not terminate the stale SSH backend.') + error.kind = 'transient-transport-error' + error.cause = cause + throw error + } } }