From e3cdbf401326fb0ebb24d80bf0245e69ca9ffa43 Mon Sep 17 00:00:00 2001 From: Steven Cahill Date: Sat, 12 Sep 2026 14:14:40 +0000 Subject: [PATCH 1/3] Support nested transactions via savepoints `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. --- lib/ecto_libsql.ex | 89 ++++++++++++++++++----- test/ecto_sandbox_savepoint_test.exs | 104 +++++++++++++++++++++++++++ 2 files changed, 174 insertions(+), 19 deletions(-) create mode 100644 test/ecto_sandbox_savepoint_test.exs diff --git a/lib/ecto_libsql.ex b/lib/ecto_libsql.ex index 94f49fcf..4a4659b2 100644 --- a/lib/ecto_libsql.ex +++ b/lib/ecto_libsql.ex @@ -302,6 +302,12 @@ defmodule EctoLibSql do |> Enum.map(fn [_full, name] -> name end) end + # DBConnection drives nested transactions with `mode: :savepoint`. SQLite allows + # duplicate savepoint names and resolves RELEASE/ROLLBACK TO against the most + # recent match, so one fixed name nests correctly - the same approach postgrex + # takes with "postgrex_savepoint". + @savepoint "ecto_libsql_savepoint" + @impl true @doc """ Begins a new database transaction. @@ -309,10 +315,29 @@ defmodule EctoLibSql do The transaction behaviour (deferred/immediate/exclusive) can be controlled via options passed to the Native module. """ - def handle_begin(_opts, state) do - case EctoLibSql.Native.begin(state) do - {:ok, new_state} -> {:ok, :begin, new_state} - {:error, reason} -> {:error, reason, state} + def handle_begin(opts, state) do + # `:mode` is overloaded here: DBConnection sets `:transaction` or `:savepoint`, + # while this adapter's own API uses it for the libSQL transaction mode + # (`:deferred`, `:immediate`, `:exclusive`, `:read_only`). Only `:savepoint` is + # intercepted; everything else, including an absent mode, begins as before so + # the transaction modes keep working. + case {Keyword.get(opts, :mode), state.trx_id} do + {:savepoint, trx_id} when is_binary(trx_id) -> + case EctoLibSql.Native.create_savepoint(state, @savepoint) do + :ok -> {:ok, %EctoLibSql.Result{}, state} + {:error, reason} -> {:error, savepoint_error(reason), state} + end + + {:savepoint, nil} -> + # A savepoint outside a transaction is not something SQLite can honour. + # Report status and let DBConnection decide rather than emitting bad SQL. + {:idle, state} + + _other -> + case EctoLibSql.Native.begin(state) do + {:ok, new_state} -> {:ok, :begin, new_state} + {:error, reason} -> {:error, reason, state} + end end end @@ -328,15 +353,24 @@ defmodule EctoLibSql do The state must contain a valid transaction ID. For embedded replicas with auto-sync enabled, this will also trigger a sync to the remote database. """ - def handle_commit(_opts, state) do - case EctoLibSql.Native.commit( - %EctoLibSql.State{conn_id: conn_id, trx_id: _trx_id, mode: mode} = state - ) do - {:ok, _} -> - {:ok, %EctoLibSql.Result{}, %EctoLibSql.State{conn_id: conn_id, mode: mode}} - - {:error, reason} -> - {:disconnect, reason, state} + def handle_commit(opts, state) do + if Keyword.get(opts, :mode, :transaction) == :savepoint do + # Releasing a savepoint leaves the enclosing transaction open, so the + # state - trx_id included - carries through untouched. + case EctoLibSql.Native.release_savepoint_by_name(state, @savepoint) do + :ok -> {:ok, %EctoLibSql.Result{}, state} + {:error, reason} -> {:error, savepoint_error(reason), state} + end + else + case EctoLibSql.Native.commit( + %EctoLibSql.State{conn_id: conn_id, trx_id: _trx_id, mode: mode} = state + ) do + {:ok, _} -> + {:ok, %EctoLibSql.Result{}, %EctoLibSql.State{conn_id: conn_id, mode: mode}} + + {:error, reason} -> + {:disconnect, reason, state} + end end end @@ -347,16 +381,33 @@ defmodule EctoLibSql do Discards all changes made within the transaction and returns the connection to autocommit mode. """ - def handle_rollback(_opts, %EctoLibSql.State{conn_id: conn_id, trx_id: _trx_id} = state) do - case EctoLibSql.Native.rollback(state) do - {:ok, _} -> - {:ok, %EctoLibSql.Result{}, %EctoLibSql.State{conn_id: conn_id, trx_id: nil}} + def handle_rollback(opts, %EctoLibSql.State{conn_id: conn_id, trx_id: _trx_id} = state) do + if Keyword.get(opts, :mode, :transaction) == :savepoint do + # ROLLBACK TO leaves the savepoint itself active (SQLite keeps it so it can + # be rolled back to again), so it has to be released explicitly afterwards + # or the next savepoint of the same name nests inside this one. + with :ok <- EctoLibSql.Native.rollback_to_savepoint_by_name(state, @savepoint), + :ok <- EctoLibSql.Native.release_savepoint_by_name(state, @savepoint) do + {:ok, %EctoLibSql.Result{}, state} + else + {:error, reason} -> {:error, savepoint_error(reason), state} + end + else + case EctoLibSql.Native.rollback(state) do + {:ok, _} -> + {:ok, %EctoLibSql.Result{}, %EctoLibSql.State{conn_id: conn_id, trx_id: nil}} - {:error, reason} -> - {:disconnect, reason, state} + {:error, reason} -> + {:disconnect, reason, state} + end end end + defp savepoint_error(reason) when is_binary(reason), do: %RuntimeError{message: reason} + defp savepoint_error(%{__exception__: true} = reason), do: reason + defp savepoint_error(reason), do: %RuntimeError{message: inspect(reason)} + + @impl true @doc """ Closes the query. Currently a no-op as queries are stateless. diff --git a/test/ecto_sandbox_savepoint_test.exs b/test/ecto_sandbox_savepoint_test.exs new file mode 100644 index 00000000..794440e7 --- /dev/null +++ b/test/ecto_sandbox_savepoint_test.exs @@ -0,0 +1,104 @@ +defmodule EctoLibSql.EctoSandboxSavepointTest do + @moduledoc """ + Coverage for `Ecto.Adapters.SQL.Sandbox`, which is how most Ecto applications run + their entire test suite. + + The sandbox holds one transaction open for the duration of each test and rolls it + back afterwards, so every `Repo.transaction/1` in application code runs nested and + is issued by DBConnection with `mode: :savepoint`. Without SAVEPOINT support in + `handle_begin/2` that mode is discarded, a plain BEGIN is issued inside the open + transaction, and the test fails with "cannot start a transaction within a + transaction" - meaning no transactional application code can be tested at all. + """ + use ExUnit.Case, async: false + + import Ecto.Query + + alias Ecto.Adapters.SQL.Sandbox + + defmodule SandboxRepo do + use Ecto.Repo, otp_app: :ecto_libsql, adapter: Ecto.Adapters.LibSql + end + + defmodule Item do + use Ecto.Schema + + schema "sandbox_items" do + field(:num, :integer) + end + end + + setup do + unique_id = :erlang.unique_integer([:positive]) + test_db = "z_ecto_libsql_test-sandbox_savepoint-#{unique_id}.db" + + {:ok, pid} = + SandboxRepo.start_link( + database: test_db, + pool_size: 1, + pool: Sandbox, + name: SandboxRepo + ) + + Sandbox.mode(SandboxRepo, :manual) + owner = Sandbox.start_owner!(SandboxRepo, shared: true) + + Ecto.Adapters.SQL.query!(SandboxRepo, """ + CREATE TABLE IF NOT EXISTS sandbox_items ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + num INTEGER + ) + """) + + on_exit(fn -> + Sandbox.stop_owner(owner) + if Process.alive?(pid), do: Supervisor.stop(pid) + Process.sleep(50) + for f <- Path.wildcard(test_db <> "*"), do: File.rm(f) + end) + + :ok + end + + defp nums, do: SandboxRepo.all(from(i in Item, order_by: i.num, select: i.num)) + + test "a transaction can be opened inside the sandbox transaction at all" do + assert {:ok, :done} = + SandboxRepo.transaction(fn -> + SandboxRepo.insert!(%Item{num: 1}) + :done + end) + + assert nums() == [1] + end + + test "rolling back only discards work done inside the savepoint" do + SandboxRepo.insert!(%Item{num: 1}) + + assert {:error, :nope} = + SandboxRepo.transaction(fn -> + SandboxRepo.insert!(%Item{num: 2}) + SandboxRepo.rollback(:nope) + end) + + # The write made before the savepoint survives, and the sandbox transaction is + # still usable afterwards - which is the whole point of rolling back to a + # savepoint rather than aborting the enclosing transaction. + assert nums() == [1] + SandboxRepo.insert!(%Item{num: 3}) + assert nums() == [1, 3] + end + + test "an exception inside the transaction is contained the same way" do + SandboxRepo.insert!(%Item{num: 1}) + + assert_raise RuntimeError, "boom", fn -> + SandboxRepo.transaction(fn -> + SandboxRepo.insert!(%Item{num: 2}) + raise "boom" + end) + end + + assert nums() == [1] + end +end From 066658e89c26fc3b4fd48fc1556485e46b93a086 Mon Sep 17 00:00:00 2001 From: Drew Robinson Date: Fri, 18 Sep 2026 09:12:04 +1000 Subject: [PATCH 2/3] style: remove stray blank line after savepoint_error/1 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 --- lib/ecto_libsql.ex | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/ecto_libsql.ex b/lib/ecto_libsql.ex index 4a4659b2..b8c3aba2 100644 --- a/lib/ecto_libsql.ex +++ b/lib/ecto_libsql.ex @@ -407,7 +407,6 @@ defmodule EctoLibSql do defp savepoint_error(%{__exception__: true} = reason), do: reason defp savepoint_error(reason), do: %RuntimeError{message: inspect(reason)} - @impl true @doc """ Closes the query. Currently a no-op as queries are stateless. From 94b577ff68a207355ee86d4673adfecb9a638ce8 Mon Sep 17 00:00:00 2001 From: Drew Robinson Date: Fri, 18 Sep 2026 17:10:55 +1000 Subject: [PATCH 3/3] fix(test): avoid race in sandbox savepoint test cleanup 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 --- test/ecto_sandbox_savepoint_test.exs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/test/ecto_sandbox_savepoint_test.exs b/test/ecto_sandbox_savepoint_test.exs index 794440e7..999eedb3 100644 --- a/test/ecto_sandbox_savepoint_test.exs +++ b/test/ecto_sandbox_savepoint_test.exs @@ -52,7 +52,18 @@ defmodule EctoLibSql.EctoSandboxSavepointTest do on_exit(fn -> Sandbox.stop_owner(owner) - if Process.alive?(pid), do: Supervisor.stop(pid) + + # Process.alive?/1 is a point-in-time check: stopping the owner above can take + # the repo supervisor down while we are still on this line, and Supervisor.stop/1 + # then exits with :noproc. Catch it rather than failing an otherwise passing test. + if Process.alive?(pid) do + try do + :ok = Supervisor.stop(pid) + catch + :exit, _ -> :ok + end + end + Process.sleep(50) for f <- Path.wildcard(test_db <> "*"), do: File.rm(f) end)