Repository navigation
Defer conptyNative.connect() to prevent event loop blocking - #885
Merged
Daniel Imms (Tyriar) merged 10 commits intoFeb 3, 2026
Merged
Daniel Imms (Tyriar) merged 10 commits into
Daniel Imms (Tyriar) merged 10 commits into
Conversation
Fixes microsoft#763 The issue was that conptyNative.connect() called ConnectNamedPipe() synchronously for both input and output pipes. While the input pipe was connected via fs.openSync() before the call, the output pipe connection happened asynchronously in a worker thread. If the worker thread hadn't connected yet when ConnectNamedPipe was called, it would block the Node.js event loop waiting for the connection. This caused a deadlock when a debugger was attached (which slows down the event loop and worker thread startup). The fix defers the conptyNative.connect() call until the worker thread signals it has connected to the output pipe (via the onReady callback). This ensures both pipes have clients connected before ConnectNamedPipe is called, so it returns immediately without blocking.
If the worker fails to signal ready within 5 seconds, complete the connection anyway to avoid leaving the PTY in a zombie state.
Clear _pendingPtyInfo in kill() to prevent the timeout or onReady callback from calling connect() on an already-killed PTY handle.
The public pid property was only set once at construction, before the deferred connection. Update it in the ready_datapipe handler so users get the correct pid value.
Add test to ensure WindowsTerminal.pid is correctly updated after the deferred connection completes. This closes the test coverage gap for the public pid property.
Daniel Imms (Tyriar)
approved these changes
Feb 3, 2026
Daniel Imms (Tyriar)
enabled auto-merge
February 3, 2026 12:44
Martin Aeschlimann (aeschli)
approved these changes
Feb 3, 2026
Member
Author
|
Thanks for the quick reviews! 🚀 |
Member
Author
|
Daniel Imms (@Tyriar) Martin Aeschlimann (@aeschli) When can I expect this patch to ship in a beta package? This is currently blocking a PR in another repo. |
desu_ (desu777)
added a commit
to Vex-Foundation/Vex
that referenced
this pull request
Sep 2, 2026
…ages; determinized terminal tests Windows terminals reported pid 0 forever. node-pty defers ConPTY's connect (microsoft/node-pty#885), so IPty.pid is 0 until the first data event, and TerminalProcess.start read it synchronously after spawn. The pid property is now emitted once, on the first data event when node-pty had no pid at spawn (VS Code's terminalProcess contract), never as 0; the create reply does not wait for it (a silent shell must still start); refreshCwd never probes pid 0 (on macOS that was an lsof subprocess per Enter per terminal). A second Windows hazard from the same sources: node-pty's conout socket error handler rethrows when the socket has fewer than two error listeners and terminal.js registers none, so a conout error was an uncaught exception in the pty host; PtyAdapter gains onError, the spawner registers the guard at spawn and classifies with node-pty's own EAGAIN/EIO set, and TerminalProcess routes a fatal pty error into its ordinary exit path. The same pid 0 was why the real-pty suite killed its own vitest worker on the Windows lane: the survivor detector received pid 0, process.kill(0, 0) is always alive, and after its budget the SIGKILL of pid 0 became TerminateProcess on the worker itself (libuv substitutes the current process). The suite now refuses to signal any non-positive pid inside the test that produced it, waits for node-pty's live pid before recording a terminal, carries a win32-only stderr phase trace (evidence tooling, deleted once the lane is green twice) and a win32 ConPTY canary that pins the deferred-pid contract. The e2e teardown hang had one owner: the market service's stop drained an in-flight DexScreener poll riding the app's Chromium transport, whose window the quit was destroying, so the promise never settled and the cleanup registry's allSettled never resolved. The drain is bounded at its owner (2 s, timer armed before the await; a late poll can neither publish nor reschedule). Every quit participant is now a named, bounded, logged stage (lifecycle/quit-stage.ts; CleanupRegistry tasks carry a name and a deadline; runAll reports the unfinished one), before-quit runs under a backstop so app.exit is reached on every path, and the e2e fixture bounds app.close at 30 s and attaches the whole main log with the unfinished participants named when a quit wedges. 20 of 20 repeats green where 2 of 5 hung. Two renderer terminal suites lost their timing dependence: the workspace-close file owns a frozen clock and its late-create case proves exactly one close persist with the debounce disarmed (the macOS double persist was the ordinary background save firing first); the controller suites wait for XtermHost's mount effect (bridge.attaches) instead of a controller-rendered tab, because RTL's waitFor disables the act environment and does not order against React's MessageChannel flush. Gates on the combined tree: full vex-app suite, wallet suite serial, build with VITE_VEX_SETUP_TOUR=1 and check:build, Playwright with VEX_E2E_REQUIRE_TOUR=1 and the Studio journey executed; process boundaries, type ratchet, unsafe-escapes and em-dash on the tracked tree; bridge go vet.
This was referenced Sep 27, 2026
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
3 participants
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.
(Note: this is my first PR here so apologies in advance if I missed any important steps!)
ConnectNamedPipeblocks the Node.js event loop when called before the worker thread has connected to the output pipe. This causes spawning PTYs to freeze, particularly noticeable when using a debugger (as in the original issue report) and when running parallel processes with limited CPU cores (e.g. in CI environments).This change defers the call to
conptyNative.connect()until the worker signals it's ready via the onReady callback. At that point,ConnectNamedPipereturns immediately because the client is already connected.The new tests added in this PR fail without the fix (innerPid is immediately set to a real PID) and pass with it.
Fix #763