diff --git a/src/Database/PDOStatement.php b/src/Database/PDOStatement.php index 5dbfc4d689..a3c156b3a7 100644 --- a/src/Database/PDOStatement.php +++ b/src/Database/PDOStatement.php @@ -10,11 +10,19 @@ * against the fresh connection, previously bound parameters/columns/attributes * are replayed, and the failed execute() is retried. * - * Recovery is attempted only for execute(), and only outside a transaction: - * re-running any other method (fetch, rowCount, ...) without a fresh execute - * would return data from an unexecuted statement, and a connection cannot be - * healed in place mid-transaction (the uncommitted state is gone, so the call - * is rethrown for Adapter::withTransaction to roll back and replay). + * Transparent retry is attempted only for execute(), and only outside a + * transaction: re-running any other method (fetch, rowCount, ...) without a + * fresh execute would return data from an unexecuted statement, and a + * connection cannot be healed in place mid-transaction (the uncommitted state + * is gone, so the call is rethrown for Adapter::withTransaction to roll back + * and replay). + * + * In every lost-connection case that cannot be retried, the poisoned connection + * is still discarded (reconnect) before the rethrow: the failed fetch/execute + * leaves the socket wire-desynced, and a pooled connection reclaimed with a + * partial result frame still buffered would bleed that frame into the next + * borrower's query. Reconnecting guarantees the connection returned to the pool + * is always clean. * * @mixin \PDOStatement * @implements \IteratorAggregate @@ -108,20 +116,28 @@ public function __call(string $method, array $args): mixed try { return $this->statement->{$method}(...$args); } catch (\Throwable $e) { - if ( - \strcasecmp($method, 'execute') !== 0 - || $this->pdo->inTransaction() - || !Connection::hasError($e) - ) { + if (!Connection::hasError($e)) { throw $e; } - Console::warning('[Database] ' . $e->getMessage()); - Console::warning('[Database] Lost connection detected. Re-preparing statement...'); + if (\strcasecmp($method, 'execute') === 0 && !$this->pdo->inTransaction()) { + Console::warning('[Database] ' . $e->getMessage()); + Console::warning('[Database] Lost connection detected. Re-preparing statement...'); - $this->reprepare(); + $this->reprepare(); - return $this->statement->{$method}(...$args); + return $this->statement->{$method}(...$args); + } + + // A lost connection during fetch/fetchAll/closeCursor, or during + // execute inside a transaction, cannot be retried in place -- but the + // socket is now wire-desynced and may still hold a partial result + // frame. Drop the connection so it can never be reclaimed to the pool + // and bleed that frame into the next borrower's query; the rethrow + // still lets withTransaction roll back and replay on a fresh one. + $this->pdo->reconnect(); + + throw $e; } } diff --git a/tests/unit/PDOStatementTest.php b/tests/unit/PDOStatementTest.php index 8bd8f280b7..c396c23f64 100644 --- a/tests/unit/PDOStatementTest.php +++ b/tests/unit/PDOStatementTest.php @@ -69,10 +69,12 @@ public function testExecuteReconnectsRePreparesAndReplaysWhenNotInTransaction(): $this->assertTrue($statement->execute()); } - public function testExecuteRethrowsAndDoesNotReconnectInsideTransaction(): void + public function testExecuteInsideTransactionReconnectsThenRethrows(): void { $pdo = $this->pdoMock(inTransaction: true); - $pdo->expects($this->never())->method('reconnect'); + // Cannot retry in place (the uncommitted state is gone), but the poisoned + // socket must still be dropped so it is not reclaimed to the pool. + $pdo->expects($this->once())->method('reconnect'); $pdo->expects($this->never())->method('prepareNative'); $statement = $this->statementMock(); @@ -133,15 +135,20 @@ public function testIsIterableAndDelegatesIterationToTheStatement(): void $this->assertSame($statement, $wrapper->getIterator()); } - public function testDoesNotReconnectForNonExecuteMethods(): void + public function testFetchPhaseLostConnectionReconnectsBeforeRethrowing(): void { + // DAT-1904: a lost connection during the fetch phase leaves the socket + // wire-desynced (a partial result frame may remain buffered). The poisoned + // connection must be dropped (reconnect) before the rethrow so it can never + // be reclaimed to the pool and bled into the next borrower's query. $pdo = $this->pdoMock(inTransaction: false); - $pdo->expects($this->never())->method('reconnect'); + $pdo->expects($this->once())->method('reconnect'); + // Not re-prepared/retried in place: a fresh fetch would read no rows. $pdo->expects($this->never())->method('prepareNative'); $statement = $this->statementMock(); $statement->expects($this->once()) - ->method('fetch') + ->method('fetchAll') ->willThrowException(new PDOException('server has gone away')); $wrapper = new PDOStatement($pdo, $statement, 'SELECT 1'); @@ -149,7 +156,29 @@ public function testDoesNotReconnectForNonExecuteMethods(): void $this->expectException(PDOException::class); $this->expectExceptionMessage('server has gone away'); - $wrapper->fetch(); + $wrapper->fetchAll(); + } + + public function testDoesNotReconnectForNonExecuteNonConnectionError(): void + { + // A business-logic error (not a lost connection) leaves the pooled + // connection untouched: reconnecting a healthy connection would thrash the + // pool on every ordinary query error. + $pdo = $this->pdoMock(inTransaction: false); + $pdo->expects($this->never())->method('reconnect'); + $pdo->expects($this->never())->method('prepareNative'); + + $statement = $this->statementMock(); + $statement->expects($this->once()) + ->method('fetchAll') + ->willThrowException(new PDOException('SQLSTATE[42000]: Syntax error')); + + $wrapper = new PDOStatement($pdo, $statement, 'SELECT 1'); + + $this->expectException(PDOException::class); + $this->expectExceptionMessage('Syntax error'); + + $wrapper->fetchAll(); } public function testBindParamReplaysCurrentValueAfterReconnect(): void