mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 00:02:31 +00:00
fix(pty): close the pseudoconsole and dispose the conout worker on Windows self-exit (F24) (#18635)
* 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.
This commit is contained in:
Neil
@@ -603,7 +603,7 @@ index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..2ae787c5bd4f3eba470584dc658a01a5
|
||||
}
|
||||
#endif
|
||||
diff --git a/src/win/conpty.cc b/src/win/conpty.cc
|
||||
index 7b286d3d644c26141df516929703aa6e129df4b2..ec6bf3932c65b89c013ff133dc6bf46a6a4082ce 100644
|
||||
index 7b286d3d644c26141df516929703aa6e129df4b2..4aed260dd68e6a171dcfd349e9a7c5c97209248e 100644
|
||||
--- a/src/win/conpty.cc
|
||||
+++ b/src/win/conpty.cc
|
||||
@@ -18,6 +18,7 @@
|
||||
@@ -614,7 +614,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..ec6bf3932c65b89c013ff133dc6bf46a
|
||||
#include <vector>
|
||||
#include <Windows.h>
|
||||
#include <strsafe.h>
|
||||
@@ -44,12 +45,29 @@ struct pty_baton {
|
||||
@@ -44,12 +45,39 @@ struct pty_baton {
|
||||
HANDLE hOut;
|
||||
HPCON hpc;
|
||||
|
||||
@@ -630,22 +630,32 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..ec6bf3932c65b89c013ff133dc6bf46a
|
||||
+ // refused to create or assign one (an outer job without breakaway rights),
|
||||
+ // in which case callers fall back to their pre-job behaviour.
|
||||
+ HANDLE hJob = nullptr;
|
||||
+
|
||||
+ // Orca: teardown needs BOTH the shell's death and an explicit kill() before
|
||||
+ // the baton can be freed, so each side records that it has run. Whichever
|
||||
+ // arrives second frees it. Freeing on the shell's death alone -- what this
|
||||
+ // file did before -- destroyed the only record of `hpc` while
|
||||
+ // ClosePseudoConsole was still owed, which is why a self-exiting shell
|
||||
+ // leaked its pseudoconsole and the console host it reaps (#18601 / F24).
|
||||
+ bool shellExited = false;
|
||||
+ bool consoleClosed = false;
|
||||
|
||||
pty_baton(int _id, HANDLE _hIn, HANDLE _hOut, HPCON _hpc) : id(_id), hIn(_hIn), hOut(_hOut), hpc(_hpc) {};
|
||||
};
|
||||
|
||||
static std::vector<std::unique_ptr<pty_baton>> ptyHandles;
|
||||
+// Orca: guards the job accessors below against the exit watcher thread. It does
|
||||
+// NOT make the whole table safe -- PtyResize/PtyClear/PtyKill read it unlocked,
|
||||
+// as they always have -- but it closes the window this patch opened, where the
|
||||
+// watcher can close hShell/hJob and free the baton between a lookup and its use.
|
||||
+// Orca: guards the job accessors below, and PtyKill, against the exit watcher
|
||||
+// thread. It does NOT make the whole table safe -- PtyResize and PtyClear still
|
||||
+// read it unlocked, as they always have -- but it closes the window this patch
|
||||
+// opened, where the watcher can close hShell/hJob and free the baton between a
|
||||
+// lookup and its use.
|
||||
+// Handle VALUES are recycled aggressively, so an unguarded read could pass the
|
||||
+// shell-pid check against an unrelated process and terminate the wrong job.
|
||||
+static std::mutex ptyJobMutex;
|
||||
static volatile LONG ptyCounter;
|
||||
|
||||
static pty_baton* get_pty_baton(int id) {
|
||||
@@ -102,8 +120,27 @@ void SetupExitCallback(Napi::Env env, Napi::Function cb, pty_baton* baton) {
|
||||
@@ -102,8 +130,31 @@ void SetupExitCallback(Napi::Env env, Napi::Function cb, pty_baton* baton) {
|
||||
// Get process exit code.
|
||||
GetExitCodeProcess(baton->hShell, (LPDWORD)(&exit_event->exit_code));
|
||||
// Clean up handles
|
||||
@@ -665,9 +675,13 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..ec6bf3932c65b89c013ff133dc6bf46a
|
||||
+ // Why inside the lock: erasing frees the baton the job accessors hold a
|
||||
+ // pointer to. Note remove_pty_baton must not be an assert() argument --
|
||||
+ // NDEBUG would compile the call away and leak every baton.
|
||||
+ const bool removed = remove_pty_baton(baton->id);
|
||||
+ assert(removed);
|
||||
+ (void)removed;
|
||||
+ baton->shellExited = true;
|
||||
+ if (baton->consoleClosed) {
|
||||
+ const bool removed = remove_pty_baton(baton->id);
|
||||
+ assert(removed);
|
||||
+ (void)removed;
|
||||
+ }
|
||||
+ // Else PtyKill has not run yet and still owns hpc. It frees the baton.
|
||||
+ }
|
||||
+ // Why the lock ends here: BlockingCall below waits on the JS thread, and the
|
||||
+ // JS thread can be waiting on ptyJobMutex inside PtyTerminateJob. Holding
|
||||
@@ -675,7 +689,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..ec6bf3932c65b89c013ff133dc6bf46a
|
||||
|
||||
auto status = tsfn.BlockingCall(exit_event, callback); // In main thread
|
||||
switch (status) {
|
||||
@@ -409,6 +446,15 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) {
|
||||
@@ -409,6 +460,15 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) {
|
||||
throw errorWithCode(info, "UpdateProcThreadAttribute failed");
|
||||
}
|
||||
|
||||
@@ -691,7 +705,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..ec6bf3932c65b89c013ff133dc6bf46a
|
||||
PROCESS_INFORMATION piClient{};
|
||||
fSuccess = !!CreateProcessW(
|
||||
nullptr,
|
||||
@@ -416,7 +462,10 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) {
|
||||
@@ -416,7 +476,10 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) {
|
||||
nullptr, // lpProcessAttributes
|
||||
nullptr, // lpThreadAttributes
|
||||
false, // bInheritHandles VERY IMPORTANT that this is false
|
||||
@@ -703,7 +717,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..ec6bf3932c65b89c013ff133dc6bf46a
|
||||
envArg, // lpEnvironment
|
||||
mutableCwd.get(), // lpCurrentDirectory
|
||||
&siEx.StartupInfo, // lpStartupInfo
|
||||
@@ -426,8 +475,47 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) {
|
||||
@@ -426,8 +489,47 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) {
|
||||
throw errorWithCode(info, "Cannot create process");
|
||||
}
|
||||
|
||||
@@ -753,7 +767,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..ec6bf3932c65b89c013ff133dc6bf46a
|
||||
if (useConptyDll && fLoadedDll)
|
||||
{
|
||||
PFNRELEASEPSEUDOCONSOLE const pfnReleasePseudoConsole = (PFNRELEASEPSEUDOCONSOLE)GetProcAddress(
|
||||
@@ -440,6 +528,8 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) {
|
||||
@@ -440,6 +542,8 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) {
|
||||
|
||||
// Update handle
|
||||
handle->hShell = piClient.hProcess;
|
||||
@@ -762,7 +776,91 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..ec6bf3932c65b89c013ff133dc6bf46a
|
||||
|
||||
// Close the thread handle to avoid resource leak
|
||||
CloseHandle(piClient.hThread);
|
||||
@@ -567,6 +657,143 @@ static Napi::Value PtyKill(const Napi::CallbackInfo& info) {
|
||||
@@ -544,29 +648,215 @@ static Napi::Value PtyKill(const Napi::CallbackInfo& info) {
|
||||
int id = info[0].As<Napi::Number>().Int32Value();
|
||||
const bool useConptyDll = info[1].As<Napi::Boolean>().Value();
|
||||
|
||||
- const pty_baton* handle = get_pty_baton(id);
|
||||
+ // Orca: resolve the DLL BEFORE touching any baton state, for the same reason
|
||||
+ // PtyConnect does it before creating anything. LoadConptyDll throws when
|
||||
+ // conpty.dll is missing, and a throw after consoleClosed was set would strand
|
||||
+ // the pseudoconsole permanently: the retry would find the work already
|
||||
+ // claimed and do nothing. Only the useConptyDll path can throw here; the
|
||||
+ // other returns kernel32.
|
||||
+ HANDLE hLibrary = LoadConptyDll(info, useConptyDll);
|
||||
+ PFNCLOSEPSEUDOCONSOLE pfnClosePseudoConsole = nullptr;
|
||||
+ if (hLibrary != nullptr) {
|
||||
+ pfnClosePseudoConsole = (PFNCLOSEPSEUDOCONSOLE)GetProcAddress(
|
||||
+ (HMODULE)hLibrary,
|
||||
+ useConptyDll ? "ConptyClosePseudoConsole" : "ClosePseudoConsole");
|
||||
+ }
|
||||
|
||||
- if (handle != nullptr) {
|
||||
- HANDLE hLibrary = LoadConptyDll(info, useConptyDll);
|
||||
- bool fLoadedDll = hLibrary != nullptr;
|
||||
- if (fLoadedDll)
|
||||
- {
|
||||
- PFNCLOSEPSEUDOCONSOLE const pfnClosePseudoConsole = (PFNCLOSEPSEUDOCONSOLE)GetProcAddress(
|
||||
- (HMODULE)hLibrary,
|
||||
- useConptyDll ? "ConptyClosePseudoConsole" : "ClosePseudoConsole");
|
||||
- if (pfnClosePseudoConsole)
|
||||
- {
|
||||
- pfnClosePseudoConsole(handle->hpc);
|
||||
+ // Orca: the baton now outlives the shell, so this runs on a self-exited pty
|
||||
+ // too -- that is the whole point. Take what we need under the lock: the
|
||||
+ // watcher thread nulls hShell the moment the shell dies, and TerminateProcess
|
||||
+ // on a handle it just closed is an invalid-handle operation. Duplicating
|
||||
+ // rather than reordering keeps upstream's close-then-terminate sequence.
|
||||
+ HPCON hpc = nullptr;
|
||||
+ HANDLE hShellDup = nullptr;
|
||||
+ bool owed = false;
|
||||
+ {
|
||||
+ std::lock_guard<std::mutex> guard(ptyJobMutex);
|
||||
+ pty_baton* handle = get_pty_baton(id);
|
||||
+ // Why the consoleClosed check: a second kill() would otherwise close the
|
||||
+ // same pseudoconsole twice. Upstream relied on the baton being gone.
|
||||
+ if (handle != nullptr && !handle->consoleClosed) {
|
||||
+ hpc = handle->hpc;
|
||||
+ owed = true;
|
||||
+ handle->consoleClosed = true;
|
||||
+ // Null hShell means a self-exited pty, where there is nothing to kill.
|
||||
+ if (useConptyDll && handle->hShell != nullptr) {
|
||||
+ if (!DuplicateHandle(GetCurrentProcess(), handle->hShell, GetCurrentProcess(),
|
||||
+ &hShellDup, 0, FALSE, DUPLICATE_SAME_ACCESS)) {
|
||||
+ // Why terminate here instead of skipping: a failed duplication leaves
|
||||
+ // hShellDup null, which is indistinguishable from the self-exit case,
|
||||
+ // and skipping would leave the shell RUNNING after its pane closed --
|
||||
+ // a worse outcome than the leak this all exists to fix. hShell is
|
||||
+ // valid under this lock and TerminateProcess does not block, so the
|
||||
+ // only cost is that this rare path kills before the console closes.
|
||||
+ hShellDup = nullptr;
|
||||
+ TerminateProcess(handle->hShell, 1);
|
||||
+ }
|
||||
+ }
|
||||
+ if (handle->shellExited) {
|
||||
+ const bool removed = remove_pty_baton(id);
|
||||
+ assert(removed);
|
||||
+ (void)removed;
|
||||
}
|
||||
+ // Else the shell is still running and the watcher frees the baton.
|
||||
}
|
||||
- if (useConptyDll) {
|
||||
- TerminateProcess(handle->hShell, 1);
|
||||
+ }
|
||||
+
|
||||
+ // Why outside the lock: ClosePseudoConsole blocks until the conout side has
|
||||
+ // drained, and the watcher must be able to take the lock while it does.
|
||||
+ if (owed) {
|
||||
+ if (pfnClosePseudoConsole)
|
||||
+ {
|
||||
+ pfnClosePseudoConsole(hpc);
|
||||
+ }
|
||||
+ if (hShellDup != nullptr) {
|
||||
+ TerminateProcess(hShellDup, 1);
|
||||
+ CloseHandle(hShellDup);
|
||||
}
|
||||
}
|
||||
|
||||
return env.Undefined();
|
||||
}
|
||||
|
||||
@@ -808,9 +906,11 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..ec6bf3932c65b89c013ff133dc6bf46a
|
||||
+ * Orca: the pids still alive in this pty's tree, straight from the kernel.
|
||||
+ *
|
||||
+ * Descendant liveness for a tree that is still tracked, including children that
|
||||
+ * detached from the console. Once the shell exits the baton is gone, so this
|
||||
+ * returns null rather than an empty list -- null means "no answer", never
|
||||
+ * "they died". Also returns null when no job was assigned.
|
||||
+ * detached from the console. Once the shell exits the watcher nulls hJob, which
|
||||
+ * ownsShell rejects, so this returns null rather than an empty list -- null
|
||||
+ * means "no answer", never "they died". (The baton itself now outlives the
|
||||
+ * shell, until kill() runs; hJob is what makes the answer null.) Also returns
|
||||
+ * null when no job was assigned.
|
||||
+ *
|
||||
+ * Does not include the ConPTY console host: CreatePseudoConsole spawns it
|
||||
+ * before this job exists, so it is not a member and ClosePseudoConsole is what
|
||||
@@ -906,7 +1006,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..ec6bf3932c65b89c013ff133dc6bf46a
|
||||
/**
|
||||
* Init
|
||||
*/
|
||||
@@ -577,6 +804,9 @@ Napi::Object init(Napi::Env env, Napi::Object exports) {
|
||||
@@ -577,6 +867,9 @@ Napi::Object init(Napi::Env env, Napi::Object exports) {
|
||||
exports.Set("resize", Napi::Function::New(env, PtyResize));
|
||||
exports.Set("clear", Napi::Function::New(env, PtyClear));
|
||||
exports.Set("kill", Napi::Function::New(env, PtyKill));
|
||||
@@ -917,7 +1017,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..ec6bf3932c65b89c013ff133dc6bf46a
|
||||
};
|
||||
|
||||
diff --git a/lib/windowsPtyAgent.js b/lib/windowsPtyAgent.js
|
||||
index a358ffb..fb3a96f 100644
|
||||
index a358ffb177357e177661033c1b092f9c9d0e5f5a..26c2a4c58799ce649f5113131e4c52f7ed2d87ad 100644
|
||||
--- a/lib/windowsPtyAgent.js
|
||||
+++ b/lib/windowsPtyAgent.js
|
||||
@@ -136,6 +136,9 @@ var WindowsPtyAgent = /** @class */ (function () {
|
||||
@@ -930,6 +1030,20 @@ index a358ffb..fb3a96f 100644
|
||||
this._outSocket.readable = false;
|
||||
this._getConsoleProcessList().then(function (consoleProcessList) {
|
||||
consoleProcessList.forEach(function (pid) {
|
||||
@@ -154,9 +157,10 @@ var WindowsPtyAgent = /** @class */ (function () {
|
||||
// Close the input write handle to signal the end of session.
|
||||
this._inSocket.destroy();
|
||||
this._ptyNative.kill(this._pty, this._useConptyDll);
|
||||
- this._outSocket.on('data', function () {
|
||||
- _this._conoutSocketWorker.dispose();
|
||||
- });
|
||||
+ // 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();
|
||||
}
|
||||
}
|
||||
else {
|
||||
diff --git a/lib/windowsTerminal.js b/lib/windowsTerminal.js
|
||||
index 3c38f89..e20b3e6 100644
|
||||
--- a/lib/windowsTerminal.js
|
||||
@@ -1015,7 +1129,7 @@ index 3c38f89..e20b3e6 100644
|
||||
\ No newline at end of file
|
||||
+//# sourceMappingURL=windowsTerminal.js.map
|
||||
diff --git a/src/windowsPtyAgent.ts b/src/windowsPtyAgent.ts
|
||||
index d705444..ce611b8 100644
|
||||
index d7054449516f0c9a62af351c2caa17331206d530..0c28a32e2e1db2b3f208ddde8443cd4e67bb1ad6 100644
|
||||
--- a/src/windowsPtyAgent.ts
|
||||
+++ b/src/windowsPtyAgent.ts
|
||||
@@ -143,6 +143,9 @@ export class WindowsPtyAgent {
|
||||
@@ -1028,6 +1142,20 @@ index d705444..ce611b8 100644
|
||||
this._outSocket.readable = false;
|
||||
this._getConsoleProcessList().then(consoleProcessList => {
|
||||
consoleProcessList.forEach((pid: number) => {
|
||||
@@ -159,9 +162,10 @@ export class WindowsPtyAgent {
|
||||
// Close the input write handle to signal the end of session.
|
||||
this._inSocket.destroy();
|
||||
(this._ptyNative as IConptyNative).kill(this._pty, this._useConptyDll);
|
||||
- this._outSocket.on('data', () => {
|
||||
- this._conoutSocketWorker.dispose();
|
||||
- });
|
||||
+ // 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();
|
||||
}
|
||||
} else {
|
||||
// Because pty.kill closes the handle, it will kill most processes by itself.
|
||||
diff --git a/src/windowsTerminal.ts b/src/windowsTerminal.ts
|
||||
index 13f6c6d..eda63c8 100644
|
||||
--- a/src/windowsTerminal.ts
|
||||
|
||||
@@ -38,6 +38,24 @@ const DESKTOP_HUNKS = {
|
||||
' 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': [
|
||||
|
||||
Generated
+3
-3
@@ -116,7 +116,7 @@ patchedDependencies:
|
||||
'@xterm/addon-webgl@0.20.0-beta.299': 94687e89a0115e6e6aa102837f986debdc029c091527ee5eb4a4e17ceaf9473e
|
||||
'@xterm/xterm@6.1.0-beta.303': 98756bcedc402bcdb7c6ab7b015d2e59cd18e97b03a2c06a27e95bb3ba429d9d
|
||||
lint-staged@16.4.0: 7333b3837f80a7fbd045964db6d76ba4fc118e49134bdbabb00585b6b7b60673
|
||||
node-pty@1.1.0: e262847f57a1d4d3f2287a843822f7dcf3c9d8655892b07a69eba464e1317eaa
|
||||
node-pty@1.1.0: 7cc9d45f3d2c38f142490d0805e75db55f0eef5174ad41c4b52abc5fbe079ad1
|
||||
|
||||
importers:
|
||||
|
||||
@@ -157,7 +157,7 @@ importers:
|
||||
version: 3.3.1
|
||||
node-pty:
|
||||
specifier: ^1.1.0
|
||||
version: 1.1.0(patch_hash=e262847f57a1d4d3f2287a843822f7dcf3c9d8655892b07a69eba464e1317eaa)
|
||||
version: 1.1.0(patch_hash=7cc9d45f3d2c38f142490d0805e75db55f0eef5174ad41c4b52abc5fbe079ad1)
|
||||
posthog-node:
|
||||
specifier: ^5.33.3
|
||||
version: 5.33.3
|
||||
@@ -12195,7 +12195,7 @@ snapshots:
|
||||
|
||||
node-int64@0.4.0: {}
|
||||
|
||||
node-pty@1.1.0(patch_hash=e262847f57a1d4d3f2287a843822f7dcf3c9d8655892b07a69eba464e1317eaa):
|
||||
node-pty@1.1.0(patch_hash=7cc9d45f3d2c38f142490d0805e75db55f0eef5174ad41c4b52abc5fbe079ad1):
|
||||
dependencies:
|
||||
node-addon-api: 7.1.1
|
||||
|
||||
|
||||
@@ -0,0 +1,134 @@
|
||||
import { readFileSync } from 'node:fs'
|
||||
import { join } from 'node:path'
|
||||
import { describe, expect, it } from 'vitest'
|
||||
|
||||
/**
|
||||
* A shell that exits by itself must still close its pseudoconsole.
|
||||
*
|
||||
* `ClosePseudoConsole` is the only thing that reaps a ConPTY's console host —
|
||||
* Orca's own job-ownership patch says so, because `CreatePseudoConsole` spawns
|
||||
* that host before the per-pty job exists and it is therefore not a job member.
|
||||
* Upstream node-pty calls it from exactly one place, `PtyKill`, which begins by
|
||||
* looking the baton up by id — and the exit watcher in `SetupExitCallback`
|
||||
* erased the baton the moment the shell died. So on the self-exit path (typing
|
||||
* `exit`, which is how panes usually close) that lookup missed, `PtyKill` did
|
||||
* nothing at all, and the pseudoconsole was never closed.
|
||||
*
|
||||
* There is a SECOND, independent defect on the same path: the `useConptyDll`
|
||||
* branch of `WindowsPtyAgent.kill()` disposed the conout worker only from an
|
||||
* `_outSocket.on('data')` handler, and no more data arrives once the shell has
|
||||
* gone — so that worker leaked too. The non-DLL branch beside it already
|
||||
* disposed unconditionally. The desktop always sets `useConptyDll`, so it hit
|
||||
* both; the relay sets neither and hit only the first.
|
||||
*
|
||||
* Measured on Windows 11 / awin, 20 cycles, handles bucketed by NT object type,
|
||||
* totals before -> after:
|
||||
*
|
||||
* self-exit, relay spawn 225 -> 285 becomes 219 -> 219 FLAT
|
||||
* self-exit, desktop spawn 239 -> 439 becomes 222 -> 222 FLAT
|
||||
* explicit kill, relay spawn 225 -> 285 becomes 219 -> 219 FLAT
|
||||
* explicit kill, desktop spawn 235 -> 395 becomes 219 -> 219 FLAT
|
||||
*
|
||||
* Neither fix alone is enough on the desktop: the pseudoconsole close is worth
|
||||
* +1 Process +1 File per terminal, the dispose +2 Thread +4 File.
|
||||
*
|
||||
* WHY THIS IS A PATCH-CONTENT PIN AND NOT A BEHAVIOURAL TEST: the defect is
|
||||
* only observable as a per-NT-type handle count, which needs
|
||||
* `NtQuerySystemInformation(SystemExtendedHandleInformation)`. Nothing in the
|
||||
* repo can read that, and the cheaper Windows-observable proxies do not
|
||||
* discriminate — the console host process is reaped either way (the leak is a
|
||||
* handle to an already-exited object, not an orphaned process), and the
|
||||
* `\\.\pipe\conpty-*` entries disappear either way. Both were measured and
|
||||
* rejected as assertions rather than assumed. So this pins the mechanism
|
||||
* instead, which is the real risk: a future resync of the vendored patch
|
||||
* silently dropping the hunk.
|
||||
*/
|
||||
|
||||
const PATCH = readFileSync(join(__dirname, '../../../config/patches/node-pty@1.1.0.patch'), 'utf8')
|
||||
|
||||
describe('node-pty patch: pseudoconsole close on the self-exit path', () => {
|
||||
it('does not let the exit watcher free the baton while the close is still owed', () => {
|
||||
// Pinned as one block: the erase must stay INSIDE the consoleClosed guard.
|
||||
// Upstream ran it unconditionally, which is the line that caused the leak,
|
||||
// and a resync that re-flattens this is the failure mode worth catching.
|
||||
expect(PATCH).toContain(
|
||||
[
|
||||
'+ baton->shellExited = true;',
|
||||
'+ if (baton->consoleClosed) {',
|
||||
'+ const bool removed = remove_pty_baton(baton->id);',
|
||||
'+ assert(removed);',
|
||||
'+ (void)removed;',
|
||||
'+ }'
|
||||
].join('\n')
|
||||
)
|
||||
})
|
||||
|
||||
it('closes the pseudoconsole from PtyKill even after the shell has exited', () => {
|
||||
// hpc is copied out under the lock, so the close survives the baton's removal.
|
||||
expect(PATCH).toContain('+ hpc = handle->hpc;')
|
||||
expect(PATCH).toContain('+ pfnClosePseudoConsole(hpc);')
|
||||
})
|
||||
|
||||
it('resolves the ConPTY DLL before it claims the close', () => {
|
||||
// LoadConptyDll throws when conpty.dll is missing. Throwing after
|
||||
// consoleClosed was set would strand the pseudoconsole for good: the retry
|
||||
// finds the work claimed and does nothing.
|
||||
const dllResolve = PATCH.indexOf('+ HANDLE hLibrary = LoadConptyDll(info, useConptyDll);')
|
||||
const claim = PATCH.indexOf('+ handle->consoleClosed = true;')
|
||||
expect(dllResolve).toBeGreaterThan(-1)
|
||||
expect(claim).toBeGreaterThan(-1)
|
||||
expect(dllResolve).toBeLessThan(claim)
|
||||
})
|
||||
|
||||
it('reaches hShell only under the null check the watcher can trip', () => {
|
||||
// Pinned as one block. The watcher nulls hShell on exit, and upstream
|
||||
// dereferenced it unconditionally; every remaining use — the duplication and
|
||||
// the failure fallback below it — must stay inside this guard.
|
||||
const guarded = PATCH.slice(
|
||||
PATCH.indexOf('+ if (useConptyDll && handle->hShell != nullptr) {'),
|
||||
PATCH.indexOf('+ if (handle->shellExited) {')
|
||||
)
|
||||
expect(guarded).not.toBe('')
|
||||
expect(guarded).toContain('DuplicateHandle(GetCurrentProcess(), handle->hShell')
|
||||
expect(guarded).toContain('TerminateProcess(handle->hShell, 1);')
|
||||
// No ADDED line outside that guard may terminate through hShell. Removed
|
||||
// (`-`) lines still carry upstream's unguarded call, which is the point.
|
||||
const strayAdds = PATCH.replace(guarded, '')
|
||||
.split('\n')
|
||||
.filter((line) => line.startsWith('+') && line.includes('TerminateProcess(handle->hShell'))
|
||||
expect(strayAdds).toEqual([])
|
||||
})
|
||||
|
||||
it('still kills the shell when DuplicateHandle fails', () => {
|
||||
// A null hShellDup is indistinguishable from the self-exit case, so a
|
||||
// swallowed failure would leave the shell running after its pane closed —
|
||||
// a worse outcome than the leak this patch exists to fix.
|
||||
expect(PATCH).toContain(
|
||||
[
|
||||
'+ hShellDup = nullptr;',
|
||||
'+ TerminateProcess(handle->hShell, 1);',
|
||||
'+ }'
|
||||
].join('\n')
|
||||
)
|
||||
})
|
||||
|
||||
it('keeps the close idempotent so a second kill cannot double-close', () => {
|
||||
expect(PATCH).toContain('+ if (handle != nullptr && !handle->consoleClosed) {')
|
||||
expect(PATCH).toContain('+ handle->consoleClosed = true;')
|
||||
})
|
||||
})
|
||||
|
||||
describe('node-pty patch: conout worker disposal on the self-exit path', () => {
|
||||
// The desktop's larger half: 8 of its 10 leaked handles per terminal.
|
||||
it('disposes the conout worker unconditionally in the useConptyDll branch', () => {
|
||||
expect(PATCH).toContain('+ this._conoutSocketWorker.dispose();')
|
||||
// The data handler is what never fired once the shell had gone.
|
||||
expect(PATCH).toContain("- this._outSocket.on('data', function () {")
|
||||
expect(PATCH).toContain('- _this._conoutSocketWorker.dispose();')
|
||||
})
|
||||
|
||||
it('applies the same change to the TypeScript source the patch also carries', () => {
|
||||
expect(PATCH).toContain('+ this._conoutSocketWorker.dispose();')
|
||||
expect(PATCH).toContain("- this._outSocket.on('data', () => {")
|
||||
})
|
||||
})
|
||||
@@ -118,8 +118,10 @@ export function terminatePtyJob(proc: IPty): JobTerminationOutcome {
|
||||
/**
|
||||
* Pids still alive in a PTY's tree, or null when there is no answer.
|
||||
*
|
||||
* Measured on Windows 11: once the shell exits, node-pty drops its handle
|
||||
* record and closes the job, so a terminated tree reports **null**, not `[]`.
|
||||
* Measured on Windows 11: once the shell exits, node-pty closes the job, so a
|
||||
* terminated tree reports **null**, not `[]`. (Its handle record now outlives
|
||||
* the shell until `kill()` runs — see config/patches/node-pty@1.1.0.patch — but
|
||||
* the nulled job handle is what makes the answer null either way.)
|
||||
* Null therefore means "unverifiable" in the sense of
|
||||
* docs/reference/ssh-execution-boundary.md — this build has no job support,
|
||||
* the terminal is not a ConPTY, or it is no longer tracked. It is never
|
||||
|
||||
Reference in New Issue
Block a user