fix(install): close parent log fd in start_detached_agent (#1576)

## 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>
This commit is contained in:
Abhay Singh 2026-07-02 09:37:01 +05:30 committed by GitHub
parent 6c48ac81f2
commit 816cb85fa8
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 62 additions and 1 deletions

View file

@ -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)).

View file

@ -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:

View file

@ -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(