diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 65ab52faf3..87eeb9da73 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -1859,23 +1859,6 @@ jobs: # with the GIL quietly re-enabled. DISABLE_SQLALCHEMY_CEXT_RUNTIME: "1" - services: - # For the image smoke test at the end of the job. The pytest run above uses the - # embedded pg0, not this. - postgres: - image: pgvector/pgvector:pg17 - env: - POSTGRES_USER: hindsight - POSTGRES_PASSWORD: hindsight - POSTGRES_DB: hindsight - ports: - - 5432:5432 - options: >- - --health-cmd "pg_isready -U hindsight" - --health-interval 5s - --health-timeout 5s - --health-retries 10 - steps: - uses: actions/checkout@v6 with: @@ -1942,6 +1925,47 @@ jobs: ./.venv-ft/bin/python -m pytest tests -v \ -m "not hs_llm_mat and not hs_llm_core" + build-freethreaded-image: + needs: [detect-changes] + # Gated on `docker` as well as `core`: this is the only job that builds + # Dockerfile.freethreaded, and it used to hang off the free-threaded pytest job, + # which does not run for a docker-only change. Two Dockerfile.freethreaded PRs + # merged without CI ever building the image before this was split out. + # + # Deliberately NOT gated on `has_secrets`, for the same reason as the pytest job + # above: the smoke test runs a mock LLM and a stub embedder, so it needs no + # provider credentials and therefore also covers fork PRs, which skip + # build-docker-images entirely. + if: >- + github.event_name == 'workflow_dispatch' || + needs.detect-changes.outputs.core == 'true' || + needs.detect-changes.outputs.docker == 'true' || + needs.detect-changes.outputs.ci == 'true' + runs-on: ubuntu-latest + timeout-minutes: 30 + name: build-freethreaded-image + + services: + # For the smoke test; the image runs against this rather than embedded pg0. + postgres: + image: pgvector/pgvector:pg17 + env: + POSTGRES_USER: hindsight + POSTGRES_PASSWORD: hindsight + POSTGRES_DB: hindsight + ports: + - 5432:5432 + options: >- + --health-cmd "pg_isready -U hindsight" + --health-interval 5s + --health-timeout 5s + --health-retries 10 + + steps: + - uses: actions/checkout@v6 + with: + ref: ${{ github.event.pull_request.head.sha || '' }} + - name: Build the free-threaded image # Its own Dockerfile, not a target of the standalone one: different interpreter, # different resolution, no control plane, no local ML. The build itself asserts diff --git a/docker/standalone/Dockerfile b/docker/standalone/Dockerfile index 4e101df417..9bf4415eda 100644 --- a/docker/standalone/Dockerfile +++ b/docker/standalone/Dockerfile @@ -168,7 +168,6 @@ WORKDIR /app # commit can now resolve different package versions. RUN apt-get update && apt-get upgrade -y \ && apt-get install -y \ - curl \ procps \ libssl3 \ libgssapi-krb5-2 \ @@ -310,7 +309,7 @@ FROM python:3.11-slim AS standalone WORKDIR /app -# Install curl, uv, and system dependencies. Node arrives separately, below. +# Install uv and system dependencies. Node arrives separately, below. # Note: libicu version varies by Debian version - try common versions in order # Runtime images use uv directly; remove pip build tooling after installation so # vulnerable setuptools-vendored packages and wheel are not shipped in production. @@ -321,7 +320,6 @@ WORKDIR /app # commit can now resolve different package versions. RUN apt-get update && apt-get upgrade -y \ && apt-get install -y \ - curl \ procps \ libssl3 \ libgssapi-krb5-2 \ diff --git a/docker/standalone/Dockerfile.freethreaded b/docker/standalone/Dockerfile.freethreaded index 33c6fadcbc..a54aff02c1 100644 --- a/docker/standalone/Dockerfile.freethreaded +++ b/docker/standalone/Dockerfile.freethreaded @@ -97,7 +97,7 @@ FROM python:3.14-slim AS api-only-freethreaded WORKDIR /app RUN apt-get update && apt-get upgrade -y \ - && apt-get install -y --no-install-recommends curl procps libssl3 libpq5 \ + && apt-get install -y --no-install-recommends procps libssl3 libpq5 \ && rm -rf /var/lib/apt/lists/* \ && useradd -m -u 1000 hindsight diff --git a/docker/standalone/start-all.sh b/docker/standalone/start-all.sh index 35cdb1e5d5..b8d0ce739c 100755 --- a/docker/standalone/start-all.sh +++ b/docker/standalone/start-all.sh @@ -124,6 +124,34 @@ resolve_api_startup_wait_seconds() { echo "$DEFAULT_API_STARTUP_WAIT_SECONDS" } +# ============================================================================= +# HTTP readiness probe +# +# The implementation is hindsight_api.http_probe, not Python embedded here: it +# needs to be linted, type-checked and unit-tested, and the parity rules it +# encodes (notably that `curl -sf` does NOT follow redirects) are too easy to +# get subtly wrong to leave in a shell string. See that module's docstring. +# +# Every image that probes anything ships the API package, so `python3 -m` finds +# it. It is held to stdlib-only imports by a test - see its docstring. cp-only +# has no Python at all and probes nothing. +# ============================================================================= +http_probe() { + local url="$1" + local timeout_seconds="${2:-5}" + + python3 -m hindsight_api.http_probe "$url" "$timeout_seconds" +} + +# A probe that cannot run at all would silently degrade into "never ready", so +# check once, up front, where it can still say why. +require_http_probe_runtime() { + if ! python3 -c "import hindsight_api.http_probe" >/dev/null 2>&1; then + echo "❌ HTTP readiness probes need python3 with hindsight_api.http_probe importable." + exit 1 + fi +} + if [ "${HINDSIGHT_START_ALL_SOURCE_ONLY:-false}" = "true" ]; then return 0 2>/dev/null || exit 0 fi @@ -143,6 +171,7 @@ ENABLE_CP="${HINDSIGHT_ENABLE_CP:-true}" # This wait loop ensures dependencies are ready before starting. # ============================================================================= if [ "${HINDSIGHT_WAIT_FOR_DEPS:-false}" = "true" ]; then + require_http_probe_runtime LLM_BASE_URL="${HINDSIGHT_API_LLM_BASE_URL:-http://host.docker.internal:1234/v1}" MAX_RETRIES="${HINDSIGHT_RETRY_MAX:-0}" # 0 = infinite RETRY_INTERVAL="${HINDSIGHT_RETRY_INTERVAL:-10}" @@ -167,7 +196,7 @@ if [ "${HINDSIGHT_WAIT_FOR_DEPS:-false}" = "true" ]; then } check_llm() { - curl -sf "${LLM_BASE_URL}/models" --connect-timeout 5 &>/dev/null + http_probe "${LLM_BASE_URL}/models" 5 &>/dev/null } echo "⏳ Waiting for dependencies to be ready..." @@ -263,6 +292,7 @@ PIDS=() # Start API if enabled if [ "$ENABLE_API" = "true" ]; then + require_http_probe_runtime cd /app/api API_HEALTH_URL="${HINDSIGHT_API_HEALTH_URL:-http://localhost:${HINDSIGHT_API_PORT:-8888}/health}" API_STARTUP_WAIT_SECONDS="$(resolve_api_startup_wait_seconds)" @@ -279,7 +309,7 @@ if [ "$ENABLE_API" = "true" ]; then wait "$API_PID" exit $? fi - if curl -sf "$API_HEALTH_URL" &>/dev/null; then + if http_probe "$API_HEALTH_URL" 5 &>/dev/null; then api_ready=true break fi diff --git a/docker/standalone/test-start-all.sh b/docker/standalone/test-start-all.sh index d733dd457c..a7a45aca66 100755 --- a/docker/standalone/test-start-all.sh +++ b/docker/standalone/test-start-all.sh @@ -8,7 +8,17 @@ source "$SCRIPT_DIR/start-all.sh" unset HINDSIGHT_START_ALL_SOURCE_ONLY TMP_DIR="$(mktemp -d)" -trap 'chmod -R u+rwx "$TMP_DIR" 2>/dev/null || true; rm -rf "$TMP_DIR"' EXIT +HTTP_SERVER_PID="" + +cleanup() { + if [ -n "$HTTP_SERVER_PID" ]; then + kill "$HTTP_SERVER_PID" 2>/dev/null || true + wait "$HTTP_SERVER_PID" 2>/dev/null || true + fi + chmod -R u+rwx "$TMP_DIR" 2>/dev/null || true + rm -rf "$TMP_DIR" +} +trap cleanup EXIT assert_contains() { local output="$1" @@ -44,6 +54,72 @@ assert_empty() { fi } +# ============================================================================= +# http_probe wiring +# +# The probe's semantics are covered by pytest, against the module that +# implements them: hindsight-api-slim/tests/test_http_probe.py. All that is +# left to check here is that this script delegates to it correctly. +# +# Skipped when hindsight_api.http_probe is not importable - this file also runs in CI +# from a bare checkout with no virtualenv, where only the pg0 helpers below +# are exercisable. +# ============================================================================= +if python3 -c "import hindsight_api.http_probe" >/dev/null 2>&1; then + HTTP_PORT_FILE="$TMP_DIR/http-port" + + python3 - "$HTTP_PORT_FILE" <<'PY' & +import http.server +import pathlib +import sys + + +class Handler(http.server.BaseHTTPRequestHandler): + def do_GET(self): + self.send_response(204 if self.path == "/ok" else 404) + self.end_headers() + + def log_message(self, *_args): + pass + + +server = http.server.ThreadingHTTPServer(("127.0.0.1", 0), Handler) +pathlib.Path(sys.argv[1]).write_text(str(server.server_port), encoding="ascii") +server.serve_forever() +PY + HTTP_SERVER_PID=$! + + for _ in $(seq 1 50); do + [ -s "$HTTP_PORT_FILE" ] && break + sleep 0.1 + done + if [ ! -s "$HTTP_PORT_FILE" ]; then + echo "HTTP probe test server did not start" + exit 1 + fi + HTTP_TEST_URL="http://127.0.0.1:$(cat "$HTTP_PORT_FILE")" + + if ! http_probe "$HTTP_TEST_URL/ok" 5 >/dev/null 2>&1; then + echo "http_probe should succeed against a healthy endpoint" + exit 1 + fi + if http_probe "$HTTP_TEST_URL/missing" 5 >/dev/null 2>&1; then + echo "http_probe should fail against a 404" + exit 1 + fi + if ! require_http_probe_runtime; then + echo "require_http_probe_runtime should pass when the module imports" + exit 1 + fi + + kill "$HTTP_SERVER_PID" 2>/dev/null || true + wait "$HTTP_SERVER_PID" 2>/dev/null || true + HTTP_SERVER_PID="" + echo "start-all HTTP probe wiring checks passed" +else + echo "start-all HTTP probe wiring checks skipped (hindsight_api.http_probe not importable)" +fi + mkdir -p "$TMP_DIR/empty" assert_empty "$(check_pg0_data_integrity "$TMP_DIR/empty")" diff --git a/hindsight-api-slim/hindsight_api/http_probe.py b/hindsight-api-slim/hindsight_api/http_probe.py new file mode 100644 index 0000000000..c6dc7ec0ee --- /dev/null +++ b/hindsight-api-slim/hindsight_api/http_probe.py @@ -0,0 +1,109 @@ +"""Container readiness probe. Standard library only, by rule. + +``docker/standalone/start-all.sh`` polls the API's health endpoint while it +starts. That used to be ``curl -sf``, which meant shipping curl - and with it +libcurl and libssh2 - in every runtime image to make one GET request. Nothing +else in those images used it, and the three packages carried nine HIGH CVEs +with no Debian fix available. + +**This module must never import the engine, the config, or any third-party +package.** It answers "is an API process up?", and pulling the application in +to ask that would put the application's startup cost - and its side effects - +on a loop that runs once per second. ``hindsight-admin`` is the cautionary +number: it takes ~5s to start in the built image because it loads the CLI and +everything behind it, against ~0.03s for this. + +That rule is a test, not a convention: +``tests/test_http_probe.py::test_imports_nothing_heavy`` imports this module in +a clean subprocess and asserts that it pulled in no third-party module and none +of the engine. Living next to the code it must not touch is exactly why the +check is automated. + +The contract is ``curl -sf`` *without* ``-L``, which is what this replaced: + +* 2xx and 3xx succeed. curl does not follow redirects unless asked, so a 302 is + a completed transfer, not a failure. This matters more than it looks: + ``urllib.request.urlopen`` *does* follow redirects, so the obvious + implementation reports a healthy service that redirects as "not ready". +* 4xx and 5xx fail, as ``-f`` does. +* Connection, DNS and timeout errors fail. +* Credentials in the URL are sent as Basic auth, as curl does. + +Exit codes are not reproduced - curl's 22 and 7 both become 1. Every call site +tests zero/non-zero only. + +One deliberate difference from what it replaced: curl was invoked with +``--connect-timeout``, which caps only the connection phase, and the API health +loop passed no timeout at all - so a server that accepted a connection and then +never answered hung the probe forever. The timeout here covers the whole +request. +""" + +from __future__ import annotations + +import base64 +import http.client +import sys +from urllib.parse import unquote, urlsplit + +__all__ = ["DEFAULT_TIMEOUT_SECONDS", "main", "probe"] + +DEFAULT_TIMEOUT_SECONDS = 5.0 + + +def probe(url: str, timeout_seconds: float = DEFAULT_TIMEOUT_SECONDS) -> bool: + """Return True if ``url`` answers the way ``curl -sf`` would call success. + + Never raises: a probe that blew up on an unexpected socket error would be + indistinguishable from a crash in the readiness loop that calls it. + """ + parts = urlsplit(url) + + if parts.scheme == "https": + connection: http.client.HTTPConnection = http.client.HTTPSConnection( + parts.hostname or "", parts.port, timeout=timeout_seconds + ) + elif parts.scheme == "http": + connection = http.client.HTTPConnection(parts.hostname or "", parts.port, timeout=timeout_seconds) + else: + return False + + path = parts.path or "/" + if parts.query: + path = f"{path}?{parts.query}" + + headers: dict[str, str] = {} + if parts.username is not None: + raw = f"{unquote(parts.username)}:{unquote(parts.password or '')}" + headers["Authorization"] = "Basic " + base64.b64encode(raw.encode()).decode() + + try: + connection.request("GET", path, headers=headers) + status = connection.getresponse().status + except Exception: + return False + finally: + connection.close() + + return status < 400 + + +def main(argv: list[str] | None = None) -> int: + args = sys.argv[1:] if argv is None else argv + if not args or len(args) > 2: + print("usage: python -m hindsight_api.http_probe URL [TIMEOUT_SECONDS]", file=sys.stderr) + return 2 + + timeout_seconds = DEFAULT_TIMEOUT_SECONDS + if len(args) == 2: + try: + timeout_seconds = float(args[1]) + except ValueError: + print(f"invalid timeout: {args[1]}", file=sys.stderr) + return 2 + + return 0 if probe(args[0], timeout_seconds) else 1 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/hindsight-api-slim/tests/test_http_probe.py b/hindsight-api-slim/tests/test_http_probe.py new file mode 100644 index 0000000000..5ece4e0184 --- /dev/null +++ b/hindsight-api-slim/tests/test_http_probe.py @@ -0,0 +1,173 @@ +"""Parity tests for the container readiness probe. + +Each case is pinned to what `curl -sf` (without -L) does for the same response, +because that is what `hindsight_api.http_probe` replaced in +`docker/standalone/start-all.sh`. The redirect cases are the point: the obvious +`urllib.request.urlopen` implementation follows redirects and fails on a 404 +behind one, where curl reports success - so a healthy service that redirects +would be reported as "not ready". +""" + +from __future__ import annotations + +import base64 +import http.server +import json +import socket +import subprocess +import sys +import threading +from collections.abc import Iterator + +import pytest + +from hindsight_api.http_probe import main, probe + +_EXPECTED_AUTH = "Basic " + base64.b64encode(b"user:pa ss").decode() + + +class _Handler(http.server.BaseHTTPRequestHandler): + def do_GET(self) -> None: # noqa: N802 - name fixed by BaseHTTPRequestHandler + if self.path == "/ok": + self.send_response(204) + elif self.path == "/redirect-to-ok": + self.send_response(302) + self.send_header("Location", "/ok") + elif self.path == "/redirect-to-missing": + self.send_response(302) + self.send_header("Location", "/missing") + elif self.path == "/server-error": + self.send_response(500) + elif self.path == "/query": + self.send_response(204 if "expected=1" in self.path else 400) + elif self.path.startswith("/query?"): + self.send_response(204 if "expected=1" in self.path else 400) + elif self.path == "/auth": + got = self.headers.get("Authorization") + self.send_response(204 if got == _EXPECTED_AUTH else 401) + else: + self.send_response(404) + self.end_headers() + + def log_message(self, *_args: object) -> None: + """Silence the default stderr access log.""" + + +@pytest.fixture(scope="module") +def base_url() -> Iterator[str]: + server = http.server.ThreadingHTTPServer(("127.0.0.1", 0), _Handler) + thread = threading.Thread(target=server.serve_forever, daemon=True) + thread.start() + try: + yield f"http://127.0.0.1:{server.server_port}" + finally: + server.shutdown() + server.server_close() + thread.join(timeout=5) + + +@pytest.fixture(scope="module") +def closed_port() -> int: + sock = socket.socket() + sock.bind(("127.0.0.1", 0)) + port = int(sock.getsockname()[1]) + sock.close() + return port + + +@pytest.mark.parametrize( + ("path", "expected", "why"), + [ + ("/ok", True, "2xx succeeds"), + ("/redirect-to-ok", True, "curl does not follow redirects; a 302 is a completed transfer"), + ( + "/redirect-to-missing", + True, + "still a 302 to curl, which never sees the 404 behind it - urlopen would fail here", + ), + ("/missing", False, "curl -f fails on 4xx"), + ("/server-error", False, "curl -f fails on 5xx"), + ("/query?expected=1", True, "query string is preserved"), + ("/query?expected=0", False, "query string is preserved"), + ], +) +def test_matches_curl_sf(base_url: str, path: str, expected: bool, why: str) -> None: + assert probe(f"{base_url}{path}", 5) is expected, why + + +def test_sends_url_credentials_as_basic_auth(base_url: str) -> None: + host = base_url.removeprefix("http://") + assert probe(f"http://user:pa%20ss@{host}/auth", 5) is True + + +def test_connection_refused_fails(closed_port: int) -> None: + assert probe(f"http://127.0.0.1:{closed_port}/ok", 2) is False + + +def test_unsupported_scheme_fails() -> None: + assert probe("ftp://127.0.0.1/ok", 2) is False + + +def test_main_exit_codes(base_url: str, closed_port: int) -> None: + assert main([f"{base_url}/ok"]) == 0 + assert main([f"{base_url}/missing"]) == 1 + assert main([f"http://127.0.0.1:{closed_port}/ok", "2"]) == 1 + assert main([]) == 2 + assert main([f"{base_url}/ok", "not-a-number"]) == 2 + + +# The probe must not drag the application in behind it. It lives inside +# hindsight_api, so this cannot assert "no hindsight_api" - it asserts the part +# that actually matters: no third-party package, and none of the engine, config +# or API surface. hindsight_api/__init__ is cheap by design (PEP 562 lazy +# attributes, see its docstring) and this test is what keeps the probe from +# being the thing that makes it expensive again. +_IMPORT_AUDIT = """ +import json, sys + +before = set(sys.modules) +import hindsight_api.http_probe # noqa: F401 +loaded = set(sys.modules) - before + +third_party = { + name.split(".")[0] for name in loaded + if name.split(".")[0] not in sys.stdlib_module_names + and not name.startswith("hindsight_api") + and not name.split(".")[0].startswith("_sysconfigdata") +} +heavy = { + name for name in loaded + if name.startswith(("hindsight_api.engine", "hindsight_api.api", "hindsight_api.config")) +} +print(json.dumps({"third_party": sorted(third_party), "heavy": sorted(heavy)})) +""" + + +def test_imports_nothing_heavy() -> None: + """Importing the probe must pull in no third-party package and none of the engine.""" + result = subprocess.run( + [sys.executable, "-c", _IMPORT_AUDIT], + capture_output=True, + text=True, + check=True, + timeout=60, + ) + loaded = json.loads(result.stdout) + assert loaded["third_party"] == [], ( + f"the readiness probe must not import third-party packages; it pulled in {loaded['third_party']}" + ) + assert loaded["heavy"] == [], ( + f"the readiness probe must not import the engine/API/config; it pulled in {loaded['heavy']}" + ) + + +def test_runnable_as_a_module() -> None: + """`python -m hindsight_api.http_probe` is how start-all.sh invokes it.""" + result = subprocess.run( + [sys.executable, "-m", "hindsight_api.http_probe"], + capture_output=True, + text=True, + timeout=60, + ) + assert result.returncode == 2 + assert "usage: python -m hindsight_api.http_probe" in result.stderr