mirror of
https://github.com/calibrain/shelfmark.git
synced 2026-11-15 08:48:26 +00:00
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.
This commit is contained in:
1 parent
973cf472a2
commit
ef40a38546
5 files changed
+102
-2
No files matched your search
@@ -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:
|
||||
|
||||
@@ -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
|
||||
|
||||
+9
-1
@@ -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)
|
||||
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
Reference in new issue
Block a user