From ca135c2015ad09d11c0177b1439a2d569c89886e Mon Sep 17 00:00:00 2001 From: Michael Lam Date: Mon, 4 May 2026 15:30:37 -0700 Subject: [PATCH] fix: harden persistent WebUI health checks --- api/routes.py | 140 ++++++++++++++++++-- docs/supervisor.md | 41 ++++++ server.py | 60 +++++++++ tests/test_issue1458_stability_hardening.py | 66 +++++++++ 4 files changed, 296 insertions(+), 11 deletions(-) create mode 100644 tests/test_issue1458_stability_hardening.py diff --git a/api/routes.py b/api/routes.py index 8ef98b63..42a90bbd 100644 --- a/api/routes.py +++ b/api/routes.py @@ -12,6 +12,7 @@ import queue import re import platform import shutil +import sqlite3 import subprocess import sys import threading @@ -19,6 +20,7 @@ import time import uuid import re from pathlib import Path +from contextlib import closing from urllib.parse import parse_qs from api.agent_sessions import MESSAGING_SOURCES @@ -1222,6 +1224,7 @@ from api.models import ( title_from, _write_session_index, SESSION_INDEX_FILE, + _active_state_db_path, load_projects, save_projects, import_cli_session, @@ -1658,6 +1661,131 @@ def _handle_insights(handler, parsed) -> bool: # ── GET routes ──────────────────────────────────────────────────────────────── +def _accept_loop_health(handler) -> dict: + server = getattr(handler, "server", None) + return { + "requests_total": int(getattr(server, "accept_loop_requests_total", 0) or 0), + "last_request_at": round(float(getattr(server, "accept_loop_last_request_at", 0.0) or 0.0), 3), + } + + +def _streams_lock_health(timeout_seconds: float = 0.5) -> dict: + t0 = time.time() + acquired = STREAMS_LOCK.acquire(timeout=timeout_seconds) + elapsed_ms = round((time.time() - t0) * 1000, 1) + if not acquired: + return { + "status": "blocked", + "timeout_seconds": timeout_seconds, + "ms": elapsed_ms, + } + try: + return { + "status": "ok", + "active_streams": len(STREAMS), + "ms": elapsed_ms, + } + finally: + STREAMS_LOCK.release() + + +def _deep_health_checks() -> tuple[dict, bool]: + """Run cheap probes that exercise the state paths used by the UI shell. + + Plain /health intentionally stays tiny. /health?deep=1 is for supervisors + and watchdogs that need to know whether the process can still touch the + shared stream map, sidebar/session path, project state, and Hermes state.db + without hitting the RST-before-write failure mode from #1458. + """ + checks: dict[str, dict] = {} + + checks["streams_lock"] = _streams_lock_health() + if checks["streams_lock"].get("status") != "ok": + return checks, False + + t0 = time.time() + try: + sessions = all_sessions() + checks["sessions"] = { + "status": "ok", + "count": len(sessions), + "ms": round((time.time() - t0) * 1000, 1), + } + except Exception as exc: + checks["sessions"] = { + "status": "error", + "error": type(exc).__name__, + "ms": round((time.time() - t0) * 1000, 1), + } + + t0 = time.time() + try: + projects = load_projects(_migrate=False) + checks["projects"] = { + "status": "ok", + "count": len(projects), + "ms": round((time.time() - t0) * 1000, 1), + } + except Exception as exc: + checks["projects"] = { + "status": "error", + "error": type(exc).__name__, + "ms": round((time.time() - t0) * 1000, 1), + } + + t0 = time.time() + try: + db_path = _active_state_db_path() + if not db_path.exists(): + checks["state_db"] = { + "status": "missing", + "ms": round((time.time() - t0) * 1000, 1), + } + else: + with closing(sqlite3.connect(str(db_path))) as conn: + conn.execute("PRAGMA schema_version").fetchone() + checks["state_db"] = { + "status": "ok", + "ms": round((time.time() - t0) * 1000, 1), + } + except Exception as exc: + checks["state_db"] = { + "status": "error", + "error": type(exc).__name__, + "ms": round((time.time() - t0) * 1000, 1), + } + + healthy = all( + check.get("status") in {"ok", "missing"} + for check in checks.values() + ) + return checks, healthy + + +def _handle_health(handler, parsed): + deep = parse_qs(parsed.query or "").get("deep", [""])[0].lower() in {"1", "true", "yes", "on"} + stream_check = _streams_lock_health() + payload = { + "status": "ok" if stream_check.get("status") == "ok" else "degraded", + "sessions": len(SESSIONS), + "active_streams": int(stream_check.get("active_streams") or 0), + "uptime_seconds": round(time.time() - SERVER_START_TIME, 1), + "accept_loop": _accept_loop_health(handler), + } + if deep: + if stream_check.get("status") != "ok": + payload["checks"] = {"streams_lock": stream_check} + return j(handler, payload, status=503) + checks, healthy = _deep_health_checks() + payload["checks"] = checks + if not healthy: + payload["status"] = "degraded" + return j(handler, payload, status=503) + if payload["status"] != "ok": + return j(handler, payload, status=503) + return j(handler, payload) + + def handle_get(handler, parsed) -> bool: """Handle all GET routes. Returns True if handled, False for 404.""" @@ -1776,17 +1904,7 @@ def handle_get(handler, parsed) -> bool: return _handle_insights(handler, parsed) if parsed.path == "/health": - with STREAMS_LOCK: - n_streams = len(STREAMS) - return j( - handler, - { - "status": "ok", - "sessions": len(SESSIONS), - "active_streams": n_streams, - "uptime_seconds": round(time.time() - SERVER_START_TIME, 1), - }, - ) + return _handle_health(handler, parsed) if parsed.path == "/api/models": return j(handler, get_available_models()) diff --git a/docs/supervisor.md b/docs/supervisor.md index 4ec433b0..85821a69 100644 --- a/docs/supervisor.md +++ b/docs/supervisor.md @@ -235,3 +235,44 @@ PID PPID CMD If PPID is ``1`` (init) when it should be the supervisor, the orphan-server loop is happening — re-check that ``--foreground`` (or one of the env vars) is reaching the process. + +## HTTP watchdog / deep health + +``KeepAlive`` / ``Restart=always`` only recover a process that exits. If the +process is still listening on the port but request handling is wedged, pair your +supervisor with an HTTP probe and force a restart when the probe fails. + +Hermes Web UI exposes two health levels: + +- ``/health`` — cheap liveness probe with ``active_streams``, uptime, and an + ``accept_loop`` heartbeat counter. +- ``/health?deep=1`` — readiness probe that briefly acquires the stream lock, + reads the sidebar/session path, reads projects state, and touches Hermes + ``state.db`` if it exists. Use this for watchdogs. + +At startup the server also tries to raise its file-descriptor soft limit to +4096 on platforms that support ``RLIMIT_NOFILE``. That is defense in depth for +persistent hosts: leaks should still be fixed, but a higher soft limit gives +you more diagnostic headroom before request handling falls over. + +Minimal macOS launchd watchdog script: + +```bash +#!/usr/bin/env bash +set -euo pipefail +LABEL="com.example.hermes-webui" +BASE="http://127.0.0.1:8787" + +if ! curl -fsS --max-time 10 "$BASE/health?deep=1" >/dev/null; then + launchctl kickstart -k "gui/$(id -u)/$LABEL" +fi +``` + +Run it every few minutes from a separate ``StartInterval`` LaunchAgent. For +systemd, prefer a timer/service pair that runs the same curl probe and +``systemctl --user restart hermes-webui.service`` on failure. + +The ``accept_loop.requests_total`` value should increase when probes arrive. If +it stays flat while the process is still alive, the server accept loop is not +making progress; capture logs/thread samples before restarting if you are +collecting diagnostics for a bug report. diff --git a/server.py b/server.py index 16691bb4..5b9a7062 100644 --- a/server.py +++ b/server.py @@ -9,6 +9,11 @@ import sys import time import traceback from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer + +try: + import resource +except ImportError: # pragma: no cover - resource is Unix-only + resource = None from urllib.parse import urlparse logger = logging.getLogger(__name__) @@ -26,6 +31,22 @@ class QuietHTTPServer(ThreadingHTTPServer): """Custom HTTP server that silently handles common network errors.""" daemon_threads = True request_queue_size = 64 + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.accept_loop_requests_total = 0 + self.accept_loop_last_request_at = 0.0 + + def _handle_request_noblock(self): + """Record accept-loop progress before dispatching a request handler. + + A process can be alive and still stop accepting/dispatching requests. + Exposing this heartbeat on /health gives supervisors and watchdogs a + cheap signal that the accept loop is still moving. + """ + self.accept_loop_requests_total += 1 + self.accept_loop_last_request_at = time.time() + return super()._handle_request_noblock() def handle_error(self, request, client_address): """Override to suppress logging for common client disconnect errors.""" @@ -129,11 +150,50 @@ class Handler(BaseHTTPRequestHandler): clear_request_profile() +def _raise_fd_soft_limit(target: int = 4096) -> dict: + """Best-effort raise of RLIMIT_NOFILE for persistent WebUI hosts. + + macOS launchd jobs often start with a 256 soft limit. If a future FD leak + regresses, that low ceiling turns a leak into a hard HTTP wedge quickly. + Raising the soft limit does not hide leaks; it buys enough headroom for + diagnostics and watchdog recovery. + """ + if resource is None: + return {"status": "unsupported"} + try: + soft, hard = resource.getrlimit(resource.RLIMIT_NOFILE) + except Exception as exc: + return {"status": "error", "error": str(exc)} + + # On Unix, RLIM_INFINITY is commonly a large int; keep the logic explicit + # so tests can use ordinary integers without depending on platform values. + desired = int(target) + if hard not in (-1, getattr(resource, "RLIM_INFINITY", object())): + desired = min(desired, int(hard)) + if soft >= desired: + return {"status": "unchanged", "soft": soft, "hard": hard} + try: + resource.setrlimit(resource.RLIMIT_NOFILE, (desired, hard)) + except Exception as exc: + return {"status": "error", "soft": soft, "hard": hard, "error": str(exc)} + return {"status": "raised", "soft": desired, "hard": hard, "previous_soft": soft} + + def main() -> None: from api.config import print_startup_config, verify_hermes_imports, _HERMES_FOUND print_startup_config() + fd_limit = _raise_fd_soft_limit() + if fd_limit.get("status") == "raised": + print( + f"[ok] Raised file descriptor soft limit " + f"{fd_limit.get('previous_soft')} -> {fd_limit.get('soft')}", + flush=True, + ) + elif fd_limit.get("status") == "error": + print(f"[!!] WARNING: Could not raise file descriptor limit: {fd_limit.get('error')}", flush=True) + # Fix sensitive file permissions before doing anything else fix_credential_permissions() diff --git a/tests/test_issue1458_stability_hardening.py b/tests/test_issue1458_stability_hardening.py new file mode 100644 index 00000000..63dd29c6 --- /dev/null +++ b/tests/test_issue1458_stability_hardening.py @@ -0,0 +1,66 @@ +"""Regression coverage for issue #1458 persistent-host hardening.""" +import json +import urllib.request + +from tests._pytest_port import BASE + + +def _get(path): + with urllib.request.urlopen(BASE + path, timeout=10) as r: + return json.loads(r.read()), r.status + + +def test_health_exposes_accept_loop_heartbeat(): + data, status = _get("/health") + + assert status == 200 + heartbeat = data.get("accept_loop") + assert isinstance(heartbeat, dict) + assert isinstance(heartbeat.get("requests_total"), int) + assert heartbeat["requests_total"] >= 1 + assert isinstance(heartbeat.get("last_request_at"), (int, float)) + assert heartbeat["last_request_at"] > 0 + + +def test_deep_health_exercises_session_project_and_sqlite_paths(): + data, status = _get("/health?deep=1") + + assert status == 200 + assert data["status"] == "ok" + checks = data.get("checks") + assert isinstance(checks, dict) + assert checks["streams_lock"]["status"] == "ok" + assert isinstance(checks["streams_lock"].get("active_streams"), int) + assert checks["sessions"]["status"] == "ok" + assert isinstance(checks["sessions"].get("count"), int) + assert checks["projects"]["status"] == "ok" + assert isinstance(checks["projects"].get("count"), int) + # The isolated test home may not have a Hermes state.db yet. Deep health + # should still report the state-db probe explicitly so watchdogs can tell + # whether sqlite was checked or absent. + assert checks["state_db"]["status"] in {"ok", "missing"} + + +def test_server_raises_fd_soft_limit_when_resource_allows(monkeypatch): + import server + + calls = [] + + class FakeResource: + RLIMIT_NOFILE = object() + + @staticmethod + def getrlimit(which): + return (256, 8192) + + @staticmethod + def setrlimit(which, limits): + calls.append((which, limits)) + + monkeypatch.setattr(server, "resource", FakeResource, raising=False) + + result = server._raise_fd_soft_limit(target=4096) + + assert result["status"] == "raised" + assert result["soft"] == 4096 + assert calls == [(FakeResource.RLIMIT_NOFILE, (4096, 8192))]