Fix transaction state management across adapters

[premtsd-code]

Summary

• Remove redundant $this->inTransaction = 0 resets in base Adapter::withTransaction() after successful rollback — all adapter implementations already handle the counter correctly
• Fix Mongo withTransaction() to bypass transaction management for nested calls, preventing state corruption of the outer transaction's counter and session
• Add missing $this->inTransaction = 0 reset in Postgres rollbackTransaction() on PDOException, matching SQL adapter behavior
• Add e2e tests covering transaction state after known exceptions, retry exhaustion, and nested transactions

Test plan

• testTransactionStateAfterKnownException — verifies inTransaction() is false after a DuplicateException
• testTransactionStateAfterRetriesExhausted — verifies state after all retries are exhausted (skipped for MongoDB which has no retry logic)
• testNestedTransactionState — verifies outer transaction commits when inner transaction throws (skipped for MongoDB which doesn't support nested transactions)

[coderabbitai]

📝 Walkthrough

Walkthrough

Adjusted transaction handling across adapters: changed Adapter::withTransaction not to reset inTransaction in two specific failure paths; added nested-transaction/savepoint support in Postgres; guarded Mongo against nested transactions and replica-set checks; and added three end-to-end tests for the new behaviors.

Changes

Cohort / File(s)	Summary
Core transaction logic src/Database/Adapter.php	Modified withTransaction control flow so the inTransaction counter is not reset in two cases: when catching specific known exceptions (DuplicateException, RestrictedException, AuthorizationException, RelationshipException, ConflictException, LimitException), and when rethrowing a non-retryable exception after retries are exhausted.
Mongo adapter src/Database/Adapter/Mongo.php	Added guard to return early when not a replica set and to bypass nested transactions when inTransaction > 0 (call callback directly). Minor control-flow simplification in withTransaction.
Postgres adapter (savepoints) src/Database/Adapter/Postgres.php	Introduced nested-transaction handling using SAVEPOINTs when inTransaction > 0; rollbackTransaction now rolls back to previous savepoint for nested transactions and resets inTransaction on exceptional rollback paths.
End-to-end tests tests/e2e/Adapter/Scopes/GeneralTests.php	Added three tests: testTransactionStateAfterKnownException, testTransactionStateAfterRetriesExhausted, and testNestedTransactionState to validate inTransaction state and behavior for known exceptions, exhausted retries, and nested transaction scenarios. Also added Mongo import.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• With transactions optimization #677: Modifies Adapter::withTransaction retry and inTransaction-reset logic with early bailouts for specific exceptions.
• Reset counter on reconnect or rollback failure #748: Adjusts transaction counter handling (inTransaction) on error/reconnect paths.
• 1.x #717: Changes rollback and inTransaction reset/rethrow behavior in Adapter transaction error handling.

Suggested reviewers

• abnegate

Poem

🐇 I hop through code and count each stitch,

When nested burrows save the pitch,
Known snags I skip, retries take flight,
Tests keep tunnels tidy through the night.
A nibble, a hop — transactions sit tight.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix transaction state management across adapters' directly aligns with the PR's primary objective of addressing transaction state reset issues across multiple database adapters (Mongo, Postgres, SQL). It accurately summarizes the main change without being vague or misleading.

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

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix-transaction-state-reset

---

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: 1

🧹 Nitpick comments (2)

tests/e2e/Adapter/Scopes/GeneralTests.php (2)

857-858: Consider using finally blocks for test cleanup to avoid orphaned collections.

Both testTransactionStateAfterKnownException and testNestedTransactionState create collections and clean them up at the end, but if an assertion fails mid-test, deleteCollection is never reached. Wrapping cleanup in finally (or using setUp/tearDown) would prevent leftover collections from affecting subsequent test runs.

Also applies to: 937-938

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/Adapter/Scopes/GeneralTests.php` around lines 857 - 858, Tests
testTransactionStateAfterKnownException and testNestedTransactionState create
collections via Database::createCollection and remove them with
Database::deleteCollection but do not guarantee cleanup if assertions fail; wrap
the create/delete logic in a try/finally or move cleanup into the test class
tearDown to ensure deleteCollection always runs, i.e., perform createCollection
before the try, run assertions inside try, and call deleteCollection inside
finally (or register collection names in setUp and remove them in tearDown) so
orphaned collections cannot remain after failures.

---

918-919: Hardcoded retry count couples test to internal implementation.

The expected 3 attempts is hardcoded at line 404 of src/Database/Adapter.php as $retries = 2 (1 initial + 2 retries). If that default changes, this test fails without clear cause. Consider extracting the retry count to a class constant in Adapter and reference it in the test, or add a comment documenting the dependency.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/Adapter/Scopes/GeneralTests.php` around lines 918 - 919, The test
is hardcoded to expect 3 attempts which couples it to Adapter's internal
$retries value; add a public class constant (e.g., Adapter::DEFAULT_RETRIES) or
a public static getter (e.g., Adapter::getDefaultRetries()) in the Adapter class
where $retries is defined, set it to the current default (2), and update the
test in GeneralTests.php to compute expected attempts as
Adapter::DEFAULT_RETRIES + 1 (initial attempt + retries) instead of the literal
3 so the test follows the implementation default.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/e2e/Adapter/Scopes/GeneralTests.php`:
- Around line 948-974: The Mongo adapter's withTransaction implementation
mishandles nested transactions by resetting $this->inTransaction to 0 in the
inner transaction's finally/cleanup path; update Mongo.php so that
withTransaction decrements $this->inTransaction (e.g., --$this->inTransaction)
rather than assigning 0 when leaving a nested transaction, ensuring
commitTransaction sees the correct nesting level and outer transactions can
commit; alternatively, if Mongo replica-set transactions cannot support nesting,
add a test skip for this case, but the preferred fix is to correct the
inTransaction counter in the withTransaction method and ensure
commitTransaction/abortTransaction rely on that counter.

---

Nitpick comments:
In `@tests/e2e/Adapter/Scopes/GeneralTests.php`:
- Around line 857-858: Tests testTransactionStateAfterKnownException and
testNestedTransactionState create collections via Database::createCollection and
remove them with Database::deleteCollection but do not guarantee cleanup if
assertions fail; wrap the create/delete logic in a try/finally or move cleanup
into the test class tearDown to ensure deleteCollection always runs, i.e.,
perform createCollection before the try, run assertions inside try, and call
deleteCollection inside finally (or register collection names in setUp and
remove them in tearDown) so orphaned collections cannot remain after failures.
- Around line 918-919: The test is hardcoded to expect 3 attempts which couples
it to Adapter's internal $retries value; add a public class constant (e.g.,
Adapter::DEFAULT_RETRIES) or a public static getter (e.g.,
Adapter::getDefaultRetries()) in the Adapter class where $retries is defined,
set it to the current default (2), and update the test in GeneralTests.php to
compute expected attempts as Adapter::DEFAULT_RETRIES + 1 (initial attempt +
retries) instead of the literal 3 so the test follows the implementation
default.