Skip to content

fix(streams): transaction probe must not lease an AR connection - #395

Merged
mhenrixon merged 1 commit into
mainfrom
fix/transaction-probe-connection-leak
Aug 4, 2026
Merged

fix(streams): transaction probe must not lease an AR connection#395
mhenrixon merged 1 commit into
mainfrom
fix/transaction-probe-connection-leak

Conversation

@mhenrixon

@mhenrixon mhenrixon commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

Stream#current_open_transaction probes for an open transaction via ActiveRecord::Base.connection. On Rails 7.2+ that call takes a sticky, executor-scoped lease, which leaks pool connections in two ways:

  1. Non-executor threads (the Coalescer's Concurrent::ScheduledTask flush thread, any app worker thread not wrapped by the Rails executor): the lease is never released — one AR pool connection pinned per thread for its lifetime.
  2. Inside connection_pool.with_connection: the sticky flag defeats the block-exit release, so the caller's connection leaks when its thread dies. The pool then reports it dead-but-in-use (statdead: N), and those slots are gone.

Both variants only fire on the durable path — ephemeral broadcasts return broadcast_ephemeral(wrapped) unless use_durable before the probe. That's why flipping streams_default_broadcast_mode = :durable surfaced it downstream: Zazu's bulk-payment fan-out spec sizes its worker threads to the exact AR pool, and one run left the pool at {connections: 6, busy: 1, dead: 5, waiting: 6} — 5 dead leases from the workers' with_connection blocks plus one live lease pinned to the coalescer flush thread. Deterministic ActiveRecord::ConnectionTimeoutError on every run.

Production impact isn't limited to specs: every coalesced/durable broadcast flushed on the coalescer thread pins an AR connection in web/worker processes.

Fix

Probe connection_pool.active_connection? instead — returns the calling thread/fiber's existing lease or nil, never a checkout. Semantics are unchanged: a transaction is per-lease, so a thread holding no connection has no open transaction to defer on. The old code's fresh checkout always answered nil in that situation anyway — at the price of the leak.

Tests

spec/pgbus/streams/transaction_probe_spec.rb (real AR via the dummy app):

  • probe from a lease-less thread leases nothing (red on main: active_connection? returns the leaked lease)
  • probe from a lease-less thread returns nil
  • broadcasting inside with_connection doesn't defeat its release (red on main: dead-owner in-use connection)
  • probe still finds the open transaction when the caller holds one

Existing AR stubs in streams_spec.rb updated to the new connection_pool.active_connection? contract. Full streams suite: 383 green. (spec/rubocop/cop/pgbus/no_ruby_timeout_spec.rb fails to load on a clean main checkout locally — pre-existing, unrelated.)

Summary by CodeRabbit

  • Bug Fixes

    • Improved transaction detection during broadcasts to avoid unnecessarily acquiring or leaking database connections.
    • Preserved correct connection cleanup on worker threads.
    • Broadcasts now proceed immediately when no transaction is open.
  • Tests

    • Added regression coverage for connection handling and open transaction detection.
    • Updated transaction test coverage to reflect active connection behavior.

Stream#current_open_transaction probed via ActiveRecord::Base.connection,
which on Rails 7.2+ takes a sticky, executor-scoped lease. Two leaks:

- On a non-executor thread (the Coalescer's flush thread, app worker
  threads) the lease is never released — one AR pool connection pinned
  per thread for its lifetime.
- Inside connection_pool.with_connection the sticky flag defeats the
  block-exit release, so the CALLER's connection leaks when its thread
  dies (pool reports it dead-but-in-use).

Both variants only fire on the durable path — ephemeral broadcasts
early-return before the probe — which is why flipping
streams_default_broadcast_mode to :durable surfaced it: a downstream
fan-out spec with an exactly-sized pool exhausted deterministically
(5 workers + 1 coalescer flush = whole pool dead in one run).

Probe connection_pool.active_connection? instead: the existing lease or
nil, never a checkout. Semantics unchanged — a transaction is per-lease,
so a thread holding no connection has no open transaction to defer on;
the old fresh checkout always answered nil anyway, at the price of the
leak.
@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 36608e9b-0e21-4f6e-9eb8-81a3fdd434d0

📥 Commits

Reviewing files that changed from the base of the PR and between b3b5c61 and 99b1691.

📒 Files selected for processing (3)
  • lib/pgbus/streams.rb
  • spec/pgbus/streams/transaction_probe_spec.rb
  • spec/pgbus/streams_spec.rb

📝 Walkthrough

Walkthrough

Stream#current_open_transaction now inspects only already-leased ActiveRecord connections. Tests cover connection cleanup, no-lease behavior, immediate broadcasts, and open transaction detection.

Changes

ActiveRecord transaction probing

Layer / File(s) Summary
Use active connection leases for transaction probing
lib/pgbus/streams.rb
current_open_transaction uses active_connection? and avoids acquiring an unused connection.
Validate connection lifecycle and transaction detection
spec/pgbus/streams/transaction_probe_spec.rb, spec/pgbus/streams_spec.rb
Tests cover unleased threads, worker-thread cleanup, immediate broadcasts, and existing open transactions. Test doubles now use the connection pool interface.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • mhenrixon/pgbus#81: Introduced the transaction helper refined by this change.
  • mhenrixon/pgbus#158: Addresses related ActiveRecord connection lifecycle behavior in a different code path.

Suggested labels: bug, testing, connections

Poem

A rabbit checks the leased connection,
No checkout springs from inspection.
Open transactions still appear,
Worker threads leave their leases clear.
Tests hop softly through the flow.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: preventing the transaction probe from leasing an ActiveRecord connection.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/transaction-probe-connection-leak

Comment @coderabbitai help to get the list of available commands.

@mhenrixon mhenrixon self-assigned this Aug 4, 2026
@mhenrixon mhenrixon added the bug Something isn't working label Aug 4, 2026
@mhenrixon
mhenrixon merged commit c02fa6f into main Aug 4, 2026
13 checks passed
@mhenrixon
mhenrixon deleted the fix/transaction-probe-connection-leak branch August 4, 2026 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant