Preserve active tenant leases when enforcing the pool limit - #356
Draft
lucianghinda wants to merge 1 commit into
Draft
lucianghinda wants to merge 1 commit into
lucianghinda wants to merge 1 commit into
Conversation
Tenant pool admission now measures registered tenant configurations and only retires idle pools through bounded lock coordination. Model pool lookup stays behind the creator lock until schema validation succeeds, failed initialization is fenced, and lifecycle tests cover cross-role capacity, stale registry state, and concurrent creation and lookup. Constraint: Rails removes a pool configuration before disconnecting its pool; a disconnect exception can leave an adapter object detached from handler lookup. Rejected: Reap the least-recently-used pool without checking active leases | this can disconnect a tenant connection in use. Confidence: high Scope-risk: moderate Directive: Recheck the private Rails pool-manager and PoolConfig seams against the minimum, current, and edge Rails appraisals before changing them. Tested: Rails 8.1.4 bin/ci; full unit and integration suites on Rails 8.1.2.1 and pinned Rails 8.2.0.alpha; seeded tenant pool lifecycle and tenant tests; repeated pending-validation races; actionlint, zizmor, RuboCop. Not-tested: Physical adapter cleanup after Rails has detached a pool and its disconnect operation raises.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Tenant destruction leaves non-writing role pools registered after dropping the tenant database.
Review effort: Balanced
Findings: 1
What changed in this PR
Adds concurrency-safe tenant pool capacity enforcement while preserving active leases and rejecting admission when all pools are occupied.
Changes:
- Introduces tenant-aware pool lifecycle, readiness, retirement, and capacity handling.
- Validates pool limits and documents locking and compatibility behavior.
- Adds extensive lifecycle, concurrency, and integration coverage.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
Appraisals |
Adds minimum Rails appraisal. |
gemfiles/rails_8_1_minimum.gemfile |
Pins Rails 8.1.2.1 dependencies. |
GUIDE.md |
Documents capacity and locking semantics. |
lib/active_record/tenanted.rb |
Defines the capacity error. |
lib/active_record/tenanted/database_configurations/base_config.rb |
Validates pool limits. |
lib/active_record/tenanted/lru.rb |
Adds snapshot and identity operations. |
lib/active_record/tenanted/tenant.rb |
Coordinates pool admission and retirement. |
lib/active_record/tenanted/tenant_connection_pool.rb |
Implements tenant pool lifecycle safeguards. |
lib/active_record/tenanted/tenant_pool_config.rb |
Installs tenant-aware pools. |
test/integration/test/pool_safety_test.rb |
Covers end-to-end capacity safety. |
test/unit/database_configurations_test.rb |
Tests limit validation. |
test/unit/lru_test.rb |
Tests new LRU operations. |
test/unit/tenant_connection_pool_test.rb |
Tests concurrency and retirement behavior. |
test/unit/tenant_pool_config_test.rb |
Tests pool factory behavior. |
test/unit/tenant_test.rb |
Expands tenant lifecycle and capacity tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+282
to
+285
| if raw_tenant_connection_pool | ||
| pool = raw_tenant_connection_pool | ||
| POOL_REGISTRY_LOCK.synchronize do | ||
| tenanted_connection_pools.delete_if_same([ current_tenant, current_role ], pool) |
lucianghinda
marked this pull request as draft
October 3, 2026 13:03
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.
Busy tenant connection pools could be evicted when new tenants pushed the process over its configured pool limit. This change enforces a hard limit across registered tenant pools and retires only idle pools after identity and checkout checks, so active tenant leases remain usable while admission fails clearly when all slots are occupied.
Pool creation and schema validation now coordinate through bounded locking. Pools remain unavailable to other model lookups while their creator completes validation; failures remove ready registry entries and fail closed. Stale LRU identities are pruned against Rails’ raw handler state. The guide documents the limit, lock ordering, and Rails compatibility seams, and a minimum-version appraisal covers Rails 8.1.2.1.
Validation passed on Ruby 4.0.6 with Rails 8.1.4
bin/ci, plus full unit and integration suites on minimum Rails 8.1.2.1 and pinned Rails 8.2.0.alpha (07cfbd77d4f3c4312b4651ca74f9ee9e28dfde5f). Each suite run reported 1,815 unit runs/3,727 assertions and nine integration configurations of 36 runs/98 assertions, with zero failures, errors, or skips. Seeded lifecycle tests passed at seeds 412041 and 412044; the full tenant test passed at seeds 412042 and 412043; the pending-validation race passed repeatedly across all nine scenarios. Independent specification, correctness/test-quality, security, and performance reviews approved the implementation.A local comparison with the same runtime, ten pre-migrated synthetic databases, normal schema validation and cap 2 measured 3,000 samples per scenario with zero failures. Hot lookup p50/p95 was 0.042/0.053 ms versus baseline 0.044/0.053 ms; idle tenant cycling was 0.991/1.341 ms versus 1.162/1.575 ms. Both ended with two registered pools. A separate four-worker diagnostic accounted for 200 operations: 126 successes, 74 typed capacity rejections, and no unexpected failures. The baseline accepted all 200 operations. These rejections are the intended all-pools-busy boundary; the shorter lock waits are not a throughput gain or an availability guarantee. These are descriptive local measurements, not a latency guarantee.
Rails removes the handler’s pool configuration before disconnecting it. If disconnect raises, the detached pool is fenced from future model lookups, but physical adapter cleanup after that exception is outside what these tests establish.