Add database pool heartbeat support#399
Merged
Merged
Conversation
Add a heartbeat_timeout pool option alongside the existing heartbeat interval. Expose the timeout through the pool option contract and concrete PoolOption class so database pools can use a dedicated validation timeout without reusing driver query or read timeouts. Keep the default at 1 second and preserve existing pool behavior when the option is not configured.
Replace the implicit stale timestamp sentinel with an explicit invalid flag on base pooled connections. Check invalid state before idle-time freshness so a recently released wrapper cannot mask a failed connection. Keep reconnect implementations responsible for clearing invalid state when they successfully rebuild the underlying connection.
Mark Redis connections invalid after retry reconnect failures instead of zeroing lastUseTime. Clear the invalid flag when standalone or cluster Redis reconnects successfully. This prevents a fresh release timestamp from making a failed pooled Redis connection look reusable.
Reset database connection error counts as part of resetForPool(). Pooled database connections are reused across requests, so execution error counts must belong to the current borrow window instead of accumulating for the worker lifetime. PooledConnection snapshots the count before reset in a later commit so the stale-connection threshold still works.
Add the PooledConnection support needed by database pool heartbeats. Track invalid wrappers explicitly, split idle eviction time from checkout freshness, and add a raw PDO ping path that uses only already-open PDO handles. Run pings in a static child coroutine with a buffered result channel so timeout handling cannot requeue discarded wrappers or retain the full connection wrapper graph. Snapshot database error counts before resetForPool() so the stale threshold is still honored while resetting counts for the next borrow window.
Start one heartbeat timer per worker-local database pool when heartbeat is configured above zero. Sweep only idle channel entries, keep borrowed connections untouched, evict expired idle extras above the minimum pool size, and validate minimum idle connections through the PooledConnection raw ping path. Discard failed or timed-out wrappers cleanly so the next borrow opens a fresh connection, and clear heartbeat timers when pools are flushed or destroyed.
Add a Laravel-style configuration section for database connection pools. Add heartbeat_timeout to the configured database pool blocks so applications can tune heartbeat validation separately from connection and wait timeouts. Leave Redis pool blocks unchanged because this implementation only adds database pool heartbeat behavior.
Document database pool heartbeat_timeout and clarify how heartbeat validation works. Explain that heartbeat pings keep minimum idle connections warm, bypass query instrumentation, and only evict idle extras above the minimum pool size. Remove the completed heartbeat todo and keep the follow-up max-lifetime recycling item for a separate decision.
Add integration coverage for database pool heartbeat behavior. Cover disabled timers, timer cleanup, minimum connection validation, expired idle extra eviction, lazy PDO preservation, instrumentation bypass, borrowed connection isolation, failed ping discard, invalid wrapper discard, and timeout handling without late requeue. Use file-backed SQLite and direct heartbeat invocation for deterministic parallel-safe coverage.
Add regression coverage for explicit invalid pooled connection state. Verify database wrappers reconnect even with a fresh release timestamp, snapshot error counts before reset, and reset smaller error counts for the next borrow window. Verify Redis retry failures mark connections invalid, reconnect clears invalid state, and fresh release timestamps do not mask invalid Redis wrappers.
|
Warning Review limit reached
More reviews will be available in 37 minutes and 18 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 (1)
📝 WalkthroughWalkthroughAdds a ChangesDB Pool Heartbeat and Connection Validity
Coroutine Cancellation Support
Sequence Diagram(s)sequenceDiagram
participant Timer
participant DbPool
participant PooledConnection
participant Channel
participant PDO
rect rgba(135, 206, 235, 0.5)
Note over Timer,DbPool: Heartbeat sweep (periodic tick)
Timer-->>DbPool: tick callback
DbPool->>DbPool: heartbeat()
loop each channel connection
DbPool->>PooledConnection: isIdleExpired()
alt idle-expired
DbPool->>DbPool: discardHeartbeatConnection()
else valid candidate
DbPool->>PooledConnection: ping(heartbeatTimeout)
PooledConnection->>PooledConnection: getOpenPdos()
PooledConnection->>Channel: new Channel(count)
loop each PDO
PooledConnection->>Channel: go { SELECT 1 }
end
Channel-->>PooledConnection: collect results
alt ping success and generation match
DbPool->>DbPool: release back to pool
else ping failed or post-flush
DbPool->>DbPool: discardHeartbeatConnection()
end
end
end
end
rect rgba(144, 238, 144, 0.5)
Note over DbPool: flushAll() or destruct
DbPool->>Timer: clearHeartbeat()
DbPool->>DbPool: parent::flushAll()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes 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 |
Greptile SummaryThis PR wires up the long-planned opt-in heartbeat feature for database connection pools: a per-pool timer fires at the configured interval, inspects every idle connection, evicts expired extras above
Confidence Score: 5/5Safe to merge — the heartbeat feature is opt-in (disabled by default with All identified edge cases are handled: double-decrement is impossible because No files require special attention. The most complex logic is in Important Files Changed
Reviews (3): Last reviewed commit: "test(database): harden heartbeat timeout..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/Integration/Database/Sqlite/DbPoolHeartbeatTest.php (1)
323-382: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScope helper classes to a test-specific namespace.
Lines 323-382 add reusable helper classes in a shared namespace; this increases collision risk across the suite.
As per coding guidelines,
tests/**/*Test.php: “Use test-specific namespaces including the test class name for helper classes to avoid Cannot redeclare class errors”.♻️ Suggested direction
- class InspectableHeartbeatDbPool extends DbPool + class DbPoolHeartbeatTestInspectableHeartbeatDbPool extends DbPool - class FailingHeartbeatDbPool extends InspectableHeartbeatDbPool + class DbPoolHeartbeatTestFailingHeartbeatDbPool extends DbPoolHeartbeatTestInspectableHeartbeatDbPool - class SlowHeartbeatDbPool extends InspectableHeartbeatDbPool + class DbPoolHeartbeatTestSlowHeartbeatDbPool extends DbPoolHeartbeatTestInspectableHeartbeatDbPoolAlso update
createPool()default$poolClassand call sites (Line 226,Line 267) to the renamed classes (or move these helpers into a namespace that includesDbPoolHeartbeatTest).🤖 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 `@tests/Integration/Database/Sqlite/DbPoolHeartbeatTest.php` around lines 323 - 382, The helper classes (InspectableHeartbeatDbPool, FailingHeartbeatDbPool, FailingHeartbeatPooledConnection, SlowHeartbeatDbPool, SlowHeartbeatPooledConnection, and SlowHeartbeatPdo) are defined in a shared namespace and risk colliding with other tests in the suite. Move these classes into a test-specific namespace that includes DbPoolHeartbeatTest to comply with coding guidelines. Update all references to these classes throughout the test file, including the default $poolClass parameter in the createPool() method and any call sites that instantiate or reference these helper classes, to use the new namespaced class names.Source: Coding guidelines
tests/Redis/RedisConnectionTest.php (1)
2112-2112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd explicit
: voidreturn types to the new test methods.Lines 2112, 2170, and 2211 introduce test methods without return types.
As per coding guidelines,
tests/**/*.php: “Add : void return types to test methods”.♻️ Suggested fix
- public function testRetryFailureMarksConnectionInvalid() + public function testRetryFailureMarksConnectionInvalid(): void - public function testReconnectClearsInvalidState() + public function testReconnectClearsInvalidState(): void - public function testInvalidStateIsNotMaskedByFreshReleaseTime() + public function testInvalidStateIsNotMaskedByFreshReleaseTime(): voidAlso applies to: 2170-2170, 2211-2211
🤖 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 `@tests/Redis/RedisConnectionTest.php` at line 2112, Add explicit `: void` return type declarations to the three test methods in RedisConnectionTest.php: testRetryFailureMarksConnectionInvalid at line 2112, and the two additional test methods at lines 2170 and 2211. Each test method signature should be updated from `public function methodName()` to `public function methodName(): void` to comply with the coding guidelines requiring return types on all test methods.Source: Coding guidelines
src/database/src/Pool/DbPool.php (1)
74-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the required title docblock for
__destruct().As per coding guidelines, “Add Laravel-style title docblocks to methods” and “method docblocks (title only) are always added.”
Proposed fix
+ /** + * Clear the heartbeat timer. + */ public function __destruct() { $this->clearHeartbeat();🤖 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/database/src/Pool/DbPool.php` around lines 74 - 76, The `__destruct()` method in the DbPool class is missing a required title docblock. Add a Laravel-style title docblock above the `__destruct()` method definition that briefly describes what the method does, such as describing its cleanup responsibility for heartbeat functionality.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 `@src/database/src/Pool/DbPool.php`:
- Around line 131-134: The clearHeartbeat() call in flushAll() does not prevent
in-flight heartbeat coroutines from requeueing connections back into the pool
after the flush completes. Implement a generation counter that increments each
time flushAll() is called, and capture the current generation value before
initiating any heartbeat ping operation. After a heartbeat ping completes
successfully but before calling release() to requeue the connection, check if
the generation has changed since the ping started. If the generation has changed
(indicating a concurrent flushAll() occurred), discard the connection instead of
calling release() to prevent stale connections from being requeued into the
flushed pool.
- Around line 146-149: The arrow closure in the heartbeat timer callback is
ignoring the `$isClosing` parameter provided by Timer::tick() and strongly
capturing `$this`, which prevents proper pool cleanup during shutdown. Modify
the closure to accept the `$isClosing` boolean parameter and return early when
it is true to stop the timer immediately during shutdown. Additionally, use a
weak reference to `$this` instead of a strong capture to avoid keeping the pool
alive unnecessarily and to ensure `__destruct()` can be called when the pool is
dropped elsewhere. Update the closure definition in the tick method call to
implement these shutdown-aware behaviors.
---
Nitpick comments:
In `@src/database/src/Pool/DbPool.php`:
- Around line 74-76: The `__destruct()` method in the DbPool class is missing a
required title docblock. Add a Laravel-style title docblock above the
`__destruct()` method definition that briefly describes what the method does,
such as describing its cleanup responsibility for heartbeat functionality.
In `@tests/Integration/Database/Sqlite/DbPoolHeartbeatTest.php`:
- Around line 323-382: The helper classes (InspectableHeartbeatDbPool,
FailingHeartbeatDbPool, FailingHeartbeatPooledConnection, SlowHeartbeatDbPool,
SlowHeartbeatPooledConnection, and SlowHeartbeatPdo) are defined in a shared
namespace and risk colliding with other tests in the suite. Move these classes
into a test-specific namespace that includes DbPoolHeartbeatTest to comply with
coding guidelines. Update all references to these classes throughout the test
file, including the default $poolClass parameter in the createPool() method and
any call sites that instantiate or reference these helper classes, to use the
new namespaced class names.
In `@tests/Redis/RedisConnectionTest.php`:
- Line 2112: Add explicit `: void` return type declarations to the three test
methods in RedisConnectionTest.php: testRetryFailureMarksConnectionInvalid at
line 2112, and the two additional test methods at lines 2170 and 2211. Each test
method signature should be updated from `public function methodName()` to
`public function methodName(): void` to comply with the coding guidelines
requiring return types on all test methods.
🪄 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: 9406202d-5cef-47a8-82bb-d697fb658a74
📒 Files selected for processing (16)
src/boost/docs/database.mdsrc/boost/todo.mdsrc/contracts/src/Pool/PoolOptionInterface.phpsrc/database/src/Connection.phpsrc/database/src/Pool/DbPool.phpsrc/database/src/Pool/PooledConnection.phpsrc/foundation/config/database.phpsrc/pool/src/Connection.phpsrc/pool/src/Pool.phpsrc/pool/src/PoolOption.phpsrc/redis/src/PhpRedisClusterConnection.phpsrc/redis/src/PhpRedisConnection.phpsrc/redis/src/RedisConnection.phptests/Integration/Database/PooledConnectionTest.phptests/Integration/Database/Sqlite/DbPoolHeartbeatTest.phptests/Redis/RedisConnectionTest.php
Remove the dev-only swoole/ide-helper dependency from the components repository because the package is behind the current Swoole runtime signatures and can mislead static analysis and IDEs. Also remove the Boost installation docs sentence that said the application skeleton includes the helper, since that is no longer true.
This reverts commit 9d51e76.
Add a low-level cancelById wrapper to the engine coroutine contract and Swoole-backed implementation so infrastructure code can cancel a specific coroutine without going through the high-level coroutine facade. Narrow the engine exists signature to match Swoole's required coroutine id parameter and add regression coverage proving cancelById throws cancellation into the target coroutine and prevents late work. Keep the direct Swoole 6.2 cancel call and use a local PHPStan ignore for the stale bundled Swoole stub until that upstream stub exposes the throw_exception parameter.
Cancel timed-out heartbeat ping coroutines with exception delivery so the child coroutine does not keep holding PDO references until the underlying socket operation returns. Track heartbeat generations on the pool so a successful in-flight heartbeat cannot requeue a connection after flush or shutdown has already cleared the timer. Make heartbeat cleanup logging best-effort while keeping the connection count synchronized with the discard decision before any yielding close work can run. Add regression coverage for timeout cancellation, flush-during-ping requeue prevention, and logger failures during heartbeat discard cleanup.
Add missing void return types to Redis connection test methods touched by the PR review cleanup. This is a mechanical test-only typing cleanup with no Redis behavior changes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 283-288: The assertions checking elapsed time and coroutine
existence are too strict and fail intermittently due to scheduler timing
variations. In the test method containing these assertions, increase the elapsed
time threshold in the assertLessThan call (the 0.05 value is too tight), and
instead of directly asserting that Coroutine::exists returns false immediately,
implement an eventual check that retries the assertion multiple times within a
reasonable window (such as checking in a loop for up to 1 second) to account for
scheduler jitter before considering the coroutine properly cleaned up.
🪄 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: 5dc2be5c-e8a3-440a-a910-c9bf3475872f
📒 Files selected for processing (7)
src/contracts/src/Engine/CoroutineInterface.phpsrc/database/src/Pool/DbPool.phpsrc/database/src/Pool/PooledConnection.phpsrc/engine/src/Coroutine.phptests/Engine/CoroutineTest.phptests/Integration/Database/Sqlite/DbPoolHeartbeatTest.phptests/Redis/RedisConnectionTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- src/database/src/Pool/DbPool.php
- src/database/src/Pool/PooledConnection.php
Increase the fake slow heartbeat query delay so the timeout regression test has a clear timing gap between the correct cancellation path and the broken no-cancel path. Keep the elapsed assertion strict enough to fail if timeout cancellation stops working, while avoiding CI noise from a threshold that matched the old fake delay too closely. Add a short bounded cleanup wait before asserting the timed-out child coroutine is gone, keeping the wait below the fake query delay so broken cancellation still fails.
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
1 participant
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.
Summary
lastUseTime = 0.0invalidation with explicit invalid state for database and Redis pooled connectionsheartbeat_timeoutand remove the completed Boost todoTesting
./vendor/bin/phpunit tests/Integration/Database/Sqlite/DbPoolHeartbeatTest.php./vendor/bin/phpunit tests/Integration/Database/PooledConnectionTest.php./vendor/bin/phpunit tests/Redis/RedisConnectionTest.phpcomposer fixSummary by CodeRabbit
heartbeat_timeoutsupport for database connection pools.heartbeat_timeoutand heartbeat behavior.