mirror of
https://github.com/stablyai/orca.git
synced 2026-10-23 08:03:39 +00:00
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.
This commit is contained in:
@@ -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;
|
||||
};
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
Generated
+3
-3
@@ -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
|
||||
|
||||
|
||||
@@ -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<void> {
|
||||
// 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;
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user