refactor: the image carries its own healthcheck and picks it by role (4295)

Operator, 2026-09-23: *"why isn't the healthcheck built into the image or
base on what command runs if one is passed in. why is it manually declared in
the stack here."*

No good reason. The container is the only thing that knows what it was asked
to run, and every compose file, stack file and README had to restate it:

    web         -> urllib /api/health
    worker      -> celery inspect ping -d celery@$HOSTNAME
    all         -> both, for every lane

Three checks written by hand, once per service, in every file anyone ever
wrote — none of them wrong until a role changed, and all of them silently
wrong after. The same duplication the lane table exists to remove one level
down, and I built it without noticing.

`entrypoint.sh` now records the role it started. The Dockerfile declares ONE
`HEALTHCHECK` that reads it and asks the right question: HTTP for web, a
self-addressed celery ping for a worker lane, both-for-every-lane for `all`,
and nothing for shell/alembic, which are one-shot and have no liveness to
probe. `docker-compose.single.yml` and the consolidated stack declare none.
A service that wants something else can still declare its own; docker prefers
it, so the escape hatch is the default docker behaviour rather than a flag.

Two details that are load-bearing:

  * The role is written ONCE, by the outermost invocation. `all` starts the
    other roles through this same script under supervisord, and a child
    overwriting the container's role would turn the composite check into a
    web-only one — silently, and only on the consolidated path. FC_ROLE is
    exported so a child sees it set and skips.
  * The celery ping is addressed to THIS node, not a bare ping. A bare one is
    answered by any worker on the broker, so in a stack with replicas a dead
    container would report healthy for as long as a sibling lived — the check
    would be measuring the cluster rather than the container it is inside.

`healthcheck_all.py` is deleted; its two probes moved into the dispatcher
rather than being a second copy beside it.

An unrecorded role PASSES. The entrypoint always writes the file, so the only
way to miss it is bypassing the entrypoint — a debugging shape, where a check
that cannot tell what it is looking at must not assert the thing is broken
(snippet #3969). Said on stdout rather than assumed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LVjrnpQjRgHdvq95rASoiR

.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

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

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())

backend/app/scripts/healthcheck_all.py

@@ -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())

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}

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

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