Add database pool max-lifetime recycling#400
Conversation
Add max_lifetime support to the shared pool option contract and implementation. Parse the new option from pool configuration with a disabled-by-default value of -1.0, matching the existing heartbeat disabled convention. Expose a boot-time setter for tests and startup configuration while preserving worker-lifetime safety guidance.
Track the creation time for each pooled database connection generation. Reject expired generations during the normal reuse check so a returned connection is reconnected before user code receives it, even when heartbeat is disabled. Have the database heartbeat discard lifetime-expired idle connections before idle trimming or ping validation, while borrowed connections remain untouched.
Add max_lifetime and env-driven heartbeat timeout examples to the database pool configuration. Document how heartbeat and max lifetime work together for long-running workers, direct database connections, and proxy or pooler setups. Keep DB_POOLED_* settings separate for the pgsql-pooled runtime connection.
Cover enabled and disabled database pool max lifetime behavior. Assert expired returned connections reconnect before reuse, heartbeat discards lifetime-expired idle connections before pinging, and borrowed connections are not recycled by heartbeat. Add focused PoolOption coverage for the new max lifetime option API.
Remove the completed database max-lifetime recycling todo. Add follow-up notes for DB lifetime jitter and Redis pool heartbeat/max-lifetime support so those decisions are tracked separately from this implementation.
|
Warning Review limit reached
More reviews will be available in 16 minutes and 52 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a ChangesMax-lifetime connection generation recycling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/pool/src/PoolOption.php (1)
33-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared
DEFAULT_MAX_LIFETIMEconstant to avoid drift.
-1.0is now a hardcoded default literal and can diverge from other fallbacks over time.♻️ Suggested refactor
class PoolOption implements PoolOptionInterface { + public const DEFAULT_MAX_LIFETIME = -1.0; + public function __construct( @@ - private float $maxLifetime = -1.0, + private float $maxLifetime = self::DEFAULT_MAX_LIFETIME, private array $events = [], ) { }// src/pool/src/Pool.php - maxLifetime: $options['max_lifetime'] ?? -1.0, + maxLifetime: $options['max_lifetime'] ?? PoolOption::DEFAULT_MAX_LIFETIME,As per coding guidelines, "Extract DEFAULT_* class constants for property initial values to prevent drift if defaults change."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pool/src/PoolOption.php` at line 33, In the PoolOption class, extract the hardcoded default value `-1.0` from the `$maxLifetime` property initialization as a class constant named `DEFAULT_MAX_LIFETIME`. Create this constant at the class level and then replace the hardcoded `-1.0` literal in the property declaration with a reference to this constant to ensure consistency across the codebase and prevent future drift if the default value needs to be updated.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/Integration/Database/Sqlite/DbPoolHeartbeatTest.php`:
- Around line 469-488: The helper classes LifetimeExpiredPingTrackingDbPool and
LifetimeExpiredPingTrackingPooledConnection need to be scoped to this specific
test to avoid class redeclaration collisions. Either move these classes into a
test-specific namespace that includes the test class name (such as
DbPoolHeartbeatTest), or rename them with the DbPoolHeartbeatTest prefix (for
example, DbPoolHeartbeatTestLifetimeExpiredPingTrackingDbPool and
DbPoolHeartbeatTestLifetimeExpiredPingTrackingPooledConnection) to clearly
indicate they are test-specific helper classes.
---
Nitpick comments:
In `@src/pool/src/PoolOption.php`:
- Line 33: In the PoolOption class, extract the hardcoded default value `-1.0`
from the `$maxLifetime` property initialization as a class constant named
`DEFAULT_MAX_LIFETIME`. Create this constant at the class level and then replace
the hardcoded `-1.0` literal in the property declaration with a reference to
this constant to ensure consistency across the codebase and prevent future drift
if the default value needs to be updated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c3d2b9bc-050e-4a1a-b885-58d88591af2b
📒 Files selected for processing (11)
src/boost/docs/database.mdsrc/boost/todo.mdsrc/contracts/src/Pool/PoolOptionInterface.phpsrc/database/src/Pool/DbPool.phpsrc/database/src/Pool/PooledConnection.phpsrc/foundation/config/database.phpsrc/pool/src/Pool.phpsrc/pool/src/PoolOption.phptests/Integration/Database/PooledConnectionTest.phptests/Integration/Database/Sqlite/DbPoolHeartbeatTest.phptests/Pool/PoolOptionTest.php
Greptile SummaryThis PR adds a disabled-by-default
Confidence Score: 5/5Safe to merge; the core recycling logic is correct and well-tested across borrowed, idle, and heartbeat paths. The
Important Files Changed
Reviews (2): Last reviewed commit: "fix(database): avoid recycling active po..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if ($connection->isLifetimeExpired($now)) { | ||
| $this->discardHeartbeatConnection($connection); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
The idle-expiry branch two lines below requires $this->currentConnections > $this->option->getMinConnections() before discarding, preserving at least the minimum pool size. The lifetime-expiry branch added here has no such guard. When a cohort of connections all share the same birth time (typical at worker startup), every heartbeat tick can discard all idle connections at once, momentarily dropping the pool to zero even if min_connections > 1. The next in-bound request then bears the reconnection cost. The todo.md entry on jitter acknowledges this, but noting the asymmetry with the idle guard may help reviewers understand why the behaviour can be surprising without changing configuration.
There was a problem hiding this comment.
Thanks. I am intentionally not adding a min_connections guard to the lifetime branch. max_lifetime is an absolute generation cap, so keeping expired connections only to preserve the idle minimum would defeat the setting. The reconnect-wave concern is valid, and src/boost/todo.md tracks jitter as the clean follow-up because that smooths expiry timing without weakening the lifetime guarantee.
Track whether a pooled database wrapper is available for reuse so time-based recycling only runs while the wrapper is idle or being borrowed from the pool again. This prevents both max_lifetime and max_idle_time checks from replacing the underlying connection while a caller still holds the wrapper, preserving active transaction state for repeated getConnection() calls within one borrow window. Keep invalid and closed connection checks outside the reuse gate so genuinely stale wrappers still reconnect immediately. Leave heartbeat predicates unchanged so idle sweeps can continue discarding expired connections before pinging. Update pooled connection tests to model last-use updates on reuse, prove expired lifetime and idle windows do not reconnect active borrows, and keep existing released-wrapper reuse coverage intact.
|
Follow-up on Greptile’s |
1 participant
Summary
Validation
Summary by CodeRabbit
New Features
Configuration
DB_HEARTBEAT,DB_HEARTBEAT_TIMEOUT,DB_MAX_LIFETIME)Documentation