diff --git a/.forgejo/workflows/build.yml b/.forgejo/workflows/build.yml index 6815889..0ff85aa 100644 --- a/.forgejo/workflows/build.yml +++ b/.forgejo/workflows/build.yml @@ -1523,9 +1523,9 @@ jobs: # entrypoint's default together with the role itself. An adopter # who writes no `command:` gets exactly this. # - # `healthcheck_all` is the right assertion and was itself never - # run: it passes only when hypercorn answers AND every lane in - # the table is answering the broker. A web-only check would go + # The image's own `healthcheck` is the right assertion, and it + # was itself never run: for the `all` role it passes only when + # hypercorn answers AND every lane in the table is answering. A web-only check would go # green with every worker dead, which is the failure mode # consolidation creates. # @@ -1536,7 +1536,7 @@ jobs: CID_ALL=$(docker run -d $ENVOPTS "$CANDIDATE") lanes_up="" for i in $(seq 1 60); do - if docker exec "$CID_ALL" python -m backend.app.scripts.healthcheck_all; then + if docker exec "$CID_ALL" python -m backend.app.scripts.healthcheck; then lanes_up=1 break fi diff --git a/Dockerfile b/Dockerfile index df8d1ae..3cc8e7c 100644 --- a/Dockerfile +++ b/Dockerfile @@ -126,6 +126,22 @@ ENV FC_VERSION=${FC_VERSION} EXPOSE 8080 +# ONE healthcheck for every role, because the image knows which role it is +# running and a deployment should not have to repeat it. `healthcheck` reads +# the role entrypoint.sh recorded and asks the right question: HTTP for web, +# a self-addressed celery ping for a worker lane, both-for-every-lane for the +# consolidated `all`. +# +# start-period covers the SLOWEST role, which is `all`: alembic, then +# hypercorn, then four celery workers registering with the broker. A web-only +# container is ready long before this; the cost of the shared number is that +# a broken one takes a little longer to be called broken. +# +# A service may still declare its own healthcheck and docker will prefer it — +# the escape hatch for a deployment that wants something different. +HEALTHCHECK --interval=30s --timeout=15s --start-period=90s --retries=3 \ + CMD ["python", "-m", "backend.app.scripts.healthcheck"] + ENTRYPOINT ["./entrypoint.sh"] # The DEFAULT is the whole application, not one lane of it. # diff --git a/backend/app/scripts/healthcheck.py b/backend/app/scripts/healthcheck.py new file mode 100644 index 0000000..78b07e2 --- /dev/null +++ b/backend/app/scripts/healthcheck.py @@ -0,0 +1,173 @@ +"""The container's healthcheck. Picks the right check from the role it runs. + +Exit 0 healthy, non-zero unhealthy. + +## Why this is in the IMAGE and not in every compose file + +Because the image is the only thing that knows what it is running. A +deployment had to declare a healthcheck per service, which meant every +compose file, stack file and README repeated the same knowledge: + + web -> curl /api/health + worker -> celery inspect ping -d celery@$HOSTNAME + all -> both, for every lane + +Three checks, written out by hand, once per service, in every file anyone +ever wrote — and none of them wrong until a role changed. Operator, 2026-09-23: +*"why isn't the healthcheck built into the image or base on what command runs +if one is passed in."* There was no reason. The role is already a fact the +container holds; asking the deployment to restate it is the same duplication +the lane table exists to remove one level down. + +So `entrypoint.sh` records the role it started, the Dockerfile declares ONE +`HEALTHCHECK` that runs this, and a stack file says nothing at all. Declaring +one anyway still works — docker lets a service override the image's — which +is the escape hatch for a deployment that genuinely wants something else. + +## What each role is asked + +* **web** — hypercorn answers `/api/health`. No database: the endpoint is a + no-DB 200 that proves the app booted and is serving after `alembic upgrade + head`, which is what a rolling deploy needs to know. +* **worker / scheduler / ml-worker** — THIS container's celery node answers a + ping over the broker. Not "some worker answered": the node name is pinned + to this container, or a healthy sibling would keep a dead one looking alive. +* **all** — both halves, for every lane in the table. The failure mode + consolidation creates is that docker can no longer see the lanes as + separate services, so a web-only check reports a healthy container with + every worker dead. +* **shell / alembic / anything else** — nothing to check. These are one-shot + or interactive; a liveness probe on them has no meaning, so it passes + rather than inventing a verdict. + +## An unrecorded role passes rather than failing + +If the role file is missing, the entrypoint did not run — someone used +`--entrypoint` or ran a bare command. That is a debugging shape, and a +healthcheck that cannot tell what it is looking at must not assert that the +thing is broken (snippet #3969: an unswept read is not a verdict). It says so +on stdout and exits 0. +""" + +from __future__ import annotations + +import os +import socket +import sys +import urllib.error +import urllib.request + +# Written by entrypoint.sh at boot. /tmp because it is the one path writable +# by every role without assuming a volume, and the value is per-container +# state that must NOT survive into a new container. +ROLE_FILE = os.environ.get("FC_ROLE_FILE", "/tmp/fc-role") + +WEB_URL = "http://localhost:8080/api/health" +WEB_TIMEOUT = 5.0 + +# A broker round trip, so it gets a deadline (rule 156). Generous relative to +# `inspect`'s 2s elsewhere: this runs every 30s with retries, and a transient +# blip flagging a worker unhealthy would roll back a deployment that is fine. +PING_TIMEOUT = 10.0 + +CELERY_ROLES = {"worker", "scheduler", "ml-worker"} +# Roles with nothing to probe. Listed rather than treated as the default, so +# an unknown role takes the "I cannot tell" path and says so. +NO_CHECK_ROLES = {"shell", "bash", "alembic"} + + +def current_role() -> str | None: + """The role this container was started with, or None if nothing recorded.""" + env = os.environ.get("FC_ROLE") + if env: + return env.strip() + try: + with open(ROLE_FILE) as fh: + return fh.read().strip() or None + except OSError: + return None + + +def _web_ok() -> tuple[bool, str]: + try: + with urllib.request.urlopen(WEB_URL, timeout=WEB_TIMEOUT) as resp: + if resp.status == 200: + return True, "" + return False, f"web returned {resp.status}" + except (urllib.error.URLError, OSError) as exc: + return False, f"web unreachable: {exc}" + + +def _this_node_ok() -> tuple[bool, str]: + """Ping THIS container's celery node, by name. + + Pinned to this node deliberately. A bare `ping()` is answered by any + worker on the broker, so in a stack with several replicas a dead one + would go on reporting healthy for as long as a sibling was alive — the + healthcheck would be measuring the cluster, not the container it is in. + """ + from ..celery_app import celery as celery_app + + node = f"{os.environ.get('CELERY_NODENAME', 'celery')}@{socket.gethostname()}" + try: + replies = celery_app.control.ping(destination=[node], timeout=PING_TIMEOUT) + except Exception as exc: # noqa: BLE001 — a probe reports, never raises + return False, f"could not reach the broker: {exc}" + if not replies: + return False, f"{node} did not answer a ping" + return True, "" + + +def _lanes_ok() -> tuple[bool, str]: + """Every lane in the table is answering. + + Deliberately ignores whether a lane is ENABLED: a disabled lane still runs + its process with its consumers cancelled, so it answers `inspect` and is + healthy. Health is "is the process alive"; whether it should be consuming + is a settings question the reconcile owns, and conflating them would make + turning a lane off in the UI mark the container unhealthy. + """ + from ..services.worker_control import inspect_lanes_sync + from ..services.worker_lanes import LANES + + live = inspect_lanes_sync() + missing = sorted(lane.name for lane in LANES if not live[lane.name].present) + if missing: + return False, "lanes not answering: " + ", ".join(missing) + return True, "" + + +def main(argv: list[str] | None = None) -> int: + role = current_role() + + if role is None: + # Not a failure. See the module docstring: the entrypoint did not run, + # so there is no role to check against and no basis for a verdict. + print("no role recorded; nothing to check") + return 0 + + if role in NO_CHECK_ROLES: + print(f"{role}: nothing to check") + return 0 + + checks = [] + if role == "all": + checks = [_web_ok, _lanes_ok] + elif role == "web": + checks = [_web_ok] + elif role in CELERY_ROLES: + checks = [_this_node_ok] + else: + print(f"unknown role {role!r}; nothing to check") + return 0 + + for check in checks: + ok, detail = check() + if not ok: + print(detail, file=sys.stderr) + return 1 + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/backend/app/scripts/healthcheck_all.py b/backend/app/scripts/healthcheck_all.py deleted file mode 100644 index 8d565e5..0000000 --- a/backend/app/scripts/healthcheck_all.py +++ /dev/null @@ -1,81 +0,0 @@ -"""Container healthcheck for the single-container layout. - -Milestone 422 step 5. Exit 0 healthy, non-zero unhealthy. - -## Why this is not just "does :8080 answer" - -In the multi-service stack every service has its OWN healthcheck, so a dead -worker turns that service unhealthy while web stays green — docker knows which -part failed. Collapsing them into one container collapses that too: a web-only -check would report a perfectly healthy container while every lane inside it -had crashed and been abandoned by supervisord after its retries. - -So this asserts both halves: hypercorn answers, AND every lane this container -was configured to run is answering the broker. - -## What it deliberately does NOT do - -It does not read the database, and it does not consult the `enabled` flag. A -DISABLED lane still has a running process with its consumers cancelled (see -the config generator), so it answers `inspect` and is healthy. Health is -"is the process alive", and whether it should be consuming is a settings -question the reconcile owns — conflating them would make turning a lane off -in the UI mark the container unhealthy. - -It also cannot distinguish "the broker is down" from "every lane is down", -and reports unhealthy either way. That is correct: a container that cannot -reach its broker is not serving, whichever half is at fault. -""" - -from __future__ import annotations - -import sys -import urllib.error -import urllib.request - -WEB_URL = "http://localhost:8080/api/health" -WEB_TIMEOUT = 5.0 - - -def _web_ok() -> tuple[bool, str]: - try: - with urllib.request.urlopen(WEB_URL, timeout=WEB_TIMEOUT) as resp: - if resp.status == 200: - return True, "" - return False, f"web returned {resp.status}" - except (urllib.error.URLError, OSError) as exc: - return False, f"web unreachable: {exc}" - - -def _lanes_ok() -> tuple[bool, str]: - from ..services.worker_control import inspect_lanes_sync - from ..services.worker_lanes import LANES - - # Every lane, ml included: one image carries them all since step 6, and a - # disabled lane still runs a process (consumers cancelled), so it answers - # inspect and is healthy. Health is "is the process alive"; whether it - # should be consuming is the reconcile's business. - expected = {lane.name for lane in LANES} - live = inspect_lanes_sync() - missing = sorted(n for n in expected if not live[n].present) - if missing: - return False, "lanes not answering: " + ", ".join(missing) - return True, "" - - -def main(argv: list[str] | None = None) -> int: - ok, detail = _web_ok() - if not ok: - print(detail, file=sys.stderr) - return 1 - - ok, detail = _lanes_ok() - if not ok: - print(detail, file=sys.stderr) - return 1 - - return 0 - - -if __name__ == "__main__": - raise SystemExit(main()) diff --git a/docker-compose.single.yml b/docker-compose.single.yml index 9d34e32..d112ae7 100644 --- a/docker-compose.single.yml +++ b/docker-compose.single.yml @@ -76,17 +76,12 @@ services: # mid-backup — which is recoverable (the work is chunked and idempotent) # but wastes however long it had run. stop_grace_period: 200s - # BOTH halves: hypercorn answers AND every configured lane is answering - # the broker. A web-only check would report a healthy container while - # every lane inside it had crashed — the failure mode consolidation - # creates, since docker can no longer see the lanes as separate services. - healthcheck: - test: ["CMD", "python", "-m", "backend.app.scripts.healthcheck_all"] - interval: 30s - timeout: 15s - retries: 3 - # Covers alembic + hypercorn boot + four celery workers registering. - start_period: 90s + # No healthcheck here either. The image declares one that reads the role + # it is running, and for this one that means BOTH halves: hypercorn + # answers AND every lane is answering the broker. A web-only check would + # report a healthy container while every lane inside it had crashed — + # the failure mode consolidation creates, since docker can no longer see + # the lanes as separate services. environment: DB_USER: ${DB_USER:-curator} DB_PASSWORD: ${DB_PASSWORD:-postgres} diff --git a/entrypoint.sh b/entrypoint.sh index 4e679c0..b7c6fa8 100755 --- a/entrypoint.sh +++ b/entrypoint.sh @@ -31,6 +31,26 @@ ROLE="${1:-all}" # service in the multi-service stack — it falls back to `celery`, exactly # celery's own default, so `celery@$HOSTNAME` healthchecks there still work. : "${CELERY_NODENAME:=celery}" + +# RECORD THE ROLE, for the image's own HEALTHCHECK to read. +# +# The container is the only thing that knows what it was asked to run, and +# before this every compose and stack file had to restate it as a healthcheck +# of its own. The Dockerfile now declares one check that dispatches on this. +# +# Written ONCE, by the outermost invocation. The `all` role starts the other +# roles through this same script under supervisord, and those children must +# not overwrite the container's role with their own — a lane starting would +# turn the composite check into a web-only one, silently. FC_ROLE is exported, +# so a child sees it set and skips. +# +# Best effort: a read-only /tmp is not a reason to refuse to boot. The +# healthcheck treats a missing file as "nothing to check" rather than as a +# failure, for the same reason. +if [ -z "${FC_ROLE:-}" ]; then + export FC_ROLE="$ROLE" + printf '%s\n' "$ROLE" > "${FC_ROLE_FILE:-/tmp/fc-role}" 2>/dev/null || true +fi shift || true case "$ROLE" in diff --git a/tests/test_healthcheck.py b/tests/test_healthcheck.py new file mode 100644 index 0000000..b10d8e7 --- /dev/null +++ b/tests/test_healthcheck.py @@ -0,0 +1,134 @@ +"""The image's healthcheck picks its check from the role it is running. + +What this protects is the DISPATCH, not the probes. Each probe is a couple of +lines around a urllib call or a celery ping; the thing that can quietly go +wrong is a container being asked the wrong question — a consolidated one +checked as if it were web-only would report healthy with every worker dead, +which is precisely the failure mode consolidation creates. +""" + +from __future__ import annotations + +import pytest + +from backend.app.scripts import healthcheck as hc + + +@pytest.fixture +def calls(monkeypatch): + """Record which probes ran, without running any of them.""" + seen = [] + + def _stub(name, ok=True): + def run(): + seen.append(name) + return (ok, "" if ok else f"{name} said no") + return run + + monkeypatch.setattr(hc, "_web_ok", _stub("web")) + monkeypatch.setattr(hc, "_lanes_ok", _stub("lanes")) + monkeypatch.setattr(hc, "_this_node_ok", _stub("node")) + return seen + + +def _role(monkeypatch, value): + monkeypatch.setattr(hc, "current_role", lambda: value) + + +def test_the_consolidated_role_checks_both_halves(monkeypatch, calls): + """`all` is the reason this file exists. Web alone would pass on a + container whose every worker had died, and docker can no longer see those + workers as separate services to notice for us.""" + _role(monkeypatch, "all") + + assert hc.main() == 0 + assert calls == ["web", "lanes"] + + +def test_web_is_not_asked_about_lanes(monkeypatch, calls): + """A web container runs no lanes, so asking would fail it for the absence + of something it was never meant to have.""" + _role(monkeypatch, "web") + + assert hc.main() == 0 + assert calls == ["web"] + + +@pytest.mark.parametrize("role", ["worker", "scheduler", "ml-worker"]) +def test_a_celery_role_pings_itself_and_nothing_else(monkeypatch, calls, role): + """No HTTP — these serve no port, and a web probe would fail every one of + them.""" + _role(monkeypatch, role) + + assert hc.main() == 0 + assert calls == ["node"] + + +@pytest.mark.parametrize("role", ["shell", "bash", "alembic"]) +def test_one_shot_roles_have_nothing_to_check(monkeypatch, calls, role): + _role(monkeypatch, role) + + assert hc.main() == 0 + assert calls == [] + + +def test_an_unrecorded_role_passes_rather_than_inventing_a_verdict( + monkeypatch, calls, +): + """The entrypoint did not run — someone used `--entrypoint` or a bare + command. A check that cannot tell what it is looking at must not assert + that the thing is broken (snippet #3969).""" + _role(monkeypatch, None) + + assert hc.main() == 0 + assert calls == [] + + +def test_an_unknown_role_passes_too(monkeypatch, calls): + """A role added to entrypoint.sh and not here is a gap in this file, not + evidence the container is unhealthy.""" + _role(monkeypatch, "something-new") + + assert hc.main() == 0 + assert calls == [] + + +def test_a_failing_probe_fails_the_check(monkeypatch): + """The guard has to be able to fail (rule 167) — every test above asserts + a 0, so one of them has to prove 0 is not all it can return.""" + monkeypatch.setattr(hc, "current_role", lambda: "web") + monkeypatch.setattr(hc, "_web_ok", lambda: (False, "web unreachable")) + + assert hc.main() == 1 + + +def test_the_lane_half_can_fail_the_consolidated_role(monkeypatch): + """The half that only the consolidated role has, and the half that + actually broke on run 7319 — web answered while three of four lanes read + as absent.""" + monkeypatch.setattr(hc, "current_role", lambda: "all") + monkeypatch.setattr(hc, "_web_ok", lambda: (True, "")) + monkeypatch.setattr( + hc, "_lanes_ok", lambda: (False, "lanes not answering: ml, worker"), + ) + + assert hc.main() == 1 + + +def test_the_role_is_read_from_the_file_the_entrypoint_writes(monkeypatch, tmp_path): + """The contract between entrypoint.sh and this module. It is a file rather + than an env var because the HEALTHCHECK exec inherits the container's + environment, not whatever the entrypoint set at runtime.""" + path = tmp_path / "fc-role" + path.write_text("all\n") + monkeypatch.delenv("FC_ROLE", raising=False) + monkeypatch.setattr(hc, "ROLE_FILE", str(path)) + + assert hc.current_role() == "all" + + +def test_a_missing_role_file_reads_as_unknown_not_as_a_crash(monkeypatch, tmp_path): + monkeypatch.delenv("FC_ROLE", raising=False) + monkeypatch.setattr(hc, "ROLE_FILE", str(tmp_path / "nope")) + + assert hc.current_role() is None