On assertion failure the backend under test is parked 'idle in transaction' —
exactly the bug this PR fixes — and would poison the shared test pool for the
rest of the session. Terminate it in a finally regardless of pass/fail.
(CodeRabbit review, tests/test_dependencies.py)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
_run_to_completion passed _CLEANUP_TIMEOUT_SECONDS to each asyncio.wait()
call, so every re-delivered CancelledError re-armed a fresh 10s window and
the deadline never elapsed. anyio re-delivers cancellation on every event
loop iteration (CancelScope._deliver_cancellation reschedules itself via
call_soon until the task leaves the scope), and Starlette's
BaseHTTPMiddleware -- registered for every request in main.py -- wraps the
request in an anyio task group. So on the API path the abandon valve was
dead exactly where it was needed: a wedged connection pinned the request
task, hot-spinning the loop, for as long as the storm lasted.
Compute one monotonic deadline before the loop and derive each wait timeout
from the remaining time.
Tests: storm helper now re-cancels at anyio's call_soon cadence instead of
5 sparse cancels (sparse cancels leave quiet ticks where a per-wait timeout
re-arms and cleanup finishes, which is why the existing storm test passed
either way). New test_run_to_completion_deadline_holds_under_cancel_storm
fails on the old loop ("not abandoned after 5.0s") and passes with the fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review findings on the cancellation-shield fix:
- Bound _run_to_completion with _CLEANUP_TIMEOUT_SECONDS. Shielded cleanup is
uninterruptible by design, so a wedged connection (dead socket, no libpq
timeout) would otherwise pin the task forever; past the bound we abandon
cleanup rather than hang. Switched the wait to asyncio.wait (shields fut,
supports a timeout) and guarded fut.exception()/fut.cancelled() so loop
shutdown can't raise from the tail.
- tracked_db: reset request_context in its own finally so a cancelled cleanup
path can't leak the task-scoped contextvar into a reused long-lived task
(deriver).
- Tests: reset-context-on-cancellation, abandon-wedged-cleanup, and a real-DB
wire cancellation test asserting no `idle in transaction` backend lingers
after a cancelled in-flight write transaction (pg_stat_activity).
Full suite: 21 passed live (incl. 4 pre-existing + 4 new real-DB tests).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cancelled client-facing requests (search, dialectic chat/stream) could
abandon an open transaction: when a request task is cancelled, the
CancelledError can be re-delivered onto the ROLLBACK/close awaits in the
session-cleanup `finally`, so ROLLBACK never reaches Postgres and close()
never runs. Supavisor (transaction mode) does not reset the orphaned
backend on disconnect, leaving it `idle in transaction` indefinitely.
These zombies accrued ~5/hour, exhausting the connection pool and pinning
the xmin horizon.
Fix, centralized in the three session entry points (get_db, get_read_db,
tracked_db) so every subsystem inherits it:
- `_run_to_completion`: run cleanup as a detached task and await it through
asyncio.shield in a loop, so a (re-delivered) cancellation cannot
interrupt ROLLBACK/close. Survives a cancel storm.
- `_finalize_session`: rollback then always close (inner try/finally);
invalidate the connection on a broken-mid-protocol InterfaceError/
OperationalError/DBAPIError so a dead connection is not returned to the
pool; never let a cleanup error mask the original.
Note: anyio.CancelScope(shield=True) — the originally-proposed primitive —
does NOT defer a native asyncio.Task.cancel() (verified empirically under
both asyncio and anyio loops), which is how Starlette and the deriver's
uvloop cancel; hence the stdlib approach. No new dependency.
Tests: cancellation, re-delivery-during-cleanup, cancel-storm,
broken-connection->invalidate, and DI-path teardown.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat: implement read DB and fix queue stale cleanup
* fix: use read_db in internal methods
* fix: mention read db in the CLAUDE.md
* fix: make TRACING checkout hook autocommit-safe; sample cleanup-gate jitter once
The DB.TRACING checkout hook ran `SELECT set_config(...)` at pool checkout,
before the dialect applies the read engine's AUTOCOMMIT isolation level. That
statement autobegins a transaction, and psycopg then refuses to switch the
connection into AUTOCOMMIT ("can't change 'autocommit' now: connection in
transaction status INTRANS"), so every read_only session 500s under TRACING and
the INTRANS connection leaks back to poison later write checkouts. Run the hook
in autocommit and restore the prior mode so it never leaves an open transaction;
set_config(..., is_local=false) is session-scoped and survives the boundary.
Add a regression test (fails without the fix) covering read_only + TRACING.
Also sample the stale-cleanup gate's jittered interval once per attempt instead
of re-rolling it every poll, so the spacing is a fixed deadline per cycle rather
than a random walk (and is testable at non-zero jitter ratios).
* fix: reset request_context in TRACING checkout-hook test
---------
Co-authored-by: Vineeth Voruganti <13438633+VVoruganti@users.noreply.github.com>
* fix(deriver): Remove connection retry logic and add jitter to polling interval
* chore(docs): Update changelog and document new configurations
* chore: increment version numbers
* feat(db): add connection retry, adaptive deriver polling, and pool metrics
Add resilience and visibility for DB connection handling under transaction-
pooler (Supavisor) saturation, where client-connection limits get exhausted
across many tenants.
- get_db/tracked_db now force an eager pool checkout with bounded exponential
backoff (tenacity), retrying SQLAlchemy TimeoutError + OperationalError so
transient pooler rejections degrade gracefully instead of 500ing. Toggle via
DB_CONNECTION_RETRY_ENABLED (+ delay/backoff knobs); ~10s default budget.
- Deriver polling backs off when idle or erroring (base -> max, x2 each cycle)
and snaps back to base on claimed work, cutting steady-state query load.
Toggle via DERIVER_POLLING_BACKOFF_ENABLED (+ max/multiplier).
- Add scrape-time db_pool_connections Prometheus gauge (checked_out/checked_in/
size/overflow, labeled api|deriver), registered in both the API lifespan and
the deriver metrics server.
- Make SqlalchemyIntegration explicit in both Sentry inits; wrap connection
acquisition in a db.pool.acquire span and capture live pool stats on
retry-exhaustion.
* feat(db): add acquisition counter and in-flight query gauge
Build on the pool-connection metrics with two signals that turn detection
into diagnosis under transaction-pooler saturation:
- db_connection_acquisitions{outcome=ok|retried|exhausted}: counts how often
connection checkout retries through pooler rejection — the alertable early
warning before requests start failing.
- db_queries_in_flight: statements actually executing on the wire (via
SQLAlchemy cursor-execute events, drift-proof across query errors). Pairs
with checked_out: the gap reveals connections held but parked (the "idle in
transaction during an external call" antipattern). Labeled namespace +
instance_type only; gated on METRICS.ENABLED for zero overhead when off.
Add DB-free unit tests for retry outcomes, polling backoff, and in-flight
gauge drift handling.
* fix: address CodeRabbit review on PR #758
- db: roll back the session on a retryable checkout failure before
retrying — a failed autobegin can leave it pending-rollback, making the
next db.connection() raise instead of re-checking-out cleanly. Cheap
Python-side cleanup when no connection was bound.
- metrics: guard DBPoolCollector.collect() so a pool-read/import hiccup
can't raise and abort the whole /metrics scrape (Prometheus drops ALL
metrics if any collector raises) — log and fall back to empty.
* fix(db): lazy retrying session + review fixes for connection backoff
Address Codex/CodeRabbit review on PR #758.
- Replace eager checkout with HonchoAsyncSession: a lazy AsyncSession that
checks out its connection (with retry) on the first DB-touching call, not at
construction. Request handlers doing non-DB work (embedding/file/LLM) before
their first query no longer pin a connection across it, while the API path
still gets checkout retry. Only the checkout is retried — the statement runs
once via super(), so writes are never duplicated. Tracing's set_config moves
into the same lazy acquire hook.
- Roll the session back on a retryable checkout failure before retrying, so a
failed autobegin can't leave it pending-rollback.
- Lower default POOL_TIMEOUT to 5s and validate it stays under the retry budget
for pooled (non-null) POOL_CLASS; update config.toml.example and v2/v3 docs.
- Clamp pool overflow gauge to >= 0 (was negative before the pool fills).
- Remove double-sleep in the deriver idle poll (true backoff cap, not 2x);
make in-flight instrumentation registration idempotent.
- Tests: HonchoAsyncSession lazy/idempotent acquire, statement-runs-once,
tracing, commit/rollback flag reset, get_db no-acquire-at-entry, polling-loop
single-sleep, and the POOL_TIMEOUT/retry-budget validator.
* fix(db): cover all DB-touching session methods; clear flag on close/reset
Address Codex follow-up review on PR #758 (polish, no behavior-critical bug).
- HonchoAsyncSession: wrap get/get_one/stream/stream_scalars/delete in addition
to execute/scalar/scalars/flush/merge/refresh/commit, so the "lazy checkout
with retry on first DB use" guarantee has no holes. connection() stays
unwrapped (acquire_connection_with_retry calls it — wrapping would recurse).
- Reset the acquired flag on close()/reset() too, so a session reused after
close/reset re-acquires (and re-wraps retry) on its next DB use.
- Fix stale comments: connection retry now applies lazily to the request path
via HonchoAsyncSession (config.py), and the FakeSession helper note.
- Tests: close/reset flag reset, and get/delete route through acquisition.
adavyas
Vineeth Voruganti
Rajat Ahuja