kai/web-terminal
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

fix(sessions): orphan cleanup was reading the wrong tmux clock

Browse Source 
Adversarial review of the previous commit found ten defects. The critical one
would have destroyed exactly the work this feature exists to protect.

CRITICAL — `#{session_activity}` is a CLIENT clock, not an output clock.
It advances on attach and on keypresses, and never moves for a detached session
however much its shell is printing. Measured on tmux 3.6a: a detached session
running `while true; do echo tick; sleep 1; done` still reported
session_activity == session_created after 6 s, while `#{window_activity}` tracked
the output exactly. So "idle for 7 days" was really "nobody has typed for 7 days",
and `Clean up idle 7d+` would have killed long-running unattended sessions that
were busy working — the walk-away case the whole app is built around.
Measured against this host's 69 sessions: the old clock condemned 36, the new one
condemns 33, so three sessions doing real work were one click from deletion.
Now reads window_activity, with a test that fails if the format string regresses.

HIGH — no timeout on any tmux call. maxBuffer bounds a flood but nothing bounded
the wait, and a synchronous exec cannot be interrupted by the event loop, so a
tmux server stopped mid-syscall hung the whole server permanently. All five calls
(including the three that predate this feature) now carry one.

HIGH — listSessions and capturePane were synchronous on a POLLED path. Measured
73 ms and 58 ms per call on this host; every home screen on every device refreshes
on a 5 s timer, and each of those was 73 ms with the event loop stopped, i.e.
73 ms in which no PTY byte reached any terminal anywhere. Both are async now, so
the manager's listOrphans/captureOrphan/killOrphansIdleSince are too. The server
is a byte-shuttle first: nothing polled may block it.

HIGH — GET /orphan-sessions had no rate limiter although, unlike every other read
route, it spawns a subprocess. It shares the preview bucket now.

HIGH — the cleanup confirmation counted CARDS, and the grid is capped at 24 while
the cleanup matches across every session the server enumerates. On this host it
would have said "24" and ended 33 shells. New GET /orphan-sessions/count-idle
answers the real question, and the dialog states that number, or refuses to
proceed if it cannot be determined.

MEDIUM — fetchOrphanSessions collapsed every failure into [], so one 429 or 500
read as "all recovered sessions are gone", tearing down every mounted xterm and
rebuilding it on the next success. It returns null for failure now, and the
refresh leaves the section untouched.

MEDIUM — the preview latch was "card was created", not "preview succeeded", so a
card that lost its single request stayed blank forever. Latches on success.

MEDIUM — overlapping refreshes. refresh() is entered from a 5 s timer, from
killOrphan and from cleanup, with awaits inside and no guard, so a slow poll could
resurrect cards for sessions already killed or re-mount them into a root that
setVisible(false) had just cleared. A generation counter, bumped on teardown and
checked after every await, invalidates stale runs.

MEDIUM — renderPreview could write to a disposed xterm (it throws "Object has
been disposed"). PreviewCard carries an explicit `disposed` flag set by a new
disposeCard() helper, so no caller can dispose a terminal and forget the flag.
Deliberately NOT el.isConnected: a card not yet appended is alive, and two
existing preview-grid tests correctly caught that conflation.

LOW — kill and cleanup discarded their results, so a refusal (403 behind a proxy
whose Origin differs, 404 on a lost race) looked like a dead button. Both report.

Tests: 2209 unit, 27 e2e, 8 orphan integration. Verified live against all 69 real
sessions with none harmed; the window_activity fix confirmed on an isolated tmux
socket (-L) so the developer's sessions were never involved.
This commit is contained in:
Yaojia Wang
2026-07-30 11:28:39 +02:00
parent 4892fa7b49
commit f6ef19ebf6
8 changed files with 330 additions and 120 deletions
Download Patch File Download Diff File Expand all files Collapse all files

98
test/manager-orphans.test.ts
View File

@@ -26,8 +26,8 @@ vi.mock('node-pty', () => ({
}));
// ── tmux CLI mock ────────────────────────────────────────────────────────────
const tmuxList = vi.fn<() => unknown[]>(() => []);
const tmuxCapture = vi.fn<(name: string) => string | null>(() => null);
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);
@@ -120,18 +120,18 @@ function summary(id: string, activityMs = 5_000, attached = false) {
beforeEach(() => {
nextPty = createMockPty();
tmuxList.mockReset().mockReturnValue([]);
tmuxCapture.mockReset().mockReturnValue(null);
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', () => {
tmuxList.mockReturnValue([summary(U1, 9_000, true)]);
it('reports a tmux session the table does not know about', async () => {
tmuxList.mockResolvedValue([summary(U1, 9_000, true)]);
const mgr = createSessionManager(CFG);
expect(mgr.listOrphans()).toEqual([
expect(await mgr.listOrphans()).toEqual([
{
id: U1,
createdAt: 1_000,
@@ -143,59 +143,59 @@ describe('listOrphans', () => {
]);
});
it('excludes sessions the table already owns — those are live, not orphans', () => {
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.mockReturnValue([summary(live.meta.id), summary(U2)]);
tmuxList.mockResolvedValue([summary(live.meta.id), summary(U2)]);
expect(mgr.listOrphans().map((o) => o.id)).toEqual([U2]);
expect((await mgr.listOrphans()).map((o) => o.id)).toEqual([U2]);
});
it('is empty when tmux is off — the non-tmux deployment is untouched', () => {
tmuxList.mockReturnValue([summary(U1)]);
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(mgr.listOrphans()).toEqual([]);
expect(await mgr.listOrphans()).toEqual([]);
expect(tmuxList).not.toHaveBeenCalled();
});
});
describe('captureOrphan', () => {
it('returns the captured screen for an untracked session', () => {
tmuxCapture.mockReturnValue('screen contents');
it('returns the captured screen for an untracked session', async () => {
tmuxCapture.mockResolvedValue('screen contents');
const mgr = createSessionManager(CFG);
expect(mgr.captureOrphan(U1)).toBe('screen contents');
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', () => {
it('refuses an id that is not a UUID v4 without invoking tmux at all', async () => {
const mgr = createSessionManager(CFG);
expect(mgr.captureOrphan('../../etc/passwd')).toBeNull();
expect(mgr.captureOrphan('-t')).toBeNull();
expect(await mgr.captureOrphan('../../etc/passwd')).toBeNull();
expect(await mgr.captureOrphan('-t')).toBeNull();
expect(tmuxCapture).not.toHaveBeenCalled();
});
it('refuses when no such tmux session exists', () => {
it('refuses when no such tmux session exists', async () => {
tmuxHas.mockReturnValue(false);
const mgr = createSessionManager(CFG);
expect(mgr.captureOrphan(U1)).toBeNull();
expect(await mgr.captureOrphan(U1)).toBeNull();
expect(tmuxCapture).not.toHaveBeenCalled();
});
it('refuses an id the table owns — a live session previews from its ring buffer', () => {
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(mgr.captureOrphan(live.meta.id)).toBeNull();
expect(await mgr.captureOrphan(live.meta.id)).toBeNull();
expect(tmuxCapture).not.toHaveBeenCalled();
});
it('returns null when tmux is off', () => {
it('returns null when tmux is off', async () => {
const mgr = createSessionManager(CFG_NO_TMUX);
expect(mgr.captureOrphan(U1)).toBeNull();
expect(await mgr.captureOrphan(U1)).toBeNull();
expect(tmuxCapture).not.toHaveBeenCalled();
});
});
@@ -241,40 +241,64 @@ describe('killOrphan', () => {
});
describe('killOrphansIdleSince', () => {
it('kills only sessions whose last activity is older than the cutoff', () => {
tmuxList.mockReturnValue([summary(U1, 1_000), summary(U2, 9_000)]);
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(mgr.killOrphansIdleSince(5_000)).toEqual([U1]);
expect(await mgr.killOrphansIdleSince(5_000)).toEqual([U1]);
expect(tmuxKill).toHaveBeenCalledWith(`web_${U1}`);
expect(tmuxKill).toHaveBeenCalledTimes(1);
});
it('never kills a session someone is attached to from a terminal', () => {
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.mockReturnValue([summary(U1, 1_000, true)]);
tmuxList.mockResolvedValue([summary(U1, 1_000, true)]);
const mgr = createSessionManager(CFG);
expect(mgr.killOrphansIdleSince(5_000)).toEqual([]);
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', () => {
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.mockReturnValue([summary(live.meta.id, 0)]);
tmuxList.mockResolvedValue([summary(live.meta.id, 0)]);
expect(mgr.killOrphansIdleSince(5_000)).toEqual([]);
expect(await mgr.killOrphansIdleSince(5_000)).toEqual([]);
expect(tmuxKill).not.toHaveBeenCalled();
});
it('does nothing when tmux is off', () => {
tmuxList.mockReturnValue([summary(U1, 0)]);
it('does nothing when tmux is off', async () => {
tmuxList.mockResolvedValue([summary(U1, 0)]);
const mgr = createSessionManager(CFG_NO_TMUX);
expect(mgr.killOrphansIdleSince(5_000)).toEqual([]);
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);
});
});

81
test/tmux.test.ts
View File

@@ -8,8 +8,15 @@
import { describe, it, expect, vi, beforeEach } from 'vitest'
const mockExec = vi.fn()
// promisify(execFile) reads the custom symbol, so give it one that returns our mock.
const mockExecAsync = vi.fn()
const fakeExecFile = Object.assign(
(...a: unknown[]) => mockExecAsync(...a),
{ [Symbol.for('nodejs.util.promisify.custom')]: (...a: unknown[]) => mockExecAsync(...a) },
)
vi.mock('node:child_process', () => ({
execFileSync: (...a: unknown[]) => mockExec(...a),
execFile: fakeExecFile,
}))
const { tmuxAvailable, tmuxName, hasSession, killSession, parseSessionList, listSessions, capturePane } =
@@ -17,6 +24,7 @@ const { tmuxAvailable, tmuxName, hasSession, killSession, parseSessionList, list
beforeEach(() => {
mockExec.mockReset()
mockExecAsync.mockReset()
})
describe('tmuxName', () => {
@@ -29,7 +37,7 @@ describe('tmuxAvailable', () => {
it('returns true when `tmux -V` succeeds', () => {
mockExec.mockReturnValue(Buffer.from('tmux 3.4'))
expect(tmuxAvailable()).toBe(true)
expect(mockExec).toHaveBeenCalledWith('tmux', ['-V'], { stdio: 'ignore' })
expect(mockExec).toHaveBeenCalledWith('tmux', ['-V'], expect.objectContaining({ stdio: 'ignore' }))
})
it('returns false when tmux is missing (exec throws)', () => {
@@ -44,7 +52,11 @@ describe('hasSession', () => {
it('returns true when has-session exits 0', () => {
mockExec.mockReturnValue(Buffer.from(''))
expect(hasSession('web_x')).toBe(true)
expect(mockExec).toHaveBeenCalledWith('tmux', ['has-session', '-t', 'web_x'], { stdio: 'ignore' })
expect(mockExec).toHaveBeenCalledWith(
'tmux',
['has-session', '-t', 'web_x'],
expect.objectContaining({ stdio: 'ignore' }),
)
})
it('returns false when has-session throws (no such session)', () => {
@@ -59,7 +71,11 @@ describe('killSession', () => {
it('invokes tmux kill-session for the name', () => {
mockExec.mockReturnValue(Buffer.from(''))
killSession('web_x')
expect(mockExec).toHaveBeenCalledWith('tmux', ['kill-session', '-t', 'web_x'], { stdio: 'ignore' })
expect(mockExec).toHaveBeenCalledWith(
'tmux',
['kill-session', '-t', 'web_x'],
expect.objectContaining({ stdio: 'ignore' }),
)
})
it('swallows errors when the session is already gone', () => {
@@ -123,28 +139,46 @@ describe('parseSessionList', () => {
})
describe('listSessions', () => {
it('asks tmux for a machine-readable list', () => {
mockExec.mockReturnValue(`web_${U1}\t100\t100\t0\t80\t24\n`)
expect(listSessions().map((s) => s.id)).toEqual([U1])
const [file, args] = mockExec.mock.calls[0] as [string, string[]]
it('asks tmux for a machine-readable list', async () => {
mockExecAsync.mockResolvedValue({ stdout: `web_${U1}\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')
expect(args[0]).toBe('list-sessions')
expect(args).toContain('-F')
})
it('returns [] when there is no tmux server (exec throws)', () => {
mockExec.mockImplementation(() => {
throw new Error('no server running')
})
expect(listSessions()).toEqual([])
it('asks for window_activity, NOT session_activity', async () => {
// session_activity is a CLIENT clock: it never advances for a detached session
// however much its shell prints. Using it would make idle-cleanup kill exactly
// the unattended, actively-working sessions this feature exists to protect.
mockExecAsync.mockResolvedValue({ stdout: '', stderr: '' })
await 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}')
})
it('bounds the call so a wedged tmux cannot hang the server', async () => {
mockExecAsync.mockResolvedValue({ stdout: '', stderr: '' })
await listSessions()
const opts = mockExecAsync.mock.calls[0]![2] as { timeout?: number; maxBuffer?: number }
expect(opts.timeout).toBeGreaterThan(0)
expect(opts.maxBuffer).toBeGreaterThan(0)
})
it('returns [] when there is no tmux server (exec rejects)', async () => {
mockExecAsync.mockRejectedValue(new Error('no server running'))
expect(await listSessions()).toEqual([])
})
})
describe('capturePane', () => {
it('captures the current screen WITHOUT attaching', () => {
mockExec.mockReturnValue('hello\n')
expect(capturePane('web_x')).toBe('hello\n')
const [file, args] = mockExec.mock.calls[0] as [string, string[]]
it('captures the current screen WITHOUT attaching', async () => {
mockExecAsync.mockResolvedValue({ stdout: 'hello\n', stderr: '' })
expect(await capturePane('web_x')).toBe('hello\n')
const [file, args] = mockExecAsync.mock.calls[0] as [string, string[]]
expect(file).toBe('tmux')
expect(args[0]).toBe('capture-pane')
expect(args).toContain('-p') // print to stdout — read-only, no client
@@ -154,10 +188,15 @@ describe('capturePane', () => {
expect(args).not.toContain('attach-session')
})
it('returns null when the session is gone', () => {
mockExec.mockImplementation(() => {
throw new Error("can't find session")
})
expect(capturePane('web_gone')).toBeNull()
it('returns null when the session is gone', async () => {
mockExecAsync.mockRejectedValue(new Error("can't find session"))
expect(await capturePane('web_gone')).toBeNull()
})
it('bounds the call', async () => {
mockExecAsync.mockResolvedValue({ stdout: '', stderr: '' })
await capturePane('web_x')
const opts = mockExecAsync.mock.calls[0]![2] as { timeout?: number }
expect(opts.timeout).toBeGreaterThan(0)
})
})