diff --git a/apps/desktop/electron/remote-lifecycle.test.ts b/apps/desktop/electron/remote-lifecycle.test.ts index 44b1af36d6..eeae7e9014 100644 --- a/apps/desktop/electron/remote-lifecycle.test.ts +++ b/apps/desktop/electron/remote-lifecycle.test.ts @@ -1959,3 +1959,53 @@ test.skipIf(process.platform === 'win32')( } } ) + +// The liveness and ownership probes answer over the same SSH channel that is +// often mid-teardown right after the served token resolved. An exec that +// returns neither sentinel is indeterminate (#111810): read as DEAD it tore +// down a live backend; read as FOREIGN it skipped the reap while removing the +// lockfile — one orphaned `serve --isolated` per failed attempt. +test('connect() does not declare a live dashboard dead when the liveness probe answers nothing once', async () => { + let liveness = 0 + + const ssh = fakeSsh([ + [/uname/, 'Linux\nx86_64'], + [/\[ -x/, 'OK'], + [/cat .*lock\.json/, ''], + [/grep -q ssh-session-token-file/, 'YES\n'], + [/python3 -c/, ''], + [/printf '%s\\n'/, ''], + [/setsid/, '777\n'], + [(cmd: string) => /kill -0 777/.test(cmd) && !cmd.includes('while'), () => (liveness++ === 0 ? '' : 'ALIVE\n')], + [/cat .*\.log/, 'HERMES_DASHBOARD_READY port=51999\n'] + ]) + + const result = await connect(connectDeps(ssh, { platform: { os: 'Linux', arch: 'x86_64' } })) + + assert.equal(result.reused, false) + assert.equal(result.pid, 777) + assert.ok(!ssh.calls.some(c => /(^|[^-\d])kill(?: -\w+)? 777\b/.test(c) && !/kill -0/.test(c)), 'must not reap a live backend') +}) + +test('cleanupStale reaps after one lost ownership answer and keeps the lockfile when none ever settles', async () => { + let ownership = 0 + + const flaky = fakeSsh([ + [/print\("OWNED"/, () => (ownership++ === 0 ? '' : 'OWNED\n')], + [/kill 777 &&/, 'TERMINATED\n'] + ]) + + await cleanupStale(flaky, OWNERSHIP_ID, ownedLock({ pid: 777 })) + assert.ok(flaky.calls.some(c => /kill 777 &&/.test(c)), 'must reap the owned backend') + assert.ok(flaky.calls.some(c => /rm -f .*backend\.lock\.json/.test(c))) + + const silent = fakeSsh([[/print\("OWNED"/, '']]) + + await assert.rejects( + cleanupStale(silent, OWNERSHIP_ID, ownedLock({ pid: 777 })), + (error: any) => error.kind === 'transient-transport-error' + ) + + assert.ok(!silent.calls.some(c => /(^|[^-\d])kill(?: -\w+)? 777\b/.test(c) && !/kill -0/.test(c)), 'must not kill unproven') + assert.ok(!silent.calls.some(c => /rm -f .*backend\.lock\.json/.test(c)), 'record must survive for the next connect to reap') +}) diff --git a/apps/desktop/electron/remote-lifecycle.ts b/apps/desktop/electron/remote-lifecycle.ts index dc39f064c9..04832019b6 100644 --- a/apps/desktop/electron/remote-lifecycle.ts +++ b/apps/desktop/electron/remote-lifecycle.ts @@ -516,21 +516,58 @@ async function removeLockfile(ssh, ownershipId) { } } +const PROBE_VERDICT_ATTEMPTS = 3 +const PROBE_VERDICT_RETRY_MS = 500 + +// Liveness and ownership probes print exactly one of two sentinels. An exec +// that resolves with neither — the channel died before the remote shell ran, +// which is exactly the state of an SSH session mid-teardown right after the +// served token was resolved — is indeterminate, not the negative verdict: +// reading it as DEAD tore down a live backend as "exited while its served +// token was being resolved", and reading it as FOREIGN skipped the reap while +// still removing the lockfile, leaving one orphaned `serve --isolated` per +// attempt (#111810). Retry over a short window; with no definite answer fail +// closed with a transient error so callers keep the ownership record. +async function execProbeVerdict(ssh, command, sentinels, failureMessage) { + for (let attempt = 0; attempt < PROBE_VERDICT_ATTEMPTS; attempt++) { + if (attempt > 0) { + await new Promise(resolve => setTimeout(resolve, PROBE_VERDICT_RETRY_MS)) + } + + let out + + try { + out = String((await ssh.exec(command)) || '').trim() + } catch (cause) { + const error: any = new Error(failureMessage) + error.kind = 'transient-transport-error' + error.cause = cause + throw error + } + + if (sentinels.includes(out)) { + return out + } + } + + const error: any = new Error(failureMessage) + error.kind = 'transient-transport-error' + throw error +} + async function remotePidAlive(ssh, pid) { if (!pid || !Number.isInteger(Number(pid))) { return false } - try { - const out = (await ssh.exec(`kill -0 ${Number(pid)} 2>/dev/null && echo ALIVE || echo DEAD`)).trim() + const verdict = await execProbeVerdict( + ssh, + `kill -0 ${Number(pid)} 2>/dev/null && echo ALIVE || echo DEAD`, + ['ALIVE', 'DEAD'], + 'Could not verify the SSH backend process.' + ) - return out === 'ALIVE' - } catch (cause) { - const error: any = new Error('Could not verify the SSH backend process.') - error.kind = 'transient-transport-error' - error.cause = cause - throw error - } + return verdict === 'ALIVE' } // Stable kernel process-start identity used to fence a later managed-update @@ -587,8 +624,7 @@ async function pidIsOurDashboard( return false } - try { - const script = + const script = 'import os,shlex,subprocess,sys\n' + `pid=${Number(pid)}\n` + `expected=os.path.expanduser(${shq(hermesPath)})\n` + @@ -635,15 +671,14 @@ async function pidIsOurDashboard( 'except (ValueError,IndexError):pass\n' + 'print("OWNED" if ok else "FOREIGN")' - const out = await ssh.exec(`python3 -c ${shq(script)}`) + const verdict = await execProbeVerdict( + ssh, + `python3 -c ${shq(script)}`, + ['OWNED', 'FOREIGN'], + 'Could not verify SSH backend process ownership.' + ) - return String(out || '').trim() === 'OWNED' - } catch (cause) { - const error: any = new Error('Could not verify SSH backend process ownership.') - error.kind = 'transient-transport-error' - error.cause = cause - throw error - } + return verdict === 'OWNED' } // Kill the stale dashboard ONLY if provably ours, then drop the lockfile. @@ -1660,7 +1695,12 @@ async function connect(deps) { void 0 } - await cleanupStale(ssh, ownershipId, ownedSpawn) + // This record IS the child this attempt spawned. A liveness probe that + // cannot be settled must not become "leave it running": assume alive so + // cleanupStale re-runs the ownership proof, which keeps the record when + // nothing can be proven and lets the next connect reap by exact ownership. + const pidAlive = await remotePidAlive(ssh, pid).catch(() => true) + await cleanupStale(ssh, ownershipId, ownedSpawn, pidAlive) throw error } }