fix(health): make /health/ready a strict readiness probe (#133)

The /health/ready endpoint previously hardcoded a 200 response, which
meant orchestrators using it as a readiness probe never saw the api
report unready even when document processing was broken. /health itself
was tolerant by design (docling-serve cold-start takes minutes; failing
liveness during that window caused restart loops), but that left no
endpoint that returned 503 when docling-serve was permanently down — so
ALBs and k8s pods would keep routing traffic to instances that couldn't
do their job.

Now:
- /health remains tolerant. 200 healthy/degraded when core stores
  (Redis, S3, queue) are up, 503 only when a core store is down. Stays
  appropriate for liveness probes that should tolerate transient
  docling-serve outages.
- /health/ready becomes strict. Returns 200 only when every dependency
  the pipeline needs (Redis, S3, queue, docling-serve) is reachable;
  503 the moment any of them isn't. Suitable for orchestrator
  readiness probes (Kubernetes readinessProbe, ECS ALB target groups).
- Module docstring + both endpoint docstrings explain the deliberate
  split so operators know which one to wire up where.
- The stale "circuit breaker handles it at request level" comment is
  gone; the retry behaviour is real but only absorbs transient
  outages, not permanent ones — which is exactly what /health/ready
  now surfaces.

Integration tests cover all four states for each endpoint. Added the
pytestmark = pytest.mark.integration marker that was missing on the
file (the original tests were being silently deselected by make
test-integration's -m integration filter).

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: bbertucc

src/api/health.py

@@ -1,4 +1,24 @@
-"""Health check endpoints."""
+"""Health check endpoints.
+
+Two endpoints with deliberately different contracts:
+
+* ``GET /health`` is **tolerant**. It returns 200 with status
+  ``healthy``/``degraded`` when the api process is serving traffic and the
+  core stores (Redis, S3) are reachable, even if docling-serve is briefly
+  unavailable. The docling-serve client has retry + a circuit breaker for
+  short outages, and CPU model load at boot takes a few minutes — failing
+  this endpoint during that window would cause restart loops. Returns 503
+  only when a core store is gone.
+
+* ``GET /health/ready`` is **strict**. It returns 503 the moment *any*
+  dependency the request path needs is unhealthy, including docling-serve.
+  Wire orchestrator readiness probes (Kubernetes readinessProbe, ECS ALB
+  target groups) here when you want traffic to stop the instant the
+  pipeline can't process documents end to end.
+
+Pair the two for the conventional k8s split: ``/health`` for liveness,
+``/health/ready`` for readiness.
+"""
 
 from typing import Any
 
@@ -10,64 +30,82 @@
 router = APIRouter(prefix="/health", tags=["Health"])
 
 
-@router.get("")
-async def health_check(
-    storage: StorageService = Depends(get_storage_service),
-    queue: QueueService = Depends(get_queue_service)
+async def _collect_checks(
+    storage: StorageService, queue: QueueService
 ) -> dict[str, Any]:
-    """
-    Health check endpoint for container orchestration.
-
-    Checks Redis, S3, and queue connectivity.
+    """Run every dependency check and return a structured result.
 
-    Args:
-        storage: Storage service (injected)
-        queue: Queue service (injected)
-
-    Returns:
-        Health status with detailed checks
+    Shared by both endpoints so they cannot drift in what they probe.
     """
-
-    # Check docling-serve health
     try:
         from ..services.docling_serve_client import get_docling_client
         docling_client = get_docling_client()
         docling_healthy = await docling_client.check_health()
     except RuntimeError:
+        # Client not yet initialised (boot race)
         docling_healthy = False
 
-    checks = {
+    return {
         "redis": await queue.check_redis_connection(),
         "s3": await storage.check_s3_access(),
         "queue_depth": await queue.check_queue_depth(),
         "docling_serve": docling_healthy,
     }
 
-    # Core checks: Redis, S3, queue must pass
-    # docling_serve is non-fatal — circuit breaker handles it at request level,
-    # and it takes ~2min to load models at boot (would cause ECS restart loops)
+
+@router.get("")
+async def health_check(
+    storage: StorageService = Depends(get_storage_service),
+    queue: QueueService = Depends(get_queue_service)
+) -> dict[str, Any]:
+    """Tolerant health endpoint suitable for dashboards and liveness probes.
+
+    Returns 200 ``healthy`` when every check passes, 200 ``degraded`` when
+    only docling-serve is down (boot warmup or transient outage that retry
+    + circuit-breaker can absorb), and 503 ``unhealthy`` when a core store
+    (Redis, S3, queue) is unreachable.
+
+    Use ``/health/ready`` for an unforgiving readiness probe.
+    """
+    checks = await _collect_checks(storage, queue)
     core_healthy = checks["redis"] and checks["s3"] and checks["queue_depth"] >= 0
     if core_healthy:
         return {
-            "status": "healthy" if docling_healthy else "degraded",
-            "checks": checks
+            "status": "healthy" if checks["docling_serve"] else "degraded",
+            "checks": checks,
         }
-    else:
-        raise HTTPException(
-            status_code=status.HTTP_503_SERVICE_UNAVAILABLE,
-            detail={
-                "status": "unhealthy",
-                "checks": checks
-            }
-        )
+    raise HTTPException(
+        status_code=status.HTTP_503_SERVICE_UNAVAILABLE,
+        detail={"status": "unhealthy", "checks": checks},
+    )
 
 
 @router.get("/ready")
-async def readiness_check() -> dict[str, str]:
-    """
-    Readiness check for Kubernetes/orchestration.
+async def readiness_check(
+    storage: StorageService = Depends(get_storage_service),
+    queue: QueueService = Depends(get_queue_service)
+) -> dict[str, Any]:
+    """Strict readiness probe for orchestrators.
+
+    Returns 200 ``ready`` only when every dependency the pipeline needs is
+    reachable: Redis, S3, queue, and docling-serve. Any failure returns
+    503 so the orchestrator stops routing traffic until end-to-end
+    document processing can succeed again.
 
-    Returns:
-        Ready status
+    Configure Kubernetes ``readinessProbe`` or ECS ALB target groups
+    against this endpoint when you want traffic gated on full pipeline
+    health. Use ``/health`` (tolerant) for liveness alongside.
     """
-    return {"status": "ready"}
+    checks = await _collect_checks(storage, queue)
+    all_ready = (
+        checks["redis"]
+        and checks["s3"]
+        and checks["queue_depth"] >= 0
+        and checks["docling_serve"]
+    )
+    if all_ready:
+        return {"status": "ready", "checks": checks}
+    raise HTTPException(
+        status_code=status.HTTP_503_SERVICE_UNAVAILABLE,
+        detail={"status": "not_ready", "checks": checks},
+    )

tests/integration/test_health.py

@@ -7,30 +7,43 @@
 from src.dependencies import get_queue_service, get_storage_service
 from src.main import app
 
+pytestmark = pytest.mark.integration
 
-@pytest.mark.asyncio
-async def test_health_check_healthy(client):
-    """Test health check when all services are healthy."""
-    # Create mock services
+
+def _override(storage_ok: bool, redis_ok: bool, queue_depth: int):
+    """Helper: install dependency overrides with given outcomes."""
     mock_storage = MagicMock()
-    mock_storage.check_s3_access = AsyncMock(return_value=True)
+    mock_storage.check_s3_access = AsyncMock(return_value=storage_ok)
 
     mock_queue = MagicMock()
-    mock_queue.check_redis_connection = AsyncMock(return_value=True)
-    mock_queue.check_queue_depth = AsyncMock(return_value=5)
+    mock_queue.check_redis_connection = AsyncMock(return_value=redis_ok)
+    mock_queue.check_queue_depth = AsyncMock(return_value=queue_depth)
 
-    # Override dependencies
     app.dependency_overrides[get_storage_service] = lambda: mock_storage
     app.dependency_overrides[get_queue_service] = lambda: mock_queue
 
+
+def _patched_docling(healthy: bool):
+    """Helper: patch the docling client to report given health."""
+    mock_docling = MagicMock()
+    mock_docling.check_health = AsyncMock(return_value=healthy)
+    return patch(
+        "src.services.docling_serve_client.get_docling_client",
+        return_value=mock_docling,
+    )
+
+
+# ---------- /health (tolerant) ----------
+
+
+@pytest.mark.asyncio
+async def test_health_check_healthy(client):
+    """All checks pass → 200 healthy."""
+    _override(storage_ok=True, redis_ok=True, queue_depth=5)
     try:
-        # Mock docling client as healthy
-        mock_docling = MagicMock()
-        mock_docling.check_health = AsyncMock(return_value=True)
-        with patch("src.services.docling_serve_client.get_docling_client", return_value=mock_docling):
+        with _patched_docling(True):
             response = client.get("/health")
 
-        # Assertions
         assert response.status_code == status.HTTP_200_OK
         data = response.json()
         assert data["status"] == "healthy"
@@ -39,31 +52,17 @@ async def test_health_check_healthy(client):
         assert data["checks"]["queue_depth"] == 5
         assert data["checks"]["docling_serve"] is True
     finally:
-        # Cleanup overrides
         app.dependency_overrides.clear()
 
 
 @pytest.mark.asyncio
 async def test_health_check_degraded_when_docling_down(client):
-    """Test health returns 200 with degraded status when docling-serve is down."""
-    mock_storage = MagicMock()
-    mock_storage.check_s3_access = AsyncMock(return_value=True)
-
-    mock_queue = MagicMock()
-    mock_queue.check_redis_connection = AsyncMock(return_value=True)
-    mock_queue.check_queue_depth = AsyncMock(return_value=0)
-
-    app.dependency_overrides[get_storage_service] = lambda: mock_storage
-    app.dependency_overrides[get_queue_service] = lambda: mock_queue
-
+    """/health stays 200 with status=degraded when only docling-serve is down."""
+    _override(storage_ok=True, redis_ok=True, queue_depth=0)
     try:
-        # Mock docling client as unhealthy (e.g. during model loading)
-        mock_docling = MagicMock()
-        mock_docling.check_health = AsyncMock(return_value=False)
-        with patch("src.services.docling_serve_client.get_docling_client", return_value=mock_docling):
+        with _patched_docling(False):
             response = client.get("/health")
 
-        # Should return 200 (not 503) — docling_serve is non-fatal
         assert response.status_code == status.HTTP_200_OK
         data = response.json()
         assert data["status"] == "degraded"
@@ -75,35 +74,62 @@ async def test_health_check_degraded_when_docling_down(client):
 
 
 @pytest.mark.asyncio
-async def test_health_check_unhealthy(client):
-    """Test health check when core services are unhealthy."""
-    # Create mock services
-    mock_storage = MagicMock()
-    mock_storage.check_s3_access = AsyncMock(return_value=False)
+async def test_health_check_unhealthy_when_core_down(client):
+    """/health returns 503 when a core store is unreachable."""
+    _override(storage_ok=False, redis_ok=False, queue_depth=-1)
+    try:
+        response = client.get("/health")
+        assert response.status_code == status.HTTP_503_SERVICE_UNAVAILABLE
+    finally:
+        app.dependency_overrides.clear()
 
-    mock_queue = MagicMock()
-    mock_queue.check_redis_connection = AsyncMock(return_value=False)
-    mock_queue.check_queue_depth = AsyncMock(return_value=-1)
 
-    # Override dependencies
-    app.dependency_overrides[get_storage_service] = lambda: mock_storage
-    app.dependency_overrides[get_queue_service] = lambda: mock_queue
+# ---------- /health/ready (strict) ----------
 
+
+@pytest.mark.asyncio
+async def test_readiness_ready_when_all_deps_ok(client):
+    """/health/ready returns 200 ready only when every dep is reachable."""
+    _override(storage_ok=True, redis_ok=True, queue_depth=0)
     try:
-        # Check health
-        response = client.get("/health")
+        with _patched_docling(True):
+            response = client.get("/health/ready")
+
+        assert response.status_code == status.HTTP_200_OK
+        data = response.json()
+        assert data["status"] == "ready"
+        assert data["checks"]["docling_serve"] is True
+    finally:
+        app.dependency_overrides.clear()
+
+
+@pytest.mark.asyncio
+async def test_readiness_not_ready_when_docling_down(client):
+    """/health/ready returns 503 when docling-serve is unhealthy."""
+    _override(storage_ok=True, redis_ok=True, queue_depth=0)
+    try:
+        with _patched_docling(False):
+            response = client.get("/health/ready")
 
-        # Assertions
         assert response.status_code == status.HTTP_503_SERVICE_UNAVAILABLE
+        data = response.json()
+        assert data["detail"]["status"] == "not_ready"
+        assert data["detail"]["checks"]["docling_serve"] is False
     finally:
-        # Cleanup overrides
         app.dependency_overrides.clear()
 
 
-def test_readiness_check(client):
-    """Test readiness check endpoint."""
-    response = client.get("/health/ready")
+@pytest.mark.asyncio
+async def test_readiness_not_ready_when_redis_down(client):
+    """/health/ready returns 503 when Redis is unreachable, even if other deps are up."""
+    _override(storage_ok=True, redis_ok=False, queue_depth=0)
+    try:
+        with _patched_docling(True):
+            response = client.get("/health/ready")
 
-    assert response.status_code == status.HTTP_200_OK
-    data = response.json()
-    assert data["status"] == "ready"
+        assert response.status_code == status.HTTP_503_SERVICE_UNAVAILABLE
+        data = response.json()
+        assert data["detail"]["status"] == "not_ready"
+        assert data["detail"]["checks"]["redis"] is False
+    finally:
+        app.dependency_overrides.clear()