diff --git a/config/patches/node-pty@1.1.0.patch b/config/patches/node-pty@1.1.0.patch index 218e40ba12b..9e3e0b740f5 100644 --- a/config/patches/node-pty@1.1.0.patch +++ b/config/patches/node-pty@1.1.0.patch @@ -430,7 +430,7 @@ index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..383df0c9c48355547c65e6c9bbba593d } #endif diff --git a/src/win/conpty.cc b/src/win/conpty.cc -index 7b286d3d644c26141df516929703aa6e129df4b2..fdb413be4ec39091b60a8b8dc49ed9cb81731730 100644 +index 7b286d3d644c26141df516929703aa6e129df4b2..cad5cbcee5db36f510158e900e21849b298f4de5 100644 --- a/src/win/conpty.cc +++ b/src/win/conpty.cc @@ -18,6 +18,7 @@ @@ -441,15 +441,23 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..fdb413be4ec39091b60a8b8dc49ed9cb #include #include #include -@@ -46,10 +47,22 @@ struct pty_baton { - - HANDLE hShell; +@@ -44,12 +45,29 @@ struct pty_baton { + HANDLE hOut; + HPCON hpc; +- HANDLE hShell; ++ HANDLE hShell = nullptr; ++ // Orca: the shell's pid, captured at spawn. The ownership guard compares ++ // against this rather than calling GetProcessId(hShell), because the exit ++ // watcher closes hShell on another thread -- reading it there is an ++ // invalid-handle operation, and under strict handle checks that is fatal. ++ DWORD shellPid = 0; ++ + // Orca: job object owning this pty's whole process tree. Null when the OS + // 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; -+ + pty_baton(int _id, HANDLE _hIn, HANDLE _hOut, HPCON _hpc) : id(_id), hIn(_hIn), hOut(_hOut), hpc(_hpc) {}; }; @@ -464,16 +472,19 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..fdb413be4ec39091b60a8b8dc49ed9cb static volatile LONG ptyCounter; static pty_baton* get_pty_baton(int id) { -@@ -103,7 +116,22 @@ void SetupExitCallback(Napi::Env env, Napi::Function cb, pty_baton* baton) { +@@ -102,8 +120,24 @@ 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 - CloseHandle(baton->hShell); +- CloseHandle(baton->hShell); - assert(remove_pty_baton(baton->id)); + // Orca: release the job once the shell is gone. Without kill-on-close this + // only frees the handle -- anything the user backgrounded is orphaned, as + // it was before this patch. + { + std::lock_guard guard(ptyJobMutex); ++ CloseHandle(baton->hShell); ++ baton->hShell = nullptr; + if (baton->hJob != nullptr) { + CloseHandle(baton->hJob); + baton->hJob = nullptr; @@ -488,7 +499,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..fdb413be4ec39091b60a8b8dc49ed9cb auto status = tsfn.BlockingCall(exit_event, callback); // In main thread switch (status) { -@@ -416,7 +444,10 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { +@@ -416,7 +450,10 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { nullptr, // lpProcessAttributes nullptr, // lpThreadAttributes false, // bInheritHandles VERY IMPORTANT that this is false @@ -500,7 +511,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..fdb413be4ec39091b60a8b8dc49ed9cb envArg, // lpEnvironment mutableCwd.get(), // lpCurrentDirectory &siEx.StartupInfo, // lpStartupInfo -@@ -426,6 +457,47 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { +@@ -426,6 +463,47 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { throw errorWithCode(info, "Cannot create process"); } @@ -548,10 +559,11 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..fdb413be4ec39091b60a8b8dc49ed9cb HANDLE hLibrary = LoadConptyDll(info, useConptyDll); bool fLoadedDll = hLibrary != nullptr; if (useConptyDll && fLoadedDll) -@@ -440,6 +512,10 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { +@@ -440,6 +518,11 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { // Update handle handle->hShell = piClient.hProcess; ++ handle->shellPid = piClient.dwProcessId; + // Why here and not earlier: LoadConptyDll above can throw, and a baton that + // carries a job but never reaches SetupExitCallback has nothing left to + // close it. @@ -559,7 +571,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..fdb413be4ec39091b60a8b8dc49ed9cb // Close the thread handle to avoid resource leak CloseHandle(piClient.hThread); -@@ -567,6 +643,91 @@ static Napi::Value PtyKill(const Napi::CallbackInfo& info) { +@@ -567,6 +650,94 @@ static Napi::Value PtyKill(const Napi::CallbackInfo& info) { return env.Undefined(); } @@ -572,8 +584,8 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..fdb413be4ec39091b60a8b8dc49ed9cb + * process tree. Matching the shell pid makes the id unforgeable. + */ +static bool ownsShell(const pty_baton* handle, DWORD expectedShellPid) { -+ return handle != nullptr && handle->hJob != nullptr && handle->hShell != nullptr && -+ expectedShellPid != 0 && GetProcessId(handle->hShell) == expectedShellPid; ++ return handle != nullptr && handle->hJob != nullptr && expectedShellPid != 0 && ++ handle->shellPid == expectedShellPid; +} + +/** @@ -604,10 +616,14 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..fdb413be4ec39091b60a8b8dc49ed9cb +/** + * Orca: the pids still alive in this pty's tree, straight from the kernel. + * -+ * This is the liveness answer a stale JS map cannot give: a pane whose shell -+ * has exited reports an empty list, so a registry entry can no longer claim a -+ * terminal is connected when nothing is running. -+ * Returns null when no job was assigned. ++ * 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. ++ * ++ * Does not include the ConPTY console host: CreatePseudoConsole spawns it ++ * before this job exists, so it is not a member and ClosePseudoConsole is what ++ * reaps it. + */ +static Napi::Value PtyListJobProcessIds(const Napi::CallbackInfo& info) { + Napi::Env env(info.Env()); @@ -632,7 +648,6 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..fdb413be4ec39091b60a8b8dc49ed9cb + const size_t bytes = sizeof(JOBOBJECT_BASIC_PROCESS_ID_LIST) + sizeof(ULONG_PTR) * capacity; + std::vector buffer(bytes, 0); + auto* list = reinterpret_cast(buffer.data()); -+ list->NumberOfAssignedProcesses = capacity; + if (QueryInformationJobObject(handle->hJob, JobObjectBasicProcessIdList, list, static_cast(bytes), nullptr)) { + auto pids = Napi::Array::New(env, list->NumberOfProcessIdsInList); + for (DWORD i = 0; i < list->NumberOfProcessIdsInList; i++) { @@ -651,7 +666,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..fdb413be4ec39091b60a8b8dc49ed9cb /** * Init */ -@@ -577,6 +738,8 @@ Napi::Object init(Napi::Env env, Napi::Object exports) { +@@ -577,6 +748,8 @@ 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)); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 37267b91798..b75ce03bfdf 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -27,7 +27,7 @@ patchedDependencies: hash: 7333b3837f80a7fbd045964db6d76ba4fc118e49134bdbabb00585b6b7b60673 path: config/patches/lint-staged@16.4.0.patch node-pty@1.1.0: - hash: a8de16410ba90ef37d6486bebf03aaebcf2963514fbf1f5bd81f56eea7d96939 + hash: c48841645e1873768dfdfec82cc018f54dec09c42293491aba043650801c09fe path: config/patches/node-pty@1.1.0.patch importers: @@ -69,7 +69,7 @@ importers: version: 3.3.1 node-pty: specifier: ^1.1.0 - version: 1.1.0(patch_hash=a8de16410ba90ef37d6486bebf03aaebcf2963514fbf1f5bd81f56eea7d96939) + version: 1.1.0(patch_hash=c48841645e1873768dfdfec82cc018f54dec09c42293491aba043650801c09fe) posthog-node: specifier: ^5.33.3 version: 5.33.3 @@ -12046,7 +12046,7 @@ snapshots: node-int64@0.4.0: {} - node-pty@1.1.0(patch_hash=a8de16410ba90ef37d6486bebf03aaebcf2963514fbf1f5bd81f56eea7d96939): + node-pty@1.1.0(patch_hash=c48841645e1873768dfdfec82cc018f54dec09c42293491aba043650801c09fe): dependencies: node-addon-api: 7.1.1 diff --git a/src/main/windows/windows-process-table.ts b/src/main/windows/windows-process-table.ts index e6d194b3e02..1b437a0316a 100644 --- a/src/main/windows/windows-process-table.ts +++ b/src/main/windows/windows-process-table.ts @@ -24,6 +24,11 @@ import { createProcessTableSnapshotReader } from '../../shared/process-table-sna * PowerShell CIM 706 / 723 ms */ +/** Which of the per-process lookups a caller actually needs. */ +export type WindowsProcessFields = { memory?: boolean; commandLine?: boolean } + +const ALL_FIELDS: WindowsProcessFields = { memory: true, commandLine: true } + export type WindowsProcessRow = { pid: number ppid: number @@ -91,7 +96,7 @@ function loadWindowsProcessTree(): WindowsProcessTreeModule | null { */ const WINDOWS_PROCESS_QUERY_TIMEOUT_MS = 3_000 -function readNativeRows(): Promise { +function readNativeRows(fields: WindowsProcessFields): Promise { const native = moduleLoader() if (!native) { // Reject rather than resolve empty: an empty table is a claim that nothing @@ -99,7 +104,13 @@ function readNativeRows(): Promise { // tree dead. "Unavailable" has to stay distinguishable from "empty". return Promise.reject(new Error('windows process table unavailable')) } - const flags = native.ProcessDataFlag.Memory | native.ProcessDataFlag.CommandLine + // Why ask for the minimum: both extra flags do an OpenProcess per process -- + // Memory adds GetProcessMemoryInfo, CommandLine adds a PEB read -- inline, + // for every process on the box, and the 1024-entry bound is patched out. + // Measured at 1050 processes: 15.9ms p50 for pid/ppid/name, 30.6ms with both. + const flags = + (fields.memory ? native.ProcessDataFlag.Memory : 0) | + (fields.commandLine ? native.ProcessDataFlag.CommandLine : 0) return new Promise((resolve, reject) => { try { const deadline = setTimeout( @@ -143,8 +154,12 @@ function readNativeRows(): Promise { // Why still cache: the snapshot is cheap but not free, and a worktree delete // tears down PTYs 32-wide. The shared TTL + single-in-flight reader collapses // that burst into one scan, exactly as the PowerShell path had to. +// Why one reader and not one per field set: the cache exists so a 32-wide +// worktree delete collapses into a single scan, and splitting it per caller +// would restore the fan-out it prevents. The shared snapshot therefore carries +// every field; the narrow read below is for callers that bypass the cache. const snapshotReader = createProcessTableSnapshotReader({ - runPs: readNativeRows, + runPs: () => readNativeRows(ALL_FIELDS), now: () => Date.now() }) @@ -163,6 +178,16 @@ export function readWindowsProcessTableFresh(): Promise { return snapshotReader.getFreshSnapshot() } +/** + * An uncached snapshot carrying only pid, ppid and name. + * + * For teardown identity, which needs ancestry and nothing else: skipping the + * two per-process OpenProcess lookups roughly halves the scan. + */ +export function readWindowsProcessAncestry(): Promise { + return readNativeRows({}) +} + /** Whether the native table can be read at all on this host. */ export function isWindowsProcessTableAvailable(): boolean { return moduleLoader() !== null diff --git a/src/main/windows/windows-pty-job.win32.test.ts b/src/main/windows/windows-pty-job.win32.test.ts index da87143082f..08db5683377 100644 --- a/src/main/windows/windows-pty-job.win32.test.ts +++ b/src/main/windows/windows-pty-job.win32.test.ts @@ -133,11 +133,14 @@ describeOnWindows('ConPTY job ownership', () => { } }, 60_000) - it('still lets a child break away from the job', async () => { - // A job with no limits denies CREATE_BREAKAWAY_FROM_JOB outright, and - // installers, msiexec and some updater paths spawn that way deliberately. - // They would fail ONLY inside an Orca terminal, which is the worst shape a - // bug report can take, so JOB_OBJECT_LIMIT_BREAKAWAY_OK is load-bearing. + it('still starts a backgrounded child from inside the job', async () => { + // Scope note: `start /b` uses CREATE_NEW_CONSOLE, not + // CREATE_BREAKAWAY_FROM_JOB, so this does NOT prove the BREAKAWAY_OK flag + // is doing its job -- it proves job membership does not block ordinary + // backgrounding. The flag itself rests on the Win32 contract (a job lacking + // JOB_OBJECT_LIMIT_BREAKAWAY_OK denies breakaway with ERROR_ACCESS_DENIED + // regardless of its other limits) and is not covered by a test here. + // Covering it needs a helper that passes the flag to CreateProcess. const nodePty = await import('node-pty') const marker = join(mkdtempSync(join(tmpdir(), 'orca-breakaway-')), 'marker.txt') const proc = nodePty.spawn('cmd.exe', [], {