fix(database): drop poisoned pooled connection on any lost-connection error (DAT-1904)#921
fix(database): drop poisoned pooled connection on any lost-connection error (DAT-1904)#921abnegate wants to merge 1 commit into
Conversation
… error (DAT-1904) A pooled DB connection is checked out per operation from a worker-shared, per-host pool. The reconnect wrapper in PDOStatement::__call() only self-healed a lost connection for execute() outside a transaction. Every other lost-connection error (fetch, fetchAll, closeCursor, or execute inside a transaction) was rethrown while the underlying connection was left untouched and reclaimed to the pool as-is. A connection lost mid-result is wire-desynced: a partial or stale result frame can remain buffered on the socket. Returning that connection to the pool poisons it. The next coroutine to pop it runs its own query and reads the leftover frame instead, returning a different row (cross-document, and cross-tenant on shared shards) and corrupting writes (the "no active transaction" class). The fetch phase must reconnect because it cannot be retried in place: re-running fetch on a fresh statement would read no rows, so the only safe recovery is to discard the desynced socket and let the caller replay. This change extends the recovery so that on any detected lost-connection error outside the execute-outside-transaction fast path, PDOStatement::__call() calls PDO::reconnect() to drop the socket before rethrowing. The existing transparent retry is unchanged. Non-connection errors still rethrow untouched, so a healthy connection is never thrashed on ordinary query errors. Callers and Adapter::withTransaction still see the rethrow and roll back or replay on the now fresh connection. Adds a regression test proving reconnect() is invoked before the exception propagates for a fetch-phase lost connection and for execute inside a transaction, and is not invoked for a non-connection error. Refs: #895, #896, appwrite-labs/cloud#4827 (downstream containment), utopia-php/pools#33 (pool-layer defense in depth) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughPDOStatement's lost-connection handling is refined so retry is attempted only for execute() outside transactions; other recoverable cases (fetch/closeCursor, or execute() inside a transaction) now explicitly reconnect before rethrowing to discard poisoned connections. Corresponding unit tests were updated to match this behavior. ChangesPDOStatement Reconnect Logic
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant PDOStatement
participant PDO
Caller->>PDOStatement: __call(method, args)
PDOStatement->>PDO: execute()/fetch()
PDO-->>PDOStatement: throws error
alt not a lost-connection error
PDOStatement-->>Caller: rethrow immediately
else execute() outside transaction
PDOStatement->>PDOStatement: log warning
PDOStatement->>PDO: reprepare()
PDOStatement->>PDO: retry original call
PDO-->>Caller: return result
else fetch/closeCursor or execute() in transaction
PDOStatement->>PDO: reconnect()
PDOStatement-->>Caller: rethrow error
end
Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 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 fixes a pool-poisoning bug where a lost connection during the fetch phase (
Confidence Score: 4/5Safe to merge — the logic restructuring is correct and all new code paths are covered by unit tests. The new reconnect-then-rethrow branch drops the desynced socket silently with no log output, unlike every other connection-recovery path in both PDOStatement and PDO, which could slow incident investigation in production. src/Database/PDOStatement.php — the new reconnect-then-rethrow branch (lines 138–140) emits no warning before calling reconnect. Important Files Changed
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
src/Database/PDOStatement.php:138-140
**Silent reconnect makes fetch-phase drops invisible in logs**
When a fetch-phase or in-transaction connection loss triggers the reconnect-then-rethrow path, no warning is emitted. The execute-outside-transaction path logs both the error message and "Lost connection detected" before acting, and `PDO::__call()` also logs before reconnecting in every case it handles. A fetch-phase drop silently discards the socket, giving operators no signal in the logs that a pooled connection was cleaned up. Consider adding a `Console::warning` pair (the original error message + a short note) before the `reconnect()` call, mirroring the existing pattern.
Reviews (1): Last reviewed commit: "fix(database): drop poisoned pooled conn..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| $this->pdo->reconnect(); | ||
|
|
||
| throw $e; |
There was a problem hiding this comment.
When a fetch-phase or in-transaction connection loss triggers the reconnect-then-rethrow path, no warning is emitted. The execute-outside-transaction path logs both the error message and "Lost connection detected" before acting, and PDO::__call() also logs before reconnecting in every case it handles. A fetch-phase drop silently discards the socket, giving operators no signal in the logs that a pooled connection was cleaned up. Consider adding a Console::warning pair (the original error message + a short note) before the reconnect() call, mirroring the existing pattern.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Database/PDOStatement.php
Line: 138-140
Comment:
**Silent reconnect makes fetch-phase drops invisible in logs**
When a fetch-phase or in-transaction connection loss triggers the reconnect-then-rethrow path, no warning is emitted. The execute-outside-transaction path logs both the error message and "Lost connection detected" before acting, and `PDO::__call()` also logs before reconnecting in every case it handles. A fetch-phase drop silently discards the socket, giving operators no signal in the logs that a pooled connection was cleaned up. Consider adding a `Console::warning` pair (the original error message + a short note) before the `reconnect()` call, mirroring the existing pattern.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Closing per course-correction. An adapter-level reconnect-on-fetch-error heals one trigger of a poisoned pooled connection, not the root cause. DAT-1904's root cause is at the connection-pool / lifecycle layer: a pooled connection can be handed to the next borrower carrying leftover state (an unconsumed result frame, or a leaked transaction counter). The correct invariant — a connection that experienced any error or still carries pending state is never reused; it is reset or destroyed before the next checkout — must be enforced there, independent of which trigger tainted it. Superseded by the pool/connection-lifecycle fix (see utopia-php/pools#33 lineage, #895/#896). |
|
Confirmed redundant after tracing the runtime path end-to-end — recording the evidence: The production DB adapter So the pool already reconnects the exact socket this PR reconnects, on the same error, one layer down — for both the result-frame and transaction-counter taint, and for every borrow-error path (not just fetch). An adapter-level reconnect here is redundant. Root fix: utopia-php/pools#33. |
Problem (DAT-1904)
A pooled DB connection is checked out per operation from a worker-shared, per-host pool. The reconnect wrapper in
PDOStatement::__call()only self-healed a lost connection for theexecute()-outside-a-transaction case. Every other lost-connection error was rethrown while the underlying PDO connection was left untouched and reclaimed to the pool as-is:fetch()/fetchAll()/closeCursor()(the fetch phase of every read, e.g.SQL::getDocument()atexecute()→fetchAll()→closeCursor(), andSQL::find())execute()inside a transactionA connection lost mid-result is wire-desynced: a partial or stale result frame can remain buffered on the socket (classically a client-side read timeout while the server keeps streaming the result set). Returning that connection to the pool poisons it.
Blast radius
The next coroutine to pop the poisoned connection runs its own query and reads the leftover frame first:
Because
Adapter::withTransaction()replays its callback up to 2× and callsgetPDO()afresh on each attempt, a reclaimed poisoned connection can be handed straight back into the replay.Fix
Extend the recovery in
PDOStatement::__call()so that on any detected lost-connection error (same detection as today,Connection::hasError()/DetectsLostConnections) that is not theexecute()-outside-transaction fast path, we callPDO::reconnect()to discard the wire-desynced socket before rethrowing. This guarantees the connection reclaimed to the pool is always clean.execute()-outside-transaction retry (reprepare + replay bindings + re-execute) is unchanged.withTransactionstill see the rethrow and roll back / replay on the now-fresh connection. In the server-genuinely-down case,reconnect()may itself throwConnection refused, which is already in the lost-connection needle set, so callers classify it identically.Why the fetch phase must reconnect rather than retry
A fetch cannot be retried in place: re-running
fetch()on a freshly prepared statement would read no rows. The only safe recovery is to drop the desynced socket and let the caller replay the whole operation on a clean connection.Tests
tests/unit/PDOStatementTest.php(mock-based, no live DB):testFetchPhaseLostConnectionReconnectsBeforeRethrowing(new, key regression) — a lost connection duringfetchAll()invokesreconnect()exactly once and does not re-prepare, then the original exception propagates. Fails without this change (reconnect called 0×).testExecuteInsideTransactionReconnectsThenRethrows(updated) —execute()losing the connection inside a transaction now drops the socket before rethrowing. Fails without this change.testDoesNotReconnectForNonExecuteNonConnectionError(new) — a non-connection error duringfetchAll()must not reconnect (no pool thrashing).execute()-outside-transaction reprepare/replay tests are unchanged and still pass.vendor/bin/pint --testpasses on the changed files. (The local full-suite run hits a pre-existing, unrelatedUtopia\Cache\Feature\Leasablefatal inWithCacheLeaseTest.phpfrom a stale localutopia-php/cacheinstall — present on cleanmain; CI's fresh install is unaffected.)Relationship to other work
startTransaction(); this hardens the poisoned-socket side so the connection is never reclaimed desynced. Different files, no overlap.Refs: #895, #896, appwrite-labs/cloud#4827.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests