22a7929a7b56932af8821cee252a9fbce00c8ac5
3 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Yaojia Wang
|
f6ef19ebf6 |
fix(sessions): orphan cleanup was reading the wrong tmux clock
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.
|
||
|
Yaojia Wang
|
4892fa7b49 |
feat(sessions): surface tmux sessions the server does not track
A `web_*` tmux session that outlives the process which created it was unreachable
from the UI, permanently. Nothing enumerated tmux — src/session/tmux.ts had
hasSession but no list — so recovery only worked if a client still had the id in
localStorage. list() walks the in-memory table, and so does reapIdle, so such a
session could not be listed, previewed, joined or killed, and never aged out.
On this host that was 69 tmux sessions with 5 clients: 64 unreachable, the oldest
from 26 Jun, one of them still running a Claude Code session with 7 agents.
This is the "catalogue" approach: make them visible and actionable WITHOUT
adopting them. Nothing is attached, because attaching makes tmux resize the window
to the new client's dimensions and SIGWINCH whatever is running inside — doing
that to dozens of live TUIs at startup is exactly the outcome to avoid. Adoption
stays an explicit user action: opening an orphan re-attaches by id through the
existing re-attach path, and it becomes an ordinary live session.
src/session/tmux.ts listSessions() + parseSessionList() (pure, so the filter
rules are unit-testable) and capturePane(), which reads a
session's current screen via `capture-pane -p -e` — no
client, no resize, colour preserved.
src/session/manager.ts listOrphans/captureOrphan/killOrphan/killOrphansIdleSince
src/server.ts GET /orphan-sessions, GET /orphan-sessions/:id/preview,
DELETE /orphan-sessions/:id, DELETE /orphan-sessions
?idleDays=N
public/ a "Recoverable" section under the home grid, reusing the
existing card/thumbnail machinery via orphanAsLive()
Guards, all enforced in the manager so no route can forget one:
- the tmux name must be `web_` + a UUID v4 (SESSION_ID_RE, the same gate the
attach protocol applies). This is what stops a hostile or malformed name from
reaching `tmux -t` — a literal `-t`, or path traversal — and it means a tmux
session the user created for their own work is never enumerated, never
previewed and never offered for deletion.
- an id the table already owns is refused everywhere: killing it via tmux would
end the shell behind a live PTY's back and leave a zombie table entry. Those go
through killById.
- every function is inert when cfg.useTmux is off, so the non-tmux deployment is
byte-for-byte unchanged.
- bulk cleanup skips ATTACHED sessions and requires idleDays >= 1: there is no
"delete everything" form of the route. "This server does not track it" is not
evidence that it is abandoned — a plain `tmux attach` and a second server
process both report as attached.
- reads carry no Origin guard (same threat model as /live-sessions); both DELETEs
do. Preview gets its own rate-limit bucket since each call spawns a process.
Two things the live run exposed and this fixes: previews were loading
sequentially (69 tmux spawns per 5 s refresh) — now once per card, fire-and-forget
— and the grid is capped at 24 cards because each thumbnail is an xterm instance.
The remainder is stated in the UI with the tmux command to reach it, never
silently truncated.
Separate wire type from LiveSessionInfo on purpose: an orphan supports none of the
live-session operations, and the Android/iOS clients decode /live-sessions, so a
variant shape there would break them.
Tests: 2203 unit (+42: parse/filter rules, all four manager guards, tmux-off
inertness) and 8 integration against real tmux — create a throwaway session,
list it, capture it while asserting it stays unattached, reject non-UUID ids,
reject a foreign Origin, kill it, 404 the second time. The integration file never
issues the bulk DELETE: it runs against the host's real tmux server and would end
the developer's own sessions. Verified live against all 69 with none harmed.
|
||
|
Yaojia Wang
|
d22dcd24f7 |
fix: address review report across security, architecture, quality, tests
Implements the fixes from docs/REVIEW_REPORT.md (4-agent parallel review). typecheck clean; 341 tests pass (16 files, +113); build:web ok; coverage thresholds (80%) enforced in vitest.config.ts. Critical: - multi-device approval race: release held approval only when the last client detaches (closing one mirror no longer cancels another's prompt) - unbounded session creation (DoS): Config.maxSessions cap (env MAX_SESSIONS), enforced in manager via the existing M4 exit(-1) path - signal-handler leak: named SIGINT/SIGTERM/uncaughtException refs removed in close() - terminal-session initialInput timer tracked + cleared on dispose - tabs.addEntry null-as-cast type hole removed (build session before entry) Should-fix: - security-headers middleware + Origin/CSRF guard on DELETE /live-sessions[/:id] - history.ts converted to fs/promises (async /sessions handler) - removed dead clientDims map + blur protocol message end-to-end - per-connection WS message rate limit (Config.maxMsgsPerSec) - /sessions behavior kept; documented as accepted LAN risk (TECH_DOC §7) Tests: - new tmux / preview-grid / terminal-session (jsdom) / tabs (jsdom) suites - extended history/config/manager/integration coverage incl. regressions Hygiene: - parsePositiveInt -> parseNonNegativeInt; ALLOWED_ORIGINS scheme validation - log-injection sanitize; isLoopback handles 127.0.0.0/8 + IPv4-mapped - operational constants moved into Config - extracted public/preview-grid.ts (DRY launcher/manage) - doc sweeps: ARCHITECTURE §8 runtime-handle exception, stale comments |