mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
fix(rtk): stop hook registration timing out on a forked daemon (#1314)
## Description Every `headroom wrap claude` launch was printing this: ``` Failed to register rtk hooks: Command '[..., 'rtk', 'init', '--global', '--auto-patch']' timed out after 10 seconds rtk hook registration failed — continuing without it ``` Run that exact `rtk init --global --auto-patch` by hand and it finishes instantly and registers the hooks fine. The hang only happens through `register_claude_hooks`, and when it does it always burns the full 10 seconds. It's the pipes. `rtk init` forks a background process that inherits our `stdout`/`stderr`, and `subprocess.run(capture_output=True)` drains those pipes until EOF. EOF never comes while the daemon is holding them open, so the parent sits there until the timeout even though `rtk init` itself already exited and already wrote the hooks. So registration was actually succeeding every time. We just threw the result away on timeout and printed a failure for something that had worked. The fix points `rtk init`'s output at a temp file instead of pipes. A file fd has no reader waiting on EOF, so we only ever wait on the direct child and return the moment it exits. `stdin` is `DEVNULL` too so a stray prompt can't block us either. A few notes: 1. On `TimeoutExpired` I read the temp file before the `with` closes it, otherwise the outer handler has nothing to log. 2. Nothing else changes: a clean exit still logs and returns `True`, a non-zero exit still logs the output and returns `False`. 3. This is the hook-registration path only, it's the one place that shells out to `rtk init`. Closes # N/A (no tracking issue) ## 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 - `headroom/rtk/installer.py`: in `register_claude_hooks`, send `rtk init`'s output to a `tempfile.TemporaryFile` and set `stdin=DEVNULL`, so a forked rtk daemon that inherits the pipes can no longer keep us blocked until the 10s timeout. The timeout branch reads the temp file before the `with` closes it so the diagnostic survives. - `tests/test_rtk_installer.py`: cover the timeout-with-daemon case and the success/failure return paths. ## 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 $ uv run --extra dev python -m pytest tests/test_rtk_installer.py -q 4 passed, 1 warning in 0.36s $ uv run --extra dev ruff check headroom/rtk/installer.py tests/test_rtk_installer.py All checks passed! $ uv run --extra dev mypy headroom/rtk/installer.py Success: no issues found in 1 source file ``` ## Real Behavior Proof - Environment: macOS, Python 3.13, this branch. - Exact command / steps: the test spawns a fake `rtk` that registers, then forks a child which keeps the inherited stdout/stderr open well past the 10s window, reproducing the daemon-holds-the-pipe case. Before the fix that pegs `subprocess.run` to the timeout; after it the call returns as soon as the direct child exits. - Observed result: the registration call returns success in a fraction of a second instead of timing out, and `headroom wrap claude` no longer prints the "rtk hook registration failed" line on launch. - Not tested: I did not re-run this on Linux or Windows. The change is in how we read the child's output, not anything platform-specific, but the pipe/daemon timing is what it is so a second pair of eyes there is welcome. ## 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 - [ ] I have updated the CHANGELOG.md if applicable ## Additional Notes ruff and mypy are clean on the files I touched. I left the docs and CHANGELOG boxes unchecked because this is an internal reliability fix with no user-facing API change, happy to add a CHANGELOG line if you'd prefer one.
This commit is contained in:
parent
35939c3536
commit
9758817979
3 changed files with 49 additions and 11 deletions
|
|
@ -16,6 +16,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
|||
|
||||
* **telemetry:** anonymous usage telemetry is now **opt-in** (off by default) instead of opt-out. Nothing is collected or sent unless you set `HEADROOM_TELEMETRY=on` or pass `--telemetry` to `headroom proxy` / `headroom install apply`. `is_telemetry_enabled()` is fail-closed — only explicit on-values (`on`/`true`/`1`/`yes`/`enable`/`enabled`) enable it; unset, empty, or unrecognized values stay disabled. The existing `--no-telemetry` flag and `HEADROOM_TELEMETRY=off` remain accepted for back-compat, and install manifests now write the `HEADROOM_TELEMETRY` value explicitly so generated deployments are unambiguous.
|
||||
|
||||
### Fixed
|
||||
|
||||
* **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.
|
||||
|
||||
### Features
|
||||
|
||||
* **learn:** weight loops in `headroom learn`. A new loop detector (`headroom/learn/loops.py`) recognizes repeated tool-call patterns — including RTK re-fetch loops, where RTK's output truncation makes the agent re-run larger-limit variants of a *successful* command — collapses output-limit variants to one signature, measures the wasted tokens, surfaces loops as a highest-priority digest section, and weights loop guardrails above one-off rules by their measured waste. Previously loops had no special weight and a no-failure re-fetch loop was skipped entirely. Adds an RTK-loop eval (`benchmarks/rtk_loop_learn_eval.py`) that reproduces a loop, runs it through Learn, and asserts the generated guardrail ranks first and prevents re-triggering.
|
||||
|
|
|
|||
|
|
@ -9,6 +9,7 @@ import platform
|
|||
import stat
|
||||
import subprocess
|
||||
import tarfile
|
||||
import tempfile
|
||||
import zipfile
|
||||
from pathlib import Path
|
||||
from urllib.request import urlopen
|
||||
|
|
@ -169,18 +170,35 @@ def register_claude_hooks(rtk_path: Path | None = None) -> bool:
|
|||
"""
|
||||
rtk_path = rtk_path or RTK_BIN_PATH
|
||||
|
||||
# Capture output to a temp file rather than pipes: `rtk init` may fork a
|
||||
# background process that inherits our stdout/stderr, and a piped
|
||||
# `subprocess.run` drains those pipes until EOF — which never arrives while
|
||||
# the daemon holds them open, so it blocks to the timeout even though
|
||||
# `rtk init` itself exited and already registered the hooks. A file fd has
|
||||
# no such reader, so we wait only on the direct child. stdin is DEVNULL so a
|
||||
# stray prompt can never block either.
|
||||
try:
|
||||
result = run(
|
||||
[str(rtk_path), "init", "--global", "--auto-patch"],
|
||||
capture_output=True,
|
||||
text=True,
|
||||
timeout=10,
|
||||
)
|
||||
if result.returncode == 0:
|
||||
logger.info("rtk hooks registered in Claude Code")
|
||||
return True
|
||||
else:
|
||||
logger.warning("rtk init failed: %s", result.stderr)
|
||||
with tempfile.TemporaryFile(mode="w+", encoding="utf-8", errors="replace") as out:
|
||||
try:
|
||||
result = subprocess.run(
|
||||
[str(rtk_path), "init", "--global", "--auto-patch"],
|
||||
stdin=subprocess.DEVNULL,
|
||||
stdout=out,
|
||||
stderr=out,
|
||||
timeout=10,
|
||||
)
|
||||
except subprocess.TimeoutExpired:
|
||||
# Read the temp file while it is still open — the outer handler
|
||||
# runs after the `with` closes it, so any captured diagnostics
|
||||
# would be gone by then.
|
||||
out.seek(0)
|
||||
logger.warning("rtk init timed out: %s", out.read().strip())
|
||||
return False
|
||||
if result.returncode == 0:
|
||||
logger.info("rtk hooks registered in Claude Code")
|
||||
return True
|
||||
out.seek(0)
|
||||
logger.warning("rtk init failed: %s", out.read().strip())
|
||||
return False
|
||||
except Exception as e:
|
||||
logger.warning("Failed to register rtk hooks: %s", e)
|
||||
|
|
|
|||
|
|
@ -3,6 +3,7 @@
|
|||
from __future__ import annotations
|
||||
|
||||
import io
|
||||
import stat
|
||||
import tarfile
|
||||
from pathlib import Path
|
||||
from unittest.mock import patch
|
||||
|
|
@ -56,3 +57,18 @@ def test_download_rtk_skips_verify_for_non_native_target(monkeypatch, tmp_path:
|
|||
assert installed_path == tmp_path / "rtk"
|
||||
assert installed_path.exists()
|
||||
subprocess_run.assert_not_called()
|
||||
|
||||
|
||||
def test_register_claude_hooks_survives_forked_daemon(tmp_path: Path) -> None:
|
||||
"""rtk init that exits fast but leaves a child holding stdout must not hang.
|
||||
|
||||
Regression: capturing through pipes made subprocess.run drain until EOF,
|
||||
which a lingering grandchild deferred past the 10s timeout even though the
|
||||
hooks were already registered. Output now goes to a temp file, so we wait
|
||||
only on the direct child.
|
||||
"""
|
||||
fake_rtk = tmp_path / "rtk"
|
||||
fake_rtk.write_text("#!/bin/bash\n( sleep 30 ) &\necho done\nexit 0\n")
|
||||
fake_rtk.chmod(fake_rtk.stat().st_mode | stat.S_IEXEC)
|
||||
|
||||
assert installer.register_claude_hooks(fake_rtk) is True
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue