mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
fix(wrap): detach the shared proxy on Windows so it survives an ungraceful agent close (#1464)
## Description Closing one `headroom wrap <agent>` instance on Windows could kill the **shared proxy** out from under every other running instance, so their requests started failing. `_start_proxy` launched the proxy as a child of whichever agent started it first, without detaching it from that agent's console and Job object. The wrapper already reference-counts clients via per-PID markers and `_make_cleanup` leaves the proxy running while other clients exist — but that only runs on a *graceful* exit. On an *ungraceful* close (closing the terminal window, `taskkill`, a crash) Windows tree-kills the whole process group/Job and the proxy dies directly, bypassing the reference counting. Every other instance's `ANTHROPIC_BASE_URL` then points at a dead `127.0.0.1:8787`, so all of its API traffic fails. ## Type of Change - [x] Bug fix (non-breaking change that fixes an issue) - [ ] New feature (non-breaking change that adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update - [ ] Performance improvement - [ ] Code refactoring (no functional changes) ## Changes Made - `_start_proxy` creates the proxy with `DETACHED_PROCESS | CREATE_NEW_PROCESS_GROUP | CREATE_BREAKAWAY_FROM_JOB` on Windows, so an ungraceful close of the launching agent can no longer reach it; only the ref-counted `_make_cleanup` ends the proxy. - Falls back without `CREATE_BREAKAWAY_FROM_JOB` (catching `OSError`) when the launcher's Job forbids breakaway; `DETACHED_PROCESS` still spares the proxy from console-close events. - Platform guard is `sys.platform == "win32"` (not `os.name == "nt"`) so mypy narrows the platform and resolves the Windows-only `subprocess` constants. - POSIX path unchanged: `creationflags=0`, detachment still via `start_new_session` (`setsid`). - Added `tests/test_cli/test_wrap_proxy_detach.py` and a CHANGELOG Bug Fixes entry. ## Testing - [x] Unit tests pass (`pytest`) - [x] Linting passes (`ruff check .`) - [x] Type checking passes (`mypy headroom`) - [x] New tests added for new functionality - [x] Manual testing performed ### Test Output ```text $ pytest tests/test_cli/test_wrap_proxy_detach.py -q .. [100%] 2 passed, 2 warnings in 1.50s $ ruff check headroom/cli/wrap.py tests/test_cli/test_wrap_proxy_detach.py All checks passed! $ mypy --follow-imports=silent headroom/cli/wrap.py tests/test_cli/test_wrap_proxy_detach.py Success: no issues found ``` ## Real Behavior Proof - Environment: Windows 11, Python 3.11.9, headroom-ai (pipx). Two concurrent `headroom wrap claude` instances sharing proxy `127.0.0.1:8787`. `_start_proxy` was also exercised directly on this host with `subprocess.Popen` stubbed. - Exact command / steps: (1) start two `headroom wrap claude` instances; (2) close the terminal window of the one that started the proxy (ungraceful — not `/exit`); (3) issue a request from the surviving instance. Separately: call `_start_proxy(8787)` with `subprocess.Popen` stubbed and read back the creation flags. - Observed result: before the fix the proxy died with the closed window and the surviving instance failed (`ANTHROPIC_BASE_URL` → dead `:8787`), because the OS tree-killed the child before the ref-count path could spare it. After the fix the detached proxy survives the close and the surviving instance keeps working; the stub harness reports `creationflags=0x1000208` (`DETACHED_PROCESS | CREATE_NEW_PROCESS_GROUP | CREATE_BREAKAWAY_FROM_JOB`) on win32 and `0` when forced off-Windows. - Not tested: real breakaway behavior under an actual restrictive Job object on this host (the OS-level effect). The `OSError` fallback path itself now has a dedicated unit test (`test_start_proxy_retries_without_breakaway_when_job_forbids_it`). ## 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 ## Screenshots (if applicable) N/A — no UI changes. ## Additional Notes - Documentation checklist item is N/A: this is a behavioral bug fix with no user-facing doc surface. - Scope is the single `subprocess.Popen` call in `_start_proxy`; the marker-based reference counting in `_make_cleanup` is unchanged and remains the only thing that intentionally stops the proxy.
This commit is contained in:
quentinmaisonneuve
GitHub
parent
d337e3b828
commit
6cba4419d0
3 changed files with 222 additions and 31 deletions
|
|
@ -38,6 +38,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
|||
* **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)).
|
||||
* **subscription:** stop zeroing the 5-hour headroom contribution counters on every poll. The rollover check compared `five_hour.resets_at` with a bare `!=`, but the usage API reports that timestamp with second-level jitter (observed flapping between `01:59:59Z` and `02:00:00Z` on consecutive polls within the same window), so a spurious "5h window rolled over" reset fired every poll interval (~5 min) and the dashboard's per-window savings stuck near 0%. Only a forward jump larger than `_ROLLOVER_MIN_ADVANCE` (1 minute) now counts as a real rollover.
|
||||
* **wrap:** keep the shared proxy alive when the agent that launched it closes *ungracefully* on Windows. `_start_proxy` spawned the proxy without detaching it, so it stayed in the launcher's console and Job object; closing that terminal window (or `taskkill`/a crash) tree-killed the proxy, bypassing the marker-based reference counting in `_make_cleanup` and breaking every other `headroom wrap` instance routed through the same port. The proxy is now created with `CREATE_NO_WINDOW | CREATE_NEW_PROCESS_GROUP | CREATE_BREAKAWAY_FROM_JOB` (with a graceful fallback when the launcher's Job forbids breakaway); POSIX behavior is unchanged. `CREATE_NO_WINDOW` (rather than `DETACHED_PROCESS`) gives the proxy its own *hidden* console: `DETACHED_PROCESS` leaves a console-subsystem exe (`python.exe`) consoleless, so Windows surfaces a visible console window whose close button kills the proxy.
|
||||
* **transforms/content_router:** stop replacing `role="tool"` output with a lossy-unrecoverable summary on the live compression path (refs [#1307](https://github.com/chopratejas/headroom/issues/1307)). `ContentRouter.apply()` routed OpenAI-style `role="tool"` string messages — `Bash`/`grep`/`ls`/`cat` output — through the ML/word-drop summarizers; when the result carried no CCR retrieve marker (CCR off, ratio >= 0.8, or the size-gate fallback) the original was unrecoverable and the agent acted on a fabricated summary. Tool-role string content is now kept verbatim unless the compressed form is CCR-recoverable. Assistant/user text is unaffected, and structurally-lossless passes (SmartCrusher/Log/Search) still apply. The Anthropic `tool_result` block path is tracked separately.
|
||||
* **rtk:** stop `rtk` hook registration from spuriously timing out during `headroom wrap`. Output is captured to a temp file instead of pipes, and `stdin` is closed, so a background process forked by `rtk init` can no longer hold the pipe open and block `subprocess.run` past its 10s timeout after the hooks were already registered.
|
||||
* **ccr:** stop re-compressing `headroom_retrieve` output, which created an infinite retrieval loop, and stop emitting retrieval markers when the `headroom_retrieve` tool is not injected, which silently dropped data ([#1077](https://github.com/chopratejas/headroom/issues/1077), [#1006](https://github.com/chopratejas/headroom/issues/1006)).
|
||||
|
|
|
|||
|
|
@ -462,39 +462,83 @@ def _start_proxy(
|
|||
if openai_api_url:
|
||||
proxy_env["GITHUB_COPILOT_API_URL"] = openai_api_url
|
||||
|
||||
proc = subprocess.Popen(
|
||||
cmd,
|
||||
stdout=stdio_log_file,
|
||||
stderr=stdio_log_file,
|
||||
env=proxy_env,
|
||||
start_new_session=os.name == "posix",
|
||||
)
|
||||
# Detach the proxy from the launching console on Windows so an ungraceful
|
||||
# close of the owning agent (closing the terminal window, taskkill, or a
|
||||
# crash) cannot tree-kill the shared proxy out from under other live
|
||||
# clients. Without this the proxy stays in the owner's console + Job
|
||||
# object; closing that window terminates the whole tree, bypassing the
|
||||
# marker-based reference counting in ``_make_cleanup`` and breaking every
|
||||
# other ``headroom wrap`` instance routed through the same port.
|
||||
# CREATE_NO_WINDOW — give the proxy its OWN, invisible console.
|
||||
# A separate console means the parent's
|
||||
# CTRL_CLOSE_EVENT never reaches it, and no
|
||||
# stray console window pops up. DETACHED_PROCESS
|
||||
# also isolates the console, but for a console
|
||||
# subsystem exe (python.exe) it leaves the proxy
|
||||
# consoleless and Windows surfaces a visible
|
||||
# console window — closing that window killed
|
||||
# the proxy, defeating the whole point.
|
||||
# CREATE_NEW_PROCESS_GROUP — isolate from the parent's Ctrl-C
|
||||
# CREATE_BREAKAWAY_FROM_JOB— survive Job kill-on-close (Windows Terminal,
|
||||
# VS Code integrated terminal, conhost)
|
||||
# CREATE_NO_WINDOW / DETACHED_PROCESS / CREATE_NEW_CONSOLE are mutually
|
||||
# exclusive — pick exactly one. On POSIX, ``start_new_session`` already
|
||||
# detaches via setsid(). ``sys.platform == "win32"`` (not ``os.name ==
|
||||
# "nt"``) so mypy narrows the platform and resolves the Windows-only
|
||||
# ``subprocess`` constants below.
|
||||
_CREATE_BREAKAWAY_FROM_JOB = 0x01000000
|
||||
creationflags = 0
|
||||
if sys.platform == "win32":
|
||||
creationflags = (
|
||||
subprocess.CREATE_NO_WINDOW
|
||||
| subprocess.CREATE_NEW_PROCESS_GROUP
|
||||
| _CREATE_BREAKAWAY_FROM_JOB
|
||||
)
|
||||
|
||||
# Wait for proxy to be ready.
|
||||
# ML components (Kompress, Magika, Tree-sitter) load synchronously before
|
||||
# uvicorn binds the port. On slower machines this can take 20-30 seconds.
|
||||
for _i in range(timeout_seconds):
|
||||
time.sleep(1)
|
||||
if _check_proxy(port):
|
||||
click.echo(f" Logs: {log_path}")
|
||||
stdio_log_file.close()
|
||||
return proc
|
||||
# Check if process died
|
||||
if proc.poll() is not None:
|
||||
stdio_log_file.close()
|
||||
# Read last few lines of log for error context
|
||||
try:
|
||||
tail = _read_text(stdio_log_path)[-500:]
|
||||
except Exception:
|
||||
tail = "(no log output)"
|
||||
raise RuntimeError(f"Proxy exited with code {proc.returncode}: {tail}")
|
||||
popen_kwargs: dict[str, Any] = {
|
||||
"stdout": stdio_log_file,
|
||||
"stderr": stdio_log_file,
|
||||
"env": proxy_env,
|
||||
"start_new_session": os.name == "posix",
|
||||
"creationflags": creationflags,
|
||||
}
|
||||
# Close the parent's copy of the stdio log handle on every exit path,
|
||||
# including when BOTH spawn attempts raise. The child keeps its own
|
||||
# inherited duplicate, so closing here never starves the proxy's logging.
|
||||
try:
|
||||
try:
|
||||
proc = subprocess.Popen(cmd, **popen_kwargs)
|
||||
except OSError:
|
||||
# The launcher's Job object forbids breakaway. Retry without that flag;
|
||||
# CREATE_NO_WINDOW still spares the proxy from console-close events.
|
||||
if sys.platform == "win32":
|
||||
popen_kwargs["creationflags"] = creationflags & ~_CREATE_BREAKAWAY_FROM_JOB
|
||||
proc = subprocess.Popen(cmd, **popen_kwargs)
|
||||
|
||||
proc.kill()
|
||||
stdio_log_file.close()
|
||||
raise RuntimeError(
|
||||
f"Proxy failed to start on port {port} within {timeout_seconds} seconds. "
|
||||
f"Set {_WRAP_PROXY_TIMEOUT_ENV} to a larger number of seconds for slow startup."
|
||||
)
|
||||
# Wait for proxy to be ready.
|
||||
# ML components (Kompress, Magika, Tree-sitter) load synchronously before
|
||||
# uvicorn binds the port. On slower machines this can take 20-30 seconds.
|
||||
for _i in range(timeout_seconds):
|
||||
time.sleep(1)
|
||||
if _check_proxy(port):
|
||||
click.echo(f" Logs: {log_path}")
|
||||
return proc
|
||||
# Check if process died
|
||||
if proc.poll() is not None:
|
||||
# Read last few lines of log for error context
|
||||
try:
|
||||
tail = _read_text(stdio_log_path)[-500:]
|
||||
except Exception:
|
||||
tail = "(no log output)"
|
||||
raise RuntimeError(f"Proxy exited with code {proc.returncode}: {tail}")
|
||||
|
||||
proc.kill()
|
||||
raise RuntimeError(
|
||||
f"Proxy failed to start on port {port} within {timeout_seconds} seconds. "
|
||||
f"Set {_WRAP_PROXY_TIMEOUT_ENV} to a larger number of seconds for slow startup."
|
||||
)
|
||||
finally:
|
||||
stdio_log_file.close()
|
||||
|
||||
|
||||
def _setup_rtk(verbose: bool = False) -> Path | None:
|
||||
|
|
|
|||
146
tests/test_cli/test_wrap_proxy_detach.py
Normal file
146
tests/test_cli/test_wrap_proxy_detach.py
Normal file
|
|
@ -0,0 +1,146 @@
|
|||
"""Detachment of the shared proxy subprocess in ``headroom wrap``.
|
||||
|
||||
``_start_proxy`` launches the proxy that every wrapped agent on a port shares.
|
||||
It must outlive an *ungraceful* close of the agent that happened to start it
|
||||
(closing the terminal window, taskkill, a crash) -- otherwise the OS tree-kills
|
||||
the proxy and breaks the other live clients, bypassing the marker-based
|
||||
reference counting in ``_make_cleanup``.
|
||||
|
||||
On Windows that means detaching from the launcher's console and Job object via
|
||||
creation flags; on POSIX ``start_new_session`` already detaches via setsid().
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from pathlib import Path
|
||||
from typing import Any
|
||||
|
||||
import pytest
|
||||
|
||||
from headroom.cli import wrap as wrap_cli
|
||||
|
||||
# Windows-only creation flag; not always exported by the host's ``subprocess``.
|
||||
_CREATE_BREAKAWAY_FROM_JOB = 0x01000000
|
||||
|
||||
|
||||
class _FakeProc:
|
||||
"""Stand-in for a live proxy process (``poll() is None``)."""
|
||||
|
||||
returncode = 0
|
||||
|
||||
def poll(self) -> None:
|
||||
return None
|
||||
|
||||
|
||||
def _capture_popen_kwargs(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> dict[str, Any]:
|
||||
"""Invoke ``_start_proxy`` with all I/O stubbed; return the Popen kwargs."""
|
||||
captured: dict[str, Any] = {}
|
||||
|
||||
def _fake_popen(cmd: Any, **kwargs: Any) -> _FakeProc:
|
||||
captured.clear()
|
||||
captured.update(kwargs)
|
||||
return _FakeProc()
|
||||
|
||||
monkeypatch.setattr(wrap_cli.subprocess, "Popen", _fake_popen)
|
||||
monkeypatch.setattr(wrap_cli, "_check_proxy", lambda port: True)
|
||||
monkeypatch.setattr(wrap_cli.time, "sleep", lambda _seconds: None)
|
||||
monkeypatch.setattr(wrap_cli, "_get_log_path", lambda: tmp_path / "proxy.log")
|
||||
monkeypatch.setattr(wrap_cli, "_resolve_wrap_proxy_timeout_seconds", lambda: 1)
|
||||
|
||||
wrap_cli._start_proxy(8787)
|
||||
return captured
|
||||
|
||||
|
||||
def test_start_proxy_detaches_on_windows(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None:
|
||||
# Force the Windows branch regardless of the host OS, and supply the
|
||||
# Windows-only ``subprocess`` constants the host lacks on POSIX.
|
||||
monkeypatch.setattr(wrap_cli.sys, "platform", "win32")
|
||||
monkeypatch.setattr(wrap_cli.subprocess, "CREATE_NO_WINDOW", 0x08000000, raising=False)
|
||||
monkeypatch.setattr(wrap_cli.subprocess, "CREATE_NEW_PROCESS_GROUP", 0x200, raising=False)
|
||||
|
||||
flags = _capture_popen_kwargs(monkeypatch, tmp_path)["creationflags"]
|
||||
|
||||
assert flags & 0x08000000 # CREATE_NO_WINDOW: own hidden console
|
||||
assert flags & 0x200 # CREATE_NEW_PROCESS_GROUP: ignore the parent's Ctrl-C
|
||||
assert flags & _CREATE_BREAKAWAY_FROM_JOB # survive Job kill-on-close
|
||||
|
||||
|
||||
def test_start_proxy_keeps_creationflags_zero_off_windows(
|
||||
monkeypatch: pytest.MonkeyPatch, tmp_path: Path
|
||||
) -> None:
|
||||
monkeypatch.setattr(wrap_cli.sys, "platform", "linux")
|
||||
|
||||
kwargs = _capture_popen_kwargs(monkeypatch, tmp_path)
|
||||
|
||||
# POSIX detaches via setsid(); no Windows creation flags are applied.
|
||||
assert kwargs["creationflags"] == 0
|
||||
assert isinstance(kwargs["start_new_session"], bool)
|
||||
|
||||
|
||||
def test_start_proxy_retries_without_breakaway_when_job_forbids_it(
|
||||
monkeypatch: pytest.MonkeyPatch, tmp_path: Path
|
||||
) -> None:
|
||||
# Force the Windows branch and supply its constants, as above.
|
||||
monkeypatch.setattr(wrap_cli.sys, "platform", "win32")
|
||||
monkeypatch.setattr(wrap_cli.subprocess, "CREATE_NO_WINDOW", 0x08000000, raising=False)
|
||||
monkeypatch.setattr(wrap_cli.subprocess, "CREATE_NEW_PROCESS_GROUP", 0x200, raising=False)
|
||||
|
||||
# First spawn raises OSError (the launcher's Job forbids breakaway); the
|
||||
# second must succeed without CREATE_BREAKAWAY_FROM_JOB.
|
||||
seen: list[int] = []
|
||||
|
||||
def _flaky_popen(cmd: Any, **kwargs: Any) -> _FakeProc:
|
||||
seen.append(kwargs["creationflags"])
|
||||
if len(seen) == 1:
|
||||
raise OSError("a process in the job cannot break away")
|
||||
return _FakeProc()
|
||||
|
||||
monkeypatch.setattr(wrap_cli.subprocess, "Popen", _flaky_popen)
|
||||
monkeypatch.setattr(wrap_cli, "_check_proxy", lambda port: True)
|
||||
monkeypatch.setattr(wrap_cli.time, "sleep", lambda _seconds: None)
|
||||
monkeypatch.setattr(wrap_cli, "_get_log_path", lambda: tmp_path / "proxy.log")
|
||||
monkeypatch.setattr(wrap_cli, "_resolve_wrap_proxy_timeout_seconds", lambda: 1)
|
||||
|
||||
wrap_cli._start_proxy(8787)
|
||||
|
||||
assert len(seen) == 2 # the call was retried exactly once
|
||||
assert seen[0] & _CREATE_BREAKAWAY_FROM_JOB # first attempt requests breakaway
|
||||
assert not seen[1] & _CREATE_BREAKAWAY_FROM_JOB # retry drops it
|
||||
assert seen[1] & 0x08000000 # but still gets its own hidden console
|
||||
assert seen[1] & 0x200 # and still gets its own process group
|
||||
|
||||
|
||||
class _TrackingFile:
|
||||
"""Records whether the parent closed its copy of the stdio log handle."""
|
||||
|
||||
def __init__(self) -> None:
|
||||
self.closed = False
|
||||
|
||||
def close(self) -> None:
|
||||
self.closed = True
|
||||
|
||||
|
||||
def test_start_proxy_closes_log_when_both_spawns_fail(
|
||||
monkeypatch: pytest.MonkeyPatch, tmp_path: Path
|
||||
) -> None:
|
||||
# Both spawn attempts raise (e.g. the Job forbids breakaway AND the retry
|
||||
# still fails). The parent must not leak the stdio log file handle.
|
||||
monkeypatch.setattr(wrap_cli.sys, "platform", "win32")
|
||||
monkeypatch.setattr(wrap_cli.subprocess, "CREATE_NO_WINDOW", 0x08000000, raising=False)
|
||||
monkeypatch.setattr(wrap_cli.subprocess, "CREATE_NEW_PROCESS_GROUP", 0x200, raising=False)
|
||||
|
||||
log_file = _TrackingFile()
|
||||
monkeypatch.setattr(wrap_cli, "open", lambda *a, **k: log_file, raising=False)
|
||||
|
||||
def _always_fails(cmd: Any, **kwargs: Any) -> _FakeProc:
|
||||
raise OSError("spawn failed on both attempts")
|
||||
|
||||
monkeypatch.setattr(wrap_cli.subprocess, "Popen", _always_fails)
|
||||
monkeypatch.setattr(wrap_cli.time, "sleep", lambda _seconds: None)
|
||||
monkeypatch.setattr(wrap_cli, "_get_log_path", lambda: tmp_path / "proxy.log")
|
||||
monkeypatch.setattr(wrap_cli, "_resolve_wrap_proxy_timeout_seconds", lambda: 1)
|
||||
|
||||
with pytest.raises(OSError):
|
||||
wrap_cli._start_proxy(8787)
|
||||
|
||||
assert log_file.closed # finally closed the handle despite the failure
|
||||
Loading…
Add table
Add a link
Reference in a new issue