Files
Nicolò Boschi 5f9bff9056 fix(docker): drop curl from the API runtime images (#4202)
* 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

* 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.

* 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.

* 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.
2026-09-08 11:11:04 +02:00

233 lines
7.7 KiB
Bash
Executable File

#!/bin/bash
set -euo pipefail
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
HINDSIGHT_START_ALL_SOURCE_ONLY=true
source "$SCRIPT_DIR/start-all.sh"
unset HINDSIGHT_START_ALL_SOURCE_ONLY
TMP_DIR="$(mktemp -d)"
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"
local expected="$2"
if [[ "$output" != *"$expected"* ]]; then
echo "Expected output to contain: $expected"
echo "Actual output:"
echo "$output"
exit 1
fi
}
assert_not_contains() {
local output="$1"
local unexpected="$2"
if [[ "$output" == *"$unexpected"* ]]; then
echo "Expected output not to contain: $unexpected"
echo "Actual output:"
echo "$output"
exit 1
fi
}
assert_empty() {
local output="$1"
if [ -n "$output" ]; then
echo "Expected no output, got:"
echo "$output"
exit 1
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")"
mkdir -p "$TMP_DIR/direct"
touch "$TMP_DIR/direct/PG_VERSION"
direct_output="$(check_pg0_data_integrity "$TMP_DIR/direct")"
assert_contains "$direct_output" "Existing pg0 data directory detected"
assert_not_contains "$direct_output" "WARNING"
mkdir -p "$TMP_DIR/legacy/instance"
touch "$TMP_DIR/legacy/instance/PG_VERSION"
legacy_output="$(check_pg0_data_integrity "$TMP_DIR/legacy")"
assert_contains "$legacy_output" "Existing pg0 data directory detected"
assert_not_contains "$legacy_output" "WARNING"
mkdir -p "$TMP_DIR/nested/instances/hindsight/data"
touch "$TMP_DIR/nested/instances/hindsight/data/PG_VERSION"
nested_output="$(check_pg0_data_integrity "$TMP_DIR/nested")"
assert_contains "$nested_output" "Existing pg0 data directory detected"
assert_not_contains "$nested_output" "WARNING"
mkdir -p "$TMP_DIR/nonempty/instances/hindsight"
touch "$TMP_DIR/nonempty/instances/hindsight/instance.json"
nonempty_output="$(check_pg0_data_integrity "$TMP_DIR/nonempty")"
assert_contains "$nonempty_output" "WARNING: pg0 data directory exists"
echo "start-all pg0 integrity checks passed"
# =============================================================================
# resolve_api_startup_wait_seconds (#3733)
# =============================================================================
assert_equals() {
local actual="$1"
local expected="$2"
if [ "$actual" != "$expected" ]; then
echo "Expected: $expected"
echo "Actual: $actual"
exit 1
fi
}
# Neither knob set: the wrapper default.
assert_equals "$(HINDSIGHT_API_STARTUP_WAIT_SECONDS= HINDSIGHT_API_MODEL_INIT_TIMEOUT= resolve_api_startup_wait_seconds)" "300"
# The documented knob raised: the wrapper waits at least that long, so raising
# it actually takes effect instead of being cut short at the default.
assert_equals "$(HINDSIGHT_API_MODEL_INIT_TIMEOUT=7200 resolve_api_startup_wait_seconds)" "7230"
# Floats are accepted — the API parses its cap as one.
assert_equals "$(HINDSIGHT_API_MODEL_INIT_TIMEOUT=7200.0 resolve_api_startup_wait_seconds)" "7230"
# A shorter cap never shortens the wrapper wait.
assert_equals "$(HINDSIGHT_API_MODEL_INIT_TIMEOUT=60 resolve_api_startup_wait_seconds)" "300"
# Garbage falls back to the default rather than breaking startup.
assert_equals "$(HINDSIGHT_API_MODEL_INIT_TIMEOUT=abc resolve_api_startup_wait_seconds)" "300"
# An explicit wrapper setting always wins.
assert_equals "$(HINDSIGHT_API_STARTUP_WAIT_SECONDS=45 HINDSIGHT_API_MODEL_INIT_TIMEOUT=7200 resolve_api_startup_wait_seconds)" "45"
echo "start-all API startup wait checks passed"
# =============================================================================
# check_pg0_writable (#1483)
# These rely on filesystem permissions, which root bypasses; skip under root.
# =============================================================================
if [ "$(id -u)" != "0" ]; then
# Writable directory: returns 0, prints nothing, leaves no artifact behind.
mkdir -p "$TMP_DIR/writable"
writable_output="$(check_pg0_writable "$TMP_DIR/writable")"
assert_empty "$writable_output"
if [ -e "$TMP_DIR/writable/.hindsight-write-test" ]; then
echo "check_pg0_writable left its write-test file behind"
exit 1
fi
# Non-writable directory: returns 1 with actionable guidance.
mkdir -p "$TMP_DIR/readonly"
chmod 000 "$TMP_DIR/readonly"
set +e
readonly_output="$(check_pg0_writable "$TMP_DIR/readonly" 2>&1)"
readonly_rc=$?
set -e
chmod 755 "$TMP_DIR/readonly"
if [ "$readonly_rc" -eq 0 ]; then
echo "check_pg0_writable should fail on a non-writable directory"
exit 1
fi
assert_contains "$readonly_output" "not writable"
assert_contains "$readonly_output" "hindsight-data:/home/hindsight/.pg0"
assert_contains "$readonly_output" "--user"
# External database configured: skip the check regardless of dir perms.
mkdir -p "$TMP_DIR/extdb"
chmod 000 "$TMP_DIR/extdb"
set +e
HINDSIGHT_API_DATABASE_URL="postgres://x" check_pg0_writable "$TMP_DIR/extdb" >/dev/null 2>&1
extdb_rc=$?
set -e
chmod 755 "$TMP_DIR/extdb"
if [ "$extdb_rc" -ne 0 ]; then
echo "check_pg0_writable should skip when an external database is configured"
exit 1
fi
echo "start-all pg0 writability checks passed"
else
echo "⚠️ Running as root; skipping pg0 writability checks (permissions are bypassed)."
fi