mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
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:
parent
6c48ac81f2
commit
816cb85fa8
3 changed files with 62 additions and 1 deletions
|
|
@ -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)).
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue