diff --git a/.gitattributes b/.gitattributes index 399e8099114..178938f7026 100644 --- a/.gitattributes +++ b/.gitattributes @@ -9,3 +9,4 @@ /skill-guides/*.md text eol=lf /skills/*/SKILL.md text eol=lf /src/cli/bundled-skill-guides.ts text eol=lf +/config/patches/node-pty@1.1.0.patch text eol=lf whitespace=-blank-at-eol diff --git a/config/patches/node-pty@1.1.0.patch b/config/patches/node-pty@1.1.0.patch index e100def9346..99f57bb25e2 100644 --- a/config/patches/node-pty@1.1.0.patch +++ b/config/patches/node-pty@1.1.0.patch @@ -1,5 +1,5 @@ diff --git a/binding.gyp b/binding.gyp -index 5f63978b07ab50aaf7523219a2170ec737a6b5db..b3309a07ef99dea7967d7bdd04b9fc3500acacae 100644 +index 5f63978b07ab50aaf7523219a2170ec737a6b5db..c73569f8bac0a79023d26123e10db8a074089cf1 100644 --- a/binding.gyp +++ b/binding.gyp @@ -5,9 +5,6 @@ @@ -18,7 +18,7 @@ index 1ac5758bedd8cf54f32280dea4e4aeb5afdee30d..e619813759c6f14694838bdfbd0ea5f8 +++ b/deps/winpty/src/winpty.gyp @@ -10,7 +10,7 @@ # make -j4 CXX=i686-w64-mingw32-g++ LDFLAGS="-static -static-libgcc -static-libstdc++" - + 'variables': { - 'WINPTY_COMMIT_HASH%': ' void): { pid: number }; ++ connect(ptyId: number, commandLine: string, cwd: string, env: string[], useConptyDll: boolean, windowsAgentJob: boolean, onExitCallback: (exitCode: number) => void): { pid: number }; + resize(ptyId: number, cols: number, rows: number, useConptyDll: boolean): void; + clear(ptyId: number, useConptyDll: boolean): void; + kill(ptyId: number, useConptyDll: boolean): void; ++ terminateJobTree?(ptyId: number, timeoutMs: number): Promise | undefined; + } + + interface IWinptyNative { diff --git a/src/unix/pty.cc b/src/unix/pty.cc -index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..61f39f0cbb91faa2c515f35d2ca850564e6368d9 100644 +index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..5c28e6465a6fd506cbc6b548b6d9cb918d3be0da 100644 --- a/src/unix/pty.cc +++ b/src/unix/pty.cc @@ -23,7 +23,9 @@ @@ -122,11 +217,11 @@ index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..61f39f0cbb91faa2c515f35d2ca85056 #include +#include #include - + #include @@ -237,13 +239,23 @@ pty_getproc(int, char *); #endif - + #if defined(__APPLE__) || defined(__OpenBSD__) +struct pty_spawn_error { + const char* step; @@ -147,12 +242,12 @@ index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..61f39f0cbb91faa2c515f35d2ca85056 - int* err); + pty_spawn_error* err); #endif - + struct DelBuf { @@ -367,10 +379,11 @@ Napi::Value PtyFork(const Napi::CallbackInfo& info) { argv[i + 3] = strdup(arg.c_str()); } - + - int err = -1; - pty_posix_spawn(argv, env, term, &winp, &master, &pid, &err); - if (err != 0) { @@ -167,7 +262,7 @@ index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..61f39f0cbb91faa2c515f35d2ca85056 throw Napi::Error::New(napiEnv, "Could not set master fd to nonblocking."); @@ -684,15 +697,73 @@ pty_getproc(int fd, char *tty) { #endif - + #if defined(__APPLE__) +static const char* +pty_errno_name(int errnum) { @@ -238,7 +333,7 @@ index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..61f39f0cbb91faa2c515f35d2ca85056 + bool acts_initialized = false; + posix_spawnattr_t attrs; + bool attrs_initialized = false; - + for (; count < 3; count++) { low_fds[count] = posix_openpt(O_RDWR); @@ -706,80 +777,118 @@ pty_posix_spawn(char** argv, char** env, @@ -255,7 +350,7 @@ index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..61f39f0cbb91faa2c515f35d2ca85056 + pty_set_spawn_error(err, "grantpt", errno); + goto done; } - + - int res = grantpt(*master) || unlockpt(*master); + res = unlockpt(*master); if (res == -1) { @@ -263,7 +358,7 @@ index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..61f39f0cbb91faa2c515f35d2ca85056 + pty_set_spawn_error(err, "unlockpt", errno); + goto done; } - + // Use TIOCPTYGNAME instead of ptsname() to avoid threading problems. - int slave; char slave_pty_name[128]; @@ -273,14 +368,14 @@ index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..61f39f0cbb91faa2c515f35d2ca85056 + pty_set_spawn_error(err, "ioctl_TIOCPTYGNAME", errno); + goto done; } - + slave = open(slave_pty_name, O_RDWR | O_NOCTTY); if (slave == -1) { - return; + pty_set_spawn_error(err, "open_slave", errno, "slave", slave_pty_name); + goto done; } - + if (termp) { res = tcsetattr(slave, TCSANOW, termp); if (res == -1) { @@ -289,7 +384,7 @@ index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..61f39f0cbb91faa2c515f35d2ca85056 + goto done; }; } - + if (winp) { res = ioctl(slave, TIOCSWINSZ, winp); if (res == -1) { @@ -298,7 +393,7 @@ index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..61f39f0cbb91faa2c515f35d2ca85056 + goto done; } } - + - posix_spawn_file_actions_t acts; - posix_spawn_file_actions_init(&acts); + res = posix_spawn_file_actions_init(&acts); @@ -312,7 +407,7 @@ index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..61f39f0cbb91faa2c515f35d2ca85056 posix_spawn_file_actions_adddup2(&acts, slave, STDERR_FILENO); posix_spawn_file_actions_addclose(&acts, slave); posix_spawn_file_actions_addclose(&acts, *master); - + - posix_spawnattr_t attrs; - posix_spawnattr_init(&attrs); - *err = posix_spawnattr_setflags(&attrs, flags); @@ -328,7 +423,7 @@ index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..61f39f0cbb91faa2c515f35d2ca85056 + pty_set_spawn_error(err, "posix_spawnattr_setflags", res); goto done; } - + sigset_t signal_set; /* Reset all signal the child to their default behavior */ sigfillset(&signal_set); @@ -339,7 +434,7 @@ index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..61f39f0cbb91faa2c515f35d2ca85056 + pty_set_spawn_error(err, "posix_spawnattr_setsigdefault", res); goto done; } - + /* Reset the signal mask for all signals */ sigemptyset(&signal_set); - *err = posix_spawnattr_setsigmask(&attrs, &signal_set); @@ -349,7 +444,7 @@ index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..61f39f0cbb91faa2c515f35d2ca85056 + pty_set_spawn_error(err, "posix_spawnattr_setsigmask", res); goto done; } - + do - *err = posix_spawn(pid, argv[0], &acts, &attrs, argv, env); - while (*err == EINTR); @@ -374,7 +469,7 @@ index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..61f39f0cbb91faa2c515f35d2ca85056 + close(*master); + *master = -1; + } - + - for (; count > 0; count--) { - close(low_fds[count]); + for (size_t i = 0; i <= count && i < 3; i++) { @@ -384,3 +479,364 @@ index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..61f39f0cbb91faa2c515f35d2ca85056 } } #endif +diff --git a/src/win/conpty.cc b/src/win/conpty.cc +index 7b286d3d644c26141df516929703aa6e129df4b2..dc8dd58bf6138d3b3cc2bede3efb6fdf7dbeb28f 100644 +--- a/src/win/conpty.cc ++++ b/src/win/conpty.cc +@@ -45,6 +45,7 @@ struct pty_baton { + HPCON hpc; + + HANDLE hShell; ++ HANDLE hJob = nullptr; + + pty_baton(int _id, HANDLE _hIn, HANDLE _hOut, HPCON _hpc) : id(_id), hIn(_hIn), hOut(_hOut), hpc(_hpc) {}; + }; +@@ -91,7 +92,18 @@ void SetupExitCallback(Napi::Env env, Napi::Function cb, pty_baton* baton) { + delete th; + }); + *th = std::thread([tsfn = std::move(tsfn), baton] { +- auto callback = [](Napi::Env env, Napi::Function cb, ExitEvent *exit_event) { ++ auto callback = [baton](Napi::Env env, Napi::Function cb, ExitEvent *exit_event) { ++ if (baton->hShell != nullptr) { ++ GetExitCodeProcess(baton->hShell, (LPDWORD)(&exit_event->exit_code)); ++ CloseHandle(baton->hShell); ++ baton->hShell = nullptr; ++ } ++ HANDLE hJob = baton->hJob; ++ baton->hJob = nullptr; ++ assert(remove_pty_baton(baton->id)); ++ if (hJob != nullptr) { ++ CloseHandle(hJob); ++ } + cb.Call({Napi::Number::New(env, exit_event->exit_code)}); + delete exit_event; + }; +@@ -99,15 +111,41 @@ void SetupExitCallback(Napi::Env env, Napi::Function cb, pty_baton* baton) { + ExitEvent *exit_event = new ExitEvent; + // Wait for process to complete. + WaitForSingleObject(baton->hShell, INFINITE); +- // Get process exit code. +- GetExitCodeProcess(baton->hShell, (LPDWORD)(&exit_event->exit_code)); +- // Clean up handles +- CloseHandle(baton->hShell); +- assert(remove_pty_baton(baton->id)); ++ ++ if (baton->hJob != nullptr) { ++ // Why: an agent root can exit before its detached tools. Do not publish ++ // PTY exit until Windows proves every member has released its handles. ++ TerminateJobObject(baton->hJob, 1); ++ while (true) { ++ JOBOBJECT_BASIC_ACCOUNTING_INFORMATION accounting{}; ++ if (QueryInformationJobObject( ++ baton->hJob, ++ JobObjectBasicAccountingInformation, ++ &accounting, ++ sizeof(accounting), ++ nullptr) && accounting.ActiveProcesses == 0) { ++ break; ++ } ++ // A permanent query/termination failure deliberately keeps the PTY ++ // owned and un-reaped so destructive callers cannot race deletion. ++ Sleep(25); ++ } ++ } + + auto status = tsfn.BlockingCall(exit_event, callback); // In main thread + switch (status) { + case napi_closing: ++ // The environment no longer accepts JS calls, so no native method can ++ // race these per-baton handles during finalization. ++ if (baton->hShell != nullptr) { ++ CloseHandle(baton->hShell); ++ baton->hShell = nullptr; ++ } ++ if (baton->hJob != nullptr) { ++ CloseHandle(baton->hJob); ++ baton->hJob = nullptr; ++ } ++ delete exit_event; + break; + + case napi_queue_full: +@@ -333,14 +371,15 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { + std::stringstream errorText; + BOOL fSuccess = FALSE; + +- if (info.Length() != 6 || ++ if (info.Length() != 7 || + !info[0].IsNumber() || + !info[1].IsString() || + !info[2].IsString() || + !info[3].IsArray() || + !info[4].IsBoolean() || +- !info[5].IsFunction()) { +- throw Napi::Error::New(env, "Usage: pty.connect(id, cmdline, cwd, env, useConptyDll, exitCallback)"); ++ !info[5].IsBoolean() || ++ !info[6].IsFunction()) { ++ throw Napi::Error::New(env, "Usage: pty.connect(id, cmdline, cwd, env, useConptyDll, windowsAgentJob, exitCallback)"); + } + + const int id = info[0].As().Int32Value(); +@@ -348,7 +387,8 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { + const std::wstring cwd(path_util::to_wstring(info[2].As())); + const Napi::Array envValues = info[3].As(); + const bool useConptyDll = info[4].As().Value(); +- Napi::Function exitCallback = info[5].As(); ++ const bool windowsAgentJob = info[5].As().Value(); ++ Napi::Function exitCallback = info[6].As(); + + // Fetch pty handle from ID and start process + pty_baton* handle = get_pty_baton(id); +@@ -416,7 +456,8 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { + nullptr, // lpProcessAttributes + nullptr, // lpThreadAttributes + false, // bInheritHandles VERY IMPORTANT that this is false +- EXTENDED_STARTUPINFO_PRESENT | CREATE_UNICODE_ENVIRONMENT, // dwCreationFlags ++ EXTENDED_STARTUPINFO_PRESENT | CREATE_UNICODE_ENVIRONMENT | ++ (windowsAgentJob ? CREATE_SUSPENDED : 0), // dwCreationFlags + envArg, // lpEnvironment + mutableCwd.get(), // lpCurrentDirectory + &siEx.StartupInfo, // lpStartupInfo +@@ -426,6 +467,39 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { + throw errorWithCode(info, "Cannot create process"); + } + ++ if (windowsAgentJob) { ++ // Job setup is atomic while the shell is suspended; partial ownership is unsafe. ++ HANDLE hJob = CreateJobObjectW(nullptr, nullptr); ++ JOBOBJECT_EXTENDED_LIMIT_INFORMATION limits{}; ++ limits.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE; ++ if (hJob == nullptr || ++ !SetInformationJobObject(hJob, JobObjectExtendedLimitInformation, &limits, sizeof(limits)) || ++ !AssignProcessToJobObject(hJob, piClient.hProcess)) { ++ const DWORD jobError = GetLastError(); ++ TerminateProcess(piClient.hProcess, 1); ++ WaitForSingleObject(piClient.hProcess, INFINITE); ++ CloseHandle(piClient.hThread); CloseHandle(piClient.hProcess); ++ if (hJob != nullptr) CloseHandle(hJob); ++ SetLastError(jobError); ++ throw errorWithCode(info, "Cannot assign process to Job"); ++ } ++ handle->hJob = hJob; ++ ++ if (ResumeThread(piClient.hThread) == static_cast(-1)) { ++ const DWORD resumeError = GetLastError(); ++ TerminateProcess(piClient.hProcess, 1); ++ WaitForSingleObject(piClient.hProcess, INFINITE); ++ CloseHandle(piClient.hThread); ++ CloseHandle(piClient.hProcess); ++ if (handle->hJob != nullptr) { ++ CloseHandle(handle->hJob); ++ handle->hJob = nullptr; ++ } ++ SetLastError(resumeError); ++ throw errorWithCode(info, "Cannot resume process"); ++ } ++ } ++ + HANDLE hLibrary = LoadConptyDll(info, useConptyDll); + bool fLoadedDll = hLibrary != nullptr; + if (useConptyDll && fLoadedDll) +@@ -567,6 +641,109 @@ static Napi::Value PtyKill(const Napi::CallbackInfo& info) { + return env.Undefined(); + } + ++struct JobTerminationResult { ++ bool drained = false; ++}; ++ ++static Napi::Value PtyTerminateJobTree(const Napi::CallbackInfo& info) { ++ Napi::Env env(info.Env()); ++ Napi::HandleScope scope(env); ++ ++ if (info.Length() != 2 || !info[0].IsNumber() || !info[1].IsNumber()) { ++ throw Napi::Error::New(env, "Usage: pty.terminateJobTree(id, timeoutMs)"); ++ } ++ ++ const int id = info[0].As().Int32Value(); ++ const pty_baton* handle = get_pty_baton(id); ++ if (handle == nullptr || handle->hJob == nullptr) { ++ return env.Undefined(); ++ } ++ ++ const double timeoutValue = info[1].As().DoubleValue(); ++ if (timeoutValue < 1 || timeoutValue > 60000) { ++ throw Napi::RangeError::New(env, "timeoutMs must be between 1 and 60000"); ++ } ++ ++ HANDLE hJob = nullptr; ++ if (!DuplicateHandle( ++ GetCurrentProcess(), ++ handle->hJob, ++ GetCurrentProcess(), ++ &hJob, ++ 0, ++ FALSE, ++ DUPLICATE_SAME_ACCESS)) { ++ return env.Undefined(); ++ } ++ if (!TerminateJobObject(hJob, 1)) { ++ CloseHandle(hJob); ++ return env.Undefined(); ++ } ++ ++ // Why: termination is asynchronous. Keep a duplicate Job handle until the ++ // OS reports no members instead of treating the root process exit as proof. ++ auto deferred = Napi::Promise::Deferred::New(env); ++ std::thread *th = new std::thread; ++ auto tsfn = Napi::ThreadSafeFunction::New( ++ env, ++ Napi::Function::New(env, [](const Napi::CallbackInfo&) {}), ++ "PtyTerminateJobTree_resource", ++ 0, ++ 1, ++ [th](Napi::Env) { ++ th->join(); ++ delete th; ++ }); ++ const DWORD timeoutMs = static_cast(timeoutValue); ++ *th = std::thread([tsfn = std::move(tsfn), deferred, hJob, timeoutMs] { ++ auto *result = new JobTerminationResult; ++ const ULONGLONG startedAt = GetTickCount64(); ++ while (true) { ++ JOBOBJECT_BASIC_ACCOUNTING_INFORMATION accounting{}; ++ if (!QueryInformationJobObject( ++ hJob, ++ JobObjectBasicAccountingInformation, ++ &accounting, ++ sizeof(accounting), ++ nullptr)) { ++ break; ++ } ++ if (accounting.ActiveProcesses == 0) { ++ result->drained = true; ++ break; ++ } ++ if (GetTickCount64() - startedAt >= timeoutMs) { ++ break; ++ } ++ Sleep(25); ++ } ++ CloseHandle(hJob); ++ ++ const auto status = tsfn.BlockingCall( ++ result, ++ [deferred](Napi::Env env, Napi::Function, JobTerminationResult *result) { ++ deferred.Resolve(Napi::Boolean::New(env, result->drained)); ++ delete result; ++ }); ++ switch (status) { ++ case napi_closing: ++ delete result; ++ break; ++ case napi_queue_full: ++ Napi::Error::Fatal("PtyTerminateJobTree", "Queue was full"); ++ case napi_ok: ++ if (tsfn.Release() != napi_ok) { ++ Napi::Error::Fatal("PtyTerminateJobTree", "ThreadSafeFunction.Release() failed"); ++ } ++ break; ++ default: ++ Napi::Error::Fatal("PtyTerminateJobTree", "ThreadSafeFunction.BlockingCall() failed"); ++ } ++ }); ++ ++ return deferred.Promise(); ++} ++ + /** + * Init + */ +@@ -577,6 +754,7 @@ 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)); ++ exports.Set("terminateJobTree", Napi::Function::New(env, PtyTerminateJobTree)); + return exports; + }; + +diff --git a/src/windowsPtyAgent.ts b/src/windowsPtyAgent.ts +index d7054449516f0c9a62af351c2caa17331206d530..43e44678826d5883691931875a91cdf7ae141dd0 100644 +--- a/src/windowsPtyAgent.ts ++++ b/src/windowsPtyAgent.ts +@@ -56,7 +56,8 @@ export class WindowsPtyAgent { + debug: boolean, + private _useConpty: boolean | undefined, + private _useConptyDll: boolean = false, +- conptyInheritCursor: boolean = false ++ conptyInheritCursor: boolean = false, ++ windowsAgentJob: boolean = false + ) { + if (this._useConpty === undefined || this._useConpty === true) { + this._useConpty = this._getWindowsBuildNumber() >= 18309; +@@ -116,7 +117,7 @@ export class WindowsPtyAgent { + this._inSocket.setEncoding('utf8'); + + if (this._useConpty) { +- const connect = (this._ptyNative as IConptyNative).connect(this._pty, commandLine, cwd, env, this._useConptyDll, c => this._$onProcessExit(c)); ++ const connect = (this._ptyNative as IConptyNative).connect(this._pty, commandLine, cwd, env, this._useConptyDll, windowsAgentJob, c => this._$onProcessExit(c)); + this._innerPid = connect.pid; + } + } +@@ -138,6 +139,13 @@ export class WindowsPtyAgent { + } + } + ++ public terminateJobTree(timeoutMs: number): Promise | undefined { ++ if (!this._useConpty) { ++ return undefined; ++ } ++ return (this._ptyNative as IConptyNative).terminateJobTree?.(this._pty, timeoutMs); ++ } ++ + public kill(): void { + // Tell the agent to kill the pty, this releases handles to the process + if (this._useConpty) { +diff --git a/src/windowsTerminal.ts b/src/windowsTerminal.ts +index 13f6c6dbb3d7965c4699568f4704be3b6a33d947..a1189c0fd9d2bc4d9cb31271c9924b60acc04585 100644 +--- a/src/windowsTerminal.ts ++++ b/src/windowsTerminal.ts +@@ -48,7 +48,7 @@ export class WindowsTerminal extends Terminal { + this._deferreds = []; + + // Create new termal. +- this._agent = new WindowsPtyAgent(file, args, parsedEnv, cwd, this._cols, this._rows, false, opt.useConpty, opt.useConptyDll, opt.conptyInheritCursor); ++ this._agent = new WindowsPtyAgent(file, args, parsedEnv, cwd, this._cols, this._rows, false, opt.useConpty, opt.useConptyDll, opt.conptyInheritCursor, opt.windowsAgentJob); + this._socket = this._agent.outSocket; + + // Not available until `ready` event emitted. +@@ -161,6 +161,10 @@ export class WindowsTerminal extends Terminal { + }); + } + ++ public terminateJobTree(timeoutMs: number): Promise | undefined { ++ return this._agent.terminateJobTree(timeoutMs); ++ } ++ + public kill(signal?: string): void { + this._deferNoArgs(() => { + if (signal) { +diff --git a/typings/node-pty.d.ts b/typings/node-pty.d.ts +index 6f050ff1d97b8d3477be7ee95d2af516bd9517b7..4387cafd16596dd0f4674549ff2deecb884f30a7 100644 +--- a/typings/node-pty.d.ts ++++ b/typings/node-pty.d.ts +@@ -108,6 +108,9 @@ declare module 'node-pty' { + * @see https://docs.microsoft.com/en-us/windows/console/createpseudoconsole + */ + conptyInheritCursor?: boolean; ++ ++ /** Orca-specific: own this ConPTY and all descendants in a kill-on-close Job. */ ++ windowsAgentJob?: boolean; + } + + /** +@@ -183,6 +186,9 @@ declare module 'node-pty' { + */ + kill(signal?: string): void; + ++ /** Terminates and waits for the Windows ConPTY Job Object to drain. */ ++ terminateJobTree?(timeoutMs: number): Promise | undefined; ++ + /** + * Pauses the pty for customizable flow control. + */ diff --git a/config/scripts/ensure-native-runtime.mjs b/config/scripts/ensure-native-runtime.mjs index 98b064cd30d..7a236c2338a 100644 --- a/config/scripts/ensure-native-runtime.mjs +++ b/config/scripts/ensure-native-runtime.mjs @@ -64,7 +64,7 @@ function ensureNodeRuntime() { if (!initial.ok) { printCheckError(initial) } - runPnpm(['rebuild', 'node-pty']) + runPnpm(['rebuild', 'node-pty'], { npm_config_build_from_source: 'true' }) verifyNodeRuntimeAfterRebuild() return } @@ -74,7 +74,12 @@ function ensureNodeRuntime() { `[native-runtime] ${formatRuntimeLabel('node')} cannot load native modules; rebuilding ${failedModules.join(', ')} for Node.` ) printCheckError(initial) - runPnpm(['rebuild', ...failedModules]) + runPnpm( + ['rebuild', ...failedModules], + failedModules.includes('node-pty') && requiresPatchedNodePtySourceBuild() + ? { npm_config_build_from_source: 'true' } + : {} + ) verifyNodeRuntimeAfterRebuild() } @@ -272,6 +277,13 @@ function loadNodePtyNativeModule() { `node-pty resolved to ${native.dir}; expected build/Release so Orca's node-pty patch is active` ) } + if ( + requiresPatchedNodePtySourceBuild() && + nativeName === 'conpty' && + typeof native.module.terminateJobTree !== 'function' + ) { + throw new Error('patched node-pty conpty.node is missing Windows Job teardown methods') + } } function assertNodePtyWindowsConptyRuntime(nativeDir) { @@ -300,10 +312,11 @@ function getPatchedNodePtyRebuildReason() { return null } - // Why: a loadable upstream node-pty prebuild is not enough; Orca's Unix - // patch only lands in the source-built build/Release artifacts. + // Why: upstream prebuilds can load while omitting Orca's patched Unix spawn + // handling or Windows Job Object teardown surface. const nodePtyDir = resolve(projectDir, 'node_modules', 'node-pty') - const artifactPaths = [resolve(nodePtyDir, 'build', 'Release', 'pty.node')] + const nativeArtifact = process.platform === 'win32' ? 'conpty.node' : 'pty.node' + const artifactPaths = [resolve(nodePtyDir, 'build', 'Release', nativeArtifact)] // Why: node-pty only builds spawn-helper on macOS; Linux builds only pty.node. if (process.platform === 'darwin') { artifactPaths.push(resolve(nodePtyDir, 'build', 'Release', 'spawn-helper')) @@ -318,10 +331,6 @@ function getPatchedNodePtyRebuildReason() { } function requiresPatchedNodePtySourceBuild() { - if (process.platform === 'win32') { - return false - } - const nodePtyPatchPath = resolve(projectDir, 'config', 'patches', 'node-pty@1.1.0.patch') if (!existsSync(nodePtyPatchPath)) { return false @@ -339,10 +348,11 @@ function getWindowsBuildNumber() { return match && match.length === 4 ? Number.parseInt(match[3], 10) : 0 } -function runPnpm(args) { +function runPnpm(args, extraEnv = {}) { const command = process.platform === 'win32' ? 'pnpm.cmd' : 'pnpm' const result = spawnSync(command, args, { cwd: projectDir, + env: { ...process.env, ...extraEnv }, stdio: 'inherit', shell: process.platform === 'win32' }) diff --git a/config/scripts/ensure-native-runtime.test.mjs b/config/scripts/ensure-native-runtime.test.mjs index 0bc8e5ae077..3e8f1eba0d4 100644 --- a/config/scripts/ensure-native-runtime.test.mjs +++ b/config/scripts/ensure-native-runtime.test.mjs @@ -49,108 +49,101 @@ describe('ensure-native-runtime', () => { } }) - it.skipIf(process.platform === 'win32')( - 'rebuilds patched node-pty artifacts even when the Node load check passes', - () => { - const projectDir = mkTempProject() + it('rebuilds patched node-pty artifacts even when the Node load check passes', () => { + const projectDir = mkTempProject() - try { - const scriptPath = join(projectDir, 'config', 'scripts', 'ensure-native-runtime.mjs') - const logPath = join(projectDir, 'native-runtime.log') - const markerPath = join(projectDir, 'rebuilt.marker') - const binDir = join(projectDir, 'bin') - copyFileSync(sourceScriptPath, scriptPath) - writeLoadableNativeModules(projectDir) - writeNodePtyPatchFile(projectDir) - writeFakePnpm(binDir) + try { + const scriptPath = join(projectDir, 'config', 'scripts', 'ensure-native-runtime.mjs') + const logPath = join(projectDir, 'native-runtime.log') + const markerPath = join(projectDir, 'rebuilt.marker') + const binDir = join(projectDir, 'bin') + copyFileSync(sourceScriptPath, scriptPath) + writeLoadableNativeModules(projectDir) + writeNodePtyPatchFile(projectDir) + writeFakePnpm(binDir) - const result = spawnSync(process.execPath, [scriptPath, '--runtime=node'], { - cwd: projectDir, - encoding: 'utf8', - env: envWithPrependedPath(binDir, { - ORCA_NATIVE_TEST_LOG: logPath, - ORCA_NATIVE_TEST_MARKER: markerPath - }) + const result = spawnSync(process.execPath, [scriptPath, '--runtime=node'], { + cwd: projectDir, + encoding: 'utf8', + env: envWithPrependedPath(binDir, { + ORCA_NATIVE_TEST_LOG: logPath, + ORCA_NATIVE_TEST_MARKER: markerPath }) + }) - expect(result.status, result.stderr).toBe(0) - expect(result.stderr).toContain( - 'Patched node-pty build artifacts are missing; rebuilding native deps.' - ) - expect(readFileSync(logPath, 'utf8')).toContain('pnpm rebuild node-pty\n') - } finally { - rmSync(projectDir, { recursive: true, force: true }) - } + expect(result.status, result.stderr).toBe(0) + expect(result.stderr).toContain( + 'Patched node-pty build artifacts are missing; rebuilding native deps.' + ) + const log = readFileSync(logPath, 'utf8') + expect(log).toContain('pnpm rebuild node-pty\n') + expect(log).toContain('build_from_source=true\n') + } finally { + rmSync(projectDir, { recursive: true, force: true }) } - ) + }) - it.skipIf(process.platform === 'win32')( - 'rebuilds when patched artifacts exist but node-pty resolves to prebuilds', - () => { - const projectDir = mkTempProject() + it('rebuilds when patched artifacts exist but node-pty resolves to prebuilds', () => { + const projectDir = mkTempProject() - try { - const scriptPath = join(projectDir, 'config', 'scripts', 'ensure-native-runtime.mjs') - const logPath = join(projectDir, 'native-runtime.log') - const markerPath = join(projectDir, 'rebuilt.marker') - const binDir = join(projectDir, 'bin') - copyFileSync(sourceScriptPath, scriptPath) - writeLoadableNativeModules(projectDir) - writeNodePtyPatchFile(projectDir) - writePatchedNodePtyBuildArtifacts(projectDir) - writeFakePnpm(binDir) + try { + const scriptPath = join(projectDir, 'config', 'scripts', 'ensure-native-runtime.mjs') + const logPath = join(projectDir, 'native-runtime.log') + const markerPath = join(projectDir, 'rebuilt.marker') + const binDir = join(projectDir, 'bin') + copyFileSync(sourceScriptPath, scriptPath) + writeLoadableNativeModules(projectDir) + writeNodePtyPatchFile(projectDir) + writePatchedNodePtyBuildArtifacts(projectDir) + writeFakePnpm(binDir) - const result = spawnSync(process.execPath, [scriptPath, '--runtime=node'], { - cwd: projectDir, - encoding: 'utf8', - env: envWithPrependedPath(binDir, { - ORCA_NATIVE_TEST_LOG: logPath, - ORCA_NATIVE_TEST_MARKER: markerPath - }) + const result = spawnSync(process.execPath, [scriptPath, '--runtime=node'], { + cwd: projectDir, + encoding: 'utf8', + env: envWithPrependedPath(binDir, { + ORCA_NATIVE_TEST_LOG: logPath, + ORCA_NATIVE_TEST_MARKER: markerPath }) + }) - expect(result.status, result.stderr).toBe(0) - expect(result.stderr).toContain("expected build/Release so Orca's node-pty patch is active") - expect(readFileSync(logPath, 'utf8')).toContain('pnpm rebuild node-pty\n') - } finally { - rmSync(projectDir, { recursive: true, force: true }) - } + expect(result.status, result.stderr).toBe(0) + expect(result.stderr).toContain("expected build/Release so Orca's node-pty patch is active") + expect(readFileSync(logPath, 'utf8')).toContain('pnpm rebuild node-pty\n') + } finally { + rmSync(projectDir, { recursive: true, force: true }) } - ) + }) - it.skipIf(process.platform === 'win32')( - 'keeps the fast path when the platform-specific patched artifacts exist', - () => { - const projectDir = mkTempProject() + it('keeps the fast path when the platform-specific patched artifacts exist', () => { + const projectDir = mkTempProject() - try { - const scriptPath = join(projectDir, 'config', 'scripts', 'ensure-native-runtime.mjs') - const logPath = join(projectDir, 'native-runtime.log') - const markerPath = join(projectDir, 'rebuilt.marker') - const binDir = join(projectDir, 'bin') - copyFileSync(sourceScriptPath, scriptPath) - writeLoadableNativeModules(projectDir, { nativeDir: '../build/Release/' }) - writeNodePtyPatchFile(projectDir) - writePatchedNodePtyBuildArtifacts(projectDir) - writeFakePnpm(binDir) + try { + const scriptPath = join(projectDir, 'config', 'scripts', 'ensure-native-runtime.mjs') + const logPath = join(projectDir, 'native-runtime.log') + const markerPath = join(projectDir, 'rebuilt.marker') + const binDir = join(projectDir, 'bin') + copyFileSync(sourceScriptPath, scriptPath) + writeLoadableNativeModules(projectDir, { nativeDir: '../build/Release/' }) + writeNodePtyPatchFile(projectDir) + writePatchedNodePtyBuildArtifacts(projectDir) + writeFakePnpm(binDir) - const result = spawnSync(process.execPath, [scriptPath, '--runtime=node'], { - cwd: projectDir, - encoding: 'utf8', - env: envWithPrependedPath(binDir, { - ORCA_NATIVE_TEST_LOG: logPath, - ORCA_NATIVE_TEST_MARKER: markerPath - }) + const result = spawnSync(process.execPath, [scriptPath, '--runtime=node'], { + cwd: projectDir, + encoding: 'utf8', + env: envWithPrependedPath(binDir, { + ORCA_NATIVE_TEST_LOG: logPath, + ORCA_NATIVE_TEST_MARKER: markerPath }) + }) - expect(result.status, result.stderr).toBe(0) - expect(result.stderr).not.toContain('Patched node-pty build artifacts are missing') - expect(readFileSync(logPath, 'utf8')).not.toContain('pnpm rebuild node-pty') - } finally { - rmSync(projectDir, { recursive: true, force: true }) - } + expect(result.status, result.stderr).toBe(0) + expect(result.stderr).not.toContain('Patched node-pty build artifacts are missing') + expect(readFileSync(logPath, 'utf8')).not.toContain('pnpm rebuild node-pty') + } finally { + rmSync(projectDir, { recursive: true, force: true }) } - ) + }) }) function mkTempProject() { @@ -211,7 +204,12 @@ exports.loadNativeModule = function loadNativeModule(nativeName) { const dir = ${JSON.stringify(nativeDir)} ?? (rebuilt ? '../build/Release/' : '../prebuilds/' + process.platform + '-' + process.arch + '/') appendFileSync(process.env.ORCA_NATIVE_TEST_LOG, \`node-pty load \${nativeName} dir=\${dir}\\n\`) - return { dir, module: {} } + return { + dir, + module: dir.replace(/\\\\/g, '/').includes('build/Release/') + ? { terminateJobTree() {} } + : {} + } } ` ) @@ -238,7 +236,13 @@ function writeNodePtyPatchFile(projectDir) { function writePatchedNodePtyBuildArtifacts(projectDir) { const buildDir = join(projectDir, 'node_modules', 'node-pty', 'build', 'Release') mkdirSync(buildDir, { recursive: true }) - writeFileSync(join(buildDir, 'pty.node'), '') + writeFileSync(join(buildDir, process.platform === 'win32' ? 'conpty.node' : 'pty.node'), '') + if (process.platform === 'win32') { + const runtimeDir = join(buildDir, 'conpty') + mkdirSync(runtimeDir, { recursive: true }) + writeFileSync(join(runtimeDir, 'conpty.dll'), '') + writeFileSync(join(runtimeDir, 'OpenConsole.exe'), '') + } if (process.platform === 'darwin') { writeFileSync(join(buildDir, 'spawn-helper'), '') } @@ -250,10 +254,23 @@ function writeFakePnpm(binDir) { writeFileSync( shimPath, ` -const { appendFileSync, writeFileSync } = require('node:fs') +const { appendFileSync, mkdirSync, writeFileSync } = require('node:fs') +const { join } = require('node:path') appendFileSync(process.env.ORCA_NATIVE_TEST_LOG, \`pnpm \${process.argv.slice(2).join(' ')}\\n\`) +appendFileSync( + process.env.ORCA_NATIVE_TEST_LOG, + \`build_from_source=\${process.env.npm_config_build_from_source ?? ''}\\n\` +) writeFileSync(process.env.ORCA_NATIVE_TEST_MARKER, 'rebuilt') +if (process.platform === 'win32') { + const buildDir = join(process.cwd(), 'node_modules', 'node-pty', 'build', 'Release') + const runtimeDir = join(buildDir, 'conpty') + mkdirSync(runtimeDir, { recursive: true }) + writeFileSync(join(buildDir, 'conpty.node'), '') + writeFileSync(join(runtimeDir, 'conpty.dll'), '') + writeFileSync(join(runtimeDir, 'OpenConsole.exe'), '') +} ` ) diff --git a/config/scripts/rebuild-native-deps.mjs b/config/scripts/rebuild-native-deps.mjs index cf91055d1e3..057f5cafccb 100644 --- a/config/scripts/rebuild-native-deps.mjs +++ b/config/scripts/rebuild-native-deps.mjs @@ -393,12 +393,13 @@ function getPatchedNodePtyRebuildReason() { return null } - // Why: Orca patches node-pty's native Unix spawn path; upstream prebuilds can - // load successfully in Electron while missing the patched fd/error handling. + // Why: upstream prebuilds can load while omitting Orca's patched Unix spawn + // handling or Windows Job Object teardown surface. const nodePtyDir = resolve(projectDir, 'node_modules', 'node-pty') - const artifactPaths = [resolve(nodePtyDir, 'build', 'Release', 'pty.node')] + const nativeArtifact = rebuildPlatform === 'win32' ? 'conpty.node' : 'pty.node' + const artifactPaths = [resolve(nodePtyDir, 'build', 'Release', nativeArtifact)] // Why: node-pty only builds spawn-helper on macOS; Linux builds only pty.node. - if (process.platform === 'darwin') { + if (rebuildPlatform === 'darwin') { artifactPaths.push(resolve(nodePtyDir, 'build', 'Release', 'spawn-helper')) } const missingArtifact = artifactPaths.find((artifactPath) => !existsSync(artifactPath)) @@ -414,9 +415,6 @@ function requiresPatchedNodePtySourceBuild() { if (!onlyModules.includes('node-pty')) { return false } - if (rebuildPlatform === 'win32') { - return false - } if (rebuildPlatform !== osPlatform() || rebuildArch !== process.arch) { return false } @@ -468,7 +466,8 @@ function loadNativeModule(moduleName) { if (moduleName === 'node-pty') { projectRequire('node-pty') const { loadNativeModule } = projectRequire('node-pty/lib/utils') - const native = loadNativeModule(getNodePtyNativeModuleName()) + const nativeName = getNodePtyNativeModuleName() + const native = loadNativeModule(nativeName) assertNodePtyWindowsConptyRuntime(native.dir) if (requirePatchedNodePtySourceBuild && !isNodePtyReleaseBuildDir(native.dir)) { throw new Error( @@ -477,6 +476,13 @@ function loadNativeModule(moduleName) { '; expected build/Release so Orca\\'s node-pty patch is active' ) } + if ( + requirePatchedNodePtySourceBuild && + nativeName === 'conpty' && + typeof native.module.terminateJobTree !== 'function' + ) { + throw new Error('patched node-pty conpty.node is missing Windows Job teardown methods') + } return } projectRequire(moduleName) diff --git a/config/scripts/rebuild-native-deps.test.mjs b/config/scripts/rebuild-native-deps.test.mjs index 1eacf9aa127..93fa37a1c75 100644 --- a/config/scripts/rebuild-native-deps.test.mjs +++ b/config/scripts/rebuild-native-deps.test.mjs @@ -10,7 +10,7 @@ import { writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' -import { join } from 'node:path' +import { dirname, join } from 'node:path' import { fileURLToPath } from 'node:url' import { describe, expect, it } from 'vitest' @@ -179,94 +179,91 @@ describe('rebuild-native-deps patched node-pty rebuild', () => { } }) - it.skipIf(process.platform === 'win32')( - 'rebuilds when Electron can load node-pty but patched build artifacts are missing', - () => { - const projectDir = mkTempProject() + it('rebuilds when Electron can load node-pty but patched build artifacts are missing', () => { + const projectDir = mkTempProject() - try { - const rebuildLogPath = join(projectDir, 'electron-rebuild.log') - writeFakeUsableElectronPackage(projectDir) - writeFakeElectronRebuild(projectDir, { logPathEnv: 'ORCA_REBUILD_TEST_LOG' }) - writeFakeLoadableNodePty(projectDir) - writeNodePtyPatchFile(projectDir) + try { + const rebuildLogPath = join(projectDir, 'electron-rebuild.log') + writeFakeUsableElectronPackage(projectDir) + writeFakeElectronRebuild(projectDir, { logPathEnv: 'ORCA_REBUILD_TEST_LOG' }) + writeFakeLoadableNodePty(projectDir) + writeNodePtyPatchFile(projectDir) - const result = runRebuildScript(projectDir, { - ORCA_REBUILD_TEST_LOG: rebuildLogPath - }) + const result = runRebuildScript(projectDir, { + ORCA_REBUILD_TEST_LOG: rebuildLogPath + }) - expect(result.status, result.stderr).toBe(0) - expect(result.stdout).toContain( - 'Patched node-pty build artifacts are missing; rebuilding from source.' - ) + expect(result.status, result.stderr).toBe(0) + expect(result.stdout).toContain( + 'Patched node-pty build artifacts are missing; rebuilding from source.' + ) - const rebuildCall = JSON.parse(readFileSync(rebuildLogPath, 'utf8').trim()) - expect(rebuildCall.onlyModules).toEqual(['node-pty']) - expect(rebuildCall.ignoreModules).toEqual(['cpu-features']) - expect(rebuildCall.force).toBe(true) - } finally { - rmSync(projectDir, { recursive: true, force: true }) - } + const rebuildCall = JSON.parse(readFileSync(rebuildLogPath, 'utf8').trim()) + expect(rebuildCall.onlyModules).toEqual( + process.platform === 'win32' ? ['node-pty', 'windows-native-registry'] : ['node-pty'] + ) + expect(rebuildCall.ignoreModules).toEqual(['cpu-features']) + expect(rebuildCall.force).toBe(true) + } finally { + rmSync(projectDir, { recursive: true, force: true }) } - ) + }) - it.skipIf(process.platform === 'win32')( - 'keeps the Electron load-probe fast path once patched node-pty artifacts exist', - () => { - const projectDir = mkTempProject() + it('keeps the Electron load-probe fast path once patched node-pty artifacts exist', () => { + const projectDir = mkTempProject() - try { - const rebuildLogPath = join(projectDir, 'electron-rebuild.log') - writeFakeUsableElectronPackage(projectDir) - writeFakeElectronRebuild(projectDir, { logPathEnv: 'ORCA_REBUILD_TEST_LOG' }) - writeFakeLoadableNodePty(projectDir, { nativeDir: '../build/Release/' }) - writeNodePtyPatchFile(projectDir) - writePatchedNodePtyBuildArtifacts(projectDir) + try { + const rebuildLogPath = join(projectDir, 'electron-rebuild.log') + writeFakeUsableElectronPackage(projectDir) + writeFakeElectronRebuild(projectDir, { logPathEnv: 'ORCA_REBUILD_TEST_LOG' }) + writeFakeLoadableNodePty(projectDir, { nativeDir: '../build/Release/' }) + writeFakeWindowsRegistry(projectDir) + writeNodePtyPatchFile(projectDir) + writePatchedNodePtyBuildArtifacts(projectDir) - const result = runRebuildScript(projectDir, { - ORCA_REBUILD_TEST_LOG: rebuildLogPath - }) + const result = runRebuildScript(projectDir, { + ORCA_REBUILD_TEST_LOG: rebuildLogPath + }) - expect(result.status, result.stderr).toBe(0) - expect(result.stdout).toContain( - 'Native modules already load in Electron; skipping rebuild.' - ) - expect(existsSync(rebuildLogPath)).toBe(false) - } finally { - rmSync(projectDir, { recursive: true, force: true }) - } + expect(result.status, result.stderr).toBe(0) + expect(result.stdout).toContain('Native modules already load in Electron; skipping rebuild.') + expect(existsSync(rebuildLogPath)).toBe(false) + } finally { + rmSync(projectDir, { recursive: true, force: true }) } - ) + }) - it.skipIf(process.platform === 'win32')( - 'rebuilds when patched artifacts exist but Electron falls back to node-pty prebuilds', - () => { - const projectDir = mkTempProject() + it('rebuilds when patched artifacts exist but Electron falls back to node-pty prebuilds', () => { + const projectDir = mkTempProject() - try { - const rebuildLogPath = join(projectDir, 'electron-rebuild.log') - writeFakeUsableElectronPackage(projectDir) - writeFakeElectronRebuild(projectDir, { logPathEnv: 'ORCA_REBUILD_TEST_LOG' }) - writeFakeLoadableNodePty(projectDir, { nativeDir: '../prebuilds/darwin-arm64/' }) - writeNodePtyPatchFile(projectDir) - writePatchedNodePtyBuildArtifacts(projectDir) + try { + const rebuildLogPath = join(projectDir, 'electron-rebuild.log') + writeFakeUsableElectronPackage(projectDir) + writeFakeElectronRebuild(projectDir, { logPathEnv: 'ORCA_REBUILD_TEST_LOG' }) + writeFakeLoadableNodePty(projectDir, { + nativeDir: `../prebuilds/${process.platform}-${process.arch}/` + }) + writeFakeWindowsRegistry(projectDir) + writeNodePtyPatchFile(projectDir) + writePatchedNodePtyBuildArtifacts(projectDir) - const result = runRebuildScript(projectDir, { - ORCA_REBUILD_TEST_LOG: rebuildLogPath - }) + const result = runRebuildScript(projectDir, { + ORCA_REBUILD_TEST_LOG: rebuildLogPath + }) - expect(result.status, result.stderr).toBe(0) - expect(result.stdout).toContain('Native modules do not load in Electron; rebuilding.') - expect(result.stdout).toContain("expected build/Release so Orca's node-pty patch is active") + expect(result.status, result.stderr).toBe(0) + expect(result.stdout).toContain('Native modules do not load in Electron; rebuilding.') + expect(result.stdout).toContain("expected build/Release so Orca's node-pty patch is active") - const rebuildCall = JSON.parse(readFileSync(rebuildLogPath, 'utf8').trim()) - expect(rebuildCall.onlyModules).toEqual(['node-pty']) - expect(rebuildCall.force).toBe(true) - } finally { - rmSync(projectDir, { recursive: true, force: true }) - } + const rebuildCall = JSON.parse(readFileSync(rebuildLogPath, 'utf8').trim()) + expect(rebuildCall.onlyModules).toEqual( + process.platform === 'win32' ? ['node-pty', 'windows-native-registry'] : ['node-pty'] + ) + expect(rebuildCall.force).toBe(true) + } finally { + rmSync(projectDir, { recursive: true, force: true }) } - ) + }) }) function mkTempProject() { @@ -283,8 +280,8 @@ function mkTempProject() { function runRebuildScript(projectDir, extraEnv = {}, args = []) { const env = { ...process.env, - npm_config_platform: 'linux', - npm_config_arch: 'x64' + npm_config_platform: process.platform, + npm_config_arch: process.arch } for (const key of Object.keys(env)) { if ( @@ -415,20 +412,26 @@ export async function rebuild(options) { ) } -function writeFakeUsableElectronPackage(projectDir, { platform = 'linux' } = {}) { +function writeFakeUsableElectronPackage(projectDir, { platform = process.platform } = {}) { writeFakeElectronPackage(projectDir) const electronDir = join(projectDir, 'node_modules', 'electron') - const platformExecutable = platform === 'win32' ? 'electron.exe' : 'electron' - const electronPath = join(electronDir, 'dist', platformExecutable) - mkdirSync(join(electronDir, 'dist'), { recursive: true }) - writeFileSync(join(electronDir, 'path.txt'), platformExecutable) + const executablePath = + platform === 'darwin' + ? 'Electron.app/Contents/MacOS/Electron' + : platform === 'win32' + ? 'electron.exe' + : 'electron' + const electronPath = join(electronDir, 'dist', executablePath) + mkdirSync(dirname(electronPath), { recursive: true }) + writeFileSync(join(electronDir, 'path.txt'), executablePath) writeFileSync(join(electronDir, 'dist', 'version'), 'v41.5.0') if (platform === 'win32') { copyFileSync(process.execPath, electronPath) - } else { - writeFileSync( - electronPath, - `#!/usr/bin/env node + return + } + writeFileSync( + electronPath, + `#!/usr/bin/env node const { spawnSync } = require('node:child_process') const result = spawnSync(process.execPath, process.argv.slice(2), { @@ -444,9 +447,8 @@ if (result.error) { process.exit(result.status ?? 0) ` - ) - chmodSync(electronPath, 0o755) - } + ) + chmodSync(electronPath, 0o755) } function writeFakeNodePtyConptyPayload(projectDir, arch) { @@ -475,7 +477,13 @@ function writeFakeLoadableNodePty(projectDir, { nativeDir = 'prebuilds/pty' } = join(nodePtyDir, 'lib', 'utils.js'), ` exports.loadNativeModule = function loadNativeModule(nativeName) { - return { dir: ${JSON.stringify(nativeDir)}, module: { nativeName } } + const dir = ${JSON.stringify(nativeDir)} + return { + dir, + module: dir.replace(/\\\\/g, '/').includes('build/Release/') + ? { terminateJobTree() {} } + : { nativeName } + } } ` ) @@ -496,6 +504,10 @@ function writeNodePtyPatchFile(projectDir) { } function writePatchedNodePtyBuildArtifacts(projectDir) { + if (process.platform === 'win32') { + writeFakeNodePtyConptyPayload(projectDir, process.arch) + return + } const buildDir = join(projectDir, 'node_modules', 'node-pty', 'build', 'Release') mkdirSync(buildDir, { recursive: true }) writeFileSync(join(buildDir, 'pty.node'), '') diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 0ed99b766af..3d567de0a8a 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -21,7 +21,7 @@ patchedDependencies: hash: 9c1de9931d86864923ff53bc9d64474a86478085ac81ea4e432648c7e23d702c path: config/patches/@xterm__xterm@6.1.0-beta.287.patch node-pty@1.1.0: - hash: 407ae07e1e0e2ff2e8b58696449c54c31e51d87535bc6aa4a7a7b0b561407282 + hash: 0b44048c9daec38eee7bca718b1de63f8bf08338c9acee12d257405a246a9161 path: config/patches/node-pty@1.1.0.patch importers: @@ -63,7 +63,7 @@ importers: version: 3.3.1 node-pty: specifier: ^1.1.0 - version: 1.1.0(patch_hash=407ae07e1e0e2ff2e8b58696449c54c31e51d87535bc6aa4a7a7b0b561407282) + version: 1.1.0(patch_hash=0b44048c9daec38eee7bca718b1de63f8bf08338c9acee12d257405a246a9161) posthog-node: specifier: ^5.33.3 version: 5.33.3 @@ -12043,7 +12043,7 @@ snapshots: undici: 6.27.0 which: 6.0.1 - node-pty@1.1.0(patch_hash=407ae07e1e0e2ff2e8b58696449c54c31e51d87535bc6aa4a7a7b0b561407282): + node-pty@1.1.0(patch_hash=0b44048c9daec38eee7bca718b1de63f8bf08338c9acee12d257405a246a9161): dependencies: node-addon-api: 7.1.1 diff --git a/src/main/daemon/pty-subprocess.test.ts b/src/main/daemon/pty-subprocess.test.ts index b378068a4c2..92c06b583ad 100644 --- a/src/main/daemon/pty-subprocess.test.ts +++ b/src/main/daemon/pty-subprocess.test.ts @@ -101,6 +101,7 @@ function mockPtyProcess(pid = 12345) { write: vi.fn(), resize: vi.fn(), kill: vi.fn(), + terminateJobTree: vi.fn(async () => true), process: 'zsh', onData: vi.fn((cb: (data: string) => void) => { onDataListeners.push(cb) @@ -196,6 +197,54 @@ describe('createPtySubprocess', () => { ) }) + it('requests and exposes spawn-time Windows Job teardown for an agent ConPTY', async () => { + const proc = mockPtyProcess() + spawnMock.mockReturnValue(proc) + const platform = Object.getOwnPropertyDescriptor(process, 'platform') + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + + try { + const handle = createPtySubprocess({ + sessionId: 'windows-agent-job', + cols: 80, + rows: 24, + launchAgent: 'claude', + env: {} + }) + + expect(spawnMock).toHaveBeenCalledWith( + expect.any(String), + expect.any(Array), + expect.objectContaining({ windowsAgentJob: true }) + ) + await expect(handle.terminateJobTree?.(1234)).resolves.toBe(true) + expect(proc.terminateJobTree).toHaveBeenCalledWith(1234) + } finally { + if (platform) { + Object.defineProperty(process, 'platform', platform) + } + } + }) + + it('does not request a Windows Job for a plain ConPTY', () => { + spawnMock.mockReturnValue(mockPtyProcess()) + const platform = Object.getOwnPropertyDescriptor(process, 'platform') + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + + try { + createPtySubprocess({ sessionId: 'windows-plain', cols: 80, rows: 24, env: {} }) + expect(spawnMock).toHaveBeenCalledWith( + expect.any(String), + expect.any(Array), + expect.not.objectContaining({ windowsAgentJob: true }) + ) + } finally { + if (platform) { + Object.defineProperty(process, 'platform', platform) + } + } + }) + it('appends Git prompt guards after the detached daemon inherited config', () => { const proc = mockPtyProcess() spawnMock.mockReturnValue(proc) diff --git a/src/main/daemon/pty-subprocess.ts b/src/main/daemon/pty-subprocess.ts index 60a8af8ac70..1312a0f7072 100644 --- a/src/main/daemon/pty-subprocess.ts +++ b/src/main/daemon/pty-subprocess.ts @@ -496,6 +496,7 @@ function spawnDaemonPtyWithWindowsFallback(args: { cols: number rows: number windowsFallbackAttempts: WindowsShellSpawnAttempt[] + windowsAgentJob: boolean }): { process: pty.IPty shellPath: string @@ -512,7 +513,8 @@ function spawnDaemonPtyWithWindowsFallback(args: { env: args.env, // Why: bundled ConPTY has the modern wrap-marker behavior xterm expects; // legacy system ConPTY can corrupt full-width TUI rows in scrollback. - ...(process.platform === 'win32' ? { useConptyDll: true } : {}) + ...(process.platform === 'win32' ? { useConptyDll: true } : {}), + ...(args.windowsAgentJob ? { windowsAgentJob: true } : {}) }) } @@ -821,7 +823,13 @@ export function createPtySubprocess(opts: PtySubprocessOptions): SubprocessHandl env, cols: size.cols, rows: size.rows, - windowsFallbackAttempts + windowsFallbackAttempts, + // Why: WSL owns a Linux process tree, while only explicit native agent + // sessions should change Windows process-launch semantics. + windowsAgentJob: + process.platform === 'win32' && + pathWin32.basename(shellPath).toLowerCase() !== 'wsl.exe' && + Boolean(opts.launchAgent) }) proc = spawned.process // Why: a Windows fallback (e.g. cmd.exe) carries its own argv-embedded @@ -1044,9 +1052,22 @@ export function createPtySubprocess(opts: PtySubprocessOptions): SubprocessHandl } }) + const nativeWindowsPty = + process.platform === 'win32' && pathWin32.basename(shellPath).toLowerCase() !== 'wsl.exe' + const windowsJobControl = proc as pty.IPty & { + terminateJobTree?(timeoutMs: number): Promise | undefined + } + return { pid: proc.pid, shellPath, + ...(nativeWindowsPty + ? { + nativeWindowsPty: true, + terminateJobTree: (timeoutMs: number) => + windowsJobControl.terminateJobTree?.call(proc, timeoutMs) + } + : {}), ...(startupCommandDeliveredInShellArgs ? { startupCommandDeliveredInShellArgs: true } : {}), getForegroundProcess: () => { // Why: node-pty's `.process` getter reports the PTY's live foreground diff --git a/src/main/daemon/session.ts b/src/main/daemon/session.ts index 1c414046cb3..3ad5434f0f0 100644 --- a/src/main/daemon/session.ts +++ b/src/main/daemon/session.ts @@ -50,6 +50,10 @@ export const PRODUCER_PAUSE_FAILSAFE_MS = 5_000 export type SubprocessHandle = { pid: number + /** True only when pid belongs to this Windows host, never wsl.exe/remote. */ + nativeWindowsPty?: boolean + /** Terminates the spawn-time Job and resolves true only after every member exits. */ + terminateJobTree?(timeoutMs: number): Promise | undefined /** Live foreground process name of the PTY (node-pty's `.process`), e.g. * 'claude' / 'codex' / 'zsh'. Null once the child has exited. */ getForegroundProcess(): string | null @@ -227,6 +231,14 @@ export class Session { return this.subprocess.pid } + get ownsNativeWindowsPty(): boolean { + return this.subprocess.nativeWindowsPty === true + } + + terminateJobTree(timeoutMs: number): Promise | undefined { + return this.subprocess.terminateJobTree?.(timeoutMs) + } + write(data: string): void { if (this._state === 'exited' || this._disposed) { return diff --git a/src/main/daemon/terminal-host-session-shutdown.ts b/src/main/daemon/terminal-host-session-shutdown.ts index 41080ff1249..2c5f947abe5 100644 --- a/src/main/daemon/terminal-host-session-shutdown.ts +++ b/src/main/daemon/terminal-host-session-shutdown.ts @@ -1,4 +1,5 @@ import type { Session } from './session' +import type { TerminalSessionTeardown } from './terminal-session-teardown' import type { TakePendingOutputResult, TerminalSnapshot } from './types' function checkpointTerminalHostSessions( @@ -28,17 +29,19 @@ function checkpointTerminalHostSessions( } } -async function disposeTerminalHostSessions(sessions: Iterable): Promise { +async function disposeTerminalHostSessions( + sessions: ReadonlyMap, + sessionTeardown: TerminalSessionTeardown +): Promise { const results = await Promise.allSettled( - [...sessions].map(async (session) => { + [...sessions].map(async ([sessionId, session]) => { session.detachAllClients() // Why: live children retain native ownership until physical exit, while // exited children must release handles without signalling a recycled pid. if (session.isAlive) { - await session.forceKillAndDisposeSubprocess() - } else { - session.disposeSubprocess() + await sessionTeardown.killSession(sessionId, session, true) } + session.disposeSubprocess() }) ) const rejected = results.find( @@ -51,6 +54,7 @@ async function disposeTerminalHostSessions(sessions: Iterable): Promise export async function shutdownTerminalHostSessions( sessions: Map, + sessionTeardown: TerminalSessionTeardown, onFinalCheckpoint?: ( sessionId: string, snapshot: TerminalSnapshot, @@ -58,6 +62,6 @@ export async function shutdownTerminalHostSessions( ) => void ): Promise { checkpointTerminalHostSessions(sessions, onFinalCheckpoint) - await disposeTerminalHostSessions(sessions.values()) + await disposeTerminalHostSessions(sessions, sessionTeardown) sessions.clear() } diff --git a/src/main/daemon/terminal-host.test.ts b/src/main/daemon/terminal-host.test.ts index cee72eee1cf..57087dcd95f 100644 --- a/src/main/daemon/terminal-host.test.ts +++ b/src/main/daemon/terminal-host.test.ts @@ -630,6 +630,9 @@ describe('TerminalHost', () => { await killing expect(retiredSubprocess.forceKill).not.toHaveBeenCalled() + killWithDescendantSweepMock.mockImplementation(async (_pid: number, finish: () => void) => + finish() + ) await expect( host.createOrAttach({ sessionId: 'agent-natural-exit', diff --git a/src/main/daemon/terminal-host.ts b/src/main/daemon/terminal-host.ts index d8458aa81ad..7f15364aad2 100644 --- a/src/main/daemon/terminal-host.ts +++ b/src/main/daemon/terminal-host.ts @@ -357,7 +357,7 @@ export class TerminalHost { } private async disposeSessions(): Promise { - await shutdownTerminalHostSessions(this.sessions, this.onFinalCheckpoint) + await shutdownTerminalHostSessions(this.sessions, this.sessionTeardown, this.onFinalCheckpoint) this.killedTombstones.clear() } diff --git a/src/main/daemon/terminal-session-teardown.test.ts b/src/main/daemon/terminal-session-teardown.test.ts new file mode 100644 index 00000000000..9e9dd6ba1ac --- /dev/null +++ b/src/main/daemon/terminal-session-teardown.test.ts @@ -0,0 +1,168 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { Session } from './session' +import { TerminalSessionTeardown } from './terminal-session-teardown' +import { shutdownTerminalHostSessions } from './terminal-host-session-shutdown' + +const killWithDescendantSweepMock = vi.hoisted(() => vi.fn()) +const requestWindowsDescendantTreeTerminationMock = vi.hoisted(() => vi.fn()) + +vi.mock('../pty-descendant-termination', () => ({ + killWithDescendantSweep: killWithDescendantSweepMock +})) +vi.mock('../windows-pty-descendant-termination', () => ({ + requestWindowsDescendantTreeTermination: requestWindowsDescendantTreeTerminationMock, + WINDOWS_PTY_JOB_DRAIN_TIMEOUT_MS: 8_000 +})) + +type MockSession = Session & { + setAlive(value: boolean): void + terminateJobTree: ReturnType + forceKillAndWaitForExit: ReturnType +} + +function createSession(opts: { agent?: boolean; nativeWindows?: boolean; hasJob?: boolean } = {}) { + let alive = true + return { + pid: 99999, + launchAgent: opts.agent ? 'claude' : null, + ownsNativeWindowsPty: opts.nativeWindows === true, + get isAlive() { + return alive + }, + isTerminating: false, + beginTermination: vi.fn(() => true), + setAlive(value: boolean) { + alive = value + }, + terminateJobTree: vi.fn(() => (opts.hasJob === false ? undefined : Promise.resolve(true))), + forceKillAndWaitForExit: vi.fn(async () => undefined), + kill: vi.fn(), + scheduleForceDisposeFallback: vi.fn(), + signalTerminationRoot: vi.fn() + } as unknown as MockSession +} + +describe('TerminalSessionTeardown Windows descendant termination', () => { + const originalPlatform = Object.getOwnPropertyDescriptor(process, 'platform') + let sessions: Map + let teardown: TerminalSessionTeardown + + beforeEach(() => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + killWithDescendantSweepMock.mockReset() + requestWindowsDescendantTreeTerminationMock.mockReset() + requestWindowsDescendantTreeTerminationMock.mockResolvedValue(undefined) + sessions = new Map() + teardown = new TerminalSessionTeardown(sessions) + }) + + afterEach(() => { + if (originalPlatform) { + Object.defineProperty(process, 'platform', originalPlatform) + } + }) + + it('terminates an agent Job before force-killing its root', async () => { + const session = createSession({ agent: true, nativeWindows: true }) + sessions.set('agent', session) + + await teardown.killSession('agent', session, true) + + expect(session.beginTermination).toHaveBeenCalledOnce() + expect(killWithDescendantSweepMock).not.toHaveBeenCalled() + expect(session.terminateJobTree).toHaveBeenCalledOnce() + expect(session.terminateJobTree.mock.invocationCallOrder[0]).toBeLessThan( + session.forceKillAndWaitForExit.mock.invocationCallOrder[0]! + ) + expect(requestWindowsDescendantTreeTerminationMock).not.toHaveBeenCalled() + }) + + it('rejects fail-closed after closing the root when the agent Job does not drain', async () => { + const session = createSession({ agent: true, nativeWindows: true }) + session.terminateJobTree.mockResolvedValue(false) + sessions.set('agent', session) + + await expect(teardown.killSession('agent', session, true)).rejects.toThrow( + 'Windows PTY Job did not drain' + ) + + expect(session.forceKillAndWaitForExit).toHaveBeenCalledOnce() + expect(teardown.get('agent')).toBeUndefined() + }) + + it('waits for root failure and retains both Windows teardown errors', async () => { + const session = createSession({ agent: true, nativeWindows: true }) + const rootError = new Error('root exit timed out') + session.terminateJobTree.mockRejectedValue(new Error('Job termination failed')) + session.forceKillAndWaitForExit.mockRejectedValue(rootError) + sessions.set('agent', session) + + const killing = teardown.killSession('agent', session, true) + await expect(killing).rejects.toMatchObject({ + message: 'Agent descendant teardown and root exit both failed', + errors: [expect.objectContaining({ message: 'Job termination failed' }), rootError] + }) + + expect(session.forceKillAndWaitForExit).toHaveBeenCalledOnce() + expect(teardown.get('agent')).toBeUndefined() + }) + + it('uses taskkill before the agent root when Job assignment was unavailable', async () => { + const session = createSession({ agent: true, nativeWindows: true, hasJob: false }) + sessions.set('agent', session) + + await expect(teardown.killSession('agent', session, true)).rejects.toThrow( + 'Windows PTY Job ownership unavailable' + ) + + expect(requestWindowsDescendantTreeTerminationMock).toHaveBeenCalledWith(99999) + expect(requestWindowsDescendantTreeTerminationMock.mock.invocationCallOrder[0]).toBeLessThan( + session.forceKillAndWaitForExit.mock.invocationCallOrder[0]! + ) + }) + + it('preserves native Job descendants for plain terminals', async () => { + const normal = createSession({ nativeWindows: true }) + sessions.set('normal', normal) + await teardown.killSession('normal', normal, true) + + expect(normal.terminateJobTree).not.toHaveBeenCalled() + }) + + it('does not signal a root that exits while taskkill is pending', async () => { + let finishFallback!: () => void + requestWindowsDescendantTreeTerminationMock.mockReturnValue( + new Promise((resolve) => { + finishFallback = resolve + }) + ) + const session = createSession({ agent: true, nativeWindows: true, hasJob: false }) + sessions.set('agent', session) + + const killing = teardown.killSession('agent', session, true) + session.setAlive(false) + sessions.delete('agent') + finishFallback() + await expect(killing).rejects.toThrow('Windows PTY Job ownership unavailable') + + expect(session.forceKillAndWaitForExit).not.toHaveBeenCalled() + }) + + it('routes daemon-wide shutdown through coordinated session teardown', async () => { + const session = createSession({ agent: true, nativeWindows: true }) + const detachAllClients = vi.fn() + const disposeSubprocess = vi.fn() + Object.assign(session, { detachAllClients, disposeSubprocess }) + sessions.set('agent', session) + const killSession = vi.fn(async () => undefined) + + await shutdownTerminalHostSessions(sessions, { + killSession + } as unknown as TerminalSessionTeardown) + + expect(killSession).toHaveBeenCalledWith('agent', session, true) + expect(detachAllClients).toHaveBeenCalledOnce() + expect(disposeSubprocess).toHaveBeenCalledOnce() + expect(sessions.size).toBe(0) + }) +}) diff --git a/src/main/daemon/terminal-session-teardown.ts b/src/main/daemon/terminal-session-teardown.ts index 29fca4f952c..9a9cff68b15 100644 --- a/src/main/daemon/terminal-session-teardown.ts +++ b/src/main/daemon/terminal-session-teardown.ts @@ -1,4 +1,8 @@ import { killWithDescendantSweep } from '../pty-descendant-termination' +import { + requestWindowsDescendantTreeTermination, + WINDOWS_PTY_JOB_DRAIN_TIMEOUT_MS +} from '../windows-pty-descendant-termination' import type { Session } from './session' type AgentTeardownOperation = { @@ -9,8 +13,8 @@ type AgentTeardownOperation = { session: Session } -/** Owns agent teardown by session id until descendant capture and root - * signalling finish, even when the root exits and its Session is reaped. */ +/** Owns agent teardown by session id until descendant termination and root + * signalling finish, even if the Session is reaped. */ export class TerminalSessionTeardown { private operations = new Map() @@ -48,6 +52,20 @@ export class TerminalSessionTeardown { sessionId: string, session: Session, immediate: boolean + ): void | Promise { + return this.killCoordinatedSession(sessionId, session, immediate, (killRoot, ownsRoot) => { + if (process.platform === 'win32' && session.ownsNativeWindowsPty) { + return this.terminateWindowsTree(session, killRoot, ownsRoot) + } + return killWithDescendantSweep(session.pid, killRoot, { ownsRoot }) + }) + } + + private killCoordinatedSession( + sessionId: string, + session: Session, + immediate: boolean, + prepareRootKill: (killRoot: () => void, ownsRoot: () => boolean) => Promise ): void | Promise { const pending = this.operations.get(sessionId) if (pending) { @@ -76,32 +94,39 @@ export class TerminalSessionTeardown { rootCompletion: Promise.resolve(), session } - const sweep = Promise.resolve( - killWithDescendantSweep( - session.pid, - () => { - // Why: natural exit reaps the PID while ps is running. Never signal that - // stale numeric PID after the Session no longer represents a live root. - if (!session.isAlive) { - return - } - entry.rootSignalled = true - if (entry.immediate) { - entry.rootCompletion = session.forceKillAndWaitForExit() - } else { - session.signalTerminationRoot() - } - }, - { - // Why: the descendant rows are only authoritative while this exact - // Session still owns the root PID captured by ps. - ownsRoot: () => this.sessions.get(sessionId) === session && session.isAlive - } - ) - ) + const ownsRoot = (): boolean => this.sessions.get(sessionId) === session && session.isAlive + const killRoot = (): void => { + // Why: natural exit reaps the PID while preparation is running. Never + // signal that stale numeric PID after this Session loses the live root. + if (!ownsRoot()) { + return + } + entry.rootSignalled = true + if (entry.immediate) { + entry.rootCompletion = session.forceKillAndWaitForExit() + } else { + session.signalTerminationRoot() + } + } + const sweep = Promise.resolve(prepareRootKill(killRoot, ownsRoot)) // Why: descendant capture completion only proves signals were requested; - // destructive callers must retain the native owner until OS-confirmed exit. - const operation = sweep.then(() => entry.rootCompletion) + // callers must retain the native owner until OS-confirmed exit. + const operation = sweep.then( + () => entry.rootCompletion, + async (preparationError: unknown) => { + try { + // Why: Windows failure paths still signal the root. Observe that + // completion before rejecting so its timeout cannot escape unhandled. + await entry.rootCompletion + } catch (rootError) { + throw new AggregateError( + [preparationError, rootError], + 'Agent descendant teardown and root exit both failed' + ) + } + throw preparationError + } + ) entry.promise = operation this.operations.set(sessionId, entry) const clearOperation = (): void => { @@ -112,4 +137,33 @@ export class TerminalSessionTeardown { void operation.then(clearOperation, clearOperation) return operation } + + private async terminateWindowsTree( + session: Session, + killRoot: () => void, + ownsRoot: () => boolean + ): Promise { + const nativeCompletion = session.terminateJobTree(WINDOWS_PTY_JOB_DRAIN_TIMEOUT_MS) + if (nativeCompletion) { + killRoot() + if (!(await nativeCompletion)) { + throw new Error(`Windows PTY Job did not drain for process ${session.pid}`) + } + return + } + if (!ownsRoot()) { + throw new Error(`Windows PTY root exited without Job ownership for process ${session.pid}`) + } + + try { + await requestWindowsDescendantTreeTermination(session.pid) + // taskkill cannot identify a descendant after its launcher exits, so a + // successful request is cleanup only and never authorizes deletion. + throw new Error(`Windows PTY Job ownership unavailable for process ${session.pid}`) + } finally { + // Why: fallback failure must not leave the agent root running, while the + // rejected operation still keeps destructive callers fail-closed. + killRoot() + } + } } diff --git a/src/main/providers/local-pty-provider.test.ts b/src/main/providers/local-pty-provider.test.ts index 3eaa8c557d6..f043a1bc6fa 100644 --- a/src/main/providers/local-pty-provider.test.ts +++ b/src/main/providers/local-pty-provider.test.ts @@ -14,7 +14,8 @@ const { resolveAgentForegroundProcessMock, readWindowsConptyProcessIdsMock, captureDescendantSnapshotMock, - terminateDescendantSnapshotMock + terminateDescendantSnapshotMock, + requestWindowsDescendantTreeTerminationMock } = vi.hoisted(() => ({ existsSyncMock: vi.fn(), statSyncMock: vi.fn(), @@ -26,7 +27,8 @@ const { resolveAgentForegroundProcessMock: vi.fn(), readWindowsConptyProcessIdsMock: vi.fn(), captureDescendantSnapshotMock: vi.fn(), - terminateDescendantSnapshotMock: vi.fn() + terminateDescendantSnapshotMock: vi.fn(), + requestWindowsDescendantTreeTerminationMock: vi.fn() })) vi.mock('fs', () => ({ @@ -59,6 +61,11 @@ vi.mock('../pty-descendant-termination', () => ({ terminateDescendantSnapshot: terminateDescendantSnapshotMock })) +vi.mock('../windows-pty-descendant-termination', () => ({ + requestWindowsDescendantTreeTermination: requestWindowsDescendantTreeTerminationMock, + WINDOWS_PTY_JOB_DRAIN_TIMEOUT_MS: 8_000 +})) + // Resolve PowerShell family names to deterministic absolute paths (the fs mock // above otherwise makes every probe miss). The real resolver — which skips the // Store App Execution Alias stub — is covered in @@ -125,6 +132,7 @@ describe('LocalPtyProvider', () => { pause: ReturnType resume: ReturnType kill: ReturnType + terminateJobTree: ReturnType process: string pid: number } @@ -155,6 +163,8 @@ describe('LocalPtyProvider', () => { terminateDescendantSnapshotMock.mockReset() prepareMacosTccLoginShellMock.mockReset() prepareMacosTccLoginShellMock.mockResolvedValue(undefined) + requestWindowsDescendantTreeTerminationMock.mockReset() + requestWindowsDescendantTreeTerminationMock.mockResolvedValue(undefined) resolveAgentForegroundProcessMock.mockReset() resolveAgentForegroundProcessMock.mockImplementation( async (_pid: number, fallbackProcess: string | null) => ({ @@ -185,6 +195,7 @@ describe('LocalPtyProvider', () => { kill: vi.fn(() => { exitCb?.({ exitCode: -1 }) }), + terminateJobTree: vi.fn(async () => true), process: 'zsh', pid: 12345 } @@ -1145,6 +1156,104 @@ describe('LocalPtyProvider', () => { expect(destroySpy).not.toHaveBeenCalled() }) + it('terminates an agent Job before closing its Windows ConPTY', async () => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + const order: string[] = [] + mockProc.terminateJobTree.mockImplementation(async () => { + order.push('job') + return true + }) + mockProc.kill.mockImplementation(() => { + order.push('conpty') + exitCb?.({ exitCode: 1 }) + }) + const { id } = await provider.spawn({ cols: 80, rows: 24, launchAgent: 'claude' }) + expect(spawnMock).toHaveBeenCalledWith( + expect.any(String), + expect.any(Array), + expect.objectContaining({ windowsAgentJob: true }) + ) + + await provider.shutdown(id, { immediate: true }) + + expect(order).toEqual(['job', 'conpty']) + expect(requestWindowsDescendantTreeTerminationMock).not.toHaveBeenCalled() + }) + + it('rejects fail-closed after closing ConPTY when the agent Job does not drain', async () => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + mockProc.terminateJobTree.mockResolvedValue(false) + const { id } = await provider.spawn({ cols: 80, rows: 24, launchAgent: 'claude' }) + + await expect(provider.shutdown(id, { immediate: true })).rejects.toThrow( + 'Windows PTY Job did not drain' + ) + + expect(mockProc.kill).toHaveBeenCalledOnce() + expect(provider.hasPty(id)).toBe(false) + }) + + it('closes ConPTY and rejects when native agent Job termination throws', async () => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + mockProc.terminateJobTree.mockRejectedValue(new Error('native Job failure')) + const { id } = await provider.spawn({ cols: 80, rows: 24, launchAgent: 'claude' }) + + await expect(provider.shutdown(id, { immediate: true })).rejects.toThrow('native Job failure') + + expect(mockProc.kill).toHaveBeenCalledOnce() + expect(provider.hasPty(id)).toBe(false) + }) + + it('preserves plain detached-child semantics for normal Windows tab close', async () => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + const { id } = await provider.spawn({ cols: 80, rows: 24 }) + + await provider.shutdown(id, { immediate: true }) + + expect(mockProc.terminateJobTree).not.toHaveBeenCalled() + expect(requestWindowsDescendantTreeTerminationMock).not.toHaveBeenCalled() + expect(spawnMock).toHaveBeenCalledWith( + expect.any(String), + expect.any(Array), + expect.not.objectContaining({ windowsAgentJob: true }) + ) + }) + + it('falls back to taskkill when the Windows PTY has no Job Object', async () => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + mockProc.terminateJobTree.mockReturnValue(undefined) + const { id } = await provider.spawn({ cols: 80, rows: 24, launchAgent: 'codex' }) + + await expect(provider.shutdown(id, { immediate: true })).rejects.toThrow( + 'Windows PTY Job ownership unavailable' + ) + + expect(requestWindowsDescendantTreeTerminationMock).toHaveBeenCalledWith(mockProc.pid) + expect(requestWindowsDescendantTreeTerminationMock.mock.invocationCallOrder[0]).toBeLessThan( + mockProc.kill.mock.invocationCallOrder[0]! + ) + }) + + it('never runs native Windows tree termination for a WSL terminal', async () => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + const { id } = await provider.spawn({ + cols: 80, + rows: 24, + shellOverride: 'wsl.exe', + launchAgent: 'claude' + }) + + await provider.shutdown(id, { immediate: true }) + + expect(mockProc.terminateJobTree).not.toHaveBeenCalled() + expect(requestWindowsDescendantTreeTerminationMock).not.toHaveBeenCalled() + expect(spawnMock).toHaveBeenCalledWith( + expect.any(String), + expect.any(Array), + expect.not.objectContaining({ windowsAgentJob: true }) + ) + }) + it('keeps shutdown and ownership pending until node-pty reports physical exit', async () => { const killSpy = vi.fn() mockProc.kill = killSpy diff --git a/src/main/providers/local-pty-provider.ts b/src/main/providers/local-pty-provider.ts index 4cc456bd799..b182e43214a 100644 --- a/src/main/providers/local-pty-provider.ts +++ b/src/main/providers/local-pty-provider.ts @@ -60,6 +60,10 @@ import { captureDescendantSnapshot, terminateDescendantSnapshot } from '../pty-descendant-termination' +import { + requestWindowsDescendantTreeTermination, + WINDOWS_PTY_JOB_DRAIN_TIMEOUT_MS +} from '../windows-pty-descendant-termination' import { readWindowsConptyProcessIds } from './windows-conpty-process-membership' import { canConfirmAgentFromConsolePresence } from './windows-console-foreground' import { forceKillPosixPtyProcessGroups } from '../pty/posix-pty-process-groups' @@ -282,6 +286,35 @@ function killLocalPtyProcess(proc: pty.IPty, immediate: boolean): void { forceKillPosixPtyProcessGroups(proc.pid, () => proc.kill('SIGKILL')) } +type WindowsPtyJobControl = pty.IPty & { + terminateJobTree?(timeoutMs: number): Promise | undefined +} + +async function terminateWindowsPtyTree(proc: pty.IPty, closeRoot: () => void): Promise { + const nativeCompletion = (proc as WindowsPtyJobControl).terminateJobTree?.call( + proc, + WINDOWS_PTY_JOB_DRAIN_TIMEOUT_MS + ) + if (nativeCompletion) { + closeRoot() + if (!(await nativeCompletion)) { + throw new Error(`Windows PTY Job did not drain for process ${proc.pid}`) + } + return + } + + try { + await requestWindowsDescendantTreeTermination(proc.pid) + // taskkill cannot identify a descendant after its launcher exits, so a + // successful request is cleanup only and never authorizes deletion. + throw new Error(`Windows PTY Job ownership unavailable for process ${proc.pid}`) + } finally { + // Why: fallback failure must not leave the agent root alive, but callers + // still receive the failure instead of treating worktree handles as free. + closeRoot() + } +} + function armLocalPtyForceKill( id: string, proc: pty.IPty, @@ -878,7 +911,13 @@ export class LocalPtyProvider implements IPtyProvider { onBeforeFallbackSpawn: historyResult?.histFile ? (env, fallbackShell) => updateHistFileForFallback(env, fallbackShell) : undefined, - windowsFallbackAttempts + windowsFallbackAttempts, + // Why: only recognized native agents need descendant ownership. WSL + // belongs to a separate execution host and plain terminals stay unchanged. + windowsAgentJob: + process.platform === 'win32' && + !isWslTerminal && + Boolean(args.launchAgent || startupAgentRecognition) }) shellPath = spawnResult.shellPath // Why: a Windows fallback (e.g. cmd.exe) embeds its own startup command in @@ -1176,12 +1215,16 @@ export class LocalPtyProvider implements IPtyProvider { const physicalExit = ptyPhysicalExits.get(id) // Why: the snapshot must precede any signal — once the shell dies, // surviving descendants reparent to pid 1 and a ppid walk can't find them. - const descendants = ptyAgentSessionIds.has(id) - ? await captureDescendantSnapshot(proc.pid) - : null - // Why: a natural exit can race the snapshot. Never signal descendants or - // a root PID after this exact PTY has lost ownership. - if (ptyProcesses.get(id) === proc) { + const descendants = + process.platform !== 'win32' && ptyAgentSessionIds.has(id) + ? await captureDescendantSnapshot(proc.pid) + : null + const closeRoot = (): void => { + // Why: a natural exit can race teardown preparation. Never signal a + // numeric PID after this exact PTY loses ownership. + if (ptyProcesses.get(id) !== proc) { + return + } if (descendants) { terminateDescendantSnapshot(descendants) } @@ -1191,6 +1234,17 @@ export class LocalPtyProvider implements IPtyProvider { operation.rootSignalled = true this.requestTrackedPtyShutdown(id, proc, operation.immediate) } + if ( + process.platform === 'win32' && + // Why: wsl.exe only launches a Linux tree; Windows Job/taskkill cannot + // prove teardown of descendants owned by that separate execution host. + ptyWslDistroById.get(id) === null && + ptyAgentSessionIds.has(id) + ) { + await terminateWindowsPtyTree(proc, closeRoot) + } else { + closeRoot() + } await waitForPtyPhysicalExit(id, physicalExit) } diff --git a/src/main/providers/local-pty-utils.ts b/src/main/providers/local-pty-utils.ts index 3e21095d535..fa42e6c0777 100644 --- a/src/main/providers/local-pty-utils.ts +++ b/src/main/providers/local-pty-utils.ts @@ -163,6 +163,8 @@ export type ShellSpawnParams = { * fails to spawn, the next real absolute executable is tried with its own * recomputed args/cwd. */ windowsFallbackAttempts?: WindowsShellSpawnAttempt[] + /** Assign native Windows agent shells to Orca's descendant-owning Job. */ + windowsAgentJob?: boolean } export type ShellSpawnResult = { @@ -186,8 +188,12 @@ export type ShellSpawnResult = { // has the modern wrap-marker behavior xterm expects; legacy system ConPTY can // corrupt full-width TUI rows in scrollback. Without this, degraded-mode and // fresh-local spawns silently behave differently from daemon terminals. -function windowsConptyDllOptions(): { useConptyDll: true } | Record { - return process.platform === 'win32' ? { useConptyDll: true } : {} +function windowsConptyDllOptions( + windowsAgentJob?: boolean +): { useConptyDll: true; windowsAgentJob?: true } | Record { + return process.platform === 'win32' + ? { useConptyDll: true, ...(windowsAgentJob ? { windowsAgentJob: true } : {}) } + : {} } function spawnWindowsFallbackChain( @@ -205,7 +211,7 @@ function spawnWindowsFallbackChain( rows, cwd: attempt.effectiveCwd, env, - ...windowsConptyDllOptions() + ...windowsConptyDllOptions(params.windowsAgentJob) }) console.warn( `[pty] Primary shell "${params.shellPath}" failed (${primaryError}), fell back to "${attempt.shellPath}"` @@ -255,7 +261,7 @@ export function spawnShellWithFallback(params: ShellSpawnParams): ShellSpawnResu rows, cwd, env, - ...windowsConptyDllOptions() + ...windowsConptyDllOptions(params.windowsAgentJob) }), shellPath } diff --git a/src/main/windows-pty-descendant-termination.test.ts b/src/main/windows-pty-descendant-termination.test.ts new file mode 100644 index 00000000000..1b7dc7ebfe8 --- /dev/null +++ b/src/main/windows-pty-descendant-termination.test.ts @@ -0,0 +1,92 @@ +import { EventEmitter } from 'node:events' +import type { ChildProcess } from 'node:child_process' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { + requestWindowsDescendantTreeTermination, + WINDOWS_DESCENDANT_KILL_TIMEOUT_MS, + type WindowsTreeKillSpawner +} from './windows-pty-descendant-termination' + +function createChild(): Pick & EventEmitter { + const child = new EventEmitter() as Pick & EventEmitter + child.kill = vi.fn(() => true) + child.unref = vi.fn() + return child +} + +describe('requestWindowsDescendantTreeTermination', () => { + afterEach(() => { + vi.useRealTimers() + }) + + it('runs the System32 taskkill binary with a forceful tree argv', async () => { + const child = createChild() + const spawnProcess = vi.fn(() => child) as unknown as WindowsTreeKillSpawner + const killing = requestWindowsDescendantTreeTermination(4321, { + spawn: spawnProcess, + env: { SystemRoot: 'C:\\Windows' } + }) + + expect(spawnProcess).toHaveBeenCalledWith( + 'C:\\Windows\\System32\\taskkill.exe', + ['/PID', '4321', '/T', '/F'], + { shell: false, stdio: 'ignore', windowsHide: true } + ) + expect(child.unref).toHaveBeenCalledOnce() + + child.emit('exit', 0, null) + await expect(killing).resolves.toBeUndefined() + }) + + it('falls back to the bare binary and reports a non-zero exit', async () => { + const child = createChild() + const spawnProcess = vi.fn(() => child) as unknown as WindowsTreeKillSpawner + const killing = requestWindowsDescendantTreeTermination(77, { + spawn: spawnProcess, + env: {} + }) + + expect(spawnProcess).toHaveBeenCalledWith( + 'taskkill', + ['/PID', '77', '/T', '/F'], + expect.any(Object) + ) + child.emit('exit', 128, null) + await expect(killing).rejects.toThrow('taskkill exited 128') + }) + + it.each([0, -1, 1.5, Number.NaN, undefined])( + 'does not spawn for invalid pid %s', + async (rootPid) => { + const spawnProcess = vi.fn() as unknown as WindowsTreeKillSpawner + + await expect( + requestWindowsDescendantTreeTermination(rootPid as number, { spawn: spawnProcess }) + ).rejects.toThrow('Invalid Windows PTY root PID') + expect(spawnProcess).not.toHaveBeenCalled() + } + ) + + it('bounds a hung taskkill process and cancels it', async () => { + vi.useFakeTimers() + const child = createChild() + const spawnProcess = vi.fn(() => child) as unknown as WindowsTreeKillSpawner + const killing = requestWindowsDescendantTreeTermination(99, { spawn: spawnProcess }) + const rejection = expect(killing).rejects.toThrow('taskkill timed out') + + await vi.advanceTimersByTimeAsync(WINDOWS_DESCENDANT_KILL_TIMEOUT_MS) + + expect(child.kill).toHaveBeenCalledOnce() + await rejection + }) + + it('reports spawn errors instead of treating them as proof', async () => { + const child = createChild() + const spawnProcess = vi.fn(() => child) as unknown as WindowsTreeKillSpawner + const killing = requestWindowsDescendantTreeTermination(123, { spawn: spawnProcess }) + + child.emit('error', new Error('blocked')) + + await expect(killing).rejects.toThrow('taskkill failed') + }) +}) diff --git a/src/main/windows-pty-descendant-termination.ts b/src/main/windows-pty-descendant-termination.ts new file mode 100644 index 00000000000..d8d4db3faac --- /dev/null +++ b/src/main/windows-pty-descendant-termination.ts @@ -0,0 +1,89 @@ +import { spawn, type ChildProcess, type SpawnOptions } from 'node:child_process' +import { win32 as pathWin32 } from 'node:path' + +export const WINDOWS_DESCENDANT_KILL_TIMEOUT_MS = 2_000 +export const WINDOWS_PTY_JOB_DRAIN_TIMEOUT_MS = 8_000 + +type WindowsTreeKillProcess = Pick + +export type WindowsTreeKillSpawner = ( + file: string, + args: string[], + options: SpawnOptions +) => WindowsTreeKillProcess + +type WindowsTreeKillDeps = { + spawn?: WindowsTreeKillSpawner + env?: NodeJS.ProcessEnv + timeoutMs?: number +} + +/** + * Best-effort fallback for Windows hosts where node-pty could not assign the + * shell to a Job Object. taskkill follows only live PPID links, so it cannot + * reach a detached child whose launcher already exited. It also accepts only + * a PID, leaving a small reuse window after node-pty closes the shell handle. + */ +export function requestWindowsDescendantTreeTermination( + rootPid: number, + deps: WindowsTreeKillDeps = {} +): Promise { + if (!Number.isInteger(rootPid) || rootPid <= 0) { + return Promise.reject(new Error(`Invalid Windows PTY root PID: ${rootPid}`)) + } + + const systemRoot = (deps.env ?? process.env).SystemRoot + const taskkillPath = systemRoot + ? pathWin32.join(systemRoot, 'System32', 'taskkill.exe') + : 'taskkill' + const spawnProcess: WindowsTreeKillSpawner = deps.spawn ?? spawn + + return new Promise((resolve, reject) => { + let settled = false + let child: WindowsTreeKillProcess | undefined + const finish = (error?: Error): void => { + if (settled) { + return + } + settled = true + clearTimeout(timer) + if (error) { + reject(error) + } else { + resolve() + } + } + const timer = setTimeout(() => { + try { + child?.kill() + } catch { + /* taskkill may have exited between the deadline and cancellation */ + } + finish(new Error(`taskkill timed out for Windows PTY root ${rootPid}`)) + }, deps.timeoutMs ?? WINDOWS_DESCENDANT_KILL_TIMEOUT_MS) + timer.unref?.() + + try { + child = spawnProcess(taskkillPath, ['/PID', String(rootPid), '/T', '/F'], { + shell: false, + stdio: 'ignore', + windowsHide: true + }) + child.once('error', (error) => + finish(new Error(`taskkill failed for Windows PTY root ${rootPid}: ${error.message}`)) + ) + child.once('exit', (code) => + finish( + code === 0 + ? undefined + : new Error( + `taskkill exited ${code ?? 'without a code'} for Windows PTY root ${rootPid}` + ) + ) + ) + child.unref() + } catch { + finish(new Error(`Could not start taskkill for Windows PTY root ${rootPid}`)) + } + }) +}