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.
This commit is contained in:
Neil
2026-09-04 06:00:12 -07:00
parent a5c6f402f4
commit 2cdcc3836c
@@ -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 —