diff --git a/CLAUDE.md b/CLAUDE.md index a7cac83..bb1b7bc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -85,6 +85,7 @@ docs/superpowers/plans/ # implementation plans - **Exception handlers in `server.py`:** `KeyError` → 404, `ValueError` → 400, Pydantic validation → 422. Use the existing handlers — don't add try/except in route bodies. - **Tool functions raise `KeyError` for "unknown id" and `ValueError` for invalid state transitions** (e.g. confirming a CANCELLED pending). The handlers translate these to clean HTTP statuses. - **The two-step submit flow is core, not optional.** `prepare_submit_job` snapshots the connection's `master` / `deploy_mode` / `spark_conf` into the `PendingSubmission`; `confirm_submit_job` is the only place `spark-submit` is invoked. Editing a connection between prepare and confirm does **not** retarget the pending job. +- **MCP session affinity: keep `GUNICORN_WORKERS=1` (or front with a sticky-session LB).** The `mcp` library stores each session in a per-process dict (`StreamableHTTPSessionManager._server_instances`); gunicorn round-robins requests across workers, so a multi-worker deploy returns "Session not found" / "Invalid or expired session ID" for the same `mcp-session-id` whenever it lands on a worker that didn't create it. `fastapi-mcp` hardcodes `stateless=False`, so there is no in-process workaround. The `on_starting` hook in `gunicorn.conf.py` logs a WARNING whenever `workers > 1` so the misconfig is loud, not silent. See the comment block above `workers =` in `gunicorn.conf.py` for full context and the nginx-sticky escape hatch. - **Test fixtures rebind module-level singletons** (`connections.store`, `submit.conn_store`, `submit.pending_store`) in `monkeypatch.setattr` because the tool modules captured the originals at import time. See `tests/integration/test_mcp_routes.py` for the pattern. - **86 tests pass** as of the last Stage 1 cleanup; run `uv run pytest` after any change. diff --git a/gunicorn.conf.py b/gunicorn.conf.py index 12bd4ab..c848055 100644 --- a/gunicorn.conf.py +++ b/gunicorn.conf.py @@ -22,11 +22,52 @@ bind = os.environ.get("GUNICORN_BIND", "0.0.0.0:8000") # 2 is a sensible default for a small containerized MCP server: lets a slow # yarn logs request run in parallel with a status check. Scale up by # setting GUNICORN_WORKERS at deploy time. +# +# ⚠️ MCP SESSION AFFINITY CONSTRAINT ⚠️ +# ---------------------------------------- +# The official `mcp` library (used under fastapi-mcp) keeps each MCP +# session's state in a per-process in-memory dict +# (`StreamableHTTPSessionManager._server_instances`). gunicorn round-robins +# HTTP requests across worker processes, so a client that does: +# 1) POST /initialize → worker A creates session_id=xyz +# 2) POST /tools/list → worker B has no record of xyz +# 3) → "Session not found" / "Invalid or expired session ID" +# gunicorn has no built-in sticky-session support, and fastapi-mcp hardcodes +# `stateless=False`, so there is no in-process way to share sessions across +# workers without an external store (Redis, etc.). +# +# Trade-off: `workers=1` eliminates the bug entirely. A single uvicorn-based +# worker still handles many concurrent connections via the asyncio event +# loop — the lost capacity is parallel CPU work, which this I/O-bound MCP +# service rarely needs. If you ever need true multi-core parallelism, you +# MUST front gunicorn with a sticky-session reverse proxy (nginx +# `ip_hash` / `hash $mcp_session_id consistent`) AND give every session +# a shared backing store — do NOT just bump GUNICORN_WORKERS. +# +# The `on_starting` hook below makes this footgun loud rather than silent. workers = int(os.environ.get("GUNICORN_WORKERS", "2")) worker_class = "uvicorn.workers.UvicornWorker" # 1 thread per worker is enough for ASGI handlers (no blocking I/O). threads = int(os.environ.get("GUNICORN_THREADS", "1")) + +def on_starting(server): + """Master-process hook: gunicorn calls this once before forking workers. + + We use it to scream at anyone who raises GUNICORN_WORKERS above 1 without + putting a sticky-session LB in front — see the workers= comment above for + why MCP sessions cannot survive worker-local storage across processes. + """ + if workers > 1: + server.log.warning( + "GUNICORN_WORKERS=%d — MCP sessions are stored in per-process " + "memory, so the same mcp-session-id will frequently land on a " + "different worker and return 'Session not found'. Either set " + "GUNICORN_WORKERS=1 or front this service with a sticky-session " + "reverse proxy (see gunicorn.conf.py for details).", + workers, + ) + # --- Lifecycle --- # Generous timeout because the slowest tool call here is yarn logs (which # can take 30+ seconds on a busy cluster). uvicorn standalone defaults to diff --git a/tests/unit/test_gunicorn_config.py b/tests/unit/test_gunicorn_config.py new file mode 100644 index 0000000..b711f4a --- /dev/null +++ b/tests/unit/test_gunicorn_config.py @@ -0,0 +1,116 @@ +# coding=utf-8 +""" +@Time :2026/6/24 +@Author :tao.chen + +Regression tests for gunicorn.conf.py. + +The MCP session-affinity constraint is documented in gunicorn.conf.py: the +mcp library stores each session in a per-process dict, so a multi-worker +gunicorn deploy returns "Session not found" ~50% of the time. The fix in +this config is an `on_starting` hook that warns loudly when workers > 1. + +These tests assert that: + 1. The hook exists and is wired up. + 2. workers == 1 is silent (no false alarms in the default single-worker + mode people use after reading the comment). + 3. workers > 1 emits a WARNING that mentions the symptom and the fix. + +If anyone ever "cleans up" the hook, these tests fail — that's the point. +""" +import importlib.util +import sys +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[2] +GUNICORN_CONF = REPO_ROOT / "gunicorn.conf.py" + + +def _load_gunicorn_conf(): + """Import gunicorn.conf.py as a module (not a package member). + + The file lives at the repo root and is loaded by gunicorn via exec(); + we use importlib so the test can introspect `on_starting` and mutate + `workers` between cases. + """ + spec = importlib.util.spec_from_file_location("gunicorn_conf_under_test", GUNICORN_CONF) + assert spec is not None and spec.loader is not None, f"cannot load {GUNICORN_CONF}" + module = importlib.util.module_from_spec(spec) + # Make sure repo root is on sys.path so the module's `import os` etc. work. + if str(REPO_ROOT) not in sys.path: + sys.path.insert(0, str(REPO_ROOT)) + spec.loader.exec_module(module) + return module + + +class _FakeServer: + """Minimal stand-in for gunicorn's `server` argument to on_starting. + + gunicorn passes a `gunicorn.arbiter.Arbiter`-like object that exposes + `server.log.warning(...)`. We only need the .log.warning sink. + """ + + def __init__(self): + self.warnings: list[str] = [] + + class _Log: + def __init__(self, sink): + self._sink = sink + + def warning(self, fmt: str, *args) -> None: # noqa: A002 - matches gunicorn API + self._sink.append(fmt % args if args else fmt) + + @property + def log(self) -> "_FakeServer._Log": + return self._Log(self.warnings) + + +@pytest.fixture +def gunicorn_conf(): + """Load gunicorn.conf.py once per test, then reset `workers` to the + project default so other tests in the suite are not affected by us + mutating the module global.""" + mod = _load_gunicorn_conf() + yield mod + # Restore the env-driven default so a stale value doesn't leak out. + import os + mod.workers = int(os.environ.get("GUNICORN_WORKERS", "2")) + + +def test_on_starting_hook_is_defined(gunicorn_conf): + """The hook must exist — the bug fix lives here.""" + assert callable(getattr(gunicorn_conf, "on_starting", None)), ( + "gunicorn.conf.py is missing the on_starting hook. The MCP session-" + "affinity warning is the only thing that keeps a multi-worker deploy " + "from silently returning 'Session not found' for half its requests." + ) + + +def test_on_starting_silent_for_single_worker(gunicorn_conf): + """workers == 1 is the safe config; no warning should fire.""" + gunicorn_conf.workers = 1 + server = _FakeServer() + gunicorn_conf.on_starting(server) + assert server.warnings == [], ( + f"workers=1 should be silent (no MCP session affinity issue), got: {server.warnings}" + ) + + +def test_on_starting_warns_for_multi_worker(gunicorn_conf): + """workers > 1 must produce a warning that names the symptom and the fix. + + The point of the hook is to be loud at deploy time, not at request time. + """ + gunicorn_conf.workers = 4 + server = _FakeServer() + gunicorn_conf.on_starting(server) + assert len(server.warnings) == 1, ( + f"expected exactly one warning when workers=4, got {len(server.warnings)}: {server.warnings}" + ) + msg = server.warnings[0] + assert "GUNICORN_WORKERS=4" in msg, "warning should name the offending worker count" + assert "Session not found" in msg, "warning should name the symptom users actually see" + assert "GUNICORN_WORKERS=1" in msg, "warning should point at the easy fix" + assert "sticky-session" in msg, "warning should mention the load-balancer escape hatch"