Fix pool exhaustion: retry on creation failure, add diagnostics - #29
Conversation
Sorry, something went wrong.
- When connection creation fails in pop(), fall through to retry loop instead of throwing immediately, allowing the pool to recover from transient connection failures - Include active and idle connection counts in error messages to help diagnose pool exhaustion vs connection leak issues - Chain the original exception as previous when connection creation failure leads to pool exhaustion - Add tests for retry-after-creation-failure and diagnostic error messages Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fix Confidence: 62/100The fix addresses two real code issues: (1) immediate throw on connection creation failure preventing retries, and (2) lack of diagnostic info in error messages. However, the root cause of the Sentry issue (3267 events of pool exhaustion) is likely in the consumer code (appwrite's realtime server holding connections too long or leaking them), not solely in this library. The improved diagnostics will help identify the real cause, and the retry-on-creation-failure fix prevents one class of pool exhaustion. But this may not fully eliminate the Sentry errors if the underlying cause is connection leaks or insufficient pool capacity in the calling application. |
Sorry, something went wrong.
WalkthroughThe changes enhance exception handling in the Pool class to improve error reporting and root cause traceability. A new $lastException variable captures the original exception when connection creation fails, enabling exception chaining through wrapped exceptions. Error messages now include pool metadata such as name, size, and active/idle connection counts. Three new test methods verify retry behavior after creation failure, error message content during pool exhaustion, and proper connection reclamation when user callbacks throw exceptions. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agentsVerify each finding against the current code and only fix it if needed. Inline comments: In `@src/Pools/Pool.php`: - Around line 283-285: The exception chain is losing the original cause because createConnection() throws a fresh \Exception without passing the caught exception as the previous; update createConnection() to rethrow its error with the original exception as the third parameter (previous) so callers (and $lastException in Pool::$lastException assignments) retain the root cause; alternatively, where Pool sets $lastException = $e after catching the createConnection() wrapper, set $lastException to the original exception (or use the wrapper's getPrevious()) so the final thrown exception preserves the original type and stack; check the createConnection() method and the Pool code around the $lastException assignments referenced (including the similar spots at the other catch blocks). - Around line 293-296: When failing to report pool exhaustion in Pool:: (around the throw at the end of the acquire path), detect if $lastException !== null and there were zero active/idle connections and instead surface a distinct terminal error about connection creation failure (include $lastException details) rather than the generic "Pool ... is empty" message; update the logic that currently builds $message and throws new Exception($message, 0, $lastException) so that when $lastException is set you either rethrow $lastException or throw a new Exception with a clear creation-failure message (and attach $lastException as the previous exception) to make the failure cause explicit.
ℹ️ Review info ⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 323d9e3a-1d82-41be-a76f-8ea2a8b0e29f
📥 CommitsReviewing files that changed from the base of the PR and between 74de7c5 and 8bb77d4.
📒 Files selected for processing (2)- src/Pools/Pool.php
- tests/Pools/Scopes/PoolTestScope.php
Sorry, something went wrong.
| // Don't throw immediately - fall through to try getting | ||
| // an existing connection from the pool | ||
| $lastException = $e; |
There was a problem hiding this comment.
⚠️ Potential issue | 🟡 Minor
The chained exception still drops the original cause.
$lastException here is only the wrapper returned by createConnection(). That method currently throws a fresh \Exception without $e as previous, so the final chain still loses the original exception type and stack.
Preserve the root cause from createConnection() } catch (\Exception $e) {
if ($attempts >= $this->getReconnectAttempts()) {
- throw new \Exception('Failed to create connection: ' . $e->getMessage());
+ throw new \Exception('Failed to create connection: ' . $e->getMessage(), 0, $e);
}
sleep($this->getReconnectSleep());
}Also applies to: 293-296, 311-313
🤖 Prompt for AI AgentsVerify each finding against the current code and only fix it if needed. In `@src/Pools/Pool.php` around lines 283 - 285, The exception chain is losing the original cause because createConnection() throws a fresh \Exception without passing the caught exception as the previous; update createConnection() to rethrow its error with the original exception as the third parameter (previous) so callers (and $lastException in Pool::$lastException assignments) retain the root cause; alternatively, where Pool sets $lastException = $e after catching the createConnection() wrapper, set $lastException to the original exception (or use the wrapper's getPrevious()) so the final thrown exception preserves the original type and stack; check the createConnection() method and the Pool code around the $lastException assignments referenced (including the similar spots at the other catch blocks).
Sorry, something went wrong.
| $activeCount = count($this->active); | ||
| $idleCount = $this->pool->count(); | ||
| $message = "Pool '{$this->name}' is empty (size {$this->size}, active {$activeCount}, idle {$idleCount})"; | ||
| throw new Exception($message, 0, $lastException); |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
Don't report repeated creation failures as pool exhaustion.
If $init keeps failing and the pool never creates a connection, Lines 293-296 still end with Pool ... is empty (active 0, idle 0). That sends callers toward leak/capacity debugging even though the real failure is connection establishment. Please keep a distinct terminal message when $lastException !== null and no connection was ever obtained.
Possible fix if ($connection === false || $connection === null) {
if ($attempts >= $this->getRetryAttempts()) {
$activeCount = count($this->active);
$idleCount = $this->pool->count();
- $message = "Pool '{$this->name}' is empty (size {$this->size}, active {$activeCount}, idle {$idleCount})";
+ $message = ($lastException !== null && $activeCount === 0 && $idleCount === 0)
+ ? "Pool '{$this->name}' failed to create a connection (size {$this->size}, active {$activeCount}, idle {$idleCount})"
+ : "Pool '{$this->name}' is empty (size {$this->size}, active {$activeCount}, idle {$idleCount})";
throw new Exception($message, 0, $lastException);
}Verify each finding against the current code and only fix it if needed. In `@src/Pools/Pool.php` around lines 293 - 296, When failing to report pool exhaustion in Pool:: (around the throw at the end of the acquire path), detect if $lastException !== null and there were zero active/idle connections and instead surface a distinct terminal error about connection creation failure (include $lastException details) rather than the generic "Pool ... is empty" message; update the logic that currently builds $message and throws new Exception($message, 0, $lastException) so that when $lastException is set you either rethrow $lastException or throw a new Exception with a clear creation-failure message (and attach $lastException as the previous exception) to make the failure cause explicit.
Sorry, something went wrong.
Update pools dependency to include the fix for pool empty resilience (utopia-php/pools#29). The upstream changes add retry on connection creation failure, diagnostic info (active/idle counts) in error messages, and exception chaining. Added tests to verify retry behavior and diagnostic error messages through the cache adapter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Bump utopia-php/pools from 0.8.x to dev-main (pending 1.x release) which includes the fix from utopia-php/pools#29: retry on connection creation failure instead of immediately throwing, and improved diagnostic error messages with active/idle counts. Also bump utopia-php/cache from 0.13.x to 1.x for pools 1.x compatibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Pull in the fix from utopia-php/pools#29 which improves pool exhaustion resilience by retrying on connection creation failure and adds diagnostic info (active/idle counts) to error messages. This addresses CLOUD-3K4N. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Fixes: CLOUD-3K4N (Pool 'console' is empty, 3267 events)
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests