mirror of
https://github.com/stablyai/orca.git
synced 2026-09-23 08:02:31 +00:00
* fix(pty): close the pseudoconsole when a Windows shell exits by itself
`ClosePseudoConsole` is the only thing that reaps a ConPTY's console host.
node-pty calls it from one place, `PtyKill`, which starts by looking the baton
up by id -- and the exit watcher in `SetupExitCallback` erased that baton the
moment the shell died. So on the self-exit path (typing `exit`, how panes
usually close) the lookup missed, `PtyKill` did nothing at all, and the
pseudoconsole was never closed.
The baton now survives until BOTH the shell has exited and `kill()` has run;
whichever arrives second frees it. `PtyKill` copies `hpc` out under the lock and
closes it afterwards, guards `TerminateProcess` on a shell handle the watcher
may already have closed, and duplicates that handle rather than reordering, so
upstream's close-then-terminate sequence is unchanged.
Measured on Windows 11, 20 self-exit cycles driven exactly as Orca drives them
(`onExit -> destroy()`), handles bucketed by NT object type:
relay spawn (no useConptyDll) 225 -> 285 (+1 Process +2 File/term)
after 219 -> 219 FLAT
desktop spawn (useConptyDll) 239 -> 439 (+1 Process +2 Thread +5 File/term)
after 235 -> 395 (+2 Thread +4 File/term)
The desktop residue is a separate defect in the `useConptyDll` branch of
`WindowsPtyAgent.kill()`, which disposes the conout worker only from an
`_outSocket.on('data')` handler -- and no data arrives after the shell has gone.
Fixing that line as well takes the desktop to 222 -> 222 FLAT, but it lives in
the `kill()` hunk owned by F23, so it is left to that change.
Refs F24.
* fix(pty): dispose the conout worker when a Windows shell exits by itself
Second, independent defect on the same self-exit path, and the larger half of
the desktop's leak. The `useConptyDll` branch of `WindowsPtyAgent.kill()`
disposed the conout worker only from an `_outSocket.on('data')` handler -- and
once the shell has gone no more data ever arrives, so the worker was never
disposed. The non-DLL branch three lines above already disposed unconditionally,
which is why only the desktop (the only spawner that sets `useConptyDll`) hit it.
Measured on Windows 11, 20 cycles, handles bucketed by NT object type, totals:
self-exit, relay spawn 225 -> 285 now 219 -> 219 FLAT
self-exit, desktop spawn 239 -> 439 now 222 -> 222 FLAT
explicit kill, relay spawn 225 -> 285 now 219 -> 219 FLAT
explicit kill, desktop spawn 235 -> 395 now 219 -> 219 FLAT
Neither fix alone is enough on the desktop: the pseudoconsole close is worth
+1 Process +1 File per terminal, this dispose +2 Thread +4 File.
The relay asset (config/relay-assets/node-pty-1.1.0-windows-pty-teardown-patch.cjs)
deliberately gets no counterpart: the relay takes the non-DLL branch, where the
dispose is already unconditional. Its reconstruction table needs the new hunk
though, or un-applying the desktop hunks no longer yields published node-pty.
Taken over from F23 at win-relay-qa's request after they verified that the
desktop never executes the non-DLL branch F23 was scoped around.
Refs F24.
* fix(pty): harden PtyKill against a failed handle duplication and a missing DLL
Both from review of #18635.
DuplicateHandle's result was dropped. On the live explicit-kill path a failed
duplication left hShellDup null, which the guard below could not tell apart from
the self-exit case, so TerminateProcess was skipped and the shell kept running
after its pane closed -- a worse outcome than the handle leak this patch exists
to fix. The failure now terminates through handle->hShell under the lock, where
it is valid and where TerminateProcess does not block. The only cost is that the
rare path kills before the console closes instead of after.
LoadConptyDll is now resolved BEFORE any baton state is touched, matching what
PtyConnect already does for the same reason. It throws when conpty.dll is
missing, and a throw after consoleClosed was set would strand the pseudoconsole
permanently: the retry finds the work claimed and does nothing.
Also corrects three comments the earlier commits made stale:
- the ptyJobMutex note still said PtyKill reads the table unlocked
- PtyListJobProcessIds said the baton is gone once the shell exits; it now
outlives the shell, and the nulled hJob is what makes the answer null
- windows-pty-job.ts said node-pty drops its handle record on exit
Re-measured on Windows 11 with the rebuilt binary, 20 cycles, all four paths
still flat: self-exit relay 219->219, self-exit desktop 222->222, explicit-kill
relay 219->219, explicit-kill desktop 219->219. Both explicit-kill runs report
22/22 shells exited, so the kill still lands.
Refs F24.
214 lines
8.8 KiB
JavaScript
214 lines
8.8 KiB
JavaScript
// The relay's copy of the ConPTY teardown release, and the guard that keeps it in lockstep with the
|
|
// desktop's own node-pty patch. pnpm patches do not cross the SSH boundary, so a relay runs the tree
|
|
// `npm install` put there; the desktop had this fix and the relay did not, and every terminal on a
|
|
// Windows SSH host leaked one File handle for the life of the relay process.
|
|
//
|
|
// The ORDER of the conin release is the fix. Releasing it at the top of the branch -- what the
|
|
// desktop patch does -- was measured at 3x WORSE than shipping nothing (File +2/terminal and a new
|
|
// Process +1/terminal); releasing it after the console-list fork and the native kill is flat.
|
|
import { createRequire } from 'node:module'
|
|
import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'
|
|
import { join, resolve } from 'node:path'
|
|
import { afterEach, describe, expect, it } from 'vitest'
|
|
|
|
const require = createRequire(import.meta.url)
|
|
const {
|
|
assertPatchedNodePtyWindowsTeardown,
|
|
patchNodePtyWindowsTeardown
|
|
} = require('../relay-assets/node-pty-1.1.0-windows-pty-teardown-patch.cjs')
|
|
const projectDir = resolve(import.meta.dirname, '..', '..')
|
|
const cleanupDirs = []
|
|
|
|
const PATCHED_FILES = ['windowsPtyAgent.js', 'windowsTerminal.js']
|
|
|
|
/** The hunks config/patches/node-pty@1.1.0.patch adds to the installed desktop tree. */
|
|
const DESKTOP_HUNKS = {
|
|
'windowsPtyAgent.js': [
|
|
[
|
|
[
|
|
' this._inSocket.readable = false;',
|
|
' // The non-DLL path previously only flipped `readable`, leaving the',
|
|
' // conin PipeWrap alive until the host exited (#947).',
|
|
' this._inSocket.destroy();',
|
|
' this._outSocket.readable = false;',
|
|
''
|
|
].join('\n'),
|
|
[
|
|
' this._inSocket.readable = false;',
|
|
' this._outSocket.readable = false;',
|
|
''
|
|
].join('\n')
|
|
],
|
|
// The useConptyDll branch, which only the DESKTOP runs -- the relay takes the
|
|
// non-DLL branch above, where the dispose is already unconditional. Listed here
|
|
// so un-applying still yields published; the relay asset needs no counterpart.
|
|
[
|
|
[
|
|
' // Orca: dispose unconditionally, as the non-DLL branch above does.',
|
|
" // Waiting for another 'data' event leaks the conout worker on every",
|
|
' // self-exiting shell, because no more data ever arrives (F24).',
|
|
' this._conoutSocketWorker.dispose();',
|
|
''
|
|
].join('\n'),
|
|
[
|
|
" this._outSocket.on('data', function () {",
|
|
' _this._conoutSocketWorker.dispose();',
|
|
' });',
|
|
''
|
|
].join('\n')
|
|
]
|
|
],
|
|
'windowsTerminal.js': [
|
|
[
|
|
' // Attach before readiness so a broken ConPTY output pipe cannot be unhandled.',
|
|
null
|
|
],
|
|
[' // A ConPTY input-pipe error must retire only this terminal.', null]
|
|
]
|
|
}
|
|
|
|
function desktopPath(file) {
|
|
return join(projectDir, 'node_modules', 'node-pty', 'lib', file)
|
|
}
|
|
|
|
afterEach(() => {
|
|
for (const dir of cleanupDirs.splice(0)) {
|
|
rmSync(dir, { recursive: true, force: true })
|
|
}
|
|
})
|
|
|
|
describe('Windows SSH relay node-pty ConPTY teardown patch', () => {
|
|
// Why reconstruct rather than vendor upstream: the installed tree IS the published file plus the
|
|
// desktop's hunks, so un-applying them yields upstream exactly -- and pinning that against this
|
|
// asset's own hashes is what fails loudly if either side of the pair moves.
|
|
it('takes the desktop error listeners verbatim', () => {
|
|
const fixture = writeNodePtyFixture('1.1.0')
|
|
patchNodePtyWindowsTeardown(fixture.root)
|
|
|
|
expect(readFileSync(join(fixture.libDir, 'windowsTerminal.js'), 'utf8')).toBe(
|
|
readFileSync(desktopPath('windowsTerminal.js'), 'utf8')
|
|
)
|
|
})
|
|
|
|
// The one hunk that must NOT match the desktop, and the reason is measured, not stylistic:
|
|
// releasing conin before `_getConsoleProcessList()` forks aborts teardown partway.
|
|
it('releases conin after the console-list fork, not before it like the desktop patch', () => {
|
|
const fixture = writeNodePtyFixture('1.1.0')
|
|
patchNodePtyWindowsTeardown(fixture.root)
|
|
const patched = readFileSync(join(fixture.libDir, 'windowsPtyAgent.js'), 'utf8')
|
|
|
|
const branch = patched.slice(
|
|
patched.indexOf('if (!this._useConptyDll) {'),
|
|
patched.indexOf('else {', patched.indexOf('if (!this._useConptyDll) {'))
|
|
)
|
|
expect(branch).toContain('this._inSocket.destroy();')
|
|
expect(branch.indexOf('this._inSocket.destroy();')).toBeGreaterThan(
|
|
branch.indexOf('this._conoutSocketWorker.dispose();')
|
|
)
|
|
expect(branch.indexOf('this._inSocket.destroy();')).toBeGreaterThan(
|
|
branch.indexOf('this._getConsoleProcessList()')
|
|
)
|
|
// Pinned so a future "sync the relay asset to config/patches" cannot copy the regression back.
|
|
expect(patched).not.toBe(readFileSync(desktopPath('windowsPtyAgent.js'), 'utf8'))
|
|
})
|
|
|
|
it('installs and verifies idempotently', () => {
|
|
const fixture = writeNodePtyFixture('1.1.0')
|
|
|
|
patchNodePtyWindowsTeardown(fixture.root)
|
|
const once = PATCHED_FILES.map((file) => readFileSync(join(fixture.libDir, file), 'utf8'))
|
|
for (const file of PATCHED_FILES) {
|
|
expect(existsSync(`${join(fixture.libDir, file)}.orca-patch-${process.pid}`)).toBe(false)
|
|
}
|
|
expect(() => assertPatchedNodePtyWindowsTeardown(fixture.root)).not.toThrow()
|
|
|
|
patchNodePtyWindowsTeardown(fixture.root)
|
|
expect(PATCHED_FILES.map((file) => readFileSync(join(fixture.libDir, file), 'utf8'))).toEqual(
|
|
once
|
|
)
|
|
})
|
|
|
|
it('refuses a different package version or unexpected source', () => {
|
|
const wrongVersion = writeNodePtyFixture('1.2.0-beta.11')
|
|
expect(() => patchNodePtyWindowsTeardown(wrongVersion.root)).toThrow('expected 1.1.0')
|
|
|
|
for (const file of PATCHED_FILES) {
|
|
const drifted = writeNodePtyFixture('1.1.0')
|
|
const path = join(drifted.libDir, file)
|
|
writeFileSync(path, `${readFileSync(path, 'utf8')}\n// drift`)
|
|
expect(() => patchNodePtyWindowsTeardown(drifted.root)).toThrow('unexpected node-pty')
|
|
}
|
|
})
|
|
|
|
it('refuses a half-applied tree, so one file cannot pass for both', () => {
|
|
for (const file of PATCHED_FILES) {
|
|
const partial = writeNodePtyFixture('1.1.0')
|
|
const fixture = writeNodePtyFixture('1.1.0')
|
|
patchNodePtyWindowsTeardown(fixture.root)
|
|
writeFileSync(join(partial.libDir, file), readFileSync(join(fixture.libDir, file), 'utf8'))
|
|
expect(() => assertPatchedNodePtyWindowsTeardown(partial.root)).toThrow('is not installed')
|
|
}
|
|
})
|
|
})
|
|
|
|
/** A published node-pty tree, rebuilt by un-applying the desktop hunks from the installed one. */
|
|
function writeNodePtyFixture(version) {
|
|
const root = mkdtempSync(join(projectDir, '.node-pty-teardown-patch-test-'))
|
|
cleanupDirs.push(root)
|
|
const libDir = join(root, 'node_modules', 'node-pty', 'lib')
|
|
mkdirSync(libDir, { recursive: true })
|
|
writeFileSync(join(root, 'node_modules', 'node-pty', 'package.json'), JSON.stringify({ version }))
|
|
for (const file of PATCHED_FILES) {
|
|
const desktop = readFileSync(desktopPath(file), 'utf8')
|
|
for (const [marker] of DESKTOP_HUNKS[file]) {
|
|
expect(desktop).toContain(marker)
|
|
}
|
|
writeFileSync(join(libDir, file), unapplyDesktopHunks(file, desktop))
|
|
}
|
|
return { root, libDir }
|
|
}
|
|
|
|
/**
|
|
* Reverse of the published-to-desktop transform.
|
|
*
|
|
* `windowsTerminal.js` is taken verbatim from the desktop, so the asset's own replacement table is
|
|
* the transform and reversing it is exact. `windowsPtyAgent.js` deliberately diverges, so its
|
|
* published form is rebuilt from the desktop hunk instead -- which is also what makes this file the
|
|
* place that notices if the desktop hunk itself ever moves.
|
|
*/
|
|
function unapplyDesktopHunks(file, desktop) {
|
|
if (file === 'windowsPtyAgent.js') {
|
|
let published = desktop
|
|
for (const [patched, original] of DESKTOP_HUNKS[file]) {
|
|
expect(published.split(patched).length - 1).toBe(1)
|
|
published = published.replace(patched, original)
|
|
}
|
|
return published
|
|
}
|
|
const asset = readFileSync(
|
|
join(projectDir, 'config', 'relay-assets', 'node-pty-1.1.0-windows-pty-teardown-patch.cjs'),
|
|
'utf8'
|
|
)
|
|
const { PATCH_TARGETS } = loadPatchTargets(asset)
|
|
const target = PATCH_TARGETS.find((entry) => entry.relativePath.at(-1) === file)
|
|
expect(target).toBeDefined()
|
|
let published = desktop
|
|
for (const [from, to] of target.replacements.toReversed()) {
|
|
expect(published.split(to).length - 1).toBe(1)
|
|
published = published.replace(to, from)
|
|
}
|
|
return published
|
|
}
|
|
|
|
function loadPatchTargets(assetSource) {
|
|
const module = { exports: {} }
|
|
const factory = new Function(
|
|
'module',
|
|
'exports',
|
|
'require',
|
|
`${assetSource}\nmodule.exports.PATCH_TARGETS = PATCH_TARGETS`
|
|
)
|
|
factory(module, module.exports, require)
|
|
return module.exports
|
|
}
|