Repository navigation
(terminal): move node-pty to 1.2.0-beta.15 (#409) - #431
Conversation
… race node-pty 1.1.0 lets the ConPTY exit thread erase a handle-table entry while PtyKill or PtyResize still use it, a heap corruption separate from the double close of #405. 1.2.0-beta.15 carries upstream #922, a mutex around that table. Pinned exactly because it is a pre-release. The beta still corrupts the heap on a second kill, so pty-ops' once-only kill guard stays. Closes #409
|
Adversarial review at 9cd7feb: changes requested. Blocking:
Non-blocking: Nit: Checked: lockfile moves only the node-pty entry (+ root engines already in package.json), resolved/integrity match the registry; our call sites use no changed API ( |
node-pty 1.2.0-beta.15's OpenConsole asks for the cursor position (CSI 6n) after an early resize and repeats it until answered. A hidden session's output goes to the hidden accumulator and a detached one's to main's reattach buffer, so the query waited there and xterm answered every copy on reveal or reattach. OpenConsole consumes one reply and passes the rest to the application as typed input. Both buffers now drop the query, including one split across two chunks; it is not answered late, since a replayed cursor would be stale. The context doc records the query measurements and the #922 stress runs (1.1.0 died in 5 of 8 kill runs, the beta in none). The changelog line no longer names the library version. Refs #409
|
Delta review 9cd7feb..fd1ee5e: the hidden-session and reattach replay of Evidence: the #922 race has no failing CI run (not deterministic, ~1 min per stress run); the local stress table (kill mode: 1.1.0 crashed 5/8, beta 0/8) is the evidence offered. Merging records an exception to the red-before-fix gate. One beta stress run hung on exit for 600 s, unexplained. To fix before merge:
Checked: |
…oss entries The nested re-strip removed bytes that are not a query (ESC [ ESC [6n 6n became empty, where xterm shows 6n) and was quadratic; on a crafted 64 KB chunk the recursive main-side version overflowed the stack. One pass now, behind an includes check. Main's reattach buffer also looks back across entries, so a query split over three chunks no longer survives, and only a chunk starting with [, 6 or n is checked for a split at all. The context doc says the filter runs on every platform and for any application's own query. Refs #409
|
Delta review at a85a24d: approved. Main and renderer strips checked against every one of the 262,144 ways to cut a 20-char sample into chunks (queries split over 1-4 entries, Evidence gate, maintainer decision: the #922 fix is upstream's native race fix, not deterministic, so no CI run can fail reliably before it; the local stress run (1.1.0 crashed 5/8 in kill mode, the beta 0/8) is the evidence, and the release candidate is the real check. The CSI 6n filter fixes behaviour the beta introduces in this Pr, so main had nothing red to show. |
Moves node-pty from
^1.0.0(1.1.0 installed) to the exact pre-release1.2.0-beta.15, for microsoft/node-pty#922. On Windows, the shell's exit thread could erase and free an entry of the native handle table while the main thread was still insidePtyKill/PtyResize. That is a second way, separate from #405, for the app to die from a heap corruption. 1.1.0 has no lock there; the beta takesg_ptyHandlesMutexaround every handle-table access (src/win/conpty.cc). The maintainer decided to ship the beta (npm view node-pty dist-tags: beta 1.2.0-beta.15, latest 1.1.0).The beta's OpenConsole also asks the terminal for its cursor position (
CSI 6n), which 1.1.0 never did. A hidden or detached session cannot answer, and replaying the query later makes xterm.js send a reply OpenConsole types into Claude or the shell. The second and third commits drop the query from both replay buffers.Closes #409
What changed
package.json:"node-pty": "1.2.0-beta.15", pinned, no caret.package-lock.json: thenode_modules/node-ptyentry (version, resolved, integrity; npm also dropped itslicenseline). One change outside the node-pty subtree: npm wrote the root package'sengines("node": ">=20 <23") into the lock's root entry. It was already inpackage.json; the lock had drifted.public/terminal-manager.js: the hidden accumulator keeps noCSI 6n, and the reveal replay strips any left, including a query split across chunks and one drained from the live write buffer.output-buffer.js: main's reattach buffer dropsCSI 6non the way in, including a query split across several chunks (the last three code units are checked across entries).includescheck. Removing a query can join the bytes around it into a new one (ESC [ ESC [6n 6nbecomesESC [6n); that one is kept. The filter runs on every platform and removes any application's ownCSI 6ntoo: answered live while the session is visible, unanswered while it is hidden or detached, where it used to be answered late with a stale position..ai/contexts/ipc-bridge.md: what the beta fixes and does not, the #922 stress results, and a "Cursor-position queries" section (measurements, the hidden/detached case, why the query is dropped rather than answered).Findings
Package layout (published tarballs)
prebuilds/win32-x64/conpty/andprebuilds/win32-arm64/conpty/still holdconpty.dllandOpenConsole.exe, at the pathscripts/after-pack.jscopies.conpty.ccstill loadsconpty\conpty.dllrelative toconpty.node, andlib/utils.jsstill prefersbuild/Releaseoverprebuilds/, so the afterPack hook is still needed and unchanged.useConptyis deprecated and ignored. Switchboard never selects winpty.API at our call sites (
main.jsspawnPty,pty-ops.js,remote-attach.js): onlyresize(cols, rows, pixelSize?)(optional, ignored on Windows) and theuseConptydeprecation differ.SWITCHBOARD_NO_CONPTY_DLLkeeps working.(terminal): close a pty once, and stop resizing or writing to a killed one (#405) #408's guard is still required. Killing a conpty-dll pty twice with raw
term.kill()exits 3221226356 (0xc0000374) on both 1.1.0 and the beta; throughkillPtyit exits 0 on both.#922, bounded stress attempt (scratch script, not in the suite: about a minute per run, and a pass proves nothing). One child process per run so a crash is an exit status; 20 iterations of 4
cmd.exe /c exitptys plus one long-lived pty, 300 ms of resize calls straight into the binding while the short shells exit, and in kill mode one agent-level kill per short pty at a random moment.useConptyDll, node 24:One beta kill run finished its 20 iterations and then never exited (killed after 600 s); not explained. The race is non-deterministic: the exit thread's erase has to land between the main thread's lookup and its last use of the entry, and touching freed memory only crashes when the allocator has already reused or unmapped it. A kill holds the entry across
ClosePseudoConsoleand then writes to it, a wider window than one resize.Cursor-position query (Electron 41, isolated pty,
useConptyDll)cmd.exe) or 4 (node shell); answered, sent once. A resize at 4 s or later: none. 1.1.0: none.ESC [ 24 ; 1 Rread verbatim by a raw-mode node child); 1.1.0 passes it through the same way.How it was tested
n(main, renderer); the repeated pass put back (main, renderer); split handling removed (main, renderer); strip removed in main; strip on replay removed. Theincludesfast paths change no output and are not mutation-tested.npm ciin the worktree with the new lock: exit 0. electron-builder's rebuild of node-pty failed (no Visual Studio on this machine), and the postinstall fallback rebuilt better-sqlite3 only, so node-pty runs from the beta's prebuilds.task checkagainst that install, run by the pre-commit hook on the head commit: 3205 + 119 pass, 0 fail; eslint 0 errors. In a separate run just before, two timing tests oftest/git-changes-runner.test.js(measureUntrackedLocal) failed; run alone, that file failed 1 time in 3. This change does not touch it.test/viewer-file-watch.test.jsfails on this machine wheneverTEMPis the 8.3 short path (JEAN-B~1, libuvfs-event.cassertion), at the previous head and in the main checkout too; with the long path it passes, so the hook ran with the longTEMP.conpty.node: spawn,resizePty,writePty, twokillPty(true, then false), resize after kill (false),onExit, exit 0. The app itself was not launched.Not verified here
conpty.dllunderbuild/Release/conpty/: release candidate only.