From 50cfbfd165c04099518c6fa872747ab03045e9dc Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Date: Fri, 11 Sep 2026 16:27:52 +0200 Subject: [PATCH] (sessions): probe descriptor liveness on both sides, verify the pid before a tmux attach A killed local CLI leaves its descriptor behind and read as busy until the app restarted; getStatus now re-probes the pid lazily, throttled. The remote inventory prints an ALIVE marker per descriptor from /proc so dead CLIs are dropped without a second ssh. The attach probe checks /proc//cmdline is a claude CLI before attaching to a recycled pid, and probe/restore ssh calls get ConnectTimeout=5. Comment sweep on the activity files merged in #259. --- .ai/contexts/cli-session-state.md | 11 +++++ .ai/contexts/session-cache.md | 72 ++++++++++++++++++++++++++++- cli-session-state.js | 41 +++++++++++----- public/app.js | 6 +-- public/remote-activity-ui.js | 7 +-- public/session-activity.js | 14 +----- public/sidebar.js | 2 - remote-attach.js | 26 +++++++++-- remote-transport.js | 33 ++++++++++--- test/cli-session-state.test.js | 41 ++++++++++++++++ test/remote-attach.test.js | 71 ++++++++++++++++++++++++++++ test/remote-transport-shell.test.js | 45 ++++++++++++++++-- test/remote-transport.test.js | 47 +++++++++++++++++-- 13 files changed, 360 insertions(+), 56 deletions(-) diff --git a/.ai/contexts/cli-session-state.md b/.ai/contexts/cli-session-state.md index b0ad2b7c..231d5c5d 100644 --- a/.ai/contexts/cli-session-state.md +++ b/.ai/contexts/cli-session-state.md @@ -159,6 +159,17 @@ session. Both `seed()` and `handleFile()` apply this gate before writing to `statusBySession`; `handleFile()` also deletes the entry outright once the liveness check fails, same as it does when the file itself disappears. +**That gate only runs on a file event — a CLI killed without a clean exit +writes no such event, so its last status stayed cached forever until this app +restarted (F2, audit-fable-2026-09-11).** `getStatus(sessionId)` now keeps the +`pid` alongside the cached `{status, statusUpdatedAt}` and re-probes +`isProcessAlive(pid)` itself, lazily, throttled to once per +`GET_STATUS_PROBE_THROTTLE_MS` (5 s) per sessionId (`now()` is injected so +tests use a fake clock instead of real delays) — a dead pid deletes the entry +and the call returns `undefined`, same as a file event would have done. This +still never touches disk and never arms `onIdle` — "the one invariant" above +is unchanged, it is a read-path liveness check, not a new trigger. + ## Canary tests `test/canary-*.test.js` is a convention this module introduces. A canary diff --git a/.ai/contexts/session-cache.md b/.ai/contexts/session-cache.md index 31bd52c2..f4faa858 100644 --- a/.ai/contexts/session-cache.md +++ b/.ai/contexts/session-cache.md @@ -434,6 +434,24 @@ untouched. plain array to `{ files, sessions }`; every stub of `transport.listFiles` across `remote-mirror.test.js`, `remote-index.test.js` and `remote-indexing-e2e.test.js` was updated to match in the same change. + - **A descriptor's pid is checked for liveness on the host itself (F9, + audit-fable-2026-09-11)** — until this fix `parseSessions` kept every + descriptor unconditionally, so a killed remote CLI's file (deleted only on + a clean exit, same as the local one — see `.ai/contexts/cli-session-state.md`) + surfaced as permanently live, and `sidebar.js`'s host-dot `liveCount` + counted it. `LIST_COMMAND`'s per-file loop now emits one more line after + each descriptor: `printf '\002ALIVE:%s\n' "$( [ -d "/proc/$pid" ] && echo + 1 || echo 0 )"`, `$pid` taken from the filename via `basename "$f" .json` + — no second ssh round trip. `parseSessions` matches that exact + `ALIVE:0`/`ALIVE:1` line immediately following a descriptor, + consumes it either way (so it can never itself be mis-parsed as a bogus + descriptor), drops the descriptor on `ALIVE:0`, and counts the drops in + its returned `dropped` field; `listFiles` logs `dropped N dead session + descriptor(s)` when non-zero. **Backward compatible by construction, not + by a version check**: a descriptor with no marker line following it (an + older host script, or simply the last line of the block) is kept exactly + as before — the absence of the marker is the compatibility signal, there + is no protocol version field. - **`remote-index.js` keeps the latest descriptors per alias, keyed and pruned exactly like folder keys.** `createRemoteIndexer()`'s private `remoteSessions` map is set from `result.sessions` inside `refreshHost()` @@ -659,6 +677,58 @@ Launching a new remote session (#222) and injection over the messaging socket a no-op on real data; it only changes routing for a hand-built or malformed descriptor. +- **Pid-reuse guard (F6, audit-fable-2026-09-11).** Descriptors now survive a + failed refresh cycle for up to the backoff window (#255, above) — long + enough for the CLI to die and the OS to hand its pid to an unrelated + process on the same host. Before this fix, `buildProbeCommand` read + *whatever* process now holds that pid's `TMUX` environment variable and, if + it was solo, reconfigured and attached to whatever tmux session that + process happened to be in — no check that it was still the session the + descriptor named. `buildProbeCommand` now appends one more + `PROBE_SEP`-delimited segment: `tr '\0' ' ' < /proc//cmdline | grep -qi + claude && echo 1 || echo 0` (`buildProcCmdlineCheck`), read back by + `parseDiscoveryProbeOutput` as `cmdlineHasClaude` (`true`/`false`, or `null` + for a probe predating this segment). `attach()` runs this check only when + `descriptor.procStart != null`; on a `false` result it returns `{ ok: + false, error: 'pid no longer belongs to this session (process start + differs)' }` before any `spawnPty` call — no attach, no `set`. A descriptor + with no `procStart` keeps today's unverified behavior. + **Not what the finding asked for, and why:** the audit wanted + `descriptor.procStart` compared against `/proc//stat`'s starttime + (field 22, clock ticks since boot). The one measured `procStart` sample + this repo has (`.ai/contexts/cli-session-state.md`) is + `"134319945380279381"`, captured on a **Windows** CLI — 18 digits, the + right order of magnitude for a Windows `FILETIME` (100 ns since 1601), not + for Linux clock ticks since boot (which would need centuries of uptime to + reach 18 digits at 100 Hz). Whether the CLI's own remote/Linux code path + produces something on the *same* scale as `/proc//stat` field 22 is + unverified — ssh access to a real host to check was out of scope for this + fix (`no ssh to any host` was a hard constraint). Comparing two values on + possibly-incompatible scales risks shipping a check that either always + refuses (units never line up) or silently never refuses (units happen to + overlap by coincidence) — worse than the cmdline check in both directions. + The `cmdline`-contains-`claude` check is strictly weaker than an exact + start-time match (it would not catch a *second* claude CLI reusing the + pid), but it does catch the audited scenario — pid reused by an unrelated + process in another tmux server — without depending on that unverified + format match. `descriptor.procStart != null` is still the gate, matching + the interface the finding asked for. + +- **`ConnectTimeout=5` on the probe and restore-on-detach ssh calls (F10, + audit-fable-2026-09-11).** `defaultRunRemoteCommand`'s ssh spawn had a kill + timer (`DEFAULT_PROBE_TIMEOUT_MS`, 15 s) but no `ConnectTimeout` — a + half-open connection (portable asleep, NAT gone stale) took the full 15 s + to fail instead of failing fast at the TCP handshake. `buildRemoteCommandArgs + (alias, command)` now builds the argv (`-o BatchMode=yes -o + ConnectTimeout=5 -n `), exported so the argv shape is + tested directly without spawning ssh. **Known, accepted gap: if the app + crashes mid-session, `before-quit`'s `detach()` never runs, so a solo + attach's restore-on-detach ssh call (`status off`/`mouse on`/…) never + fires** — the remote tmux session is left with the solo-attach options set + until something else attaches and detaches cleanly. `ConnectTimeout` bounds + how long a *reachable-but-slow* restore takes; it does nothing for a + restore that never gets scheduled at all. + ### `stop()` cancels, `dispose()` ends -- they are not the same thing `createSshTransport().dispose()` is **terminal**: it sets a flag every later @@ -686,7 +756,7 @@ usable" and "dispose() is terminal", in `test/remote-index.test.js`. - `remote-hosts.test.js` — covers folder-key parsing, alias validation and the `isSafeRelPath` guard - `remote-mirror.test.js` — covers the inventory diff, the no-op second pull, deletions, and both failure modes, against a fake transport - `remote-transport.test.js` — covers the ssh/scp argv, inventory parsing, the timeout kill and `dispose()`, with `spawn` injected; also covers `LIST_COMMAND`'s exact text (issue #211's `.key`-exclusion and single-ssh-call pins), `splitListOutput()` and `parseSessions()` -- `remote-transport-shell.test.js` — runs `LIST_COMMAND` through a real `sh -c`, not a fake stdout fixture: a missing `.claude/projects` must exit non-zero, a missing `.claude/sessions` must still exit 0 with the marker present, and a `.key` file plus a directory named like a descriptor must both be excluded from what reaches stdout +- `remote-transport-shell.test.js` — runs `LIST_COMMAND` through a real `sh -c`, not a fake stdout fixture: a missing `.claude/projects` must exit non-zero, a missing `.claude/sessions` must still exit 0 with the marker present, a `.key` file plus a directory named like a descriptor must both be excluded from what reaches stdout, and (F9) the ALIVE marker reflects real `/proc` liveness for both a live pid (the shell's own `$$`, so it reads as alive on any host) and a dead one - `remote-index.test.js` — covers "no host declared: no timer, no ssh call", the 60 s floor, per-host failure isolation and alias pruning, and that `getRemoteSessions()` is cleared (not left stale) after a cycle whose `sync()` throws - `remote-indexing-e2e.test.js` — covers the `::` prefix reaching session rows, the search entries, the metrics and the sidebar - `dom-sidebar-remote-session.test.js` — covers the remote badge and the read-only click routing diff --git a/cli-session-state.js b/cli-session-state.js index 98bc3c67..5d1a15e4 100644 --- a/cli-session-state.js +++ b/cli-session-state.js @@ -12,20 +12,23 @@ const RESCAN_STATUS = 'idle'; const FLUSH_MS = 150; const MIN_RESCAN_INTERVAL_MS = 1000; const MAX_SEEDED_FILES = 200; +const GET_STATUS_PROBE_THROTTLE_MS = 5000; let dir = DEFAULT_DIR; let activeSessions = null; let onIdle = null; let log = null; let isProcessAlive = defaultIsProcessAlive; +let now = Date.now; let watcher = null; let flushTimer = null; const pending = new Set(); const known = new Map(); const lastRescanAt = new Map(); -// sessionId -> { status, statusUpdatedAt } for live pids only -- see .ai/contexts/cli-session-state.md +// sessionId -> { status, statusUpdatedAt, pid } for live pids only -- see .ai/contexts/cli-session-state.md const statusBySession = new Map(); +const lastProbeAt = new Map(); function defaultIsProcessAlive(pid) { try { @@ -42,6 +45,7 @@ function init(ctx) { onIdle = ctx.onIdle; log = ctx.log || { info() {}, debug() {}, warn() {}, error() {} }; isProcessAlive = ctx.isProcessAlive || defaultIsProcessAlive; + now = ctx.now || Date.now; stop(); } @@ -77,7 +81,7 @@ function handleFile(name) { text = fs.readFileSync(path.join(dir, name), 'utf8'); } catch { const stale = known.get(name); - if (stale && stale.sessionId) statusBySession.delete(stale.sessionId); + if (stale && stale.sessionId) forgetSession(stale.sessionId); known.delete(name); return; } @@ -86,12 +90,12 @@ function handleFile(name) { if (!state) return; const prev = known.get(name); - if (prev && prev.sessionId && prev.sessionId !== state.sessionId) statusBySession.delete(prev.sessionId); + if (prev && prev.sessionId && prev.sessionId !== state.sessionId) forgetSession(prev.sessionId); known.set(name, { procStart: state.procStart, status: state.status, sessionId: state.sessionId }); if (isProcessAlive(state.pid)) { - statusBySession.set(state.sessionId, { status: state.status, statusUpdatedAt: state.statusUpdatedAt }); + statusBySession.set(state.sessionId, { status: state.status, statusUpdatedAt: state.statusUpdatedAt, pid: state.pid }); } else { - statusBySession.delete(state.sessionId); + forgetSession(state.sessionId); } const reused = !!prev && prev.procStart !== state.procStart; @@ -137,12 +141,17 @@ function seed() { if (state) { known.set(name, { procStart: state.procStart, status: state.status, sessionId: state.sessionId }); if (isProcessAlive(state.pid)) { - statusBySession.set(state.sessionId, { status: state.status, statusUpdatedAt: state.statusUpdatedAt }); + statusBySession.set(state.sessionId, { status: state.status, statusUpdatedAt: state.statusUpdatedAt, pid: state.pid }); } } } } +function forgetSession(sessionId) { + statusBySession.delete(sessionId); + lastProbeAt.delete(sessionId); +} + function ensureWatching() { if (watcher) return true; if (!onIdle || !activeSessions) return false; @@ -186,16 +195,24 @@ function stop() { known.clear(); statusBySession.clear(); lastRescanAt.clear(); + lastProbeAt.clear(); } -/** - * Pure lookup: the last {status, statusUpdatedAt} parsed for `sessionId`, or - * undefined if no state file has ever named it. Never touches disk, never - * arms anything -- see .ai/contexts/cli-session-state.md ("the one invariant"). - */ +// Lookup + throttled lazy liveness re-probe -- see .ai/contexts/cli-session-state.md ("the one invariant" still holds: never arms onIdle). function getStatus(sessionId) { const entry = statusBySession.get(sessionId); - return entry ? { status: entry.status, statusUpdatedAt: entry.statusUpdatedAt } : undefined; + if (!entry) return undefined; + + const t = now(); + const last = lastProbeAt.get(sessionId) || 0; + if (t - last >= GET_STATUS_PROBE_THROTTLE_MS) { + lastProbeAt.set(sessionId, t); + if (!isProcessAlive(entry.pid)) { + forgetSession(sessionId); + return undefined; + } + } + return { status: entry.status, statusUpdatedAt: entry.statusUpdatedAt }; } module.exports = { diff --git a/public/app.js b/public/app.js index be5ad8ca..26433ab5 100644 --- a/public/app.js +++ b/public/app.js @@ -809,11 +809,7 @@ function updateRunningIndicators() { const id = item.dataset.sessionId; const running = activePtyIds.has(id); item.classList.toggle('has-running-pty', running); - // A remote row's busy state is owned by the remote adapter (the watch - // channel), not by local PTY presence — it never enters activePtyIds, - // so purging it here on every unrelated local PTY start/stop would wipe - // its spinner. See .ai/contexts/session-cache.md ("Remote hosts — busy - // spinner"). + // remote rows are owned by the remote adapter — see .ai/contexts/session-cache.md ("Remote hosts — busy spinner") if (!running && !item.dataset.remoteAlias) { item.classList.remove('has-busy-agents'); purgeActivityFor(id, 'pty-gone'); diff --git a/public/remote-activity-ui.js b/public/remote-activity-ui.js index a9e9da6c..03db6f8f 100644 --- a/public/remote-activity-ui.js +++ b/public/remote-activity-ui.js @@ -14,12 +14,7 @@ function clearRemoteActivityTimer(sessionId) { function armRemoteDecayTimer(sessionId, ms) { remoteActivityDecayTimers.set(sessionId, setTimeout(() => { remoteActivityDecayTimers.delete(sessionId); - // 20s of transcript silence means "stopped writing", not "response - // ready" — a remote adapter has no PTY to confirm the turn actually - // ended (long tool call, parent delegating to subagents). Clear busy - // without arming the unread marker. Covers both onRemoteActivityEvent's - // decay and seedRemoteActivity's seed-decay — both arm through this - // function. See .ai/contexts/session-cache.md ("Remote hosts — busy spinner"). + // silence is "stopped writing", not "response ready" — see .ai/contexts/session-cache.md ("Remote hosts — busy spinner") setActivity(sessionId, false, 'remote-decay', { armReady: false }); }, ms)); } diff --git a/public/session-activity.js b/public/session-activity.js index ed01d63b..b84dc267 100644 --- a/public/session-activity.js +++ b/public/session-activity.js @@ -34,13 +34,7 @@ function applyActivityClasses(sessionId) { if (window.ATRACE) window.atrace('class.apply', sessionId, { el: item.id || null, 'response-ready': ready, 'cli-busy': item.classList.contains('cli-busy'), fn: 'applyActivityClasses' }); } -// Drop all busy/unread/attention state for a session outside the normal -// active/idle transition — e.g. its PTY just stopped (app.js's -// updateRunningIndicators, via: 'pty-gone'). Sole writer of the three -// collections besides setActivity/rekeyActivityState, so a caller never -// deletes from them directly. Does not touch has-busy-agents/subagent state -// (sidebar.js's clearActiveSubagentsFor) or has-running-pty — those are -// owned elsewhere. +// Purge outside the active/idle transition (e.g. PTY gone); the only writer of the three collections besides setActivity/rekeyActivityState. function purgeActivityFor(sessionId, via) { if (window.ATRACE) window.atrace('store.purge', sessionId, { reason: via, busy: sessionBusyState.get(sessionId) ?? null, ready: responseReadySessions.has(sessionId), attention: attentionSessions.has(sessionId), fn: 'purgeActivityFor' }); attentionSessions.delete(sessionId); @@ -53,11 +47,7 @@ function purgeActivityFor(sessionId, via) { } // Central activity dispatcher. `via` is trace-only — see docs/activity-trace.md. -// `opts.armReady` (default true) gates whether going idle may arm -// response-ready; it is an explicit opt-out, never derived from `via`. Pass -// `{ armReady: false }` for a source that can only infer "stopped writing" -// from silence (no PTY to ask "is a response actually ready?") — see -// .ai/contexts/session-cache.md ("Remote hosts — busy spinner"). +// opts.armReady=false: going idle must not arm response-ready — see .ai/contexts/session-cache.md ("Remote hosts — busy spinner") function setActivity(sessionId, active, via, opts) { const armReady = !(opts && opts.armReady === false); if (active) { diff --git a/public/sidebar.js b/public/sidebar.js index 99007520..ff1783a9 100644 --- a/public/sidebar.js +++ b/public/sidebar.js @@ -1318,8 +1318,6 @@ function buildSessionItem(session) { if (parentHasActiveSubagent(session.sessionId)) item.classList.add('has-busy-agents'); if (window.ATRACE && item.className !== 'session-item js-stateful') window.atrace('class.render', session.sessionId, { el: item.id, cls: item.className, fn: 'buildSessionItem' }); item.dataset.sessionId = session.sessionId; - // Read by app.js's updateRunningIndicators — a remote row's busy state is - // owned by the remote adapter, not local PTY presence (F7). if (session.remoteAlias) item.dataset.remoteAlias = session.remoteAlias; const modified = new Date(session.modified); diff --git a/remote-attach.js b/remote-attach.js index 550faabe..02c57a11 100644 --- a/remote-attach.js +++ b/remote-attach.js @@ -85,6 +85,11 @@ function parseProbeOutput(stdout) { }; } +// see .ai/contexts/session-cache.md ("Remote hosts — tmux attach", pid-reuse guard) +function buildProcCmdlineCheck(pid) { + return `tr '\\0' ' ' < /proc/${pid}/cmdline 2>/dev/null | grep -qi claude && echo 1 || echo 0`; +} + // see .ai/contexts/session-cache.md ("Remote hosts — tmux attach", socket discovery) function buildProbeCommand(pid, target) { return `sock=$(tr '\\0' '\\n' < /proc/${pid}/environ 2>/dev/null | grep -m1 '^TMUX=' | cut -d= -f2- | cut -d, -f1); ` + @@ -94,7 +99,8 @@ function buildProbeCommand(pid, target) { `; printf '${PROBE_SEP}'; tmux -S "$sock" show-options -A -t ${target} status 2>/dev/null` + `; printf '${PROBE_SEP}'; tmux -S "$sock" show-options -A -t ${target} mouse 2>/dev/null` + `; printf '${PROBE_SEP}'; tmux -S "$sock" show-options -A -t ${target} window-size 2>/dev/null` + - `; printf '${PROBE_SEP}'; tmux -S "$sock" list-clients -t ${target} 2>/dev/null | wc -l`; + `; printf '${PROBE_SEP}'; tmux -S "$sock" list-clients -t ${target} 2>/dev/null | wc -l` + + `; printf '${PROBE_SEP}'; ${buildProcCmdlineCheck(pid)}`; } // see .ai/contexts/session-cache.md ("Remote hosts — tmux attach", solo attach parity, issue #253) @@ -132,7 +138,7 @@ function parseClientCount(text) { } // see .ai/contexts/session-cache.md ("Remote hosts — tmux attach", socket discovery) -// parts: [socket, size, status, mouse, window-size, clientCount]; trailing ones optional +// parts: [socket, size, status, mouse, window-size, clientCount, cmdlineHasClaude]; trailing ones optional function parseDiscoveryProbeOutput(stdout) { const text = typeof stdout === 'string' ? stdout : ''; const parts = text.split(PROBE_SEP); @@ -141,7 +147,8 @@ function parseDiscoveryProbeOutput(stdout) { const probed = parseProbeOutput(parts.slice(1, 5).join(PROBE_SEP)); if (!probed) return null; const clientCount = parseClientCount(parts[5]); - return { socket, cols: probed.cols, rows: probed.rows, pre: probed.pre, clientCount }; + const cmdlineHasClaude = parts[6] === '1' ? true : parts[6] === '0' ? false : null; + return { socket, cols: probed.cols, rows: probed.rows, pre: probed.pre, clientCount, cmdlineHasClaude }; } // see .ai/contexts/session-cache.md ("Remote hosts — tmux attach") @@ -161,13 +168,18 @@ function defaultResolveSshPath() { return 'ssh'; } +// see .ai/contexts/session-cache.md ("Remote hosts — tmux attach", ConnectTimeout on the probe/restore ssh) +function buildRemoteCommandArgs(alias, command) { + return ['-o', 'BatchMode=yes', '-o', 'ConnectTimeout=5', '-n', alias, command]; +} + // see .ai/contexts/session-cache.md ("Remote hosts — tmux attach") function defaultRunRemoteCommand(alias, command, { timeoutMs } = {}) { const { spawn } = require('child_process'); return new Promise((resolve) => { let child; try { - child = spawn('ssh', ['-o', 'BatchMode=yes', '-n', alias, command], { + child = spawn('ssh', buildRemoteCommandArgs(alias, command), { windowsHide: true, stdio: ['ignore', 'pipe', 'pipe'], }); } catch (err) { @@ -253,6 +265,11 @@ function createTmuxAttachAdapter(opts = {}) { return { ok: false, error: 'could not parse the remote window size' }; } + // pid-reuse guard — see .ai/contexts/session-cache.md ("Remote hosts — tmux attach", pid-reuse guard) + if (discovery.cmdlineHasClaude === false) { + return { ok: false, error: `pid ${descriptor.pid} now belongs to a process that is not a claude CLI — the session is gone` }; + } + // see .ai/contexts/session-cache.md ("Remote hosts — tmux attach", solo vs shared) const hasLocalSize = !!localSize && Number.isInteger(localSize.cols) && localSize.cols > 0 @@ -333,4 +350,5 @@ module.exports = { buildProbeCommand, buildAttachCommand, buildRestoreCommand, + buildRemoteCommandArgs, }; diff --git a/remote-transport.js b/remote-transport.js index 90a6cda2..9436655d 100644 --- a/remote-transport.js +++ b/remote-transport.js @@ -21,13 +21,18 @@ const SSH_BASE_OPTS = [ '-o', `ConnectTimeout=${DEFAULT_CONNECT_TIMEOUT_S}`, ]; +// STX-prefixed liveness marker printed after each descriptor — see .ai/contexts/session-cache.md ("Remote SSH hosts", liveness) +const ALIVE_MARKER_PREFIX = 'ALIVE:'; + // see .ai/contexts/session-cache.md ("Remote SSH hosts (issue #211)" and -// "Remote hosts — meta.json sidecars") +// "Remote hosts — meta.json sidecars"). F9: each descriptor is followed by a +// \002ALIVE:0|1 line so parseSessions() can drop dead pids without a 2nd ssh. const LIST_COMMAND = `find ${REMOTE_PROJECTS_REL} -type f \\( -name '*.jsonl' -o -name '*.meta.json' \\) -printf '%T@\\t%s\\t%P\\n' || exit $?; ` + `printf '\\001SWITCHBOARD-SESSIONS\\001\\n'; ` + `find ${REMOTE_SESSIONS_REL} -maxdepth 1 -type f -name '[0-9]*.json' 2>/dev/null | LC_ALL=C sort | ` + - `head -n ${MAX_SESSION_DESCRIPTORS} | while IFS= read -r f; do head -c ${MAX_SESSION_DESCRIPTOR_BYTES} "$f"; printf '\\n'; done`; + `head -n ${MAX_SESSION_DESCRIPTORS} | while IFS= read -r f; do head -c ${MAX_SESSION_DESCRIPTOR_BYTES} "$f"; printf '\\n'; ` + + `pid=$(basename "$f" .json); printf '\\002ALIVE:%s\\n' "$( [ -d "/proc/$pid" ] && echo 1 || echo 0 )"; done`; function parseInventory(stdout) { const out = []; @@ -56,13 +61,21 @@ function splitListOutput(stdout) { return { inventoryBlock: stdout.slice(0, idx), sessionsBlock: stdout.slice(afterIdx + 1) }; } -// see .ai/contexts/session-cache.md ("Remote SSH hosts (issue #211)") +// see .ai/contexts/session-cache.md ("Remote SSH hosts (issue #211)", liveness marker) function parseSessions(block) { + const lines = block.split('\n'); const sessions = []; const warnings = []; - for (const rawLine of block.split('\n')) { - const line = rawLine.replace(/\r$/, ''); + let dropped = 0; + for (let i = 0; i < lines.length; i++) { + const line = lines[i].replace(/\r$/, ''); if (!line) continue; + + const next = i + 1 < lines.length ? lines[i + 1].replace(/\r$/, '') : undefined; + const hasMarker = next === `${ALIVE_MARKER_PREFIX}0` || next === `${ALIVE_MARKER_PREFIX}1`; + const alive = hasMarker ? next === `${ALIVE_MARKER_PREFIX}1` : null; + if (hasMarker) i++; // consume the marker line unconditionally, valid JSON or not + let parsed; try { parsed = JSON.parse(line); @@ -82,9 +95,13 @@ function parseSessions(block) { warnings.push('skipped a session descriptor: missing/invalid sessionId'); continue; } + if (alive === false) { + dropped++; + continue; + } sessions.push(parsed); } - return { sessions, warnings }; + return { sessions, warnings, dropped }; } /** @@ -169,8 +186,9 @@ function createSshTransport(opts = {}) { if (res.code !== 0) throw new Error(`ssh inventory failed (exit ${res.code}): ${res.stderr.trim() || 'no stderr'}`); const { inventoryBlock, sessionsBlock } = splitListOutput(res.stdout); const files = parseInventory(inventoryBlock); - const { sessions, warnings } = parseSessions(sessionsBlock); + const { sessions, warnings, dropped } = parseSessions(sessionsBlock); for (const w of warnings) log.warn(`[remote:${alias}] ${w}`); + if (dropped) log.warn(`[remote:${alias}] dropped ${dropped} dead session descriptor(s)`); return { files, sessions }; } @@ -243,6 +261,7 @@ module.exports = { parseSessions, splitListOutput, LIST_COMMAND, + ALIVE_MARKER_PREFIX, REMOTE_PROJECTS_REL, REMOTE_SESSIONS_REL, SESSIONS_MARKER, diff --git a/test/cli-session-state.test.js b/test/cli-session-state.test.js index 2ac1215b..a174258a 100644 --- a/test/cli-session-state.test.js +++ b/test/cli-session-state.test.js @@ -58,6 +58,7 @@ function boot(dir, activeSessions, opts = {}) { activeSessions, log: silentLog, isProcessAlive: opts.isProcessAlive || (() => true), + now: opts.now, onIdle: (sessionId, session) => rescans.push({ sessionId, session }), }); const attached = cliSessionState.ensureWatching(); @@ -386,3 +387,43 @@ test('parseState rejects everything that is not a usable state file', () => { pid: 1, sessionId: 'a', status: 'idle', statusUpdatedAt: 5, procStart: '7', }); }); + +// F2 (audit-fable-2026-09-11): a CLI killed without a clean exit never fires +// a file event, so getStatus() must re-probe liveness itself, throttled. +test('getStatus re-probes liveness lazily and drops a pid that died between two calls, more than 5s apart', async () => { + const dir = mkTmp(); + let alive = true; + let clock = 1_000_000; + try { + writeState(dir, 4242, { status: 'busy', statusUpdatedAt: 1000 }); + boot(dir, oneSession(), { isProcessAlive: () => alive, now: () => clock }); + await waitFor(() => cliSessionState.getStatus('sess-1') !== undefined); + + alive = false; + clock += 5000; // exactly at the throttle boundary — re-probe fires + assert.equal(cliSessionState.getStatus('sess-1'), undefined, + 'a pid that died since the last probe must be dropped on the next getStatus() past the throttle'); + } finally { + cliSessionState.stop(); + fs.rmSync(dir, { recursive: true, force: true }); + } +}); + +test('getStatus keeps returning the cached status within the 5s probe throttle even after the pid dies', async () => { + const dir = mkTmp(); + let alive = true; + let clock = 1_000_000; + try { + writeState(dir, 4242, { status: 'busy', statusUpdatedAt: 1000 }); + boot(dir, oneSession(), { isProcessAlive: () => alive, now: () => clock }); + await waitFor(() => cliSessionState.getStatus('sess-1') !== undefined); + + alive = false; + clock += 4999; // still inside the throttle window — no re-probe + assert.deepEqual(cliSessionState.getStatus('sess-1'), { status: 'busy', statusUpdatedAt: 1000 }, + 'within the throttle window the cached status must be served without probing'); + } finally { + cliSessionState.stop(); + fs.rmSync(dir, { recursive: true, force: true }); + } +}); diff --git a/test/remote-attach.test.js b/test/remote-attach.test.js index d84bd4ee..1fe12bcd 100644 --- a/test/remote-attach.test.js +++ b/test/remote-attach.test.js @@ -17,9 +17,11 @@ const { createTmuxAttachAdapter, parseTmuxField, parseProbeOutput, + parseDiscoveryProbeOutput, buildProbeCommand, buildAttachCommand, buildRestoreCommand, + buildRemoteCommandArgs, } = require('../remote-attach'); const PROBE_SEP = ''; @@ -498,12 +500,81 @@ test('no builder ever emits a backtick', () => { buildRestoreCommand('/tmp/tmux-0/main', 'main:@0.%0', { status: 'on', mouse: 'off', windowSize: 'manual' }), buildRestoreCommand('/tmp/tmux-0/main', 'main:@0.%0', { status: null, mouse: null, windowSize: null }), buildRestoreCommand('/tmp/tmux-0/main', 'main:@0.%0', {}), + buildRemoteCommandArgs('vps', 'echo hi').join(' '), ]; for (const cmd of commands) { assert.ok(!cmd.includes('`'), `command must not contain a backtick: ${cmd}`); } }); +// F10 (audit-fable-2026-09-11): the probe and restore-on-detach ssh must not +// hang past a broken/half-open connection waiting for the (much longer) kill +// timer. +test('buildRemoteCommandArgs adds ConnectTimeout=5 alongside BatchMode, alias last before the command', () => { + const args = buildRemoteCommandArgs('vps', 'echo hi'); + assert.deepEqual(args, ['-o', 'BatchMode=yes', '-o', 'ConnectTimeout=5', '-n', 'vps', 'echo hi']); +}); + +// --- F6 (audit-fable-2026-09-11): pid-reuse guard before a tmux attach ----- + +test('parseDiscoveryProbeOutput reads the trailing cmdline-check segment as cmdlineHasClaude', () => { + const fields = (cmdline) => { + const base = [FAKE_SOCKET, '200x50', 'status on', '', '', '0']; + return (cmdline == null ? base : [...base, cmdline]).join(PROBE_SEP); + }; + assert.equal(parseDiscoveryProbeOutput(fields('1')).cmdlineHasClaude, true); + assert.equal(parseDiscoveryProbeOutput(fields('0')).cmdlineHasClaude, false); + assert.equal(parseDiscoveryProbeOutput(fields(null)).cmdlineHasClaude, null, + 'a probe predating this segment (old fixture/host script) must read as unknown, not false'); +}); + +// Design note (see .ai/contexts/session-cache.md, "pid-reuse guard"): the +// CLI's own `procStart` field is a Windows FILETIME-scale value in the one +// sample this repo has measured (cli-session-state.md) -- there is no +// evidence it lines up with Linux's /proc//stat starttime (clock ticks +// since boot) on a remote host, and ssh access to check was out of scope +// here. Comparing the two numerically risks either shipping a check that +// always mismatches (attach always refused) or one whose units silently +// don't line up (false confidence). This adapter instead verifies +// `/proc//cmdline` still contains "claude" -- weaker than an exact +// start-time match, but it catches the audited scenario (pid reused by an +// unrelated process in another tmux server) without depending on an +// unverified cross-platform format match. `descriptor.procStart != null` is +// still what gates the check, per the interface asked for. +test('attach() proceeds when the probed cmdline still says claude', async () => { + const spawnCalls = []; + const adapter = makeAdapter({ + probeStdout: ['200x50', 'status on', '', '', '0', '1'].join(PROBE_SEP), + spawnCalls, + }); + const result = await adapter.attach('vps', { sessionId: 's1', pid: 4242, tmux: 'main:@0.%0', procStart: '123456' }); + assert.equal(result.ok, true); + assert.equal(spawnCalls.length, 1); +}); + +test('attach() refuses before spawnPty when the probed cmdline no longer says claude', async () => { + const spawnCalls = []; + const adapter = makeAdapter({ + probeStdout: ['200x50', 'status on', '', '', '0', '0'].join(PROBE_SEP), + spawnCalls, + }); + const result = await adapter.attach('vps', { sessionId: 's1', pid: 4242, tmux: 'main:@0.%0', procStart: '123456' }); + assert.equal(result.ok, false); + assert.match(result.error, /pid 4242 now belongs to a process that is not a claude CLI/); + assert.equal(spawnCalls.length, 0, 'no pty may be spawned once the pid-reuse guard refuses'); +}); + +test('attach() proceeds unverified when the probe carries no cmdline segment (older probe output)', async () => { + const spawnCalls = []; + const adapter = makeAdapter({ + probeStdout: ['200x50', 'status on', '', '', '0'].join(PROBE_SEP), + spawnCalls, + }); + const result = await adapter.attach('vps', { sessionId: 's1', pid: 4242, tmux: 'main:@0.%0', procStart: '123456' }); + assert.equal(result.ok, true, 'a probe without the cmdline segment cannot refuse'); + assert.equal(spawnCalls.length, 1); +}); + // Solo attach must actually apply the session-scoped option sets end-to-end // through attach(), not just at the buildAttachCommand unit level. test('attach() applies the session-scoped option sets in the real ssh argv when solo', async () => { diff --git a/test/remote-transport-shell.test.js b/test/remote-transport-shell.test.js index a30ce110..8b539e13 100644 --- a/test/remote-transport-shell.test.js +++ b/test/remote-transport-shell.test.js @@ -13,7 +13,7 @@ const os = require('os'); const path = require('path'); const { spawnSync } = require('child_process'); -const { LIST_COMMAND, SESSIONS_MARKER, splitListOutput, parseSessions } = require('../remote-transport'); +const { LIST_COMMAND, ALIVE_MARKER_PREFIX, SESSIONS_MARKER, splitListOutput, parseSessions } = require('../remote-transport'); function shAvailable() { const r = spawnSync('sh', ['-c', 'exit 0']); @@ -54,7 +54,6 @@ test('LIST_COMMAND: only a digit-named .json FILE is read — never a .key file, const sessionsDir = path.join(dir, '.claude', 'sessions'); fs.mkdirSync(path.join(dir, '.claude', 'projects'), { recursive: true }); fs.mkdirSync(sessionsDir, { recursive: true }); - fs.writeFileSync(path.join(sessionsDir, '1.json'), JSON.stringify({ pid: 1, sessionId: 'abc' })); fs.writeFileSync(path.join(sessionsDir, '1.abc.key'), 'top-secret-key-material-should-never-appear'); fs.mkdirSync(path.join(sessionsDir, '2.json'), { recursive: true }); @@ -69,7 +68,14 @@ test('LIST_COMMAND: only a digit-named .json FILE is read — never a .key file, // skip only this one assertion below, not the rest of the test. } - const result = spawnSync('sh', ['-c', LIST_COMMAND], { cwd: dir, encoding: 'utf8' }); + // F9 (audit-fable-2026-09-11): LIST_COMMAND now drops a descriptor whose + // pid is not alive. The valid descriptor is written by the shell itself, + // naming its own $$, so it reads as alive on any host (real Linux /proc + // or Git Bash's narrower emulation) -- a hardcoded pid like "1" would + // read as dead here and make this test about liveness, not the .key/dir/ + // symlink exclusion it's actually for. + const script = `printf '{"pid":%s,"sessionId":"abc"}' "$$" > ".claude/sessions/$$.json"; ${LIST_COMMAND}`; + const result = spawnSync('sh', ['-c', script], { cwd: dir, encoding: 'utf8' }); assert.equal(result.status, 0); assert.ok(!result.stdout.includes('top-secret-key-material-should-never-appear'), @@ -85,3 +91,36 @@ test('LIST_COMMAND: only a digit-named .json FILE is read — never a .key file, assert.equal(sessions[0].sessionId, 'abc'); } finally { fs.rmSync(dir, { recursive: true, force: true }); } }); + +// F9 (audit-fable-2026-09-11): the ALIVE marker must reflect a real /proc +// check on whatever host runs the shell, for both the "alive" and the "dead" +// case, then parseSessions() must act on it. Uses the shell's own $$ as the +// "alive" pid — self-referential, so it is a real live process under this +// same shell's own /proc view whether that's a real Linux /proc (CI's +// ubuntu-latest leg) or Git Bash's narrower emulation (this machine, CI's +// windows-2022 leg) — no assumption about a specific pid being visible. +test('LIST_COMMAND: ALIVE marker reflects real /proc liveness, and parseSessions drops only the dead one', { skip: SH_SKIP }, () => { + const dir = sandbox(); + try { + const sessionsDir = path.join(dir, '.claude', 'sessions'); + fs.mkdirSync(path.join(dir, '.claude', 'projects'), { recursive: true }); + fs.mkdirSync(sessionsDir, { recursive: true }); + // A pid astronomically unlikely to exist on any OS's pid space. + fs.writeFileSync(path.join(sessionsDir, '999999999.json'), JSON.stringify({ pid: 999999999, sessionId: 'dead' })); + + // The live descriptor is written by the shell itself, naming its own + // $$, right before LIST_COMMAND runs in the same shell instance. + const script = `printf '{"pid":%s,"sessionId":"live"}' "$$" > ".claude/sessions/$$.json"; ${LIST_COMMAND}`; + const result = spawnSync('sh', ['-c', script], { cwd: dir, encoding: 'utf8' }); + + assert.equal(result.status, 0, result.stderr); + assert.ok(result.stdout.includes(`${ALIVE_MARKER_PREFIX}1`), 'the live descriptor must be marked ALIVE:1'); + assert.ok(result.stdout.includes(`${ALIVE_MARKER_PREFIX}0`), 'the dead descriptor must be marked ALIVE:0'); + + const { sessionsBlock } = splitListOutput(result.stdout); + const { sessions, dropped } = parseSessions(sessionsBlock); + assert.equal(sessions.length, 1, 'only the live descriptor survives parseSessions'); + assert.equal(sessions[0].sessionId, 'live'); + assert.equal(dropped, 1); + } finally { fs.rmSync(dir, { recursive: true, force: true }); } +}); diff --git a/test/remote-transport.test.js b/test/remote-transport.test.js index bc3c1442..9e6697a6 100644 --- a/test/remote-transport.test.js +++ b/test/remote-transport.test.js @@ -15,7 +15,7 @@ const { Readable } = require('stream'); const { createSshTransport, parseInventory, parseSessions, splitListOutput, - LIST_COMMAND, SESSIONS_MARKER, MAX_SESSION_DESCRIPTORS, MAX_SESSION_DESCRIPTOR_BYTES, + LIST_COMMAND, ALIVE_MARKER_PREFIX, SESSIONS_MARKER, MAX_SESSION_DESCRIPTORS, MAX_SESSION_DESCRIPTOR_BYTES, } = require('../remote-transport'); function fakeChild() { @@ -109,10 +109,12 @@ test('LIST_COMMAND is pinned exactly — any widening of the sessions glob must `find .claude/projects -type f \\( -name '*.jsonl' -o -name '*.meta.json' \\) -printf '%T@\\t%s\\t%P\\n' || exit $?; ` + `printf '\\001SWITCHBOARD-SESSIONS\\001\\n'; ` + `find .claude/sessions -maxdepth 1 -type f -name '[0-9]*.json' 2>/dev/null | LC_ALL=C sort | ` + - `head -n ${MAX_SESSION_DESCRIPTORS} | while IFS= read -r f; do head -c ${MAX_SESSION_DESCRIPTOR_BYTES} "$f"; printf '\\n'; done`; + `head -n ${MAX_SESSION_DESCRIPTORS} | while IFS= read -r f; do head -c ${MAX_SESSION_DESCRIPTOR_BYTES} "$f"; printf '\\n'; ` + + `pid=$(basename "$f" .json); printf '\\002ALIVE:%s\\n' "$( [ -d "/proc/$pid" ] && echo 1 || echo 0 )"; done`; assert.equal(LIST_COMMAND, expected); }); + // issue #244: the projects find must list both the transcript and its sidecar. test('LIST_COMMAND lists .meta.json sidecars alongside .jsonl transcripts', () => { assert.ok(LIST_COMMAND.includes("-name '*.jsonl' -o -name '*.meta.json'")); @@ -270,8 +272,45 @@ test('parseSessions drops a descriptor with a missing or non-integer pid', () => }); test('parseSessions on a blank/empty block returns no sessions and no warnings', () => { - assert.deepEqual(parseSessions(''), { sessions: [], warnings: [] }); - assert.deepEqual(parseSessions('\n\n'), { sessions: [], warnings: [] }); + assert.deepEqual(parseSessions(''), { sessions: [], warnings: [], dropped: 0 }); + assert.deepEqual(parseSessions('\n\n'), { sessions: [], warnings: [], dropped: 0 }); +}); + +// F9 (audit-fable-2026-09-11): a block with one alive and one dead descriptor +// keeps only the alive one, dropped is counted. +test('parseSessions drops a descriptor marked dead by ALIVE_MARKER_PREFIX, keeps the alive one', () => { + const block = [ + JSON.stringify({ pid: 1, sessionId: 'alive-one' }), + `${ALIVE_MARKER_PREFIX}1`, + JSON.stringify({ pid: 2, sessionId: 'dead-one' }), + `${ALIVE_MARKER_PREFIX}0`, + ].join('\n'); + const { sessions, warnings, dropped } = parseSessions(block); + assert.deepEqual(sessions, [{ pid: 1, sessionId: 'alive-one' }]); + assert.deepEqual(warnings, []); + assert.equal(dropped, 1); +}); + +// Backward compatible: an older host script with no ALIVE marker at all must +// keep behaving exactly as before this fix. +test('parseSessions keeps a descriptor when the ALIVE marker is absent (older host script)', () => { + const block = JSON.stringify({ pid: 1, sessionId: 'no-marker' }) + '\n'; + const { sessions, warnings, dropped } = parseSessions(block); + assert.deepEqual(sessions, [{ pid: 1, sessionId: 'no-marker' }]); + assert.deepEqual(warnings, []); + assert.equal(dropped, 0); +}); + +test('parseSessions: the ALIVE marker line is consumed and never itself warns as invalid JSON', () => { + const block = [ + JSON.stringify({ pid: 1, sessionId: 'a' }), + `${ALIVE_MARKER_PREFIX}1`, + JSON.stringify({ pid: 2, sessionId: 'b' }), + `${ALIVE_MARKER_PREFIX}1`, + ].join('\n'); + const { sessions, warnings } = parseSessions(block); + assert.deepEqual(sessions, [{ pid: 1, sessionId: 'a' }, { pid: 2, sessionId: 'b' }]); + assert.deepEqual(warnings, [], 'the marker lines must never be parsed as their own descriptor'); }); test('a non-zero ssh exit is an error, not an empty inventory', async () => {