Skip to content

Release pooled connections when their borrowing coroutine exits - #14

Merged
adhikjoshi merged 2 commits into
mainfrom
fix/daemon-connection-residual
Aug 20, 2026
Merged

adhikjoshi merged 2 commits into
mainfrom
fix/daemon-connection-residual

Conversation

@adhikjoshi

@adhikjoshi adhikjoshi commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Problem

Worker::handle() only tears down the HTTP request coroutine's pooled connections. Connections borrowed by child coroutines (Coroutine::create in app code) and coroutine-based daemons bypassed release() entirely, with two consequences:

  1. Permanent counter drift → false pool exhaustion. Each such borrow leaks one pool slot forever. Reproduced end-to-end on main with a real Octane server against MySQL: after just 10 child-coroutine borrows, the pool pins at 10/0 and 0/20 subsequent HTTP requests succeed — the worker is down while looking healthy. Observed in prod as the one uncensored-chat connection that outlived every recycling cycle and grew its leaked-statement count unbounded.
  2. Unbounded statement leaks on long-lived borrows. max_lifetime recycling only runs at release, so a connection that never releases accumulates leaked server-side prepared statements indefinitely (the error-1461 incident class).

Fix — defense in depth

  • DatabaseManager arms a Coroutine::defer at borrow time (once per coroutine): at coroutine exit it collects the coroutine's cycle garbage at the safe point (same ordering as the Worker) and releases whatever the context still holds. A no-op for request coroutines — the Worker empties the context first.
  • DatabasePool tracks every connection it created via WeakReference and heals the counter for any that were garbage-collected without release — including the object-id-reuse race where an abandoned connection dies inside factory->make() and the new connection inherits its id.
  • poolStats() now reports tracked_connections and long_borrowed_connections (borrows held past max_lifetime — a live pointer at daemons that never release).

Verification

  • End-to-end (real octane:start, real MySQL, this branch): 50 child-coroutine borrows → pool at 1 connection fully returned, 20/20 requests OK, 0 leaked statements (main: exhausted at 10, service down).
  • Mixed load (wrk + 200 spawns, 30s): 732 req/s, zero non-2xx, all 10 pool slots returned — no throughput regression.
  • PR Fix server-side prepared statement leak from pooled connections #13 leak-ordering regression storm: 92 leaked vs 566 on old code — unchanged band.
  • 167 package tests green; every new guard is mutation-tested (fix removed → its test goes red): exit-hook release, gc-before-release ordering, heal-on-overwrite for reused object ids, no-op-prune skip, vanished-slot reconciliation.
  • Swoole semantics verified empirically on this build: defers run before context destruction; Channel holds real zval references (a channel-held connection can never be misread as vanished).
  • Adversarial review round completed; all findings fixed and covered by the mutation-tested cases above.

Known residual

A daemon coroutine that literally never ends still holds its connection (nothing can force-release a borrow in use); long_borrowed_connections now makes those visible, and such loops should call releaseConnections() per iteration — documented in the code.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

Worker::handle() only tears down the HTTP request coroutine's borrows.
Connections borrowed by child coroutines and coroutine-based daemons
bypassed release() entirely: the pool counter drifted one slot per
borrow toward false 'Connection pool exhausted', and long-lived
borrowers accumulated leaked server-side prepared statements that
max_lifetime recycling could never reach (recycling happens at release).

Two mechanisms, defense in depth:
- DatabaseManager arms a Coroutine::defer at borrow time that releases
  whatever the coroutine still holds when it ends. A no-op for the
  request path (the Worker empties the context first).
- DatabasePool tracks every connection it created via WeakReference and
  heals the counter for any that were garbage-collected without
  release() - covering paths the defer cannot see.

poolStats() now also reports tracked_connections and
long_borrowed_connections (borrows held past max_lifetime - a live
pointer at daemons that never release).
- Heal a dead tracked slot when its object id is reused by a new
  connection: allocation-triggered gc inside factory->make() can free an
  abandoned connection and hand its id to the connection being created,
  and overwriting the dead weak reference would strand that slot's
  counter increment forever - reopening the drift-to-exhaustion class.
- Collect the coroutine's cycle garbage in the exit hook before
  releasing, mirroring the Worker's safe-point ordering, so statements
  hidden in cycles close against a still-idle connection.
- Skip the all-pool prune when the exit hook's release walk finds an
  empty context - the request path already released and pruned through
  the Worker, and paid a redundant full channel drain per request.
- Reconcile in getStats() so a quiet pool does not report vanished
  borrowers as live or long-borrowed.
- Document the bounded double-defer after Context::clear() and the
  explicit-release pattern for daemon loops.
@adhikjoshi
adhikjoshi merged commit ce65804 into main Aug 20, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant