refactor(transaction): own connection recovery in the adapter, drop Swoole PDOProxy dependency#896
Conversation
|
Warning Review limit reached
More reviews will be available in 4 minutes and 35 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughIntroduces ChangesPDOStatement Reconnect Wrapper
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
abnegate
force-pushed
the
fix/adapter-owned-connection-recovery
branch
from
f2f8a74 to
fa02875
Compare
Greptile Summary
Confidence Score: 5/5The changes are narrowly scoped to PDO connection recovery and statement wrapping, with no outstanding code issues identified. The implementation is backed by targeted unit coverage for reconnect, re-prepare, binding replay, forwarding, and transaction cleanup recovery paths.
 T-Rex LogsWhat T-Rex did
Reviews (6): Last reviewed commit: "refactor(transaction): own connection re..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
abnegate
force-pushed
the
fix/adapter-owned-connection-recovery
branch
2 times, most recently
from
c04e719 to
3c35369
Compare
abnegate
force-pushed
the
fix/adapter-owned-connection-recovery
branch
from
3c35369 to
5a807d7
Compare
…le PDOProxy The Swoole PDOProxy keeps its own transaction counter that desyncs on a mid-transaction connection loss and is never reset on reconnect or pool checkin, poisoning pooled connections (every later startTransaction trusts the stale counter and fails with "There is no active transaction" until restart). utopia-php/database#896 moves connection-loss recovery into the adapter (reconnect-on-execute via Utopia\Database\PDOStatement + withTransaction replay) with the real PDO as the single source of truth, so the proxy is no longer needed and the desync is structurally impossible. - registers.php: wrap connections in Utopia\Database\PDO directly instead of Swoole\Database\PDOProxy. - Set PDO::ATTR_ERRMODE => ERRMODE_EXCEPTION explicitly (previously enforced by the proxy; also defaulted by #896, set here for clarity). - Require the #896 branch via a VCS repository until it is tagged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Database/PDO.php`:
- Around line 64-72: The prepareNative method lacks defensive reconnection logic
for when native prepares are used (ATTR_EMULATE_PREPARES => false). Although
current configurations enforce emulated prepares where connection loss surfaces
at execution time and is properly recovered, if native prepares are enabled in
the future, connection loss during the prepare call would bypass the recovery
mechanism in PDO::__call() and propagate uncaught. Add defensive reconnection
handling to prepareNative by checking for transaction context before attempting
to reconnect and retry the prepare operation, following the same recovery
pattern already implemented in PDOStatement::__call() for execution-time
failures.
In `@src/Database/PDOStatement.php`:
- Around line 87-101: The __call method unconditionally retries the statement
execution after a reconnect without verifying whether the first execution
already completed on the server, which can cause non-transactional write
operations to be applied twice and bypass adapter execution hooks like
statement_timeout. Gate the statement replay at line 101 to only retry for
idempotent operations or operations within an active transaction, or move the
retry orchestration to a higher-level layer that can ensure idempotency and
manage adapter session state. Modify the retry logic after reprepare() to check
the operation type or transaction context before re-executing the statement
method.
- Around line 131-142: The bindParam() and bindColumn() methods store variables
by value instead of by reference, causing stale copies to be used during
reprepare() rebinding. Fix this by storing references to the actual variables in
the $this->params and $this->columns arrays rather than copying their values.
Additionally, the bindColumn() method has an incorrect signature with nullable
parameter defaults (?int $type = null, ?int $maxLength = null) that do not match
PDO's native expectations; update these to use non-nullable defaults matching
PDO's actual signature (int $type = PDO::PARAM_STR, int $maxLength = 0).
Finally, when reprepare() loops through and re-binds these stored parameters and
columns, ensure the iteration maintains reference semantics so that caller-side
mutations and fetched values are properly reflected in the original caller's
variables.
In `@tests/unit/PDOStatementTest.php`:
- Around line 61-64: The mock expectation for prepareNative() in the test is
incomplete. Currently, the ->with() matcher only specifies the first argument
('SELECT :id'), but the actual reprepare() method invokes prepareNative() with
two arguments: the query string and an options parameter. Update the ->with()
expectation to match both arguments that are actually passed to prepareNative(),
ensuring the test accurately reflects the real method contract and prevents
brittleness from future changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 941a84a9-7122-4ee8-a97e-ebefde00b43f
📒 Files selected for processing (7)
src/Database/Adapter/Postgres.phpsrc/Database/Adapter/SQL.phpsrc/Database/Adapter/SQLite.phpsrc/Database/PDO.phpsrc/Database/PDOStatement.phptests/unit/PDOStatementTest.phptests/unit/PDOTest.php
| public function __call(string $method, array $args): mixed | ||
| { | ||
| try { | ||
| return $this->statement->{$method}(...$args); | ||
| } catch (\Throwable $e) { | ||
| if ($this->pdo->inTransaction() || !Connection::hasError($e)) { | ||
| throw $e; | ||
| } | ||
|
|
||
| Console::warning('[Database] ' . $e->getMessage()); | ||
| Console::warning('[Database] Lost connection detected. Re-preparing statement...'); | ||
|
|
||
| $this->reprepare(); | ||
|
|
||
| return $this->statement->{$method}(...$args); |
There was a problem hiding this comment.
Gate statement replay to idempotent or transaction-managed operations.
Line 101 retries the same statement after reconnect without knowing whether the first execution reached the server. For non-transactional writes such as updates/increments, a disconnect after server-side apply but before client acknowledgement can double-apply the mutation; the internal reconnect also bypasses adapter execution hooks such as Postgres’ per-query statement_timeout. Move retry orchestration to a layer that can prove idempotency/reapply adapter session state, or make write replay opt-in through transaction-level retry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Database/PDOStatement.php` around lines 87 - 101, The __call method
unconditionally retries the statement execution after a reconnect without
verifying whether the first execution already completed on the server, which can
cause non-transactional write operations to be applied twice and bypass adapter
execution hooks like statement_timeout. Gate the statement replay at line 101 to
only retry for idempotent operations or operations within an active transaction,
or move the retry orchestration to a higher-level layer that can ensure
idempotency and manage adapter session state. Modify the retry logic after
reprepare() to check the operation type or transaction context before
re-executing the statement method.
5a807d7 to
682c863
Compare
|
Thanks for the thorough pass — addressed the P1s in
SQLite transactions reconnect — I don't think this one is reachable, but want a second opinion before closing it: the reconnect branch requires both Full unit suite (365), PHPStan L7, and Pint are green. |
…le PDOProxy The Swoole PDOProxy keeps its own transaction counter that desyncs on a mid-transaction connection loss and is never reset on reconnect or pool checkin, poisoning pooled connections (every later startTransaction trusts the stale counter and fails with "There is no active transaction" until restart). utopia-php/database#896 moves connection-loss recovery into the adapter (reconnect-on-execute via Utopia\Database\PDOStatement + withTransaction replay) with the real PDO as the single source of truth, so the proxy is no longer needed and the desync is structurally impossible. - registers.php: wrap connections in Utopia\Database\PDO directly instead of Swoole\Database\PDOProxy. - Set PDO::ATTR_ERRMODE => ERRMODE_EXCEPTION explicitly (previously enforced by the proxy; also defaulted by #896, set here for clarity). - Require the #896 branch via a VCS repository until it is tagged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
abnegate
force-pushed
the
fix/adapter-owned-connection-recovery
branch
from
682c863 to
9bab43f
Compare
|
On the remaining CodeRabbit items: Match Gate statement replay to idempotent / transaction-managed operations — I've scoped this rather than coded it, and want to lay out why I think that's right for this PR:
So this PR doesn't introduce new exposure; it reduces it. True exactly-once writes (idempotency keys / dedup) and replaying session state across a reconnect are worthwhile hardening but are an architectural change well beyond removing the Swoole proxy. Happy to open a separate tracking issue for both rather than fold them in here. The other two (prepare-time reconnect for native prepares, and by-reference |
…le PDOProxy The Swoole PDOProxy keeps its own transaction counter that desyncs on a mid-transaction connection loss and is never reset on reconnect or pool checkin, poisoning pooled connections (every later startTransaction trusts the stale counter and fails with "There is no active transaction" until restart). utopia-php/database#896 moves connection-loss recovery into the adapter (reconnect-on-execute via Utopia\Database\PDOStatement + withTransaction replay) with the real PDO as the single source of truth, so the proxy is no longer needed and the desync is structurally impossible. - registers.php: wrap connections in Utopia\Database\PDO directly instead of Swoole\Database\PDOProxy. - Set PDO::ATTR_ERRMODE => ERRMODE_EXCEPTION explicitly (previously enforced by the proxy; also defaulted by #896, set here for clarity). - Require the #896 branch via a VCS repository until it is tagged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…woole PDOProxy dependency The Swoole PDOProxy keeps its own transaction counter that is incremented on beginTransaction but only decremented on a *successful* commit/rollback, and is never reset on reconnect or on pool checkin (utopia-php/pools does not call its reset() hook). A connection lost mid-transaction therefore leaks the counter and poisons the pooled connection: every later startTransaction trusts the stale counter, rolls back a transaction the real connection no longer holds, and fails with "There is no active transaction". This produced a sustained write outage across all projects on cloud nyc3. Make the library self-sufficient for connection-loss recovery so consumers no longer need to wrap connections in a Swoole PDOProxy: - PDOStatement wraps prepared statements and transparently re-prepares on the reconnected PDO when the connection is lost at execution time, replaying bound params/attributes. Recovery is skipped inside a transaction, where it rethrows so withTransaction can replay the whole transaction from the start. - PDO::prepare() returns the wrapper; prepareNative() re-prepares raw on the reconnected connection. ERRMODE_EXCEPTION is enforced by default. - withTransaction() reconnects on a lost connection before replaying, so the retry runs on a fresh, transaction-less connection. - Transaction state now has a single source of truth (the real PDO via Utopia\Database\PDO::inTransaction()); there is no separate counter to desync. - Replace the Swoole\Database\PDOStatementProxy type hints in the SQL adapters. Stacked on #895. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
abnegate
force-pushed
the
fix/adapter-owned-connection-recovery
branch
from
9bab43f to
12c3bf1
Compare
|
Re-review on For the record, on the SQLite transactions reconnect flag from the first pass (not re-raised since): I believe it's unreachable rather than fixed-in-code, and want to flag my reasoning in case you disagree. The reconnect path needs both Full unit suite (366), PHPStan L7, Pint green at |
…le PDOProxy The Swoole PDOProxy keeps its own transaction counter that desyncs on a mid-transaction connection loss and is never reset on reconnect or pool checkin, poisoning pooled connections (every later startTransaction trusts the stale counter and fails with "There is no active transaction" until restart). utopia-php/database#896 moves connection-loss recovery into the adapter (reconnect-on-execute via Utopia\Database\PDOStatement + withTransaction replay) with the real PDO as the single source of truth, so the proxy is no longer needed and the desync is structurally impossible. - registers.php: wrap connections in Utopia\Database\PDO directly instead of Swoole\Database\PDOProxy. - Set PDO::ATTR_ERRMODE => ERRMODE_EXCEPTION explicitly (previously enforced by the proxy; also defaulted by #896, set here for clarity). - Require the #896 branch via a VCS repository until it is tagged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Problem
#895 stops the symptom of the nyc3 write outage, but the root cause is upstream of this library: consumers wrap connections in
Swoole\Database\PDOProxy, which keeps its owninTransactioncounter that is incremented onbeginTransaction, decremented only on a successful commit/rollback, not reset byreconnect(), and not reset on pool checkin (utopia-php/poolsnever calls the proxy'sreset()— that hook only fires under Swoole's ownPDOPool).A connection lost mid-transaction leaks the counter and poisons the pooled connection: every later
startTransactioncleanup probe trusts the stalegetPDO()->inTransaction(), issues arollBackthe real connection no longer holds, and fails withThere is no active transaction— across all projects, until pods restart. (nyc3: ~110k errors over ~1.5h, writes only.)This can't be fixed by patching the cleanup probe while a second, unreconciled source of truth for "am I in a transaction" lives in the proxy. The fix removes the need for the proxy and makes the adapter own connection-loss recovery, with the real
\PDOas the single source of truth.What changed
PDOStatement(new) — wraps prepared statements. On a lost connection at execution time, when not in a transaction, it reconnects the owningPDO, re-prepares against the fresh connection, replays bound params/attributes/options, and retries. This restores the only genuinely load-bearing behaviour the SwoolePDOStatementProxyprovided — reconnect-on-execute — and it's what heals a stale pooled connection at theprepare('ROLLBACK')->execute()probe that frontsstartTransaction(that probe runs atinTransaction === 0). Inside a transaction it rethrows; the connection state is read from the real\PDO, so there is no separate counter to desync.PDO::prepare()returns the wrapper;prepareNative()re-prepares a raw\PDOStatementfor the wrapper.ERRMODE_EXCEPTIONis defaulted in the constructor so the library no longer depends on the proxy/consumer to set it.Swoole\Database\PDOStatementProxytype hints inSQL/Postgres/SQLitewith the newPDOStatement.Connection-level calls (
beginTransaction/rollBack/commit/exec) are already covered by the pre-existingUtopia\Database\PDO::__callreconnect, andAdapter::withTransaction's pre-existing retry loop replays the closure — so the mid-transaction-death path recovers by replay on the connection that__callreconnects. No change towithTransactionis needed.Reviewed
Two independent reviews of this PR led to: removing a speculative
withTransactionreconnect block that an earlier revision added — it was unreachable on the real SQL path (a dead-connectionrollBackthrows and is caught earlier in the loop) and, when reached (e.g. a nested transaction), would have discarded the outerBEGINand committed the inner write standalone (silent partial commit). Recovery is correctly carried byPDOStatementre-prepare +PDO::__call, both unit-covered. Also threadedprepare()$optionsthrough re-prepare and tightened a@throws.Known limitation (pre-existing, out of scope): connection loss inside a nested transaction is not safely recoverable by the retry loop in any version; this PR does not make it worse (it no longer adds a reconnect there).
Tests
PDOStatementTest(reconnect/re-prepare/replay outside a txn; rethrow inside; non-connection rethrow; forwarding),PDOTest(PDO::__callreconnect, wrapper return),SQLTransactionTest(desynced-cleanup recovery, MySQL + Postgres).phpunit(full unit suite),phpstan --level 7,pintgreen locally.Recommended follow-up: an e2e test that severs a live MySQL/Postgres connection mid-transaction (
KILL/wait_timeout) and asserts recovery — the recovery path is currently only unit-mocked.Consumer follow-ups
PDOProxy, constructUtopia\Database\PDOdirectly. Re-pin from the branch alias to the release tag once this merges + tags.Summary by CodeRabbit
Bug Fixes
Internal Improvements