mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 16:02:32 +00:00
Neil
2b1254d681
* fix(windows): own PTY process trees with job objects Teardown used to answer 'is this tree mine, and how do I kill it?' by scraping the process table, walking parent pids back to Orca, and running taskkill /T /F only if the walk said yes. Every step is a guess, and the code said so itself: windows-pty-root-identity.ts:35 already named the fix -- 'an inherited handle / Job Object'. The guesses fail in the ways users report. A pid walk cannot survive pid reuse, so teardown refused whenever it could not prove ownership, and a refused kill is an orphaned agent tree holding the worktree directory open (#9045, #10475, #10087). A descendant that reparented is invisible to the walk. The scrape itself could be blocked by policy, which read as 'no evidence'. node-pty now creates a job object per ConPTY and assigns the shell under CREATE_SUSPENDED, before it can spawn anything -- assigning afterwards leaves a window in which a fast child escapes. Termination is one TerminateJobObject; liveness is QueryInformationJobObject. Verified on Windows 11 against a shell whose grandchild was spawned detached: job membership came back [shell, grandchild] and one call killed both. Neither a parent-pid walk nor GetConsoleProcessList sees that grandchild -- it leaves the console and reparents, which is exactly the claude.exe/node.exe/cmd.exe orphan in #9045. KILL_ON_JOB_CLOSE means a daemon that dies without unwinding no longer strands shells (#9195, #10415). The job is the daemon's, not the app's, so an app-main crash still leaves sessions alive -- the guarantee win-crash-survival-e2e asserts. Both entry points report unavailable rather than a false success when a pty has no job: an outer job without BREAKAWAY_OK can refuse the assignment, and a pty from an older build has none. Reading 'we could not tell' as 'already dead' is the original bug, so the old probe stays as the fallback. * test(windows): pin job ownership against a real detached grandchild The unit tests pin the contract; this pins what the contract is for. A grandchild spawned detached leaves the pane's console and reparents, so GetConsoleProcessList and a parent-pid walk both miss it -- that is the process that outlived its pane and held the worktree directory open. Includes a guard that this build actually has job support, so a node-pty rebuilt from unpatched sources fails loudly instead of letting every assertion pass vacuously. * fix(windows): correct the job liveness contract to what Windows actually does I claimed an emptied tree would report [] and that this was the evidence a stale registry entry lacks (#15549). Running it on Windows 11 showed otherwise: node-pty drops its handle record and closes the job when the shell exits, so a dead tree reports null. Null therefore means unverifiable in the sense of docs/reference/ssh-execution-boundary.md -- no job support, not a ConPTY, or no longer tracked -- and is never evidence that processes died. A caller reading it as proof of death would have been right by accident after a normal exit and wrong on a host that refused the assignment. What the API does add is descendant liveness for a tree that is still tracked, including children that detached from the console. * fix(windows): stop a clean shell exit from reaping backgrounded processes Measured on Windows 11: with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE on the per-PTY job, releasing the handle when the shell exits also killed whatever the user had backgrounded. Typing 'exit' in a pane reaped a detached server that survived before this patch. That is a behaviour change nobody asked for. The approved change was that killing the terminal daemon reaps its shells -- not that a clean exit reaps your background job. The job's purpose is to make an EXPLICIT teardown exact, which TerminateJobObject still does. Reaping a dead daemon's shells now needs the daemon-level job the design called for: the daemon assigns itself, children inherit membership, and its closure on daemon death reaps them without touching clean-exit semantics. Not in this PR; noted in the reference doc. * test(windows): pin that a clean exit leaves backgrounded work alone The counterpart to the tree-kill test. Without it, re-adding JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE would look like a tightening rather than the regression it is. * fix(windows): stop a winpty pty id from matching a ConPTY job winpty.cc and conpty.cc each mint their 'pty' id from an independent counter, and windowsPtyAgent stores both in the same _pty field. So a winpty-backed terminal's id can collide with a live ConPTY baton -- and closing that pane would have terminated an unrelated pane's entire process tree. Both job entry points now take the shell pid and the native side refuses unless GetProcessId(hShell) matches, which makes the id unforgeable. Two more from the same read-through: - ResumeThread's failure was ignored. A shell left suspended is a pane that never prints and never exits, which is far harder to diagnose than a failed spawn; it now cleans up and throws. - handle->hJob was assigned before LoadConptyDll, which can throw. A baton carrying a job but never reaching SetupExitCallback has nothing left to close it, so the assignment moved down beside hShell. * docs(windows): record the unsynchronised node-pty baton table Pre-existing upstream -- the exit thread erases while the main thread reads -- but terminatePtyJob adds an instance of it, so it belongs in writing rather than in someone's head. * fix(windows): close four gaps found in review BREAKAWAY. The per-PTY job set no limits, so a child asking for CREATE_BREAKAWAY_FROM_JOB was refused with ERROR_ACCESS_DENIED. Installers, msiexec and some updater and service-control paths spawn that way deliberately -- they worked before this patch and would have failed only inside an Orca terminal, which is the worst shape a bug report can take. JOB_OBJECT_LIMIT_BREAKAWAY_OK restores it; a child still has to ask, so ordinary descendants stay owned. EMPTY IS NOT UNAVAILABLE. The native reader returns an empty list -- not an error -- when CreateToolhelp32Snapshot fails, which is what an EDR hook or a restricted token produces. Callers read that as 'nothing is running' and teardown concludes a live PTY root is already gone. The snapshot must contain the querying process; nothing else is unfalsifiable, and one predicate catches empty, truncated and permission-filtered tables alike. NO DEADLINE. Replacing execFile dropped its 3s timeout. The vendored reader latches a module-global while a request is in flight and clears it only after draining its callbacks, with no try/catch -- so one wedge leaves every later call queued behind a promise that never settles, and the process table is dead for the life of the app. The bound is back. GUESSED IMAGE PATH. executablePath was derived from the first space-delimited token, which reads 'C:\Program' out of an unquoted 'C:\Program Files\nodejs\node.exe ...'. Wrong evidence is worse than none, and the only consumer already had the full path in , so the field is gone rather than repaired. Also: remove_pty_baton no longer sits inside assert(), which NDEBUG would compile away along with the call, and the job accessors hold a lock across lookup and use -- handle values are recycled, so an unguarded read could pass the shell-pid check against an unrelated process and terminate the wrong job. * fix(windows): apply the job lock once per accessor The patch script matched a string its own replacement still contained, so PtyTerminateJob got two lock_guards named guard and PtyListJobProcessIds got none. MSVC caught it: error C2374 redefinition. * test(windows): pin that a child can still break away from the job Verified on Windows 11: 'start /b' writes its marker and no access-denied appears. Without JOB_OBJECT_LIMIT_BREAKAWAY_OK this fails, and it fails only inside an Orca terminal -- so the failure would look like Orca corrupting unrelated software rather than like a job-object change. * fix(windows): stop the ownership guard from reading a closing handle The guard called GetProcessId(hShell) to prove identity, but the exit watcher closes hShell on another thread -- so the guard could read a closed handle, and under strict handle checks that is fatal rather than merely wrong. Worse, it widened the gap between validating hJob and using it from two instructions to a kernel round-trip, and handle values recycle: the likeliest occupant of a freshly recycled value in this process is another pane's job. The pid never needed a handle. It is captured at spawn and compared as a DWORD, so the guard touches no handle at all, and hShell is now closed inside the same lock as hJob. Also from review: - reject CR/LF in a cmd argument. cmd ends the command at a raw line break whatever the quote state, so there is no escape for it; encoding one anyway truncates the argument and can leave the remainder to run as a command. Agent prompts are this encoder's motivating input. - ask the process table only for the fields a caller needs. Memory and CommandLine each cost an OpenProcess per process, inline, for every process on the box -- and the 1024 bound is patched out. Ancestry reads now skip both. - corpus gains the degenerate quote-only and two-quote arguments. - PtyListJobProcessIds' docblock still taught the empty-list contract that was corrected on the TS side, and now records that the ConPTY console host is never a job member. - drop a write to NumberOfAssignedProcesses, which is output-only. - pty_baton::hShell is initialised; ownsShell was only safe because && short-circuited ahead of it. The backgrounded-child test is rescoped: 'start /b' uses CREATE_NEW_CONSOLE, not CREATE_BREAKAWAY_FROM_JOB, so it proves job membership does not block backgrounding -- not that BREAKAWAY_OK works. That flag rests on the Win32 contract, and I have said so rather than letting the test imply coverage it does not have. * fix(windows): bound retries after the process table wedges The 3s deadline stops a caller hanging, but the timed-out call leaves its callback in the vendored module's queue -- and that queue drains only when the latched request completes, which in this wedge never happens. Retrying at the caller's poll rate would add a closure per tick forever. A 30s cooldown bounds it to one probe, and a late callback clears the cooldown because it proves the reader recovered. Also pins the deadlock invariant in the patch: the exit thread's lock must close before tsfn.BlockingCall, because that waits on the JS thread and the JS thread can be waiting on the same mutex inside PtyTerminateJob. Correct today by scoping; a comment so a later refactor does not widen it. * revert(windows): drop the field-selection API, which cannot pay off I added it for a real perf finding -- Memory and CommandLine each cost an OpenProcess per process -- and then never wired a caller, so the claim that ancestry reads skip them was wrong. Wiring it would have been worse than leaving it dead. The only ancestry consumer is the teardown identity probe, which needs a snapshot that started AFTER it asked, for pid-recycle detection. Bypassing the shared reader to get narrow fields would let that request join a scan already in flight -- trading a correctness guarantee for milliseconds. Field selection only pays off if callers can ask for less, and they cannot: one shared snapshot serves every caller so a 32-wide teardown collapses into a single scan, which means it has to carry every field. The reasoning now lives next to the flags instead of in a dead export. * fix(process): three P1s from review — a crash vector and two wedge bugs STDIN EPIPE COULD TAKE DOWN THE MAIN PROCESS. A child that exits without reading makes the queued write fail with EPIPE, and an unhandled error on a stream is an uncaught exception. The child's own error listener does not cover its stdin stream, so runProcess({ input }) against a short-lived child was a crash, not a failed call. THE COOLDOWN LEAKED A BATCH PER CYCLE INSTEAD OF BOUNDING IT. At expiry every concurrent caller passed the check before any of them re-armed it, so each enqueued a callback into the still-latched native queue and each cycle leaked another batch. The cooldown is now re-armed BEFORE probing, so exactly one caller gets through. A SYNCHRONOUS THROW LEFT ITS DEADLINE RUNNING. The timer was declared inside the try, so catch could not clear it; it fired later and wedged a reader that had already recovered. Hoisted and cleared, and wedge state now carries a generation so a request that lost its deadline cannot mutate it on behalf of the one that replaced it. Found by review once the prompts were short enough for the reviewer to finish -- the previous two rounds died on prompt length. * fix(process): stop a stream error from crashing the main process Same class as the stdin EPIPE finding, two instances further on: stdout and stderr had data listeners and no error listeners, and an unhandled error on a stream is an uncaught exception. Scoped to runProcess, which owns the child outright. spawnProcess hands the streams to its caller, and a blanket handler there defeats callers that track and remove their own listeners -- the SSH ProxyCommand transport does exactly that, and its cleanup test caught the attempt. Documented on spawnProcess so the boundary is explicit rather than inferred. * fix(windows): validate the ConPTY DLL before creating the process LoadConptyDll throws when conpty.dll is missing -- a real state, and one this branch hit during development. It ran after CreateProcessW and ResumeThread but before the baton and the exit watcher were installed, so a throw leaked the job, process and thread handles and left an untracked shell tree running. Once per attempt, so a broken install accumulates orphan shells on every retry. Resolving the DLL first costs nothing and leaves exactly two throws after creation: the CreateProcessW failure, where nothing exists yet, and the resume failure, which already cleans up after itself. This also closes the same leak for hProcess and hThread, which predates the job work. * feat(windows): add the daemon-level job the design called for The plan specified two nested jobs and I built one. That gap is why dropping KILL_ON_JOB_CLOSE from the per-PTY job cost the approved guarantee that a dead daemon reaps its shells -- I had one job trying to answer two questions, and the two answers conflict. They are separate jobs. The per-PTY job answers 'kill exactly this pane's tree, now', and cannot be kill-on-close because its handle is released when the shell exits, which would reap whatever the user backgrounded. The daemon assigns itself to a second job that IS kill-on-close; its handle is released only when the daemon dies. Children inherit membership, so every pty is covered and the per-PTY jobs nest inside it. Daemon, never app: an app-main crash must still leave sessions alive, which win-crash-survival-e2e asserts. Both jobs carry BREAKAWAY_OK, or a child asking to break away is refused at whichever level lacks it. Restores #9195 and #10415, which I withdrew from this PR earlier. * docs(windows): record what the host job does not cover An app-hosted PTY gets a per-PTY job but no crash reaping, because the alternative is a kill-on-close job on the app -- which is precisely what the crash-survival guarantee forbids. * ci(windows): run the win32 suites in the PR windows job Both were skip-on-non-win32 and had only ever run on one machine I drive by hand -- which went unreachable at exactly the moment I needed to verify the percent-escaping fix. Verification that depends on one box is not verification. The job already builds node-pty from patched source and already runs a useConptyDll test, so the ConPTY runtime files are in place by this step. This also makes the encoder a gate: the corpus is the only thing standing between an agent prompt and a mangled argv, and it now runs against real cmd.exe on every PR. * fix(deps): refresh the lockfile for the current patch hashes pnpm records a hash per patched dependency, and I regenerated both patches repeatedly across the review rounds without refreshing the lockfile. Every local run used --frozen-lockfile's looser sibling, so nothing caught it until CI did: ERR_PNPM_LOCKFILE_CONFIG_MISMATCH Cannot proceed with the frozen installation. The current "patchedDependencies" configuration doesn't match the value found in the lockfile Verified with pnpm install --frozen-lockfile locally this time. * ci(windows): build node-pty from source before the win32 suites CI proved the encoder fix on real cmd.exe -- 26/26 -- and in the same run proved the job suite had been testing an unpatched binary. node-pty prefers its upstream prebuild, which does not contain this patch, so every job-object export was absent and isPtyJobOwnershipAvailable() was false. That guard is why the failure was loud rather than a vacuous pass, and it is the reason the assertion exists. Packaging was never affected: rebuild-native-deps.mjs already builds node-pty from source for Electron and restores the ConPTY runtime files. The gap was the node-runtime test environment only. Not changing requiresPatchedNodePtySourceBuild's win32 exemption here. Its premise -- that the patch is Unix-only -- is now false, but lifting it also needs pnpm rebuild to force a source build, and I cannot validate that on macOS and Linux from here. Recorded as a follow-up instead of changed blind. * test(windows): gate the host-job guarantee in CI The daemon-level job had one hand-run proof and no automated coverage -- the same shape of gap that let an unpatched node-pty go unnoticed until CI caught it. It needs a real second process, because the assertion is about what happens when that process is force-killed: a host in a kill-on-close job must strand neither its pty nor a grandchild spawned detached, which is the process a parent-pid walk cannot see. Runs in the Windows PR job alongside the per-pty and encoder suites, so both halves of the two-job design are now gated rather than asserted. * fix(windows): serialise host-job creation Two callers racing PtyAssignCurrentProcessToJob would each create a job, put the process in both, and leak the first handle -- and the handle is what keeps a kill-on-close job alive, so a leaked one is never released. 'Only JS calls it' is not a guarantee: a worker thread with its own N-API env shares these statics. Also records the ordering requirement it depends on. AssignProcessToJobObject adds only the named process; children inherit membership, but a pty that already exists does not join retroactively and would not be reaped. The daemon assigns at startup, before the ConPTY warmup and before any session, which is correct today and now stated rather than implied. * fix(daemon): keep the host job off the startup path Assigning the host job at daemon startup resolves the node-pty native module, which loads the ConPTY addon -- and paying that before the endpoint is published delayed readiness enough that daemon-boot-smoke failed on windows-latest, deterministically. windows-conpty-warmup already carries the comment for this exact hazard ('setImmediate keeps the ready/handshake path ahead of the warm-up') and I put an eager load in front of it anyway. Moved to the pty spawn path, which already pays ConPTY cost, and memoised. Children inherit job membership, so assigning immediately before the first spawn still covers every pty -- and nothing can spawn one before the endpoint exists.
184 lines
9.2 KiB
Markdown
184 lines
9.2 KiB
Markdown
# Reading the Windows process table
|
|
|
|
Orca needs three things from the Windows process table: who a PID's parent is
|
|
(descendant walks and teardown identity), what a process is running (agent
|
|
recognition), and how much memory/CPU it uses (Resource Manager).
|
|
|
|
Node cannot answer the first one without native code. That is why seven
|
|
independent readers existed, each forking `powershell.exe` to run
|
|
`Get-CimInstance Win32_Process`, with a `wmic` fallback that Windows 11 24H2 has
|
|
since removed.
|
|
|
|
## Use the native snapshot
|
|
|
|
`src/main/windows/windows-process-table.ts` is the only module that may read the
|
|
table. It wraps a Toolhelp32 snapshot from `@vscode/windows-process-tree`.
|
|
|
|
```ts
|
|
import { readWindowsProcessTable, readWindowsProcessTableFresh } from '../windows/windows-process-table'
|
|
```
|
|
|
|
- `readWindowsProcessTable()` — shared TTL cache. Use for anything periodic.
|
|
- `readWindowsProcessTableFresh()` — a snapshot that starts after the call. Use
|
|
for teardown identity, where a cached row can predate the exit it is being
|
|
asked about.
|
|
|
|
Both **reject** when the table cannot be read. Do not convert that into an empty
|
|
array. An empty table is a claim that nothing is running, and callers act on
|
|
that claim by declaring a tree dead or a shell childless. "Unavailable" has to
|
|
stay distinguishable from "empty" — collapsing the two is how a PTY tree
|
|
survived its own teardown (#9045).
|
|
|
|
Measured on Windows 11 with 1050 processes (p50 / p95):
|
|
|
|
| | p50 | p95 |
|
|
| --- | --- | --- |
|
|
| pid + ppid + name | 15.9 ms | 17.5 ms |
|
|
| + memory + command line | 30.6 ms | 33.7 ms |
|
|
| `Get-CimInstance` via PowerShell | 706 ms | 723 ms |
|
|
|
|
## Why the package is patched
|
|
|
|
`config/patches/@vscode__windows-process-tree@0.8.0.patch` carries two hunks.
|
|
|
|
1. **Spectre mitigation.** The upstream `binding.gyp` requires Spectre-mitigated
|
|
libraries, which Orca's Windows build agents do not install. `node-pty` is
|
|
patched the same way for the same reason.
|
|
2. **The 1024-process cap.** `GetRawProcessList` stopped after 1024 entries.
|
|
Measured on a real host with 1051 processes, the module returned exactly
|
|
1024 and the querying process was itself among the 27 missing. A truncated
|
|
snapshot silently hides the descendants a teardown is trying to reap — the
|
|
exact failure the native path exists to remove.
|
|
|
|
The typings claim `commandLine` is truncated at 512 characters. Measured, it is
|
|
not: the longest observed on a real host was 26,059.
|
|
|
|
## Packaging
|
|
|
|
The addon is Windows-only, so it follows the same contract as
|
|
`windows-native-registry` (asserted by
|
|
`config/scripts/package-electron-runtime-contract.test.mjs`):
|
|
|
|
- an `optionalDependency`, so a macOS/Linux install tolerates its absence;
|
|
- **not** in `pnpm.onlyBuiltDependencies` — pnpm installs optional dependencies
|
|
on every host, and macOS/Linux must never run `node-gyp` for it;
|
|
- listed in the win32 branch of `rebuild-native-deps.mjs` and
|
|
`ensure-native-runtime.mjs`;
|
|
- copied into the packaged `node_modules` for win32 only.
|
|
|
|
## What the snapshot does not provide
|
|
|
|
`CreationDate` (process start time) has no equivalent. Anything using a start
|
|
time to prove a PID has not been recycled — daemon identity, managed-hook
|
|
ownership, and CPU accounting in the memory collector — still reads it through
|
|
its own query. Those callers are not migrated.
|
|
|
|
Start time is a proxy for identity, not identity. The durable answer for the
|
|
process trees Orca itself spawns is an inherited handle: a job object names the
|
|
tree Orca created, so no start-time comparison is needed. Those readers should
|
|
be resolved that way rather than by adding a start time to this module.
|
|
|
|
Do not adopt `getProcessCpuUsage()` from the package. It takes both CPU samples
|
|
inside one call with a blocking `Sleep(1000)` in the middle, which would hold a
|
|
libuv threadpool slot for a full second out of the Resource Manager's two-second
|
|
poll.
|
|
|
|
## Owning a PTY's process tree
|
|
|
|
`src/main/windows/windows-pty-job.ts` is the counterpart to reading the table:
|
|
it answers "is this tree mine, and how do I kill it?" with a handle instead of
|
|
an inference.
|
|
|
|
node-pty is patched (`config/patches/node-pty@1.1.0.patch`) to create a job
|
|
object per ConPTY and assign the shell to it under `CREATE_SUSPENDED`, before
|
|
the shell can spawn anything. Assigning after the fact leaves a window in which
|
|
a fast child escapes the job.
|
|
|
|
- `terminatePtyJob(proc)` — one `TerminateJobObject` call for the whole tree.
|
|
- `listPtyJobProcessIds(proc)` — the live pids under a tree that is still
|
|
tracked, including children that detached from the console.
|
|
|
|
Measured on Windows 11 against a shell whose grandchild was spawned `detached`:
|
|
job membership was `[shell, grandchild]` and one call killed both. Neither a
|
|
parent-pid walk nor `GetConsoleProcessList` sees that grandchild — it leaves
|
|
the console and reparents, which is what left `claude.exe`/`node.exe`/`cmd.exe`
|
|
holding worktree directories open (#9045, #10475, #10897).
|
|
|
|
The per-PTY job deliberately does **not** set
|
|
`JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE`. Measured on Windows 11: with that flag,
|
|
releasing the handle when the shell exits also kills whatever the user left
|
|
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.
|
|
|
|
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,
|
|
so every pty is covered and the per-PTY jobs nest inside it. Its handle is
|
|
released only when the daemon process dies, so a crashed daemon reaps its tree
|
|
without changing what a clean shell exit means.
|
|
|
|
The split is the point. One job answers "kill exactly this pane's tree, now";
|
|
the other answers "do not strand anything if the host dies". Trying to get both
|
|
from one job is what reaped users' backgrounded work on a clean `exit`.
|
|
|
|
It belongs to the daemon and never to the app: an app-main crash must still
|
|
leave sessions alive, which `.github/workflows/win-crash-survival-e2e.yml`
|
|
asserts. The app spawns the daemon `detached` and is itself in no job, so
|
|
nothing is inherited across that boundary.
|
|
|
|
The consequence is that a PTY hosted by the app rather than the daemon gets a
|
|
per-PTY job but no crash reaping. That is deliberate — the alternative is a
|
|
kill-on-close job on the app, which is exactly what the crash-survival
|
|
guarantee forbids.
|
|
|
|
Once the shell exits, node-pty drops its handle record and closes the job, so a
|
|
terminated tree reports `null` rather than `[]`. Null means *unverifiable* in
|
|
the sense of [`ssh-execution-boundary.md`](./ssh-execution-boundary.md) — no job
|
|
support, not a ConPTY, or no longer tracked. It is never evidence that
|
|
processes died.
|
|
|
|
Both functions report `unavailable` / `null` rather than a false success when a
|
|
pty has no job — an outer job without `JOB_OBJECT_LIMIT_BREAKAWAY_OK` (some EDR
|
|
and container hosts) can refuse the assignment, and a pty started before this
|
|
build has none. Callers must fall back, not conclude the tree is gone. That
|
|
conflation is the original bug.
|
|
|
|
### Known limitation: the baton table is not synchronised
|
|
|
|
node-pty keeps its per-terminal handles in a plain `std::vector` and erases from
|
|
it on a detached exit thread, while `get_pty_baton` is called from the main JS
|
|
thread. That race predates this change — `PtyResize`, `PtyClear` and `PtyKill`
|
|
all read the table the same way — but `terminatePtyJob` adds an instance of it:
|
|
the exit thread can close `hJob` between the lookup and `TerminateJobObject`.
|
|
|
|
Losing that race normally just returns `FALSE`, which surfaces as `unavailable`
|
|
and falls back. The case that would not be benign is a recycled `HANDLE` value,
|
|
where the call could reach a different job in the same process. Fixing it
|
|
properly means synchronising node-pty's handle table rather than adding a lock
|
|
around one accessor, so it is deliberately left alone here.
|
|
|
|
### The patch must actually be compiled
|
|
|
|
node-pty prefers its upstream prebuild and only builds from source when
|
|
`npm_config_build_from_source` is set or no prebuild exists for the platform.
|
|
The Windows prebuild does **not** contain this patch, so a plain `pnpm install`
|
|
on Windows yields a node-pty without the job-object exports — and
|
|
`terminatePtyJob` then reports `unavailable` on every call, which is
|
|
indistinguishable from a correctly degraded build.
|
|
|
|
Packaging is unaffected: `rebuild-native-deps.mjs` rebuilds node-pty from source
|
|
for Electron and restores the ConPTY runtime files that a bare `node-gyp
|
|
rebuild` skips. The gap is the **node-runtime test environment**, which is why
|
|
the Windows CI job rebuilds from source before running the win32 suites.
|
|
|
|
`isPtyJobOwnershipAvailable()` exists for exactly this: the win32 suite asserts
|
|
it is true before asserting anything else, so an unpatched binary fails loudly
|
|
instead of passing every case vacuously. That guard is what caught this.
|
|
|
|
`requiresPatchedNodePtySourceBuild()` in `ensure-native-runtime.mjs` still
|
|
exempts win32, on the premise that the patch is Unix-only. That premise is now
|
|
false, but lifting the exemption also needs `pnpm rebuild` to force a source
|
|
build — otherwise the assertion fires and the remedy does not fix it. Left as a
|
|
follow-up rather than changed blind.
|