Skip to content

kill() on ConPTY can hard-kill an unrelated live process up to 5s later via bare-PID fallback in _getConsoleProcessList timeout #967

Description

@Zalbaag

In lib/windowsPtyAgent.js, WindowsPtyAgent.prototype.kill() (ConPTY,
non-DLL branch) calls this._getConsoleProcessList().then(list => list.forEach(pid => process.kill(pid))) without awaiting it, then
synchronously closes the pseudo-console. The forked list child therefore
always loses the race (console already closed), no message ever arrives,
and the 5s timer runs agent.kill(); resolve([_this._innerPid]) — "just send
back the shell PID".

Up to five seconds after kill(), the library hard-kills that PID by bare
number. The owning application has typically already reaped the shell by then
(in our case: tree-killed and ESRCH-confirmed). If the OS recycled the PID in
the interim, the kill lands on an unrelated live process — silently (ESRCH
swallow on success-path failure only; a live victim reports nothing).

Rehearsed, not inspected (plain node, wrapped in-process process.kill):
spawn pty shell (pid 11164) -> kill() -> wrapped process.kill(11164) fires
at dt=5.0s post-return; shell ESRCH-confirmed gone. Locally we resolve []
instead (safe here only because our app kills its own tree with image-verified
fail-closed kills plus taskkill); restoring the fallback brings the call back
(both directions rehearsed). Your own comment in the winpty branch of the same
function names the hazard: "Process IDs can be reused as soon as all handles
to them are dropped."

Why resolve([]) is NOT proposed as the upstream fix: for any consumer relying
on node-pty itself to kill the console process list, dropping the fallback
reintroduces microsoft/vscode#26807 (detached node servers surviving kill()).
It is valid only where the caller owns teardown. The proposals below preserve
the list-kill for those consumers.

Ask (two separable options):

  1. Close-without-enumerate option for kill() (callers that already kill
    their own tree don't need the console-list fork at all).
  2. Alternatively, attach-before-close: await the list before
    ClosePseudoConsole so a timeout genuinely means a dead console.

Cross-reference: downstream we consume this via
homebridge/node-pty-prebuilt-multiarch, whose rebuild would pick up a fix
here. No PR attached — issue only.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions