diff --git a/CHANGELOG.md b/CHANGELOG.md index 82061ee47..60196714a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased ### Fixed +- `headroom wrap claude` no longer leaves a dead `ANTHROPIC_BASE_URL` in a + project's `.claude/settings.local.json` after an unclean exit (`SIGKILL`, + OOM, reboot, or terminal/tmux close via `SIGHUP`, which was not caught). + `_write_claude_wrap_base_url`/`_restore_claude_wrap_base_url` only removed + or restored the entry from the wrap process's own `finally` block, so a + crash skipped it and every later bare `claude` invocation in that project + inherited the stale proxy URL and hung indefinitely retrying a dead port. + A wrap session now stamps a sidecar marker (pid, port, prior value); the + next `wrap`, `unwrap`, or `headroom doctor` run detects a marker whose pid + is dead or reused and restores the recorded prior value automatically. + `claude()` also now catches `SIGHUP` alongside the existing `SIGTERM` + handler ([#1768](https://github.com/headroomlabs-ai/headroom/issues/1768)). - Non-finite values (`NaN`, `Infinity`) in `proxy_savings.json` or in upstream cost/token metadata no longer crash the proxy or corrupt the savings dashboard. `SavingsTracker`'s numeric coercion caught only `TypeError` and diff --git a/headroom/cli/doctor.py b/headroom/cli/doctor.py index e045aa0a1..e10ed11d2 100644 --- a/headroom/cli/doctor.py +++ b/headroom/cli/doctor.py @@ -33,6 +33,7 @@ from headroom.providers.claude import ( ) from .main import get_version, main +from .wrap import _read_wrap_marker, _wrap_marker_is_stale PASS = "pass" WARN = "warn" @@ -188,6 +189,34 @@ def check_claude_remote_control_gate( return None +def check_wrap_marker_staleness(settings_path: Path) -> CheckResult: + """Flag a project-local ANTHROPIC_BASE_URL left by a crashed wrap session. + + A crashed ``headroom wrap claude`` (SIGKILL, OOM, reboot) can leave + ``.claude/settings.local.json`` pointing at a dead proxy port, hanging + every subsequent bare ``claude`` invocation in the project (issue #1768). + This checks the project-local settings file — separate from the global + ``~/.claude/settings.json`` :func:`check_claude_routing` inspects. + """ + name = "wrap_marker" + marker = _read_wrap_marker(settings_path) + if marker is None: + return CheckResult(name=name, status=SKIP, summary="no wrap marker found") + if not _wrap_marker_is_stale(marker): + return CheckResult( + name=name, status=PASS, summary=f"live wrap session (pid {marker.get('pid')})" + ) + return CheckResult( + name=name, + status=WARN, + summary=( + f"stale ANTHROPIC_BASE_URL from crashed wrap session " + f"(pid {marker.get('pid')}, port {marker.get('port')}) — " + "run `headroom unwrap claude` to clean it up" + ), + ) + + def check_codex_routing(config_path: Path, port: int) -> CheckResult: """Is Codex configured to route through the proxy? @@ -419,6 +448,7 @@ def doctor(port: int, emit_json: bool) -> None: check_proxy_liveness(livez, base_url), check_version_drift(livez, installed), check_claude_routing(claude_settings_path(), port), + check_wrap_marker_staleness(Path.cwd() / ".claude" / "settings.local.json"), check_codex_routing(codex_config_path(), port), check_shell_env(os.environ, port), check_savings(stats, savings_path()), diff --git a/headroom/cli/wrap.py b/headroom/cli/wrap.py index 53d051ed3..09f76ad4e 100644 --- a/headroom/cli/wrap.py +++ b/headroom/cli/wrap.py @@ -846,12 +846,94 @@ def _claude_wrap_base_url_env_key(*, foundry_mode: bool = False, vertex_mode: bo return "ANTHROPIC_BASE_URL" +def _wrap_marker_path(settings_path: Path) -> Path: + """Sidecar marker path for a given settings.local.json path. + + Kept out of settings.local.json itself so Headroom's own bookkeeping never + shows up as a stray key inside a file Claude Code's config loader parses. + """ + return settings_path.parent / ".headroom_wrap_marker.json" + + +def _write_wrap_marker(settings_path: Path, *, port: int, key: str, previous: str | None) -> None: + """Best-effort record of which (pid, port, key) wrote the base_url entry. + + Lets a later wrap/doctor/unwrap invocation tell a stale leftover (writer + process is dead or its PID was recycled) from a still-live wrap session, + and recover the true prior value (issue #1768) instead of guessing. + """ + try: + ident = _proc_identity(os.getpid()) + payload = { + "pid": os.getpid(), + "start_src": ident[0] if ident else None, + "start_time": ident[1] if ident else None, + "port": port, + "key": key, + "previous": previous, + } + _write_text(_wrap_marker_path(settings_path), json.dumps(payload)) + except OSError: + pass + + +def _read_wrap_marker(settings_path: Path) -> dict[str, Any] | None: + marker = _wrap_marker_path(settings_path) + try: + rec = json.loads(_read_text(marker)) + except (OSError, ValueError): + return None + return rec if isinstance(rec, dict) else None + + +def _wrap_marker_is_stale(marker: dict[str, Any]) -> bool: + """True if ``marker`` describes a writer that is provably gone. + + Missing/invalid pid, a dead pid, or a live pid whose recorded identity no + longer matches (PID reuse) all count as stale — the entry it describes was + left behind by a wrap session that no longer exists. + """ + pid = marker.get("pid") + if not isinstance(pid, int): + return True + if not _pid_alive(pid): + return True + return _identity_mismatch(marker.get("start_src"), marker.get("start_time"), pid) + + +def _clear_wrap_marker(settings_path: Path, *, key: str) -> None: + marker = _read_wrap_marker(settings_path) + if marker is not None and marker.get("key") == key: + _wrap_marker_path(settings_path).unlink(missing_ok=True) + + +def _check_and_clear_stale_wrap_marker(settings_path: Path, *, key: str) -> str | None: + """If a stale wrap marker for ``key`` exists, restore its recorded prior + value and clear the marker. Returns the restored value, or None if there + was nothing stale to clean up. + + Called before writing a fresh base_url entry so a crashed wrap session's + leftover doesn't get treated as this session's own state to restore later. + """ + marker = _read_wrap_marker(settings_path) + if marker is None or marker.get("key") != key or not _wrap_marker_is_stale(marker): + return None + previous = marker.get("previous") + click.echo( + f"headroom: clearing stale {key} left by crashed wrap session (pid {marker.get('pid')})", + err=True, + ) + _restore_claude_wrap_base_url(previous, settings_path=settings_path, _key_override=key) + return previous + + def _write_claude_wrap_base_url( proxy_url: str, *, foundry_mode: bool = False, vertex_mode: bool = False, settings_path: Path | None = None, + port: int | None = None, ) -> str | None: """Persist proxy URL into project-local settings env key for daemon child inheritance. @@ -863,6 +945,10 @@ def _write_claude_wrap_base_url( initial launch — routes through the Headroom proxy without touching the global user settings file or affecting sessions in other projects. Returns the previous value so the caller can restore it on exit (issue #951). + + When ``port`` is given, also stamps a sidecar marker recording this + process's identity and the previous value, so a later crash can be + detected and self-healed (issue #1768). """ path = settings_path or (Path.cwd() / ".claude" / "settings.local.json") payload: dict[str, Any] = {} @@ -880,6 +966,8 @@ def _write_claude_wrap_base_url( payload["env"] = env_map path.parent.mkdir(parents=True, exist_ok=True) _write_text(path, json.dumps(payload, indent=2) + "\n") + if port is not None: + _write_wrap_marker(path, port=port, key=key, previous=previous) return previous @@ -889,16 +977,22 @@ def _restore_claude_wrap_base_url( foundry_mode: bool = False, vertex_mode: bool = False, settings_path: Path | None = None, + _key_override: str | None = None, ) -> None: """Restore (or remove) the env key written by _write_claude_wrap_base_url. Called in both the wrap-session finally block and unwrap_claude so the project-local settings entry is never left pointing at a dead proxy. When ``previous`` is None the key is removed; when it has a value it is - restored — preserving any URL the project already had set. + restored — preserving any URL the project already had set. Also clears + this key's sidecar wrap marker, if any (issue #1768). """ path = settings_path or (Path.cwd() / ".claude" / "settings.local.json") + key = _key_override or _claude_wrap_base_url_env_key( + foundry_mode=foundry_mode, vertex_mode=vertex_mode + ) if not path.exists(): + _clear_wrap_marker(path, key=key) return try: payload = json.loads(_read_text(path)) @@ -909,9 +1003,9 @@ def _restore_claude_wrap_base_url( env_map = payload.get("env") if not isinstance(env_map, dict): return - key = _claude_wrap_base_url_env_key(foundry_mode=foundry_mode, vertex_mode=vertex_mode) if previous is None: if key not in env_map: + _clear_wrap_marker(path, key=key) return del env_map[key] if env_map: @@ -925,6 +1019,7 @@ def _restore_claude_wrap_base_url( _write_text(path, json.dumps(payload, indent=2) + "\n") else: path.unlink(missing_ok=True) + _clear_wrap_marker(path, key=key) def _setup_headroom_mcp( @@ -3072,26 +3167,33 @@ def _pid_alive(pid: int) -> bool: return pid_alive(pid) +def _identity_mismatch(src: Any, recorded: Any, pid: int) -> bool: + """True only if ``pid``'s current identity *provably* differs from the + recorded ``(src, recorded)`` identity (i.e. the PID was recycled). + + Conservative by design: any uncertainty (unknown/legacy identity, unknown + start time, mismatched source) returns ``False`` — never claim a mismatch + without proof, since the caller uses this to decide whether to trust or + discard state tied to a live PID. + """ + if not isinstance(src, str) or not isinstance(recorded, int | float): + return False # legacy / identity-less record — can't tell + ident = _proc_identity(pid) + if ident is None or ident[0] != src: + return False # can't compare like-for-like — don't claim mismatch + # Start times are stable per process; >1s apart means a different process. + return abs(ident[1] - float(recorded)) > 1.0 + + def _marker_pid_reused(marker: Path, pid: int) -> bool: """True only if the live ``pid`` is *provably* a different process than the one that wrote ``marker`` (i.e. the PID was recycled after a crash). - - Conservative by design: any uncertainty (legacy marker, unknown start time, - mismatched source) returns ``False`` so a real client is never pruned. """ try: rec = json.loads(_read_text(marker)) except (OSError, ValueError): return False - src = rec.get("start_src") - recorded = rec.get("start_time") - if not isinstance(src, str) or not isinstance(recorded, int | float): - return False # legacy / identity-less marker — can't tell - ident = _proc_identity(pid) - if ident is None or ident[0] != src: - return False # can't compare like-for-like — don't prune - # Start times are stable per process; >1s apart means a different process. - return abs(ident[1] - float(recorded)) > 1.0 + return _identity_mismatch(rec.get("start_src"), rec.get("start_time"), pid) def _live_proxy_clients(port: int, *, exclude_self: bool = True) -> list[int]: @@ -3600,6 +3702,10 @@ def claude( _register_proxy_client(port) signal.signal(signal.SIGINT, _ignore_child_sigint) signal.signal(signal.SIGTERM, cleanup) + if hasattr(signal, "SIGHUP"): + # Terminal close / tmux kill-session sends SIGHUP, not SIGTERM — without + # this, the finally block's base_url restore never runs (issue #1768). + signal.signal(signal.SIGHUP, cleanup) # Memory sync BEFORE proxy startup — sync headroom DB ↔ Claude's files if memory: @@ -3764,6 +3870,13 @@ def claude( # daemon's environment) also route through Headroom. _settings_vertex[0] = bool(use_vertex) _settings_foundry[0] = bool(foundry_upstream) and not _settings_vertex[0] + _wrap_settings_path = Path.cwd() / ".claude" / "settings.local.json" + _check_and_clear_stale_wrap_marker( + _wrap_settings_path, + key=_claude_wrap_base_url_env_key( + foundry_mode=_settings_foundry[0], vertex_mode=_settings_vertex[0] + ), + ) _saved_base_url[0] = _write_claude_wrap_base_url( ( _foundry_proxy_url(proxy_url) @@ -3774,6 +3887,8 @@ def claude( ), foundry_mode=_settings_foundry[0], vertex_mode=_settings_vertex[0], + settings_path=_wrap_settings_path, + port=port, ) # Per-project savings attribution: tag every request with the launch @@ -3817,6 +3932,7 @@ def claude( _saved_base_url[0], foundry_mode=_settings_foundry[0], vertex_mode=_settings_vertex[0], + settings_path=_wrap_settings_path, ) cleanup() @@ -3884,9 +4000,19 @@ def unwrap_claude( else: click.echo(" Kept rtk Claude hooks (--keep-rtk).") - _restore_claude_wrap_base_url(None) - _restore_claude_wrap_base_url(None, foundry_mode=True) - _restore_claude_wrap_base_url(None, vertex_mode=True) + _unwrap_settings_path = Path.cwd() / ".claude" / "settings.local.json" + for _foundry, _vertex in ((False, False), (True, False), (False, True)): + _key = _claude_wrap_base_url_env_key(foundry_mode=_foundry, vertex_mode=_vertex) + _marker = _read_wrap_marker(_unwrap_settings_path) + _prior = ( + _marker.get("previous") if _marker is not None and _marker.get("key") == _key else None + ) + _restore_claude_wrap_base_url( + _prior, + foundry_mode=_foundry, + vertex_mode=_vertex, + settings_path=_unwrap_settings_path, + ) click.echo() click.echo("✓ Claude is no longer durably wrapped by Headroom.") diff --git a/tests/test_cli/test_unwrap_claude.py b/tests/test_cli/test_unwrap_claude.py index 00003567a..0a9d2877c 100644 --- a/tests/test_cli/test_unwrap_claude.py +++ b/tests/test_cli/test_unwrap_claude.py @@ -223,10 +223,26 @@ def test_unwrap_claude_restores_all_base_url_modes(runner: CliRunner) -> None: ) assert result.exit_code == 0, result.output + settings_path = Path.cwd() / ".claude" / "settings.local.json" assert restore_calls == [ - {"previous": None}, - {"previous": None, "foundry_mode": True}, - {"previous": None, "vertex_mode": True}, + { + "previous": None, + "foundry_mode": False, + "vertex_mode": False, + "settings_path": settings_path, + }, + { + "previous": None, + "foundry_mode": True, + "vertex_mode": False, + "settings_path": settings_path, + }, + { + "previous": None, + "foundry_mode": False, + "vertex_mode": True, + "settings_path": settings_path, + }, ] diff --git a/tests/test_cli/test_wrap_claude_base_url.py b/tests/test_cli/test_wrap_claude_base_url.py index c03a82c06..3639f6040 100644 --- a/tests/test_cli/test_wrap_claude_base_url.py +++ b/tests/test_cli/test_wrap_claude_base_url.py @@ -190,3 +190,94 @@ def test_write_restore_roundtrip(tmp_path: Path) -> None: assert "ANTHROPIC_BASE_URL" not in payload.get("env", {}) assert payload["env"]["OTHER"] == "x" assert payload["model"] == "opus" + + +# --- stale wrap marker (issue #1768) -------------------------------------- + + +def _marker(tmp_path: Path) -> Path: + return wrap_cli._wrap_marker_path(_settings(tmp_path)) + + +def test_write_with_port_creates_marker(tmp_path: Path) -> None: + path = _settings(tmp_path) + wrap_cli._write_claude_wrap_base_url("http://127.0.0.1:8787", settings_path=path, port=8787) + marker = json.loads(_marker(tmp_path).read_text(encoding="utf-8")) + assert marker["port"] == 8787 + assert marker["key"] == "ANTHROPIC_BASE_URL" + assert marker["previous"] is None + assert marker["pid"] > 0 + + +def test_write_without_port_skips_marker(tmp_path: Path) -> None: + path = _settings(tmp_path) + wrap_cli._write_claude_wrap_base_url("http://127.0.0.1:8787", settings_path=path) + assert not _marker(tmp_path).exists() + + +def test_restore_clears_marker_for_matching_key(tmp_path: Path) -> None: + path = _settings(tmp_path) + wrap_cli._write_claude_wrap_base_url("http://127.0.0.1:8787", settings_path=path, port=8787) + assert _marker(tmp_path).exists() + wrap_cli._restore_claude_wrap_base_url(None, settings_path=path) + assert not _marker(tmp_path).exists() + + +def test_wrap_marker_is_stale_when_pid_missing() -> None: + assert wrap_cli._wrap_marker_is_stale({}) is True + + +def test_wrap_marker_is_stale_when_pid_dead(tmp_path: Path) -> None: + path = _settings(tmp_path) + wrap_cli._write_claude_wrap_base_url("http://127.0.0.1:8787", settings_path=path, port=8787) + marker = json.loads(_marker(tmp_path).read_text(encoding="utf-8")) + marker["pid"] = 999_999_999 # astronomically unlikely to be a live pid + assert wrap_cli._wrap_marker_is_stale(marker) is True + + +def test_wrap_marker_is_not_stale_for_live_pid(tmp_path: Path) -> None: + path = _settings(tmp_path) + wrap_cli._write_claude_wrap_base_url("http://127.0.0.1:8787", settings_path=path, port=8787) + marker = json.loads(_marker(tmp_path).read_text(encoding="utf-8")) + assert wrap_cli._wrap_marker_is_stale(marker) is False + + +def test_wrap_marker_is_stale_when_pid_reused(tmp_path: Path) -> None: + path = _settings(tmp_path) + wrap_cli._write_claude_wrap_base_url("http://127.0.0.1:8787", settings_path=path, port=8787) + marker = json.loads(_marker(tmp_path).read_text(encoding="utf-8")) + marker["start_time"] = (marker["start_time"] or 0) - 10_000 # fabricate a mismatched identity + assert wrap_cli._wrap_marker_is_stale(marker) is True + + +def test_check_and_clear_stale_wrap_marker_restores_previous(tmp_path: Path) -> None: + path = _settings(tmp_path) + path.parent.mkdir(parents=True) + path.write_text( + json.dumps({"env": {"ANTHROPIC_BASE_URL": "http://old.proxy:9000"}}), encoding="utf-8" + ) + wrap_cli._write_wrap_marker( + path, port=8787, key="ANTHROPIC_BASE_URL", previous="http://old.proxy:9000" + ) + marker = json.loads(_marker(tmp_path).read_text(encoding="utf-8")) + marker["pid"] = 999_999_999 + _marker(tmp_path).write_text(json.dumps(marker), encoding="utf-8") + + restored = wrap_cli._check_and_clear_stale_wrap_marker(path, key="ANTHROPIC_BASE_URL") + assert restored == "http://old.proxy:9000" + payload = json.loads(path.read_text(encoding="utf-8")) + assert payload["env"]["ANTHROPIC_BASE_URL"] == "http://old.proxy:9000" + assert not _marker(tmp_path).exists() + + +def test_check_and_clear_stale_wrap_marker_leaves_live_marker(tmp_path: Path) -> None: + path = _settings(tmp_path) + wrap_cli._write_claude_wrap_base_url("http://127.0.0.1:8787", settings_path=path, port=8787) + restored = wrap_cli._check_and_clear_stale_wrap_marker(path, key="ANTHROPIC_BASE_URL") + assert restored is None + assert _marker(tmp_path).exists() + + +def test_check_and_clear_stale_wrap_marker_noop_when_no_marker(tmp_path: Path) -> None: + path = _settings(tmp_path) + assert wrap_cli._check_and_clear_stale_wrap_marker(path, key="ANTHROPIC_BASE_URL") is None diff --git a/tests/test_cli/test_wrap_stale_marker.py b/tests/test_cli/test_wrap_stale_marker.py new file mode 100644 index 000000000..966c551b2 --- /dev/null +++ b/tests/test_cli/test_wrap_stale_marker.py @@ -0,0 +1,52 @@ +from __future__ import annotations + +import json +from pathlib import Path + +from headroom.cli import doctor as doctor_cli +from headroom.cli import wrap as wrap_cli + + +def _settings(tmp_path: Path) -> Path: + return tmp_path / ".claude" / "settings.local.json" + + +def test_doctor_skips_with_no_marker(tmp_path: Path) -> None: + result = doctor_cli.check_wrap_marker_staleness(_settings(tmp_path)) + assert result.status == doctor_cli.SKIP + + +def test_doctor_passes_with_live_marker(tmp_path: Path) -> None: + path = _settings(tmp_path) + wrap_cli._write_claude_wrap_base_url("http://127.0.0.1:8787", settings_path=path, port=8787) + result = doctor_cli.check_wrap_marker_staleness(path) + assert result.status == doctor_cli.PASS + + +def test_doctor_flags_stale_wrap_marker(tmp_path: Path) -> None: + path = _settings(tmp_path) + wrap_cli._write_claude_wrap_base_url("http://127.0.0.1:8787", settings_path=path, port=8787) + marker_path = wrap_cli._wrap_marker_path(path) + marker = json.loads(marker_path.read_text(encoding="utf-8")) + marker["pid"] = 999_999_999 + marker_path.write_text(json.dumps(marker), encoding="utf-8") + + result = doctor_cli.check_wrap_marker_staleness(path) + assert result.status == doctor_cli.WARN + assert "999999999" in result.summary + assert "headroom unwrap claude" in result.summary + + +def test_claude_command_registers_sighup_next_to_sigterm() -> None: + """`claude()` must catch SIGHUP (terminal close) the same way it catches + SIGTERM, or a crashed-by-terminal-close wrap session never restores its + base_url (issue #1768). Full signal delivery isn't practical to exercise + via CliRunner (would require spawning/killing a real subprocess), so this + asserts the registration is present in claude()'s source, guarded for + platforms without SIGHUP. + """ + import inspect + + src = inspect.getsource(wrap_cli.claude.callback) + assert 'hasattr(signal, "SIGHUP")' in src + assert "signal.signal(signal.SIGHUP, cleanup)" in src