Repository navigation
fix(pty-daemon): pause flooding PTYs on backpressure instead of destroying the shared socket #5747
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -35,6 +35,8 @@ export interface ServerOptions { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| daemonVersion: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| bufferCap?: number; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| outboundBufferCap?: number; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** Pause producing PTYs when a subscriber buffers past this (flow control). */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| outboundPauseThreshold?: number; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Override for the PTY-spawn factory. Production leaves this unset; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * `defaultSpawn` (real node-pty) is used. Tests inject a fake here so | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -45,10 +47,20 @@ export interface ServerOptions { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const DEFAULT_OUTBOUND_BUFFER_CAP_BYTES = 8 * 1024 * 1024; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Flow control: when a subscriber's socket buffers more than this, pause the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // producing PTYs instead of letting the buffer grow toward the destroy cap. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // The kernel PTY buffer then fills and the foreground process blocks on | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // write — the flood throttles itself at the source (VS Code ptyHost does the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // same via pause/resume; our congestion signal is local writableLength | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // instead of renderer ACKs). Resume on socket 'drain'. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const DEFAULT_OUTBOUND_PAUSE_THRESHOLD_BYTES = 1 * 1024 * 1024; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| interface ConnState extends Conn { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| socket: net.Socket; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| decoder: FrameDecoder; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| negotiated: number | null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** Session ids whose PTYs we paused because this conn was congested. */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pausedSessions: Set<string>; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export class Server { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -317,11 +329,17 @@ export class Server { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| decoder: new FrameDecoder(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| negotiated: null, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| subscriptions: new Set(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pausedSessions: new Set(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| send: (msg, payload) => | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| writeMessage(socket, msg, payload, outboundBufferCap), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.conns.add(conn); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Flow control: the buffer only reaches the pause threshold after | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // write() has returned false, so a 'drain' (buffer fully flushed) is | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // guaranteed to follow and is our resume signal. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| socket.on("drain", () => this.resumePausedSessions(conn)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| socket.on("data", (chunk) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| conn.decoder.push(chunk); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -460,13 +478,24 @@ export class Server { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * subscribed to this session id receives the output / exit frames. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private wireSession(session: Session): void { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const pauseThreshold = | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Lowering Prompt for AI agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.opts.outboundPauseThreshold ?? | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| DEFAULT_OUTBOUND_PAUSE_THRESHOLD_BYTES; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| session.pty.onData((chunk) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.store.appendOutput(session, chunk); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const out: ServerMessage = { type: "output", id: session.id }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let congested = false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (const c of this.conns) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!c.subscriptions.has(session.id)) continue; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| c.send(out, chunk); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!c.socket.destroyed && c.socket.writableLength > pauseThreshold) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| c.pausedSessions.add(session.id); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Replay bursts bypass the new per-session backpressure check, so subscribing a slow socket to enough buffered sessions can still hit Prompt for AI agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| congested = true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Paused PTYs emit no further onData, so this fires at most once | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // per congestion episode; the conn's 'drain' resumes us. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (congested) session.pty.pause(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| session.pty.onExit((info) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| session.exited = true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -483,6 +512,7 @@ export class Server { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| c.send(ev); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| c.subscriptions.delete(session.id); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| c.pausedSessions.delete(session.id); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Delete the session immediately. Without this, every closed | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // terminal pane left a row in the store forever — list-reply | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -500,6 +530,28 @@ export class Server { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private dropConn(conn: ConnState): void { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.conns.delete(conn); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // A destroyed socket never drains — release its paused producers here. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.resumePausedSessions(conn); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Resume every PTY paused on behalf of `conn`, unless another congested | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * conn still holds it paused. Called on socket 'drain' and on conn drop. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private resumePausedSessions(conn: ConnState): void { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (const id of conn.pausedSessions) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| conn.pausedSessions.delete(id); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let heldElsewhere = false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (const other of this.conns) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (other !== conn && other.pausedSessions.has(id)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| heldElsewhere = true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| break; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (heldElsewhere) continue; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const session = this.store.get(id); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (session && !session.exited) session.pty.resume(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+541
to
+554
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win Fix potential infinite loop during Set iteration. Iterating over Clone the 🔒️ Proposed fix to isolate the iteration state private resumePausedSessions(conn: ConnState): void {
- for (const id of conn.pausedSessions) {
- conn.pausedSessions.delete(id);
+ const toResume = Array.from(conn.pausedSessions);
+ conn.pausedSessions.clear();
+ for (const id of toResume) {
let heldElsewhere = false;
for (const other of this.conns) {
if (other !== conn && other.pausedSessions.has(id)) {
heldElsewhere = true;
break;
}
}
if (heldElsewhere) continue;
const session = this.store.get(id);
if (session && !session.exited) session.pty.resume();
}
}📝 Committable suggestion
Suggested change
🧰 Tools🪛 ast-grep (0.44.1)[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec. (detect-child-process-typescript) 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+541
to
555
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
const ids = [...conn.pausedSessions];
conn.pausedSessions.clear();
for (const id of ids) { … }This also makes it clearer that the set is meant to be fully drained on every call. Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/pty-daemon/src/Server/Server.ts
Line: 541-555
Comment:
**Set mutated while being iterated**
`conn.pausedSessions.delete(id)` removes the element that was just yielded by the outer `for...of` iterator over that same Set. This is technically safe in JavaScript — the spec guarantees the remaining elements are still visited — but it's an unusual pattern that relies on this subtlety. A harder-to-misread form would be to snapshot the set first and clear it up front:
```ts
const ids = [...conn.pausedSessions];
conn.pausedSessions.clear();
for (const id of ids) { … }
```
This also makes it clearer that the set is meant to be fully drained on every call.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| // Flow control: a flooding PTY with a slow consumer must pause the producer | ||
| // (kernel backpressure) instead of destroying the shared daemon socket. | ||
| // | ||
| // Regression: writeMessage() used to socket.destroy() when writableLength | ||
| // exceeded the outbound cap — one flooding terminal severed the connection | ||
| // for every session in the org. | ||
| // | ||
| // Runs under Node (`node --experimental-strip-types --test`). | ||
|
|
||
| import { strict as assert } from "node:assert"; | ||
| import * as os from "node:os"; | ||
| import * as path from "node:path"; | ||
| import { after, before, test } from "node:test"; | ||
| import { Server } from "../src/Server/index.ts"; | ||
| import { connectAndHello, payloadAsString } from "./helpers/client.ts"; | ||
|
|
||
| const sockPath = path.join(os.tmpdir(), `pty-daemon-flow-${process.pid}.sock`); | ||
| let server: Server; | ||
|
|
||
| // Small thresholds so the flood trips them in milliseconds. The old destroy | ||
| // behavior fires at outboundBufferCap; with flow control the buffer must stop | ||
| // growing at outboundPauseThreshold, far below the cap. | ||
| const PAUSE_THRESHOLD = 64 * 1024; | ||
| const DESTROY_CAP = 4 * 1024 * 1024; | ||
|
|
||
| const FLOOD_META = { | ||
| shell: "/bin/sh", | ||
| argv: ["-c", 'yes "flood-line-0123456789-abcdefghijklmnopqrstuvwxyz"'], | ||
| cols: 80, | ||
| rows: 24, | ||
| }; | ||
|
|
||
| before(async () => { | ||
| server = new Server({ | ||
| socketPath: sockPath, | ||
| daemonVersion: "0.0.0-test", | ||
| outboundPauseThreshold: PAUSE_THRESHOLD, | ||
| outboundBufferCap: DESTROY_CAP, | ||
| }); | ||
| await server.listen(); | ||
| }); | ||
|
|
||
| after(async () => { | ||
| await server.close(); | ||
| }); | ||
|
|
||
| test("slow consumer pauses the flood instead of destroying the connection", async () => { | ||
| const c = await connectAndHello(sockPath); | ||
| c.send({ type: "open", id: "flow-0", meta: FLOOD_META }); | ||
| await c.waitFor((m) => m.type === "open-ok" && m.id === "flow-0"); | ||
| c.send({ type: "subscribe", id: "flow-0", replay: false }); | ||
| await c.waitFor( | ||
| (m) => m.type === "output" && payloadAsString(m).includes("flood-line"), | ||
| 3000, | ||
| ); | ||
|
|
||
| // Stop reading. At full `yes` rate the old behavior blows through any | ||
| // buffer cap within this window and destroys the socket. | ||
| c.socket.pause(); | ||
| await new Promise((r) => setTimeout(r, 1500)); | ||
| assert.equal( | ||
| c.closed(), | ||
| false, | ||
| "daemon destroyed the shared connection under flood (flow control missing)", | ||
| ); | ||
|
|
||
| // Resume reading: buffered output drains, PTY resumes, data flows again. | ||
| c.socket.resume(); | ||
| await c.waitForNext( | ||
| (m) => m.type === "output" && payloadAsString(m).includes("flood-line"), | ||
| 3000, | ||
| ); | ||
| assert.equal(c.closed(), false); | ||
|
|
||
| c.send({ type: "close", id: "flow-0", signal: "SIGKILL" }); | ||
| await c.waitFor((m) => m.type === "closed" && m.id === "flow-0", 3000); | ||
| await c.close(); | ||
| }); | ||
|
|
||
| test("consumer disconnect while paused resumes the PTY for the next subscriber", async () => { | ||
| const c1 = await connectAndHello(sockPath); | ||
| c1.send({ type: "open", id: "flow-1", meta: FLOOD_META }); | ||
| await c1.waitFor((m) => m.type === "open-ok" && m.id === "flow-1"); | ||
| c1.send({ type: "subscribe", id: "flow-1", replay: false }); | ||
| await c1.waitFor((m) => m.type === "output" && m.id === "flow-1", 3000); | ||
|
|
||
| // Congest c1 until the PTY is paused, then drop the socket hard. The | ||
| // daemon must resume the PTY via the conn-drop path (a destroyed socket | ||
| // never emits 'drain'). | ||
| c1.socket.pause(); | ||
| await new Promise((r) => setTimeout(r, 800)); | ||
| c1.socket.destroy(); | ||
|
Comment on lines
+89
to
+92
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The test pauses Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/pty-daemon/test/flow-control.test.ts
Line: 89-92
Comment:
**Fixed 800 ms sleep to guarantee PTY is paused**
The test pauses `c1`'s socket and sleeps for 800 ms, expecting the PTY to have been paused within that window. On a busy CI runner (or a system where `yes` is slow to start / the socket send buffer is large), 800 ms may not be enough time for the server's `writableLength` to exceed the 64 KB `PAUSE_THRESHOLD`, so `conn.pausedSessions` would be empty when `c1.socket.destroy()` fires and `resumePausedSessions` becomes a no-op — the PTY was never paused. `c2`'s `waitFor` would then succeed trivially (the PTY was always running), meaning the conn-drop resume path was never exercised and the test is a false positive.
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
| const c2 = await connectAndHello(sockPath); | ||
| c2.send({ type: "subscribe", id: "flow-1", replay: false }); | ||
| await c2.waitFor( | ||
| (m) => | ||
| m.type === "output" && | ||
| m.id === "flow-1" && | ||
| payloadAsString(m).includes("flood-line"), | ||
| 3000, | ||
| ); | ||
|
|
||
| c2.send({ type: "close", id: "flow-1", signal: "SIGKILL" }); | ||
| await c2.waitFor((m) => m.type === "closed" && m.id === "flow-1", 3000); | ||
| await c2.close(); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drainguarantee depends on an unvalidated invariantThe comment correctly explains why
drainis guaranteed:writableLength > outboundPauseThresholdimplieswritableLength > highWaterMark(default 16 KB for a Node.jsnet.Socket), which meanssocket.write()has already returnedfalse, guaranteeing a futuredrain. This holds for the defaults (1 MiB pause threshold >> 16 KB HWM) and the test value (64 KB), but theServerOptionsinterface accepts any value. If a caller passesoutboundPauseThresholdbelow the socket'shighWaterMark(e.g., 4 KB), the PTY could be paused with nodrainguaranteed to fire, silently stalling the session until conn drop. An assertion or clamp at construction time would prevent this.Prompt To Fix With AI