Round two of adversarial review. The skeptic confirmed the session_activity finding
with extra evidence from this host and raised three things the first pass missed.
SECURITY — `tmux -t <name>` resolves exact name, then NAME PREFIX, then fnmatch.
The whole module rests on "we only ever touch `web_` + a UUID v4, so a tmux session
the user made for their own work is never enumerated and never offered for
deletion". Prefix matching goes around that: a session named `web_<uuid>_mine` is
refused by parseSessionList (its suffix is not a UUID) and so never appears in the
listing — but `-t web_<uuid>` matches it. So a DELETE aimed at an id that does not
exist would end the user's session, and a preview would print its screen.
Verified on tmux 3.6a, isolated socket:
has-session -t web_<uuid> → succeeds against web_<uuid>_MINE
has-session -t =web_<uuid> → "can't find session"
capture-pane -p -t web_<uuid> → printed the DECOY's screen
capture-pane -p -t =web_<uuid>: → "can't find session"
Two target forms are needed, because `=` only qualifies the session part of a
target: session targets (has-session, kill-session) take `=name`, while
capture-pane takes a PANE and rejects a bare `=name` outright ("can't find pane"),
so it needs `=name:`. Both are now the only way a name reaches tmux, with an
integration test that stands up a real decoy and asserts the listing omits it and
both the preview and the DELETE report 404 while it stays alive.
CORRECTNESS — take max(window_activity, session_activity), not window_activity
alone. The previous commit swapped one clock for the other, but they are not
ordered: window_activity tracks pane output while an attach with no output bumps
only session_activity. On this host one session's session_activity was 4 s NEWER
than its window_activity (and another's window_activity was 25 days newer). The
question is "has anything happened here", so the answer is the later of the two.
Also documents the caveat that window_activity resolves against the session's
current window only — exact for the single-window sessions this app creates, and
bounded by the max for anything else.
CORRECTNESS — parseSessionList accepted an empty numeric field. Number('') is 0
and 0 is finite, so a blank clock parsed as "created at the epoch, idle ever
since": instantly eligible for the idle cleanup. Fields must now be actual digits.
SAFETY — bulk cleanup re-reads the world immediately before killing. The candidate
list is a snapshot and the kill loop takes time, so a session could be attached, or
adopted into the table, inside that window. This is the irreversible path; it now
verifies each candidate against a fresh listing rather than trusting the snapshot.
PERFORMANCE — captureOrphan no longer probes with hasSession first. That was a
second tmux spawn per thumbnail, and a SYNCHRONOUS one on the path a whole grid
refreshes, buying nothing: capture-pane already fails cleanly on a missing session
and we already turn that into null. The guard splits into a pure half
(mayActOnOrphan: UUID shape + not in the table) used by both paths, and the
existence probe, which only the destructive path needs.
Tests: 2213 unit, 9 orphan integration (incl. the decoy regression). Confirmed
`has-session -t =<name>` still resolves the 69 real sessions, so the Case 3.5
re-attach path is unaffected.
Note: test/integration/server.test.ts "H1 shell state survives a server restart"
flakes ~1 run in 3 when all 27 run together on a loaded machine, and passes 4/4 in
isolation. That is the pre-existing real-PTY timing flake already recorded in
PROGRESS_LOG, not this change — it exercises Case 3.5, which is verified above.
324 lines
11 KiB
TypeScript
324 lines
11 KiB
TypeScript
/**
|
||
* A — orphan tmux sessions: the manager side.
|
||
*
|
||
* An "orphan" is a `web_*` tmux session on the host that this server does NOT have
|
||
* in its table (it outlived the process that made it). These tests pin the three
|
||
* rules that keep the feature safe:
|
||
*
|
||
* 1. never enumerate or act on a tmux session that is not ours (`web_` + UUID v4);
|
||
* 2. never act on an id the table already owns — that path must stay killById, or
|
||
* we would kill the shell out from under a live PTY and leave a zombie entry;
|
||
* 3. do nothing at all when tmux is off, so the non-tmux deployment is unchanged.
|
||
*
|
||
* Both node-pty and the tmux CLI wrappers are mocked: no shell, no tmux binary.
|
||
*/
|
||
|
||
import { describe, it, expect, beforeEach, vi } from 'vitest';
|
||
|
||
import type { Config, Dims, WebSocketLike } from '../src/types.js';
|
||
import { WS_OPEN } from '../src/types.js';
|
||
import { createMockPty, type MockIPty } from './helpers/mock-pty.js';
|
||
|
||
let nextPty: MockIPty = createMockPty();
|
||
|
||
vi.mock('node-pty', () => ({
|
||
spawn: () => nextPty,
|
||
}));
|
||
|
||
// ── tmux CLI mock ────────────────────────────────────────────────────────────
|
||
const tmuxList = vi.fn<() => Promise<unknown[]>>(async () => []);
|
||
const tmuxCapture = vi.fn<(name: string) => Promise<string | null>>(async () => null);
|
||
const tmuxKill = vi.fn<(name: string) => void>(() => {});
|
||
const tmuxHas = vi.fn<(name: string) => boolean>(() => true);
|
||
|
||
vi.mock('../src/session/tmux.js', () => ({
|
||
tmuxAvailable: () => true,
|
||
tmuxName: (id: string) => `web_${id}`,
|
||
hasSession: (n: string) => tmuxHas(n),
|
||
killSession: (n: string) => tmuxKill(n),
|
||
listSessions: () => tmuxList(),
|
||
capturePane: (n: string) => tmuxCapture(n),
|
||
}));
|
||
|
||
const { createSessionManager } = await import('../src/session/manager.js');
|
||
|
||
const U1 = '05caa88b-1f5f-45d9-bbbe-fc5955314870';
|
||
const U2 = '3a63f3c7-acfe-4081-992f-c4dbd55b3d6b';
|
||
|
||
const BASE_CFG = {
|
||
port: 3000,
|
||
bindHost: '0.0.0.0',
|
||
shellPath: '/bin/zsh',
|
||
homeDir: '/home/tester',
|
||
idleTtlMs: 10_000,
|
||
scrollbackBytes: 2 * 1024 * 1024,
|
||
maxPayloadBytes: 1024 * 1024,
|
||
wsPath: '/term',
|
||
maxSessions: 50,
|
||
maxMsgsPerSec: 2000,
|
||
permTimeoutMs: 300_000,
|
||
reapIntervalMs: 60_000,
|
||
previewBytes: 24 * 1024,
|
||
useTmux: true,
|
||
allowedOrigins: [],
|
||
projectRoots: ['/home/tester'],
|
||
projectScanDepth: 4,
|
||
projectScanTtlMs: 10_000,
|
||
projectDirtyCheck: true,
|
||
editorCmd: 'code',
|
||
vapidPublicKey: undefined,
|
||
vapidPrivateKey: undefined,
|
||
vapidSubject: undefined,
|
||
pushStorePath: '/home/tester/.push.json',
|
||
pushMaxSubs: 50,
|
||
notifyDone: true,
|
||
notifyDnd: false,
|
||
decisionTokenTtlMs: 300_000,
|
||
timelineMax: 200,
|
||
timelineEnabled: true,
|
||
stuckTtlMs: 10_000,
|
||
stuckAlert: true,
|
||
diffTimeoutMs: 2000,
|
||
diffMaxBytes: 2 * 1024 * 1024,
|
||
diffMaxFiles: 300,
|
||
statuslineTtlMs: 30_000,
|
||
worktreeEnabled: true,
|
||
worktreeRoot: undefined,
|
||
worktreeTimeoutMs: 10_000,
|
||
maxFanoutLanes: 6,
|
||
defaultPermissionMode: 'default' as const,
|
||
allowAutoMode: false,
|
||
queueEnabled: true,
|
||
queueMaxItems: 10,
|
||
queueItemMaxBytes: 4096,
|
||
queueSettleMs: 1500,
|
||
} satisfies Config;
|
||
|
||
const CFG: Config = BASE_CFG;
|
||
const CFG_NO_TMUX: Config = { ...BASE_CFG, useTmux: false };
|
||
const DIMS: Dims = { cols: 80, rows: 24 };
|
||
|
||
function createMockWs(): WebSocketLike & { readyState: number } {
|
||
return {
|
||
readyState: WS_OPEN,
|
||
send() {},
|
||
close() {},
|
||
};
|
||
}
|
||
|
||
/** A tmux summary row as src/session/tmux.ts would produce it. */
|
||
function summary(id: string, activityMs = 5_000, attached = false) {
|
||
return {
|
||
id,
|
||
createdAtMs: 1_000,
|
||
lastActivityAtMs: activityMs,
|
||
attached,
|
||
cols: 120,
|
||
rows: 40,
|
||
};
|
||
}
|
||
|
||
beforeEach(() => {
|
||
nextPty = createMockPty();
|
||
tmuxList.mockReset().mockResolvedValue([]);
|
||
tmuxCapture.mockReset().mockResolvedValue(null);
|
||
tmuxKill.mockReset();
|
||
tmuxHas.mockReset().mockReturnValue(true);
|
||
});
|
||
|
||
describe('listOrphans', () => {
|
||
it('reports a tmux session the table does not know about', async () => {
|
||
tmuxList.mockResolvedValue([summary(U1, 9_000, true)]);
|
||
const mgr = createSessionManager(CFG);
|
||
|
||
expect(await mgr.listOrphans()).toEqual([
|
||
{
|
||
id: U1,
|
||
createdAt: 1_000,
|
||
lastActivityAt: 9_000,
|
||
attached: true,
|
||
cols: 120,
|
||
rows: 40,
|
||
},
|
||
]);
|
||
});
|
||
|
||
it('excludes sessions the table already owns — those are live, not orphans', async () => {
|
||
const mgr = createSessionManager(CFG);
|
||
const live = mgr.handleAttach(createMockWs(), null, DIMS, 1_000);
|
||
tmuxList.mockResolvedValue([summary(live.meta.id), summary(U2)]);
|
||
|
||
expect((await mgr.listOrphans()).map((o) => o.id)).toEqual([U2]);
|
||
});
|
||
|
||
it('is empty when tmux is off — the non-tmux deployment is untouched', async () => {
|
||
tmuxList.mockResolvedValue([summary(U1)]);
|
||
const mgr = createSessionManager(CFG_NO_TMUX);
|
||
|
||
expect(await mgr.listOrphans()).toEqual([]);
|
||
expect(tmuxList).not.toHaveBeenCalled();
|
||
});
|
||
});
|
||
|
||
describe('captureOrphan', () => {
|
||
it('returns the captured screen for an untracked session', async () => {
|
||
tmuxCapture.mockResolvedValue('screen contents');
|
||
const mgr = createSessionManager(CFG);
|
||
|
||
expect(await mgr.captureOrphan(U1)).toBe('screen contents');
|
||
expect(tmuxCapture).toHaveBeenCalledWith(`web_${U1}`);
|
||
});
|
||
|
||
it('refuses an id that is not a UUID v4 without invoking tmux at all', async () => {
|
||
const mgr = createSessionManager(CFG);
|
||
|
||
expect(await mgr.captureOrphan('../../etc/passwd')).toBeNull();
|
||
expect(await mgr.captureOrphan('-t')).toBeNull();
|
||
expect(tmuxCapture).not.toHaveBeenCalled();
|
||
});
|
||
|
||
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(tmuxHas).not.toHaveBeenCalled();
|
||
});
|
||
|
||
it('refuses an id the table owns — a live session previews from its ring buffer', async () => {
|
||
const mgr = createSessionManager(CFG);
|
||
const live = mgr.handleAttach(createMockWs(), null, DIMS, 1_000);
|
||
|
||
expect(await mgr.captureOrphan(live.meta.id)).toBeNull();
|
||
expect(tmuxCapture).not.toHaveBeenCalled();
|
||
});
|
||
|
||
it('returns null when tmux is off', async () => {
|
||
const mgr = createSessionManager(CFG_NO_TMUX);
|
||
expect(await mgr.captureOrphan(U1)).toBeNull();
|
||
expect(tmuxCapture).not.toHaveBeenCalled();
|
||
});
|
||
});
|
||
|
||
describe('killOrphan', () => {
|
||
it('kills the tmux session by its prefixed name', () => {
|
||
const mgr = createSessionManager(CFG);
|
||
|
||
expect(mgr.killOrphan(U1)).toBe(true);
|
||
expect(tmuxKill).toHaveBeenCalledWith(`web_${U1}`);
|
||
});
|
||
|
||
it('refuses a non-UUID id without invoking tmux', () => {
|
||
const mgr = createSessionManager(CFG);
|
||
|
||
expect(mgr.killOrphan('web_x')).toBe(false);
|
||
expect(mgr.killOrphan('-t')).toBe(false);
|
||
expect(mgr.killOrphan('')).toBe(false);
|
||
expect(tmuxKill).not.toHaveBeenCalled();
|
||
});
|
||
|
||
it('refuses an id the table owns — that is killById’s job', () => {
|
||
const mgr = createSessionManager(CFG);
|
||
const live = mgr.handleAttach(createMockWs(), null, DIMS, 1_000);
|
||
|
||
expect(mgr.killOrphan(live.meta.id)).toBe(false);
|
||
expect(tmuxKill).not.toHaveBeenCalled();
|
||
});
|
||
|
||
it('reports false when the session is already gone', () => {
|
||
tmuxHas.mockReturnValue(false);
|
||
const mgr = createSessionManager(CFG);
|
||
|
||
expect(mgr.killOrphan(U1)).toBe(false);
|
||
expect(tmuxKill).not.toHaveBeenCalled();
|
||
});
|
||
|
||
it('does nothing when tmux is off', () => {
|
||
const mgr = createSessionManager(CFG_NO_TMUX);
|
||
expect(mgr.killOrphan(U1)).toBe(false);
|
||
expect(tmuxKill).not.toHaveBeenCalled();
|
||
});
|
||
});
|
||
|
||
describe('killOrphansIdleSince', () => {
|
||
it('kills only sessions whose last activity is older than the cutoff', async () => {
|
||
tmuxList.mockResolvedValue([summary(U1, 1_000), summary(U2, 9_000)]);
|
||
const mgr = createSessionManager(CFG);
|
||
|
||
expect(await mgr.killOrphansIdleSince(5_000)).toEqual([U1]);
|
||
expect(tmuxKill).toHaveBeenCalledWith(`web_${U1}`);
|
||
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
|
||
// reach into either.
|
||
tmuxList.mockResolvedValue([summary(U1, 1_000, true)]);
|
||
const mgr = createSessionManager(CFG);
|
||
|
||
expect(await mgr.killOrphansIdleSince(5_000)).toEqual([]);
|
||
expect(tmuxKill).not.toHaveBeenCalled();
|
||
});
|
||
|
||
it('never kills a session the table owns, however idle tmux thinks it is', async () => {
|
||
const mgr = createSessionManager(CFG);
|
||
const live = mgr.handleAttach(createMockWs(), null, DIMS, 1_000);
|
||
tmuxList.mockResolvedValue([summary(live.meta.id, 0)]);
|
||
|
||
expect(await mgr.killOrphansIdleSince(5_000)).toEqual([]);
|
||
expect(tmuxKill).not.toHaveBeenCalled();
|
||
});
|
||
|
||
it('does nothing when tmux is off', async () => {
|
||
tmuxList.mockResolvedValue([summary(U1, 0)]);
|
||
const mgr = createSessionManager(CFG_NO_TMUX);
|
||
|
||
expect(await mgr.killOrphansIdleSince(5_000)).toEqual([]);
|
||
expect(tmuxKill).not.toHaveBeenCalled();
|
||
});
|
||
});
|
||
|
||
describe('countOrphansIdleSince', () => {
|
||
it('counts exactly what killOrphansIdleSince would kill, killing nothing', async () => {
|
||
// The UI confirmation must state this number, not the number of cards it is
|
||
// showing: the grid is capped, so counting cards understates the blast radius.
|
||
tmuxList.mockResolvedValue([summary(U1, 1_000), summary(U2, 9_000)]);
|
||
const mgr = createSessionManager(CFG);
|
||
|
||
expect(await mgr.countOrphansIdleSince(5_000)).toBe(1);
|
||
expect(tmuxKill).not.toHaveBeenCalled();
|
||
});
|
||
|
||
it('does not count attached sessions', async () => {
|
||
tmuxList.mockResolvedValue([summary(U1, 1_000, true), summary(U2, 1_000)]);
|
||
const mgr = createSessionManager(CFG);
|
||
|
||
expect(await mgr.countOrphansIdleSince(5_000)).toBe(1);
|
||
});
|
||
|
||
it('is 0 when tmux is off', async () => {
|
||
const mgr = createSessionManager(CFG_NO_TMUX);
|
||
expect(await mgr.countOrphansIdleSince(5_000)).toBe(0);
|
||
});
|
||
});
|