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.
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.
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.
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.
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.
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.
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.
* fix(linux): restore Ubuntu 20.04 launch by pinning node-pty glibc symbols (#9902)
The bundled node-pty pty.node is compiled from source in release CI on
ubuntu-latest (glibc 2.39). glibc's 2.32-2.34 libpthread/libutil merge
relocated openpty/forkpty (GLIBC_2.34) and pthread_sigmask (GLIBC_2.32)
into libc under new symbol versions, so the from-source build bound to
versions absent on Ubuntu 20.04 (glibc 2.31). The main process imports
node-pty at startup, so the app crashed on launch. pty.node is the sole
blocker (Electron needs GLIBC_2.25; other native modules <= 2.17).
- Patch node-pty: a .symver shim pins the 3 symbols to their pre-merge
version (GLIBC_2.2.5 x64 / GLIBC_2.17 arm64), and Linux-only ldflags
force libutil.so.1/libpthread.so.0 back into DT_NEEDED. Guarded to
Linux; macOS/Windows untouched.
- Add a packaging gate (verify-linux-glibc-floor.cjs, afterPack): reads
each bundled native binary's objdump -p version needs and fails the
Linux build if any strong GLIBC_/GLIBCXX_/CXXABI_ node exceeds stock
Ubuntu 20.04 (glibc 2.31 / GLIBCXX_3.4.28 / CXXABI_1.3.12). Catches
GLIBC_ABI_DT_RELR, rejects GLIBC_PRIVATE, skips weak needs, fail-closed.
- Docs + tests; the lazy sherpa-onnx speech prebuilt (GLIBCXX_3.4.29,
never loaded at launch) is a documented libstdc++-floor exemption.
* fix(linux): assert DT_NEEDED provider deps in the glibc-floor gate
Harden the packaging gate (flagged in adversarial re-eval): the version-floor
check alone can false-pass if the patch's forced `-l:libutil.so.1` ever silently
drops — the pinned openpty@GLIBC_2.2.5 still resolves from libc's compat alias at
build time, but fails to load on Ubuntu 20.04 where openpty/forkpty live only in
libutil. The gate now also asserts that any binary importing openpty/forkpty
keeps libutil.so.1 in DT_NEEDED. Validated on a real symver-pinned .so with
libutil dropped (now fails) vs. present (passes). Documents the recommended
real-host smoke-test follow-up.
* Revert "Preload the daemon windowsHide shim via --require; wrap promisify custom (#7499)"
This reverts commit 8f396badaf.
* Revert "Hide console windows for children of the node.exe-hosted daemon (#7486)"
This reverts commit f0fdd3a716.
* Revert "fix(daemon): relocate daemon host image out of the install-dir kill zone (#7473)"
This reverts commit f4faafa987.
* Revert "fix(pty): keep runtime dirs a surviving daemon still uses (#7463)"
This reverts commit 3cd23a13a1.
* Revert "Relocate node-pty ConPTY runtime outside the Windows install dir (fixes update-time terminal loss) (#7421)"
This reverts commit 509c41e2bf.
Since #7473 the terminal daemon runs under a standalone node.exe.
Electron's bundled Node defaults windowsHide to true; plain node.exe
defaults it to false, so every child_process call in the daemon that
does not pass the flag - the periodic PowerShell CIM process probes,
node-pty's kill-path conpty_console_list_agent fork - now allocates a
visible console, which opens and closes a Windows Terminal window on
the user's screen every few seconds.
Fix: daemon-entry installs a child_process shim (first import, before
any module captures bindings like promisify(execFile)) that defaults
windowsHide: true across spawn/exec/execFile/fork and their sync
variants, restoring the Electron default the daemon has always relied
on. Explicit windowsHide from a caller still wins. Also adds
windowsHide to node-pty's console-list agent fork in the existing
patch as defense in depth.
Verified on Windows: reproduced the flash with the rc.5 production
daemon (WindowsTerminal windows, ~3s cadence matching the CIM probe
interval, conhost spawned visible-capable "0x4"); with the shim, a
node.exe-hosted daemon's children (OpenConsole, powershell, node
helpers) all run without a visible-capable console and session kill
still works end to end.
* Improve node-pty spawn diagnostics
* Preserve original error stack when adding node-pty recovery hint
Mutate the existing Error's message instead of replacing the object so
the original stack trace and custom fields survive into telemetry/logs.
Co-authored-by: Orca <help@stably.ai>
---------
Co-authored-by: Orca <help@stably.ai>
* fix(pty): release ptmx fd on natural exit + defuse SIGHUP-to-recycled-pid
Daemons accumulated ptmx fds over time because node-pty's UnixTerminal
only releases the master fd when destroy() runs. On the natural-exit
path (the common case — user closes a tab, shell runs `exit`) nothing
ever calls destroy(), so the fd leaks until GC. On macOS this
eventually hits kern.tty.ptmx_max=511 and all new terminals fail to
spawn.
Fix: release the fd synchronously on every teardown path (natural
exit, explicit kill, stale SSH spawn, daemon shutdown) and close the
concurrent SIGHUP-to-recycled-pid hazard inside node-pty's
UnixTerminal.destroy().
- src/main/daemon/pty-subprocess.ts: synchronous POSIX proc.kill
neutralization inside proc.onExit; dead guards on forceKill/signal
so they never target a reaped-and-possibly-recycled pid
- src/main/daemon/session.ts: new disposeSubprocess() for already-
exited sessions (fd release only, no SIGKILL) — avoids sending
SIGKILL to a recycled pid during daemon shutdown
- src/main/daemon/terminal-host.ts: dispose loop routes on isAlive —
live sessions get forceKillAndDisposeSubprocess (SIGKILL + fd
release), exited sessions get disposeSubprocess (fd release only)
- src/main/providers/local-pty-provider.ts: same POSIX kill
neutralization at top of onExit for the legacy local path
- src/relay/pty-handler.ts: same neutralization in wireAndStore;
disposed flag guards all public entry points; dispose() uses
SIGKILL (not SIGTERM) before destroy since the relay is exiting;
killTimer fallback + immediate-shutdown + stale-spawn cleanup all
call disposeManagedPty + ptys.delete so wedged children (D-state,
bad NFS) can't leak map entries against the 50-PTY cap
Windows is exempt everywhere — WindowsTerminal.destroy IS a kill()
call internally (closes the ConPTY agent), so neutralizing would
turn destroy into a no-op and leak the agent.
See docs/fix-pty-fd-leak.md for the full design.
Co-authored-by: Orca <help@stably.ai>
* fix(pty): patch node-pty native off-by-one leaking /dev/ptmx per spawn
node-pty 1.1.0's pty_posix_spawn on macOS walks low_fds[0..2] in an
allocation loop that breaks at the first fd >= STDERR_FILENO, then
cleans up via `for (; count > 0; count--) close(low_fds[count])`. In
the typical case (break at count=0) the cleanup body never runs and
low_fds[0] — a /dev/ptmx handle — leaks per spawn. Fixed upstream in
microsoft/node-pty af053f2 (PR #882), not in any 1.1.0 release.
Backport the 3-line cleanup-loop fix as a pnpm patch. E2E validated
against a dev daemon: 200 spawn/kill cycles kept the daemon's ptmx
fd count flat at baseline; prior runs reproduced linear 1-per-spawn
growth. Also documents the native root cause as a status addendum in
docs/fix-pty-fd-leak.md — the JS-side destroy() discipline previously
landed is still load-bearing for the SIGHUP-to-recycled-pid hazard and
for synchronous fd release on daemon shutdown.
Co-authored-by: Orca <help@stably.ai>
* fix(pty): capture stable kill spy ref in pty.test.ts
destroyPtyProcess reassigns proc.kill = () => {} on POSIX to defuse
the SIGHUP-to-recycled-pid hazard (see docs/fix-pty-fd-leak.md). After
that reassignment, proc.kill.mock is undefined and the assertions
crashed in CI. Capture a stable reference to the vi.fn() before it
gets reassigned.
Co-authored-by: Orca <help@stably.ai>
---------
Co-authored-by: Orca <help@stably.ai>
Neil
Jinwoo Hong
Jinjing
