fix(windows): assign spawned shells to a kill-on-exit Job Object

[Sora-bluesky]

Fixes #69033.

Problem

On Windows, shell subprocesses spawned by the terminal tool are orphaned when the Hermes parent exits (session end / TUI restart / crash). Users see 20+ leaked bash.exe / find.exe / grep.exe / head.exe accumulating across sessions, one with ~8 CPU-hours before being noticed. The root cause is that start_new_session=True (POSIX setsid) is a silent no-op on Windows, every shell-spawn site in the codebase (_popen_bash for docker/ssh/singularity, local.py for the local backend, and even the slash-worker/MCP paths cited in the issue as "already solved") lacks real subtree cleanup on ungraceful parent exit.

Note that the issue's attribution to _popen_bash alone is incomplete, the local backend spawns directly in local.py (bypassing _popen_bash, as base.py's docstring notes), so both call sites need the fix.

Fix

A process-wide Windows Job Object with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE. When Hermes exits, cleanly or via SIGKILL, the OS closes the job handle and every process assigned to it dies.

Getting this right on Windows required more work than the POSIX equivalent:

1. Assign atomically, not after Popen returns. Handle-only assignment after Popen() is racy for fast native pipelines: bash can start find.exe | grep.exe | head.exe before the parent thread runs AssignProcessToJobObject, and job membership is inherited only by children created AFTER assignment. Fix: CREATE_SUSPENDED → assign → ResumeThread, so the child structurally cannot execute until it's already in the job. subprocess.Popen closes the thread handle internally before returning, so we get the handle by installing a wrapper over subprocess._winapi.CreateProcess.

2. Install the wrapper once, never restore it. Dynamic patch/restore under a lock was our first attempt; adversarial review found it leaked stale patches under thread interleaving (capture-before-lock → wrong restore) and swept unrelated concurrent subprocess.Popen calls from other libraries into our job. Fix: install exactly once via double-checked locking, and make the wrapper inert by default, it only suspends/assigns/resumes when the CALLING thread's threading.local (_spawn_owner.job) is set, which spawn_bash_with_kill_on_exit sets around one popen_fn() call. Unrelated threads pass straight through to the real CreateProcess.

3. Singleton Job Object under double-checked locking. Two first callers racing on _get_kill_on_exit_job() could each create a job, publish one, and let the other be GC'd, closing its handle immediately kills whichever shell was just assigned to the loser. Fixed with a threading.Lock around create-and-publish.

4. Fail open, but visibly. Job assignment failures (unavailable pywin32, nested-job restrictions, WinError from an outer restrictive job) never block the spawn, the terminal keeps working. But the first failure logs a WARNING with the WinError code, so the fix isn't silently disabled on any given install.

5. Correct cleanup on ResumeThread failure. If ResumeThread fails after assignment, TerminateProcess runs on the still-suspended child before we re-raise, so it can't hang forever with pipes attached. CPython's _winapi returns raw int handles rather than PyHANDLE objects, so cleanup uses _winapi.CloseHandle(int(h)), a naive .Close() would raise AttributeError and leak both handles.

Applied at both spawn sites via a shared helper (spawn_bash_with_kill_on_exit), so _popen_bash (docker/ssh/singularity) and local.py cannot drift.

Known limitations

Documented honestly rather than papered over:

• Trace-hook publication window. During the one-time install, there is a millisecond window between publishing the original CreateProcess capture and installing the wrapper. A trace hook interleaving between those two operations could let a concurrent first-caller spawn through the real function. This does not affect the fix's goal (orphan cleanup on Hermes exit still fires for every shell installed thereafter), and the install happens once at first use.
• Single-level thread-local gate. _spawn_owner.job is set/cleared around one call; the only caller (spawn_bash_with_kill_on_exit) does not nest. A hypothetical reentrant spawn from a guarded audit hook would either clear the outer owner (later outer spawns escape) or be swept into the job. Documented in the docstring, extend to a stack if a nested caller ever appears.

Tests

22 passed in tests/test_windows_terminal_kill_on_exit_job.py, including:

• Real integration: parent spawns a child via the wrapper, child immediately spawns a grandchild, close the job handle, poll, both die within 5s (Windows-gated).
• Concurrent singleton first-callers with threading.Barrier, RED-verified against an unlocked reimplementation (produced 2 jobs, dropped to 1 with the fix).
• Unrelated thread's subprocess.Popen survives when we close our job handle, proves the thread-local gate isolates us from other libraries.
• importlib.reload safety: install → reload → install again, no recursion (real repro, RED-verified against the unmarked wrapper).
• ResumeThread failure path terminates the child, closes both handles via _winapi.CloseHandle, warns on TerminateProcess failure, re-raises the original error.
• pywin32-unavailable path emits the one-time warning.

Also: 4 rounds of adversarial review (each catching a real defect I had missed) shaped the design, the review trail is why the final shape is atomic and thread-isolated rather than the naive "assign after Popen" I started with.

The bash-native equivalent E2E was investigated and dropped: MSYS reports internal $$ / $! PIDs that don't match the Win32 ProcessId that Popen or WMIC see, making deterministic pid tracking impractical. The Python-Python E2E exercises the identical CreateProcess + Job-inheritance mechanism.

I corrected the issue's root-cause attribution (_popen_bash isn't the only path, local.py also spawns directly), verified pywin32 is a declared win32 dependency, and confirmed gateway.py already uses Job Objects for the opposite purpose (CREATE_BREAKAWAY_FROM_JOB, detach-and-survive), so this is a genuinely new use of the primitive rather than a copy of an existing pattern.

Reporter's item 3 (startup orphan-sweep) is a good follow-up but is genuinely separate, this PR closes the source of the leak; a sweeper for legacy accumulated orphans from prior Hermes versions can land later.

[Sora-bluesky]

Rebased onto main. One conflict, in tools/environments/base.py.

main added encoding="utf-8", errors="replace" to the same subprocess.Popen call this PR wraps in spawn_bash_with_kill_on_exit, so both changes ended up on the same call. I kept both. Nothing else in the range changed, and git range-diff shows the other commits identical. The contributors/emails commit is gone because that mapping is already in main.

I also added three assertions to test_popen_bash_uses_kill_on_exit_wrapper. It checked that the spawn goes through the wrapper, but not that the call still passes text, encoding and errors, so this merge could have dropped main's change without any test failing.

[teknium1]

Thanks for the detailed Windows-specific design and the real Job Object coverage. The foreground/local and shared backend premise is still present on current main.

Problems

• terminal(background=true) has a separate local shell path: tools/terminal_tool.py:2552 calls ProcessRegistry.spawn_local, whose direct shell Popen(..., start_new_session=True) remains at tools/process_registry.py:780-792. Since this PR changes only tools/environments/base.py and tools/environments/local.py, that Windows shell can still survive an ungraceful Hermes exit. Please cover this path, and account for the separate winpty branch at tools/process_registry.py:726-762.
• tests/test_windows_terminal_kill_on_exit_job.py:707 uses inspect.getsource() and a substring assertion. AGENTS.md:1381-1385 bans source-reading tests; this check does not prove _run_bash routes its Popen through the wrapper.

Suggested changes

• Add behavioral coverage for the background local spawn and replace the source inspection with a mocked-Popen/wrapper contract test that executes LocalEnvironment._run_bash.

Automated hermes-sweeper review.

[teknium1]

Please extend this cleanup mechanism to ProcessRegistry.spawn_local as well. terminal(background=true) routes local work there (tools/terminal_tool.py:2552), and its direct login-shell Popen(..., start_new_session=True) remains at tools/process_registry.py:780-792, leaving the reported crash-orphan class unfixed for background shells.

[Sora-bluesky]

Partly done in 8f74aaaea, pushed the same day as this review — sorry for leaving the thread open, the change went up without a reply here.

The Popen you pointed at is now wrapped in spawn_bash_with_kill_on_exit(...) (tools/process_registry.py:780-796), with start_new_session=True kept inside the wrapper at :794, and the comment above it cites this review so the reason survives the next refactor.

What that does not cover: spawn_local's use_pty=True branch spawns through _PtyProcessCls.spawn and returns at :762, before reaching the wrapped call. So the PTY path is still outside the job object and the crash-orphan class you described survives there. I would rather say that than let the thread close on a half fix — do you want the PTY path brought into this PR, or split out?

[teknium1]

This is a banned source-inspection test (AGENTS.md:1381-1385) and only proves the helper name exists somewhere in the module. Replace it with a behavioral test that executes LocalEnvironment._run_bash while spying on the wrapper/Popen contract.

[Sora-bluesky]

Confirmed: spawn_local's non-PTY shell called subprocess.Popen directly, and the job patch is gated per-thread, so that spawn never entered the kill-on-exit job. Fixed in 8f74aaaea by routing it through spawn_bash_with_kill_on_exit (no-op on POSIX; start_new_session stays for the POSIX cleanup story). A call-contract regression pins it, same style as the base.py one — it fails if the wrapper is bypassed. 125 passed on the two suites; the 4 failures in test_process_registry.py are identical on the parent commit, so they predate this change.

One honest scope note: the PTY branch spawns through winpty, which bypasses Python's subprocess.Popen and the patched subprocess._winapi.CreateProcess, so it stays outside this mechanism.

[Sora-bluesky]

Thanks for the follow-up.

Two notes on the current head (8f74aaaea):

The source-inspection finding still applies. test_local_backend_run_bash_uses_kill_on_exit_wrapper still calls inspect.getsource() at line 707. The separate regression added for the non-PTY background path is test_spawn_local_background_shell_uses_kill_on_exit_wrapper, which checks that ProcessRegistry.spawn_local(..., use_pty=False) calls spawn_bash_with_kill_on_exit and uses the returned process. That one does not replace the source-reading test, so I still need to swap it for behavioral coverage. My earlier comment here was wrong about that.

The winpty PTY branch also remains outside this mechanism. As I noted in the PR, it bypasses Python's subprocess.Popen and the patched subprocess._winapi.CreateProcess.

[Sora-bluesky]

Replaced in a8f5d1c2a. The test now runs LocalEnvironment._run_bash with spawn_bash_with_kill_on_exit and subprocess.Popen mocked, and asserts the spawn goes through the wrapper and the Popen kwargs survive (text/encoding/errors/start_new_session/stdout/stderr/stdin).

I checked two mutations against it: bypassing the wrapper fails the test, and dropping encoding="utf-8" from the Popen call fails it too. The old source check could only catch the first.

[GottZ]

This was generated by AI during triage.

Summary

One PR addresses issue #69033. #69076 adds atomic Windows Job Object assignment for the shared container/remote, foreground-local, and non-PTY background shell paths, while the documented winpty PTY path remains outside the mechanism.

Related pull requests

• fix(windows): assign spawned shells to a kill-on-exit Job Object #69076 best fix — (+1353/-24) — n/a: The diff routes the Popen-based shell paths through a suspend/assign/resume wrapper backed by JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE and adds behavioral contract and Windows integration tests. Consistent with the keep_open review on fix(windows): assign spawned shells to a kill-on-exit Job Object #69076, its background-path and source-inspection objections are addressed in the visible diff, but the contributor-documented winpty PTY scope remains unresolved.

Suggested consolidation

Keep #69076 open with a salvage path: retain the Job Object implementation and coverage for the shared backend, foreground-local, and non-PTY background paths, while requiring an explicit design or scope resolution for the winpty PTY branch before further disposition.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I69033(["issue #69033 (open)"])
    P69076["PR #69076 (open)"]
    P69076 -->|best fix| I69033
    class I69033 open
    class P69076 open
    class P69076 best
    class P69076 target
    click I69033 "https://github.com/NousResearch/hermes-agent/issues/69033"
    click P69076 "https://github.com/NousResearch/hermes-agent/pull/69076"

Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 1 pull request and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 62 kB of PR diffs, 12 kB of issue/PR text, 6 kB of discussion (8 comments), 3 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.