Support nested transactions via savepoints - #122
Conversation
`handle_begin/2`, `handle_commit/2` and `handle_rollback/2` all matched on
`_opts` and discarded them, so DBConnection's `mode: :savepoint` never
arrived. A nested transaction issued a plain `BEGIN` inside the open one and
failed with `cannot start a transaction within a transaction`.
This matters most for `Ecto.Adapters.SQL.Sandbox`, which holds a transaction
open for the duration of every test and therefore makes each
`Repo.transaction/1` in application code a nested one. On this adapter none
of that code could be tested at all: an app with a `phx.gen.auth` install and
the sandbox configured the usual way sees its whole auth suite fail here.
There is no sandbox coverage in the test suite at present, which is probably
why it went unnoticed; `test/ecto_sandbox_savepoint_test.exs` adds some.
The NIF already exposed `savepoint/3`, `release_savepoint/3` and
`rollback_to_savepoint/3` - they were simply never called. Wire them into the
three callbacks following the structure postgrex uses. SQLite permits
duplicate savepoint names and resolves RELEASE and ROLLBACK TO against the
most recent match, so a single fixed name nests correctly.
Three details worth noting:
- `:mode` is overloaded. DBConnection sets `:transaction` or `:savepoint`,
while this adapter's own API uses the same key for the libSQL transaction
mode (`:deferred`, `:immediate`, `:exclusive`, `:read_only`). Only
`:savepoint` is intercepted, so those keep working.
- Releasing a savepoint leaves the enclosing transaction open, so the state
(trx_id included) carries through untouched rather than being rebuilt as
idle the way a real COMMIT rebuilds it.
- ROLLBACK TO does not discard the savepoint - SQLite keeps it live so it
can be rolled back to again - so it is released explicitly afterwards.
Without that the next savepoint of the same name nests inside the old one
and the stack never unwinds.
Nested `Repo.transaction/1` calls that Ecto flattens itself are unaffected,
since those never reach `handle_begin/2` a second time.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe adapter now handles nested transaction callbacks with a fixed savepoint. The test suite verifies commit, explicit rollback, and exception rollback within an Ecto SQL Sandbox transaction. ChangesNested savepoints
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant EctoRepo
participant EctoLibSql
participant SavepointNIF
participant LibSQLDatabase
EctoRepo->>EctoLibSql: Start nested transaction
EctoLibSql->>SavepointNIF: Create named savepoint
SavepointNIF->>LibSQLDatabase: Store savepoint
EctoRepo->>EctoLibSql: Commit or rollback nested transaction
EctoLibSql->>SavepointNIF: Release or roll back savepoint
SavepointNIF->>LibSQLDatabase: Apply savepoint operation
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. A rabbit guards the savepoint gate Comment |
|
@CodeRabbit review |
|
A double blank line before the following @impl attribute failed `mix format --check-formatted` in CI. No behavioural change. Claude-Session: https://claude.ai/code/session_01QeALzUVYXSgNWcYMEJnqeY
The on_exit callback checked Process.alive?/1 and then called Supervisor.stop/1, which is a time-of-check/time-of-use race: stopping the sandbox owner can take the repo supervisor down while the callback is between those two lines, and Supervisor.stop/1 then exits with :noproc. This failed the Elixir job on macos-latest while ubuntu passed, with the test body itself having passed - only the cleanup exited. Matches the try/catch pattern already used in the other test files added alongside this one. Claude-Session: https://claude.ai/code/session_01QeALzUVYXSgNWcYMEJnqeY
|
@CodeRabbit review |
✅ Action performedReview finished.
|
The problem
handle_begin/2,handle_commit/2andhandle_rollback/2all match on_optsand discard them, so DBConnection'smode: :savepointnever arrives. A nested transaction issues a plainBEGINinside the open one and fails:The NIF already exposes
savepoint/3,release_savepoint/3androllback_to_savepoint/3— they are simply never called.Why it matters
Ecto.Adapters.SQL.Sandboxholds a transaction open for the duration of every test, so everyRepo.transaction/1in application code runs nested when tested. On this adapter none of that code can be tested at all.I hit this on an app with a stock
phx.gen.authinstall: 84 tests failed, all with the message above, becauseAccounts.update_user_and_delete_all_tokens/1wraps its two writes inRepo.transact/1.There is no sandbox coverage in the suite at present, which is probably why this went unnoticed —
test/ecto_sandbox_savepoint_test.exsadds some.The change
Wires the existing NIF primitives into the three callbacks, keyed off
opts[:mode], following the structure postgrex uses. SQLite permits duplicate savepoint names and resolvesRELEASE/ROLLBACK TOagainst the most recent match, so one fixed name nests correctly.Three details worth flagging for review:
:modeis overloaded. DBConnection sets:transaction/:savepoint, while this adapter's own API uses the same key for the libSQL transaction mode (:deferred,:immediate,:exclusive,:read_only). Only:savepointis intercepted so those keep working — my first attempt did not, and the existingLibSQL transaction modestests caught it.trx_idincluded) carries through untouched rather than being rebuilt as idle the way a realCOMMITrebuilds it.ROLLBACK TOdoes not discard the savepoint — SQLite keeps it live so it can be rolled back to again — so it is released explicitly afterwards. Without that the next savepoint of the same name nests inside the old one and the stack never unwinds.Nested
Repo.transaction/1calls that Ecto flattens itself are unaffected, since those never reachhandle_begin/2a second time.Testing
test/ecto_sandbox_savepoint_test.exs— 3 tests. All 3 fail ondevwithcannot start a transaction within a transactionand pass with this change.dev, 841 passed with this change (the 3 new ones). No existing test changed behaviour, including the two inecto_sql_transaction_compat_test.exsthat document the poisoned-outer-transaction semantics.Summary by CodeRabbit
New Features
Bug Fixes
Tests