From 5f59b2ff8c8aff93ee2dcd9278371591387eef49 Mon Sep 17 00:00:00 2001 From: Max <112043822+Maxaubert@users.noreply.github.com> Date: Sun, 4 Oct 2026 15:28:57 +0200 Subject: [PATCH] fix(core): quitting waits for every shell to be gone, on node-pty 1.2.0-beta.15 (#127) Closing the app with several shells could crash it at quit, two ways, both measured. node-pty 1.1.0's exit threads raced on an unlocked vector and asserted (the owner's "Assertion failed! remove_pty_baton" dialog), fixed upstream in #922, so node-pty is pinned to 1.2.0-beta.15. And an exit callback landing while Node tears down threw and Electron aborted (0xc0000409, about one quit in four with ten shells; WER dump: FreeEnvironment -> ThreadSafeFunction::CallJS -> abort). Every kill now goes through killPty, and will-quit holds the quit on shellsGone (3 s cap) while a shell is dying, reading the agent's exitCode, set by the native callback, rather than the exit event that lags 1-2.7 s for a warm shell. The quitManyShells e2e quits with ten shells three times and needs a clean exit 0; it failed on both node-pty versions without the wait. Core 0.23.1, app 0.28.3. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FHHaWKR4M5QtW7Wecyuk4t --- CLAUDE.md | 8 ++++ core/main/terminal.sessions.test.ts | 37 ++++++++++++++- core/main/terminal.ts | 74 ++++++++++++++++++++++++----- core/package.json | 2 +- package-lock.json | 12 ++--- package.json | 6 +-- src/main/index.ts | 20 ++++++-- tools/e2e/run.mjs | 39 +++++++++++++++ 8 files changed, 171 insertions(+), 27 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 0517cf0..5b99843 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -549,6 +549,14 @@ the owner's own call, #99.) - **Bundled ConPTY.** Shells spawn with `useConptyDll: true`; the inbox conhost fast-fails the whole app (0xc0000409) when a pty is killed mid-read. `node-pty` stays `asarUnpack`ed and `npmRebuild: false` (it ships N-API prebuilds; a rebuild dies in node-gyp). +- **THE QUIT WAITS FOR EVERY SHELL TO BE GONE** (#127, 2026-10-04; owner's screenshot of node-pty's + "Assertion failed! remove_pty_baton" dialog). Two crashes at quit, both MEASURED: node-pty 1.1.0's + exit threads race on an unlocked vector (fixed upstream in #922, so `node-pty` is pinned to + `1.2.0-beta.15`, the owner's pick); and an exit callback that lands while Node tears down throws and + Electron aborts, 0xc0000409, about one quit in four with ten shells (WER dump: `FreeEnvironment` -> + `ThreadSafeFunction::CallJS` -> abort). So every kill goes through `killPty` and `will-quit` holds + the quit on `shellsGone` (3 s cap). It waits for the agent's `exitCode`, set by the native callback, + not the exit EVENT, which lags 1-2.7 s for a pwsh killed mid-start (a warm shell). `quitManyShells`. - **`titleBarStyle: 'hidden'`, never `frame: false`**: DWM will not composite acrylic behind a frameless window. - **Material before colour** (`material.ts`, measured on Electron 43): `setBackgroundMaterial('none')` diff --git a/core/main/terminal.sessions.test.ts b/core/main/terminal.sessions.test.ts index 573de4f..1b3bde0 100644 --- a/core/main/terminal.sessions.test.ts +++ b/core/main/terminal.sessions.test.ts @@ -15,6 +15,8 @@ interface FakePty { } const made: FakePty[] = [] let gate: Promise = Promise.resolve() +// A real pty dies on its own thread, some time after kill() returns. +let lateExit = false vi.mock('node-pty', () => ({ spawn: () => { @@ -25,7 +27,7 @@ vi.mock('node-pty', () => ({ exit: () => exits.forEach((cb) => cb()), kill: () => { p.killed = true - p.exit() + if (!lateExit) p.exit() }, onData: () => ({ dispose: () => {} }), onExit: (cb) => { @@ -49,15 +51,46 @@ vi.mock('./shells', () => ({ shellById: (_id: unknown, list: Array<{ id: string }>) => list[0] })) -const { killAll, killTerm, livePids, prewarmShell, spawnTerm } = await import('./terminal') +const { killAll, killTerm, livePids, prewarmShell, shellsGone, spawnTerm } = await import('./terminal') const send = (): void => {} beforeEach(() => { + lateExit = false killAll() made.length = 0 gate = Promise.resolve() }) +describe('the quit waits for killed shells to be gone (#127)', () => { + const settled = async (p: Promise): Promise => + Promise.race([p.then(() => true), new Promise((r) => setTimeout(() => r(false), 20))]) + + it('resolves at once when nothing was killed', async () => { + expect(await settled(shellsGone(5000))).toBe(true) + }) + + it('waits until every killed shell has exited', async () => { + expect(await spawnTerm('q1', 'C:\\x', 'pwsh', send)).toBe(true) + expect(await spawnTerm('q2', 'C:\\x', 'pwsh', send)).toBe(true) + lateExit = true + killAll() + const gone = shellsGone(5000) + expect(await settled(gone)).toBe(false) + made[0].exit() + expect(await settled(gone)).toBe(false) + made[1].exit() + expect(await settled(gone)).toBe(true) + }) + + it('gives up after the timeout when a shell never answers', async () => { + expect(await spawnTerm('q3', 'C:\\x', 'pwsh', send)).toBe(true) + lateExit = true + killAll() + expect(await settled(shellsGone(5))).toBe(true) + made[0].exit() // let it go, so the next test starts clean + }) +}) + describe('a tab closed while its shell is starting (#12)', () => { it('starts no shell, or kills the one that started, and registers nothing', async () => { let open!: () => void diff --git a/core/main/terminal.ts b/core/main/terminal.ts index 9a44b0e..5376d7d 100644 --- a/core/main/terminal.ts +++ b/core/main/terminal.ts @@ -175,6 +175,64 @@ interface WarmShell { const warm = new Map() const rootKey = (root: string): string => root.toLowerCase() +/** + * Every shell we end, until node-pty has said it is gone (#127). Its exit + * watcher is a native thread that calls back into JavaScript; when the app + * quit first, the call landed while Node was tearing its environment down, + * threw, and Electron aborted (0xc0000409, no dialog). MEASURED in a WER dump, + * 2026-10-04: node::FreeEnvironment -> CleanupHandles -> ThreadSafeFunction:: + * CallJS -> Napi::Error -> abort, about one quit in four with ten shells open. + * So a kill is remembered here and the quit waits (`shellsGone`). + */ +const dying = new Set>() +function killPty(p: IPty): void { + const gone = new Promise((resolve) => { + let poll: ReturnType | undefined + const done = (): void => { + if (poll) clearInterval(poll) + resolve() + } + try { + p.onExit(done) + } catch { + done() + return + } + // The exit EVENT waits for the output pipe to close: 1.0 to 2.7 s for a + // pwsh killed while it starts, which is what a warm shell is at quit + // (MEASURED). What the quit must outlive is only the native callback, and + // that sets the Windows agent's exitCode the moment it runs. Read it where + // node-pty has it; the event stands for everything else. + const agent = (p as unknown as { _agent?: { exitCode?: number } })._agent + if (agent && 'exitCode' in agent) { + poll = setInterval(() => { + if (agent.exitCode !== undefined) done() + }, 20) + } + }) + dying.add(gone) + void gone.then(() => dying.delete(gone)) + try { + p.kill() + } catch { + /* already gone */ + } +} + +/** How many shells we killed that have not exited yet. */ +export function shellsDying(): number { + return dying.size +} + +/** Resolves once every shell we killed has exited, or after `timeoutMs`. */ +export function shellsGone(timeoutMs: number): Promise { + if (!dying.size) return Promise.resolve() + return Promise.race([ + Promise.all([...dying]).then(() => undefined), + new Promise((resolve) => setTimeout(resolve, timeoutMs)) + ]) +} + export async function prewarmShell(root: string, shellId: string | undefined): Promise { const key = rootKey(root) if (warm.has(key)) return @@ -187,10 +245,10 @@ export async function prewarmShell(root: string, shellId: string | undefined): P warm.delete(k) try { w.sub.dispose() - w.pty.kill() } catch { /* already gone */ } + if (!w.exited) killPty(w.pty) } try { const pty = await import('node-pty') @@ -229,10 +287,10 @@ function killWarm(root?: string): void { warm.delete(k) try { w.sub.dispose() - w.pty.kill() } catch { /* already gone */ } + if (!w.exited) killPty(w.pty) } } export { killWarm } @@ -351,11 +409,7 @@ async function spawnPending( }) // Closed while node-pty loaded: the tab is gone, so is this shell. if (killedWhilePending.has(id)) { - try { - p.kill() - } catch { - /* already gone */ - } + killPty(p) return false } const batcher = new OutputBatcher((data) => send('term:data', id, data), 8) @@ -420,11 +474,7 @@ export function killTerm(id: string): void { } // No flush: this death is ours (tab close, quit), nobody is listening, and // at quit the webContents a flush would send into may already be gone. - try { - s.pty.kill() - } catch { - /* already gone */ - } + killPty(s.pty) } /** The live sessions' shell pids, for the agent poll. */ diff --git a/core/package.json b/core/package.json index a7db379..321805e 100644 --- a/core/package.json +++ b/core/package.json @@ -1,6 +1,6 @@ { "name": "prism-term-core", - "version": "0.23.0", + "version": "0.23.1", "description": "What Prism Terminal and Prism share: the terminal (pty, shells, agent detection and indicator, themes, links, the panel, dictation) and the update chip with its window. TypeScript source, compiled by the host.", "license": "MIT", "private": true, diff --git a/package-lock.json b/package-lock.json index d39ab20..349adc4 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "prism-terminal", - "version": "0.28.2", + "version": "0.28.3", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "prism-terminal", - "version": "0.28.2", + "version": "0.28.3", "license": "MIT", "dependencies": { "@xterm/addon-fit": "^0.11.0", @@ -14,7 +14,7 @@ "@xterm/addon-unicode11": "^0.9.0", "@xterm/addon-web-links": "^0.12.0", "@xterm/xterm": "^6.0.0", - "node-pty": "^1.1.0", + "node-pty": "1.2.0-beta.15", "react": "^19.2.7", "react-dom": "^19.2.7" }, @@ -6107,9 +6107,9 @@ "license": "MIT" }, "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": { diff --git a/package.json b/package.json index a09d008..35b810d 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "prism-terminal", "productName": "Prism Terminal", - "version": "0.28.2", + "version": "0.28.3", "description": "A tabbed Windows terminal for AI CLIs.", "main": "./out/main/index.js", "author": "Max", @@ -28,7 +28,7 @@ "@xterm/addon-unicode11": "^0.9.0", "@xterm/addon-web-links": "^0.12.0", "@xterm/xterm": "^6.0.0", - "node-pty": "^1.1.0", + "node-pty": "1.2.0-beta.15", "react": "^19.2.7", "react-dom": "^19.2.7" }, @@ -56,6 +56,6 @@ "vitest": "^3.2.7" }, "allowScripts": { - "node-pty@1.1.0": true + "node-pty@1.2.0-beta.15": true } } diff --git a/src/main/index.ts b/src/main/index.ts index 9a537b6..001c712 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -16,7 +16,7 @@ import { createWindowEdge } from './windowEdge' import { DEFAULT_WINDOW_EDGES, validWindowEdges, type WindowEdges } from '@shared/windowEdges' import { detectShells } from '@core/main/shells' import { createTabsStore } from './tabsStore' -import { killAll } from '@core/main/terminal' +import { killAll, shellsDying, shellsGone } from '@core/main/terminal' import { installUpdate, updateCalls, watchForUpdates } from './update' import { previewUpdate, runPreviewInstall, wantsPreview } from '@core/main/updatePreview' import { createVerbSwitch } from './verbSwitch' @@ -731,12 +731,26 @@ if (!app.requestSingleInstanceLock()) { quitting = true }) app.on('window-all-closed', () => app.quit()) - // Every shell dies with the app; a pty with no window is an orphan. - app.on('will-quit', () => { + // Every shell dies with the app; a pty with no window is an orphan. And the + // app waits for them to be GONE before it ends (#127): a shell still dying + // when Node tears down calls back into it and Electron aborts. Three seconds + // at most, so a shell that never answers cannot hold the quit. + let shellsSettled = false + app.on('will-quit', (e) => { stopDwmHelper() stopDictation() killAll() tabs.flush() + // Nothing dying: no hold. A hold that ends at once is worse than none: + // its app.quit() lands inside the quit it cancelled and Electron drops it, + // so the app stayed up (MEASURED, the opacityAlpha quit). Hence also the + // fresh tick before quitting again. + if (shellsSettled || shellsDying() === 0) return + e.preventDefault() + void shellsGone(3000).then(() => { + shellsSettled = true + setTimeout(() => app.quit(), 0) + }) }) app.whenReady().then(() => { diff --git a/tools/e2e/run.mjs b/tools/e2e/run.mjs index e501fd7..474dee9 100644 --- a/tools/e2e/run.mjs +++ b/tools/e2e/run.mjs @@ -1380,6 +1380,45 @@ const scenarios = { /** Closing the last tab lands on the start screen; the X is what quits, and * what was open when it quit is what comes back. */ + // QUITTING WITH MANY SHELLS IS CLEAN (#127; owner, 2026-10-04, a screenshot + // of "Assertion failed! conpty.node ... remove_pty_baton(baton->id)" as the + // stable copy closed for an update). node-pty 1.1.0's exit threads erased + // from one vector with no lock, so shells dying together raced, and its + // prebuild asserts: a modal dialog that holds the process open. Ten live + // shells, quit, three times: the process must exit by itself, quickly, 0. + async quitManyShells(ok) { + for (let round = 1; round <= 3; round += 1) { + // A profile per round: the same one would restore the last round's tabs. + const w = world() + const { app, page } = await launch(w, { args: [w.alpha] }) + await until(async () => (await tabLabels(page)).length === 1) + for (let i = 1; i < 10; i += 1) { + await page.keyboard.press('Control+t') + await until(async () => (await tabLabels(page)).length === i + 1, 8000, 50) + await until(async () => /PS |>/.test(await termText(page)), 8000, 50) + } + ok((await tabLabels(page)).length === 10, `round ${round}: ten shells open`) + const proc = app.process() + // What the process says as it dies: a native abort names itself here. + let said = '' + proc.stderr?.on('data', (d) => (said = (said + d).slice(-3000))) + const exited = new Promise((r) => proc.once('exit', (code, signal) => r({ code, signal }))) + const t0 = Date.now() + app.close().catch(() => {}) + const end = await Promise.race([exited, sleep(10000).then(() => null)]) + if (!end) { + try { + proc.kill() + } catch { + /* already gone */ + } + } + ok(end !== null, `round ${round}: the app quit by itself (${end ? Date.now() - t0 : '>10000'} ms)`) + ok(end?.code === 0, `round ${round}: with exit code 0 (${JSON.stringify(end)})`) + if (end?.code !== 0 && said.trim()) console.log(` stderr ${said.trim().split('\n').slice(-12).join('\n ')}`) + } + }, + async lastTab(ok) { const w = world() let { app, page } = await launch(w, { args: [w.alpha, w.beta] })