fix: address R2 review findings - shutdown wiring, /health auth bypass, HEALTHCHECK

- Wire GracefulShutdownHandler into uvicorn lifecycle via serve_with_shutdown()
  with finally block guaranteeing shutdown runs after server.serve()
- Move /health endpoint OUTSIDE BearerTokenMiddleware for K8s probe access
- Set uvicorn timeout_graceful_shutdown to half of shutdown budget
- Change Dockerfile OPENSPACE_SHUTDOWN_TIMEOUT from 30 to 8 (fits Docker 10s)
- Replace HEALTHCHECK with real HTTP /health endpoint check (not false-positive import)
- Add 2 new tests: /health-outside-auth, shutdown-after-uvicorn, Docker timeout alignment
- 41 deployment tests, 1925 total passed

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: Brian Krafft

Dockerfile

@@ -63,18 +63,18 @@ ENV OPENSPACE_MCP_HOST=0.0.0.0 \
     OPENSPACE_MCP_PORT=8000 \
     OPENSPACE_MCP_TRANSPORT=streamable-http \
     OPENSPACE_LOG_LEVEL=INFO \
-    OPENSPACE_SHUTDOWN_TIMEOUT=30 \
+    OPENSPACE_SHUTDOWN_TIMEOUT=8 \
     OPENSPACE_METRICS_ENABLED=true \
     OPENSPACE_SKILL_STORE_PATH=/app/skills \
     PYTHONUNBUFFERED=1
 
 EXPOSE 8000
 
-# Health check: Python-based (works for both stdio and HTTP transports)
-# For HTTP transports, /health endpoint is available via Starlette
-# For stdio transport, override with HEALTHCHECK NONE in docker-compose
+# Health check: hits the real /health endpoint (unauthenticated)
+# Default transport is streamable-http, so HTTP check works.
+# For stdio transport, override with HEALTHCHECK NONE in docker-compose.
 HEALTHCHECK --interval=30s --timeout=10s --start-period=15s --retries=3 \
-    CMD python -c "from openspace.observability.health import health; r=health.check(); exit(0 if r.get('status')=='healthy' else 1)" || exit 1
+    CMD python -c "import urllib.request; urllib.request.urlopen('http://localhost:8000/health')" || exit 1
 
 # Graceful shutdown: Docker sends SIGTERM, handler drains in-flight tasks
 STOPSIGNAL SIGTERM

openspace/mcp/server.py

@@ -250,9 +250,14 @@ def run_mcp_server(mcp=None) -> None:
     else:
         starlette_app = mcp.streamable_http_app()
 
-    # Add /health endpoint for container orchestrators
+    # Auth middleware chain: request → BearerAuth → RateLimit → MCP app
+    rate_limited_app = RateLimitMiddleware(starlette_app)
+    protected_app = BearerTokenMiddleware(rate_limited_app, token)
+
+    # /health is OUTSIDE auth — K8s probes can't send bearer tokens
+    from starlette.applications import Starlette
     from starlette.responses import JSONResponse
-    from starlette.routing import Route
+    from starlette.routing import Mount, Route
 
     from openspace.observability.health import health
 
@@ -261,40 +266,43 @@ async def health_endpoint(request):
         status_code = 200 if result.get("status") == "healthy" else 503
         return JSONResponse(result, status_code=status_code)
 
-    # Wrap the MCP app with health route
-    from starlette.applications import Starlette
-    from starlette.routing import Mount
-
-    health_app = Starlette(
+    app = Starlette(
         routes=[
             Route("/health", health_endpoint, methods=["GET"]),
-            Mount("/", app=starlette_app),
+            Mount("/", app=protected_app),
         ]
     )
 
-    # Middleware chain: request → BearerAuth → RateLimit → health+MCP app
-    rate_limited_app = RateLimitMiddleware(health_app)
-    protected_app = BearerTokenMiddleware(rate_limited_app, token)
-
     logger.info(
         "Starting MCP server with bearer auth + rate limiting on %s:%d (%s transport)",
         args.host,
         args.port,
         args.transport,
     )
 
-    # Install graceful shutdown handler
+    # Graceful shutdown: uvicorn handles SIGTERM → drain HTTP → our hooks run after
     from openspace.deploy.shutdown import GracefulShutdownHandler
 
     shutdown_handler = GracefulShutdownHandler(timeout=deploy_cfg.shutdown_timeout)
 
+    # Give uvicorn half the budget for HTTP draining, our handler gets the rest
+    uvicorn_timeout = max(deploy_cfg.shutdown_timeout // 2, 1)
+
     config = uvicorn.Config(
-        protected_app,
+        app,
         host=args.host,
         port=args.port,
         log_level=deploy_cfg.log_level.lower(),
+        timeout_graceful_shutdown=uvicorn_timeout,
     )
     server = uvicorn.Server(config)
+
+    async def serve_with_shutdown():
+        try:
+            await server.serve()
+        finally:
+            await shutdown_handler.shutdown()
+
     import anyio
 
-    anyio.run(server.serve)
+    anyio.run(serve_with_shutdown)

tests/test_deployment.py

@@ -326,12 +326,31 @@ def test_dockerfile_non_root_user(self):
         user_lines = [l for l in lines if l.strip().startswith("USER ")]
         assert user_lines, "Dockerfile must have USER instruction (not in comment)"
 
+    def test_dockerfile_shutdown_timeout_under_docker_default(self):
+        """Dockerfile OPENSPACE_SHUTDOWN_TIMEOUT must be < Docker's 10s default."""
+        content = (ROOT / "Dockerfile").read_text()
+        import re
+
+        match = re.search(r"OPENSPACE_SHUTDOWN_TIMEOUT=(\d+)", content)
+        assert match, "Dockerfile must set OPENSPACE_SHUTDOWN_TIMEOUT"
+        timeout = int(match.group(1))
+        assert timeout < 10, (
+            f"Shutdown timeout {timeout}s must be < Docker's 10s stop_grace_period"
+        )
+
     def test_dockerfile_healthcheck(self):
         content = (ROOT / "Dockerfile").read_text()
         lines = content.splitlines()
         hc_lines = [l for l in lines if l.strip().startswith("HEALTHCHECK")]
         assert hc_lines, "Dockerfile must have HEALTHCHECK instruction"
 
+    def test_dockerfile_healthcheck_hits_real_endpoint(self):
+        """HEALTHCHECK must hit /health HTTP endpoint, not just import."""
+        content = (ROOT / "Dockerfile").read_text()
+        assert "localhost:8000/health" in content, (
+            "HEALTHCHECK must hit real /health endpoint for accurate checks"
+        )
+
     def test_dockerfile_no_secrets(self):
         """Dockerfile must not contain hardcoded secrets."""
         content = (ROOT / "Dockerfile").read_text().lower()
@@ -378,23 +397,37 @@ def test_deploy_config_wired_into_server(self):
         source = inspect.getsource(run_mcp_server)
         assert "DeployConfig" in source, "DeployConfig must be used in run_mcp_server"
 
-    def test_health_endpoint_wired_into_server(self):
-        """HTTP transports have /health route."""
+    def test_health_endpoint_outside_auth(self):
+        """/health must be accessible WITHOUT bearer auth for K8s probes."""
         import inspect
 
         from openspace.mcp.server import run_mcp_server
 
         source = inspect.getsource(run_mcp_server)
-        assert "/health" in source, "/health route must exist for HTTP transports"
-
-    def test_shutdown_handler_wired_into_server(self):
-        """GracefulShutdownHandler is created in run_mcp_server."""
+        # /health route must be mounted BEFORE auth middleware in ASGI chain
+        health_pos = source.find("Route(\"/health\"")
+        bearer_pos = source.find("BearerTokenMiddleware")
+        assert health_pos > 0, "/health route must exist"
+        assert bearer_pos > 0, "BearerTokenMiddleware must exist"
+        # In the code, auth wraps the MCP app; health wraps auth+MCP at top level
+        # So health_endpoint definition comes AFTER protected_app is built
+        assert health_pos > bearer_pos, (
+            "/health must be outside auth — route should wrap protected_app"
+        )
+
+    def test_shutdown_runs_after_uvicorn(self):
+        """GracefulShutdownHandler.shutdown() must run after server.serve()."""
         import inspect
 
         from openspace.mcp.server import run_mcp_server
 
         source = inspect.getsource(run_mcp_server)
-        assert "GracefulShutdownHandler" in source
+        assert "serve_with_shutdown" in source, (
+            "Shutdown must be wired via serve_with_shutdown wrapper"
+        )
+        assert "finally:" in source, (
+            "Shutdown must run in finally block to guarantee execution"
+        )
 
 
 # ═══════════════════════════════════════════════════════════════════════