Fix pool exhaustion: retry on creation failure, add diagnostics

[claudear]

Summary

• Fix connection creation failure handling: When createConnection() fails in pop(), the method now falls through to the retry loop instead of throwing immediately. This allows the pool to recover from transient connection failures by retrying creation on subsequent attempts.
• Add diagnostic info to error messages: The "Pool is empty" error now includes active and idle connection counts (e.g., Pool 'console' is empty (size 75, active 75, idle 0)), making it easy to distinguish pool capacity issues from connection leaks.
• Chain exceptions: When connection creation failure leads to pool exhaustion, the original creation exception is chained as the previous exception for better debugging.

Fixes: CLOUD-3K4N (Pool 'console' is empty, 3267 events)

Test plan

• Added testPopRetriesAfterConnectionCreationFailure - verifies pool retries after transient creation failure
• Added testPoolEmptyErrorIncludesActiveCount - verifies error message includes active/idle counts
• Added testUseReclainsConnectionOnCallbackException - verifies use() reclaims on callback failure
• All 84 existing tests pass
• PHPStan static analysis passes clean

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Connection pool error messages now include pool size and active connection counts for better diagnostics
  • Root cause exceptions are properly preserved and chained when connection operations fail after retries

• Tests

  • Added test coverage for connection creation retry behavior, error messaging accuracy, and connection reclamation on callback failures

[claudear]

Fix Confidence: 62/100

The 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.

[coderabbitai]

Walkthrough

The 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)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the two main changes: retry on connection creation failure and diagnostic improvements in error messages.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/pool-empty-resilience

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify 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

📥 Commits

Reviewing 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

[coderabbitai]

⚠️ 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 Agents

Verify 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).

[coderabbitai]

⚠️ 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);
                     }

🤖 Prompt for AI Agents

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.