ConPTY kill(): the console-list agent races the pseudoconsole teardown, so the leftover-process sweep never runs

Open
#952 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
4/5
Estimated time
3-5 days
Newbie friendliness
52/100
Issue type
Bug
Clarity
Mostly clear
Activity status
Quiet
Tech stack
cpp, node.js

Research direction

Start with kill() and _getConsoleProcessList() in lib/windowsPtyAgent.js, then read conpty_console_list_agent.js and the AttachConsole call in src/win/conpty_console_list.cc. Run the supplied Windows reproduction to observe the race, stderr stack trace, and surviving process stages. Done means the console-process sweep is reliable during ConPTY teardown and a failed helper does not emit an inherited stack trace or impose the 5000 ms fallback.

Written by the indexing model from the issue text.

Description

Summary

On the ConPTY path, WindowsPtyAgent.kill() forks the console-list agent and then destroys the pseudoconsole on the next statement. The agent frequently loses that race, throws AttachConsole failed at module scope onto the inherited stderr, and never sends its message — so the 5000 ms timeout resolves [this._innerPid] and the leftover-process sweep runs five seconds late against a pid that was already killed.

The sweep exists to prevent detached console processes surviving a kill (the comment on the winpty branch cites microsoft/vscode#26807). Measured here, it does not prevent them.

The code

lib/windowsPtyAgent.js, kill():

this._getConsoleProcessList().then(consoleProcessList => {
  consoleProcessList.forEach(pid => { try { process.kill(pid) } catch (e) {} })
})
this._ptyNative.kill(this._pty, this._useConptyDll)   // synchronous, next statement
this._conoutSocketWorker.dispose()

_getConsoleProcessList() forks conpty_console_list_agent, which calls getConsoleProcessList(shellPid) at module scope. src/win/conpty_console_list.cc throws when AttachConsole fails:

if (!AttachConsole(pid)) {
  throw Napi::Error::New(env, "AttachConsole failed");
}

Forking a node process takes long enough that the pseudoconsole is often gone before the agent attaches. Two consequences follow from that throw:

  1. The agent dies without process.send, so the promise falls through to the 5000 ms timeout and resolves [this._innerPid] — a pid _ptyNative.kill has already dealt with. The sweep becomes a no-op.
  2. fork() defaults to silent: false, so the child's stderr is inherited. The uncaught throw prints a full stack trace into the host application's stderr, from a path the host did not call and cannot catch.

Measurements

node-pty 1.1.0, Node 24.13.1, Windows 11 26200, useConptyDll false (the default).

Frequency. 12 rounds of pty.spawn + kill(), nothing else in the process — 5 of 12 printed the stack trace:

node_modules\node-pty\lib\conpty_console_list_agent.js:13
var consoleProcessList = getConsoleProcessList(shellPid);
                         ^
Error: AttachConsole failed
    at Object.<anonymous> (...\lib\conpty_console_list_agent.js:13:26)

Leak. Spawn a shell, run a pipeline, kill(), wait 6 s — past the 5 s fallback. A stage survived in 5 of 5 rounds:

round 0: 3 pipeline stages before kill -> 1 still alive 6s after kill
round 1: 3 -> 1
round 2: 3 -> 1
round 3: 3 -> 1
round 4: 3 -> 1

Repro:

const pty = require('node-pty')
const delay = ms => new Promise(r => setTimeout(r, ms))

for (let i = 0; i < 5; i++) {
  const p = pty.spawn('C:\\Program Files\\Git\\usr\\bin\\bash.exe', ['--noprofile', '--norc', '-i'], { cols: 100, rows: 24 })
  p.onData(() => {})
  await delay(700)
  p.write('sleep 60 | cat\n')
  await delay(1200)
  // count sleep.exe / cat.exe here
  p.kill()
  await delay(6000)
  // count again — survivors remain
}

Why it is worth fixing rather than silencing

A consumer cannot work around either half. The stack trace comes from a forked child it did not create, so it cannot be caught or redirected. The failed sweep is silent — the promise resolves normally with a shorter list, so there is no signal that the console list was never read.

We compensate downstream by resolving the console list ourselves at signal time and fanning out over it, but that only covers our own signal path; node-pty's kill still leaves what it leaves.

Suggested fixes

Smallest correct change is to read the console list before destroying the pseudoconsole, since that is the only moment the console is guaranteed to exist:

const consoleProcessList = await this._getConsoleProcessList()
this._ptyNative.kill(this._pty, this._useConptyDll)
consoleProcessList.forEach(pid => { try { process.kill(pid) } catch (e) {} })

This makes kill() async, which may not be acceptable. Two smaller changes are independently worth having either way:

  • Catch in the agent. conpty_console_list_agent.js should wrap the call and process.send({ consoleProcessList: [] }) on failure, so a lost race costs nothing instead of five seconds and a stack trace.
  • Pass silent: true to fork(). A helper's failure should not print into the host's stderr from a path the host never called.

Context

Found while investigating a stray stack trace in CI for a Windows plugin that supplies a ProcessInspector on top of node-pty's ConPTY. Full analysis, including the control that ruled out our own console probe as the source: sjh9714/dsh-win32#26.

Dominant language
TypeScript
Stars
2k
Forks
337
Avg merge
21h 58m
Merged PRs (30d)
3

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from microsoft/node-pty

All issues in microsoft/node-pty

Similar issues

More TypeScript issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.