diff --git a/.gitignore b/.gitignore index 913dfc4a045..669ec4528f9 100644 --- a/.gitignore +++ b/.gitignore @@ -114,6 +114,7 @@ docs/** !docs/reference/windows-cmd-shim-resolution.md !docs/reference/windows-daemon-host-relocation.md !docs/reference/windows-edr-posture.md +!docs/reference/windows-msys-job-breakaway.md !docs/reference/windows-process-enumeration.md !docs/reference/wsl-runner-verification.md !docs/reference/remote-wire-compatibility.md diff --git a/AGENTS.md b/AGENTS.md index 5ff66b95b0f..4d93a3ab738 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -55,6 +55,7 @@ Orca targets macOS, Linux, and Windows. Keep all platform-dependent behavior beh - **Windows setup scripts**: the setup/issue-command runner is a `.cmd` batch file unless the script starts with a `#!` line — never derive that from the user's terminal-shell preference, and never launch a `.cmd` runner with a bare `cmd.exe /c` from a Git Bash pane (MSYS rewrites the `/c`). See [`docs/reference/windows-setup-shell.md`](./docs/reference/windows-setup-shell.md). - **Windows child processes**: start them through `runProcess`/`spawnProcess` in `src/shared/child-process/` — never `child_process` directly. It pins `windowsHide`, refuses `shell: true`, and encodes `.cmd`/`.bat` arguments so neither `CommandLineToArgvW` nor `cmd.exe` mangles them. A ratchet test fails on any new direct import. Recognised npm/pnpm `.cmd` shims are resolved to their real target so the spawn skips `cmd.exe` entirely; see [`docs/reference/windows-cmd-shim-resolution.md`](./docs/reference/windows-cmd-shim-resolution.md) before adding a shim shape or debugging one. - **Windows process enumeration**: read the table through `src/main/windows/windows-process-table.ts`, never by forking `powershell.exe`. See [`docs/reference/windows-process-enumeration.md`](./docs/reference/windows-process-enumeration.md). +- **Windows MSYS/Git Bash panes**: their children break away from the per-PTY job unless it is created without `JOB_OBJECT_LIMIT_BREAKAWAY_OK`, and a `conpty.node` built before that fix passes every existing gate. Before changing the per-PTY job or debugging `windows-msys-job.win32.test.ts`, read [`docs/reference/windows-msys-job-breakaway.md`](./docs/reference/windows-msys-job-breakaway.md). - **Windows daemon-host relocation**: the terminal daemon runs from a copy of the app runtime under `%LOCALAPPDATA%`, which is what survives an auto-update. Before touching that copy, its exe name, or the NSIS uninstall macro, read [`docs/reference/windows-daemon-host-relocation.md`](./docs/reference/windows-daemon-host-relocation.md). - **Windows EDR signal**: don't add `-ExecutionPolicy Bypass`, `-EncodedCommand`, `cmd.exe /c` with escaped free text, per-operation interpreter spawning, or runtime `Add-Type` compilation without reading [`docs/reference/windows-edr-posture.md`](./docs/reference/windows-edr-posture.md) first — behavioural EDR scores each of those, and being signed does not clear them. - **WSL commands**: build argv with `buildWslExecArgs` (always `--exec` — under `--`, `wsl.exe` expands `$name` in every argument and silently rewrites the script), and fence anything whose stdout you parse with `buildWslCapturedLoginShellCommand`, because the interactive login shell prints the distro banner to stdout. See [`docs/reference/wsl-command-execution.md`](./docs/reference/wsl-command-execution.md). diff --git a/docs/reference/windows-msys-job-breakaway.md b/docs/reference/windows-msys-job-breakaway.md new file mode 100644 index 00000000000..8b0261628c1 --- /dev/null +++ b/docs/reference/windows-msys-job-breakaway.md @@ -0,0 +1,100 @@ +# Why an MSYS pane's children escape the per-PTY job + +Every child started from a Git Bash / MSYS2 / Cygwin pane leaves the pane's job +object unless the job is created **without** `JOB_OBJECT_LIMIT_BREAKAWAY_OK`. +`terminatePtyJob` then reports `terminated` and leaves the child running — the +orphan that holds a worktree directory open. + +The denial is already in `config/patches/node-pty@1.1.0.patch` +(`usesCygwinRuntime`, added in #19068). This page records the measurement +behind it, because the failure mode it prevents is indistinguishable from a +stale native addon and the existing gates cannot tell the two apart. + +## The mechanism + +The MSYS/Cygwin runtime asks for `CREATE_BREAKAWAY_FROM_JOB` on the +`CreateProcessW` inside its `spawn`/`exec` path. A job that carries +`JOB_OBJECT_LIMIT_BREAKAWAY_OK` grants it, so the child is created outside the +job; a job without that limit denies it with `ERROR_ACCESS_DENIED`, and the +runtime retries without the flag rather than failing the spawn. `fork` is not +affected — forked Cygwin processes stay in the job either way. + +Measured on Windows 11 `10.0.26200.9168`, Git `2.55.0.windows.3`, +bash `5.3.15(1)-release`, node `v24.18.0`, `useConptyDll: true`, for +`node-pty.spawn('C:\Program Files\Git\bin\bash.exe', ['--noprofile','--norc','-i'])` +— `+J` / `-J` is membership of the per-PTY job, read with +`QueryInformationJobObject(JobObjectBasicProcessIdList)`: + +``` +bin\bash.exe +J ConPTY shell (assigned by node-pty) + └ ..\usr\bin\bash.exe +J launcher hand-off, plain CreateProcess + └ usr\bin\bash.exe +J Cygwin fork for the typed command + └ node.exe -J Cygwin exec -- ESCAPES HERE +``` + +`bin\bash.exe` is a 47 KB launcher, not an MSYS binary: `C:\Program Files\Git\bin` +holds only `bash.exe`, `git.exe` and `sh.exe`, with no `msys-2.0.dll`. Its +hand-off to `bin\..\usr\bin\bash.exe` is an ordinary `CreateProcess` and keeps +job membership. Only the MSYS runtime's own spawn breaks away. + +The shell-replacement shape (`bash -c 'exec "$BASH" --noprofile --norc -i'`) +loses membership one step earlier, at the `exec`, and everything below inherits +the loss: + +``` +bin\bash.exe +J + └ ..\usr\bin\bash.exe +J + └ usr\bin\bash.exe -J Cygwin exec -- ESCAPES HERE + └ usr\bin\bash -J + └ node.exe -J +``` + +Both shapes leak. The `exec` is not the cause; it only moves the escape earlier. + +## The A/B that pins it + +One source tree, one toolchain, one variable — `usesCygwinRuntime` forced to +`false` so the per-PTY job keeps `JOB_OBJECT_LIMIT_BREAKAWAY_OK`: + +| per-PTY job limit | `listPtyJobProcessIds` | child reaped by `terminatePtyJob` | runs | +| ---------------------- | ---------------------- | --------------------------------- | ---- | +| `BREAKAWAY_OK` set | 2 pids, child absent | no | 0/2 | +| `BREAKAWAY_OK` cleared | 5 pids, child present | yes | 4/4 | + +The job **is** the right boundary. With breakaway denied it holds the whole MSYS +tree, including the child that detached from the console, and one +`terminateJob` reaps all of it. No alternative tracking mechanism is needed. + +Denying breakaway did not break ordinary launches from the pane: `git`, +`cmd //c`, an absolute-path `node`, a `&`-backgrounded job with `disown`, and +`where.exe` all returned 0 with no `Access is denied`, identically to the +breakaway-allowed control. Untested: a **non-Cygwin** program that itself passes +`CREATE_BREAKAWAY_FROM_JOB` (installers, updaters) and therefore has no runtime +to retry for it. That needs a helper that calls `CreateProcess` with the flag; +`start /b` does not exercise it (it uses `CREATE_NEW_CONSOLE`). + +## A stale addon looks exactly like the bug + +`config/scripts/node-pty-job-ownership.cjs` asserts only that `terminateJob`, +`listJobProcessIds` and `assignCurrentProcessToJob` are exported. All three +predate #19068, so a `conpty.node` built before it passes every gate, +`isPtyJobOwnershipAvailable()` returns true, `windows-pty-job.win32.test.ts` +passes 6/6 — and `windows-msys-job.win32.test.ts` fails with a two-pid job list +that reads as a source defect rather than a build-freshness one. + +When that test fails, check the binary before the code: + +```js +// UTF-16LE, because usesCygwinRuntime holds the literals +readFileSync(conptyNodePath).includes(Buffer.from('msys-2.0.dll', 'utf16le')) +``` + +False means the addon predates the fix; rebuild node-pty from patched source. +Note that a git worktree sharing `node_modules` with its main checkout shares +that checkout's `build/Release/conpty.node`, so pinning the _source_ to a commit +does not pin the _addon_. + +The gate should assert the same marker, the way `stagedRelayAddonIsUnpatched()` +in `src/main/windows/windows-process-table.ts` already sniffs a patched addon by +a binary import name. Symbol presence cannot distinguish patch revisions; a +marker or an exported revision number can.