From 3cd23a13a1404878b0c8f87e7e13a51623e65365 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 5 Jul 2026 17:21:42 -0700 Subject: [PATCH] fix(pty): keep runtime dirs a surviving daemon still uses (#7463) The node-pty native runtime is relocated to a per-app-version dir under userData so live terminals survive NSIS updates. On every launch, removeStaleRuntimeVersions() deleted every *other* version's dir, gated on a "renameSync succeeds => unused => safe to delete" heuristic. That heuristic is false on Windows: running .exe images and mapped .dll files are opened with FILE_SHARE_DELETE, so both the directory rename and the recursive delete succeed while a daemon adopted across the update is still using them. Existing PTYs keep streaming via deferred-delete handles, but the next spawn re-loads conpty.dll from the now-unlinked version dir and fails with ERROR_PATH_NOT_FOUND (code 3) -- the "Cannot find conpty.dll ... error code: 3" new-tab failure. Gate cleanup on real daemon liveness instead: collectInUseRuntimeVersions() reads the daemon-v.pid files under userData/daemon and, for each live daemon, protects the appVersion (runtime dir) it recorded. Deletion now skips the current version and any in-use version, dropping the rename dance. Every failure mode leaks a dir rather than deleting one in use. Rewrites the misleading cleanup test (which modeled no live daemon) and adds coverage for surviving/dead daemons, appVersion:null pin files, multiple protocol versions, and malformed/non-pid files. Co-authored-by: Neil --- .../pty/node-pty-runtime-relocation.test.ts | 174 +++++++++++++++++- src/main/pty/node-pty-runtime-relocation.ts | 91 +++++++-- 2 files changed, 241 insertions(+), 24 deletions(-) diff --git a/src/main/pty/node-pty-runtime-relocation.test.ts b/src/main/pty/node-pty-runtime-relocation.test.ts index 1dfbe0338..5b4ce4886 100644 --- a/src/main/pty/node-pty-runtime-relocation.test.ts +++ b/src/main/pty/node-pty-runtime-relocation.test.ts @@ -12,7 +12,10 @@ vi.mock('electron', () => ({ } })) +import { getDaemonPidPath, serializeDaemonPidFile } from '../daemon/daemon-spawner' +import type { DaemonPidFile } from '../daemon/daemon-spawner' import { + collectInUseRuntimeVersions, ensureRelocatedNodePtyNativeRuntime, resolveNodePtyNativeSourceDir } from './node-pty-runtime-relocation' @@ -42,6 +45,23 @@ function seedSourceDir(dir: string): void { writeFileSync(join(dir, 'conpty', 'OpenConsole.exe'), 'console-host') } +// Writes a daemon pid file exactly as the daemon does (userData/daemon/daemon-v.pid). +function writeDaemonPidFile( + daemonRuntimeDir: string, + protocolVersion: number, + pidFile: DaemonPidFile +): void { + mkdirSync(daemonRuntimeDir, { recursive: true }) + writeFileSync(getDaemonPidPath(daemonRuntimeDir, protocolVersion), serializeDaemonPidFile(pidFile)) +} + +// Deterministic liveness that treats the given pids as running, so tests never +// depend on the host's real process table. +function aliveFor(...alivePids: number[]): (pid: number) => boolean { + const set = new Set(alivePids) + return (pid) => set.has(pid) +} + describe('resolveNodePtyNativeSourceDir', () => { it('prefers the rebuilt build/Release binding over prebuilds', () => { const pkg = join(tempDir, 'node-pty') @@ -66,13 +86,98 @@ describe('resolveNodePtyNativeSourceDir', () => { }) }) +describe('collectInUseRuntimeVersions', () => { + it('returns an empty set when the daemon dir does not exist', () => { + expect(collectInUseRuntimeVersions(join(tempDir, 'no-daemon-dir')).size).toBe(0) + }) + + it('collects the app version a live daemon pins', () => { + const daemonDir = join(tempDir, 'daemon') + writeDaemonPidFile(daemonDir, 18, { pid: 4321, startedAtMs: null, appVersion: '1.4.124-rc.1' }) + + const inUse = collectInUseRuntimeVersions(daemonDir, aliveFor(4321)) + + expect([...inUse]).toEqual(['1.4.124-rc.1']) + }) + + it('omits the version of a daemon whose pid is dead', () => { + const daemonDir = join(tempDir, 'daemon') + writeDaemonPidFile(daemonDir, 18, { pid: 4321, startedAtMs: null, appVersion: '1.4.124-rc.1' }) + + const inUse = collectInUseRuntimeVersions(daemonDir, aliveFor(/* nobody */)) + + expect(inUse.size).toBe(0) + }) + + it('collects one version per live daemon across protocol versions', () => { + const daemonDir = join(tempDir, 'daemon') + writeDaemonPidFile(daemonDir, 18, { pid: 100, startedAtMs: null, appVersion: '1.4.124-rc.1' }) + writeDaemonPidFile(daemonDir, 19, { pid: 200, startedAtMs: null, appVersion: '1.4.124-rc.2' }) + + const inUse = collectInUseRuntimeVersions(daemonDir, aliveFor(100, 200)) + + expect([...inUse].sort()).toEqual(['1.4.124-rc.1', '1.4.124-rc.2']) + }) + + it('ignores a daemon whose pid file records no app version', () => { + const daemonDir = join(tempDir, 'daemon') + // Pre-relocation daemon: JSON pid file without appVersion. + writeDaemonPidFile(daemonDir, 18, { pid: 4321, startedAtMs: null }) + // Legacy daemon: bare-integer pid file (appVersion parses to null). + mkdirSync(daemonDir, { recursive: true }) + writeFileSync(getDaemonPidPath(daemonDir, 17), '9999') + + const inUse = collectInUseRuntimeVersions(daemonDir, aliveFor(4321, 9999)) + + expect(inUse.size).toBe(0) + }) + + it('ignores non-pid files in the daemon dir', () => { + const daemonDir = join(tempDir, 'daemon') + mkdirSync(daemonDir, { recursive: true }) + writeFileSync(join(daemonDir, 'daemon-v18.token'), 'secret') + writeFileSync(join(daemonDir, 'daemon-v18.sock'), 'x') + writeFileSync(join(daemonDir, 'notes.txt'), 'x') + + expect(collectInUseRuntimeVersions(daemonDir, aliveFor(1, 2, 3)).size).toBe(0) + }) + + it('skips a malformed pid file without throwing', () => { + const daemonDir = join(tempDir, 'daemon') + mkdirSync(daemonDir, { recursive: true }) + writeFileSync(getDaemonPidPath(daemonDir, 18), 'not-a-pid-at-all') + writeDaemonPidFile(daemonDir, 19, { pid: 200, startedAtMs: null, appVersion: '2.0.0' }) + + const inUse = collectInUseRuntimeVersions(daemonDir, aliveFor(200)) + + expect([...inUse]).toEqual(['2.0.0']) + }) + + it('treats the current process as alive under the default liveness probe', () => { + const daemonDir = join(tempDir, 'daemon') + // startedAtMs null so startTimeMatches short-circuits true on every platform. + writeDaemonPidFile(daemonDir, 18, { + pid: process.pid, + startedAtMs: null, + appVersion: '3.0.0' + }) + + expect([...collectInUseRuntimeVersions(daemonDir)]).toEqual(['3.0.0']) + }) +}) + describe('ensureRelocatedNodePtyNativeRuntime', () => { it('copies the runtime tree (without symbols) and returns the version dir', () => { const sourceDir = join(tempDir, 'source') seedSourceDir(sourceDir) const destRoot = join(tempDir, 'dest') - const destDir = ensureRelocatedNodePtyNativeRuntime({ sourceDir, destRoot, version: '1.2.3' }) + const destDir = ensureRelocatedNodePtyNativeRuntime({ + sourceDir, + destRoot, + version: '1.2.3', + daemonRuntimeDir: join(tempDir, 'daemon') + }) expect(destDir).toBe(join(destRoot, '1.2.3')) expect(readFileSync(join(destDir!, 'conpty.node'), 'utf8')).toBe('binding') @@ -85,10 +190,20 @@ describe('ensureRelocatedNodePtyNativeRuntime', () => { const sourceDir = join(tempDir, 'source') seedSourceDir(sourceDir) const destRoot = join(tempDir, 'dest') - ensureRelocatedNodePtyNativeRuntime({ sourceDir, destRoot, version: '1.2.3' }) + ensureRelocatedNodePtyNativeRuntime({ + sourceDir, + destRoot, + version: '1.2.3', + daemonRuntimeDir: join(tempDir, 'daemon') + }) writeFileSync(join(sourceDir, 'conpty.node'), 'changed-after-first-copy') - const destDir = ensureRelocatedNodePtyNativeRuntime({ sourceDir, destRoot, version: '1.2.3' }) + const destDir = ensureRelocatedNodePtyNativeRuntime({ + sourceDir, + destRoot, + version: '1.2.3', + daemonRuntimeDir: join(tempDir, 'daemon') + }) expect(readFileSync(join(destDir!, 'conpty.node'), 'utf8')).toBe('binding') }) @@ -100,30 +215,70 @@ describe('ensureRelocatedNodePtyNativeRuntime', () => { mkdirSync(join(destRoot, '1.2.3'), { recursive: true }) writeFileSync(join(destRoot, '1.2.3', 'conpty.node'), 'torn partial copy') - const destDir = ensureRelocatedNodePtyNativeRuntime({ sourceDir, destRoot, version: '1.2.3' }) + const destDir = ensureRelocatedNodePtyNativeRuntime({ + sourceDir, + destRoot, + version: '1.2.3', + daemonRuntimeDir: join(tempDir, 'daemon') + }) expect(readFileSync(join(destDir!, 'conpty.node'), 'utf8')).toBe('binding') }) - it('removes stale version dirs but keeps the current one', () => { + it('reclaims a stale version dir once no live daemon pins it', () => { const sourceDir = join(tempDir, 'source') seedSourceDir(sourceDir) const destRoot = join(tempDir, 'dest') - ensureRelocatedNodePtyNativeRuntime({ sourceDir, destRoot, version: '1.0.0' }) + const daemonRuntimeDir = join(tempDir, 'daemon') + ensureRelocatedNodePtyNativeRuntime({ sourceDir, destRoot, version: '1.0.0', daemonRuntimeDir }) - const destDir = ensureRelocatedNodePtyNativeRuntime({ sourceDir, destRoot, version: '2.0.0' }) + // The 1.0.0 daemon exited, so its pid is dead — nothing pins 1.0.0. + writeDaemonPidFile(daemonRuntimeDir, 18, { pid: 4321, startedAtMs: null, appVersion: '1.0.0' }) + + const destDir = ensureRelocatedNodePtyNativeRuntime({ + sourceDir, + destRoot, + version: '2.0.0', + daemonRuntimeDir, + isDaemonPidAlive: aliveFor(/* 4321 is dead */) + }) expect(destDir).toBe(join(destRoot, '2.0.0')) expect(existsSync(join(destRoot, '1.0.0'))).toBe(false) expect(existsSync(join(destRoot, '2.0.0', 'conpty.node'))).toBe(true) }) + it('preserves a stale version dir a surviving daemon still pins', () => { + const sourceDir = join(tempDir, 'source') + seedSourceDir(sourceDir) + const destRoot = join(tempDir, 'dest') + const daemonRuntimeDir = join(tempDir, 'daemon') + ensureRelocatedNodePtyNativeRuntime({ sourceDir, destRoot, version: '1.0.0', daemonRuntimeDir }) + + // The 1.0.0 daemon survived the update and still loads conpty.dll from its + // 1.0.0 runtime dir on every new spawn — deleting it would strand it. + writeDaemonPidFile(daemonRuntimeDir, 18, { pid: 4321, startedAtMs: null, appVersion: '1.0.0' }) + + const destDir = ensureRelocatedNodePtyNativeRuntime({ + sourceDir, + destRoot, + version: '2.0.0', + daemonRuntimeDir, + isDaemonPidAlive: aliveFor(4321) + }) + + expect(destDir).toBe(join(destRoot, '2.0.0')) + expect(existsSync(join(destRoot, '1.0.0', 'conpty.node'))).toBe(true) + expect(existsSync(join(destRoot, '2.0.0', 'conpty.node'))).toBe(true) + }) + it('fails open when the source dir is missing', () => { expect( ensureRelocatedNodePtyNativeRuntime({ sourceDir: join(tempDir, 'does-not-exist'), destRoot: join(tempDir, 'dest'), - version: '1.2.3' + version: '1.2.3', + daemonRuntimeDir: join(tempDir, 'daemon') }) ).toBeNull() }) @@ -140,7 +295,8 @@ describe.runIf(process.platform === 'win32')('patched node-pty loader override', const destDir = ensureRelocatedNodePtyNativeRuntime({ sourceDir: sourceDir!, destRoot: join(tempDir, 'relocated-runtime'), - version: 'loader-test' + version: 'loader-test', + daemonRuntimeDir: join(tempDir, 'daemon') }) expect(destDir).not.toBeNull() diff --git a/src/main/pty/node-pty-runtime-relocation.ts b/src/main/pty/node-pty-runtime-relocation.ts index a3821dde5..0e8600662 100644 --- a/src/main/pty/node-pty-runtime-relocation.ts +++ b/src/main/pty/node-pty-runtime-relocation.ts @@ -3,13 +3,14 @@ import { existsSync, mkdirSync, readdirSync, - renameSync, + readFileSync, rmSync, writeFileSync } from 'node:fs' import { createRequire } from 'node:module' import { dirname, join } from 'node:path' import { app } from 'electron' +import { parseDaemonPidFile, startTimeMatches } from '../daemon/daemon-health' /** * Relocates node-pty's Windows native runtime (conpty.node, conpty.dll, @@ -55,7 +56,61 @@ function copyRuntimeTree(sourceDir: string, destDir: string): void { } } -function removeStaleRuntimeVersions(destRoot: string, keepVersion: string): void { +type IsDaemonPidAlive = (pid: number, startedAtMs: number | null) => boolean + +function isDaemonPidAliveDefault(pid: number, startedAtMs: number | null): boolean { + try { + process.kill(pid, 0) + } catch { + return false + } + return startTimeMatches(pid, startedAtMs) +} + +/** + * Runtime version dirs still claimed by a live daemon, read from the daemon + * pid files under `daemonRuntimeDir` (`daemon-v.pid`). + * + * Why: a daemon deliberately survives app updates, and it was forked with + * ORCA_NODE_PTY_NATIVE_DIR pinned to its own app-version runtime dir — which + * it reloads conpty.dll from on every new spawn. Each pid file records that + * `appVersion`, so a live daemon's version dir must never be reclaimed. + */ +export function collectInUseRuntimeVersions( + daemonRuntimeDir: string, + isPidAlive: IsDaemonPidAlive = isDaemonPidAliveDefault +): Set { + const inUse = new Set() + let entries + try { + entries = readdirSync(daemonRuntimeDir, { withFileTypes: true }) + } catch { + return inUse + } + for (const entry of entries) { + if (!entry.isFile() || !/^daemon-v\d+\.pid$/.test(entry.name)) { + continue + } + let parsed + try { + parsed = parseDaemonPidFile(readFileSync(join(daemonRuntimeDir, entry.name), 'utf8')) + } catch { + continue + } + // appVersion null => a pre-relocation daemon that loads from the install + // dir and thus pins no version dir here. + if (parsed && parsed.appVersion !== null && isPidAlive(parsed.pid, parsed.startedAtMs)) { + inUse.add(parsed.appVersion) + } + } + return inUse +} + +function removeStaleRuntimeVersions( + destRoot: string, + keepVersion: string, + inUseVersions: ReadonlySet +): void { let entries try { entries = readdirSync(destRoot, { withFileTypes: true }) @@ -63,19 +118,18 @@ function removeStaleRuntimeVersions(destRoot: string, keepVersion: string): void return } for (const entry of entries) { - if (!entry.isDirectory() || entry.name === keepVersion) { + if (!entry.isDirectory() || entry.name === keepVersion || inUseVersions.has(entry.name)) { continue } - // Why: an adopted daemon from a previous version may still run (or later - // respawn) binaries out of its own version dir. Renaming a directory - // fails on Windows while anything inside is open, so a successful rename - // proves the dir is unused and safe to delete. - const doomedPath = join(destRoot, `${entry.name}.stale`) + // Why: Windows maps the daemon's conpty.dll/OpenConsole.exe with + // FILE_SHARE_DELETE, so a rename/delete succeeds even while a surviving + // daemon still needs them — the old rename heuristic deleted in-use dirs + // and stranded the daemon (new spawn -> conpty.dll ERROR_PATH_NOT_FOUND). + // Only dirs no live daemon claims via its pid file are safe to remove. try { - renameSync(join(destRoot, entry.name), doomedPath) - rmSync(doomedPath, { recursive: true, force: true }) + rmSync(join(destRoot, entry.name), { recursive: true, force: true }) } catch { - // Still in use (or already being cleaned) — retry on a future launch. + // Still locked or already gone — retry on a future launch. } } } @@ -84,8 +138,10 @@ export function ensureRelocatedNodePtyNativeRuntime(options: { sourceDir: string destRoot: string version: string + daemonRuntimeDir: string + isDaemonPidAlive?: IsDaemonPidAlive }): string | null { - const { sourceDir, destRoot, version } = options + const { sourceDir, destRoot, version, daemonRuntimeDir, isDaemonPidAlive } = options const destDir = join(destRoot, version) try { // Why: the marker is written only after a full copy, so a crash mid-copy @@ -95,7 +151,8 @@ export function ensureRelocatedNodePtyNativeRuntime(options: { copyRuntimeTree(sourceDir, destDir) writeFileSync(join(destDir, RELOCATION_COMPLETE_MARKER), '') } - removeStaleRuntimeVersions(destRoot, version) + const inUseVersions = collectInUseRuntimeVersions(daemonRuntimeDir, isDaemonPidAlive) + removeStaleRuntimeVersions(destRoot, version, inUseVersions) return destDir } catch { // Fail open: node-pty keeps loading from the install dir, which is the @@ -126,10 +183,14 @@ export function installRelocatedNodePtyNativeRuntime(): void { if (!sourceDir) { return } + const userData = app.getPath('userData') const destDir = ensureRelocatedNodePtyNativeRuntime({ sourceDir, - destRoot: join(app.getPath('userData'), 'node-pty-runtime'), - version: app.getVersion() + destRoot: join(userData, 'node-pty-runtime'), + version: app.getVersion(), + // Keyed to daemon-init's runtimeDir (userData/daemon), where surviving + // daemons write daemon-v.pid recording the runtime version they pin. + daemonRuntimeDir: join(userData, 'daemon') }) if (destDir) { process.env[NODE_PTY_NATIVE_DIR_ENV_VAR] = destDir