From 40a3ce21d9513d6a854647f59c47379396f96810 Mon Sep 17 00:00:00 2001 From: Emi Griffith Date: Thu, 23 Jul 2026 14:59:47 -0700 Subject: [PATCH 1/9] shell: add shellcheck CI guard and drive the tree to zero findings No static analysis has ever run over the ~2k lines of shell that deploy, provision secrets, and bootstrap hosts as root over SSH. Add shell-lint.yml (pinned shellcheck v0.11.0 + sha256, -x, default severity, fail on any finding) and fix everything it reports, plus two defects it structurally cannot see. Not path-filtered, matching secrets-guard's call: a backstop that only runs when you expect it to isn't a backstop. Scripts are discovered with find, so new ones are covered on landing. The version is pinned to a static release rather than apt's, so a drifted shellcheck can't fail CI on an unrelated push. render-secrets.sh: the mktemp holding DECRYPTED vault contents was only removed on the success path and one failure branch, so a sops decrypt failure left plaintext POSTGRES_PASSWORD in /tmp indefinitely on a live host. A RETURN trap makes removal unconditional, and the two sops calls now `|| return 1` explicitly instead of relying on the caller's set -e (a bare set -e abort skips the trap). The function stays free of `set -e` itself -- it is sourced, and shell options would leak into the caller. autoscale.sh: ran `set -eu` without pipefail while piping docker stats into awk, so a failed left side was swallowed and the loop scaled on empty input. Promoted to pipefail with avg_cpu's failure treated as a missed sample, so a daemon hiccup can't kill the autoscaler. Verified busybox ash in docker:27-cli supports pipefail and the script still parses there. capture-fixtures.sh: `jq . || cat` ran cat after jq had already consumed stdin, silently writing a truncated fixture; now a real if/else that fails loudly. deploy.sh/deploy-stack.sh: `# shellcheck source=` paths corrected for the monorepo layout, and /etc/thermograph.env marked unfollowable (it is rendered at deploy time and cannot exist at lint time). --- .forgejo/workflows/shell-lint.yml | 75 ++++++++++++++++++++++++++ backend/scripts/smoke.sh | 2 +- frontend/scripts/capture-fixtures.sh | 4 +- infra/deploy/deploy.sh | 14 +++-- infra/deploy/render-secrets.sh | 16 +++--- infra/deploy/secrets/dry-run-render.sh | 4 ++ infra/deploy/stack/autoscale.sh | 11 +++- infra/deploy/stack/deploy-stack.sh | 10 +++- 8 files changed, 120 insertions(+), 16 deletions(-) create mode 100644 .forgejo/workflows/shell-lint.yml diff --git a/.forgejo/workflows/shell-lint.yml b/.forgejo/workflows/shell-lint.yml new file mode 100644 index 0000000..6aa69eb --- /dev/null +++ b/.forgejo/workflows/shell-lint.yml @@ -0,0 +1,75 @@ +name: shell-lint + +# Shellcheck over every *.sh in the tree. These scripts run as root over SSH +# against production hosts (deploy, secrets provisioning, host bootstrap) with +# no test suite in front of them -- a quoting bug or an unset-variable typo is +# found *on the host* or not at all. The tree was driven to zero findings in +# one pass; this guard keeps it there. +# +# Deliberately NOT path-filtered (same call as secrets-guard): a backstop that +# only runs when you expect it to isn't a backstop, and shellcheck over the +# ~26 scripts here is seconds. Scripts are discovered with `find`, not listed, +# so a new script is covered the moment it lands -- that is the entire point. +# +# Shellcheck is installed from the pinned official static-binary release, NOT +# `apt-get install shellcheck` (the observability-validate precedent): the +# distro version drifts with the job image, and a drifted shellcheck can grow +# NEW warnings that fail CI on an unrelated push. The pin (v0.11.0, matching +# the koalaman/shellcheck:stable image the zero-findings pass was run against) +# plus the sha256 makes the check reproducible; bump both together, and expect +# to fix any new findings in the same PR as the bump. + +on: + pull_request: + push: + branches: [dev, main, release] + +env: + SHELLCHECK_VERSION: v0.11.0 + SHELLCHECK_SHA256: 8c3be12b05d5c177a04c29e3c78ce89ac86f1595681cab149b65b97c4e227198 + +jobs: + shellcheck: + runs-on: docker + steps: + - uses: actions/checkout@v4 + + - name: Install shellcheck ${{ env.SHELLCHECK_VERSION }} (pinned static binary) + run: | + set -euo pipefail + # node:20-bookworm (this runner's job image) ships curl, tar and xz, + # so the pinned tarball needs no apt-get at all -- faster than the + # distro install it replaces. + curl -fsSL -o /tmp/shellcheck.tar.xz \ + "https://github.com/koalaman/shellcheck/releases/download/${SHELLCHECK_VERSION}/shellcheck-${SHELLCHECK_VERSION}.linux.x86_64.tar.xz" + echo "${SHELLCHECK_SHA256} /tmp/shellcheck.tar.xz" | sha256sum -c - + tar -xJf /tmp/shellcheck.tar.xz -C /tmp + install -m 0755 "/tmp/shellcheck-${SHELLCHECK_VERSION}/shellcheck" /usr/local/bin/shellcheck + shellcheck --version + + - name: Shellcheck every *.sh in the tree + run: | + set -euo pipefail + mapfile -t files < <(find . -name '*.sh' -not -path './.git/*' | sort) + if [ ${#files[@]} -eq 0 ]; then + echo "no *.sh files yet — nothing to lint" + exit 0 + fi + echo "shellcheck over ${#files[@]} scripts" + # -x follows sourced files: several scripts carry + # `# shellcheck source=` directives that only resolve with it. + # Default severity, no excludes: the tree is at zero findings, so + # anything shellcheck reports is a regression, not noise. + if shellcheck -x -f gcc "${files[@]}" > /tmp/findings.txt; then + echo "ok: all scripts clean" + exit 0 + fi + # gcc format is file:line:col: level: message — reshape each line + # into a ::error annotation so findings land on the diff in the PR + # view. `rest` soaks up any further colons inside the message. + while IFS=: read -r f l _ rest; do + [ -n "$l" ] || continue + echo "::error file=${f#./},line=${l}::${rest# }" + done < /tmp/findings.txt + echo "shellcheck found problems in the files above" + exit 1 diff --git a/backend/scripts/smoke.sh b/backend/scripts/smoke.sh index 4854c3b..7165756 100755 --- a/backend/scripts/smoke.sh +++ b/backend/scripts/smoke.sh @@ -29,7 +29,7 @@ echo "==> starting backend + db ($IMG)" "${COMPOSE[@]}" up -d echo "==> waiting for /healthz (up to 90s)" -for i in $(seq 1 45); do +for _ in $(seq 1 45); do if curl -fsS -o /dev/null "$PORT_URL/healthz"; then ok=1; break; fi sleep 2 done diff --git a/frontend/scripts/capture-fixtures.sh b/frontend/scripts/capture-fixtures.sh index de7578b..7641883 100755 --- a/frontend/scripts/capture-fixtures.sh +++ b/frontend/scripts/capture-fixtures.sh @@ -18,7 +18,7 @@ trap 'scripts/backend-for-tests.sh down >/dev/null 2>&1 || true' EXIT fetch() { # echo "==> $2.json <- $1" - curl -fsS "$BASE/$1" --max-time 40 | { command -v jq >/dev/null && jq . || cat; } > "$OUT/$2.json" + curl -fsS "$BASE/$1" --max-time 40 | { if command -v jq >/dev/null; then jq .; else cat; fi; } > "$OUT/$2.json" } fetch "api/v2/content/home" home @@ -28,4 +28,4 @@ fetch "api/v2/content/city/$SLUG" city fetch "api/v2/content/city/$SLUG/month/$MONTH" month fetch "api/v2/content/city/$SLUG/records" records -echo "==> captured $(ls "$OUT"/*.json | wc -l) fixtures for $SLUG into $OUT/" +echo "==> captured $(find "$OUT" -maxdepth 1 -name '*.json' | wc -l) fixtures for $SLUG into $OUT/" diff --git a/infra/deploy/deploy.sh b/infra/deploy/deploy.sh index b4a56ce..4b8f8f3 100755 --- a/infra/deploy/deploy.sh +++ b/infra/deploy/deploy.sh @@ -70,11 +70,16 @@ fi # existing /etc/thermograph.env. Then source it so a by-hand run interpolates the # same as the systemd unit does. See deploy/render-secrets.sh + deploy/secrets/. if [ -f "$INFRA_DIR/deploy/render-secrets.sh" ]; then - # shellcheck source=deploy/render-secrets.sh + # shellcheck source=infra/deploy/render-secrets.sh . "$INFRA_DIR/deploy/render-secrets.sh" render_thermograph_secrets "$INFRA_DIR" fi -set -a; . /etc/thermograph.env 2>/dev/null || true; set +a +# /etc/thermograph.env is rendered at deploy time from the SOPS vault — it +# cannot exist at lint time, so don't ask shellcheck to follow it. +set -a +# shellcheck source=/dev/null +. /etc/thermograph.env 2>/dev/null || true +set +a # Pre-warm the ~750 city-page archives so /climate pages serve from cache and a # search-engine crawl never bursts the archive API quota. Detached inside the @@ -156,7 +161,10 @@ TAGS_FILE="$INFRA_DIR/deploy/.image-tags.env" _incoming_backend="${BACKEND_IMAGE_TAG:-}" _incoming_frontend="${FRONTEND_IMAGE_TAG:-}" if [ -f "$TAGS_FILE" ]; then - set -a; . "$TAGS_FILE"; set +a + set -a + # shellcheck source=/dev/null # untracked runtime artifact this script writes below + . "$TAGS_FILE" + set +a fi [ -n "$_incoming_backend" ] && BACKEND_IMAGE_TAG="$_incoming_backend" [ -n "$_incoming_frontend" ] && FRONTEND_IMAGE_TAG="$_incoming_frontend" diff --git a/infra/deploy/render-secrets.sh b/infra/deploy/render-secrets.sh index b44a965..d9bc7ee 100755 --- a/infra/deploy/render-secrets.sh +++ b/infra/deploy/render-secrets.sh @@ -48,15 +48,21 @@ render_thermograph_secrets() { key_env=("SOPS_AGE_KEY=$keymat") fi local tmp; tmp=$(mktemp) + # The tmp file holds DECRYPTED secrets: a RETURN trap makes its removal + # unconditional (fires on every explicit return, success or failure, and is + # scoped to this function — it doesn't leak into or clobber the caller's traps). + trap 'rm -f "$tmp"' RETURN : > "$tmp" - # set -e in the caller makes a decrypt failure fatal here (no partial env). common - # first, host second, so a host value overrides a shared one (last-wins). + # Decrypt failures return 1 explicitly (no partial env, no reliance on the + # caller's shell options — a bare set -e abort would skip the RETURN trap and + # strand plaintext in /tmp). common first, host second, so a host value + # overrides a shared one (last-wins). if [ -f "$repo/deploy/secrets/common.yaml" ]; then env "${key_env[@]}" sops -d --input-type yaml --output-type dotenv \ - "$repo/deploy/secrets/common.yaml" >> "$tmp" + "$repo/deploy/secrets/common.yaml" >> "$tmp" || return 1 fi env "${key_env[@]}" sops -d --input-type yaml --output-type dotenv \ - "$repo/deploy/secrets/${env_name}.yaml" >> "$tmp" + "$repo/deploy/secrets/${env_name}.yaml" >> "$tmp" || return 1 # Write /etc/thermograph.env. Prefer an in-place write when the existing file is # writable by us (e.g. a group-writable 0660 root: on a box whose CI @@ -72,9 +78,7 @@ render_thermograph_secrets() { elif install -m 0640 "$tmp" /etc/thermograph.env 2>/dev/null; then : elif sudo install -m 0640 -o "$(id -un)" -g "$(id -gn)" "$tmp" /etc/thermograph.env 2>/dev/null; then : else - rm -f "$tmp" echo "!! cannot write /etc/thermograph.env (need file write access or passwordless sudo)" >&2 return 1 fi - rm -f "$tmp" } diff --git a/infra/deploy/secrets/dry-run-render.sh b/infra/deploy/secrets/dry-run-render.sh index 03533f1..44b41a1 100755 --- a/infra/deploy/secrets/dry-run-render.sh +++ b/infra/deploy/secrets/dry-run-render.sh @@ -19,6 +19,10 @@ tmp="$(mktemp)"; live="$(mktemp)"; trap 'rm -f "$tmp" "$live"' EXIT [ -f "$common" ] && SOPS_AGE_KEY="$keymat" sops -d --input-type yaml --output-type dotenv "$common" >> "$tmp" SOPS_AGE_KEY="$keymat" sops -d --input-type yaml --output-type dotenv "$host" >> "$tmp" +# sudo is only for the READ of the root-owned live env; the redirect target is our +# own mktemp file, so the redirect correctly runs as the invoking user (sudo tee +# would wrongly write it as root). +# shellcheck disable=SC2024 sudo cat /etc/thermograph.env > "$live" python3 - "$live" "$tmp" <<'PY' import sys diff --git a/infra/deploy/stack/autoscale.sh b/infra/deploy/stack/autoscale.sh index 2a36ade..b737bab 100755 --- a/infra/deploy/stack/autoscale.sh +++ b/infra/deploy/stack/autoscale.sh @@ -18,7 +18,11 @@ # - Clamped to [MIN_REPLICAS, MAX_REPLICAS]; scaling waits for convergence # (--detach=false), so a stuck rollout blocks further changes rather than # stacking them. -set -eu +# pipefail: without it a `docker stats` failure on the left of the avg_cpu +# pipe is silently swallowed by awk's exit 0 and the loop trusts the output. +# (Runs under busybox ash in docker:*-cli, which supports pipefail.) +# shellcheck disable=SC3040 # pipefail is not POSIX, but the busybox ash this runs under has it +set -euo pipefail STACK_NAME="${STACK_NAME:-thermograph}" SERVICE="${STACK_NAME}_web" @@ -64,7 +68,10 @@ while :; do cur=$(replicas) [ -n "$cur" ] || { log "service $SERVICE not found; waiting"; continue; } - cpu=$(avg_cpu) + # With pipefail, a transient `docker stats` failure makes avg_cpu return + # non-zero; treat that as a missed sample (don't trust partial output, and + # don't let set -e kill the autoscaler over a daemon hiccup). + cpu=$(avg_cpu) || cpu="" [ -n "$cpu" ] || continue # no running tasks visible this sample if [ "$cpu" -gt "$UP_AT" ]; then diff --git a/infra/deploy/stack/deploy-stack.sh b/infra/deploy/stack/deploy-stack.sh index 8833aec..8516a6b 100755 --- a/infra/deploy/stack/deploy-stack.sh +++ b/infra/deploy/stack/deploy-stack.sh @@ -49,11 +49,16 @@ export STACK_NAME # install the uid-10001-readable copy the tasks' env-entrypoint shim sources. # 10001 = the app images' `thermograph` user; the file is 0400 to that uid. if [ -f "$APP_DIR/infra/deploy/render-secrets.sh" ]; then - # shellcheck source=deploy/render-secrets.sh + # shellcheck source=infra/deploy/render-secrets.sh . "$APP_DIR/infra/deploy/render-secrets.sh" render_thermograph_secrets "$APP_DIR/infra" fi -set -a; . /etc/thermograph.env 2>/dev/null || true; set +a +# /etc/thermograph.env is rendered at deploy time from the SOPS vault — it +# cannot exist at lint time, so don't ask shellcheck to follow it. +set -a +# shellcheck source=/dev/null +. /etc/thermograph.env 2>/dev/null || true +set +a sudo install -o 10001 -g 0 -m 0400 /etc/thermograph.env /etc/thermograph/stack.env \ || install -o 10001 -g 0 -m 0400 /etc/thermograph.env /etc/thermograph/stack.env @@ -65,6 +70,7 @@ export REGISTRY_HOST TAGS_FILE="$APP_DIR/infra/deploy/.stack-image-tags.env" _incoming_backend="${BACKEND_IMAGE_TAG:-}" _incoming_frontend="${FRONTEND_IMAGE_TAG:-}" +# shellcheck source=/dev/null # untracked runtime artifact this script writes below if [ -f "$TAGS_FILE" ]; then set -a; . "$TAGS_FILE"; set +a; fi [ -n "$_incoming_backend" ] && BACKEND_IMAGE_TAG="$_incoming_backend" [ -n "$_incoming_frontend" ] && FRONTEND_IMAGE_TAG="$_incoming_frontend" -- 2.45.2 From 56be0dd9b7d9b7d08169e2717ad146884713ee9b Mon Sep 17 00:00:00 2001 From: Emi Griffith Date: Thu, 23 Jul 2026 15:03:17 -0700 Subject: [PATCH 2/9] render-secrets: drop the RETURN trap, it broke deploy silently The RETURN trap added in the previous commit leaked out of the function and killed every deploy on a SOPS-configured host. A RETURN trap set inside a SOURCED function is not function-scoped: it persists in the caller's shell after the function returns, and a RETURN trap also fires when a `.`/source completes. deploy.sh sources /etc/thermograph.env six lines after calling render_thermograph_secrets, which re-fired the trap at top level where `tmp` -- function-local -- is unset. Under deploy.sh's `set -u` that is fatal, and silent: that line already sends stderr to /dev/null, so the deploy rendered secrets and then died with no diagnostic before pulling or rolling anything. Replaced with explicit `rm -f "$tmp"` on each exit path, plus a comment recording why the tidier-looking trap is wrong here so it doesn't come back. The original defect the trap was meant to fix stays fixed: the decrypt-failure path removes the plaintext temp file before returning 1. The write section now captures its status in `rc` and cleans up once, rather than ending on `rm` -- as the last command it was masking a failed in-place `cat` write to status 0, so a half-written /etc/thermograph.env would have deployed as if it succeeded. Verified in a container against the real call pattern (strict-mode caller, source lib, call, then source the rendered env): success path returns 0 and the caller survives the subsequent source; decrypt-failure path aborts the caller with no /etc/thermograph.env written; both leave zero temp files. --- infra/deploy/render-secrets.sh | 38 ++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/infra/deploy/render-secrets.sh b/infra/deploy/render-secrets.sh index d9bc7ee..d4a434e 100755 --- a/infra/deploy/render-secrets.sh +++ b/infra/deploy/render-secrets.sh @@ -48,21 +48,26 @@ render_thermograph_secrets() { key_env=("SOPS_AGE_KEY=$keymat") fi local tmp; tmp=$(mktemp) - # The tmp file holds DECRYPTED secrets: a RETURN trap makes its removal - # unconditional (fires on every explicit return, success or failure, and is - # scoped to this function — it doesn't leak into or clobber the caller's traps). - trap 'rm -f "$tmp"' RETURN - : > "$tmp" - # Decrypt failures return 1 explicitly (no partial env, no reliance on the - # caller's shell options — a bare set -e abort would skip the RETURN trap and - # strand plaintext in /tmp). common first, host second, so a host value - # overrides a shared one (last-wins). + # The tmp file holds DECRYPTED secrets, so every exit path below removes it. + # + # Deliberately NOT a `trap ... RETURN`, which looks like the tidier way to make + # that unconditional: a RETURN trap set inside a SOURCED function persists in + # the CALLER's shell after the function returns, and a RETURN trap also fires + # when a `.`/source completes. deploy.sh sources /etc/thermograph.env a few + # lines after calling us, which would re-fire the trap at top level where `tmp` + # (function-local) is unset -- fatal under deploy.sh's `set -u`, and silent, + # because that line already redirects stderr to /dev/null. Explicit removal on + # each path is duller and correct. + : > "$tmp" || { rm -f "$tmp"; return 1; } + # Decrypt failures return 1 explicitly, so a partial env is never written and + # correctness doesn't depend on the caller's shell options. common first, host + # second, so a host value overrides a shared one (last-wins). if [ -f "$repo/deploy/secrets/common.yaml" ]; then env "${key_env[@]}" sops -d --input-type yaml --output-type dotenv \ - "$repo/deploy/secrets/common.yaml" >> "$tmp" || return 1 + "$repo/deploy/secrets/common.yaml" >> "$tmp" || { rm -f "$tmp"; return 1; } fi env "${key_env[@]}" sops -d --input-type yaml --output-type dotenv \ - "$repo/deploy/secrets/${env_name}.yaml" >> "$tmp" || return 1 + "$repo/deploy/secrets/${env_name}.yaml" >> "$tmp" || { rm -f "$tmp"; return 1; } # Write /etc/thermograph.env. Prefer an in-place write when the existing file is # writable by us (e.g. a group-writable 0660 root: on a box whose CI @@ -73,12 +78,19 @@ render_thermograph_secrets() { # next line of deploy.sh (`. /etc/thermograph.env` as that non-root user) can't read # it, so POSTGRES_PASSWORD never enters the env and `docker compose` dies on # interpolation. Fail loudly rather than deploy against stale secrets. + # rc + a single cleanup point: the plaintext tmp must go whichever branch runs, + # but the old trailing `rm` was also the function's last command, so it masked a + # failed in-place `cat` write to status 0. Capture the status instead, so a + # half-written /etc/thermograph.env fails loudly rather than deploying stale. + local rc=0 if [ -f /etc/thermograph.env ] && [ -w /etc/thermograph.env ]; then - cat "$tmp" > /etc/thermograph.env + cat "$tmp" > /etc/thermograph.env || rc=1 elif install -m 0640 "$tmp" /etc/thermograph.env 2>/dev/null; then : elif sudo install -m 0640 -o "$(id -un)" -g "$(id -gn)" "$tmp" /etc/thermograph.env 2>/dev/null; then : else echo "!! cannot write /etc/thermograph.env (need file write access or passwordless sudo)" >&2 - return 1 + rc=1 fi + rm -f "$tmp" + return "$rc" } -- 2.45.2 From 4ff113f2cf3dd47f0087d7667ba54df80f2ae9d9 Mon Sep 17 00:00:00 2001 From: emi Date: Fri, 24 Jul 2026 19:28:47 +0000 Subject: [PATCH 3/9] Stop logging /healthz + internal SSR hop, truncate logged IPs (#35) --- backend/core/metrics.py | 13 ++++ backend/deploy/entrypoint.sh | 5 +- backend/tests/core/test_metrics.py | 10 +++ backend/tests/web/test_request_logging.py | 87 +++++++++++++++++++++++ backend/web/app.py | 36 ++++++++-- 5 files changed, 145 insertions(+), 6 deletions(-) create mode 100644 backend/tests/web/test_request_logging.py diff --git a/backend/core/metrics.py b/backend/core/metrics.py index 36b085b..7652bb5 100644 --- a/backend/core/metrics.py +++ b/backend/core/metrics.py @@ -96,6 +96,12 @@ def classify_inbound(path: str, base: str = "") -> str: the app's mount prefix (e.g. ``/thermograph`` or ``""``). """ p = path or "/" + # The liveness probe (Dockerfile HEALTHCHECK, Caddy health_uri) is by far the + # highest-volume single path in prod (measured ~47% of the access log) and is + # never real traffic — same posture as the metrics check below. Never under + # BASE (see /healthz's own docstring in web/app.py), so check before stripping. + if p.rstrip("/") == "/healthz": + return "health" # The dashboard polls the metrics endpoint; never count it as traffic, whatever base # prefix it arrives under — e.g. a `/thermograph/api/v2/metrics` probe against a # root-served prod app would otherwise land in "other" and show as an inbound error. @@ -118,6 +124,13 @@ def classify_inbound(path: str, base: str = "") -> str: # category keeps record_inbound from double-counting every interaction. if seg == "event": return "event" + # The SSR content API (backend/api/content_routes.py) is called only by + # the frontend_ssr service's own api_client.py, never by a browser — a + # server-to-server hop, not a page view. Its own category is what lets + # that (measured ~32% of the access log on prod) be excluded from the + # access log below without also hiding real external traffic. + if seg == "content": + return "internal" return f"api:{seg}" if seg else "api:other" if p.endswith((".js", ".css", ".html", ".webmanifest", ".png", ".svg", ".ico", ".json", ".woff", ".woff2", ".map", ".txt", ".xml")): diff --git a/backend/deploy/entrypoint.sh b/backend/deploy/entrypoint.sh index 9b6f399..9baf4b3 100755 --- a/backend/deploy/entrypoint.sh +++ b/backend/deploy/entrypoint.sh @@ -85,4 +85,7 @@ if [ "${RUN_MIGRATIONS:-1}" != "0" ]; then fi echo "==> Starting uvicorn on 0.0.0.0:${PORT:-8137} with ${WORKERS:-4} worker(s)" -exec uvicorn app:app --host 0.0.0.0 --port "${PORT:-8137}" --workers "${WORKERS:-4}" +# --no-access-log: the app's own request-logging middleware (audit.log_access, +# web/app.py) already writes a structured line per request; uvicorn's own access +# log just duplicated every one of them for no benefit. +exec uvicorn app:app --host 0.0.0.0 --port "${PORT:-8137}" --workers "${WORKERS:-4}" --no-access-log diff --git a/backend/tests/core/test_metrics.py b/backend/tests/core/test_metrics.py index 010f246..d58ad47 100644 --- a/backend/tests/core/test_metrics.py +++ b/backend/tests/core/test_metrics.py @@ -40,6 +40,16 @@ def test_classify_inbound_categories(): # dashboard probing /thermograph/... against a root-served (base="") prod app. assert c("/thermograph/api/v2/metrics", "") == "metrics" assert c("/api/v2/metrics", "") == "metrics" + # The SSR content API (backend/api/content_routes.py) is a server-to-server + # hop from frontend_ssr's own api_client.py, never a browser -- its own + # category is what lets it be excluded from the access log without also + # hiding real external traffic under the generic "api:*" bucket. + assert c("/thermograph/api/v2/content/hub", "/thermograph") == "internal" + assert c("/api/v2/content/city/tokyo", "") == "internal" + # The liveness probe: never under BASE, whatever base the app happens to be + # mounted at (it's registered at a fixed path -- see web/app.py's healthz). + assert c("/healthz") == "health" + assert c("/healthz", "/thermograph") == "health" # pages, static, seo assert c("/thermograph/", "/thermograph") == "page" assert c("/thermograph/calendar", "/thermograph") == "page" diff --git a/backend/tests/web/test_request_logging.py b/backend/tests/web/test_request_logging.py new file mode 100644 index 0000000..b14882b --- /dev/null +++ b/backend/tests/web/test_request_logging.py @@ -0,0 +1,87 @@ +"""The request-logging middleware (web/app.py's revalidate_static): what gets +counted, what gets written to the access log, and what gets truncated before +it's persisted. Companion to core/test_metrics.py's classify_inbound tests -- +these exercise the actual ASGI request path, not just the classifier function. +""" +import pytest +from fastapi.testclient import TestClient + +from web import app as appmod + +metrics = appmod.metrics # the exact module object app.py's own calls use -- +audit = appmod.audit # see the note on module-reload isolation below. + + +@pytest.fixture +def client(): + return TestClient(appmod.app) + + +def _patch_log_access(monkeypatch): + calls = [] + monkeypatch.setattr(audit, "log_access", lambda record: calls.append(record)) + return calls + + +def test_healthz_is_never_audited(monkeypatch, client): + """/healthz is ~47% of prod's daily access log; it must never even reach + audit.log_access (not just be cheap once there -- see classify_inbound's + "health" category and the exclusion in revalidate_static).""" + calls = _patch_log_access(monkeypatch) + r = client.get("/healthz") + assert r.status_code == 200 + assert calls == [] + + +def test_internal_category_is_never_audited(monkeypatch, client): + """The SSR content API's own category ("internal") is excluded from the + access log the same way -- it's a server-to-server hop (frontend_ssr's + api_client.py calling backend), not a page view.""" + calls = _patch_log_access(monkeypatch) + monkeypatch.setattr(metrics, "classify_inbound", lambda path, base: "internal") + r = client.get("/thermograph/api/version") + assert r.status_code == 200 + assert calls == [] + + +def test_access_log_ip_is_truncated_not_raw(monkeypatch, client): + calls = _patch_log_access(monkeypatch) + r = client.get("/thermograph/api/version", + headers={"X-Forwarded-For": "203.0.113.77, 10.0.0.1"}) + assert r.status_code == 200 + assert len(calls) == 1 + assert calls[0]["ip"] == "203.0.113.0" # /24, not the exact address + assert calls[0]["ip"] != "203.0.113.77" + + +def test_loggable_ip_truncation(): + assert appmod._loggable_ip("203.0.113.77") == "203.0.113.0" + assert appmod._loggable_ip("2001:db8:1234:5678::1") == "2001:db8:1234::" + assert appmod._loggable_ip(None) is None + assert appmod._loggable_ip("") == "" + assert appmod._loggable_ip("not-an-ip") == "not-an-ip" # best-effort, never raises + + +def test_rate_limiter_still_sees_the_full_precision_ip(monkeypatch, client): + """_client_ip (which feeds both the access log and the event rate limiter) + is never itself truncated -- only what audit.log_access persists is. A + /24-truncated rate-limit key would let one abuser exhaust a whole NAT'd + office's quota, which is exactly what must NOT happen here.""" + seen = {} + monkeypatch.setattr(metrics, "record_event", lambda name, **kw: seen.update(kw)) + r = client.post("/thermograph/api/v2/event", json={"event": "home.locate"}, + headers={"X-Forwarded-For": "203.0.113.77"}) + assert r.status_code == 204 + assert seen["ip"] == "203.0.113.77" + + +def test_event_route_records_end_to_end(client): + """Full HTTP round trip through the real ASGI route -- confirms the write + path backend/api/event -> metrics.record_event -> the counters store works + end to end (the existing suite only ever called record_event directly).""" + before = metrics.snapshot()["events_total"] + r = client.post("/thermograph/api/v2/event", json={"event": "home.locate"}) + assert r.status_code == 204 + after = metrics.snapshot() + assert after["events_total"] == before + 1 + assert after["events"]["home.locate"]["direct"] # no Referer sent -> "direct" diff --git a/backend/web/app.py b/backend/web/app.py index 0253319..b8a5d35 100644 --- a/backend/web/app.py +++ b/backend/web/app.py @@ -3,6 +3,7 @@ import contextlib import datetime import hashlib import hmac +import ipaddress import json import os import queue @@ -263,13 +264,34 @@ def api_version(): def _client_ip(request) -> "str | None": """The real client IP: the left-most X-Forwarded-For hop when a proxy fronts us - (Caddy on prod sets it), otherwise the direct peer address (LAN dev).""" + (Caddy on prod sets it), otherwise the direct peer address (LAN dev). Full + precision — the rate limiter (metrics._rate_ok) needs the exact address, since + a truncated key would let one abuser exhaust a whole NAT'd office's quota. + Truncate at the point of persistence instead (see _loggable_ip).""" xff = request.headers.get("x-forwarded-for") if xff: return xff.split(",")[0].strip() return request.client.host if request.client else None +def _loggable_ip(ip: "str | None") -> "str | None": + """Coarsen a client IP before it's written to the access log: /24 for IPv4, + /48 for IPv6. Keeps rough geo/abuse signal without keeping a full, joinable + address sitting in structured logs for the log's 30-day retention -- the + public privacy page promises IPs aren't logged beyond normal request handling, + and a raw address shipped to Loki was a live gap against that. Never the + input to the rate limiter (_client_ip's callers pass the untruncated value + there) -- this is only what gets persisted.""" + if not ip: + return ip + try: + addr = ipaddress.ip_address(ip) + except ValueError: + return ip # not a parseable address (e.g. a test/placeholder value) -- pass through + prefix = 24 if addr.version == 4 else 48 + return str(ipaddress.ip_network(f"{addr}/{prefix}", strict=False).network_address) + + @app.middleware("http") async def revalidate_static(request, call_next): """Serve the frontend with no-cache (NOT no-store): browsers may keep a copy @@ -288,11 +310,15 @@ async def revalidate_static(request, call_next): # loop so one request's instrumentation can't stall every other request # this worker is serving concurrently. await run_in_threadpool(metrics.record_inbound, cat, response.status_code) - # Retain per-request client IPs for later analysis; skip static assets and the - # dashboard's own metrics polling to keep the log to real, meaningful traffic. - if cat not in ("static", "metrics", "event"): + # Retain per-request client IPs for later analysis; skip static assets, the + # dashboard's own metrics polling, the liveness probe (health -- by far the + # highest-volume single path, and never real traffic), and the internal + # SSR->API hop (internal -- frontend_ssr's own server-to-server calls, not + # a page view) to keep the log to real, meaningful traffic. The IP itself is + # truncated (see _loggable_ip) -- coarse geo/abuse signal, not a full address. + if cat not in ("static", "metrics", "event", "health", "internal"): await run_in_threadpool(audit.log_access, { - "ip": _client_ip(request), "method": request.method, + "ip": _loggable_ip(_client_ip(request)), "method": request.method, "path": path, "status": response.status_code, "cat": cat}) # Threshold-gated slow-request line: RunAudit only times the 7 graded # endpoints, so this is the only latency signal for auth/account/content -- 2.45.2 From 864d38d772eaf96f6d930b2f482d29d32244f491 Mon Sep 17 00:00:00 2001 From: emi Date: Fri, 24 Jul 2026 19:28:58 +0000 Subject: [PATCH 4/9] ops/dbq.sh: read-only Postgres queries across all environments (#18) --- infra/ops/ICEBERG-HANDOFF.md | 133 +++++++++++++++++++++++++++++++++++ infra/ops/README.md | 65 +++++++++++++++++ infra/ops/dbq.sh | 68 ++++++++++++++++++ 3 files changed, 266 insertions(+) create mode 100644 infra/ops/ICEBERG-HANDOFF.md create mode 100644 infra/ops/README.md create mode 100755 infra/ops/dbq.sh diff --git a/infra/ops/ICEBERG-HANDOFF.md b/infra/ops/ICEBERG-HANDOFF.md new file mode 100644 index 0000000..d27810b --- /dev/null +++ b/infra/ops/ICEBERG-HANDOFF.md @@ -0,0 +1,133 @@ +# Iceberg: implementing the agent-queryable service + +Handoff spec for whoever builds the Iceberg layer. The Postgres equivalent is +already shipped (`infra/ops/dbq.sh`) — **mirror it**. This document is the +contract, the environment facts you'll trip over, and how the result gets +verified. + +## Definition of done + +`infra/ops/iceberg.sh` exists and behaves like `dbq.sh`: + +```sh +infra/ops/iceberg.sh prod -c "select count(*) from ." +infra/ops/iceberg.sh dev -c "show tables" +echo "select 1" | infra/ops/iceberg.sh beta -f - +``` + +An agent (or operator) can query Iceberg in any environment, read-only, with no +new network exposure and no interactive steps. + +## The contract (non-negotiable) + +1. **Same interface shape as `dbq.sh`** — `iceberg.sh [flags] + ""`. Extra flags pass through to the engine; **stdin is forwarded** so + `-f -` works. Non-interactive, deterministic, safe to call from CI. +2. **Read-only enforced by the system, not by convention.** A dedicated + read-only identity — not an admin/superuser credential. You must be able to + *demonstrate* a refused write (see Acceptance). +3. **No new network endpoints.** Run the engine where the data is already + reachable (containerised), rather than publishing ports or opening firewall + rules. See the prod constraint below — this is not negotiable, it's physics. +4. **Env-keyed dispatch, names resolved at call time.** Prod is Docker Swarm; + task container names change on **every redeploy**. Resolve via + `docker ps --filter name=…`; never hardcode a container name. +5. **No secrets in the repo** or in workflow files. + +## Environment facts you need + +- Monorepo `emi/thermograph` (domain dirs: `backend/ frontend/ infra/ + observability/`). Ops tooling lives in `infra/ops/`. +- Environments: **dev** = LAN compose stack on the dev machine; **beta** = + `75.119.132.91`; **prod** = `169.58.46.181`. SSH as `agent` with + `~/.ssh/thermograph_agent_ed25519` (passwordless sudo on both boxes). +- **Prod runs Docker Swarm.** Its app database sits on the overlay network + `thermograph_internal` (`10.0.2.0/24`), which **the prod host itself cannot + route to**. Consequence, learned the hard way: `ssh -L` tunnelling works for + beta but is *impossible* for prod. Any design that depends on a tunnel or a + published port will fail on prod — that's why `dbq.sh` execs into the + container instead. Assume the same for anything you deploy there. +- The overlay is `attachable=true`, and the **dev machine is a Swarm worker** in + the prod cluster (NodeAddr `10.10.0.3`) — so `docker run --network + thermograph_internal …` from the dev box is a plausible route to prod-side + services. Untested; verify before relying on it. +- WireGuard mesh: prod `10.10.0.1`, beta `10.10.0.2`, dev `10.10.0.3`. +- `rclone` and `age` are already installed on prod and beta. +- Reference implementation to copy: **`infra/ops/dbq.sh`** + `infra/ops/README.md`. + +## Storage — almost certainly your warehouse + +Contabo Object Storage (S3-compatible), already provisioned and in use: + +- Endpoint `https://eu2.contabostorage.com` · bucket `era5-thermograph` · + region `default` · ~2 TB · **private** (keep it that way). +- ⚠️ **Contabo requires PATH-STYLE addressing.** Set + `force_path_style=true` / `s3.path-style-access=true` (whatever your engine's + FileIO calls it) and `region=default`. Virtual-host-style addressing **fails**. + This is validated, not theoretical. +- ⚠️ **The `backups/` prefix is in use** by the nightly backup jobs (prod DB + + Forgejo). Put Iceberg data under its own prefix (e.g. `iceberg/` or + `warehouse/`). Do not write outside your prefix. +- Credentials already exist — **reuse them, don't mint new ones silently**: + - SOPS vault: `infra/deploy/secrets/{prod,beta}.yaml` as `THERMOGRAPH_S3_*` + (rendered to `/etc/thermograph.env` at deploy by + `infra/deploy/render-secrets.sh`). + - Forgejo Actions secrets: `S3_ENDPOINT`, `S3_BUCKET`, `S3_ACCESS_KEY`, + `S3_SECRET_KEY`. + - If read-only S3 keys are needed for the query identity, ask the operator to + mint a scoped keypair rather than reusing the read-write one. + +## Secrets handling + +- Host-side: SOPS + age. Recipient `age1xx4dzs0dxlwvkv9sjuqzsphl7lfrxannkfken374yu2qvvcte9sqzktqt2`; + the private key is on each host at `/etc/thermograph/age.key` and on the + operator's machine at `~/.config/sops/age/keys.txt`. +- CI-side: Forgejo Actions secrets. +- ⚠️ **beta gotcha:** `/etc/thermograph.env` on beta is `agent:agent 0640` — the + `deploy` user **cannot read it**. If your service or job runs as `deploy` on + beta, pull credentials from Actions secrets instead, or change the ownership + deliberately and say so. + +## Decisions you must make — and report back + +1. **Catalog**: REST (Lakekeeper / Nessie / Polaris), JDBC-on-Postgres, Hive, or + filesystem/hadoop — and where it runs per environment. +2. **Engine**: DuckDB + iceberg extension, Trino, Spark, or pyiceberg. Prefer the + lightest that satisfies the contract; it must run containerised and headless. +3. **Warehouse location**: bucket + prefix, and whether environments are isolated + by separate prefixes, namespaces, or buckets. +4. **Read-only mechanism**: scoped S3 keys? catalog RBAC? engine restricted mode? + State which, and how a write is refused. +5. **Where the engine runs** for each env (local container on dev, on the box for + beta/prod, or attached to the overlay) — consistent with constraint #3. + +## Acceptance criteria + +The work is accepted when all of these are demonstrated with pasted output: + +- [ ] `infra/ops/iceberg.sh -c "
/metadata/`. `iceberg.sh` resolves that at call time +(numeric metadata version, newest wins) and exposes each table directory as a +view of the same name — which is also how it keeps reading fresh snapshots of +a table that is still being loaded. + +Credentials are resolved at call time and never stored in the repo: the +target host's `/etc/thermograph.env` (`THERMOGRAPH_LAKE_S3_*`) wins if +present; otherwise the calling machine decrypts the SOPS vault +(`infra/deploy/secrets/prod.yaml`) and hands the two values to the remote +shell over stdin — never argv, so never visible in `ps`, and never written to +a file on the target. + +### Safety + +Every session locks itself down *before* user SQL runs: + +```sql +SET allowed_directories=['s3://era5-thermograph/iceberg/', 's3://era5-thermograph/era5/']; +SET enable_external_access=false; +SET lock_configuration=true; +``` + +Both prefixes are required for reads: `era5_daily` was built with pyiceberg +`add_files` over the existing hive parquet, so its metadata lives under +`iceberg/` while its data files stay in place under `era5/daily/`. DuckDB +itself then refuses everything else — the live `backups/` prefix, all local +files — and the lockdown cannot be SET away by query text: + +``` +$ infra/ops/iceberg.sh prod -c "COPY (SELECT 1) TO 's3://era5-thermograph/backups/x.csv'" +Permission Error: Cannot access file "s3://era5-thermograph/backups/x.csv" - file system operations are disabled by configuration +$ infra/ops/iceberg.sh dev -c "SET enable_external_access=true" +Invalid Input Error: Cannot change configuration option "enable_external_access" - the configuration has been locked +``` + +**Known gap:** the only provisioned keypair for the bucket is read-write, so +a raw `COPY TO` targeting a path *inside* the two allowed prefixes is not +refused — the DuckDB iceberg extension cannot write Iceberg tables, but it +could still clobber raw objects under `iceberg/` or `era5/`. Path-based +restriction cannot both allow reading a prefix and forbid writing it; closing +the gap needs a scoped read-only keypair (the object-storage equivalent of +`thermograph_ro`). When the operator mints one, render it as +`THERMOGRAPH_LAKE_S3_*` on the hosts or swap it into the vault — +`iceberg.sh` needs no code change. diff --git a/infra/ops/iceberg.sh b/infra/ops/iceberg.sh new file mode 100755 index 0000000..693e54d --- /dev/null +++ b/infra/ops/iceberg.sh @@ -0,0 +1,200 @@ +#!/usr/bin/env bash +# iceberg -- run READ-ONLY SQL against the Thermograph Iceberg lake (LAN dev / beta / prod). +# +# infra/ops/iceberg.sh dev -c "show tables" +# infra/ops/iceberg.sh prod -c "select count(*) from era5_daily" +# infra/ops/iceberg.sh beta -json -c "select * from era5_daily limit 3" +# echo "select 1" | infra/ops/iceberg.sh prod -f - +# +# Why an ephemeral container instead of a query service: the lake is one shared +# warehouse in Contabo object storage (s3://era5-thermograph/iceberg), readable +# from every environment, so the environments differ only in WHERE the engine +# runs -- a throwaway `docker run` of a small DuckDB image on the target box. +# No daemon, no listening port, no tunnel (prod's overlay cannot be tunnelled), +# and no container names to resolve, so prod redeploys cannot break it. +# +# There is no catalog service: a table's current state is its newest metadata +# JSON under /
/metadata/, resolved at call time and exposed +# as a view named after the table directory. +# +# Safety: before user SQL runs, the session pins external access to the lake +# prefixes (iceberg/ metadata + era5/ data files) and locks the configuration +# -- anything else, notably backups/ and all local files, is refused by +# DuckDB itself, and the lockdown cannot be SET away. The provisioned S3 +# keypair is read-write, so a write *inside* those prefixes is still possible +# until a scoped read-only keypair exists (see infra/ops/README.md). +# +# Credentials are resolved at call time, never stored here: the target host's +# /etc/thermograph.env (THERMOGRAPH_LAKE_S3_*) wins if present; otherwise the +# calling machine decrypts the SOPS vault and feeds the two values to the +# remote shell over stdin (never argv, so never visible in `ps`). +# +# Any extra arguments are passed straight through to the duckdb CLI, so -c, +# -json, -csv, -line, -f etc. all work. With `-f -`, stdin is forwarded and +# read as piped SQL. A single bare argument is treated as `-c` SQL. +set -euo pipefail + +KEYFILE="${THERMOGRAPH_AGENT_KEY:-$HOME/.ssh/thermograph_agent_ed25519}" + +usage() { + sed -n '2,8p' "$0" | sed 's/^# \{0,1\}//' >&2 + echo "environments: dev | beta | prod" >&2 + exit 2 +} + +env_name="${1:-}" +[ -n "$env_name" ] || usage +shift + +case "$env_name" in + dev) ssh_target= ;; + beta) ssh_target=agent@75.119.132.91 ;; + prod) ssh_target=agent@169.58.46.181 ;; + -h|--help) usage ;; + *) echo "iceberg: unknown environment '$env_name' (want: dev|beta|prod)" >&2; exit 2 ;; +esac + +[ $# -gt 0 ] || usage + +# The engine image: duckdb CLI with the httpfs + iceberg extensions installed +# at build time, so a query needs no downloads. Built on the target host the +# first time it's needed; the tag is a hash of this Dockerfile, so editing it +# here rolls every host forward automatically on the next call. +dockerfile=$(cat <<'DOCKERFILE' +FROM debian:bookworm-slim +RUN apt-get update \ + && apt-get install -y --no-install-recommends ca-certificates curl \ + && rm -rf /var/lib/apt/lists/* +RUN curl -fsSL https://github.com/duckdb/duckdb/releases/download/v1.5.5/duckdb_cli-linux-amd64.gz \ + | gunzip > /usr/local/bin/duckdb \ + && chmod +x /usr/local/bin/duckdb +RUN duckdb -c "INSTALL httpfs; INSTALL iceberg;" +COPY <<'ENTRY' /usr/local/bin/iceberg-query +#!/bin/sh +# Entrypoint of the thermograph-iceberg image; run by iceberg.sh, not directly. +set -eu +: "${LAKE_S3_ACCESS_KEY:?}" "${LAKE_S3_SECRET_KEY:?}" +WAREHOUSE="${LAKE_WAREHOUSE:-s3://era5-thermograph/iceberg}" +ENDPOINT="${LAKE_S3_ENDPOINT:-eu2.contabostorage.com}" +# Prefixes a query may touch. era5/ is needed because era5_daily was built +# with add_files over the existing hive parquet: its metadata lives in the +# warehouse but its data files stay in place under era5/daily/. +ALLOWED="${LAKE_ALLOWED_DIRS:-$WAREHOUSE/,s3://era5-thermograph/era5/}" + +esc() { printf %s "$1" | sed "s/'/''/g"; } +SECRET="CREATE SECRET lake (TYPE s3, KEY_ID '$(esc "$LAKE_S3_ACCESS_KEY")', SECRET '$(esc "$LAKE_S3_SECRET_KEY")', ENDPOINT '$ENDPOINT', REGION 'default', URL_STYLE 'path');" + +# One view per table dir, over its newest metadata JSON (numeric version wins, +# so v10 beats v9 and 00010-... beats 00009-...). grep drops statement noise +# like CREATE SECRET's "true" row; an empty warehouse yields no views. +VIEWS=$(duckdb -batch -noheader -list \ + -cmd "LOAD httpfs; $SECRET" \ + -c "SELECT 'CREATE VIEW \"' || tbl || '\" AS SELECT * FROM iceberg_scan(''' || file || ''');' + FROM (SELECT file, + split_part(replace(file, '$WAREHOUSE/', ''), '/', 1) AS tbl, + row_number() OVER ( + PARTITION BY split_part(replace(file, '$WAREHOUSE/', ''), '/', 1) + ORDER BY coalesce(try_cast(regexp_extract(parse_filename(file), '[0-9]+') AS BIGINT), -1) DESC, + file DESC) AS rn + FROM glob('$WAREHOUSE/*/metadata/*.metadata.json')) + WHERE rn = 1;" | grep '^CREATE VIEW ' || true) + +# Lock the session down BEFORE user SQL: external access is pinned to the +# allowed prefixes, local files are unreachable, and the config can't be +# un-SET. Views bind lazily, so reads still work under the lockdown. +allowed_sql= +oifs=$IFS; IFS=, +for d in $ALLOWED; do allowed_sql="$allowed_sql${allowed_sql:+, }'$d'"; done +IFS=$oifs + +INIT="LOAD httpfs; LOAD iceberg; $SECRET $VIEWS +SET allowed_directories=[$allowed_sql]; +SET enable_external_access=false; +SET lock_configuration=true;" + +# A single bare argument is the SQL itself (dbq.sh calling convention). +if [ $# -eq 1 ] && [ "${1#-}" = "$1" ]; then set -- -c "$1"; fi + +# duckdb has no "-f - means stdin" convention; hand it the real thing. +i=0; n=$#; prev= +while [ "$i" -lt "$n" ]; do + a=$1; shift + if [ "$prev" = "-f" ] && [ "$a" = "-" ]; then a=/dev/stdin; fi + prev=$a + set -- "$@" "$a" + i=$((i+1)) +done + +# The .output pair silences the init's own result rows (CREATE SECRET prints +# a "true"); errors still reach stderr, and user SQL output is untouched. +exec duckdb -batch -cmd ".output /dev/null" -cmd "$INIT" -cmd ".output" "$@" +ENTRY +RUN chmod +x /usr/local/bin/iceberg-query +DOCKERFILE +) + +tag="thermograph-iceberg:$(printf '%s' "$dockerfile" | sha256sum | cut -c1-12)" +b64=$(printf '%s' "$dockerfile" | base64 -w0) + +# Quote the duckdb arguments so they survive the remote shell intact. +args=$(printf '%q ' "$@") + +# Credentials: env override first, then a call-time sops decrypt of the vault +# on the calling machine. Either may come up empty here -- the remote side +# still gets a chance to fill them from its own /etc/thermograph.env. +ak="${THERMOGRAPH_LAKE_S3_ACCESS_KEY:-}" +sk="${THERMOGRAPH_LAKE_S3_SECRET_KEY:-}" +if [ -z "$ak" ] || [ -z "$sk" ]; then + repo_root=$(cd -- "$(dirname -- "$0")/../.." && pwd) + vault="${THERMOGRAPH_LAKE_VAULT:-$repo_root/infra/deploy/secrets/prod.yaml}" + sops_bin=$(command -v sops || echo "$HOME/.local/bin/sops") + if [ -r "$vault" ] && [ -x "$sops_bin" ]; then + ak=$("$sops_bin" -d --extract '["THERMOGRAPH_LAKE_S3_ACCESS_KEY"]' "$vault" 2>/dev/null || true) + sk=$("$sops_bin" -d --extract '["THERMOGRAPH_LAKE_S3_SECRET_KEY"]' "$vault" 2>/dev/null || true) + fi +fi + +# The engine image is (re)built on the target host only when its tag is +# missing, then run with the credentials passed via environment -- docker +# inherits them from the remote shell, which read them from stdin, so they +# never appear on a command line. The rest of stdin flows through to duckdb. +remote=$(cat <&2 + exit 1 +fi +export LAKE_S3_ACCESS_KEY="\$ak" LAKE_S3_SECRET_KEY="\$sk" +docker image inspect $tag >/dev/null 2>&1 \ + || echo $b64 | base64 -d | docker build -q -t $tag - >/dev/null +exec docker run --rm -i -e LAKE_S3_ACCESS_KEY -e LAKE_S3_SECRET_KEY $tag iceberg-query $args +REMOTE +) + +# Stdin is forwarded only for the documented `-f -` form. Anything else gets +# just the credential lines -- unconditionally forwarding an idle stdin would +# leave the local pipeline waiting on input the query never reads. +want_stdin=false +prev= +for a in "$@"; do + if [ "$prev" = "-f" ] && [ "$a" = "-" ]; then want_stdin=true; fi + prev=$a +done + +feed() { + printf '%s\n%s\n' "$ak" "$sk" || true + if $want_stdin; then cat || true; fi +} + +if [ -z "$ssh_target" ]; then + feed | bash -c "$remote" +else + feed | ssh -i "$KEYFILE" -o StrictHostKeyChecking=no -o ConnectTimeout=15 \ + "$ssh_target" "$remote" +fi -- 2.45.2 From cda6e75b389a7a6ddc62c10b12119b89c72ae505 Mon Sep 17 00:00:00 2001 From: emi Date: Fri, 24 Jul 2026 19:45:49 +0000 Subject: [PATCH 9/9] data/climate: fix geocode_nominatim's NameError on every call (P0, live) (#51) --- backend/data/climate.py | 127 +++++++++++++++++------------ backend/tests/data/test_climate.py | 101 +++++++++++++++++++++++ 2 files changed, 178 insertions(+), 50 deletions(-) diff --git a/backend/data/climate.py b/backend/data/climate.py index fcf4ff1..b198c23 100644 --- a/backend/data/climate.py +++ b/backend/data/climate.py @@ -1009,10 +1009,13 @@ _REVGEO_CACHE: dict[str, str | None] = {} _REVGEO_MIN_INTERVAL = 1.1 # seconds between successive Nominatim reverse calls _revgeo_last = 0.0 -_REVGEO_QUEUE: "queue.Queue[tuple[float, float, str, concurrent.futures.Future]]" = queue.Queue() +_REVGEO_QUEUE: "queue.Queue[tuple]" = queue.Queue() _REVGEO_WORKER_LOCK = threading.Lock() _revgeo_worker_started = False _REVGEO_WAIT_TIMEOUT = 10.0 # seconds a caller waits for ITS OWN request before giving up +_GEOCODE_WAIT_TIMEOUT = 35.0 # forward /geocode is a deliberate user search, not a map-pan + # enrichment -- worth a longer wait than reverse geocoding's, + # covering the 30s per-request timeout plus queue wait. def reverse_geocode_cached(lat: float, lon: float) -> tuple[bool, str | None]: @@ -1065,31 +1068,46 @@ def _fetch_revgeo_label(lat: float, lon: float) -> str | None: def _revgeo_worker() -> None: - """Drains _REVGEO_QUEUE one request at a time, forever. The sole caller of - _fetch_revgeo_label, so the ~1/sec pacing below is enforced just by doing the - work serially — no lock needed, since nothing else ever touches Nominatim. - Re-checks the cache before fetching (a request queued behind an identical one - is answered from what the earlier request just cached, no duplicate call), - and always finishes the fetch + persists the result even if the original - caller already gave up waiting (see reverse_geocode's timeout).""" + """Drains _REVGEO_QUEUE one job at a time, forever — both reverse ("revgeo") + and forward ("geocode") jobs, so the ~1/sec pacing below is enforced just by + doing the work serially on this one thread, no lock needed, since nothing + else ever touches Nominatim. Reverse jobs re-check the cache before fetching + (a request queued behind an identical one is answered from what the earlier + request just cached, no duplicate call) and always finish + persist even if + the original caller already gave up waiting (see reverse_geocode's timeout). + Forward jobs have no cache (see geocode_nominatim) but share the same pacer.""" global _revgeo_last while True: - lat, lon, key, fut = _REVGEO_QUEUE.get() + job = _REVGEO_QUEUE.get() + kind = job[0] try: - found, label = reverse_geocode_cached(lat, lon) - if not found: + if kind == "revgeo": + _, lat, lon, key, fut = job + found, label = reverse_geocode_cached(lat, lon) + if not found: + wait = _REVGEO_MIN_INTERVAL - (time.monotonic() - _revgeo_last) + if wait > 0: + time.sleep(wait) + label = _fetch_revgeo_label(lat, lon) + _revgeo_last = time.monotonic() + _REVGEO_CACHE[key] = label + store.put_revgeo(key, label) # survive restarts (a None label retries after its TTL) + if not fut.done(): + fut.set_result(label) + else: # "geocode" + _, name, count, fut = job wait = _REVGEO_MIN_INTERVAL - (time.monotonic() - _revgeo_last) if wait > 0: time.sleep(wait) - label = _fetch_revgeo_label(lat, lon) - _revgeo_last = time.monotonic() - _REVGEO_CACHE[key] = label - store.put_revgeo(key, label) # survive restarts (a None label retries after its TTL) - if not fut.done(): - fut.set_result(label) + try: + results = _fetch_geocode_forward(name, count) + finally: + _revgeo_last = time.monotonic() + if not fut.done(): + fut.set_result(results) except Exception: # noqa: BLE001 - never let a bad request kill the worker if not fut.done(): - fut.set_result(None) + fut.set_result(None if kind == "revgeo" else []) finally: _REVGEO_QUEUE.task_done() @@ -1129,44 +1147,28 @@ def reverse_geocode(lat: float, lon: float) -> str | None: return label _start_revgeo_worker() fut: "concurrent.futures.Future[str | None]" = concurrent.futures.Future() - _REVGEO_QUEUE.put((lat, lon, key, fut)) + _REVGEO_QUEUE.put(("revgeo", lat, lon, key, fut)) try: return fut.result(timeout=_REVGEO_WAIT_TIMEOUT) except concurrent.futures.TimeoutError: return None -def geocode_nominatim(name: str, count: int = 5) -> list[dict]: - """Forward place-name lookup via OpenStreetMap Nominatim (keyless). - - Replaces the former Open-Meteo geocoder. Nominatim covers the long tail the - local GeoNames index can't — neighbourhoods, postcodes, sub-1000-population - villages, and alternate/native-language spellings — so /geocode falls back to - it on a local miss. It carries no population, so results keep Nominatim's own - relevance order; name/admin/country map straight across. - - Shares the reverse geocoder's lock and ~1/sec pacing: both hit the same host, - so serializing them together keeps total Nominatim traffic under the usage - policy. Only the low-volume /geocode miss path reaches here — autocomplete - (/suggest) is served purely from the local index and never calls out. - """ - global _revgeo_last - with _REVGEO_LOCK: - wait = _REVGEO_MIN_INTERVAL - (time.monotonic() - _revgeo_last) - if wait > 0: - time.sleep(wait) - try: - r = _request( - "https://nominatim.openstreetmap.org/search", - {"q": name, "format": "jsonv2", "addressdetails": 1, - "limit": count, "accept-language": "en"}, - 30, - phase="geocode", - headers={"User-Agent": "Thermograph/0.1 (local weather grading app)"}, - ) - rows = r.json() or [] - finally: - _revgeo_last = time.monotonic() +def _fetch_geocode_forward(name: str, count: int) -> list[dict]: + """The actual Nominatim forward-search HTTP call + result shaping. Called + ONLY from _revgeo_worker (never on a caller's thread), mirroring + _fetch_revgeo_label — but unlike that function, a bad response is allowed to + raise here; _revgeo_worker's own except clause turns it into an empty list, + same net effect without a second layer of exception-swallowing.""" + r = _request( + "https://nominatim.openstreetmap.org/search", + {"q": name, "format": "jsonv2", "addressdetails": 1, + "limit": count, "accept-language": "en"}, + 30, + phase="geocode", + headers={"User-Agent": "Thermograph/0.1 (local weather grading app)"}, + ) + rows = r.json() or [] out = [] for g in rows: a = g.get("address", {}) or {} @@ -1187,3 +1189,28 @@ def geocode_nominatim(name: str, count: int = 5) -> list[dict]: "population": None, # Nominatim has no population; order is relevance-based }) return out + + +def geocode_nominatim(name: str, count: int = 5) -> list[dict]: + """Forward place-name lookup via OpenStreetMap Nominatim (keyless). + + Replaces the former Open-Meteo geocoder. Nominatim covers the long tail the + local GeoNames index can't — neighbourhoods, postcodes, sub-1000-population + villages, and alternate/native-language spellings — so /geocode falls back to + it on a local miss. It carries no population, so results keep Nominatim's own + relevance order; name/admin/country map straight across. + + Shares the reverse geocoder's worker thread and ~1/sec pacing: both hit the + same host, and both jobs are drained serially by the one thread in + _revgeo_worker, so there is exactly one writer of _revgeo_last — no lock + needed, same reasoning as reverse_geocode. Only the low-volume /geocode miss + path reaches here — autocomplete (/suggest) is served purely from the local + index and never calls out. + """ + _start_revgeo_worker() + fut: "concurrent.futures.Future[list[dict]]" = concurrent.futures.Future() + _REVGEO_QUEUE.put(("geocode", name, count, fut)) + try: + return fut.result(timeout=_GEOCODE_WAIT_TIMEOUT) or [] + except concurrent.futures.TimeoutError: + return [] diff --git a/backend/tests/data/test_climate.py b/backend/tests/data/test_climate.py index 95e869f..68d1491 100644 --- a/backend/tests/data/test_climate.py +++ b/backend/tests/data/test_climate.py @@ -615,3 +615,104 @@ def test_reverse_geocode_timeout_returns_none_without_blocking_the_caller(monkey elapsed = time_mod.monotonic() - t0 assert label is None assert elapsed < 0.2 # returned near the wait timeout, not after the 0.3s fetch + + +# --- forward geocode: shares the reverse-geocode worker, not a second lock ----- +# Regression coverage for the NameError _REVGEO_LOCK bug (geocode_nominatim +# referenced a lock that was removed when reverse geocoding moved to the +# worker/queue design, so every forward lookup 502'd in production). These +# exercise the real queue/worker plumbing rather than mocking geocode_nominatim +# itself away, so a reintroduced bare `with _REVGEO_LOCK:` or any other +# not-actually-defined-name bug fails loudly here instead of shipping unseen. + +def test_geocode_nominatim_resolves_via_the_worker_thread(monkeypatch): + monkeypatch.setattr( + climate, "_fetch_geocode_forward", + lambda name, count: [{"name": name, "admin1": None, "country": "Testland", + "country_code": "TL", "lat": 1.0, "lon": 2.0, + "population": None}], + ) + results = climate.geocode_nominatim("Nowheresville") + assert results[0]["name"] == "Nowheresville" + assert results[0]["country"] == "Testland" + + +def test_geocode_nominatim_timeout_returns_empty_list(monkeypatch): + """Same shape as reverse_geocode's timeout test: a caller waits only up to + _GEOCODE_WAIT_TIMEOUT, not as long as the fetch itself takes.""" + import time as time_mod + monkeypatch.setattr(climate, "_GEOCODE_WAIT_TIMEOUT", 0.05) + + def slow_fetch(name, count): + time_mod.sleep(0.3) + return [{"name": "Too Slow"}] + + monkeypatch.setattr(climate, "_fetch_geocode_forward", slow_fetch) + t0 = time_mod.monotonic() + results = climate.geocode_nominatim("anywhere") + elapsed = time_mod.monotonic() - t0 + assert results == [] + assert elapsed < 0.2 + + +def test_geocode_nominatim_a_bad_fetch_degrades_to_empty_list_not_a_crash(monkeypatch): + """_fetch_geocode_forward is allowed to raise (matches _fetch_revgeo_label's + contract loosely -- the worker's except clause is the actual safety net); + confirm a raising fetch never reaches the caller as an exception.""" + def boom(name, count): + raise RuntimeError("Nominatim is down") + monkeypatch.setattr(climate, "_fetch_geocode_forward", boom) + assert climate.geocode_nominatim("anywhere") == [] + + +def test_geocode_nominatim_shares_the_reverse_geocode_pacer(monkeypatch): + """Forward and reverse jobs are drained by the SAME worker thread off the + SAME queue, so a forward call advances _revgeo_last exactly like a reverse + one does -- this is what makes a second lock unnecessary.""" + monkeypatch.setattr(climate, "_revgeo_last", 0.0) + monkeypatch.setattr(climate, "_fetch_geocode_forward", lambda name, count: []) + before = climate._revgeo_last + climate.geocode_nominatim("anywhere") + assert climate._revgeo_last > before + + +def test_fetch_geocode_forward_parses_nominatim_response(monkeypatch): + """Executes _fetch_geocode_forward's real body (the function the NameError + bug prevented from ever running) against a stubbed HTTP transport -- not a + monkeypatch of geocode_nominatim itself, unlike the API-layer tests.""" + class _FakeResp: + def json(self): + return [ + {"name": "West Seattle", "lat": "47.57", "lon": "-122.38", + "display_name": "West Seattle, Seattle, King County, Washington, United States", + "address": {"suburb": "West Seattle", "city": "Seattle", + "state": "Washington", "country": "United States", + "country_code": "us"}}, + # No `name`, no recognized address component -- falls back to the + # head of display_name, exercising that branch too. + {"lat": "51.5", "lon": "-0.1", + "display_name": "Some Unnamed Place, Greater London, England", + "address": {"country": "United Kingdom", "country_code": "gb"}}, + ] + + captured = {} + + def fake_request(url, params, timeout, *, phase, headers=None, attempts=climate.MAX_ATTEMPTS): + captured["url"] = url + captured["params"] = params + captured["phase"] = phase + return _FakeResp() + + monkeypatch.setattr(climate, "_request", fake_request) + results = climate._fetch_geocode_forward("west seattle", 5) + + assert captured["url"] == "https://nominatim.openstreetmap.org/search" + assert captured["params"]["q"] == "west seattle" + assert captured["phase"] == "geocode" + + assert results[0] == { + "name": "West Seattle", "admin1": "Washington", "country": "United States", + "country_code": "US", "lat": 47.57, "lon": -122.38, "population": None, + } + assert results[1]["name"] == "Some Unnamed Place" + assert results[1]["country_code"] == "GB" -- 2.45.2