From ef40a38546cb7932f002267f04d5a2be5b5e1fee Mon Sep 17 00:00:00 2001 From: splitsec2 <35583321+splitsec2@users.noreply.github.com> Date: Sat, 24 Oct 2026 17:12:58 -0600 Subject: [PATCH] fix: report a stopped download coordinator in /api/health (#1568) If the download coordinator thread dies, every task sits at "Queued" and nothing says why. The rest of the app keeps answering, `/api/health` stays green and the container looks fine. #823 and #1166 were both that, and the loop's `except Exception` cannot catch everything that can end a thread. `/api/health` now reports it. The response gets a `coordinator` field, `running` or `stopped`, and when it is stopped the status is `error` and the HTTP code is 503. The Docker healthcheck in the Dockerfile and the one in the readme both use `curl -f`, so the container shows as unhealthy and anything watching health (Uptime Kuma, autoheal) can act on it. It needs no login, same as before, and shows no queue contents. That 503 is the one decision here. Keeping the 200 and only adding a `degraded` entry would not trip a status-code health check, which is the whole point, so I went with 503. Easy to flip if you would rather not. The orchestrator gets a small `coordinator_running()` that asks whether the thread is alive. `start()` already knows how to restart a dead thread, so restarting it automatically would be the true fix, but I would rather the failure be visible first and decide on that separately. Tests: three for the route (ok, 503, no login needed) and three for `coordinator_running` (never started, alive, dead). They fail without the change. One existing test hits `/api/health` through an app that never starts the coordinator, so it now patches that. The readme's health section has a sentence about the 503. Full suite passes, ruff and basedpyright are clean. --- readme.md | 2 + shelfmark/download/orchestrator.py | 10 ++++ shelfmark/main.py | 10 +++- tests/core/test_api_key_env.py | 4 +- tests/core/test_health_endpoint.py | 78 ++++++++++++++++++++++++++++++ 5 files changed, 102 insertions(+), 2 deletions(-) create mode 100644 tests/core/test_health_endpoint.py diff --git a/readme.md b/readme.md index cea2f7eb..a9f63abe 100644 --- a/readme.md +++ b/readme.md @@ -288,6 +288,8 @@ healthcheck: retries: 3 ``` +The endpoint answers 503 when the download coordinator, the part that picks queued downloads up, is not running. Without it every download stays queued, so a container that reports unhealthy for this reason needs a restart. + ## Logging Logs are available via: diff --git a/shelfmark/download/orchestrator.py b/shelfmark/download/orchestrator.py index c0964f89..5ed2bc97 100644 --- a/shelfmark/download/orchestrator.py +++ b/shelfmark/download/orchestrator.py @@ -1344,6 +1344,16 @@ _coordinator_thread: threading.Thread | None = None _coordinator_lock = Lock() +def coordinator_running() -> bool: + """Whether the download coordinator thread is alive. + + The coordinator is the only thing that picks queued downloads up. If it dies, every task + stays "Queued" while the rest of the app looks healthy, so /api/health reports this. + """ + with _coordinator_lock: + return _coordinator_thread is not None and _coordinator_thread.is_alive() + + def start() -> None: """Start the download coordinator thread. Safe to call multiple times.""" global _coordinator_thread diff --git a/shelfmark/main.py b/shelfmark/main.py index 2bb39f8f..7b3757e9 100644 --- a/shelfmark/main.py +++ b/shelfmark/main.py @@ -1506,7 +1506,9 @@ def api_health() -> Response | tuple[Response, int]: No authentication required. Returns: - flask.Response: JSON with status "ok" and optional degraded features. + flask.Response: JSON with status "ok" and optional degraded features. Answers 503 with + status "error" when the download coordinator is not running, since nothing is + downloaded without it. """ response: dict[str, object] = {"status": "ok"} @@ -1515,6 +1517,12 @@ def api_health() -> Response | tuple[Response, int]: if not backend.WEBSOCKET_AVAILABLE: response["degraded"] = {"websocket": "WebSocket unavailable - real-time updates disabled"} + coordinator_up = backend.coordinator_running() + response["coordinator"] = "running" if coordinator_up else "stopped" + if not coordinator_up: + response["status"] = "error" + return jsonify(response), 503 + return jsonify(response) diff --git a/tests/core/test_api_key_env.py b/tests/core/test_api_key_env.py index 031da2a7..cf19cbf3 100644 --- a/tests/core/test_api_key_env.py +++ b/tests/core/test_api_key_env.py @@ -307,7 +307,9 @@ class TestMismatchFallsThrough: class TestScopeAndModes: def test_health_and_auth_paths_and_root_ignore_key(self, wired): client = wired.app.test_client() - assert client.get("/api/health", headers=_bearer("s3cret")).status_code == 200 + # The test app never starts the coordinator, which /api/health reports. + with patch.object(wired.backend, "coordinator_running", return_value=True): + assert client.get("/api/health", headers=_bearer("s3cret")).status_code == 200 assert ( client.get("/api/auth/check", headers=_bearer("s3cret")).get_json()["authenticated"] is False diff --git a/tests/core/test_health_endpoint.py b/tests/core/test_health_endpoint.py new file mode 100644 index 00000000..83f0cb92 --- /dev/null +++ b/tests/core/test_health_endpoint.py @@ -0,0 +1,78 @@ +"""/api/health must say so when the download coordinator is not running.""" + +from __future__ import annotations + +import importlib +import threading +from unittest.mock import patch + +import pytest + +from shelfmark.download import orchestrator + + +@pytest.fixture(scope="module") +def main_module(): + """Import `shelfmark.main` with background startup disabled.""" + with patch("shelfmark.download.orchestrator.start"): + import shelfmark.main as main + + importlib.reload(main) + return main + + +@pytest.fixture +def client(main_module): + return main_module.app.test_client() + + +def test_health_is_ok_while_the_coordinator_runs(main_module, client): + with patch.object(main_module.backend, "coordinator_running", return_value=True): + resp = client.get("/api/health") + + assert resp.status_code == 200 + assert resp.get_json()["status"] == "ok" + assert resp.get_json()["coordinator"] == "running" + + +def test_health_fails_when_the_coordinator_is_gone(main_module, client): + with patch.object(main_module.backend, "coordinator_running", return_value=False): + resp = client.get("/api/health") + + assert resp.status_code == 503 + assert resp.get_json()["status"] == "error" + assert resp.get_json()["coordinator"] == "stopped" + + +def test_health_needs_no_login(main_module, client): + with ( + patch.object(main_module, "get_auth_mode", return_value="builtin"), + patch.object(main_module.backend, "coordinator_running", return_value=True), + ): + resp = client.get("/api/health") + + assert resp.status_code == 200 + + +class TestCoordinatorRunning: + def test_false_before_it_was_ever_started(self, monkeypatch): + monkeypatch.setattr(orchestrator, "_coordinator_thread", None) + assert orchestrator.coordinator_running() is False + + def test_true_while_the_thread_is_alive(self, monkeypatch): + release = threading.Event() + thread = threading.Thread(target=release.wait, daemon=True) + thread.start() + monkeypatch.setattr(orchestrator, "_coordinator_thread", thread) + try: + assert orchestrator.coordinator_running() is True + finally: + release.set() + thread.join(timeout=2) + + def test_false_once_the_thread_has_died(self, monkeypatch): + thread = threading.Thread(target=lambda: None, daemon=True) + thread.start() + thread.join(timeout=2) + monkeypatch.setattr(orchestrator, "_coordinator_thread", thread) + assert orchestrator.coordinator_running() is False