mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
816cb85fa8
## 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>
|
||
|---|---|---|
| .. | ||
| test_health.py | ||
| test_native_installers.py | ||
| test_paths.py | ||
| test_planner.py | ||
| test_providers.py | ||
| test_runtime.py | ||
| test_state.py | ||
| test_supervisors.py | ||