From d6f6147c4369a013e2806f24b48b31c15d64f7f7 Mon Sep 17 00:00:00 2001 From: SZWzz <79047567+SZWzz@users.noreply.github.com> Date: Sun, 6 Sep 2026 18:43:56 +0800 Subject: [PATCH] fix(desktop): publish a scalar remote spawn pid --- .../desktop/electron/remote-lifecycle.test.ts | 105 +++++++++++++++++- apps/desktop/electron/remote-lifecycle.ts | 50 ++++++--- 2 files changed, 136 insertions(+), 19 deletions(-) diff --git a/apps/desktop/electron/remote-lifecycle.test.ts b/apps/desktop/electron/remote-lifecycle.test.ts index 9a8c0ba87c33e..77c341545ec52 100644 --- a/apps/desktop/electron/remote-lifecycle.test.ts +++ b/apps/desktop/electron/remote-lifecycle.test.ts @@ -1,5 +1,5 @@ import assert from 'node:assert/strict' -import { exec as execCallback, spawn } from 'node:child_process' +import { exec as execCallback, execFile as execFileCallback, spawn } from 'node:child_process' import { chmod, mkdir, mkdtemp, readFile, rm, symlink, writeFile } from 'node:fs/promises' import os from 'node:os' import path from 'node:path' @@ -11,6 +11,7 @@ import { profileSshOverride } from './connection-config' import { assertRemoteInstallUpdateClear, buildSpawnCommand, + buildSpawnPayload, classifySshReuseProof, cleanupStale, connect, @@ -46,6 +47,7 @@ import { const OWNERSHIP_ID = '0123456789abcdef0123456789abcdef' const SPAWN_NONCE = '0123456789abcdef' const exec = promisify(execCallback) +const execFile = promisify(execFileCallback) test('SSH reuse proof rejects a backend whose runtime was replaced', () => { assert.equal( @@ -1547,8 +1549,8 @@ test('buildSpawnCommand payload variables keep $HOME expandable (no double quoti } }) -test('buildSpawnCommand lockfile publication is POSIX sh (no bash substitution)', () => { - const cmd = buildSpawnCommand('/x/hermes', 'work', { +test('buildSpawnPayload lockfile publication is POSIX sh (no bash substitution)', () => { + const cmd = buildSpawnPayload('/x/hermes', 'work', { hermesHome: '~/.hermes', logPath: spawnLogPath(OWNERSHIP_ID, SPAWN_NONCE), ownershipId: OWNERSHIP_ID, @@ -1560,9 +1562,11 @@ test('buildSpawnCommand lockfile publication is POSIX sh (no bash substitution)' // ${var//pat/rep} is bash-only; dash aborts the payload on it AFTER the // serve was spawned, so the client sees an unknown failure, deletes the - // token file, and orphans the backend. + // token file, and orphans the backend. The sed swap must target only the + // quoted JSON pid field so the published pid is a JSON integer (readLockfile + // rejects a quoted-string pid as malformed-pid skew and fails closed). assert.doesNotMatch(cmd, /\$\{lock_json\/\//, 'must not use ${var//} substitution under sh') - assert.ok(cmd.includes('sed "s/__PID__/${child}/"'), 'pid substitution must use sed') + assert.ok(cmd.includes(`sed 's/"pid":"__PID__"/"pid":'"$child"'/`), 'pid substitution must target the JSON pid field') }) test('buildSpawnCommand scopes umask 077 to the mkdir subshell (no leak into serve)', () => { @@ -1586,6 +1590,97 @@ test('buildSpawnCommand scopes umask 077 to the mkdir subshell (no leak into ser 'scoped mkdir must still precede the serve spawn' ) }) +test.skipIf(process.platform !== 'linux')( + 'buildSpawnPayload publishes a runtime-valid lockfile pid under dash', + async () => { + const directory = await mkdtemp(path.join(os.tmpdir(), 'hermes-remote-payload-runtime-')) + const home = path.join(directory, 'home') + const hermesPath = path.join(directory, 'hermes') + const pidPath = path.join(directory, 'hermes.pid') + const hermesHome = '~/.hermes' + const logPath = spawnLogPath(OWNERSHIP_ID, SPAWN_NONCE) + const lockDirectory = path.join(home, '.hermes', 'desktop-ssh', OWNERSHIP_ID) + const lockPath = path.join(lockDirectory, 'backend.lock.json') + const tokenPath = path.join(lockDirectory, `${SPAWN_NONCE}.token`) + + const payload = buildSpawnPayload(hermesPath, 'work', { + hermesHome, + logPath, + ownershipId: OWNERSHIP_ID, + reservationNonce: SPAWN_NONCE, + spawnNonce: SPAWN_NONCE, + tokenFilePath: spawnTokenPath(OWNERSHIP_ID, SPAWN_NONCE), + lockMetadata: { + ownershipId: OWNERSHIP_ID, + spawnNonce: SPAWN_NONCE, + port: 0, + profile: 'work', + hermesPath, + hermesHome, + logPath, + tokenFingerprint: fingerprintToken('stored-token'), + protocolVersion: PROTOCOL_VERSION, + startedAt: '2026-07-14T00:00:00.000Z' + } + }) + + let hermesPid = 0 + + try { + await mkdir(lockDirectory, { recursive: true, mode: 0o700 }) + await writeFile(tokenPath, 'token', { mode: 0o600 }) + await writeFile(hermesPath, '#!/bin/sh\nprintf \'%s\\n\' "$$" > "$PID_FILE"\nexec sleep 30\n', { mode: 0o700 }) + + const { stdout } = await execFile('dash', ['-c', payload, 'hermes-update-mutex', '3'], { + env: { ...process.env, HOME: home, PID_FILE: pidPath }, + timeout: 10_000 + }) + + assert.match(stdout.trim(), /^[0-9]+$/) + const lock = JSON.parse(await readFile(lockPath, 'utf8')) + assert.equal(Number.isInteger(lock.pid), true) + + for (let attempt = 0; attempt < 40; attempt += 1) { + try { + const candidate = Number((await readFile(pidPath, 'utf8')).trim()) + + if (Number.isInteger(candidate) && candidate > 0) { + hermesPid = candidate + + break + } + } catch (error: any) { + if (error?.code !== 'ENOENT') { + throw error + } + } + + await new Promise(resolve => setTimeout(resolve, 25)) + } + + assert.ok(hermesPid > 0, 'the fake Hermes backend did not start') + assert.equal(lock.pid, hermesPid) + } finally { + if (!hermesPid) { + try { + hermesPid = Number((await readFile(pidPath, 'utf8')).trim()) + } catch { + void 0 + } + } + + if (Number.isInteger(hermesPid) && hermesPid > 0) { + try { + process.kill(hermesPid, 'SIGTERM') + } catch { + void 0 + } + } + + await rm(directory, { recursive: true, force: true }) + } + } +) test('spawnRemoteDashboard removes a token file when upload reporting fails', async () => { const failure = new Error('channel closed') diff --git a/apps/desktop/electron/remote-lifecycle.ts b/apps/desktop/electron/remote-lifecycle.ts index e6c77f8fe68e3..71199c4a6489e 100644 --- a/apps/desktop/electron/remote-lifecycle.ts +++ b/apps/desktop/electron/remote-lifecycle.ts @@ -1106,7 +1106,7 @@ async function terminateOwnedDashboardForUpdate(ssh, expected) { // Detach so the backend survives the SSH channel closing: setsid (Linux) // starts a new session; macOS has no setsid, so fall back to nohup (HUP-immune; // fd-detachment is already handled by /dev/null || true; ` + `exec env HERMES_DESKTOP=1${opts.guestOnboarding === true ? ' HERMES_GUEST_ONBOARDING=1' : ''} ${hermes} ${profileArgs}${subCmd}` - const detachedShell = `eval "exec $1>&-"; ${dashCmd} > ${logPath} 2>&1 & echo $!` + // Keep Hermes in the foreground of the detached setsid/nohup shell. The outer + // shell backgrounds that process and emits the only PID. If this inner shell + // also echoes `$!`, command substitution captures two PIDs and the lockfile + // substitution becomes invalid under POSIX sh. + const detachedShell = `eval "exec $1>&-"; ${dashCmd} > ${logPath} 2>&1` const detachedSpawn = `child=$("$(command -v setsid || echo nohup)" sh -c ${shq(detachedShell)} hermes-update-child "$1" & echo $!)` if (!opts.ownershipId || !opts.lockMetadata) { - return withRemoteUpdateMutex( - `${markerClear}; marker_clear || exit 75; ` + + return { + updateMutex, + payload: + `${markerClear}; marker_clear || exit 75; ` + `mkdir -p "$(dirname ${logPath})" && ` + `${detachedSpawn}; ` + - `marker_clear || { kill "$child" 2>/dev/null || true; wait "$child" 2>/dev/null || true; exit 75; }; echo "$child"`, - updateMutex - ) + `marker_clear || { kill "$child" 2>/dev/null || true; wait "$child" 2>/dev/null || true; exit 75; }; echo "$child"` + } } const reservation = expandRemotePath(connectReservationPath(opts.ownershipId)) @@ -1155,8 +1160,10 @@ function buildSpawnCommand(hermesPath, profile, opts: any = {}) { const metadata = JSON.stringify({ schemaVersion: LOCKFILE_SCHEMA_VERSION, ...opts.lockMetadata, pid: '__PID__' }) const reservationNonce = validateSpawnNonce(opts.reservationNonce || crypto.randomBytes(8).toString('hex')) - return withRemoteUpdateMutex( - `(umask 077 && mkdir -p "$(dirname ${reservation})"); ` + + return { + updateMutex, + payload: + `(umask 077 && mkdir -p "$(dirname ${reservation})"); ` + // reservation/lockPath/ownerPath are expandRemotePath() output — already // shell-quoted fragments ("$HOME"'/…'). Embed raw so the assignment // expands $HOME; shq() here would store the quote characters literally @@ -1175,17 +1182,31 @@ function buildSpawnCommand(hermesPath, profile, opts: any = {}) { `if kill -0 "$existing_pid" 2>/dev/null; then ${tokenPath ? `rm -f ${tokenPath}; ` : ''}printf EXISTING; exit 0; fi; rm -f "$lock";; esac; fi; ` + `${markerClear}; marker_clear || exit 75; mkdir -p "$(dirname ${logPath})" && ` + `${detachedSpawn}; ` + + `case "$child" in ''|*[!0-9]*) exit 76;; esac; ` + `marker_clear || { kill "$child" 2>/dev/null || true; wait "$child" 2>/dev/null || true; exit 75; }; ` + // ${var//pat/rep} is a bashism — this payload runs under plain sh (dash // on Ubuntu), which aborts the whole script on it with "Bad // substitution" AFTER the child was spawned, orphaning the backend and - // skipping the lockfile publication. Substitute with sed instead. - `lock_json=$(printf '%s' ${shq(metadata)} | sed "s/__PID__/\${child}/"); ` + + // skipping the lockfile publication. Substitute with sed instead, and + // replace only the JSON pid field's quoted placeholder so the published + // record carries a real JSON number pid: readLockfile requires an integer + // (malformed-pid skew fails closed otherwise) and the reuse regex only + // matches "pid":. + `lock_json=$(printf '%s' ${shq(metadata)} | sed 's/"pid":"__PID__"/"pid":'"$child"'/') || { kill "$child" 2>/dev/null || true; wait "$child" 2>/dev/null || true; exit 76; }; ` + `temporary_lock="\${lock}.${reservationNonce}.tmp"; ` + `printf '%s' "$lock_json" > "$temporary_lock" && mv -f "$temporary_lock" "$lock" || { kill "$child" 2>/dev/null || true; wait "$child" 2>/dev/null || true; exit 76; }; ` + - `echo "$child"`, - updateMutex - ) + `echo "$child"` + } +} + +function buildSpawnPayload(hermesPath, profile, opts: any = {}) { + return buildSpawnCommandParts(hermesPath, profile, opts).payload +} + +function buildSpawnCommand(hermesPath, profile, opts: any = {}) { + const { updateMutex, payload } = buildSpawnCommandParts(hermesPath, profile, opts) + + return withRemoteUpdateMutex(payload, updateMutex) } async function remoteSupportsSshOwnership(ssh, hermesPath) { @@ -1748,6 +1769,7 @@ export { adoptOwnedServedToken, assertRemoteInstallUpdateClear, buildSpawnCommand, + buildSpawnPayload, classifySshReuseProof, cleanupStale, connect,