fix(desktop): a lost SSH probe answer no longer kills or orphans a live remote backend
The Desktop SSH bootstrap proves the remote `hermes serve --isolated` alive (`kill -0 … && echo ALIVE || echo DEAD`) and owned (argv probe printing OWNED/FOREIGN) over the same SSH channel that is often mid-teardown right after the served token was resolved. Both probes treated ANY answer that was not the positive sentinel as the negative verdict, so an exec that resolved with empty output was read as death: the boot failed with "remote dashboard exited while its served token was being resolved", the post-spawn cleanup ran the ownership probe on the same channel, read the lost answer as FOREIGN, skipped the kill and removed the lockfile — one orphaned ~144 MB backend per failed attempt, ten in one session on a 1 GB host (#111810). `execProbeVerdict` now runs both probes: an answer that is neither sentinel is indeterminate and is retried over a short bounded window (3 attempts, 500 ms); with no definite answer it fails closed with a `transient-transport-error`, so `cleanupStale` keeps the ownership record for the next connect to reap instead of leaving a lockless orphan. The post-spawn catch probes liveness first so a child that genuinely died at startup skips the ownership proof and the original error ("exited before announcing") stays visible. Slimmer redo of #111828 by @kokhlo: same mechanism, without the parallel `verifyRemotePidAlive`/`verifyPidOwnership`/`readOwnershipVerdict` layer that left `remotePidAlive` and the original probe as dead duplicates. Fixes #111810 Co-authored-by: Konstantin Khlopkov <47825603+kokhlo@users.noreply.github.com>
This commit is contained in:
@@ -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')
|
||||
})
|
||||
|
||||
@@ -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
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user