fix(process_registry): release Popen/PTY handles when a session finishes

[RGerrish]

Bug

Background processes that finish keep their OS file descriptors open until the finished-process TTL (30 min by default) elapses. Under heavy background churn the gateway process can exhaust its file descriptor budget, and new terminal(background=true) spawns start failing with a "file descriptor limit" style error.

The registry never rejects spawns — every spawn path calls _prune_if_needed() first and evicts the oldest finished entry at MAX_PROCESSES (64). The real defect is the retained-handle leak, not a registry-cap rejection.

The broken code

tools/process_registry.py, _move_to_finished():

def _move_to_finished(self, session: ProcessSession):
    with self._lock:
        was_running = self._running.pop(session.id, None) is not None
        self._finished[session.id] = session
    session._completion_event.set()
    self._write_checkpoint()
    ...

It moves the session to _finished but never closes the handles:

• Local spawns store a live subprocess.Popen on session.process with stdout=PIPE (spawn at ~line 801-815) — the parent's read-end pipe FD stays open.
• PTY spawns store a live ptyprocess.PtyProcess on session._pty (spawn at ~line 757-766) — the PTY master FD stays open.
• _prune_if_needed() (line 1914) deletes the dict entry but does not close the handles either.

So a finished session holds 1+ pipe/PTY FDs for the entire FINISHED_TTL_SECONDS window. 64 finished sessions × 1-2 FDs each can push a launchd-spawned gateway (typically a 256-FD soft limit) over the edge.

How it was found

1. A user reported the error: "The system has hit a file descriptor limit from all the background processes tracked in this session." A gateway restart cleared it; nothing else did.
2. Reviewing tools/process_registry.py showed MAX_PROCESSES never blocks spawns — _prune_if_needed() evicts oldest-finished before insert. So the registry-count rationale was wrong; the symptom pointed at retained handles instead.
3. Confirmed the leak: _move_to_finished() and _prune_if_needed() move/delete session records without touching session.process (Popen) or session._pty (PtyProcess). The pipes were only released when the dict entry finally disappeared AND GC ran — i.e. after the TTL.
4. Wrote the failing test first (red on main): tests/tools/test_process_registry.py::TestFinishedHandleRelease — proc.stdout.closed stays False after finish on main.

Reproduction

# From repo root (needs a python3 venv with pytest; run_tests.sh preferred in CI)
scripts/run_tests.sh tests/tools/test_process_registry.py::TestFinishedHandleRelease -q
# 3 of 4 tests FAIL on main — finished sessions still hold their pipe/PTY FDs

Or manually:

import subprocess, sys, time
from tools.process_registry import ProcessRegistry, ProcessSession

reg = ProcessRegistry()
proc = subprocess.Popen([sys.executable, "-c", "print('x')"],
                        stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
s = ProcessSession(id="s1", command="x", task_id="t", started_at=time.time())
s.process = proc
proc.wait(timeout=5)
reg._running[s.id] = s
reg._move_to_finished(s)
print(proc.stdout.closed)  # False on main (leak) — True with the fix

Fix

_move_to_finished() now calls _release_finished_handles(session), which closes the session's Popen stdout/stderr/stdin streams and PTY master — right after the reader loop has drained EOF. This is lossless:

• poll() / wait() / read_log() serve output from the buffered session.output_buffer — they never touch the pipe after finish.
• kill_process() / write_stdin() / send_eof() early-return on session.exited before touching handles.
• Sessions without a local Popen (env backends, detached recovery) are a safe no-op.

Tests

4 new cases in TestFinishedHandleRelease:

• test_move_to_finished_closes_popen_pipes — stdout pipe FD released at finish (red on main)
• test_move_to_finished_closes_pty — PTY master released (red on main)
• test_move_to_finished_safe_without_handles — env/detached sessions don't crash
• test_poll_still_serves_output_after_handle_release — buffered output still queryable after close (red on main)

Related

The finished-process TTL is independently being made configurable in #75144 (retention knob for how long finished output stays queryable) — that PR does not include this handle-release fix; the two are complementary.

[teknium1]

Thanks for tracing the retained-descriptor path — the underlying leak is present on current main: local spawns retain Popen.stdout (tools/process_registry.py:780-794), while finished sessions retain their ProcessSession objects without a close path (tools/process_registry.py:1149-1153).

Problems

• tools/process_registry.py:1161 assumes every _move_to_finished() call follows reader EOF. That is not true: kill_process() marks the session exited and calls _move_to_finished() immediately after termination (tools/process_registry.py:1648-1660), while the reader can still be selecting or reading the same handle. Closing it there can race the reader and lose an unbuffered output tail.

Suggested changes

• Release handles after reader completion, or synchronize the kill path with reader shutdown; preserve a deliberate cleanup path for orphan-held pipes.
• Add a live-reader + kill_process() regression test. The new tests model already-finished handles and do not exercise this race.

Remote main is one commit ahead of the PR base (9e4492fd74e6), affecting the reader-loop region rather than this hunk, so the staleness is limited but the race needs a substantive adjustment.

Automated hermes-sweeper review.

[teknium1]

This helper is not reached only after reader completion: kill_process() marks the session exited and calls _move_to_finished() immediately after terminating the process (current main tools/process_registry.py:1648-1660), while the reader may still be selecting or reading this handle. Closing it here can race that reader and drop its tail; defer release until reader completion or explicitly coordinate the kill path, with a regression test for that race.

[ciphercommand]

Confirmed — this race is real and is now covered by the same bounded drain-owner fix as the orphan-descendant case (commit b1271f142).

kill_process() → _move_to_finished() → _release_finished_handles() now goes through _should_defer_pipe_close() at the single release choke point: when the reader thread is still alive and the pipe has not reached true EOF, the close is deferred and the read end is handed to the bounded drain owner (≤ _ORPHAN_DRAIN_WINDOW_S = 5.0s), so the reader's tail is never dropped by a concurrent kill. The gate also excludes the reader's own finally path and Windows (blocking path), so it can't deadlock the reader or double-close.

The regression tests exercise the kill path directly: both tests in tests/tools/test_process_registry_orphan_pipe.py call registry.kill_process() and then assert the descendant survives, late output is captured, and the handle releases cleanly. Verification: orphan-pipe tests 2/2, existing tests/tools/test_process_registry.py 76/76.

[MongLong0214]

There is one more lifecycle case that should block this from closing handles in _move_to_finished(): the orphan-held pipe path in current main.

ProcessRegistry._reader_loop() deliberately stops after the direct child exits and the pipe stays idle for about 600 ms, even when a background descendant still owns the write end. The source comment explicitly says this path may exit without EOF so a grandchild cannot park the reader forever.

Closing Popen.stdout from _move_to_finished() in that state removes the parent-side read end while the descendant is still alive. A later write by that descendant can receive EPIPE / SIGPIPE. The current tests cover already-finished or mocked handles, so they do not exercise this process-tree behavior.

Please add a deterministic regression that:

1. starts a shell which backgrounds a descendant inheriting stdout;
2. lets the direct shell exit and waits past the idle-after-exit window;
3. has the descendant write again after that window; and
4. verifies the descendant is not terminated by the registry's handle release and that the chosen late-output policy is explicit.

The release boundary also needs to stay physical: the FD charge should not be returned merely because the session moved to _finished; it should be returned only after the read/PTY handle was actually closed. If preserving descendant behavior requires keeping a bounded drain owner, that state still consumes the descriptor budget.

This is separate from the live-reader race already noted in review: it is reachable after the reader loop intentionally stops without EOF.

[ciphercommand]

We independently reproduced this against a local copy of the branch and confirmed the claim — then built a bounded drain owner that fixes it. Details below for folding into main.

Repro (deterministic regression test, tests/tools/test_process_registry_orphan_pipe.py):

• Real process tree, no mocks: a shell backgrounds a descendant that inherits stdout, the direct shell exits, we wait past the ~600 ms idle-after-exit window, then the descendant writes again.
• Before the fix this FAILS on purpose with ORPHAN-DESCENDANT POLICY VIOLATION: the sentinel marker is absent (descendant died EPIPE/SIGPIPE), proc.stdout.closed is True right after finish, and the late output is lost. Control case (read end left open) shows the descendant survives and the output is captured — so the killer is unambiguously the unconditional handle close in _move_to_finished(), not shell semantics.
• After the fix it passes 2/2.

Fix (commit b1271f142, "bounded drain owner"):

• New _ORPHAN_DRAIN_WINDOW_S = 5.0 constant. When the reader stops without EOF (the orphan-escape path), a daemon drain owner keeps the read end open for at most that window, capturing the descendant's late output into output_buffer, then closes on deadline or true EOF.
• Hard bound: a pipe-hoarding descendant can never park the owner indefinitely — Fix-1 FD-hygiene intent preserved.
• _should_defer_pipe_close() makes every non-reader path (poll/wait reconcile, kill) defer the close until the pipe hits true EOF or the drain window expires.
• The FD charge is returned only when the read/PTY handle is actually closed (_finish_drain_owner → _release_finished_handles(force=True)), not merely when the session moves to _finished — matching the release-boundary point in your review.
• Windows-safe no-op; idempotent against reader/reconcile races.

Verification: orphan-pipe tests 2/2 (descendant survives AND late output captured; bounded-property test proves the window actually closes handles), and the existing tests/tools/test_process_registry.py suite stays 76/76 (including the guard test block that covers the previously-fixed cases).

The same process-tree class applies to the kill_process() race noted earlier in review (tools/process_registry.py:1648-1660): the kill path now defers the close via the same _should_defer_pipe_close() gate, so the reader can't race a released handle there either.

[calebhicks]

Disposition: keep open; not superseded

Studio production carries a mechanically rebased downstream patch as host/hermes-patches/0003-process-registry-fd.patch (tracked in SchoolAI-Limited/mc-agents-runtime, v0.20.1 pin). Its focused transport-cleanup coverage passes locally (10 targeted tests). The carry is explicitly temporary and is to be dropped only after a safe upstream merge.

Current upstream main (fe0a56e) still has no equivalent fix, so this is not an implemented_on_main close. The current PR head (53db502) should not be merged as-is: the existing review correctly identifies the live-reader/kill race and orphan-held-pipe path. The downstream carry covers the former with synchronization, but does not add the bounded drain-owner policy needed for late writes from descendants after the reader intentionally stops without EOF.

Recommendation: keep the PR open for salvage/update, with a deterministic real-process orphan-pipe regression and an explicit bounded drain/late-output policy. Revisit closure only after the fix lands on upstream main or a replacement PR supersedes it.