fix(shell): guard Windows pty teardown against blocked process creation - #3918
Draft
posthog[bot] wants to merge 1 commit into
Draft
fix(shell): guard Windows pty teardown against blocked process creation#3918posthog[bot] wants to merge 1 commit into
posthog[bot] wants to merge 1 commit into
Conversation
Closing a terminal on Windows could produce a `spawn UNKNOWN` error report from the Electron main process. node-pty's `WindowsPtyAgent.kill()` forks a `conpty_console_list_agent` helper to enumerate the ConPTY console process list. That `fork()` throws synchronously when CreateProcess fails outright (antivirus/EDR blocking process creation), and it sits inside a Promise executor whose `.then()` has no rejection handler — so it surfaced as an unhandled rejection during routine shell teardown, and the console-process reap was skipped entirely. Extend patches/node-pty.patch to guard the fork and fall back to the shell PID (matching the existing timeout path), attach an `error` listener to the forked helper, and add a `.catch()` on the reap chain so teardown can never surface as an unhandled rejection. Separately, harden ShellService teardown: the three duplicated teardown blocks now share one `teardownSession()` that drops the session from the map before releasing the pty and guards `pty.destroy()`, so a synchronous failure can't leak a session entry or abort the `destroyAll()` shutdown sweep. Generated-By: PostHog Code Task-Id: aa8c2e1f-6579-4308-bc35-da21fbfca990
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Closing a terminal on Windows could produce a
spawn UNKNOWNerror report from the Electron main process, affecting a small number of Windows users.node-pty's
WindowsPtyAgent.kill()forks aconpty_console_list_agenthelper to enumerate the ConPTY console process list so it can force-kill leftover console processes.fork()throws synchronously when CreateProcess fails outright — libuv's genericUNKNOWN, typically antivirus or EDR blocking process creation.That
fork()sits inside anew Promise(...)executor, and the consuming.then()has no rejection handler. So the failure became an unhandled rejection on every affected terminal close, and the console-process reap was skipped entirely.Worth correcting one detail from the originating report: this is not a synchronous throw escaping
pty.destroy(), so atry/catchat our call sites would not have caught it, and the reported side effects (leaked session map entry, aborted shutdown sweep) did not actually occur for this error — the rejection is asynchronous, sosessions.delete()still ran and the loop still completed. The captured event confirms it:mechanism.type = onunhandledrejection, and our own handler tagged itunhandledRejection, notuncaughtException. The defect is still real, just one layer down — and still unguarded in the latest upstream beta, so patching is the route.Changes
The actual fix — extend
patches/node-pty.patch(this repo already patches node-pty):fork()in_getConsoleProcessList()and fall back to the shell PID, mirroring the existing 5s-timeout fallback. This also restores the force-kill for the shell process, which was being skipped.errorlistener to the forked helper (an unhandlederrorevent on aChildProcessthrows)..catch()on the reap chain so teardown can never surface as an unhandled rejection.Defence-in-depth, not what fixes the above —
ShellServiceteardown was duplicated across three call sites. They now share oneteardownSession()that drops the session from the map before releasing the pty and guardspty.destroy(). This closes a genuine latent hazard of the shape the report described (the native_ptyNative.kill()insidedestroy()can throw synchronously), which would otherwise leak a session entry and abort thedestroyAll()shutdown sweep, leaving shells running after quit.How did you test this?
child_process.forkto throwspawn UNKNOWNand drivingWindowsPtyAgent.prototype._getConsoleProcessListthrough the exact shapekill()uses (.then()with no rejection handler):unhandledRejection: spawn UNKNOWN, reaped PIDsnull(reap skipped) — matching the captured eventnode_modules/node-ptyafter install; re-ran the repro against the installed package.pnpm install --frozen-lockfilepasses, confirming the updatedpatch_hashinpnpm-lock.yamlis consistent.shell.test.ts; full file passes (9/9). Confirmed non-vacuous by revertingshell.tstomain— both new tests fail withspawn UNKNOWNand the other 7 still pass.biome checkclean on the touched paths.tsc --noEmitreports zero errors in shell files (the 54 remaining are pre-existing, from unbuilt@posthog/agenttypes).fork()boundary instead.Automatic notifications
Created with PostHog Desktop from this inbox report.