Prevent false leaked-connection reports during reaper maintenance#57370
Open
curi wants to merge 1 commit into
Open
Prevent false leaked-connection reports during reaper maintenance#57370curi wants to merge 1 commit into
curi wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR prevents ActiveRecord::TestCase#check_connection_leaks from falsely reporting leaked connections when ConnectionPool::Reaper is legitimately holding a connection for maintenance by synchronizing the leak scan with pool.reaper_lock.
Changes:
- Wrap the teardown leak scan (
pool.reap+pool.connectionsinspection) inpool.reaper_lockto avoid racing ongoing reaper maintenance. - Add a regression test that simulates a reaper-maintenance hold from a thread named
AR Pool Reaperand assertscheck_connection_leaksdoes not raise.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| activerecord/test/cases/test_case.rb | Synchronizes leak scanning with reaper_lock to avoid misclassifying maintenance-held connections as leaks. |
| activerecord/test/cases/test_case_test.rb | Adds regression coverage for the reaper-maintenance race that previously caused false leak reports. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
curi
force-pushed
the
fix-ar-leak-check-reaper-race
branch
from
10cfbe2 to
74686d9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation / Background
ActiveRecord::TestCase#check_connection_leaksscans each pool during teardown to catch connections left owned by background threads.ConnectionPool::Reapercan legitimately hold a connection underreaper_lockwhile it performs maintenance. The leak check currently inspectspool.connectionswithout taking that lock, so teardown can race the reaper, report a leaked connection owned byAR Pool Reaper, and kill the maintenance thread even though no test leaked anything.Detail
Synchronize the leak scan with
pool.reaper_lockso teardown waits for any in-flight maintenance to finish before classifying in-use connections as leaks.Add a regression test that reproduces the false positive by holding a connection for maintenance from a thread named
AR Pool Reaper. The new test fails againstorigin/mainand passes with this patch.Additional information
This surfaced as an unrelated CI failure while validating #57367, where the reported leaked owner was the pool reaper thread.
Tests
cd activerecord && ARCONN=postgresql bundle exec ruby -Itest test/cases/test_case_test.rb -vcd activerecord && ARCONN=postgresql bundle exec ruby -Itest test/cases/reaper_test.rbcd activerecord && ARCONN=postgresql bundle exec ruby -Itest test/cases/associations/has_and_belongs_to_many_associations_test.rbbundle exec rubocop activerecord/test/cases/test_case.rb activerecord/test/cases/test_case_test.rb