From 2cdcc3836c85aa78e9459bf7b90fc9fcf0e2680e Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 4 Sep 2026 06:00:12 -0700 Subject: [PATCH] test(pty): make the F24 patch pins catch the regressions they name Two of the pins added in #18635 did not discriminate. Found by review of the merged change; both are test-only defects, the fix itself is unaffected. `resolves the ConPTY DLL before it claims the close` searched the whole patch for `HANDLE hLibrary = LoadConptyDll(info, useConptyDll);`. That line occurs twice -- PtyConnect's copy comes first -- so indexOf always matched PtyConnect, and the ordering assertion held no matter where PtyKill resolved the DLL. Verified by simulation: moving PtyKill's resolve back below the claim left the suite green. `reaches hShell only under the null check` used a marker as a slice END bound without checking it existed. If that marker vanished the slice ran to the end of the patch, the stray-line filter found nothing, and the test passed silently. Both now anchor inside the PtyKill hunk only, located by its header's function context rather than line numbers. `indexIn` throws on a missing marker instead of returning -1, so a marker that moves fails the assertion that depends on it rather than making it vacuous. Adds the pin that was missing entirely: PtyKill's half of the two-sided baton free. Without it a self-exit followed by kill() -- the ordinary pane close -- leaks one baton and one entry in the vector get_pty_baton scans linearly. Mutation-tested rather than only revert-tested, because wholesale reverting the patch is what hid this: it fails every assertion for the trivial reason that nothing matches. Simulating each specific regression instead: - move PtyKill's DLL resolve below the claim -> 1 failed (was: 0) - drop PtyKill's baton free, line-count-neutral -> 2 failed (was: 0) Wholesale revert still fails all 9. Refs F24. --- ...-pty-self-exit-pseudoconsole-close.test.ts | 64 ++++++++++++++++--- 1 file changed, 55 insertions(+), 9 deletions(-) diff --git a/src/main/pty/node-pty-self-exit-pseudoconsole-close.test.ts b/src/main/pty/node-pty-self-exit-pseudoconsole-close.test.ts index 5f19772530a..1f9cae275c3 100644 --- a/src/main/pty/node-pty-self-exit-pseudoconsole-close.test.ts +++ b/src/main/pty/node-pty-self-exit-pseudoconsole-close.test.ts @@ -46,6 +46,35 @@ import { describe, expect, it } from 'vitest' const PATCH = readFileSync(join(__dirname, '../../../config/patches/node-pty@1.1.0.patch'), 'utf8') +/** + * Just the `PtyKill` hunk. Several markers below also occur in the `PtyConnect` + * hunk above it, and a bare `indexOf` on the whole patch silently matched the + * wrong one — an assertion that then held regardless of what `PtyKill` did. + */ +const ptyKillHunk = (() => { + // Anchored on the hunk header's function context rather than its line + // numbers, which shift whenever anything above it in the patch changes. + const header = /^@@ .* @@ static Napi::Value PtyKill\(.*$/m.exec(PATCH) + if (!header) { + throw new Error('no PtyKill hunk in config/patches/node-pty@1.1.0.patch') + } + const from = header.index + const next = PATCH.indexOf('\n@@ ', from + 1) + return PATCH.slice(from, next === -1 ? undefined : next) +})() + +/** + * `indexOf` that throws instead of returning -1. A missing marker must fail the + * assertion that depends on it, not quietly make a slice or comparison vacuous. + */ +function indexIn(haystack: string, marker: string): number { + const at = haystack.indexOf(marker) + if (at === -1) { + throw new Error(`marker not found in the PtyKill hunk: ${marker}`) + } + return at +} + describe('node-pty patch: pseudoconsole close on the self-exit path', () => { it('does not let the exit watcher free the baton while the close is still owed', () => { // Pinned as one block: the erase must stay INSIDE the consoleClosed guard. @@ -73,10 +102,15 @@ describe('node-pty patch: pseudoconsole close on the self-exit path', () => { // LoadConptyDll throws when conpty.dll is missing. Throwing after // consoleClosed was set would strand the pseudoconsole for good: the retry // finds the work claimed and does nothing. - const dllResolve = PATCH.indexOf('+ HANDLE hLibrary = LoadConptyDll(info, useConptyDll);') - const claim = PATCH.indexOf('+ handle->consoleClosed = true;') - expect(dllResolve).toBeGreaterThan(-1) - expect(claim).toBeGreaterThan(-1) + // + // Anchored inside PtyKill, not by a bare indexOf: the identical line also + // appears in the PtyConnect hunk, earlier in the file, and matching that one + // made this assertion pass no matter where PtyKill resolved the DLL. + const dllResolve = indexIn( + ptyKillHunk, + '+ HANDLE hLibrary = LoadConptyDll(info, useConptyDll);' + ) + const claim = indexIn(ptyKillHunk, '+ handle->consoleClosed = true;') expect(dllResolve).toBeLessThan(claim) }) @@ -84,11 +118,9 @@ describe('node-pty patch: pseudoconsole close on the self-exit path', () => { // Pinned as one block. The watcher nulls hShell on exit, and upstream // dereferenced it unconditionally; every remaining use — the duplication and // the failure fallback below it — must stay inside this guard. - const guarded = PATCH.slice( - PATCH.indexOf('+ if (useConptyDll && handle->hShell != nullptr) {'), - PATCH.indexOf('+ if (handle->shellExited) {') - ) - expect(guarded).not.toBe('') + const start = indexIn(ptyKillHunk, '+ if (useConptyDll && handle->hShell != nullptr) {') + const end = indexIn(ptyKillHunk, '+ if (handle->shellExited) {') + const guarded = ptyKillHunk.slice(start, end) expect(guarded).toContain('DuplicateHandle(GetCurrentProcess(), handle->hShell') expect(guarded).toContain('TerminateProcess(handle->hShell, 1);') // No ADDED line outside that guard may terminate through hShell. Removed @@ -99,6 +131,20 @@ describe('node-pty patch: pseudoconsole close on the self-exit path', () => { expect(strayAdds).toEqual([]) }) + it('frees the baton from PtyKill when the shell has already exited', () => { + // The other half of the two-sided handshake. Without it a self-exit followed + // by kill() — the ordinary pane close — leaks one baton and one entry in the + // vector get_pty_baton scans linearly, forever. + expect(ptyKillHunk).toContain( + [ + '+ if (handle->shellExited) {', + '+ const bool removed = remove_pty_baton(id);', + '+ assert(removed);', + '+ (void)removed;' + ].join('\n') + ) + }) + it('still kills the shell when DuplicateHandle fails', () => { // A null hShellDup is indistinguishable from the self-exit case, so a // swallowed failure would leave the shell running after its pane closed —