Merge orphan-review-round2: exact tmux targets, dual activity clocks

src/session/manager.ts

@@ -431,12 +431,18 @@ export function createSessionManager(
      hostile or malformed name reaching `tmux -t`; the second keeps a live
      session's shell from being killed behind its own PTY's back. */
 
-  /** True when `id` is a real, untracked tmux session of ours that we may act on. */
-  function isActionableOrphan(id: string): boolean {
+  /** The pure half of the guard: is this an id we are allowed to act on at all?
+   *  No tmux call, so it is free to run on a polled path. */
+  function mayActOnOrphan(id: string): boolean {
     if (!cfg.useTmux) return false;
     if (!SESSION_ID_RE.test(id)) return false;
-    if (sessions.has(id)) return false; // tracked ⇒ use get()/killById instead
-    return hasSession(tmuxName(id));
+    return !sessions.has(id); // tracked ⇒ use get()/killById instead
+  }
+
+  /** …plus an existence probe. Only for the DESTRUCTIVE path, which wants to
+   *  distinguish "already gone" (report false) from "killed it". */
+  function isActionableOrphan(id: string): boolean {
+    return mayActOnOrphan(id) && hasSession(tmuxName(id));
   }
 
   async function listOrphans(): Promise<OrphanSessionInfo[]> {
@@ -454,9 +460,14 @@ export function createSessionManager(
       }));
   }
 
-  /** The orphan's current screen, or null. Read-only: never attaches. */
+  /** The orphan's current screen, or null. Read-only: never attaches.
+   *
+   *  Deliberately does NOT probe with hasSession first. That was a second tmux spawn
+   *  per thumbnail — and a SYNCHRONOUS one, on the path a grid of cards refreshes —
+   *  buying nothing: `capture-pane` against a missing session already fails cleanly
+   *  and we already turn that into null. */
   async function captureOrphan(id: string): Promise<string | null> {
-    if (!isActionableOrphan(id)) return null;
+    if (!mayActOnOrphan(id)) return null;
     return capturePane(tmuxName(id));
   }
 
@@ -476,10 +487,20 @@ export function createSessionManager(
    * "this server does not track it" is not evidence that it is abandoned.
    */
   async function killOrphansIdleSince(cutoffMs: number): Promise<string[]> {
+    const candidates = (await listOrphans()).filter(
+      (o) => !o.attached && o.lastActivityAt < cutoffMs,
+    );
+    if (candidates.length === 0) return [];
+
+    // Re-read the world immediately before killing. The list above is a snapshot,
+    // and the loop that follows takes time: a session can be attached, or adopted
+    // into our table, in that window — and this is the irreversible path, so it
+    // re-checks rather than trusting the snapshot.
+    const fresh = new Map((await listOrphans()).map((o) => [o.id, o]));
     const killed: string[] = [];
-    for (const o of await listOrphans()) {
-      if (o.attached) continue;
-      if (o.lastActivityAt >= cutoffMs) continue;
+    for (const o of candidates) {
+      const now = fresh.get(o.id);
+      if (now === undefined || now.attached) continue; // gone, adopted, or now in use
       if (killOrphan(o.id)) killed.push(o.id);
     }
     return killed;

src/session/tmux.ts

@@ -28,19 +28,35 @@ const TMUX_TIMEOUT_MS = 3000
 /**
  * Tab-separated, machine-readable. tmux reports both clocks in unix SECONDS.
  *
- * The activity clock is `window_activity`, NOT `session_activity`. That is not a
- * detail: `session_activity` tracks CLIENT activity — it advances on attach and on
- * keypresses — and never moves for a detached session no matter how much its shell
- * is printing. Measured on tmux 3.6a with a detached session in a `while true; do
- * echo tick; sleep 1; done` loop: after 6s, session_activity was still equal to
- * session_created while window_activity had advanced. Using session_activity as the
- * liveness signal would make the idle-cleanup kill precisely the long-running,
- * unattended, actively-working sessions this whole feature exists to protect.
+ * BOTH activity clocks are requested, because neither alone answers "has anything
+ * happened here" and they are not ordered:
+ *
+ *  - `session_activity` tracks CLIENT activity — it advances on attach and on
+ *    keypresses, and never moves for a detached session no matter how much its shell
+ *    is printing. Measured on tmux 3.6a with a detached session in a `while true; do
+ *    echo tick; sleep 1; done` loop: after 6 s it still equalled session_created
+ *    while window_activity had advanced. Using it alone as the liveness signal makes
+ *    idle-cleanup kill precisely the long-running, unattended, actively-working
+ *    sessions this feature exists to protect — on one real host, 3 of them.
+ *  - `window_activity` tracks pane OUTPUT, but an attach with no output bumps only
+ *    session_activity. Observed on the same host: one session whose session_activity
+ *    was 4 s newer than its window_activity, another whose window_activity was 25
+ *    days newer.
+ *
+ * So the parser takes max(window_activity, session_activity).
+ *
+ * CAVEAT: in `list-sessions`, `#{window_activity}` resolves against the session's
+ * CURRENT window only. Every session this app creates is single-window (plain
+ * `new-session`), so it is exact here; a user who opens a second window inside one
+ * (Ctrl-b c) and leaves output only in a background window could still read as
+ * quieter than it is. Taking the max limits that to "as stale as session_activity",
+ * never staler.
  */
 const LIST_FORMAT = [
   '#{session_name}',
   '#{session_created}',
   '#{window_activity}',
+  '#{session_activity}',
   '#{session_attached}',
   '#{window_width}',
   '#{window_height}',
@@ -78,10 +94,38 @@ export function tmuxName(sessionId: string): string {
   return `web_${sessionId}`
 }
 
+/**
+ * A tmux `-t` target that matches ONE session by exact name.
+ *
+ * Without the `=`, tmux resolves a target as exact name, then NAME PREFIX, then
+ * fnmatch. That is a hole in this module's central promise: a session named
+ * `web_<uuid>_mine` is refused by parseSessionList (its suffix is not a UUID) and so
+ * is never listed — but `-t web_<uuid>` prefix-matches it, so a kill aimed at a
+ * non-existent id would end the user's own session instead. Verified on tmux 3.6a:
+ * `has-session -t web_<uuid>` succeeds against `web_<uuid>_MINE`, and fails with the
+ * `=` prefix. Every -t in this file goes through here.
+ */
+function exact(name: string): string {
+  return `=${name}`
+}
+
+/**
+ * The same, for a target-PANE. `capture-pane` takes a pane, and tmux rejects a bare
+ * `=name` there outright ("can't find pane: =web_x") — the `=` only qualifies the
+ * session part of a target, so the session has to be named as such with a trailing
+ * `:`. Verified on tmux 3.6a against a decoy named `web_<uuid>_decoy`: with the real
+ * session gone, `-t web_<uuid>` printed the DECOY's screen while `-t =web_<uuid>:`
+ * failed with "can't find session". Without this the preview leaks the contents of a
+ * session the user created for their own work.
+ */
+function exactPane(name: string): string {
+  return `=${name}:`
+}
+
 /** Does a tmux session with this name already exist (e.g. after a restart)? */
 export function hasSession(name: string): boolean {
   try {
-    execFileSync('tmux', ['has-session', '-t', name], {
+    execFileSync('tmux', ['has-session', '-t', exact(name)], {
       stdio: 'ignore',
       timeout: TMUX_TIMEOUT_MS,
     })
@@ -94,7 +138,7 @@ export function hasSession(name: string): boolean {
 /** Kill a tmux session (ends the shell). No-op if it's already gone. */
 export function killSession(name: string): void {
   try {
-    execFileSync('tmux', ['kill-session', '-t', name], {
+    execFileSync('tmux', ['kill-session', '-t', exact(name)], {
       stdio: 'ignore',
       timeout: TMUX_TIMEOUT_MS,
     })
@@ -120,9 +164,10 @@ export function parseSessionList(stdout: string): TmuxSessionSummary[] {
   for (const line of stdout.split('\n')) {
     if (line === '') continue
     const parts = line.split('\t')
-    if (parts.length < 6) continue
+    if (parts.length < 7) continue
 
-    const [name, created, activity, attached, width, height] = parts as [
+    const [name, created, winActivity, sessActivity, attached, width, height] = parts as [
+      string,
       string,
       string,
       string,
@@ -134,24 +179,36 @@ export function parseSessionList(stdout: string): TmuxSessionSummary[] {
     const id = name.slice('web_'.length)
     if (!SESSION_ID_RE.test(id)) continue
 
-    const createdSec = Number(created)
-    const activitySec = Number(activity)
-    const cols = Number(width)
-    const rows_ = Number(height)
+    // Number('') is 0, which IS finite — so an empty field would sail through as
+    // "created at the epoch, idle ever since", i.e. instantly eligible for the idle
+    // cleanup. Require actual digits.
+    const num = (v: string): number => (/^\d+$/.test(v) ? Number(v) : NaN)
+    const createdSec = num(created)
+    const winSec = num(winActivity)
+    const sessSec = num(sessActivity)
+    const cols = num(width)
+    const rows_ = num(height)
     if (
       !Number.isFinite(createdSec) ||
-      !Number.isFinite(activitySec) ||
+      !Number.isFinite(winSec) ||
+      !Number.isFinite(sessSec) ||
       !Number.isFinite(cols) ||
       !Number.isFinite(rows_)
     ) {
       continue
     }
+    // The two clocks are NOT ordered: window_activity tracks pane output, while an
+    // attach bumps session_activity without producing any. Observed on a real host:
+    // one session with session_activity 4s NEWER than window_activity, another with
+    // window_activity 25 DAYS newer. The question being asked is "has anything
+    // happened here", so take whichever is later.
+    const activitySec = Math.max(winSec, sessSec)
 
     rows.push({
       id,
       createdAtMs: createdSec * 1000,
       lastActivityAtMs: activitySec * 1000,
-      attached: attached !== '0' && attached !== '',
+      attached: attached !== '0',
       cols,
       rows: rows_,
     })
@@ -192,7 +249,7 @@ export async function listSessions(): Promise<TmuxSessionSummary[]> {
  */
 export async function capturePane(name: string): Promise<string | null> {
   try {
-    const { stdout } = await execFileAsync('tmux', ['capture-pane', '-p', '-e', '-t', name], {
+    const { stdout } = await execFileAsync('tmux', ['capture-pane', '-p', '-e', '-t', exactPane(name)], {
       encoding: 'utf8',
       maxBuffer: CAPTURE_MAX_BUFFER,
       timeout: TMUX_TIMEOUT_MS,

test/integration/orphan-sessions.test.ts

@@ -187,6 +187,42 @@ describe('orphan sessions — tmux enabled', () => {
     ).toContain(name)
   })
 
+  itTmux('cannot reach a look-alike session by tmux prefix matching', async () => {
+    // tmux -t resolves exact name, then NAME PREFIX, then fnmatch. `web_<uuid>_mine`
+    // is refused by parseSessionList (suffix is not a UUID) and so is never listed —
+    // but a bare `-t web_<uuid>` prefix-matches it, which would let a DELETE aimed at
+    // a non-existent id end a session the user created for their own work.
+    const decoyId = randomUUID()
+    const decoy = `web_${decoyId}_mine`
+    execFileSync('tmux', ['new-session', '-d', '-s', decoy, 'sleep 300'], { stdio: 'ignore' })
+    try {
+      // It must not appear in the listing…
+      const list = (await (await fetch(`${origin()}/orphan-sessions`)).json()) as OrphanSessionInfo[]
+      expect(list.some((o) => o.id === decoyId)).toBe(false)
+
+      // …and the exact id must be reported as absent, not silently matched to it.
+      const preview = await fetch(`${origin()}/orphan-sessions/${decoyId}/preview`)
+      expect(preview.status).toBe(404)
+
+      const del = await fetch(`${origin()}/orphan-sessions/${decoyId}`, {
+        method: 'DELETE',
+        headers: { Origin: origin() },
+      })
+      expect(del.status).toBe(404)
+
+      // The decoy is still alive — that is the whole point.
+      expect(
+        execFileSync('tmux', ['ls', '-F', '#{session_name}'], { encoding: 'utf8' }),
+      ).toContain(decoy)
+    } finally {
+      try {
+        execFileSync('tmux', ['kill-session', '-t', `=${decoy}`], { stdio: 'ignore' })
+      } catch {
+        // already gone
+      }
+    }
+  })
+
   itTmux('kills it, and 404s the second time', async () => {
     const first = await fetch(`${origin()}/orphan-sessions/${id}`, {
       method: 'DELETE',

test/manager-orphans.test.ts

@@ -177,12 +177,16 @@ describe('captureOrphan', () => {
     expect(tmuxCapture).not.toHaveBeenCalled();
   });
 
-  it('refuses when no such tmux session exists', async () => {
+  it('returns null when the session is gone, without a second probe spawn', async () => {
+    // No hasSession pre-check here on purpose: it was a second tmux spawn per
+    // thumbnail — a SYNCHRONOUS one, on the polled grid-refresh path — and
+    // capture-pane already fails cleanly on a missing session.
     tmuxHas.mockReturnValue(false);
+    tmuxCapture.mockResolvedValue(null);
     const mgr = createSessionManager(CFG);
 
     expect(await mgr.captureOrphan(U1)).toBeNull();
-    expect(tmuxCapture).not.toHaveBeenCalled();
+    expect(tmuxHas).not.toHaveBeenCalled();
   });
 
   it('refuses an id the table owns — a live session previews from its ring buffer', async () => {
@@ -250,6 +254,21 @@ describe('killOrphansIdleSince', () => {
     expect(tmuxKill).toHaveBeenCalledTimes(1);
   });
 
+  it('re-checks attachment right before killing, not just in the snapshot', async () => {
+    // The candidate list is a snapshot and the kill loop takes time; a session can be
+    // attached in that window. This is the irreversible path, so it re-reads.
+    let call = 0;
+    tmuxList.mockImplementation(async () => {
+      call += 1;
+      // first read: idle and unattached → a candidate. second read: now attached.
+      return [summary(U1, 1_000, call > 1)];
+    });
+    const mgr = createSessionManager(CFG);
+
+    expect(await mgr.killOrphansIdleSince(5_000)).toEqual([]);
+    expect(tmuxKill).not.toHaveBeenCalled();
+  });
+
   it('never kills a session someone is attached to from a terminal', async () => {
     // "Untracked by this server" is not "abandoned" — a plain `tmux attach` or a
     // second server process both show up as attached, and bulk cleanup must not

test/tmux.test.ts

@@ -54,7 +54,7 @@ describe('hasSession', () => {
     expect(hasSession('web_x')).toBe(true)
     expect(mockExec).toHaveBeenCalledWith(
       'tmux',
-      ['has-session', '-t', 'web_x'],
+      ['has-session', '-t', '=web_x'],
       expect.objectContaining({ stdio: 'ignore' }),
     )
   })
@@ -68,12 +68,12 @@ describe('hasSession', () => {
 })
 
 describe('killSession', () => {
-  it('invokes tmux kill-session for the name', () => {
+  it('targets the EXACT name so it cannot prefix-match another session', () => {
     mockExec.mockReturnValue(Buffer.from(''))
     killSession('web_x')
     expect(mockExec).toHaveBeenCalledWith(
       'tmux',
-      ['kill-session', '-t', 'web_x'],
+      ['kill-session', '-t', '=web_x'],
       expect.objectContaining({ stdio: 'ignore' }),
     )
   })
@@ -93,7 +93,7 @@ const U2 = '3a63f3c7-acfe-4081-992f-c4dbd55b3d6b'
 
 describe('parseSessionList', () => {
   it('parses tmux rows into summaries, converting seconds to ms', () => {
-    const out = `web_${U1}\t1783911991\t1783911992\t0\t80\t23\n`
+    const out = `web_${U1}\t1783911991\t1783911992\t1783911990\t0\t80\t23\n`
     expect(parseSessionList(out)).toEqual([
       {
         id: U1,
@@ -107,32 +107,53 @@ describe('parseSessionList', () => {
   })
 
   it('sorts most-recently-active first', () => {
-    const out = [`web_${U1}\t100\t100\t0\t80\t24`, `web_${U2}\t100\t900\t1\t80\t24`].join('\n')
+    const out = [`web_${U1}\t100\t100\t100\t0\t80\t24`, `web_${U2}\t100\t900\t100\t1\t80\t24`].join('\n')
     expect(parseSessionList(out).map((s) => s.id)).toEqual([U2, U1])
   })
 
   it('reports a session attached from a real terminal', () => {
-    const out = `web_${U1}\t100\t100\t1\t80\t24`
+    const out = `web_${U1}\t100\t100\t100\t1\t80\t24`
     expect(parseSessionList(out)[0]!.attached).toBe(true)
   })
 
   it('ignores sessions that are not ours — never touch the user’s own tmux', () => {
-    const out = ['mywork\t100\t100\t0\t80\t24', `web_${U1}\t100\t100\t0\t80\t24`].join('\n')
+    const out = ['mywork\t100\t100\t100\t0\t80\t24', `web_${U1}\t100\t100\t100\t0\t80\t24`].join('\n')
     expect(parseSessionList(out).map((s) => s.id)).toEqual([U1])
   })
 
   it('ignores a web_-prefixed name whose suffix is not a UUID v4', () => {
     // Only ids the protocol could have produced are actionable: anything else is
     // someone else's session that merely starts with web_.
-    const out = ['web_../../etc\t100\t100\t0\t80\t24', 'web_-t\t100\t100\t0\t80\t24'].join('\n')
+    const out = ['web_../../etc\t100\t100\t100\t0\t80\t24', 'web_-t\t100\t100\t100\t0\t80\t24'].join('\n')
     expect(parseSessionList(out)).toEqual([])
   })
 
   it('skips malformed and blank rows instead of throwing', () => {
-    const out = ['', 'garbage', `web_${U1}\tNaN\t100\t0\t80\t24`, `web_${U2}\t100\t100\t0\t80\t24`].join('\n')
+    const out = [
+      '',
+      'garbage',
+      `web_${U1}\tNaN\t100\t100\t0\t80\t24`,
+      `web_${U2}\t100\t100\t100\t0\t80\t24`,
+    ].join('\n')
     expect(parseSessionList(out).map((s) => s.id)).toEqual([U2])
   })
 
+  it('rejects an EMPTY numeric field instead of reading it as 0', () => {
+    // Number('') is 0 and 0 is finite, so a blank clock would parse as "created at
+    // the epoch, idle ever since" — instantly eligible for the idle cleanup.
+    const out = `web_${U1}\t\t100\t100\t0\t80\t24`
+    expect(parseSessionList(out)).toEqual([])
+  })
+
+  it('takes the NEWER of the two activity clocks', () => {
+    // They are not ordered: window_activity tracks pane output, session_activity is
+    // bumped by an attach that produces none. "Has anything happened" is the max.
+    const winNewer = `web_${U1}\t100\t900\t100\t0\t80\t24`
+    const sessNewer = `web_${U2}\t100\t100\t900\t0\t80\t24`
+    expect(parseSessionList(winNewer)[0]!.lastActivityAtMs).toBe(900_000)
+    expect(parseSessionList(sessNewer)[0]!.lastActivityAtMs).toBe(900_000)
+  })
+
   it('returns an empty list for empty output (no tmux server)', () => {
     expect(parseSessionList('')).toEqual([])
   })
@@ -140,7 +161,7 @@ describe('parseSessionList', () => {
 
 describe('listSessions', () => {
   it('asks tmux for a machine-readable list', async () => {
-    mockExecAsync.mockResolvedValue({ stdout: `web_${U1}\t100\t100\t0\t80\t24\n`, stderr: '' })
+    mockExecAsync.mockResolvedValue({ stdout: `web_${U1}\t100\t100\t100\t0\t80\t24\n`, stderr: '' })
     expect((await listSessions()).map((s) => s.id)).toEqual([U1])
     const [file, args] = mockExecAsync.mock.calls[0] as [string, string[]]
     expect(file).toBe('tmux')
@@ -157,7 +178,9 @@ describe('listSessions', () => {
     const [, args] = mockExecAsync.mock.calls[0] as [string, string[]]
     const fmt = args[args.indexOf('-F') + 1]!
     expect(fmt).toContain('#{window_activity}')
-    expect(fmt).not.toContain('#{session_activity}')
+    // session_activity is also requested — the parser takes the max — but
+    // window_activity is the one that must never be missing.
+    expect(fmt).toContain('#{session_activity}')
   })
 
   it('bounds the call so a wedged tmux cannot hang the server', async () => {
@@ -183,7 +206,13 @@ describe('capturePane', () => {
     expect(args[0]).toBe('capture-pane')
     expect(args).toContain('-p') // print to stdout — read-only, no client
     expect(args).toContain('-e') // keep escape sequences so colour survives
-    expect(args).toContain('web_x')
+    // '=web_x:' forces an EXACT session match for a target-PANE. A bare target
+    // prefix-matches, which leaks the screen of a session parseSessionList
+    // deliberately refuses to enumerate; a plain '=web_x' is rejected by tmux
+    // outright ("can't find pane"), because '=' only qualifies the session part.
+    expect(args).toContain('=web_x:')
+    expect(args).not.toContain('web_x')
+    expect(args).not.toContain('=web_x')
     // Attaching would resize the session and SIGWINCH whatever is running in it.
     expect(args).not.toContain('attach-session')
   })