fix: release request sessions before route telemetry

[neubig]

Concurrent API requests could occupy the entire database pool, then each wait for a second connection needed by route telemetry. FastAPI still owned the request sessions because the telemetry middleware ran between response headers and response cleanup. Even successful requests therefore waited for the pool timeout, starving other API requests.

Move route telemetry into the existing pure-ASGI TelemetryContextMiddleware, after the inner application completes its response and dependency cleanup. Remove the separate call_next middleware. Status/exception reporting and context extraction remain covered; database pool configuration is unchanged.

Refs #347. This fixes the session-ownership cycle; run-summary batching and the separately proposed pool-configuration changes remain outside this PR.

Validation: a real two-slot SQLite/FastAPI regression fails before the change with both request connections retained, then passes after while successfully reading stored consent. All30 focused middleware, telemetry, and CORS tests pass, as do Ruff, pycodestyle, and pyright. The production-only two-file patch also applies cleanly to the deployed c756d24 version without migrations or dependency changes; live recovery is tracked separately.

HUMAN: This PR was authored with AI assistance.

[github-actions]

Warning

Your comment is too long (maximum is 65536 characters), so the coverage report was not added. See the job log for how to reduce it.

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: efbadb66939d64d2eb745779d116cfb08c72fc64
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/7fa51fc3-f660-48cf-ac06-dc1a3fc5701f

This comment was posted by an AI agent (OpenHands).

[all-hands-bot]

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Review: fix: release request sessions before route telemetry

Good taste — Elegant, minimal solution that eliminates a real concurrency bug without adding complexity.

Analysis

The PR correctly identifies and fixes a database pool exhaustion cycle: the old call_next-based HTTP middleware ran telemetry between response headers and dependency cleanup, meaning request-scoped DB sessions were still held when telemetry opened its own session from the same pool. Under concurrent load, every request held one connection and waited for another, deadlocking the pool.

The fix moves route telemetry into the pure-ASGI TelemetryContextMiddleware's finally block. This is architecturally correct: await self.app(scope, receive, capture_status) in ASGI returns only after the entire request lifecycle completes — including FastAPI's dependency finally blocks that close request sessions. The finally block therefore runs after those sessions are released, breaking the ownership cycle.

Key correctness points verified:

• Status capture: The capture_status send-wrapper correctly intercepts http.response.start to record the status code.
• Exception path: On exception, status_code=500 and exception_type are set, the exception is re-raised, and the finally block still captures telemetry. Since capture_automation_event swallows its own exceptions internally, telemetry cannot mask the original error.
• Scope attributes: Request(scope) is constructed early but only reads scope attributes (route, endpoint, url.path) that are populated by the router before finally runs. No body reading occurs.
• No leftover references: api_route_telemetry_middleware is fully removed from both app.py and middleware.py.

Testing

The regression test (test_route_telemetry_releases_request_connections_first) is a genuine integration test: it creates a 2-connection SQLite pool, fires two concurrent requests that hold sessions via an asyncio.Barrier, and asserts that pool.checkedout() < 2 when telemetry's callback runs and pool.checkedout() == 0 afterward. This directly exercises the pool exhaustion scenario and would fail with the old call_next approach.

The status/exception test parametrizes success (201) and failure (ValueError -> 500) paths, verifying both status code and exception type propagation.

[RISK ASSESSMENT]

• Overall PR: LOW
  • No API changes, no migrations, no dependency changes.
  • The fix is a middleware restructuring that preserves all existing telemetry behavior (status capture, exception reporting, consent checking, route filtering).
  • The deferred import in the middleware is a pre-existing pattern for avoiding circular dependencies.
  • Tests are real integration tests, not mock-based.

VERDICT: Worth merging

KEY INSIGHT: Moving telemetry from call_next-based HTTP middleware to a pure-ASGI finally block is the correct fix — ASGI middleware completes after full dependency cleanup, guaranteeing request sessions are released before telemetry opens its own.

Automated review used the wrong decision (APPROVED instead of COMMENT) and is dismissed. Findings are reposted as a comment.

[all-hands-bot]

Posted an APPROVED review on PR #458 (OpenHands/automation).

Verdict: ✅ Worth merging — 🟢 LOW risk, no material issues found.

Summary of review:
The PR fixes a database pool exhaustion cycle by moving route telemetry from a call_next-based HTTP middleware (which ran between response headers and dependency cleanup) into the pure-ASGI TelemetryContextMiddleware's finally block. This ensures await self.app(...) completes — including FastAPI's dependency cleanup that closes request-scoped DB sessions — before telemetry opens its own connection from the same pool.

Key verification points:

• Status capture via capture_status send-wrapper is correct
• Exception path properly sets 500 + exception_type and re-raises
• Request(scope) reads only scope attributes populated by the router before finally runs
• No leftover references to the removed api_route_telemetry_middleware
• Tests are genuine integration tests (real SQLite pool, concurrent requests, barrier synchronization) that would fail with the old approach

Per the repo's custom code review guide, the review was submitted as APPROVED (not COMMENTED) since the verdict is "Worth merging" with LOW risk and no blocking issues.

This comment was posted by an AI agent (OpenHands).