From 1d4783cdff0ef315111c860df1bbd4b4360d3ba4 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 21 Aug 2026 16:27:56 -0700 Subject: [PATCH] feat(windows): add the daemon-level job the design called for The plan specified two nested jobs and I built one. That gap is why dropping KILL_ON_JOB_CLOSE from the per-PTY job cost the approved guarantee that a dead daemon reaps its shells -- I had one job trying to answer two questions, and the two answers conflict. They are separate jobs. The per-PTY job answers 'kill exactly this pane's tree, now', and cannot be kill-on-close because its handle is released when the shell exits, which would reap whatever the user backgrounded. The daemon assigns itself to a second job that IS kill-on-close; its handle is released only when the daemon dies. Children inherit membership, so every pty is covered and the per-PTY jobs nest inside it. Daemon, never app: an app-main crash must still leave sessions alive, which win-crash-survival-e2e asserts. Both jobs carry BREAKAWAY_OK, or a child asking to break away is refused at whichever level lacks it. Restores #9195 and #10415, which I withdrew from this PR earlier. --- config/patches/node-pty@1.1.0.patch | 50 +++++++++++++++- docs/reference/windows-process-enumeration.md | 18 ++++-- pnpm-lock.yaml | 6 +- src/main/daemon/daemon-entry.ts | 9 +++ src/main/windows/windows-pty-job.test.ts | 57 ++++++++++++++++--- src/main/windows/windows-pty-job.ts | 30 ++++++++++ 6 files changed, 152 insertions(+), 18 deletions(-) diff --git a/config/patches/node-pty@1.1.0.patch b/config/patches/node-pty@1.1.0.patch index 5e08964bf4d..da7d8e559dc 100644 --- a/config/patches/node-pty@1.1.0.patch +++ b/config/patches/node-pty@1.1.0.patch @@ -430,7 +430,7 @@ index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..383df0c9c48355547c65e6c9bbba593d } #endif diff --git a/src/win/conpty.cc b/src/win/conpty.cc -index 7b286d3d644c26141df516929703aa6e129df4b2..17c5eb23519ef7771a288ae4bc30a2be49f6c7bc 100644 +index 7b286d3d644c26141df516929703aa6e129df4b2..d63bf76070efae27043efbe1d2ccaafdc2ede037 100644 --- a/src/win/conpty.cc +++ b/src/win/conpty.cc @@ -18,6 +18,7 @@ @@ -589,7 +589,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..17c5eb23519ef7771a288ae4bc30a2be // Close the thread handle to avoid resource leak CloseHandle(piClient.hThread); -@@ -567,6 +657,94 @@ static Napi::Value PtyKill(const Napi::CallbackInfo& info) { +@@ -567,6 +657,137 @@ static Napi::Value PtyKill(const Napi::CallbackInfo& info) { return env.Undefined(); } @@ -680,16 +680,60 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..17c5eb23519ef7771a288ae4bc30a2be + } + return env.Null(); +} ++ ++/** ++ * Orca: put THIS process in a kill-on-close job, so its whole descendant tree ++ * dies with it. ++ * ++ * Why here and not per-pty: a per-pty job cannot carry KILL_ON_JOB_CLOSE, ++ * because its handle is released when the shell exits and that would reap ++ * whatever the user had backgrounded. This job's handle is released only when ++ * the process itself dies, so it reaps a crashed host without changing what a ++ * clean shell exit means. Children inherit job membership, so every pty the ++ * caller later spawns is covered without further work, and the per-pty jobs ++ * simply nest inside this one. ++ * ++ * The handle is deliberately never closed: it must outlive every caller, and ++ * process teardown is what releases it. ++ */ ++static Napi::Value PtyAssignCurrentProcessToJob(const Napi::CallbackInfo& info) { ++ Napi::Env env(info.Env()); ++ Napi::HandleScope scope(env); ++ ++ static HANDLE hHostJob = nullptr; ++ if (hHostJob != nullptr) { ++ return Napi::Boolean::New(env, true); ++ } ++ ++ HANDLE job = CreateJobObjectW(nullptr, nullptr); ++ if (job == nullptr) { ++ return Napi::Boolean::New(env, false); ++ } ++ JOBOBJECT_EXTENDED_LIMIT_INFORMATION limits{}; ++ // BREAKAWAY_OK for the same reason as the per-pty job: without it a child ++ // asking for CREATE_BREAKAWAY_FROM_JOB is refused outright. ++ limits.BasicLimitInformation.LimitFlags = ++ JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE | JOB_OBJECT_LIMIT_BREAKAWAY_OK; ++ if (!SetInformationJobObject(job, JobObjectExtendedLimitInformation, &limits, sizeof(limits)) || ++ !AssignProcessToJobObject(job, GetCurrentProcess())) { ++ // An outer job that forbids nesting refuses this; the caller degrades. ++ CloseHandle(job); ++ return Napi::Boolean::New(env, false); ++ } ++ hHostJob = job; ++ return Napi::Boolean::New(env, true); ++} + /** * Init */ -@@ -577,6 +755,8 @@ Napi::Object init(Napi::Env env, Napi::Object exports) { +@@ -577,6 +798,9 @@ Napi::Object init(Napi::Env env, Napi::Object exports) { exports.Set("resize", Napi::Function::New(env, PtyResize)); exports.Set("clear", Napi::Function::New(env, PtyClear)); exports.Set("kill", Napi::Function::New(env, PtyKill)); + exports.Set("terminateJob", Napi::Function::New(env, PtyTerminateJob)); + exports.Set("listJobProcessIds", Napi::Function::New(env, PtyListJobProcessIds)); ++ exports.Set("assignCurrentProcessToJob", Napi::Function::New(env, PtyAssignCurrentProcessToJob)); return exports; }; diff --git a/docs/reference/windows-process-enumeration.md b/docs/reference/windows-process-enumeration.md index ceacb109eff..e86c32d7ef9 100644 --- a/docs/reference/windows-process-enumeration.md +++ b/docs/reference/windows-process-enumeration.md @@ -111,10 +111,20 @@ running, so typing `exit` in a pane reaped a `start /b` server that used to survive. The job exists to make an *explicit* teardown exact, not to redefine what a clean exit means. -That means reaping a dead daemon's shells (#9195, #10415) is **not** delivered -by this job. It needs a separate daemon-level job that the daemon assigns -itself to at startup; children inherit job membership, so its closure on daemon -death reaps the shells without touching clean-exit semantics. +Reaping a dead daemon's shells (#9195, #10415) is therefore a **second, nested +job**, not this one. The terminal daemon assigns itself to a kill-on-close job +at startup (`assignHostProcessToKillOnCloseJob`); children inherit membership, +so every pty is covered and the per-PTY jobs nest inside it. Its handle is +released only when the daemon process dies, so a crashed daemon reaps its tree +without changing what a clean shell exit means. + +The split is the point. One job answers "kill exactly this pane's tree, now"; +the other answers "do not strand anything if the host dies". Trying to get both +from one job is what reaped users' backgrounded work on a clean `exit`. + +It belongs to the daemon and never to the app: an app-main crash must still +leave sessions alive, which `.github/workflows/win-crash-survival-e2e.yml` +asserts. Once the shell exits, node-pty drops its handle record and closes the job, so a terminated tree reports `null` rather than `[]`. Null means *unverifiable* in diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 172f66681cc..15d5350e40f 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -27,7 +27,7 @@ patchedDependencies: hash: 7333b3837f80a7fbd045964db6d76ba4fc118e49134bdbabb00585b6b7b60673 path: config/patches/lint-staged@16.4.0.patch node-pty@1.1.0: - hash: b328ea7c778420b4828f758f2e7a49ce345fc6260e5d07e75ff4751a6cd78957 + hash: 080d3e9d9fcd3b28a7e9a41cc93d9d761d8a57302ec4f5c4a7600d32d265548b path: config/patches/node-pty@1.1.0.patch importers: @@ -69,7 +69,7 @@ importers: version: 3.3.1 node-pty: specifier: ^1.1.0 - version: 1.1.0(patch_hash=b328ea7c778420b4828f758f2e7a49ce345fc6260e5d07e75ff4751a6cd78957) + version: 1.1.0(patch_hash=080d3e9d9fcd3b28a7e9a41cc93d9d761d8a57302ec4f5c4a7600d32d265548b) posthog-node: specifier: ^5.33.3 version: 5.33.3 @@ -12046,7 +12046,7 @@ snapshots: node-int64@0.4.0: {} - node-pty@1.1.0(patch_hash=b328ea7c778420b4828f758f2e7a49ce345fc6260e5d07e75ff4751a6cd78957): + node-pty@1.1.0(patch_hash=080d3e9d9fcd3b28a7e9a41cc93d9d761d8a57302ec4f5c4a7600d32d265548b): dependencies: node-addon-api: 7.1.1 diff --git a/src/main/daemon/daemon-entry.ts b/src/main/daemon/daemon-entry.ts index addfa50b16a..440ed34d225 100644 --- a/src/main/daemon/daemon-entry.ts +++ b/src/main/daemon/daemon-entry.ts @@ -10,6 +10,7 @@ import { readFileSync } from 'node:fs' import { startDaemon, type DaemonHandle } from './daemon-main' import { createPtySubprocess } from './pty-subprocess' import { warmWindowsConptyOnce } from './windows-conpty-warmup' +import { assignHostProcessToKillOnCloseJob } from '../windows/windows-pty-job' import { warmPwshAvailabilityCache } from '../pwsh' import { createDaemonFileLog, createNoopDaemonFileLog } from './daemon-file-log' import { PROTOCOL_VERSION } from './types' @@ -125,6 +126,14 @@ async function main(): Promise { // Fail-open: a broken log path must never block daemon startup. const daemonLog = logFilePath ? createDaemonFileLog(logFilePath) : createNoopDaemonFileLog() daemonLog.log('startup', { protocolVersion: PROTOCOL_VERSION, socketPath }) + if (process.platform === 'win32') { + // Why here and not in the app: an app-main crash must still leave sessions + // alive. A daemon death reaping its shells is the intended change -- CLI + // trees used to keep running detached after the daemon died (#9195, + // #10415). Children inherit job membership, so every pty spawned later is + // covered and the per-pty jobs nest inside this one. + daemonLog.log('host-job', { assigned: assignHostProcessToKillOnCloseJob() }) + } void warmPwshAvailabilityCache() // Why: detached daemons destroy stderr, so the preflight's console.warn is lost; diff --git a/src/main/windows/windows-pty-job.test.ts b/src/main/windows/windows-pty-job.test.ts index 7fcfdba455d..8fed2175199 100644 --- a/src/main/windows/windows-pty-job.test.ts +++ b/src/main/windows/windows-pty-job.test.ts @@ -2,6 +2,7 @@ import { afterEach, describe, expect, it, vi } from 'vitest' import type { IPty } from 'node-pty' import { __setConptyJobNativeForTests, + assignHostProcessToKillOnCloseJob, isPtyJobOwnershipAvailable, listPtyJobProcessIds, terminatePtyJob @@ -16,7 +17,11 @@ afterEach(() => { describe('terminatePtyJob', () => { it('terminates the job for a pty that has one', () => { const terminateJob = vi.fn().mockReturnValue(true) - __setConptyJobNativeForTests(() => ({ terminateJob, listJobProcessIds: vi.fn() })) + __setConptyJobNativeForTests(() => ({ + terminateJob, + listJobProcessIds: vi.fn(), + assignCurrentProcessToJob: vi.fn().mockReturnValue(true) + })) expect(terminatePtyJob(ptyWithHandle(7))).toBe('terminated') expect(terminateJob).toHaveBeenCalledWith(7, 4242) @@ -31,7 +36,8 @@ describe('terminatePtyJob', () => { // agent tree was declared dead and left holding the worktree open. __setConptyJobNativeForTests(() => ({ terminateJob: vi.fn().mockReturnValue(nativeResult), - listJobProcessIds: vi.fn() + listJobProcessIds: vi.fn(), + assignCurrentProcessToJob: vi.fn().mockReturnValue(true) })) expect(terminatePtyJob(ptyWithHandle(7))).toBe('unavailable') }) @@ -42,7 +48,8 @@ describe('terminatePtyJob', () => { ])('reports unavailable for %s', (_case, id) => { __setConptyJobNativeForTests(() => ({ terminateJob: vi.fn().mockReturnValue(true), - listJobProcessIds: vi.fn() + listJobProcessIds: vi.fn(), + assignCurrentProcessToJob: vi.fn().mockReturnValue(true) })) expect(terminatePtyJob(ptyWithHandle(id))).toBe('unavailable') }) @@ -52,7 +59,11 @@ describe('terminatePtyJob', () => { // in the same field, so an id alone can name a live ConPTY pane. The native // side refuses on a pid mismatch; this pins that we always send the pid. const terminateJob = vi.fn().mockReturnValue(true) - __setConptyJobNativeForTests(() => ({ terminateJob, listJobProcessIds: vi.fn() })) + __setConptyJobNativeForTests(() => ({ + terminateJob, + listJobProcessIds: vi.fn(), + assignCurrentProcessToJob: vi.fn().mockReturnValue(true) + })) terminatePtyJob(ptyWithHandle(3, 9001)) expect(terminateJob).toHaveBeenCalledWith(3, 9001) @@ -61,7 +72,8 @@ describe('terminatePtyJob', () => { it('reports unavailable when the shell pid is unusable', () => { __setConptyJobNativeForTests(() => ({ terminateJob: vi.fn().mockReturnValue(true), - listJobProcessIds: vi.fn() + listJobProcessIds: vi.fn(), + assignCurrentProcessToJob: vi.fn().mockReturnValue(true) })) expect(terminatePtyJob(ptyWithHandle(3, 0))).toBe('unavailable') }) @@ -78,7 +90,8 @@ describe('terminatePtyJob', () => { terminateJob: vi.fn().mockImplementation(() => { throw new Error('handle closed') }), - listJobProcessIds: vi.fn() + listJobProcessIds: vi.fn(), + assignCurrentProcessToJob: vi.fn().mockReturnValue(true) })) expect(terminatePtyJob(ptyWithHandle(7))).toBe('unavailable') }) @@ -91,7 +104,8 @@ describe('listPtyJobProcessIds', () => { // sees it. Job membership does. __setConptyJobNativeForTests(() => ({ terminateJob: vi.fn(), - listJobProcessIds: vi.fn().mockReturnValue([107184, 91480]) + listJobProcessIds: vi.fn().mockReturnValue([107184, 91480]), + assignCurrentProcessToJob: vi.fn().mockReturnValue(true) })) expect(listPtyJobProcessIds(ptyWithHandle(1))).toEqual([107184, 91480]) }) @@ -106,8 +120,35 @@ describe('listPtyJobProcessIds', () => { __setConptyJobNativeForTests(() => ({ terminateJob: vi.fn(), - listJobProcessIds: vi.fn().mockReturnValue(null) + listJobProcessIds: vi.fn().mockReturnValue(null), + assignCurrentProcessToJob: vi.fn().mockReturnValue(true) })) expect(listPtyJobProcessIds(ptyWithHandle(1))).toBeNull() }) }) + +describe('assignHostProcessToKillOnCloseJob', () => { + it('reports whether the host process is now kill-on-close', () => { + const assignCurrentProcessToJob = vi.fn().mockReturnValue(true) + __setConptyJobNativeForTests(() => ({ + terminateJob: vi.fn(), + listJobProcessIds: vi.fn(), + assignCurrentProcessToJob + })) + expect(assignHostProcessToKillOnCloseJob()).toBe(true) + }) + + it('degrades to false when the OS refuses the assignment', () => { + // An outer job that forbids nesting; orphan behaviour is then simply what + // it was before, rather than a crash. + __setConptyJobNativeForTests(() => ({ + terminateJob: vi.fn(), + listJobProcessIds: vi.fn(), + assignCurrentProcessToJob: vi.fn().mockReturnValue(false) + })) + expect(assignHostProcessToKillOnCloseJob()).toBe(false) + + __setConptyJobNativeForTests(() => null) + expect(assignHostProcessToKillOnCloseJob()).toBe(false) + }) +}) diff --git a/src/main/windows/windows-pty-job.ts b/src/main/windows/windows-pty-job.ts index 5fe556e34f8..fad91abec2e 100644 --- a/src/main/windows/windows-pty-job.ts +++ b/src/main/windows/windows-pty-job.ts @@ -29,6 +29,7 @@ const requireFromMain = createRequire(__filename) type ConptyNative = { terminateJob: (id: number, shellPid: number) => boolean listJobProcessIds: (id: number, shellPid: number) => number[] | null + assignCurrentProcessToJob: () => boolean } let cachedNative: ConptyNative | null | undefined @@ -126,6 +127,35 @@ export function listPtyJobProcessIds(proc: IPty): readonly number[] | null { } } +/** + * Put this process in a kill-on-close job so its descendants die with it. + * + * Why this is separate from the per-PTY jobs: those cannot carry + * KILL_ON_JOB_CLOSE, because their handle is released when the shell exits and + * that would reap whatever the user had backgrounded. This one is released only + * when the process itself dies, so it reaps a crashed host without changing + * what a clean exit means. Children inherit membership, so the per-PTY jobs + * nest inside it and every pty is covered. + * + * Call it from the terminal daemon, not from the app: an app-main crash must + * still leave sessions alive, which is what win-crash-survival-e2e asserts. A + * daemon death reaping its shells is the intended change (#9195, #10415). + * + * Returns false when the OS refuses -- an outer job that forbids nesting -- in + * which case orphan behaviour is simply what it was before. + */ +export function assignHostProcessToKillOnCloseJob(): boolean { + const native = nativeLoader() + if (!native) { + return false + } + try { + return native.assignCurrentProcessToJob() + } catch { + return false + } +} + /** Whether this build can own PTY trees with job objects at all. */ export function isPtyJobOwnershipAvailable(): boolean { return nativeLoader() !== null