From d726f50ace163a8f58cec10ae05485d91a61541d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Boschi?= Date: Mon, 7 Sep 2026 16:14:16 +0200 Subject: [PATCH 1/4] fix(docker): drop curl from the API runtime images curl's only in-container consumer was the readiness loop in start-all.sh; there is no HEALTHCHECK instruction anywhere. It is also the sole reverse-dependency of libcurl4t64, which brings libssh2-1t64, so one line in each install list accounted for nine HIGH findings - all status=affected with no Debian fix published, so `apt-get upgrade` could not clear them and not shipping the package was the only remediation. Trivy 0.74.0 HIGH+CRITICAL, on locally built slim images: api-only 3C / 60H -> 3C / 51H standalone 3C / 60H -> 3C / 51H with exactly the curl, libcurl4t64 and libssh2-1t64 findings removed and nothing new. Replace it with http_probe, which reproduces `curl -sf` WITHOUT -L rather than approximating it. The distinction matters: curl does not follow redirects unless asked, so a 302 is a completed transfer and succeeds regardless of what it points at. urllib.request.urlopen follows it and raises on a 404 behind it, which would report a healthy service that redirects as "not ready". http_probe uses http.client and tests `status < 400` itself, bypassing urllib's redirect handler, and carries userinfo through as Basic auth the way curl does. Verified equivalent to `curl -sf` on 2xx, 3xx-to-good, 3xx-to-bad, 4xx, 5xx, query strings, userinfo auth and connection refused. Exit codes are not reproduced (curl's 22 and 7 become 1); every call site tests zero/non-zero. One deliberate difference, since it is a change and not a translation: curl was called 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 never answered hung the probe forever. The timeout now covers the whole request. There is no wget fallback. BusyBox wget cannot reproduce these semantics (no --max-redirect, so it always follows), and it is not needed: every image that probes anything is Python-based. cp-only, the one image with neither, performs no probe at all and dropped curl in #4197. Missing python3 now fails loudly at startup instead of degrading into a readiness loop that can never succeed. Closes #4198 --- docker/standalone/Dockerfile | 4 +- docker/standalone/Dockerfile.freethreaded | 2 +- docker/standalone/start-all.sh | 77 +++++++++++++++- docker/standalone/test-start-all.sh | 106 +++++++++++++++++++++- 4 files changed, 182 insertions(+), 7 deletions(-) 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..5bfca456c4 100755 --- a/docker/standalone/start-all.sh +++ b/docker/standalone/start-all.sh @@ -124,6 +124,77 @@ resolve_api_startup_wait_seconds() { echo "$DEFAULT_API_STARTUP_WAIT_SECONDS" } +# ============================================================================= +# HTTP readiness probe +# +# Replaces `curl -sf`, so that curl - which nothing else in these images uses - +# does not have to ship in the runtime layers. Every image that probes anything +# is Python-based, so python3 is always present; there is deliberately no wget +# fallback, because BusyBox wget cannot reproduce the semantics below (it has no +# --max-redirect and always follows). +# +# The semantics are 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. Following it instead would +# turn a healthy service that redirects into "not ready". +# - >= 400 fails, as -f does. +# - connection/DNS/timeout errors fail. +# Exit codes are only ever tested for zero/non-zero, so curl's distinct codes +# (22, 7, ...) are not reproduced. +# +# One deliberate difference: curl was called 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 would hang the +# probe forever. The timeout here covers the whole request. +http_probe() { + local url="$1" + local timeout_seconds="${2:-5}" + + HTTP_PROBE_URL="$url" HTTP_PROBE_TIMEOUT="$timeout_seconds" python3 -c " +import base64, os, sys +import http.client +from urllib.parse import urlsplit, unquote + +parts = urlsplit(os.environ[\"HTTP_PROBE_URL\"]) +timeout = float(os.environ[\"HTTP_PROBE_TIMEOUT\"]) + +if parts.scheme == \"https\": + conn = http.client.HTTPSConnection(parts.hostname, parts.port, timeout=timeout) +elif parts.scheme == \"http\": + conn = http.client.HTTPConnection(parts.hostname, parts.port, timeout=timeout) +else: + sys.exit(1) + +path = parts.path or \"/\" +if parts.query: + path += \"?\" + parts.query + +headers = {} +if parts.username is not None: + raw = unquote(parts.username) + \":\" + unquote(parts.password or \"\") + headers[\"Authorization\"] = \"Basic \" + base64.b64encode(raw.encode()).decode() + +try: + conn.request(\"GET\", path, headers=headers) + status = conn.getresponse().status +except Exception: + sys.exit(1) +finally: + conn.close() + +sys.exit(0 if status < 400 else 1) +" +} + +# Probing without python3 would silently degrade into "never ready", so check +# once, up front, where it can still say why. +require_http_probe_runtime() { + if ! command -v python3 >/dev/null 2>&1; then + echo "❌ python3 is required for HTTP readiness probes but is not on PATH." + exit 1 + fi +} + if [ "${HINDSIGHT_START_ALL_SOURCE_ONLY:-false}" = "true" ]; then return 0 2>/dev/null || exit 0 fi @@ -143,6 +214,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 +239,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 +335,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 +352,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..e18926f53f 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,100 @@ assert_empty() { fi } +# ============================================================================= +# http_probe +# +# The contract is `curl -sf` without -L, which this replaced. The redirect +# cases are the ones that matter: curl does not follow redirects unless asked, +# so a 302 is a completed transfer and succeeds no matter what it points at. +# A probe that followed them would report a healthy service as "not ready". +# +# The server runs until the trap kills it - it is deliberately not a +# "handle N requests" loop, because then adding a test case here would make the +# script block forever on the request the server had stopped waiting for. +# ============================================================================= +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): + 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 == "/500": + self.send_response(500) + else: + self.send_response(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")" + +assert_probe_succeeds() { + if ! http_probe "$HTTP_TEST_URL$1" 5 >/dev/null 2>&1; then + echo "http_probe should succeed for $1 (curl -sf does)" + exit 1 + fi +} + +assert_probe_fails() { + if http_probe "$HTTP_TEST_URL$1" 5 >/dev/null 2>&1; then + echo "http_probe should fail for $1 (curl -sf does)" + exit 1 + fi +} + +assert_probe_succeeds "/ok" +assert_probe_succeeds "/redirect-to-ok" +# The regression this guards: urllib.request.urlopen follows the redirect and +# raises on the 404 behind it, where curl -sf reports success. +assert_probe_succeeds "/redirect-to-missing" +assert_probe_fails "/missing" +assert_probe_fails "/500" + +# Nothing listening: a connection error fails like curl's exit 7. +CLOSED_PORT_URL="http://127.0.0.1:$(python3 -c ' +import socket +s = socket.socket() +s.bind(("127.0.0.1", 0)) +port = s.getsockname()[1] +s.close() +print(port) +')/ok" +if http_probe "$CLOSED_PORT_URL" 2 >/dev/null 2>&1; then + echo "http_probe should fail when nothing is listening" + exit 1 +fi + +echo "start-all HTTP probe checks passed" + mkdir -p "$TMP_DIR/empty" assert_empty "$(check_pg0_data_integrity "$TMP_DIR/empty")" From c6ea13e32606a99e0a101d6fd686734d4e19138a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Boschi?= Date: Mon, 7 Sep 2026 16:58:08 +0200 Subject: [PATCH 2/4] refactor(docker): move the readiness probe into hindsight_api.http_probe The first version of this probe was Python embedded in a shell string inside start-all.sh. That was a bad shape for code encoding rules this fiddly: every quote had to survive two levels of escaping, ruff and ty never saw it, and it could only be exercised through the shell. Move it to hindsight_api/http_probe.py, shipped with the code and covered by tests/test_http_probe.py, which pins each case to what `curl -sf` does for the same response. start-all.sh keeps a three-line wrapper that shells out to `python3 -m hindsight_api.http_probe`. hindsight-admin was the obvious home and is the wrong one: it takes 5.1s to start in the built image, against 0.028s for bare stdlib, because it pulls in the CLI and everything behind it. The readiness loop polls once per second, so importing the API to ask whether the API is up would break the loop it drives. This module imports stdlib only; measured 0.035s per probe in the image. `hindsight_api/__init__` is cheap by design and has to stay that way for this to hold - its docstring already says so. The shell test drops to checking the wiring, since the semantics now have a real home, and skips when the package is not importable: test-start-all.sh also runs in CI from a bare checkout with no virtualenv. Reformatting by `ruff format` on first contact is the point - the embedded version could never have received it. --- .github/workflows/test.yml | 58 ++++++--- docker/standalone/start-all.sh | 68 ++--------- docker/standalone/test-start-all.sh | 100 ++++++---------- .../hindsight_api/http_probe.py | 106 ++++++++++++++++ hindsight-api-slim/tests/test_http_probe.py | 113 ++++++++++++++++++ 5 files changed, 308 insertions(+), 137 deletions(-) create mode 100644 hindsight-api-slim/hindsight_api/http_probe.py create mode 100644 hindsight-api-slim/tests/test_http_probe.py 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/start-all.sh b/docker/standalone/start-all.sh index 5bfca456c4..2103781870 100755 --- a/docker/standalone/start-all.sh +++ b/docker/standalone/start-all.sh @@ -127,70 +127,26 @@ resolve_api_startup_wait_seconds() { # ============================================================================= # HTTP readiness probe # -# Replaces `curl -sf`, so that curl - which nothing else in these images uses - -# does not have to ship in the runtime layers. Every image that probes anything -# is Python-based, so python3 is always present; there is deliberately no wget -# fallback, because BusyBox wget cannot reproduce the semantics below (it has no -# --max-redirect and always follows). +# 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. # -# The semantics are 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. Following it instead would -# turn a healthy service that redirects into "not ready". -# - >= 400 fails, as -f does. -# - connection/DNS/timeout errors fail. -# Exit codes are only ever tested for zero/non-zero, so curl's distinct codes -# (22, 7, ...) are not reproduced. -# -# One deliberate difference: curl was called 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 would hang the -# probe forever. The timeout here covers the whole request. +# Every image that probes anything ships the API package, so `python3 -m` finds +# it. cp-only has neither and probes nothing. +# ============================================================================= http_probe() { local url="$1" local timeout_seconds="${2:-5}" - HTTP_PROBE_URL="$url" HTTP_PROBE_TIMEOUT="$timeout_seconds" python3 -c " -import base64, os, sys -import http.client -from urllib.parse import urlsplit, unquote - -parts = urlsplit(os.environ[\"HTTP_PROBE_URL\"]) -timeout = float(os.environ[\"HTTP_PROBE_TIMEOUT\"]) - -if parts.scheme == \"https\": - conn = http.client.HTTPSConnection(parts.hostname, parts.port, timeout=timeout) -elif parts.scheme == \"http\": - conn = http.client.HTTPConnection(parts.hostname, parts.port, timeout=timeout) -else: - sys.exit(1) - -path = parts.path or \"/\" -if parts.query: - path += \"?\" + parts.query - -headers = {} -if parts.username is not None: - raw = unquote(parts.username) + \":\" + unquote(parts.password or \"\") - headers[\"Authorization\"] = \"Basic \" + base64.b64encode(raw.encode()).decode() - -try: - conn.request(\"GET\", path, headers=headers) - status = conn.getresponse().status -except Exception: - sys.exit(1) -finally: - conn.close() - -sys.exit(0 if status < 400 else 1) -" + python3 -m hindsight_api.http_probe "$url" "$timeout_seconds" } -# Probing without python3 would silently degrade into "never ready", so check -# once, up front, where it can still say why. +# 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 ! command -v python3 >/dev/null 2>&1; then - echo "❌ python3 is required for HTTP readiness probes but is not on PATH." + if ! python3 -c "import hindsight_api.http_probe" >/dev/null 2>&1; then + echo "❌ HTTP readiness probes need python3 with hindsight_api importable." exit 1 fi } diff --git a/docker/standalone/test-start-all.sh b/docker/standalone/test-start-all.sh index e18926f53f..314f5ce5a9 100755 --- a/docker/standalone/test-start-all.sh +++ b/docker/standalone/test-start-all.sh @@ -55,20 +55,20 @@ assert_empty() { } # ============================================================================= -# http_probe +# http_probe wiring # -# The contract is `curl -sf` without -L, which this replaced. The redirect -# cases are the ones that matter: curl does not follow redirects unless asked, -# so a 302 is a completed transfer and succeeds no matter what it points at. -# A probe that followed them would report a healthy service as "not ready". +# 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. # -# The server runs until the trap kills it - it is deliberately not a -# "handle N requests" loop, because then adding a test case here would make the -# script block forever on the request the server had stopped waiting for. +# Skipped when the API package is not importable - this file also runs in CI +# from a bare checkout with no virtualenv, where only the pg0 helpers below +# are exercisable. # ============================================================================= -HTTP_PORT_FILE="$TMP_DIR/http-port" +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' & + python3 - "$HTTP_PORT_FILE" <<'PY' & import http.server import pathlib import sys @@ -76,18 +76,7 @@ import sys class Handler(http.server.BaseHTTPRequestHandler): def do_GET(self): - 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 == "/500": - self.send_response(500) - else: - self.send_response(404) + self.send_response(204 if self.path == "/ok" else 404) self.end_headers() def log_message(self, *_args): @@ -98,56 +87,39 @@ 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")" - -assert_probe_succeeds() { - if ! http_probe "$HTTP_TEST_URL$1" 5 >/dev/null 2>&1; then - echo "http_probe should succeed for $1 (curl -sf does)" + 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")" -assert_probe_fails() { - if http_probe "$HTTP_TEST_URL$1" 5 >/dev/null 2>&1; then - echo "http_probe should fail for $1 (curl -sf does)" + 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 -} -assert_probe_succeeds "/ok" -assert_probe_succeeds "/redirect-to-ok" -# The regression this guards: urllib.request.urlopen follows the redirect and -# raises on the 404 behind it, where curl -sf reports success. -assert_probe_succeeds "/redirect-to-missing" -assert_probe_fails "/missing" -assert_probe_fails "/500" - -# Nothing listening: a connection error fails like curl's exit 7. -CLOSED_PORT_URL="http://127.0.0.1:$(python3 -c ' -import socket -s = socket.socket() -s.bind(("127.0.0.1", 0)) -port = s.getsockname()[1] -s.close() -print(port) -')/ok" -if http_probe "$CLOSED_PORT_URL" 2 >/dev/null 2>&1; then - echo "http_probe should fail when nothing is listening" - exit 1 + 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 not importable)" fi -echo "start-all HTTP probe checks passed" - 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..05d8899ddf --- /dev/null +++ b/hindsight-api-slim/hindsight_api/http_probe.py @@ -0,0 +1,106 @@ +"""HTTP readiness probe for the container entrypoint. + +``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 purely 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 replaces it. It lives here, rather than as Python embedded in the shell +script, so that it is linted, type-checked and unit-tested like the rest of the +package: the embedded version could only be exercised through the shell, and +every quote in it had to survive two levels of escaping. + +**Only the standard library may be imported here.** The probe runs once per +second in the readiness loop, so import cost is the budget: ``python -m +hindsight_api.http_probe`` measures ~0.12s in the built image, almost all of it +interpreter startup, and pulling in anything from the engine would blow that. +``hindsight_api/__init__`` is cheap by design (see its docstring) and must stay +that way for this to hold. + +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 + +DEFAULT_TIMEOUT_SECONDS = 5.0 + + +def probe(url: str, timeout_seconds: float = DEFAULT_TIMEOUT_SECONDS) -> bool: + """Return True if ``url`` answers like ``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..7200072734 --- /dev/null +++ b/hindsight-api-slim/tests/test_http_probe.py @@ -0,0 +1,113 @@ +"""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 socket +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 From 18ea2a3c8f5b57c9b3e864ac3b9e0c1e7ba511d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Boschi?= Date: Tue, 8 Sep 2026 09:33:02 +0200 Subject: [PATCH 3/4] refactor(probe): make the readiness probe its own package, isolated from the API hindsight_api.http_probe was the wrong home. The probe answers "is an API process up?", and living inside the package it probes invited exactly the coupling that would break it: an import of the engine or the config would put API startup cost - and API startup side effects - on a loop that runs once a second. Move it to hindsight_probe, a sibling top-level package in the same distribution. Its dependencies are now explicit by construction: none. It imports the standard library and nothing else. Packaging alone does not enforce that. Both packages install into the same virtualenv, so `import hindsight_api` from the probe would still resolve at runtime. So the rule is a test, not a convention: test_imports_nothing_but_the_standard_library imports the package in a clean subprocess and asserts that nothing outside sys.stdlib_module_names was pulled in. Adding `import hindsight_api` to the probe fails it with the offending name. The audit ignores _sysconfigdata_*, a platform-specific stdlib internal whose name embeds the build triple and so is absent from stdlib_module_names everywhere. Both Dockerfiles now copy the package; the api-builder previously copied only hindsight_api, so the first build without this shipped an image whose probe could not import. That surfaced as require_http_probe_runtime failing at startup with a clear message rather than a readiness loop that could never succeed, which is what that guard is for. Verified in the built Linux image: no non-stdlib imports, hindsight_api never loaded, 0.036s per probe, and the full end-to-end boot still reaches "Hindsight is running" with /health answering 200. --- docker/standalone/Dockerfile | 3 + docker/standalone/Dockerfile.freethreaded | 3 + docker/standalone/start-all.sh | 13 +++-- docker/standalone/test-start-all.sh | 6 +- .../__init__.py} | 48 ++++++++-------- .../hindsight_probe/__main__.py | 8 +++ hindsight-api-slim/pyproject.toml | 6 +- ..._http_probe.py => test_hindsight_probe.py} | 55 ++++++++++++++++++- 8 files changed, 108 insertions(+), 34 deletions(-) rename hindsight-api-slim/{hindsight_api/http_probe.py => hindsight_probe/__init__.py} (65%) create mode 100644 hindsight-api-slim/hindsight_probe/__main__.py rename hindsight-api-slim/tests/{test_http_probe.py => test_hindsight_probe.py} (67%) diff --git a/docker/standalone/Dockerfile b/docker/standalone/Dockerfile index 9bf4415eda..2d806ab95a 100644 --- a/docker/standalone/Dockerfile +++ b/docker/standalone/Dockerfile @@ -68,6 +68,9 @@ RUN if [ "$INCLUDE_LOCAL_MODELS" = "true" ]; then \ # Copy source code (alembic migrations are inside hindsight_api/) WORKDIR /app/api COPY hindsight-api-slim/hindsight_api ./hindsight_api +# Sibling top-level package, not part of hindsight_api: the readiness probe the +# entrypoint shells out to. See hindsight_probe/__init__.py for why it is separate. +COPY hindsight-api-slim/hindsight_probe ./hindsight_probe # Install the local package from the same validated lock after source is present. WORKDIR /app diff --git a/docker/standalone/Dockerfile.freethreaded b/docker/standalone/Dockerfile.freethreaded index a54aff02c1..024ca2fcc1 100644 --- a/docker/standalone/Dockerfile.freethreaded +++ b/docker/standalone/Dockerfile.freethreaded @@ -67,6 +67,9 @@ COPY hindsight-api-slim/README.md ./api/ # Source is needed to build the package, so this is one step rather than # deps-then-source; `uv pip install` has no --no-install-package. COPY hindsight-api-slim/hindsight_api ./api/hindsight_api +# Sibling top-level package, not part of hindsight_api: the readiness probe the +# entrypoint shells out to. See hindsight_probe/__init__.py for why it is separate. +COPY hindsight-api-slim/hindsight_probe ./api/hindsight_probe # Not `uv sync --locked`: uv.lock pins the resolution for the default interpreter and # this one resolves different wheels. That is exactly why this ships as its own tag diff --git a/docker/standalone/start-all.sh b/docker/standalone/start-all.sh index 2103781870..4a10322eb9 100755 --- a/docker/standalone/start-all.sh +++ b/docker/standalone/start-all.sh @@ -127,26 +127,27 @@ resolve_api_startup_wait_seconds() { # ============================================================================= # HTTP readiness probe # -# The implementation is hindsight_api.http_probe, not Python embedded here: it +# The implementation is the hindsight_probe package, 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. cp-only has neither and probes nothing. +# Every image that probes anything ships that package, so `python3 -m` finds +# it. It imports stdlib only and never the API - 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" + python3 -m hindsight_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 importable." + if ! python3 -c "import hindsight_probe" >/dev/null 2>&1; then + echo "❌ HTTP readiness probes need python3 with hindsight_probe importable." exit 1 fi } diff --git a/docker/standalone/test-start-all.sh b/docker/standalone/test-start-all.sh index 314f5ce5a9..b9ac74663e 100755 --- a/docker/standalone/test-start-all.sh +++ b/docker/standalone/test-start-all.sh @@ -61,11 +61,11 @@ assert_empty() { # 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 the API package is not importable - this file also runs in CI +# Skipped when hindsight_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 +if python3 -c "import hindsight_probe" >/dev/null 2>&1; then HTTP_PORT_FILE="$TMP_DIR/http-port" python3 - "$HTTP_PORT_FILE" <<'PY' & @@ -117,7 +117,7 @@ PY HTTP_SERVER_PID="" echo "start-all HTTP probe wiring checks passed" else - echo "start-all HTTP probe wiring checks skipped (hindsight_api not importable)" + echo "start-all HTTP probe wiring checks skipped (hindsight_probe not importable)" fi mkdir -p "$TMP_DIR/empty" diff --git a/hindsight-api-slim/hindsight_api/http_probe.py b/hindsight-api-slim/hindsight_probe/__init__.py similarity index 65% rename from hindsight-api-slim/hindsight_api/http_probe.py rename to hindsight-api-slim/hindsight_probe/__init__.py index 05d8899ddf..9bdb14d758 100644 --- a/hindsight-api-slim/hindsight_api/http_probe.py +++ b/hindsight-api-slim/hindsight_probe/__init__.py @@ -1,22 +1,28 @@ -"""HTTP readiness probe for the container entrypoint. +"""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 purely 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 replaces it. It lives here, rather than as Python embedded in the shell -script, so that it is linted, type-checked and unit-tested like the rest of the -package: the embedded version could only be exercised through the shell, and -every quote in it had to survive two levels of escaping. - -**Only the standard library may be imported here.** The probe runs once per -second in the readiness loop, so import cost is the budget: ``python -m -hindsight_api.http_probe`` measures ~0.12s in the built image, almost all of it -interpreter startup, and pulling in anything from the engine would blow that. -``hindsight_api/__init__`` is cheap by design (see its docstring) and must stay -that way for this to hold. +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 package deliberately does not belong to ``hindsight_api``.** It is a +sibling top-level package in the same distribution, and it must never import +the API - not the engine, not the config, not the package root. Two reasons, +both load-bearing: + +* **Startup cost.** The readiness loop runs this once per second. Bare + interpreter startup is ~0.03s; ``hindsight-admin``, which pulls in the CLI + and everything behind it, takes ~5s in the built image. A probe that costs + more than the interval it runs on breaks the loop it exists to drive. +* **What it is probing.** This asks whether an API process is up. Importing the + API to do so risks initialising the very machinery whose absence it is + meant to detect. + +``tests/test_hindsight_probe.py::test_imports_nothing_but_the_standard_library`` +enforces this in a clean subprocess rather than trusting the convention: the +distribution installs both packages into the same virtualenv, so nothing at the +packaging layer would stop an ``import hindsight_api`` here from resolving. The contract is ``curl -sf`` *without* ``-L``, which is what this replaced: @@ -45,11 +51,13 @@ 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 like ``curl -sf`` would call success. + """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. @@ -88,7 +96,7 @@ def probe(url: str, timeout_seconds: float = DEFAULT_TIMEOUT_SECONDS) -> bool: 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) + print("usage: python -m hindsight_probe URL [TIMEOUT_SECONDS]", file=sys.stderr) return 2 timeout_seconds = DEFAULT_TIMEOUT_SECONDS @@ -100,7 +108,3 @@ def main(argv: list[str] | None = None) -> int: return 2 return 0 if probe(args[0], timeout_seconds) else 1 - - -if __name__ == "__main__": - raise SystemExit(main()) diff --git a/hindsight-api-slim/hindsight_probe/__main__.py b/hindsight-api-slim/hindsight_probe/__main__.py new file mode 100644 index 0000000000..69d5d1fde8 --- /dev/null +++ b/hindsight-api-slim/hindsight_probe/__main__.py @@ -0,0 +1,8 @@ +"""Entry point for ``python -m hindsight_probe``.""" + +from __future__ import annotations + +from hindsight_probe import main + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/hindsight-api-slim/pyproject.toml b/hindsight-api-slim/pyproject.toml index 8c072de7d3..563a887ad1 100644 --- a/hindsight-api-slim/pyproject.toml +++ b/hindsight-api-slim/pyproject.toml @@ -174,20 +174,24 @@ hindsight-local-mcp = "hindsight_api.mcp_local:main" hindsight-admin = "hindsight_api.admin.cli:main" [tool.hatch.build.targets.wheel] -packages = ["hindsight_api"] +# hindsight_probe is a separate top-level package on purpose - see its docstring. +packages = ["hindsight_api", "hindsight_probe"] [tool.hatch.build.targets.wheel.sources] "hindsight_api" = "hindsight_api" +"hindsight_probe" = "hindsight_probe" [tool.hatch.build.targets.sdist] include = [ "hindsight_api/**/*", + "hindsight_probe/**/*", ] [tool.hatch.build] include = [ "hindsight_api/**/*.py", "hindsight_api/alembic/**/*", + "hindsight_probe/**/*.py", ] [tool.pytest.ini_options] diff --git a/hindsight-api-slim/tests/test_http_probe.py b/hindsight-api-slim/tests/test_hindsight_probe.py similarity index 67% rename from hindsight-api-slim/tests/test_http_probe.py rename to hindsight-api-slim/tests/test_hindsight_probe.py index 7200072734..0b0a731340 100644 --- a/hindsight-api-slim/tests/test_http_probe.py +++ b/hindsight-api-slim/tests/test_hindsight_probe.py @@ -1,7 +1,7 @@ """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 +because that is what `hindsight_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 @@ -12,13 +12,16 @@ 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 +from hindsight_probe import main, probe _EXPECTED_AUTH = "Basic " + base64.b64encode(b"user:pa ss").decode() @@ -111,3 +114,51 @@ def test_main_exit_codes(base_url: str, closed_port: int) -> None: 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 never reach into the API. Both packages install into the same +# virtualenv, so nothing at the packaging layer stops `import hindsight_api` +# here from resolving - only this test does. See hindsight_probe's docstring for +# why it matters: the loop runs once a second, and importing the API to ask +# whether the API is up risks starting the machinery it is checking for. +_IMPORT_AUDIT = """ +import json, sys + +before = set(sys.modules) +import hindsight_probe # noqa: F401 +new_top_level = {name.split(".")[0] for name in set(sys.modules) - before} +# _sysconfigdata_* is a platform-specific stdlib internal whose name embeds the +# build triple, so it is absent from stdlib_module_names on every platform. +print(json.dumps(sorted( + name for name in new_top_level + if name not in sys.stdlib_module_names + and name != "hindsight_probe" + and not name.startswith("_sysconfigdata") +))) +""" + + +def test_imports_nothing_but_the_standard_library() -> None: + """Importing the probe must pull in no third-party module, and no API module.""" + result = subprocess.run( + [sys.executable, "-c", _IMPORT_AUDIT], + capture_output=True, + text=True, + check=True, + timeout=60, + ) + assert json.loads(result.stdout) == [], ( + f"hindsight_probe must import only the standard library; it pulled in {result.stdout.strip()}" + ) + + +def test_runnable_as_a_module() -> None: + """`python -m hindsight_probe` is how start-all.sh invokes it.""" + result = subprocess.run( + [sys.executable, "-m", "hindsight_probe"], + capture_output=True, + text=True, + timeout=60, + ) + assert result.returncode == 2 + assert "usage: python -m hindsight_probe" in result.stderr From 1bd2f8b0ae451a8c4ae05f94ad98fb762a4b7772 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Boschi?= Date: Tue, 8 Sep 2026 10:31:25 +0200 Subject: [PATCH 4/4] refactor(probe): keep the readiness probe inside hindsight_api Reverts the separate hindsight_probe package. It was justified on a bad measurement: an earlier cold-cache timing suggested `import hindsight_api` cost ~0.12s against ~0.03s for a standalone package. Measured properly, warm, in the built image, they are the same - ~0.03s each - and importing hindsight_api pulls in zero third-party modules. Its PEP 562 lazy-attribute design already does the work the split was meant to do, so the split bought nothing and cost a second top-level package, four pyproject entries and a COPY in each Dockerfile. What was worth keeping is the enforcement, which is orthogonal to where the module lives. test_imports_nothing_heavy imports the probe in a clean subprocess and asserts it pulled in no third-party package and nothing from hindsight_api.engine, .api or .config. Adding `from hindsight_api.engine import memory_engine` to the probe fails it with 43 packages named, numpy, sqlalchemy and asyncpg among them - which is the failure mode the rule exists to prevent. Verified in the built image: no third-party or engine imports, 0.034s per import, probe wiring works, curl absent. --- docker/standalone/Dockerfile | 3 - docker/standalone/Dockerfile.freethreaded | 3 - docker/standalone/start-all.sh | 14 ++--- docker/standalone/test-start-all.sh | 6 +- .../http_probe.py} | 35 ++++++------ .../hindsight_probe/__main__.py | 8 --- hindsight-api-slim/pyproject.toml | 6 +- ..._hindsight_probe.py => test_http_probe.py} | 57 +++++++++++-------- 8 files changed, 61 insertions(+), 71 deletions(-) rename hindsight-api-slim/{hindsight_probe/__init__.py => hindsight_api/http_probe.py} (74%) delete mode 100644 hindsight-api-slim/hindsight_probe/__main__.py rename hindsight-api-slim/tests/{test_hindsight_probe.py => test_http_probe.py} (71%) diff --git a/docker/standalone/Dockerfile b/docker/standalone/Dockerfile index 2d806ab95a..9bf4415eda 100644 --- a/docker/standalone/Dockerfile +++ b/docker/standalone/Dockerfile @@ -68,9 +68,6 @@ RUN if [ "$INCLUDE_LOCAL_MODELS" = "true" ]; then \ # Copy source code (alembic migrations are inside hindsight_api/) WORKDIR /app/api COPY hindsight-api-slim/hindsight_api ./hindsight_api -# Sibling top-level package, not part of hindsight_api: the readiness probe the -# entrypoint shells out to. See hindsight_probe/__init__.py for why it is separate. -COPY hindsight-api-slim/hindsight_probe ./hindsight_probe # Install the local package from the same validated lock after source is present. WORKDIR /app diff --git a/docker/standalone/Dockerfile.freethreaded b/docker/standalone/Dockerfile.freethreaded index 024ca2fcc1..a54aff02c1 100644 --- a/docker/standalone/Dockerfile.freethreaded +++ b/docker/standalone/Dockerfile.freethreaded @@ -67,9 +67,6 @@ COPY hindsight-api-slim/README.md ./api/ # Source is needed to build the package, so this is one step rather than # deps-then-source; `uv pip install` has no --no-install-package. COPY hindsight-api-slim/hindsight_api ./api/hindsight_api -# Sibling top-level package, not part of hindsight_api: the readiness probe the -# entrypoint shells out to. See hindsight_probe/__init__.py for why it is separate. -COPY hindsight-api-slim/hindsight_probe ./api/hindsight_probe # Not `uv sync --locked`: uv.lock pins the resolution for the default interpreter and # this one resolves different wheels. That is exactly why this ships as its own tag diff --git a/docker/standalone/start-all.sh b/docker/standalone/start-all.sh index 4a10322eb9..b8d0ce739c 100755 --- a/docker/standalone/start-all.sh +++ b/docker/standalone/start-all.sh @@ -127,27 +127,27 @@ resolve_api_startup_wait_seconds() { # ============================================================================= # HTTP readiness probe # -# The implementation is the hindsight_probe package, not Python embedded here: it +# 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 that package, so `python3 -m` finds -# it. It imports stdlib only and never the API - see its docstring. cp-only has -# no Python at all and probes nothing. +# 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_probe "$url" "$timeout_seconds" + 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_probe" >/dev/null 2>&1; then - echo "❌ HTTP readiness probes need python3 with hindsight_probe importable." + 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 } diff --git a/docker/standalone/test-start-all.sh b/docker/standalone/test-start-all.sh index b9ac74663e..a7a45aca66 100755 --- a/docker/standalone/test-start-all.sh +++ b/docker/standalone/test-start-all.sh @@ -61,11 +61,11 @@ assert_empty() { # 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_probe is not importable - this file also runs in CI +# 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_probe" >/dev/null 2>&1; then +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' & @@ -117,7 +117,7 @@ PY HTTP_SERVER_PID="" echo "start-all HTTP probe wiring checks passed" else - echo "start-all HTTP probe wiring checks skipped (hindsight_probe not importable)" + echo "start-all HTTP probe wiring checks skipped (hindsight_api.http_probe not importable)" fi mkdir -p "$TMP_DIR/empty" diff --git a/hindsight-api-slim/hindsight_probe/__init__.py b/hindsight-api-slim/hindsight_api/http_probe.py similarity index 74% rename from hindsight-api-slim/hindsight_probe/__init__.py rename to hindsight-api-slim/hindsight_api/http_probe.py index 9bdb14d758..c6dc7ec0ee 100644 --- a/hindsight-api-slim/hindsight_probe/__init__.py +++ b/hindsight-api-slim/hindsight_api/http_probe.py @@ -6,23 +6,18 @@ else in those images used it, and the three packages carried nine HIGH CVEs with no Debian fix available. -**This package deliberately does not belong to ``hindsight_api``.** It is a -sibling top-level package in the same distribution, and it must never import -the API - not the engine, not the config, not the package root. Two reasons, -both load-bearing: - -* **Startup cost.** The readiness loop runs this once per second. Bare - interpreter startup is ~0.03s; ``hindsight-admin``, which pulls in the CLI - and everything behind it, takes ~5s in the built image. A probe that costs - more than the interval it runs on breaks the loop it exists to drive. -* **What it is probing.** This asks whether an API process is up. Importing the - API to do so risks initialising the very machinery whose absence it is - meant to detect. - -``tests/test_hindsight_probe.py::test_imports_nothing_but_the_standard_library`` -enforces this in a clean subprocess rather than trusting the convention: the -distribution installs both packages into the same virtualenv, so nothing at the -packaging layer would stop an ``import hindsight_api`` here from resolving. +**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: @@ -96,7 +91,7 @@ def probe(url: str, timeout_seconds: float = DEFAULT_TIMEOUT_SECONDS) -> bool: 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_probe URL [TIMEOUT_SECONDS]", file=sys.stderr) + print("usage: python -m hindsight_api.http_probe URL [TIMEOUT_SECONDS]", file=sys.stderr) return 2 timeout_seconds = DEFAULT_TIMEOUT_SECONDS @@ -108,3 +103,7 @@ def main(argv: list[str] | None = None) -> int: return 2 return 0 if probe(args[0], timeout_seconds) else 1 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/hindsight-api-slim/hindsight_probe/__main__.py b/hindsight-api-slim/hindsight_probe/__main__.py deleted file mode 100644 index 69d5d1fde8..0000000000 --- a/hindsight-api-slim/hindsight_probe/__main__.py +++ /dev/null @@ -1,8 +0,0 @@ -"""Entry point for ``python -m hindsight_probe``.""" - -from __future__ import annotations - -from hindsight_probe import main - -if __name__ == "__main__": - raise SystemExit(main()) diff --git a/hindsight-api-slim/pyproject.toml b/hindsight-api-slim/pyproject.toml index 563a887ad1..8c072de7d3 100644 --- a/hindsight-api-slim/pyproject.toml +++ b/hindsight-api-slim/pyproject.toml @@ -174,24 +174,20 @@ hindsight-local-mcp = "hindsight_api.mcp_local:main" hindsight-admin = "hindsight_api.admin.cli:main" [tool.hatch.build.targets.wheel] -# hindsight_probe is a separate top-level package on purpose - see its docstring. -packages = ["hindsight_api", "hindsight_probe"] +packages = ["hindsight_api"] [tool.hatch.build.targets.wheel.sources] "hindsight_api" = "hindsight_api" -"hindsight_probe" = "hindsight_probe" [tool.hatch.build.targets.sdist] include = [ "hindsight_api/**/*", - "hindsight_probe/**/*", ] [tool.hatch.build] include = [ "hindsight_api/**/*.py", "hindsight_api/alembic/**/*", - "hindsight_probe/**/*.py", ] [tool.pytest.ini_options] diff --git a/hindsight-api-slim/tests/test_hindsight_probe.py b/hindsight-api-slim/tests/test_http_probe.py similarity index 71% rename from hindsight-api-slim/tests/test_hindsight_probe.py rename to hindsight-api-slim/tests/test_http_probe.py index 0b0a731340..5ece4e0184 100644 --- a/hindsight-api-slim/tests/test_hindsight_probe.py +++ b/hindsight-api-slim/tests/test_http_probe.py @@ -1,7 +1,7 @@ """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_probe` replaced in +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 @@ -21,7 +21,7 @@ import pytest -from hindsight_probe import main, probe +from hindsight_api.http_probe import main, probe _EXPECTED_AUTH = "Basic " + base64.b64encode(b"user:pa ss").decode() @@ -116,30 +116,35 @@ def test_main_exit_codes(base_url: str, closed_port: int) -> None: assert main([f"{base_url}/ok", "not-a-number"]) == 2 -# The probe must never reach into the API. Both packages install into the same -# virtualenv, so nothing at the packaging layer stops `import hindsight_api` -# here from resolving - only this test does. See hindsight_probe's docstring for -# why it matters: the loop runs once a second, and importing the API to ask -# whether the API is up risks starting the machinery it is checking for. +# 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_probe # noqa: F401 -new_top_level = {name.split(".")[0] for name in set(sys.modules) - before} -# _sysconfigdata_* is a platform-specific stdlib internal whose name embeds the -# build triple, so it is absent from stdlib_module_names on every platform. -print(json.dumps(sorted( - name for name in new_top_level - if name not in sys.stdlib_module_names - and name != "hindsight_probe" - and not name.startswith("_sysconfigdata") -))) +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_but_the_standard_library() -> None: - """Importing the probe must pull in no third-party module, and no API module.""" +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, @@ -147,18 +152,22 @@ def test_imports_nothing_but_the_standard_library() -> None: check=True, timeout=60, ) - assert json.loads(result.stdout) == [], ( - f"hindsight_probe must import only the standard library; it pulled in {result.stdout.strip()}" + 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_probe` is how start-all.sh invokes it.""" + """`python -m hindsight_api.http_probe` is how start-all.sh invokes it.""" result = subprocess.run( - [sys.executable, "-m", "hindsight_probe"], + [sys.executable, "-m", "hindsight_api.http_probe"], capture_output=True, text=True, timeout=60, ) assert result.returncode == 2 - assert "usage: python -m hindsight_probe" in result.stderr + assert "usage: python -m hindsight_api.http_probe" in result.stderr