diff --git a/.ai/contexts/ipc-bridge.md b/.ai/contexts/ipc-bridge.md index c2b20b41..ff387f1d 100644 --- a/.ai/contexts/ipc-bridge.md +++ b/.ai/contexts/ipc-bridge.md @@ -247,6 +247,97 @@ closes a given pty once (a `WeakSet` of killed ptys), and `resizePty` / proves it against the real node-pty in a child process (Windows only): with the guard removed the child dies with 3221226356. +node-pty 1.2.0-beta.15 (#409) adds a mutex around the handle table +(microsoft/node-pty#922). In 1.1.0 the shell's exit thread erases the pty's +entry, and frees it, without a lock, while the main thread may be inside +`PtyKill` or `PtyResize` holding a pointer to that entry, or walking the table +while the erase shifts it. The beta takes `g_ptyHandlesMutex` on both sides. +It does not make a second kill safe: `PtyKill` still leaves the handle +registered with its `hpc`, and `WindowsTerminal.kill` has no once-only guard. +Measured 2026-10-03 with the child above against the beta: two raw +`term.kill()` still exit 3221226356, two `killPty` exit 0. The guard stays. + +The race itself was measured with a scratch stress script, not kept in the +suite (each run takes about a minute, and a pass proves nothing). One child +process per run, so a crash is an exit status: 20 iterations, each starting 4 +`cmd.exe /c exit` ptys plus one long-lived pty, then 300 ms of resize calls +straight into the binding while the short shells exit, and in kill mode one +agent-level kill per short pty at a random moment in the first 150 ms. +Results, 2026-10-03, `useConptyDll`, plain node 24: + +| Mode | 1.1.0 | 1.2.0-beta.15 | +| --- | --- | --- | +| kill | 5 of 8 runs died, 0xc0000005 | 0 of 8 died | +| resize | 0 of 3 died | 0 of 3 died | + +One beta kill run completed its 20 iterations and then never exited (killed +after 600 s); not explained. The crash is not deterministic: it needs the exit +thread's erase to land inside the few instructions between the main thread's +lookup and its last use of the entry, and reading or writing freed memory only +crashes when the allocator has already reused or unmapped it. A kill holds +the pointer across `ClosePseudoConsole`, which can wait for OpenConsole, and then +writes to the entry; a resize holds it for one `ResizePseudoConsole` call. + +### Cursor-position queries + +The beta's bundled OpenConsole asks the terminal where its cursor is: a plain +DSR (`CSI 6n`). 1.1.0 sent none in the same runs. Measured 2026-10-03 on an +isolated pty under Electron 41 with `useConptyDll` (`cmd.exe`, then a raw-mode +node reader as the shell): + +- With no resize, no query in 10 s. +- A resize within about the first 3 s of the pty brings one query about 3.3 to + 4 s after spawn. Unanswered, it is repeated about every 520 ms and dropped + after 3 (`cmd.exe`) or 4 (node) queries. Answered, it is sent once. A resize + at 4 s or later brought none, and a second resize no second series. +- OpenConsole keeps waiting after the last repeat: the first CPR that arrives + later, even seconds later, is consumed. Every other CPR it receives, a reply + it is not waiting for, is passed to the shell's application as typed input, + verbatim (`ESC [ 24 ; 1 R` read by the child's stdin). 1.1.0 passes such a + reply through the same way. + +A visible session answers at once: xterm.js replies with a plain CPR +(`CSI row;col R`), which `composer-state.js` does not treat as a report (see +`.ai/contexts/trigger-watcher.md`, CPR), so the answer pushes the quiet clock +once. + +A hidden single-view session cannot answer: its output goes to the hidden +accumulator, not to xterm. A session with no renderer attached cannot either. +Both replay paths drop the query instead of keeping it: + +- `appendToHiddenAccumulator` / `replayHiddenBuffer` (`public/terminal-manager.js`) + remove `CSI 6n` from what they keep, including a query split across chunks + and one left in the live write buffer when the session was hidden. +- `appendToOutputBuffer` (`output-buffer.js`), main's reattach buffer, removes it + on the way in, a query split across several chunks included (the last + three code units of the buffer are checked, across entries). That buffer also + holds the queries a visible renderer already answered. + +The filter is not specific to Windows or to OpenConsole: it runs on every +platform and removes any `CSI 6n` in a session's output, including one an +application asked itself (a shell's line editor measuring the cursor, for +example). Such an application gets its answer while its session is visible; a +query it sent while the session was hidden or detached goes unanswered, where +it used to be answered late, at reveal or reattach, with a stale position. + +The removal is a single pass. Removing a query can join the bytes around it +into a new one (`ESC [ ESC [6n 6n` becomes `ESC [6n`); that one is kept, so +such input is replayed with one query. The earlier version repeated the pass +until no query was left: it removed bytes that are not a query (xterm.js shows +the trailing `6n` of that input as text) and was quadratic, and on a crafted +64 KB chunk the main-side version, which recursed, overflowed the stack. + +Kept, each query would be answered by xterm.js at reveal or reattach: one reply +consumed by an OpenConsole still waiting, every other one typed into Claude or +the shell. Answering once at reveal instead was rejected: the reply would give +the cursor of a replay that happened after the output OpenConsole had already +moved on from, and nothing tells the renderer whether OpenConsole is still +waiting or a reply was already sent. Left unanswered, OpenConsole keeps its own +idea of the cursor, as it did on 1.1.0 where it never asked; the next query it +sends while the session is visible is answered live. Other terminal queries +in a replay (`CSI c`, `CSI > q`, OSC colour queries) are not filtered; their +replay is older than this change and not addressed here. + Swallowed errors are not silent: `setPtyOpLogger(log)` in `main.js` routes them to `log.debug` as `[pty] skipped session= reason=`. Debug level is deliberate — the file transport is at `info` in packaged builds, so a resize diff --git a/CHANGELOG.md b/CHANGELOG.md index f0e85c99..2e546ab7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ What changes for you in each release of Switchboard. How to write an entry: [doc - A trigger that gave up waiting for a session now says, in its result file's `reason`, when the session was blocked on a dialog such as a permission prompt or a question: for a single trigger, a chain's first wait, and a chain step whose turn never finished. Without a dialog the result is as before. (#379) ### Fixed - The Changes panel no longer shows `fatal: .git/index: index file open failed: Permission denied` now and then on Windows: a session's local changes are read one git call at a time instead of three at once. (#421) +- On Windows, closing or resizing a terminal at the moment its shell exits should no longer crash the whole app: a known cause is fixed in the terminal library. (#409) ## v0.0.87 — 2026-10-02 diff --git a/output-buffer.js b/output-buffer.js index 2819b06e..1ffea455 100644 --- a/output-buffer.js +++ b/output-buffer.js @@ -39,7 +39,9 @@ // The reattach loop in main.js iterates `for (const chunk of outputBuffer)` // and forwards each chunk verbatim to xterm.js. Because we only ever // remove bytes from the front of the conceptual stream (never insert, split, -// or reorder), xterm.js sees a consistent terminal state. The line-boundary +// or reorder), xterm.js sees a consistent terminal state. (Cursor-position +// queries are the one exception: dropped on the way in, they draw nothing.) +// The line-boundary // trim means the replay starts at a newline (not mid-escape), so cursor/color // state from before the trim window is simply absent — same trade-off the // original whole-chunk front-trim had, just at a byte-accurate boundary. @@ -63,6 +65,61 @@ const COALESCE_THRESHOLD = 64; const MAX_BUFFER_SIZE = 256 * 1024; +// Cursor-position query (DSR 6). Never replayed: see .ai/contexts/ipc-bridge.md, +// "Cursor-position queries". +const CURSOR_POSITION_QUERY = '\x1b[6n'; + +// One pass: a query re-formed by the removal is left alone, see the doc above. +function removeCursorPositionQueries(data) { + return data.includes(CURSOR_POSITION_QUERY) ? data.split(CURSOR_POSITION_QUERY).join('') : data; +} + +// Length of the query prefix `prev` ends with that `next` completes, 0 if none. +function splitQueryPrefixLength(prev, next) { + for (let k = CURSOR_POSITION_QUERY.length - 1; k > 0; k--) { + if (prev.endsWith(CURSOR_POSITION_QUERY.slice(0, k)) && next.startsWith(CURSOR_POSITION_QUERY.slice(k))) return k; + } + return 0; +} + +// The last n code units of the buffer, across entries. +function bufferTail(state, n) { + const parts = []; + let size = 0; + for (let i = state.outputBuffer.length - 1; i >= 0 && size < n; i--) { + const part = state.outputBuffer[i].slice(-(n - size)); + parts.unshift(part); + size += part.length; + } + return parts.join(''); +} + +function dropBufferEnd(state, count) { + let left = count; + while (left > 0) { + const last = state.outputBuffer.length - 1; + const entry = state.outputBuffer[last]; + if (entry.length <= left) { + state.outputBuffer.pop(); + state.outputBufferSize -= entry.length; + left -= entry.length; + } else { + state.outputBuffer[last] = entry.slice(0, -left); + state.outputBufferSize -= left; + left = 0; + } + } +} + +// Only a chunk starting with '[', '6' or 'n' can complete a split query. +function dropSplitQuery(state, chunk) { + if (!'[6n'.includes(chunk[0])) return chunk; + const k = splitQueryPrefixLength(bufferTail(state, CURSOR_POSITION_QUERY.length - 1), chunk); + if (k === 0) return chunk; + dropBufferEnd(state, k); + return chunk.slice(CURSOR_POSITION_QUERY.length - k); +} + /** * Push `data` into `state.outputBuffer` and maintain the two invariants: * 1. `state.outputBufferSize` ≤ `max` (the 256 KB ceiling by default) @@ -74,10 +131,12 @@ const MAX_BUFFER_SIZE = 256 * 1024; * `state`. Designed to be unit-testable in isolation from main.js / Electron. * * @param {{ outputBuffer: string[], outputBufferSize: number }} state - * @param {string} data + * @param {string} chunk * @param {number} max Maximum retained UTF-16 code units (normally MAX_BUFFER_SIZE) */ -function appendToOutputBuffer(state, data, max) { +function appendToOutputBuffer(state, chunk, max) { + if (!chunk) return; + const data = removeCursorPositionQueries(dropSplitQuery(state, chunk)); if (!data) return; state.outputBuffer.push(data); diff --git a/package-lock.json b/package-lock.json index 7dbeb551..7959064c 100644 --- a/package-lock.json +++ b/package-lock.json @@ -21,7 +21,7 @@ "electron-updater": "^6.8.9", "marked": "^17.0.4", "morphdom": "^2.7.8", - "node-pty": "^1.0.0", + "node-pty": "1.2.0-beta.15", "ws": "^8.21.1" }, "devDependencies": { @@ -47,6 +47,9 @@ "husky": "^9.1.7", "jsdom": "^29.1.1", "png2icons": "^2.0.1" + }, + "engines": { + "node": ">=20 <23" } }, "node_modules/@asamuzakjp/css-color": { @@ -6734,11 +6737,10 @@ } }, "node_modules/node-pty": { - "version": "1.1.0", - "resolved": "https://registry.npmjs.org/node-pty/-/node-pty-1.1.0.tgz", - "integrity": "sha512-20JqtutY6JPXTUnL0ij1uad7Qe1baT46lyolh2sSENDd4sTzKZ4nmAFkeAARDKwmlLjPx6XKRlwRUxwjOy+lUg==", + "version": "1.2.0-beta.15", + "resolved": "https://registry.npmjs.org/node-pty/-/node-pty-1.2.0-beta.15.tgz", + "integrity": "sha512-vORSzHXi4Ofl7HemVWpuudLqCPdaQb4LfpRCUpE5HPxhp4JYscl8zZwxh11p26v2wvW24WMwnMfLjhRLixrfxA==", "hasInstallScript": true, - "license": "MIT", "dependencies": { "node-addon-api": "^7.1.0" } diff --git a/package.json b/package.json index 2ffa3d7e..0c87467c 100644 --- a/package.json +++ b/package.json @@ -40,7 +40,7 @@ "electron-updater": "^6.8.9", "marked": "^17.0.4", "morphdom": "^2.7.8", - "node-pty": "^1.0.0", + "node-pty": "1.2.0-beta.15", "ws": "^8.21.1" }, "devDependencies": { diff --git a/public/terminal-manager.js b/public/terminal-manager.js index 5997afd7..d583d87d 100644 --- a/public/terminal-manager.js +++ b/public/terminal-manager.js @@ -710,13 +710,34 @@ function drainLiveBufferIntoHiddenAccumulator(sessionId) { acc.raw = buf.chunks.join('') + acc.raw; // leftover is older — goes first } +// Cursor-position query (DSR 6): never kept for a replay — see +// .ai/contexts/ipc-bridge.md, "Cursor-position queries". +const CURSOR_POSITION_QUERY = '\x1b[6n'; + +// One pass: a query re-formed by the removal is left alone, see the doc above. +function removeCursorPositionQueries(str) { + return str.includes(CURSOR_POSITION_QUERY) ? str.split(CURSOR_POSITION_QUERY).join('') : str; +} + +// Length of the query prefix `prev` ends with that `next` completes, 0 if +// none. Only a chunk starting with '[', '6' or 'n' can complete one. +function splitQueryPrefixLength(prev, next) { + if (!'[6n'.includes(next[0])) return 0; + for (let k = CURSOR_POSITION_QUERY.length - 1; k > 0; k--) { + if (prev.endsWith(CURSOR_POSITION_QUERY.slice(0, k)) && next.startsWith(CURSOR_POSITION_QUERY.slice(k))) return k; + } + return 0; +} + function appendToHiddenAccumulator(sessionId, data) { let acc = hiddenAccumulators.get(sessionId); if (!acc) { acc = { raw: '', reset: false }; hiddenAccumulators.set(sessionId, acc); } - acc.raw += data; + const k = splitQueryPrefixLength(acc.raw, data); + const head = k ? acc.raw.slice(0, -k) : acc.raw; + acc.raw = head + removeCursorPositionQueries(k ? data.slice(CURSOR_POSITION_QUERY.length - k) : data); if (acc.raw.length > HIDDEN_BUFFER_MAX_LEN) { const trimmed = trimHiddenBuffer(acc.raw, HIDDEN_BUFFER_MAX_LEN); acc.raw = trimmed.data; @@ -778,9 +799,11 @@ function replayHiddenBuffer(sessionId) { if (!acc || !acc.raw) return; const entry = openSessions.get(sessionId); if (!entry) return; // destroySession may have removed it first + const raw = removeCursorPositionQueries(acc.raw); + if (!raw && !acc.reset) return; if (acc.reset) entry.terminal.reset(); - if (window.ATRACE) noteRenderWrite(sessionId, 1, acc.raw.length); - entry.terminal.write(acc.raw); + if (window.ATRACE) noteRenderWrite(sessionId, 1, raw.length); + entry.terminal.write(raw); } // Entry point for PTY data (wired to window.api.onTerminalData in app.js). diff --git a/test/output-buffer.test.js b/test/output-buffer.test.js index 695cc523..1f301731 100644 --- a/test/output-buffer.test.js +++ b/test/output-buffer.test.js @@ -237,3 +237,67 @@ test('appendToOutputBuffer: small chunk on a MAX-1 spine keeps full history', () // The newest bytes ('c') must survive at the tail. assert.ok(joined(state).endsWith('c'.repeat(chunkSize)), 'newest chunk must remain at the tail'); }); + +// --------------------------------------------------------------------------- +// Cursor-position queries (CSI 6n) are not replayed on reattach: the renderer +// would answer each one, and ConPTY hands a reply it is not waiting for to the +// application as typed input. See .ai/contexts/ipc-bridge.md. +// --------------------------------------------------------------------------- + +test('GIVEN chunks carrying cursor-position queries WHEN appended THEN the replay buffer keeps the text without the queries', () => { + const state = makeState(); + appendToOutputBuffer(state, 'A\x1b[6nB', MAX); + appendToOutputBuffer(state, '\x1b[6n\x1b[6nC', MAX); + assert.equal(joined(state), 'ABC'); + assert.equal(state.outputBufferSize, 3); +}); + +test('GIVEN a cursor-position query split across two chunks WHEN both are appended THEN neither half stays in the replay buffer', () => { + const state = makeState(); + appendToOutputBuffer(state, 'A\x1b[', MAX); + appendToOutputBuffer(state, '6nB', MAX); + appendToOutputBuffer(state, 'C\x1b', MAX); + appendToOutputBuffer(state, '[6nD', MAX); + appendToOutputBuffer(state, 'E\x1b[6', MAX); + appendToOutputBuffer(state, 'nF', MAX); + assert.equal(joined(state), 'ABCDEF'); + assert.equal(state.outputBufferSize, 6); +}); + +test('GIVEN other sequences that look like the query WHEN appended THEN they are kept byte for byte', () => { + const state = makeState(); + appendToOutputBuffer(state, '\x1b[16n\x1b[?6n\x1b[6;1H\x1b[', MAX); + appendToOutputBuffer(state, '5n', MAX); + assert.equal(joined(state), '\x1b[16n\x1b[?6n\x1b[6;1H\x1b[5n'); +}); + +test('GIVEN a chunk that is only a query WHEN appended THEN no empty entry is pushed', () => { + const state = makeState(); + appendToOutputBuffer(state, 'A', MAX); + appendToOutputBuffer(state, '\x1b[6n', MAX); + assert.deepEqual(state.outputBuffer, ['A']); +}); + +test('GIVEN a chunk that is only the first half of a query WHEN the second half arrives THEN no empty entry is left behind', () => { + const state = makeState(); + appendToOutputBuffer(state, 'A', MAX); + appendToOutputBuffer(state, '\x1b[', MAX); + appendToOutputBuffer(state, '6n', MAX); + assert.deepEqual(state.outputBuffer, ['A']); + assert.equal(state.outputBufferSize, 1); +}); + +test('GIVEN a query inside an unfinished sequence WHEN appended THEN the query is removed once and the bytes around it are kept', () => { + const state = makeState(); + appendToOutputBuffer(state, 'A\x1b[\x1b[6n6nB', MAX); + assert.equal(joined(state), 'A\x1b[6nB'); +}); + +test('GIVEN a cursor-position query split across three chunks WHEN all are appended THEN none of it stays in the replay buffer', () => { + const state = makeState(); + appendToOutputBuffer(state, 'A\x1b', MAX); + appendToOutputBuffer(state, '[', MAX); + appendToOutputBuffer(state, '6nB', MAX); + assert.equal(joined(state), 'AB'); + assert.equal(state.outputBufferSize, 2); +}); diff --git a/test/terminal-hidden-suspend.test.js b/test/terminal-hidden-suspend.test.js index 07553ef4..a5db16da 100644 --- a/test/terminal-hidden-suspend.test.js +++ b/test/terminal-hidden-suspend.test.js @@ -716,3 +716,88 @@ test('grid-open sync guard: same deferral on a plain showSession reveal of a ses destroy(); } }); + +// Cursor-position queries (CSI 6n) while hidden. node-pty 1.2's ConPTY asks +// one after an early resize and repeats it until answered; a hidden session +// cannot answer, and xterm answering a replayed query on reveal sends a reply +// nobody is waiting for, which ConPTY hands to the application as typed input. +// See .ai/contexts/ipc-bridge.md, "Cursor-position queries". + +test('GIVEN a hidden session WHEN ConPTY cursor-position queries arrive, even split across chunks, THEN the accumulator does not keep them', () => { + const { window, inCtx, destroy } = setupTerminalDom(); + try { + window.createTerminalEntry({ sessionId: 'active' }); + window.createTerminalEntry({ sessionId: 'hidden' }); + window.activeSessionId = 'active'; + window.gridViewActive = false; + + window.handleTerminalData('hidden', 'A\x1b[6n'); + window.handleTerminalData('hidden', 'B\x1b[6n\x1b[6n'); + window.handleTerminalData('hidden', 'C\x1b'); + window.handleTerminalData('hidden', '[6nD'); + window.handleTerminalData('hidden', 'E\x1b'); + window.handleTerminalData('hidden', '['); + window.handleTerminalData('hidden', '6nF'); + window.handleTerminalData('hidden', 'G\x1b[6'); + window.handleTerminalData('hidden', 'nH'); + + assert.strictEqual(inCtx(`hiddenAccumulators.get('hidden').raw`), 'ABCDEFGH'); + } finally { + destroy(); + } +}); + +test('GIVEN a query left in the live buffer when the session was hidden WHEN it is revealed THEN the replay carries no cursor-position query', () => { + const { window, spies, destroy } = setupTerminalDom(); + try { + window.createTerminalEntry({ sessionId: 'a' }); + window.createTerminalEntry({ sessionId: 'b' }); + window.activeSessionId = 'a'; + window.gridViewActive = false; + + window.handleTerminalData('a', 'OLD\x1b[6n'); + window.showSession('b'); + window.activeSessionId = 'b'; + window.handleTerminalData('a', 'NEW'); + + window.showSession('a'); + + assert.strictEqual(spies.write, 1); + assert.strictEqual(spies.writes[0], 'OLDNEW'); + } finally { + destroy(); + } +}); + +test('GIVEN a hidden session WHEN it receives other escape sequences THEN they are replayed untouched', () => { + const { window, spies, destroy } = setupTerminalDom(); + try { + window.createTerminalEntry({ sessionId: 'active' }); + window.createTerminalEntry({ sessionId: 'hidden' }); + window.activeSessionId = 'active'; + window.gridViewActive = false; + + window.handleTerminalData('hidden', '\x1b[16n\x1b[?6n\x1b[6;1H\x1b[5n'); + window.showSession('hidden'); + + assert.strictEqual(spies.writes[0], '\x1b[16n\x1b[?6n\x1b[6;1H\x1b[5n'); + } finally { + destroy(); + } +}); + +test('GIVEN a hidden session WHEN a query arrives inside an unfinished sequence THEN the query is removed once and the bytes around it are kept', () => { + const { window, inCtx, destroy } = setupTerminalDom(); + try { + window.createTerminalEntry({ sessionId: 'active' }); + window.createTerminalEntry({ sessionId: 'hidden' }); + window.activeSessionId = 'active'; + window.gridViewActive = false; + + window.handleTerminalData('hidden', 'A\x1b[\x1b[6n6nB'); + + assert.strictEqual(inCtx(`hiddenAccumulators.get('hidden').raw`), 'A\x1b[6nB'); + } finally { + destroy(); + } +});