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<N>.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 <neil@stably.ai>
This commit is contained in:
Brennan Benson
GitHub
parent
f3f67a51f5
commit
3cd23a13a1
|
|
@ -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<N>.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()
|
||||
|
||||
|
|
|
|||
|
|
@ -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<N>.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<string> {
|
||||
const inUse = new Set<string>()
|
||||
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<string>
|
||||
): 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<N>.pid recording the runtime version they pin.
|
||||
daemonRuntimeDir: join(userData, 'daemon')
|
||||
})
|
||||
if (destDir) {
|
||||
process.env[NODE_PTY_NATIVE_DIR_ENV_VAR] = destDir
|
||||
|
|
|
|||
Loading…
Reference in New Issue