From 4c38cabb4ea53abfb17d15f5f0347104dc4d412a Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Sat, 26 Sep 2026 15:11:17 -0700 Subject: [PATCH] fix(windows): synchronize native terminal table lifetime (#23257) * fix(i18n): restore diff note draft catalog entries * fix(windows): synchronize native terminal table lifetime * fix(ci): preserve focused Playwright file selection * test(windows): retain PTY stress failure evidence * test: isolate hourly identity inputs from release version changes * test(ci): require focused commands on every platform Assert each golden platform step and its focused arguments before deduplicating Playwright discovery probes. --------- Co-authored-by: m4air Co-authored-by: Neil --- config/patches/node-pty@1.1.0.patch | 170 ++++++++++++------ config/scripts/windows-pty-table-stress.cjs | 156 ++++++++++++++++ pnpm-lock.yaml | 6 +- ...-pty-self-exit-pseudoconsole-close.test.ts | 4 +- .../windows/windows-msys-job.win32.test.ts | 15 +- .../windows/windows-pty-job.win32.test.ts | 16 ++ 6 files changed, 307 insertions(+), 60 deletions(-) create mode 100644 config/scripts/windows-pty-table-stress.cjs diff --git a/config/patches/node-pty@1.1.0.patch b/config/patches/node-pty@1.1.0.patch index d36bf36b63f..40b240f3296 100644 --- a/config/patches/node-pty@1.1.0.patch +++ b/config/patches/node-pty@1.1.0.patch @@ -707,7 +707,7 @@ index 98733dc0cd752b554bd94e45904ca341ad141bba..3dd5ad9b3124dbd5ba7f76679b26650d // Stop processing immediately on unexpected error and log this._writeQueue.length = 0; diff --git a/src/win/conpty.cc b/src/win/conpty.cc -index 7b286d3d644c26141df516929703aa6e129df4b2..4b06d18576c807c3d1181a7bd714140c6678cf86 100644 +index 7b286d3d64..5239ba4e40 100644 --- a/src/win/conpty.cc +++ b/src/win/conpty.cc @@ -18,6 +18,7 @@ @@ -718,7 +718,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4b06d18576c807c3d1181a7bd714140c #include #include #include -@@ -44,12 +45,40 @@ struct pty_baton { +@@ -44,15 +45,37 @@ struct pty_baton { HANDLE hOut; HPCON hpc; @@ -749,18 +749,25 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4b06d18576c807c3d1181a7bd714140c }; static std::vector> ptyHandles; -+// Orca: guards the job accessors below, and PtyKill, against the exit watcher -+// thread. It does NOT make the whole table safe -- PtyResize and PtyClear still -+// read it unlocked, as they always have -- but it closes the window this patch -+// opened, where the watcher can close hShell/hJob and free the baton between a -+// lookup and its use. -+// Handle VALUES are recycled aggressively, so an unguarded read could pass the -+// shell-pid check against an unrelated process and terminate the wrong job. ++// Orca: every table access shares the exit watcher's lock; handles can be recycled. +static std::mutex ptyJobMutex; static volatile LONG ptyCounter; - static pty_baton* get_pty_baton(int id) { -@@ -102,8 +131,31 @@ void SetupExitCallback(Napi::Env env, Napi::Function cb, pty_baton* baton) { +-static pty_baton* get_pty_baton(int id) { ++static pty_baton* get_pty_baton_locked(int id) { + auto it = std::find_if(ptyHandles.begin(), ptyHandles.end(), [id](const auto& ptyHandle) { + return ptyHandle->id == id; + }); +@@ -62,7 +85,7 @@ static pty_baton* get_pty_baton(int id) { + return nullptr; + } + +-static bool remove_pty_baton(int id) { ++static bool remove_pty_baton_locked(int id) { + auto it = std::remove_if(ptyHandles.begin(), ptyHandles.end(), [id](const auto& ptyHandle) { + return ptyHandle->id == id; + }); +@@ -102,8 +125,31 @@ void SetupExitCallback(Napi::Env env, Napi::Function cb, pty_baton* baton) { // Get process exit code. GetExitCodeProcess(baton->hShell, (LPDWORD)(&exit_event->exit_code)); // Clean up handles @@ -782,7 +789,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4b06d18576c807c3d1181a7bd714140c + // NDEBUG would compile the call away and leak every baton. + baton->shellExited = true; + if (baton->consoleClosed) { -+ const bool removed = remove_pty_baton(baton->id); ++ const bool removed = remove_pty_baton_locked(baton->id); + assert(removed); + (void)removed; + } @@ -794,7 +801,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4b06d18576c807c3d1181a7bd714140c auto status = tsfn.BlockingCall(exit_event, callback); // In main thread switch (status) { -@@ -242,6 +294,20 @@ +@@ -242,6 +288,20 @@ HRESULT CreateNamedPipesAndPseudoConsole(const Napi::CallbackInfo& info, return HRESULT_FROM_WIN32(GetLastError()); } @@ -815,15 +822,40 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4b06d18576c807c3d1181a7bd714140c static Napi::Value PtyStartProcess(const Napi::CallbackInfo& info) { Napi::Env env(info.Env()); Napi::HandleScope scope(env); -@@ -303,6 +369,7 @@ +@@ -301,8 +361,10 @@ static Napi::Value PtyStartProcess(const Napi::CallbackInfo& info) { + // We were able to instantiate a conpty + const int ptyId = InterlockedIncrement(&ptyCounter); marshal.Set("pty", Napi::Number::New(env, ptyId)); - ptyHandles.emplace_back( - std::make_unique(ptyId, hIn, hOut, hpc)); -+ ptyHandles.back()->allowJobBreakaway = !usesCygwinRuntime(shellpath); +- ptyHandles.emplace_back( +- std::make_unique(ptyId, hIn, hOut, hpc)); ++ auto baton = std::make_unique(ptyId, hIn, hOut, hpc); ++ baton->allowJobBreakaway = !usesCygwinRuntime(shellpath); ++ std::lock_guard guard(ptyJobMutex); ++ ptyHandles.emplace_back(std::move(baton)); } else { throw Napi::Error::New(env, "Cannot launch conpty"); } -@@ -409,6 +476,15 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { +@@ -350,11 +412,15 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { + const bool useConptyDll = info[4].As().Value(); + Napi::Function exitCallback = info[5].As(); + +- // Fetch pty handle from ID and start process +- pty_baton* handle = get_pty_baton(id); +- if (!handle) { +- throw Napi::Error::New(env, "Invalid pty handle"); ++ pty_baton* handle; ++ { ++ std::lock_guard guard(ptyJobMutex); ++ handle = get_pty_baton_locked(id); ++ if (!handle || handle->consoleClosed) { ++ throw Napi::Error::New(env, "Invalid pty handle"); ++ } + } ++ // No watcher exists for this baton until SetupExitCallback below; pipe waits stay unlocked. + + // Prepare command line + std::unique_ptr mutableCommandline = std::make_unique(cmdline.length() + 1); +@@ -409,6 +475,15 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { throw errorWithCode(info, "UpdateProcThreadAttribute failed"); } @@ -839,7 +871,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4b06d18576c807c3d1181a7bd714140c PROCESS_INFORMATION piClient{}; fSuccess = !!CreateProcessW( nullptr, -@@ -416,7 +492,10 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { +@@ -416,7 +491,10 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { nullptr, // lpProcessAttributes nullptr, // lpThreadAttributes false, // bInheritHandles VERY IMPORTANT that this is false @@ -851,7 +883,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4b06d18576c807c3d1181a7bd714140c envArg, // lpEnvironment mutableCwd.get(), // lpCurrentDirectory &siEx.StartupInfo, // lpStartupInfo -@@ -426,8 +505,48 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { +@@ -426,8 +504,48 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { throw errorWithCode(info, "Cannot create process"); } @@ -902,25 +934,52 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4b06d18576c807c3d1181a7bd714140c if (useConptyDll && fLoadedDll) { PFNRELEASEPSEUDOCONSOLE const pfnReleasePseudoConsole = (PFNRELEASEPSEUDOCONSOLE)GetProcAddress( -@@ -440,6 +559,8 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { +@@ -438,8 +556,12 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { + } + } - // Update handle - handle->hShell = piClient.hProcess; -+ handle->shellPid = piClient.dwProcessId; -+ handle->hJob = hJob; +- // Update handle +- handle->hShell = piClient.hProcess; ++ { ++ std::lock_guard guard(ptyJobMutex); ++ handle->hShell = piClient.hProcess; ++ handle->shellPid = piClient.dwProcessId; ++ handle->hJob = hJob; ++ } // Close the thread handle to avoid resource leak CloseHandle(piClient.hThread); -@@ -544,27 +665,213 @@ static Napi::Value PtyKill(const Napi::CallbackInfo& info) { +@@ -472,9 +594,10 @@ static Napi::Value PtyResize(const Napi::CallbackInfo& info) { + SHORT rows = static_cast(info[2].As().Uint32Value()); + const bool useConptyDll = info[3].As().Value(); + +- const pty_baton* handle = get_pty_baton(id); ++ std::lock_guard guard(ptyJobMutex); ++ const pty_baton* handle = get_pty_baton_locked(id); + +- if (handle != nullptr) { ++ if (handle != nullptr && !handle->consoleClosed) { + HANDLE hLibrary = LoadConptyDll(info, useConptyDll); + bool fLoadedDll = hLibrary != nullptr; + if (fLoadedDll) +@@ -513,9 +636,10 @@ static Napi::Value PtyClear(const Napi::CallbackInfo& info) { + return env.Undefined(); + } + +- const pty_baton* handle = get_pty_baton(id); ++ std::lock_guard guard(ptyJobMutex); ++ const pty_baton* handle = get_pty_baton_locked(id); + +- if (handle != nullptr) { ++ if (handle != nullptr && !handle->consoleClosed) { + HANDLE hLibrary = LoadConptyDll(info, useConptyDll); + bool fLoadedDll = hLibrary != nullptr; + if (fLoadedDll) +@@ -544,29 +668,215 @@ static Napi::Value PtyKill(const Napi::CallbackInfo& info) { int id = info[0].As().Int32Value(); const bool useConptyDll = info[1].As().Value(); - const pty_baton* handle = get_pty_baton(id); -- -- if (handle != nullptr) { -- HANDLE hLibrary = LoadConptyDll(info, useConptyDll); -- bool fLoadedDll = hLibrary != nullptr; -- if (fLoadedDll) + // Orca: resolve the DLL BEFORE touching any baton state, for the same reason + // PtyConnect does it before creating anything. LoadConptyDll throws when + // conpty.dll is missing, and a throw after consoleClosed was set would strand @@ -934,7 +993,18 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4b06d18576c807c3d1181a7bd714140c + (HMODULE)hLibrary, + useConptyDll ? "ConptyClosePseudoConsole" : "ClosePseudoConsole"); + } -+ + +- if (handle != nullptr) { +- HANDLE hLibrary = LoadConptyDll(info, useConptyDll); +- bool fLoadedDll = hLibrary != nullptr; +- if (fLoadedDll) +- { +- PFNCLOSEPSEUDOCONSOLE const pfnClosePseudoConsole = (PFNCLOSEPSEUDOCONSOLE)GetProcAddress( +- (HMODULE)hLibrary, +- useConptyDll ? "ConptyClosePseudoConsole" : "ClosePseudoConsole"); +- if (pfnClosePseudoConsole) +- { +- pfnClosePseudoConsole(handle->hpc); + // Orca: the baton now outlives the shell, so this runs on a self-exited pty + // too -- that is the whole point. Take what we need under the lock: the + // watcher thread nulls hShell the moment the shell dies, and TerminateProcess @@ -945,7 +1015,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4b06d18576c807c3d1181a7bd714140c + bool owed = false; + { + std::lock_guard guard(ptyJobMutex); -+ pty_baton* handle = get_pty_baton(id); ++ pty_baton* handle = get_pty_baton_locked(id); + // Why the consoleClosed check: a second kill() would otherwise close the + // same pseudoconsole twice. Upstream relied on the baton being gone. + if (handle != nullptr && !handle->consoleClosed) { @@ -965,9 +1035,9 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4b06d18576c807c3d1181a7bd714140c + hShellDup = nullptr; + TerminateProcess(handle->hShell, 1); + } -+ } + } + if (handle->shellExited) { -+ const bool removed = remove_pty_baton(id); ++ const bool removed = remove_pty_baton_locked(id); + assert(removed); + (void)removed; + } @@ -979,19 +1049,11 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4b06d18576c807c3d1181a7bd714140c + // drained, and the watcher must be able to take the lock while it does. + if (owed) { + if (pfnClosePseudoConsole) - { -- PFNCLOSEPSEUDOCONSOLE const pfnClosePseudoConsole = (PFNCLOSEPSEUDOCONSOLE)GetProcAddress( -- (HMODULE)hLibrary, -- useConptyDll ? "ConptyClosePseudoConsole" : "ClosePseudoConsole"); -- if (pfnClosePseudoConsole) -- { -- pfnClosePseudoConsole(handle->hpc); -- } -- } ++ { ++ pfnClosePseudoConsole(hpc); + } - if (useConptyDll) { - TerminateProcess(handle->hShell, 1); -+ pfnClosePseudoConsole(hpc); -+ } + if (hShellDup != nullptr) { + TerminateProcess(hShellDup, 1); + CloseHandle(hShellDup); @@ -999,8 +1061,8 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4b06d18576c807c3d1181a7bd714140c } return env.Undefined(); -+} -+ + } + +/** + * Orca: confirm a baton really is the pty the caller means. + * @@ -1032,7 +1094,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4b06d18576c807c3d1181a7bd714140c + // Held across the lookup AND the Win32 call: the watcher thread can otherwise + // close these handles and free the baton in between. + std::lock_guard guard(ptyJobMutex); -+ const pty_baton* handle = get_pty_baton(info[0].As().Int32Value()); ++ const pty_baton* handle = get_pty_baton_locked(info[0].As().Int32Value()); + if (!ownsShell(handle, info[1].As().Uint32Value())) { + return Napi::Boolean::New(env, false); + } @@ -1064,7 +1126,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4b06d18576c807c3d1181a7bd714140c + // Held across the lookup AND the Win32 call: the watcher thread can otherwise + // close these handles and free the baton in between. + std::lock_guard guard(ptyJobMutex); -+ const pty_baton* handle = get_pty_baton(info[0].As().Int32Value()); ++ const pty_baton* handle = get_pty_baton_locked(info[0].As().Int32Value()); + if (!ownsShell(handle, info[1].As().Uint32Value())) { + return env.Null(); + } @@ -1138,10 +1200,12 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4b06d18576c807c3d1181a7bd714140c + } + hHostJob = job; + return Napi::Boolean::New(env, true); - } - ++} ++ /** -@@ -577,6 +884,9 @@ Napi::Object init(Napi::Env env, Napi::Object exports) { + * Init + */ +@@ -577,6 +887,9 @@ Napi::Object init(Napi::Env env, Napi::Object exports) { exports.Set("resize", Napi::Function::New(env, PtyResize)); exports.Set("clear", Napi::Function::New(env, PtyClear)); exports.Set("kill", Napi::Function::New(env, PtyKill)); diff --git a/config/scripts/windows-pty-table-stress.cjs b/config/scripts/windows-pty-table-stress.cjs new file mode 100644 index 00000000000..b703e18d19c --- /dev/null +++ b/config/scripts/windows-pty-table-stress.cjs @@ -0,0 +1,156 @@ +'use strict' + +const assert = require('node:assert/strict') +const { createHash } = require('node:crypto') +const { readFileSync, writeSync } = require('node:fs') +const { dirname, resolve } = require('node:path') + +function report(phase, details = {}) { + writeSync(1, `${JSON.stringify({ phase, hostPid: process.pid, ...details })}\n`) +} + +async function exerciseTable() { + assert.equal(process.platform, 'win32', 'This probe requires real Windows ConPTY') + const rounds = Number(process.env.ORCA_PTY_TABLE_STRESS_ROUNDS ?? 8) + assert.ok(Number.isInteger(rounds) && rounds > 0 && rounds <= 2000) + const pty = require('node-pty') + const nativePath = require.resolve('node-pty/lib/utils') + const loaded = require(nativePath).loadNativeModule('conpty') + const native = loaded.module + const addonPath = resolve(dirname(nativePath), loaded.dir, 'conpty.node') + report('native', { + addonPath, + sha256: createHash('sha256').update(readFileSync(addonPath)).digest('hex'), + node: process.version, + rounds + }) + // Unlike production's fallback, this crash probe requires a host that permits nested jobs. + const hostJobAssigned = native.assignCurrentProcessToJob() + report('host-job-precondition', { assigned: hostJobAssigned }) + assert.equal(hostJobAssigned, true, 'Probe precondition: host must permit a crash-cleanup job') + + const spawned = [] + function spawn(round, slot) { + report('spawn', { round, slot }) + const proc = pty.spawn(process.env.ComSpec || 'cmd.exe', ['/d', '/q'], { + cwd: process.cwd(), + cols: 80, + rows: 24, + useConptyDll: true + }) + const record = { proc, output: '', exited: false, closed: false } + const marker = `ORCA_PTY_READY_${spawned.length}` + let resolveReady + record.ready = new Promise((resolve) => { + resolveReady = resolve + }) + record.exit = new Promise((resolveExit) => { + proc.onExit((event) => { + record.exited = true + resolveReady(false) + resolveExit(event) + }) + }) + proc.onData((chunk) => { + record.output = (record.output + chunk).slice(-2048) + if (record.output.includes(marker)) { + resolveReady(true) + } + }) + spawned.push(record) + // Escaping one letter keeps echoed input from satisfying the output marker. + proc.write(`echo ${marker.replace('READY', 'REA^DY')}\r`) + return record + } + + async function waitForReady(records) { + let timer + try { + await Promise.race([ + Promise.all( + records.map(async (record) => { + assert.equal(await record.ready, true, `Shell exited before ready: ${record.output}`) + }) + ), + new Promise((_, reject) => { + timer = setTimeout(() => { + const transcripts = records.map(({ proc, output }) => ({ pid: proc.pid, output })) + reject(new Error(`PTY readiness timed out: ${JSON.stringify(transcripts)}`)) + }, 15_000) + }) + ]) + } finally { + clearTimeout(timer) + } + for (const { proc } of records) { + const members = native.listJobProcessIds(proc._pty, proc.pid) + assert.ok(members?.includes(proc.pid), `Ready shell ${proc.pid} must retain its job`) + } + report('ready', { shellPids: records.map(({ proc }) => proc.pid) }) + } + + function close(record, round, slot) { + report('kill', { round, slot, shellPid: record.proc.pid }) + record.proc.kill() + record.closed = true + } + + let failure + try { + const survivor = spawn(-1, -1) + await waitForReady([survivor]) + const slots = [] + for (let round = 0; round < rounds; round += 1) { + for (let slot = 0; slot < 3; slot += 1) { + if (slots[slot]) { + close(slots[slot], round, slot) + } + // The next lookup/insertion overlaps the previous shell's native exit watcher. + slots[slot] = spawn(round, slot) + report('resize-clear-list', { round, slot, shellPid: survivor.proc.pid }) + survivor.proc.resize(80 + (round % 2), 24) + survivor.proc.clear() + const members = native.listJobProcessIds(survivor.proc._pty, survivor.proc.pid) + assert.ok(members?.includes(survivor.proc.pid), 'A live survivor must retain its job') + } + // A ready terminal runs kill/resize/clear immediately instead of deferring them. + await waitForReady(slots) + } + assert.equal(survivor.exited, false, survivor.output) + report('overlap-complete', { terminals: spawned.length }) + } catch (error) { + failure = { error } + } finally { + for (const record of spawned) { + if (!record.closed) { + close(record, -1, -1) + } + } + let timer + try { + await Promise.race([ + Promise.all(spawned.map((record) => record.exit)), + new Promise((_, reject) => { + timer = setTimeout(() => reject(new Error('PTY exit callbacks did not drain')), 15_000) + }) + ]) + } catch (error) { + if (failure) { + report('drain-error', { message: error.stack }) + } else { + failure = { error } + } + } finally { + clearTimeout(timer) + } + } + if (failure) { + throw failure.error + } + report('complete', { terminals: spawned.length }) +} + +exerciseTable().catch((error) => { + report('error', { message: error.stack }) + process.exitCode = 1 +}) diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index a41ac44581e..c05265397b9 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -117,7 +117,7 @@ patchedDependencies: '@xterm/addon-webgl@0.20.0-beta.299': 94687e89a0115e6e6aa102837f986debdc029c091527ee5eb4a4e17ceaf9473e '@xterm/xterm@6.1.0-beta.303': dd0ccc59cd1ccf99f4d76e5aa2456da165fa0804dce19a833d7638bd07ffa393 lint-staged@16.4.0: 7333b3837f80a7fbd045964db6d76ba4fc118e49134bdbabb00585b6b7b60673 - node-pty@1.1.0: 346cb29d33dd6eeb14910ff411c7584b0b2ff9a4b271c4b6d23c3b48e6548f74 + node-pty@1.1.0: 92c95cffab383d86b3b13a460c75a08074468ebf1f3911db8e874e083201f192 importers: @@ -161,7 +161,7 @@ importers: version: 3.3.1 node-pty: specifier: ^1.1.0 - version: 1.1.0(patch_hash=346cb29d33dd6eeb14910ff411c7584b0b2ff9a4b271c4b6d23c3b48e6548f74) + version: 1.1.0(patch_hash=92c95cffab383d86b3b13a460c75a08074468ebf1f3911db8e874e083201f192) posthog-node: specifier: ^5.52.5 version: 5.52.5 @@ -13098,7 +13098,7 @@ snapshots: node-int64@0.4.0: {} - node-pty@1.1.0(patch_hash=346cb29d33dd6eeb14910ff411c7584b0b2ff9a4b271c4b6d23c3b48e6548f74): + node-pty@1.1.0(patch_hash=92c95cffab383d86b3b13a460c75a08074468ebf1f3911db8e874e083201f192): dependencies: node-addon-api: 7.1.1 diff --git a/src/main/pty/node-pty-self-exit-pseudoconsole-close.test.ts b/src/main/pty/node-pty-self-exit-pseudoconsole-close.test.ts index 1f9cae275c3..6d8b6411ba5 100644 --- a/src/main/pty/node-pty-self-exit-pseudoconsole-close.test.ts +++ b/src/main/pty/node-pty-self-exit-pseudoconsole-close.test.ts @@ -84,7 +84,7 @@ describe('node-pty patch: pseudoconsole close on the self-exit path', () => { [ '+ baton->shellExited = true;', '+ if (baton->consoleClosed) {', - '+ const bool removed = remove_pty_baton(baton->id);', + '+ const bool removed = remove_pty_baton_locked(baton->id);', '+ assert(removed);', '+ (void)removed;', '+ }' @@ -138,7 +138,7 @@ describe('node-pty patch: pseudoconsole close on the self-exit path', () => { expect(ptyKillHunk).toContain( [ '+ if (handle->shellExited) {', - '+ const bool removed = remove_pty_baton(id);', + '+ const bool removed = remove_pty_baton_locked(id);', '+ assert(removed);', '+ (void)removed;' ].join('\n') diff --git a/src/main/windows/windows-msys-job.win32.test.ts b/src/main/windows/windows-msys-job.win32.test.ts index e7e0bee950a..9bf5485102e 100644 --- a/src/main/windows/windows-msys-job.win32.test.ts +++ b/src/main/windows/windows-msys-job.win32.test.ts @@ -37,8 +37,12 @@ describeOnWindows('MSYS terminal job ownership', () => { }) let output = '' let childPid: number | undefined + let exit: { exitCode: number; signal?: number } | undefined + proc.onExit((event) => { + exit = event + }) proc.onData((chunk) => { - output += chunk + output = (output + chunk).slice(-32_768) const match = /MSYS_OWNED_CHILD=(\d+)/.exec(output) if (match) { childPid = Number(match[1]) @@ -48,7 +52,14 @@ describeOnWindows('MSYS terminal job ownership', () => { proc.write( `${quotePosixShell(process.execPath.replace(/\\/g, '/'))} ${quotePosixShell(script.replace(/\\/g, '/'))}\r` ) - await vi.waitFor(() => expect(childPid).toBeDefined(), { timeout: 15_000 }) + try { + await vi.waitFor(() => expect(childPid).toBeDefined(), { timeout: 15_000 }) + } catch (cause) { + throw new Error( + JSON.stringify({ shellPid: proc.pid, exit, jobPids: listPtyJobProcessIds(proc), output }), + { cause } + ) + } expect(isAlive(childPid!)).toBe(true) expect(listPtyJobProcessIds(proc)).toContain(childPid) expect(terminatePtyJob(proc)).toBe('terminated') diff --git a/src/main/windows/windows-pty-job.win32.test.ts b/src/main/windows/windows-pty-job.win32.test.ts index c5f408f7311..cb15463bff8 100644 --- a/src/main/windows/windows-pty-job.win32.test.ts +++ b/src/main/windows/windows-pty-job.win32.test.ts @@ -3,6 +3,7 @@ import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, describe, expect, it, vi } from 'vitest' import type { IPty } from 'node-pty' +import { runProcess } from '../../shared/child-process/run-process' import { isPtyJobOwnershipAvailable, listPtyJobProcessIds, @@ -102,6 +103,21 @@ describeOnWindows('ConPTY job ownership', () => { expect(isPtyJobOwnershipAvailable()).toBe(true) }) + it('keeps the native table intact while shell cleanup overlaps new terminals', async () => { + const result = await runProcess({ + program: process.execPath, + args: [join(process.cwd(), 'config', 'scripts', 'windows-pty-table-stress.cjs')], + env: { ...process.env, ORCA_BACKGROUND_LAUNCH: '1' }, + timeoutMs: 90_000 + }) + const status = result.code === null ? 'null' : `0x${(result.code >>> 0).toString(16)}` + expect( + result, + `Native host exited ${status} (${result.signal}); timedOut=${result.timedOut}\n${result.stdout}\n${result.stderr}` + ).toMatchObject({ code: 0, timedOut: false }) + expect(result.stdout).toContain('"phase":"complete"') + }, 100_000) + it('counts a detached grandchild as part of the pane tree', async () => { const { proc, grandchildPid } = await spawnShellWithDetachedGrandchild()