From 314506003a16297006225147fef8bdcec2186da8 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 7 Sep 2026 00:35:55 -0700 Subject: [PATCH 1/2] fix: retain MSYS shell descendants in their terminal job (#19068) * fix: retain MSYS shell descendants in their terminal job * test: complete MSYS regression CI registration and teardown contract * fix(windows): deny job breakaway for the whole Cygwin/MSYS shell family The per-PTY job probed only msys-2.0.dll, and only for bash.exe/sh.exe. Cygwin ships the same spawn.cc breakaway logic under cygwin1.dll, and an MSYS2 zsh escapes exactly like its bash does, so both kept the orphan bug. Probe the runtime DLL on the shell's own search path instead of matching shell names: that is the property that decides whether the runtime will ask for CREATE_BREAKAWAY_FROM_JOB, and it drops the name special-casing. * chore(patch): restore the conpty.cc index line The earlier hand-edit dropped it while every sibling section kept one. Recomputed against the real blobs: applying this patch to 7b286d3d yields exactly 4b06d185, so git apply -3 has its fallback back. --- .github/workflows/pr.yml | 1 + config/patches/node-pty@1.1.0.patch | 107 +++++++++++------- config/scripts/pr-code-change-scope.mjs | 1 + docs/reference/windows-process-enumeration.md | 14 +++ pnpm-lock.yaml | 6 +- .../windows/windows-msys-job.win32.test.ts | 65 +++++++++++ 6 files changed, 153 insertions(+), 41 deletions(-) create mode 100644 src/main/windows/windows-msys-job.win32.test.ts diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 9279f35b39f..268b6ad66e3 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -856,6 +856,7 @@ jobs: src/main/agent-hooks/windows-hook-payload-delivery.test.ts src/main/agent-hooks/windows-direct-cmd-hook-command.test.ts src/main/windows/windows-pty-job.win32.test.ts + src/main/windows/windows-msys-job.win32.test.ts src/main/windows/windows-host-job.win32.test.ts src/main/windows/windows-process-tree-command-line-patch.test.ts src/main/windows/windows-process-table-native-addon.win32.test.ts diff --git a/config/patches/node-pty@1.1.0.patch b/config/patches/node-pty@1.1.0.patch index 8f5045b932a..961e750da6b 100644 --- a/config/patches/node-pty@1.1.0.patch +++ b/config/patches/node-pty@1.1.0.patch @@ -603,7 +603,7 @@ index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..2ae787c5bd4f3eba470584dc658a01a5 } #endif diff --git a/src/win/conpty.cc b/src/win/conpty.cc -index 7b286d3d644c26141df516929703aa6e129df4b2..4aed260dd68e6a171dcfd349e9a7c5c97209248e 100644 +index 7b286d3d644c26141df516929703aa6e129df4b2..4b06d18576c807c3d1181a7bd714140c6678cf86 100644 --- a/src/win/conpty.cc +++ b/src/win/conpty.cc @@ -18,6 +18,7 @@ @@ -614,7 +614,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4aed260dd68e6a171dcfd349e9a7c5c9 #include #include #include -@@ -44,12 +45,39 @@ struct pty_baton { +@@ -44,12 +45,40 @@ struct pty_baton { HANDLE hOut; HPCON hpc; @@ -630,6 +630,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4aed260dd68e6a171dcfd349e9a7c5c9 + // 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; ++ bool allowJobBreakaway = true; + + // Orca: teardown needs BOTH the shell's death and an explicit kill() before + // the baton can be freed, so each side records that it has run. Whichever @@ -655,7 +656,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4aed260dd68e6a171dcfd349e9a7c5c9 static volatile LONG ptyCounter; static pty_baton* get_pty_baton(int id) { -@@ -102,8 +130,31 @@ void SetupExitCallback(Napi::Env env, Napi::Function cb, pty_baton* baton) { +@@ -102,8 +131,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 @@ -689,7 +690,36 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4aed260dd68e6a171dcfd349e9a7c5c9 auto status = tsfn.BlockingCall(exit_event, callback); // In main thread switch (status) { -@@ -409,6 +460,15 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { +@@ -242,6 +294,20 @@ + return HRESULT_FROM_WIN32(GetLastError()); + } + ++// Cygwin and MSYS request breakaway for every child whenever the job allows it, ++// so their shells need one that does not. The runtime DLL on the exe's search ++// path is the signal; Git for Windows ships bash.exe in bin\ beside usr\bin\. ++static bool usesCygwinRuntime(const std::wstring& shellpath) { ++ const size_t separator = shellpath.find_last_of(L"\\/"); ++ if (separator == std::wstring::npos) return false; ++ const std::wstring directory = shellpath.substr(0, separator + 1); ++ for (const wchar_t* dll : {L"msys-2.0.dll", L"cygwin1.dll"}) { ++ if (path_util::file_exists(directory + dll) || ++ path_util::file_exists(directory + L"..\\usr\\bin\\" + dll)) return true; ++ } ++ return false; ++} ++ + static Napi::Value PtyStartProcess(const Napi::CallbackInfo& info) { + Napi::Env env(info.Env()); + Napi::HandleScope scope(env); +@@ -303,6 +369,7 @@ + marshal.Set("pty", Napi::Number::New(env, ptyId)); + ptyHandles.emplace_back( + std::make_unique(ptyId, hIn, hOut, hpc)); ++ ptyHandles.back()->allowJobBreakaway = !usesCygwinRuntime(shellpath); + } else { + throw Napi::Error::New(env, "Cannot launch conpty"); + } +@@ -409,6 +476,15 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { throw errorWithCode(info, "UpdateProcThreadAttribute failed"); } @@ -705,7 +735,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4aed260dd68e6a171dcfd349e9a7c5c9 PROCESS_INFORMATION piClient{}; fSuccess = !!CreateProcessW( nullptr, -@@ -416,7 +476,10 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { +@@ -416,7 +492,10 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { nullptr, // lpProcessAttributes nullptr, // lpThreadAttributes false, // bInheritHandles VERY IMPORTANT that this is false @@ -717,7 +747,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4aed260dd68e6a171dcfd349e9a7c5c9 envArg, // lpEnvironment mutableCwd.get(), // lpCurrentDirectory &siEx.StartupInfo, // lpStartupInfo -@@ -426,8 +489,47 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { +@@ -426,8 +505,48 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { throw errorWithCode(info, "Cannot create process"); } @@ -735,13 +765,14 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4aed260dd68e6a171dcfd349e9a7c5c9 + // EXPLICIT teardown exact, not to redefine what a clean exit means. + HANDLE hJob = CreateJobObjectW(nullptr, nullptr); + if (hJob != nullptr) { -+ // Why BREAKAWAY_OK and not a bare job: with no limits set, a child asking -+ // for CREATE_BREAKAWAY_FROM_JOB is refused with ERROR_ACCESS_DENIED. -+ // Installers, msiexec and some updater and service-control paths spawn that -+ // way deliberately, so a bare job breaks them ONLY inside an Orca terminal. -+ // With this flag a child has to ask, so ordinary descendants stay owned. ++ // Native shells retain explicit breakaway for installers and updaters. ++ // Cygwin/MSYS shells take it automatically for ordinary children whenever ++ // this flag is present, so they get strict per-PTY membership instead. ++ // Explicit breakaway requests inside such a pane are consequently denied; ++ // ordinary backgrounding and clean shell exit remain supported. + JOBOBJECT_EXTENDED_LIMIT_INFORMATION jobLimits{}; -+ jobLimits.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_BREAKAWAY_OK; ++ jobLimits.BasicLimitInformation.LimitFlags = ++ handle->allowJobBreakaway ? JOB_OBJECT_LIMIT_BREAKAWAY_OK : 0; + if (!SetInformationJobObject(hJob, JobObjectExtendedLimitInformation, &jobLimits, sizeof(jobLimits)) || + !AssignProcessToJobObject(hJob, piClient.hProcess)) { + // Why tolerate failure: an outer job without JOB_OBJECT_LIMIT_BREAKAWAY_OK @@ -767,7 +798,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4aed260dd68e6a171dcfd349e9a7c5c9 if (useConptyDll && fLoadedDll) { PFNRELEASEPSEUDOCONSOLE const pfnReleasePseudoConsole = (PFNRELEASEPSEUDOCONSOLE)GetProcAddress( -@@ -440,6 +542,8 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { +@@ -440,6 +559,8 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { // Update handle handle->hShell = piClient.hProcess; @@ -776,11 +807,16 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4aed260dd68e6a171dcfd349e9a7c5c9 // Close the thread handle to avoid resource leak CloseHandle(piClient.hThread); -@@ -544,29 +648,215 @@ static Napi::Value PtyKill(const Napi::CallbackInfo& info) { +@@ -544,27 +665,213 @@ 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 @@ -794,18 +830,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4aed260dd68e6a171dcfd349e9a7c5c9 + (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 @@ -841,18 +866,26 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4aed260dd68e6a171dcfd349e9a7c5c9 + const bool removed = remove_pty_baton(id); + assert(removed); + (void)removed; - } ++ } + // Else the shell is still running and the watcher frees the baton. - } -- if (useConptyDll) { -- TerminateProcess(handle->hShell, 1); ++ } + } + + // Why outside the lock: ClosePseudoConsole blocks until the conout side has + // drained, and the watcher must be able to take the lock while it does. + if (owed) { + if (pfnClosePseudoConsole) -+ { + { +- PFNCLOSEPSEUDOCONSOLE const pfnClosePseudoConsole = (PFNCLOSEPSEUDOCONSOLE)GetProcAddress( +- (HMODULE)hLibrary, +- useConptyDll ? "ConptyClosePseudoConsole" : "ClosePseudoConsole"); +- if (pfnClosePseudoConsole) +- { +- pfnClosePseudoConsole(handle->hpc); +- } +- } +- if (useConptyDll) { +- TerminateProcess(handle->hShell, 1); + pfnClosePseudoConsole(hpc); + } + if (hShellDup != nullptr) { @@ -862,8 +895,8 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4aed260dd68e6a171dcfd349e9a7c5c9 } return env.Undefined(); - } - ++} ++ +/** + * Orca: confirm a baton really is the pty the caller means. + * @@ -1001,12 +1034,10 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4aed260dd68e6a171dcfd349e9a7c5c9 + } + hHostJob = job; + return Napi::Boolean::New(env, true); -+} -+ + } + /** - * Init - */ -@@ -577,6 +867,9 @@ Napi::Object init(Napi::Env env, Napi::Object exports) { +@@ -577,6 +884,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/pr-code-change-scope.mjs b/config/scripts/pr-code-change-scope.mjs index befcb06fe1f..96917d23ef1 100644 --- a/config/scripts/pr-code-change-scope.mjs +++ b/config/scripts/pr-code-change-scope.mjs @@ -224,6 +224,7 @@ const WINDOWS_PACKAGE_TESTS = [ 'src/main/agent-hooks/windows-hook-payload-delivery.test.ts', 'src/main/agent-hooks/windows-direct-cmd-hook-command.test.ts', 'src/main/windows/windows-pty-job.win32.test.ts', + 'src/main/windows/windows-msys-job.win32.test.ts', 'src/main/windows/windows-host-job.win32.test.ts', 'src/main/windows/windows-process-tree-command-line-patch.test.ts', 'src/main/windows/windows-process-table-native-addon.win32.test.ts', diff --git a/docs/reference/windows-process-enumeration.md b/docs/reference/windows-process-enumeration.md index 0f7f17bd433..80cc7663f4c 100644 --- a/docs/reference/windows-process-enumeration.md +++ b/docs/reference/windows-process-enumeration.md @@ -575,6 +575,20 @@ running, so typing `exit` in a pane reaped a `start /b` server that used to survive. The job exists to make an _explicit_ teardown exact, not to redefine what a clean exit means. +Git Bash needs one additional restriction. The Cygwin runtime — and the MSYS2 +fork of it that Git for Windows ships — reads `JOB_OBJECT_LIMIT_BREAKAWAY_OK` +off its own job and then adds `CREATE_BREAKAWAY_FROM_JOB` to **every** child it +spawns when that flag is set (`spawn.cc`, there since 2011), so offering +breakaway hands the whole tree its escape. The per-PTY job therefore omits +`BREAKAWAY_OK` whenever `msys-2.0.dll` or `cygwin1.dll` sits on the shell's DLL +search path — beside the executable, or under `usr/bin` for Git's `bin` +launcher. Native shells keep explicit breakaway. Denying it costs Cygwin +nothing, because it *pre-checks* the limit rather than retrying, so no spawn +fails; but a *native* program that passes `CREATE_BREAKAWAY_FROM_JOB` itself +inside such a pane now gets `ERROR_ACCESS_DENIED`. `nohup` and `disown` are +unaffected — they are Cygwin signal/session concepts, unrelated to job +membership. The daemon's host job is unchanged. + Reaping a dead daemon's shells (#9195, #10415) is therefore a **second, nested job**, not this one. The terminal daemon assigns itself to a kill-on-close job at startup (`assignHostProcessToKillOnCloseJob`); children inherit membership, diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 103ed90f4fe..e4a40c0fe47 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -116,7 +116,7 @@ patchedDependencies: '@xterm/addon-webgl@0.20.0-beta.299': 94687e89a0115e6e6aa102837f986debdc029c091527ee5eb4a4e17ceaf9473e '@xterm/xterm@6.1.0-beta.303': 98756bcedc402bcdb7c6ab7b015d2e59cd18e97b03a2c06a27e95bb3ba429d9d lint-staged@16.4.0: 7333b3837f80a7fbd045964db6d76ba4fc118e49134bdbabb00585b6b7b60673 - node-pty@1.1.0: 7cc9d45f3d2c38f142490d0805e75db55f0eef5174ad41c4b52abc5fbe079ad1 + node-pty@1.1.0: bac3a53fb15efc9b3b944fbe3c4718b5174a0b3bd6ead84e21975edad4bc6615 importers: @@ -160,7 +160,7 @@ importers: version: 3.3.1 node-pty: specifier: ^1.1.0 - version: 1.1.0(patch_hash=7cc9d45f3d2c38f142490d0805e75db55f0eef5174ad41c4b52abc5fbe079ad1) + version: 1.1.0(patch_hash=bac3a53fb15efc9b3b944fbe3c4718b5174a0b3bd6ead84e21975edad4bc6615) posthog-node: specifier: ^5.33.3 version: 5.33.3 @@ -12285,7 +12285,7 @@ snapshots: node-int64@0.4.0: {} - node-pty@1.1.0(patch_hash=7cc9d45f3d2c38f142490d0805e75db55f0eef5174ad41c4b52abc5fbe079ad1): + node-pty@1.1.0(patch_hash=bac3a53fb15efc9b3b944fbe3c4718b5174a0b3bd6ead84e21975edad4bc6615): dependencies: node-addon-api: 7.1.1 diff --git a/src/main/windows/windows-msys-job.win32.test.ts b/src/main/windows/windows-msys-job.win32.test.ts new file mode 100644 index 00000000000..e7e0bee950a --- /dev/null +++ b/src/main/windows/windows-msys-job.win32.test.ts @@ -0,0 +1,65 @@ +import { mkdtempSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { describe, expect, it, vi } from 'vitest' +import { removeTreeSync } from '../../shared/windows-transient-lock-removal' +import { resolveGitBashPath } from '../git-bash' +import { quotePosixShell } from '../../shared/wsl-login-shell-command' +import { listPtyJobProcessIds, terminatePtyJob } from './windows-pty-job' + +const describeOnWindows = process.platform === 'win32' ? describe : describe.skip + +function isAlive(pid: number): boolean { + try { + process.kill(pid, 0) + return true + } catch (error) { + return (error as NodeJS.ErrnoException).code === 'EPERM' + } +} + +describeOnWindows('MSYS terminal job ownership', () => { + it('retains and terminates a child across Git Bash shell replacement', async () => { + const shell = resolveGitBashPath() + expect(shell, 'Git for Windows must be installed on the native test runner').not.toBeNull() + const directory = mkdtempSync(join(tmpdir(), 'orca-msys-job-')) + const script = join(directory, 'owned-child.js') + writeFileSync( + script, + "console.log('MSYS_OWNED_CHILD=' + process.pid); setInterval(() => {}, 1000)\n" + ) + const pty = await import('node-pty') + const proc = pty.spawn(shell!, ['-c', 'exec "$BASH" --noprofile --norc -i'], { + cwd: tmpdir(), + cols: 120, + rows: 30, + useConptyDll: true + }) + let output = '' + let childPid: number | undefined + proc.onData((chunk) => { + output += chunk + const match = /MSYS_OWNED_CHILD=(\d+)/.exec(output) + if (match) { + childPid = Number(match[1]) + } + }) + try { + proc.write( + `${quotePosixShell(process.execPath.replace(/\\/g, '/'))} ${quotePosixShell(script.replace(/\\/g, '/'))}\r` + ) + await vi.waitFor(() => expect(childPid).toBeDefined(), { timeout: 15_000 }) + expect(isAlive(childPid!)).toBe(true) + expect(listPtyJobProcessIds(proc)).toContain(childPid) + expect(terminatePtyJob(proc)).toBe('terminated') + await vi.waitFor(() => expect(isAlive(childPid!)).toBe(false), { timeout: 5_000 }) + } finally { + // The failing baseline can leave this exact fixture child outside the job. + if (childPid && isAlive(childPid)) { + process.kill(childPid) + } + proc.kill() + removeTreeSync(directory) + } + }, 30_000) +}) From ffff6eaca203ab1547ab532990881c4d556fff47 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 7 Sep 2026 01:01:49 -0700 Subject: [PATCH 2/2] fix(test): admit the adoption-replay create fixture through the structured gate (#19246) Semantic conflict between two green PRs. #19176 added this replay test while `agentSession.*` still admitted a `runtime` client on its negotiated capability alone; #18700 then made `experimentalStructuredNativeChat` one rule for every caller. Neither branch saw the other, and main runs no post-merge test gate, so `agentSession.create` started refusing at the envelope level and the test's `ok: true` expectation broke. #18700's rule is the intended behaviour and `create` starts work, so it belongs behind the gate. The fixture is what is stale: it builds a real `OrcaRuntimeService` whose client settings are unset. Enable the setting the way #18700 already did for the sibling pre-commit fixture. The assertions about durable-identity replay are untouched and now actually run. --- .../methods/structured-agent-session-adoption-replay.test.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/main/runtime/rpc/methods/structured-agent-session-adoption-replay.test.ts b/src/main/runtime/rpc/methods/structured-agent-session-adoption-replay.test.ts index 42fdbc772a0..d83043edd5a 100644 --- a/src/main/runtime/rpc/methods/structured-agent-session-adoption-replay.test.ts +++ b/src/main/runtime/rpc/methods/structured-agent-session-adoption-replay.test.ts @@ -138,6 +138,11 @@ describe('committed adopting create RPC replay', () => { undefined, { prepareCodexStructuredLaunch: selectAccountHome } ) + // The structured surface is settings-gated for every caller, not just mobile; this test + // probes durable-identity replay, which only runs once the gate admits the call. + vi.spyOn(runtime, 'getClientSettings').mockReturnValue({ + experimentalStructuredNativeChat: true + } as ReturnType) vi.spyOn(runtime, 'getStructuredAgentSessionCreateSupport').mockResolvedValue({ supported: true })