fix(pools): recover failed pool connections#75
Merged
Conversation
Pool::use() now tracks borrow failure and release(failed) recovers the connection via its reset()/reconnect() hooks before reclaiming, or destroys it when recovery is unavailable or fails. A wire-desynced or transaction- poisoned connection can no longer be handed to the next borrower — the root of the DAT-1904 cross-document read bleed and the db#895/#896 write-outage class. Includes the recover-and-reuse happy-path test and a telemetry lock bump to 0.4.5 (observable-gauge multi-observer, required by the gauge tests). Ported from utopia-php/pools#33 (standalone repo is now a mirror). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Greptile SummaryThis PR updates pool cleanup so failed borrows do not return poisoned connections. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (3): Last reviewed commit: "style(pools): make test anonymous class ..." | Re-trigger Greptile |
…oy cleanup fails When recovery fails and destroy() itself throws (e.g. the adapter's synchronized lock fails before the active entry is removed), release() previously swallowed the error with the connection still tracked in $active and counted in connectionsCreated — permanently leaking a pool slot. forget() now untracks the connection as a last resort, taking the lock when possible and falling back to direct cleanup when the lock is the thing that failed. Also drops two always-true assertIsResource assertions and applies rector rules (readonly anonymous classes, never returns) flagged by the monorepo checks. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The pools CI job fails at bin/monorepo check on Rector's dry-run: ReadOnlyAnonymousClassRector requires the anonymous Stringable test resource to be declared readonly since its only property is never mutated after construction. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Summary
Port of utopia-php/pools#33 to the canonical monorepo (the standalone pools repo is now a mirror with a redirect workflow, so the fix must land here to ship).
Root fix for the DAT-1904 production incident (a by-id read on nyc3 prod returned a different database's document) and the same defect family as the nyc3 write outage (utopia-php/database#895/#896, transaction-counter desync):
Pool::use()tracks whether the borrow callback threw and callsrelease($connection, failed: true)in itsfinally.release(failed)runsrecover(): if the resource exposesreset()/reconnect()it is recovered then reclaimed (for the DB adapters this swaps in a fresh PDO — discarding any wire-desynced socket / stale result frame — and zeroes the leakedinTransactioncounter); if recovery is unavailable, returns false, or throws, the connection is destroyed instead of reclaimed.Also includes:
reconnect()is reused, not destroyed).utopia-php/telemetrylock bump 0.4.0 → 0.4.5 (within^0.4): the observable-gauge tests read$gauge->callbacks[](multi-observer, 0.4.1+); the old lock madetestPoolTelemetry/testMultiplePoolsShareGauges…fail.Testing
Full pools suite green locally: 49 tests, 168 assertions (Stack adapter; Swoole adapter shares the scopes).
Ship path
Merge here → release → cloud consumes via
composer update(pools resolves through server-ce) → DAT-1904 guarded fleet-wide. The cloud-side containment (appwrite-labs/cloud#4827 Store identity guard) stays as defense-in-depth.Supersedes utopia-php/pools#33.
🤖 Generated with Claude Code