From 77fdd2801585f9e1eb847d00364d3ae9ea2f7582 Mon Sep 17 00:00:00 2001 From: Hako Date: Sat, 19 Sep 2026 14:03:46 +0900 Subject: [PATCH] fix(server): share one pty when clients attach the same pane concurrently ensureAttachment checked the attachment map, then awaited the terminal lookup before storing the new record. Two attaches to the same pane in that window each spawned their own `herdr terminal attach`. The later record replaced the earlier one in the map, but the orphaned pty's onData still looked the pane up by id, so it streamed into the surviving attachment: one client went silent, the other got output from two ptys, and the orphan's exit closed the surviving attachment. A concurrent attach now joins the in-flight creation through a pending map that is cleared on success and failure alike. A new contract test races two attaches on a fresh pane and checks a resize reaches both clients; with the fix reverted it fails 3/3. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 2 +- server/api.contract.test.ts | 41 +++++++++++++++++++++++++++++++++++++ server/index.ts | 17 +++++++++++++-- 3 files changed, 57 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index ad03631cd9..523f84bd0d 100644 --- a/README.md +++ b/README.md @@ -159,7 +159,7 @@ bun run build bun test ``` -The suite runs against the live herdr server; there are no mocks. It's read-only apart from the `herdr-web-ui-test` workspaces it creates and deletes. It covers the generator freshness and determinism gate, the HTTP contract, the WS attach stream, roles (an observe connection cannot resize or type, enforced server-side), the status collector's pushed `pane-status` and `pane-exited` for unattached panes, token auth, the bind address, the herdr client, the PTY sidecar's env pass-through (`server/pty/session.test.ts`) and the pure client modules (key bar, draft, notifications, snapshot merge). At the time of writing that's 63 tests across 9 files. +The suite runs against the live herdr server; there are no mocks. It's read-only apart from the `herdr-web-ui-test` workspaces it creates and deletes. It covers the generator freshness and determinism gate, the HTTP contract, the WS attach stream, roles (an observe connection cannot resize or type, enforced server-side), concurrent attaches to one pane sharing a single pty, the status collector's pushed `pane-status` and `pane-exited` for unattached panes, token auth, the bind address, the herdr client, the PTY sidecar's env pass-through (`server/pty/session.test.ts`) and the pure client modules (key bar, draft, notifications, snapshot merge). At the time of writing that's 63 tests across 9 files. ## API diff --git a/server/api.contract.test.ts b/server/api.contract.test.ts index 5b55e8daf3..0a1f350650 100644 --- a/server/api.contract.test.ts +++ b/server/api.contract.test.ts @@ -370,6 +370,47 @@ describe("WebSocket roles and status push", () => { }, 40_000); }); +describe("WebSocket concurrent attach", () => { + /** A pane nobody holds yet: the race is in CREATING the attachment. */ + let raceWorkspaceId: string | null = null; + let racePaneId: string | null = null; + + beforeAll(async () => { + const created = await herdrRpc<{ workspace: { workspace_id: string }; root_pane: { pane_id: string } }>( + "workspace.create", + { label: "herdr-web-ui-test-race", cwd: "/tmp", focus: false }, + ); + raceWorkspaceId = created.workspace.workspace_id; + racePaneId = created.root_pane.pane_id; + }); + + afterAll(async () => { + if (raceWorkspaceId) await herdrRpc("workspace.close", { workspace_id: raceWorkspaceId }).catch(() => undefined); + }); + + it("shares one pty between clients whose attaches to the same pane race", async () => { + const paneId = racePaneId!; + const first = await RecordingSocket.connect(`ws://localhost:${server.port}/ws`); + const second = await RecordingSocket.connect(`ws://localhost:${server.port}/ws`); + try { + // same tick: both attaches are in flight before either resolves the terminal + first.send({ type: "attach", pane_id: paneId, cols: 100, rows: 30 }); + second.send({ type: "attach", pane_id: paneId, cols: 120, rows: 40 }); + await first.waitFor((m) => m.type === "pty-data" && m.pane_id === paneId, "first pty-data", 15_000); + await second.waitFor((m) => m.type === "pty-data" && m.pane_id === paneId, "second pty-data", 15_000); + + // one attachment means one client set: a resize reaches both. Two attachments + // would leave one client on an orphaned record that never hears it. + first.send({ type: "resize", pane_id: paneId, cols: 90, rows: 25 }); + await first.waitFor((m) => m.type === "pane-geometry" && m.cols === 90 && m.rows === 25, "first hears resize", 5000); + await second.waitFor((m) => m.type === "pane-geometry" && m.cols === 90 && m.rows === 25, "second hears resize", 5000); + } finally { + first.close(); + second.close(); + } + }, 40_000); +}); + /** * Bun's WebSocket client sends request headers, but this project compiles with lib.dom, * whose WebSocket type only knows subprotocols (bun-types steps aside via diff --git a/server/index.ts b/server/index.ts index 1177e9bb89..f02f0038d0 100644 --- a/server/index.ts +++ b/server/index.ts @@ -52,6 +52,8 @@ export function createServer( options: { port?: number; hostname?: string; token?: string } = {}, ): { port: number; hostname: string; stop: () => void } { const attachments = new Map(); + /** attachments still resolving their terminal, so concurrent attaches share one pty */ + const pendingAttachments = new Map>(); const clients = new Set(); const hostname = options.hostname ?? process.env["HOST"] ?? "0.0.0.0"; /** Empty token = gate disabled; every route then behaves exactly as it did before auth existed. */ @@ -104,10 +106,21 @@ export function createServer( broadcast(paneId, { type: "pane-geometry", pane_id: paneId, cols, rows }); } - async function ensureAttachment(paneId: string, cols: number, rows: number, forObserver: boolean): Promise { + function ensureAttachment(paneId: string, cols: number, rows: number, forObserver: boolean): Promise { const existing = attachments.get(paneId); - if (existing) return existing; + if (existing) return Promise.resolve(existing); + // a second attach arriving while the first is still resolving the terminal joins + // that creation: two creations would spawn two ptys, and the orphaned one keeps + // streaming into the surviving attachment and kills it when it exits + const pending = pendingAttachments.get(paneId); + if (pending) return pending; + + const created = spawnAttachment(paneId, cols, rows, forObserver).finally(() => pendingAttachments.delete(paneId)); + pendingAttachments.set(paneId, created); + return created; + } + async function spawnAttachment(paneId: string, cols: number, rows: number, forObserver: boolean): Promise { const { terminalId, rect } = await terminalInfoFor(paneId); // an observer-first attachment spawns at the pane's own grid (fallback 80x24 when // the layout has no rect for it): the attach must not seed the shared pty with a