A reviewer catches a database connection held while waiting

PR #917 added the MCP server. Its implementer and separate local reviewer were both Codex agents. The reviewer found that waiting for simulation results could keep an authentication database connection open for up to 30 seconds. Concurrent waiting clients could exhaust the available connections.

The implementer shortened the connection lifetime and added a test that checks no dependency connection remains open when the polling loop sleeps. The run finished without a recorded direct or broad intervention signal.

The review finding

get_mcp_actor receives conn=Depends(get_db), so FastAPI keeps that yielded connection open for the whole /api/mcp request. For the wait_for_run tool, the handler then loops and sleeps up to 30 seconds before returning, which violates the spec requirement that wait_for_run not hold a database connection while waiting and can exhaust DB connections under concurrent MCP clients. Authenticate with a short-lived connection that is closed before dispatching the tool, or otherwise avoid a yield-scoped DB dependency around the polling loop.

Review attempt 3, against bd7aed5f83f90885d9ea35cbab55f85a091810c1.

The merged correction

Merge 06b8f934bb0b12f9fc8551c794ebe291a1845033.

api/mcp/router.py

Lines 102-108 in the recorded merge.

    conn = repository.get_connection(str(settings.database_url))
    try:
        user, service_key_id, kind = authenticate_user_from_service_key(token, conn=conn)
    except Exception as exc:
        raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="Authentication required.") from exc
    finally:
        conn.close()

tests/test_mcp_core.py

Lines 388-438 in the recorded merge.

async def test_wait_for_run_does_not_hold_get_db_connection_while_sleeping(monkeypatch, mcp_client):
    client, database_url, app = mcp_client
    _owner_user_id, _service_key_id, token = _seed_user_and_service_key(database_url)
    active_get_db_connections = 0
    rest_calls = 0

    def counting_get_db():
        nonlocal active_get_db_connections
        conn = repository.get_connection(database_url)
        active_get_db_connections += 1
        try:
            yield conn
        finally:
            active_get_db_connections -= 1
            conn.close()

    async def fake_rest_call(request, actor, method, path, *, json_body=None):
        nonlocal rest_calls
        rest_calls += 1
        status = "queued" if rest_calls == 1 else "succeeded"
        return httpx.Response(
            200,
            json={
                "id": 55,
                "status": status,
                "run_kind": "simulation",
                "stats": {},
            },
        )

    async def fake_sleep(_seconds):
        assert active_get_db_connections == 0
        await real_sleep(0)

    mcp_router = importlib.import_module("api.mcp.router")

    app.dependency_overrides[get_db] = counting_get_db
    real_sleep = mcp_router.asyncio.sleep
    monkeypatch.setattr(mcp_router, "_rest_call", fake_rest_call)
    monkeypatch.setattr(mcp_router.asyncio, "sleep", fake_sleep)

    resp = await _rpc(
        client,
        token,
        "tools/call",
        {"name": "wait_for_run", "arguments": {"run_id": 55, "max_wait_seconds": 1}},
    )

    assert resp.status_code == 200, resp.text
    assert resp.json()["result"]["structuredContent"]["run_id"] == 55
    assert rest_calls == 2

Source record

Selected finding, final approval, gate histories and code excerpts.

The historical review and merged code were inspected; the historical tests were not rerun for this publication. The code shown is excerpted from the recorded commit, not a reconstructed example.

More review findings.