Skip to content

Commit 06fdde2

Browse files
authored
Merge pull request #115 from ScriptKittyOS/fix/sandbox-write-lock
test: a sandboxed test takes the write lock before it reads
2 parents 5c1a9ba + 73fbe25 commit 06fdde2

3 files changed

Lines changed: 80 additions & 2 deletions

File tree

‎test/support/data_case.ex‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,12 +50,46 @@ defmodule Trinity.DataCase do
5050
# test writes inside the test's sandbox and the rows go with it.
5151
rpid = Ecto.Adapters.SQL.Sandbox.start_owner!(Trinity.Repo.Receipts, opts)
5252

53+
# SCR-376 and SCR-377: make each sandboxed transaction a writer before it reads anything.
54+
#
55+
# `Ecto.Adapters.SQL.Sandbox` begins every test's transaction with an explicit
56+
# `mode: :transaction`, which `Exqlite.Connection` resolves ahead of the configured
57+
# `default_transaction_mode: :immediate`, and which is a plain DEFERRED `BEGIN`. So slice 005's
58+
# fix does not reach the suite: a test that reads and then writes is upgrading, and SQLite
59+
# answers `SQLITE_BUSY` on an upgrade **without consulting the busy handler**, whatever
60+
# `busy_timeout` says. Five failures across three tables came from exactly that.
61+
#
62+
# A write that is the transaction's *first* statement is not an upgrade, so the handler is
63+
# consulted and the writer waits instead of failing. That is measured, not assumed:
64+
# `test/trinity/repo/sqlite_transaction_mode_test.exs` drives two connections through this
65+
# exact sequence and asserts both the success and that it genuinely contended.
66+
#
67+
# `WHERE 1 = 0` touches no row. The table is the repo's **own** migration table, read from its
68+
# configuration rather than assumed: `Trinity.Repo.Receipts` sets
69+
# `migration_source: "receipts_schema_migrations"` (config/config.exs), and hard-coding
70+
# `schema_migrations` here failed every test using this template, not only the ones that touch
71+
# receipts. The statement is here for the lock it takes, not for the rows it does not change.
72+
#
73+
# Cost: measured as negligible, because there is nothing to serialise. Every file using this
74+
# case template is `async: false` (the only `async: true` occurrence of it is this file's own
75+
# documentation), so sandboxed tests already run one at a time.
76+
Enum.each([Trinity.Repo, Trinity.Repo.Receipts], &take_write_lock/1)
77+
5378
on_exit(fn ->
5479
Ecto.Adapters.SQL.Sandbox.stop_owner(rpid)
5580
Ecto.Adapters.SQL.Sandbox.stop_owner(pid)
5681
end)
5782
end
5883

84+
# SQLite only. Postgres has no SQLITE_BUSY and no upgrade path to guard, so taking a lock there
85+
# would be a cost with no reason.
86+
defp take_write_lock(repo) do
87+
if repo.__adapter__() == Ecto.Adapters.SQLite3 do
88+
source = repo.config()[:migration_source] || "schema_migrations"
89+
Ecto.Adapters.SQL.query!(repo, "DELETE FROM #{source} WHERE 1 = 0", [])
90+
end
91+
end
92+
5993
@doc """
6094
A helper that transforms changeset errors into a map of messages.
6195

‎test/trinity/repo/sqlite_transaction_mode_test.exs‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,4 +86,40 @@ defmodule Trinity.Repo.SqliteTransactionModeTest do
8686
assert :ok = Sqlite3.execute(a, "INSERT INTO t (v) VALUES ('from a')")
8787
:ok = Sqlite3.execute(a, "COMMIT")
8888
end
89+
90+
# Added 2026-10-07 for SCR-376 and SCR-377. The two cases above settle the question slice 005
91+
# asked: DEFERRED-then-upgrade cannot wait, IMMEDIATE can. Neither answers the question the
92+
# **suite** poses, because `Ecto.Adapters.SQL.Sandbox` begins every test's transaction with an
93+
# explicit `mode: :transaction`, which is a plain DEFERRED `BEGIN` and overrides
94+
# `default_transaction_mode: :immediate`. The sandbox's BEGIN is not ours to change.
95+
#
96+
# So: inside a transaction that is already DEFERRED, does issuing a **write as the first
97+
# statement** — no read, therefore no upgrade — reach the busy handler? If it does, a sandboxed
98+
# test can be made a writer at its start and the five Database busy failures lose their
99+
# mechanism. If it does not, there is no mitigation at this layer and saying so is the finding.
100+
test "DEFERRED, first statement a write: the handler is consulted, because this is not an upgrade",
101+
%{a: a, b: b} do
102+
:ok = Sqlite3.execute(b, "BEGIN IMMEDIATE")
103+
:ok = Sqlite3.execute(b, "INSERT INTO t (v) VALUES ('from b')")
104+
105+
:ok = Sqlite3.execute(a, "BEGIN DEFERRED")
106+
107+
started = System.monotonic_time(:millisecond)
108+
task = Task.async(fn -> Sqlite3.execute(a, "INSERT INTO t (v) VALUES ('from a')") end)
109+
Process.sleep(200)
110+
:ok = Sqlite3.execute(b, "COMMIT")
111+
result = Task.await(task, 5_000)
112+
elapsed = System.monotonic_time(:millisecond) - started
113+
114+
assert :ok = result,
115+
"a first-statement write in a DEFERRED transaction was refused rather than waiting. " <>
116+
"Then taking the write lock at the start of a sandboxed test is not a mitigation for " <>
117+
"SCR-377, and the fix has to be somewhere else."
118+
119+
assert elapsed >= 200,
120+
"it returned in #{elapsed} ms, before the holder committed at 200 ms, so it never " <>
121+
"contended and this case proves nothing"
122+
123+
:ok = Sqlite3.execute(a, "COMMIT")
124+
end
89125
end

‎test/trinity/sandbox/sandbox_test.exs‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -123,8 +123,16 @@ defmodule Trinity.SandboxTest do
123123
test "what survives is named rather than discovered: os.time and os.clock still work" do
124124
# Not a hole, and not an accident either. These reach no resource, but they are
125125
# non-deterministic, which matters when a script's output can reach a receipt.
126-
assert {:ok, [_], _} = Sandbox.run("return os.time()")
127-
assert {:ok, [_], _} = Sandbox.run("return os.clock()")
126+
#
127+
# SCR-378: the limit is named here, generously, and that is not a weakening. What this test
128+
# asserts is that the two functions are **available**; the default wall clock it used to
129+
# inherit was incidental to that, and on a loaded runner a trivial call exceeded it and the
130+
# test failed for a reason it was not about. The limit's own binding is proven on its own, by
131+
# "an infinite loop is killed on wall clock and the caller is told which limit bound", which
132+
# passes its own `max_time_ms: 200` and asserts the refusal by value. A budget asserted in two
133+
# places is a budget that gets loosened in the wrong one.
134+
assert {:ok, [_], _} = Sandbox.run("return os.time()", max_time_ms: 30_000)
135+
assert {:ok, [_], _} = Sandbox.run("return os.clock()", max_time_ms: 30_000)
128136
end
129137
end
130138
end

0 commit comments

Comments
 (0)