From 816cb85fa8ee8d349fe673e7affd9a54acb1207d Mon Sep 17 00:00:00 2001
From: Abhay Singh <abhaysingh0293@gmail.com>
Date: Thu, 2 Jul 2026 09:37:01 +0530
Subject: [PATCH] fix(install): close parent log fd in start_detached_agent
 (#1576)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

## Description

`start_detached_agent()` opens the agent log file and hands it to
`subprocess.Popen` as `stdout`/`stderr`, then returns the process
**without
closing the parent's copy of the file descriptor**. The child inherits
the fd
and writes to it, but the parent keeps its own copy open forever.

The result: every `headroom install start` leaks one file descriptor in
the
parent, and the leaked handle pins the log file open so it can't be
rotated.
On a tight `ulimit` or inside a container, repeated starts can walk
straight
into the fd limit.

```python
# headroom/install/runtime.py — before
log_file = open(log_file_path, "a", encoding="utf-8", errors="replace")
kwargs = {"stdout": log_file, "stderr": log_file, ...}
return subprocess.Popen(command, **kwargs)   # parent's log_file never closed
```

The fix closes the parent's copy in a `try/finally` right after `Popen`
returns:

```python
try:
    proc = subprocess.Popen(command, **kwargs)
finally:
    # The child has inherited the log file descriptor, so the parent's
    # copy is dead weight. Closing it (even when Popen raises) avoids
    # leaking one fd per `headroom install start` and lets the log file
    # be rotated.
    log_file.close()
return proc
```

The `finally` is deliberate: it also covers the case where `Popen`
itself
raises (bad executable, fork failure), which would otherwise leak the
just-opened handle. This matches the `with open(...)` pattern already
used by
`run_foreground()` a few lines above.

Closes #1554

## Type of Change

- [x] Bug fix (non-breaking change that fixes an issue)

## Changes Made

- `headroom/install/runtime.py`: close the parent's log file descriptor
in a `try/finally` after `subprocess.Popen` in `start_detached_agent()`,
so it is released on the normal path and when `Popen` raises.
- `tests/test_install/test_runtime.py`: add two regression tests — one
for a normal start, one for `Popen` raising — asserting the parent's log
handle is closed afterwards.
- `CHANGELOG.md`: Bug Fixes entry under Unreleased.

## Testing

- [x] Unit tests pass (`pytest`)
- [x] Linting passes (`ruff check .`)
- [ ] Type checking passes (`mypy headroom`)
- [x] New tests added for new functionality
- [x] Manual testing performed

### Test Output

Before the fix (`runtime.py` reverted to its parent commit, new tests
kept) —
the assertion inspects the *actual* log file handle and finds it still
open:

```text
E   AssertionError: assert False is True
E    +  where False = <_io.TextIOWrapper name='...\deploy\demo\runner.log' mode='a' encoding='utf-8'>.closed
FAILED tests/test_install/test_runtime.py::test_start_detached_agent_closes_parent_log_fd
FAILED tests/test_install/test_runtime.py::test_start_detached_agent_closes_log_fd_when_popen_raises
============================== 2 failed in 0.43s ==============================
```

After the fix:

```text
tests\test_install\test_runtime.py ................
====================== 16 passed, 1 deselected in 0.35s =======================
```

(The one deselected test,
`test_runtime_start_lock_blocks_another_process`, is a
pre-existing failure on my Windows box — it fails identically on a clean
checkout of `main` and is unrelated to this change.)

## Real Behavior Proof

- Environment: Windows 11, Python 3.12.11, headroom built from this
branch (`uv sync --extra dev`).
- Exact command / steps: ran the two regression tests, which drive the
real `start_detached_agent` code path — real `open()`, real
`Popen(stdout=...)`, real (or missing) `close()`. Only
`subprocess.Popen` is stubbed so the test never launches an actual
detached agent; the fd-lifecycle bug lives entirely in how the parent
handles its own handle, and that runs for real.
- Observed result: the log file handle the parent passed to `Popen` is
`.closed == False` before the fix and `.closed == True` after — for both
the normal path and the `Popen`-raises path (output above).
- Not tested: I intentionally did not spin up many real detached agents
to watch the OS fd table grow — on Windows that means flashing console
windows and isn't a clean signal anyway. The handle-state assertion on
the real file object is the deterministic equivalent. Did not run the
full `mypy headroom` pass (one-line lifecycle change, no new types).

## Review Readiness

- [x] I have performed a self-review
- [x] This PR is ready for human review

## Checklist

- [x] My code follows the project's style guidelines
- [x] I have performed a self-review of my code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] New and existing unit tests pass locally with my changes
- [x] I have updated the CHANGELOG.md if applicable

## Additional Notes

- Single logical change, no new dependencies, default behavior otherwise
unchanged.
- Rebased/merged latest `main` to clear a `CHANGELOG.md` conflict.
- @chopratejas this is a sibling of #1555 (the `wrap.py` readiness-loop
handle leak you've got filed). I scoped this PR to #1554 only; happy to
follow up on #1555 separately if you'd like.

Co-authored-by: JerrettDavis <mxjerrett@gmail.com>
---
 CHANGELOG.md                       |  1 +
 headroom/install/runtime.py        | 10 +++++-
 tests/test_install/test_runtime.py | 52 ++++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 94d86d103..62840aeca 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -59,6 +59,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Bug Fixes
 
+* **install:** stop leaking a file descriptor on every `headroom install start`. `start_detached_agent()` opened the agent log file and handed it to `subprocess.Popen` but never closed the parent's copy, so each call leaked one fd (and pinned the log file open against rotation). The parent now closes its copy in a `try/finally` once the child has inherited it — the close also runs if `Popen` raises ([#1554](https://github.com/headroomlabs-ai/headroom/issues/1554)).
 * **proxy:** include the system prompt, tools, and the response-shaping request fields in the SemanticCache key. `_compute_key` hashed only `{model, messages}`, so two non-streaming requests with identical messages but a different top-level `system` prompt, tool set, sampling config, or output-shaping field collided on one key and the second caller was served the first's cached response — generated under different request semantics, in the default config (`cache_enabled` defaults on). The key now folds the request fields that shape generation — `temperature`/`top_p`/`top_k`/`max_tokens`/`stop`, plus OpenAI `tool_choice`/`response_format`/`parallel_tool_calls`/`seed`/`presence_penalty`/`frequency_penalty`/`logit_bias`/`n`/`logprobs`/`top_logprobs`/`reasoning_effort`/`verbosity`/`modalities` and Anthropic `thinking`/`tool_choice`/`output_config` — canonicalizing `system`/`tools` so a moved `cache_control` breakpoint does not fragment it, and the handlers snapshot the fields once at the cache read and reuse them at write so a body mutated by the pipeline cannot diverge the key. Non-streaming path only.
 * **learn (verbosity):** `--verbosity --apply --all` now aggregates the savings baseline across every project instead of overwriting it per project (last-project-wins), which previously left the output shaper with a tiny, unrepresentative baseline. The applied verbosity level comes from the project with the most samples ([#1288](https://github.com/headroomlabs-ai/headroom/pull/1288)).
 * **proxy/anthropic:** restore token-mode compression on continued Claude Code turns with a frozen prefix and deferred CCR tool injection. Token mode now runs request-side compression even when the client did not pre-register `headroom_retrieve`, relying on the existing marker-triggered injection override to keep emitted CCR markers redeemable ([#1487](https://github.com/headroomlabs-ai/headroom/issues/1487)).
diff --git a/headroom/install/runtime.py b/headroom/install/runtime.py
index 02a979fea..d6fd560a5 100644
--- a/headroom/install/runtime.py
+++ b/headroom/install/runtime.py
@@ -275,7 +275,15 @@ def start_detached_agent(profile: str) -> subprocess.Popen[str]:
         )
     else:
         kwargs["start_new_session"] = True
-    return subprocess.Popen(command, **kwargs)
+    try:
+        proc = subprocess.Popen(command, **kwargs)
+    finally:
+        # The child has inherited the log file descriptor, so the parent's
+        # copy is dead weight. Closing it (even when Popen raises) avoids
+        # leaking one fd per `headroom install start` and lets the log file
+        # be rotated. Wrapped in try/finally so a Popen failure can't leak.
+        log_file.close()
+    return proc
 
 
 def start_persistent_docker(manifest: DeploymentManifest) -> None:
diff --git a/tests/test_install/test_runtime.py b/tests/test_install/test_runtime.py
index d0c751711..c9f144fc4 100644
--- a/tests/test_install/test_runtime.py
+++ b/tests/test_install/test_runtime.py
@@ -7,6 +7,8 @@ import sys
 import types
 from pathlib import Path
 
+import pytest
+
 from headroom.install.models import DeploymentManifest, InstallPreset
 from headroom.install.runtime import (
     _clear_pid,
@@ -361,6 +363,56 @@ def test_run_foreground_and_detached_helpers(monkeypatch, tmp_path: Path) -> Non
     assert start_detached_agent("demo") is fake_proc_posix
 
 
+def test_start_detached_agent_closes_parent_log_fd(monkeypatch, tmp_path: Path) -> None:
+    """The parent must close its copy of the log file after Popen.
+
+    The child inherits the descriptor, so leaving the parent's copy open
+    leaks one fd per call and pins the log file open against rotation.
+    """
+    monkeypatch.setattr(Path, "home", lambda: tmp_path)
+    monkeypatch.setattr("headroom.install.runtime.resolve_headroom_command", lambda: ["headroom"])
+    monkeypatch.setattr("headroom.install.runtime.sys.platform", "linux")
+
+    captured: dict[str, object] = {}
+
+    class FakeProc:
+        pid = 999
+
+    def fake_popen(command: list[str], **kwargs):
+        captured["stdout"] = kwargs["stdout"]
+        captured["stderr"] = kwargs["stderr"]
+        return FakeProc()
+
+    monkeypatch.setattr("headroom.install.runtime.subprocess.Popen", fake_popen)
+
+    start_detached_agent("demo")
+
+    log_handle = captured["stdout"]
+    # Same handle is passed to both streams, and the parent closed it.
+    assert captured["stderr"] is log_handle
+    assert log_handle.closed is True
+
+
+def test_start_detached_agent_closes_log_fd_when_popen_raises(monkeypatch, tmp_path: Path) -> None:
+    """A Popen failure must not leak the just-opened log file handle."""
+    monkeypatch.setattr(Path, "home", lambda: tmp_path)
+    monkeypatch.setattr("headroom.install.runtime.resolve_headroom_command", lambda: ["headroom"])
+    monkeypatch.setattr("headroom.install.runtime.sys.platform", "linux")
+
+    captured: dict[str, object] = {}
+
+    def boom(command: list[str], **kwargs):
+        captured["stdout"] = kwargs["stdout"]
+        raise OSError("spawn failed")
+
+    monkeypatch.setattr("headroom.install.runtime.subprocess.Popen", boom)
+
+    with pytest.raises(OSError, match="spawn failed"):
+        start_detached_agent("demo")
+
+    assert captured["stdout"].closed is True
+
+
 def test_start_stop_wait_and_runtime_status_branches(monkeypatch, tmp_path: Path) -> None:
     calls: list[list[str]] = []
     monkeypatch.setattr(