Skip to content

Commit 1f676b1

Browse files
Ameclaude
authored andcommitted
fix(workspaces): unstick PTY on socket drop + dev terminal transport hardening
PTY backpressure deadlock: detach()/attach() cleared the `paused` flag without resuming the PTY read stream, so a socket dropped mid-backpressure (e.g. a vite ws-proxy ECONNRESET) left the child blocked on its next stdout write forever — the "running along and then freezes" symptom. Add resumePty() and call it on both the detach (socket dropped) and attach-kick (new client) paths; the in-memory ring buffer absorbs output when no client is attached, so pausing the PTY without a consumer was never needed. New spec covers detach-while-paused, attach-kick, and the never-paused no-op. Frontend: auto-reconnect the terminal WebSocket with capped backoff (was: freeze on any drop, zero recovery). term.reset() before a reconnect's cold replay so the server's full-buffer replay repaints instead of duplicating scrollback. 4001 (kicked) / 4404 (session gone) deliberately don't reconnect. Dev transport: connect the terminal WS straight to the backend port, bypassing the vite dev proxy whose WS forwarding chokes on the terminal byte stream (read ECONNRESET) and adds a buffer+copy hop per frame. Gated to import.meta.env.DEV — stripped from production (same-origin) builds. Loopback auth passthrough + the Guardian-injected :5173 origin allowlist admit the direct connection unchanged. Observability: add `host` to upgrade.accepted so direct-vs-proxied is grep-able from the backend log (origin stays :5173 either way). Downgrade the 3 path.trace traces info→event (file-only unless WEB_TERMINAL_LOG_LEVEL=debug) to de-noise `pnpm dev`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 41d2061 commit 1f676b1

9 files changed

Lines changed: 333 additions & 58 deletions

File tree

src/webui/routes/workspaces.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -610,7 +610,7 @@ export function createWorkspaceRoutes(svc: WorkspaceService): Hono {
610610
// we're ABOUT to do, before bootstrap or spawn. If a downstream step
611611
// diverges (e.g. claude CLI writes jsonl to a different projectKey),
612612
// we compare this against the transcript.watch.register trace.
613-
launcherLogger.info('path.trace', {
613+
launcherLogger.event('path.trace', {
614614
where: 'resume.attempt',
615615
wsId: id,
616616
recordId: token,

src/webui/workspaces-ws.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@ export function attachWorkspacesWS(httpServer: HttpServer, svc: WorkspaceService
8484
if (!isOriginAllowed(req, svc)) {
8585
launcherLogger.warn('upgrade.origin_rejected', {
8686
origin: req.headers.origin ?? null,
87+
host: req.headers.host ?? null,
8788
remoteAddress: req.socket.remoteAddress ?? null,
8889
});
8990
socket.write('HTTP/1.1 403 Forbidden\r\n\r\n');
@@ -139,6 +140,12 @@ export function attachWorkspacesWS(httpServer: HttpServer, svc: WorkspaceService
139140
rows,
140141
since: since ?? null,
141142
origin: req.headers.origin ?? null,
143+
// Host the browser actually connected to. Discriminates the dev
144+
// transport: `localhost:<backendPort>` = direct (proxy bypassed),
145+
// `localhost:5173` = forwarded through the Vite dev proxy (which
146+
// preserves the inbound Host). origin stays 5173 either way, so host is
147+
// the only field that tells them apart.
148+
host: req.headers.host ?? null,
142149
remoteAddress: req.socket.remoteAddress ?? null,
143150
});
144151
try {
Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,155 @@
1+
/**
2+
* Regression tests for the backpressure-pause / socket-drop deadlock.
3+
*
4+
* The PTY read stream is paused when an attached WebSocket falls behind
5+
* (bufferedAmount >= high watermark). If that socket then dies *while paused*
6+
* — e.g. a vite ws-proxy ECONNRESET — the session must resume the PTY, or the
7+
* child blocks forever on its next stdout write ("running along and then
8+
* freezes"). These tests pin the resume on both the detach (socket dropped)
9+
* and the attach-kick (a new client replaces the stalled one) paths.
10+
*
11+
* node-pty is mocked so we can drive onData / pause / resume deterministically
12+
* without spawning a real child.
13+
*/
14+
15+
import { EventEmitter } from 'node:events';
16+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
17+
import * as pty from 'node-pty';
18+
19+
import { PersistentSession, type PersistentSessionOptions } from './persistent-session.js';
20+
import type { Logger } from './logger.js';
21+
22+
vi.mock('node-pty', () => ({ spawn: vi.fn() }));
23+
24+
const mockSpawn = vi.mocked(pty.spawn);
25+
26+
/** Minimal IPty stand-in that lets the test inject PTY output and observe
27+
* pause/resume calls. */
28+
function makeFakeTerm() {
29+
let dataCb: ((d: unknown) => void) | undefined;
30+
return {
31+
pid: 4321,
32+
pause: vi.fn(),
33+
resume: vi.fn(),
34+
resize: vi.fn(),
35+
kill: vi.fn(),
36+
write: vi.fn(),
37+
clear: vi.fn(),
38+
onData: (cb: (d: unknown) => void) => {
39+
dataCb = cb;
40+
return { dispose: () => {} };
41+
},
42+
onExit: () => ({ dispose: () => {} }),
43+
/** test helper — push bytes through the captured onData handler */
44+
emitData: (d: Buffer) => dataCb?.(d),
45+
};
46+
}
47+
48+
/** ws.WebSocket stand-in: EventEmitter (for on/off) + send/close + the two
49+
* fields the backpressure logic reads. */
50+
class FakeWs extends EventEmitter {
51+
readonly OPEN = 1;
52+
readyState = 1;
53+
bufferedAmount = 0;
54+
send = vi.fn((data: unknown, optsOrCb?: unknown, cb?: unknown) => {
55+
const callback = typeof optsOrCb === 'function' ? optsOrCb : cb;
56+
if (typeof callback === 'function') callback(undefined);
57+
});
58+
close = vi.fn();
59+
}
60+
61+
const silentLogger: Logger = {
62+
debug: () => {},
63+
info: () => {},
64+
warn: () => {},
65+
error: () => {},
66+
event: () => {},
67+
child: () => silentLogger,
68+
};
69+
70+
function makeOptions(over: Partial<PersistentSessionOptions> = {}): PersistentSessionOptions {
71+
return {
72+
wsId: 'ws-1',
73+
recordId: 'rec-1',
74+
name: 'c1',
75+
command: ['claude'],
76+
cwd: '/tmp',
77+
env: {},
78+
initialCols: 80,
79+
initialRows: 24,
80+
logger: silentLogger,
81+
replayBufferBytes: 1 << 20,
82+
highWatermarkBytes: 1024, // small so one write trips backpressure
83+
lowWatermarkBytes: 256,
84+
onDisposed: () => {},
85+
...over,
86+
};
87+
}
88+
89+
describe('PersistentSession backpressure / socket-drop deadlock', () => {
90+
let term: ReturnType<typeof makeFakeTerm>;
91+
92+
beforeEach(() => {
93+
term = makeFakeTerm();
94+
mockSpawn.mockReturnValue(term as unknown as pty.IPty);
95+
});
96+
97+
afterEach(() => {
98+
vi.clearAllMocks();
99+
});
100+
101+
it('resumes the PTY when a backpressure-paused socket drops (detach)', () => {
102+
const session = new PersistentSession(makeOptions());
103+
const ws = new FakeWs();
104+
session.attach(ws as never, 80, 24, undefined);
105+
106+
// Socket is full: the next chunk of PTY output trips the high watermark
107+
// and pauses the read stream.
108+
ws.bufferedAmount = 2048;
109+
term.emitData(Buffer.from('a lot of output'));
110+
expect(term.pause).toHaveBeenCalledTimes(1);
111+
expect(term.resume).not.toHaveBeenCalled();
112+
113+
// The socket dies mid-backpressure (ECONNRESET) → 'close' → detach().
114+
ws.emit('close');
115+
116+
// Without the fix the PTY stays paused forever and the agent freezes.
117+
expect(term.resume).toHaveBeenCalledTimes(1);
118+
119+
session.dispose('test');
120+
});
121+
122+
it('resumes a stalled PTY when a new client kicks the old one (attach)', () => {
123+
const session = new PersistentSession(makeOptions());
124+
const ws1 = new FakeWs();
125+
session.attach(ws1 as never, 80, 24, undefined);
126+
127+
ws1.bufferedAmount = 2048;
128+
term.emitData(Buffer.from('a lot of output'));
129+
expect(term.pause).toHaveBeenCalledTimes(1);
130+
131+
// A fresh tab attaches and kicks ws1. The new attach must un-stick the PTY
132+
// even though the kick path (not detach) cleared the old socket.
133+
const ws2 = new FakeWs();
134+
session.attach(ws2 as never, 80, 24, undefined);
135+
136+
expect(term.resume).toHaveBeenCalledTimes(1);
137+
138+
session.dispose('test');
139+
});
140+
141+
it('does not resume a PTY that was never paused', () => {
142+
const session = new PersistentSession(makeOptions());
143+
const ws = new FakeWs();
144+
session.attach(ws as never, 80, 24, undefined);
145+
146+
// Output flows while the socket drains fine — no pause, no resume churn.
147+
term.emitData(Buffer.from('small'));
148+
ws.emit('close');
149+
150+
expect(term.pause).not.toHaveBeenCalled();
151+
expect(term.resume).not.toHaveBeenCalled();
152+
153+
session.dispose('test');
154+
});
155+
});

src/workspaces/persistent-session.ts

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -305,7 +305,10 @@ export class PersistentSession {
305305
}
306306

307307
this.ws = ws;
308-
this.paused = false;
308+
// A previous client may have dropped mid-backpressure, leaving the PTY
309+
// paused at the OS level. Clearing only the flag (not resuming the term)
310+
// would strand it paused forever; resumePty() un-sticks both.
311+
this.resumePty();
309312
this.resize(cols, rows);
310313

311314
// Compute replay window. Cold attach (since=undefined) replays the full
@@ -351,6 +354,12 @@ export class PersistentSession {
351354
const ws = this.ws;
352355
this.ws = null;
353356
this.unwireWs(ws);
357+
// No consumer left — let the PTY run free into the in-memory ring buffer
358+
// (onPtyData appends regardless of socket). If the socket dropped while
359+
// we were backpressure-paused, NOT resuming here strands the PTY paused
360+
// forever and the agent blocks on its next stdout write — the "running
361+
// along and then freezes" symptom.
362+
this.resumePty();
354363
if (this.cursorTimer) {
355364
clearInterval(this.cursorTimer);
356365
this.cursorTimer = null;
@@ -425,6 +434,19 @@ export class PersistentSession {
425434
}
426435
}
427436

437+
/** Un-pause the PTY read stream if backpressure paused it. Safe to call when
438+
* already running. Keeping the `paused` flag and the term stream in lockstep
439+
* is the whole point — clearing one without the other deadlocks the PTY. */
440+
private resumePty(): void {
441+
if (!this.paused) return;
442+
this.paused = false;
443+
try {
444+
this.term.resume();
445+
} catch {
446+
// PTY may be dying; ignore.
447+
}
448+
}
449+
428450
private onWsMessage(ws: WebSocket, raw: unknown, isBinary: boolean): void {
429451
if (this.disposed) return;
430452
if (this.ws !== ws) return; // stale (this ws was kicked)

src/workspaces/service.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -501,7 +501,7 @@ export async function createWorkspaceService(opts: CreateWorkspaceServiceOptions
501501
// raison d'être of the workspace-sessions.log file: any two fields that
502502
// should be equal but aren't are the bug, eyeball-comparable. Keep this
503503
// verbose; the file is grep-only, not human-tailed.
504-
launcherLogger.info('path.trace', {
504+
launcherLogger.event('path.trace', {
505505
where: 'session.spawn',
506506
wsId,
507507
recordId: ctx.recordId,

src/workspaces/transcript-watcher.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,7 @@ export class TranscriptWatcher {
160160
// Compare watchDir + projectKey against the spawn path.trace; any
161161
// divergence means the CLI will write jsonl to a place we're not
162162
// watching, and resumeHint will never be populated.
163-
this.logger.info('path.trace', {
163+
this.logger.event('path.trace', {
164164
where: 'transcript.watch.register',
165165
wsId: session.wsId,
166166
recordId: session.recordId,

0 commit comments

Comments
 (0)