mirror of
https://github.com/stablyai/orca.git
synced 2026-10-23 08:03:39 +00:00
fix(windows): stop a clean shell exit from reaping backgrounded processes
Measured on Windows 11: with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE on the per-PTY job, releasing the handle when the shell exits also killed whatever the user had backgrounded. Typing 'exit' in a pane reaped a detached server that survived before this patch. That is a behaviour change nobody asked for. The approved change was that killing the terminal daemon reaps its shells -- not that a clean exit reaps your background job. The job's purpose is to make an EXPLICIT teardown exact, which TerminateJobObject still does. Reaping a dead daemon's shells now needs the daemon-level job the design called for: the daemon assigns itself, children inherit membership, and its closure on daemon death reaps them without touching clean-exit semantics. Not in this PR; noted in the reference doc.
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..4ae9ddec6ac3e9fdaa380000d3017bbb394f22d6 100644
|
||||
index 7b286d3d644c26141df516929703aa6e129df4b2..a1a63d6380ba5b0a669339435064b9f51e0706dc 100644
|
||||
--- a/src/win/conpty.cc
|
||||
+++ b/src/win/conpty.cc
|
||||
@@ -46,6 +46,11 @@ struct pty_baton {
|
||||
@@ -449,9 +449,9 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4ae9ddec6ac3e9fdaa380000d3017bbb
|
||||
GetExitCodeProcess(baton->hShell, (LPDWORD)(&exit_event->exit_code));
|
||||
// Clean up handles
|
||||
CloseHandle(baton->hShell);
|
||||
+ // Orca: closing the last job handle is what reaps whatever the shell left
|
||||
+ // behind. Without this the job outlives the pane and its descendants keep
|
||||
+ // running (and keep holding the worktree directory open).
|
||||
+ // Orca: release the job once the shell is gone. Without kill-on-close this
|
||||
+ // only frees the handle -- anything the user backgrounded is orphaned, as
|
||||
+ // it was before this patch.
|
||||
+ if (baton->hJob != nullptr) {
|
||||
+ CloseHandle(baton->hJob);
|
||||
+ baton->hJob = nullptr;
|
||||
@@ -480,14 +480,14 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..4ae9ddec6ac3e9fdaa380000d3017bbb
|
||||
+ // descendant that reparented, which is why detached agent children outlived
|
||||
+ // their pane and held the worktree directory open.
|
||||
+ //
|
||||
+ // JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE means the tree also dies if this host
|
||||
+ // process dies without unwinding, so a crashed daemon cannot strand shells.
|
||||
+ // Deliberately WITHOUT JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE. Measured on
|
||||
+ // Windows 11: with that flag, closing the handle when the shell exits also
|
||||
+ // kills whatever the user left running, so typing `exit` in a pane reaped a
|
||||
+ // `start /b` server that used to survive. This job exists to make an
|
||||
+ // EXPLICIT teardown exact, not to redefine what a clean exit means.
|
||||
+ HANDLE hJob = CreateJobObjectW(nullptr, nullptr);
|
||||
+ if (hJob != nullptr) {
|
||||
+ JOBOBJECT_EXTENDED_LIMIT_INFORMATION jobLimits{};
|
||||
+ jobLimits.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE;
|
||||
+ if (!SetInformationJobObject(hJob, JobObjectExtendedLimitInformation, &jobLimits, sizeof(jobLimits)) ||
|
||||
+ !AssignProcessToJobObject(hJob, piClient.hProcess)) {
|
||||
+ if (!AssignProcessToJobObject(hJob, piClient.hProcess)) {
|
||||
+ // Why tolerate failure: an outer job without JOB_OBJECT_LIMIT_BREAKAWAY_OK
|
||||
+ // (some EDR and container hosts) refuses the assignment. The pty must
|
||||
+ // still start; ownership just degrades to the older best-effort path.
|
||||
|
||||
@@ -104,11 +104,17 @@ parent-pid walk nor `GetConsoleProcessList` sees that grandchild — it leaves
|
||||
the console and reparents, which is what left `claude.exe`/`node.exe`/`cmd.exe`
|
||||
holding worktree directories open (#9045, #10475, #10897).
|
||||
|
||||
`JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE` is set, so the tree is also reaped if the
|
||||
hosting process dies without unwinding. The job belongs to the **terminal
|
||||
daemon**, not to the app: an app-main crash leaves sessions alive (asserted by
|
||||
`.github/workflows/win-crash-survival-e2e.yml`), while a daemon death now reaps
|
||||
its shells instead of stranding them (#9195, #10415).
|
||||
The per-PTY job deliberately does **not** set
|
||||
`JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE`. Measured on Windows 11: with that flag,
|
||||
releasing the handle when the shell exits also kills whatever the user left
|
||||
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.
|
||||
|
||||
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: 8fc49f17011b6611a5b8c00e83a6f12e14e75aada2b0ef26dc5393f8376d20e8
|
||||
hash: 65d30811c4f422be2637e01f1ad25c385ddc6b59d5c90309e5656964dda192b0
|
||||
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=8fc49f17011b6611a5b8c00e83a6f12e14e75aada2b0ef26dc5393f8376d20e8)
|
||||
version: 1.1.0(patch_hash=65d30811c4f422be2637e01f1ad25c385ddc6b59d5c90309e5656964dda192b0)
|
||||
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=8fc49f17011b6611a5b8c00e83a6f12e14e75aada2b0ef26dc5393f8376d20e8):
|
||||
node-pty@1.1.0(patch_hash=65d30811c4f422be2637e01f1ad25c385ddc6b59d5c90309e5656964dda192b0):
|
||||
dependencies:
|
||||
node-addon-api: 7.1.1
|
||||
|
||||
|
||||
@@ -18,6 +18,10 @@ import { createRequire } from 'node:module'
|
||||
* A job object replaces the inference with a handle. node-pty puts each shell
|
||||
* into its own job at creation, before it can spawn anything, so membership is
|
||||
* the kernel's answer rather than ours.
|
||||
*
|
||||
* The job is not kill-on-close: it makes an explicit teardown exact, and
|
||||
* deliberately does not change what a clean shell exit means for anything the
|
||||
* user backgrounded.
|
||||
*/
|
||||
|
||||
const requireFromMain = createRequire(__filename)
|
||||
|
||||
Reference in New Issue
Block a user