diff --git a/.ai/contexts/ipc-bridge.md b/.ai/contexts/ipc-bridge.md index 4a987bfe..372de541 100644 --- a/.ai/contexts/ipc-bridge.md +++ b/.ai/contexts/ipc-bridge.md @@ -226,6 +226,16 @@ write is bounded by the try/catch on the write"). Callers get a boolean instead of an exception; the first-resize nudge uses it to skip its follow-up when the PTY is already gone. +A kill is once-only. With `useConptyDll`, node-pty's `kill` ends in +`ConptyClosePseudoConsole(hpc)`, and the native handle is only dropped when the +shell's exit thread runs; a second kill before that finds the handle still +registered and closes an already-freed pseudo console, a heap corruption +(0xc0000374) that kills the whole main process with no JS error. So `killPty` +closes a given pty once (a `WeakSet` of killed ptys), and `resizePty` / +`writePty` return false on a killed pty. `test/pty-ops-conpty-kill.test.js` +proves it against the real node-pty in a child process (Windows only): with the +guard removed the child dies with 3221226356. + 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 03b1dd3a..d2c90046 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ What changes for you in each release of Switchboard. How to write an entry: [doc ## Unreleased ### Fixed +- Stopping a terminal twice in quick succession, or resizing it while it is being stopped, no longer closes the Windows pseudo console twice, which could kill the whole app with no error. (#405) - A sandboxed session, or a sandboxed schedule, whose Additional Directories include a `.claude` or `.git` directory, or a path inside one, is now refused instead of binding it read-write over its read-only protection; add the project directory instead. A session started in a `.claude` or `.git` directory is refused too, except below `.claude/worktrees`, and Additional Directories naming your home directory or a parent of it are refused however the path is written. A relative `add-dirs` entry in a schedule is taken from the schedule's directory. (#385) - A session that has exited no longer keeps a busy dot in the sidebar, and the status bar's running count drops as soon as the session ends instead of waiting for the next refresh. (#375) - Quitting, closing the window or reloading while a file in the file panel has unsaved edits now asks first, in any session, kept-aside tabs included: Save writes them (a file that changed on disk is not overwritten), Discard drops them, Cancel stays. If Switchboard does not answer within a few seconds, it closes anyway. (#373) diff --git a/pty-ops.js b/pty-ops.js index d92fcb1a..530bee30 100644 --- a/pty-ops.js +++ b/pty-ops.js @@ -5,6 +5,9 @@ const os = require('os'); let logger = null; +// see .ai/contexts/ipc-bridge.md ("PTY operations race the exit") +const killedPtys = new WeakSet(); + /** Install the sink used to report swallowed PTY errors. `null` disables it. */ function setPtyOpLogger(next) { logger = next && typeof next.debug === 'function' ? next : null; @@ -34,15 +37,23 @@ function withPty(session, label, fn, sessionId) { } } +function isKilled(session) { + return !!(session && session.pty && killedPtys.has(session.pty)); +} + function resizePty(session, cols, rows, sessionId) { + if (isKilled(session)) return false; return withPty(session, 'resize', (pty) => pty.resize(cols, rows), sessionId); } function killPty(session, sessionId) { + if (!session || !session.pty || killedPtys.has(session.pty)) return false; + killedPtys.add(session.pty); return withPty(session, 'kill', (pty) => pty.kill(), sessionId); } function writePty(session, data, sessionId) { + if (isKilled(session)) return false; return withPty(session, 'write', (pty) => pty.write(data), sessionId); } diff --git a/test/pty-ops-conpty-kill.test.js b/test/pty-ops-conpty-kill.test.js new file mode 100644 index 00000000..d91ce0eb --- /dev/null +++ b/test/pty-ops-conpty-kill.test.js @@ -0,0 +1,63 @@ +// test/pty-ops-conpty-kill.test.js — the real node-pty + bundled conpty.dll, in a child process. +// A second ClosePseudoConsole on the same handle corrupts the heap (0xc0000374) and takes the +// process down; the child is the only place that can be observed. see .ai/contexts/ipc-bridge.md +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const path = require('node:path'); +const fs = require('node:fs'); +const os = require('node:os'); +const { spawnSync } = require('node:child_process'); + +const HEAP_CORRUPTION = 3221226356; // 0xc0000374 + +const CHILD = ` +const pty = require(process.env.NODE_PTY_DIR); +const { killPty } = require(process.env.PTY_OPS); +const term = pty.spawn(process.env.ComSpec || 'cmd.exe', [], { cols: 80, rows: 24, useConptyDll: true }); +term.onData(() => {}); +setTimeout(() => { + const session = { pty: term }; + killPty(session, 's1'); + killPty(session, 's1'); + setTimeout(() => process.exit(0), 1500); +}, 1000); +`; + +// The packaged app finds conpty.dll beside the conpty.node it loads (scripts/after-pack.js +// copies prebuilds/win32-/conpty/ there). A source-built node_modules has no such +// folder in build/Release, so the child loads a temp copy that has only the prebuilds. +function stageNodePty() { + const src = path.dirname(require.resolve('node-pty/package.json')); + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'sb-nodepty-')); + const dst = path.join(dir, 'node-pty'); + const prebuild = path.join('prebuilds', `${process.platform}-${process.arch}`); + fs.cpSync(path.join(src, 'lib'), path.join(dst, 'lib'), { recursive: true }); + fs.cpSync(path.join(src, prebuild), path.join(dst, prebuild), { recursive: true }); + fs.copyFileSync(path.join(src, 'package.json'), path.join(dst, 'package.json')); + return { dir, nodePty: dst }; +} + +function runChild(extraEnv) { + const staged = stageNodePty(); + try { + return spawnSync(process.execPath, ['-e', CHILD], { + cwd: path.join(__dirname, '..'), + env: { ...process.env, PTY_OPS: path.join(__dirname, '..', 'pty-ops.js'), NODE_PTY_DIR: staged.nodePty, ...extraEnv }, + timeout: 30000, + encoding: 'utf8', + }); + } finally { + // OpenConsole.exe may still hold the copy open for a moment; the temp dir is disposable. + try { fs.rmSync(staged.dir, { recursive: true, force: true, maxRetries: 10, retryDelay: 300 }); } catch {} + } +} + +test('killing a conpty-dll pty twice back to back does not corrupt the heap', + { skip: process.platform !== 'win32' ? 'ConptyClosePseudoConsole exists only on Windows' : false }, + () => { + const r = runChild({}); + assert.notEqual(r.status, HEAP_CORRUPTION, 'child died of STATUS_HEAP_CORRUPTION'); + assert.equal(r.status, 0, `child stderr: ${r.stderr}`); + }); diff --git a/test/pty-ops.test.js b/test/pty-ops.test.js index 81b88957..fd9fc5bc 100644 --- a/test/pty-ops.test.js +++ b/test/pty-ops.test.js @@ -43,9 +43,9 @@ test('resizePty forwards cols/rows to a live pty and reports success', () => { test('killPty and writePty forward to a live pty and report success', () => { const pty = makeLivePty(); - assert.equal(killPty({ pty }, 's1'), true); assert.equal(writePty({ pty }, 'hi', 's1'), true); - assert.deepEqual(pty.calls, [['kill'], ['write', 'hi']]); + assert.equal(killPty({ pty }, 's1'), true); + assert.deepEqual(pty.calls, [['write', 'hi'], ['kill']]); }); // ── The race the crash came from ───────────────────────────────────────────── @@ -140,3 +140,22 @@ test('a keystroke to a pty that exits mid-write does not escape handleTerminalIn // The composer still saw the keystroke — the guard must not skip the bookkeeping. assert.equal(session.composerState.pending, 5); }); + +// ── A killed pty is closed once (double ClosePseudoConsole corrupts the heap) ── + +test('killPty kills a live pty once and ignores later kills', () => { + const pty = makeLivePty(); + const session = { pty }; + assert.equal(killPty(session, 's1'), true); + assert.equal(killPty(session, 's1'), false); + assert.deepEqual(pty.calls, [['kill']]); +}); + +test('resizePty and writePty do nothing once the pty was killed', () => { + const pty = makeLivePty(); + const session = { pty }; + killPty(session, 's1'); + assert.equal(resizePty(session, 80, 24, 's1'), false); + assert.equal(writePty(session, 'x', 's1'), false); + assert.deepEqual(pty.calls, [['kill']]); +});