diff --git a/.gitignore b/.gitignore index 50898c0..890785d 100644 --- a/.gitignore +++ b/.gitignore @@ -5,6 +5,10 @@ # Build and dependency output /_build/ +# A second build tree for the Postgres adapter, which is chosen at compile time by TRINITY_DB and +# so cannot share _build with the SQLite one. `MIX_BUILD_ROOT=_build_pg TRINITY_DB=postgres` is how +# the postgres leg is reproduced locally. +/_build_pg/ /deps/ /cover/ /doc/ diff --git a/config/test.exs b/config/test.exs index 2ee2e1b..c5df85c 100644 --- a/config/test.exs +++ b/config/test.exs @@ -174,6 +174,17 @@ else # a write while another connection holds the write lock, because waiting there could deadlock. # No timeout affects that path. The raise is kept because it does help ordinary lock waiting, # but it is not the fix for what is recorded as R26 in docs/06-risk-register.md. + # + # And the line below does not reach a sandboxed test at all, which is worth knowing before + # trusting it. `Ecto.Adapters.SQL.Sandbox.post_checkout/3` begins each test's transaction with + # `handle_begin([mode: :transaction] ++ opts, ...)`, and `Exqlite.Connection` resolves the mode + # as `Keyword.get(options, :mode, state.default_transaction_mode)`, so the explicit option wins + # over this setting and `:transaction` is a plain DEFERRED `BEGIN`. Every sandboxed test is + # therefore a DEFERRED transaction, and one that reads before it writes is on R26's unguarded + # upgrade path with no timeout able to help. `default_transaction_mode` below still governs the + # transactions the application itself opens, which is why it stays. + # `test/trinity/repo/sandbox_transaction_mode_test.exs` asserts both dependency lines so this + # paragraph fails rather than rots. config :trinity, Trinity.Repo, database: Path.expand("../trinity_test.db", __DIR__), pool: Ecto.Adapters.SQL.Sandbox, @@ -260,6 +271,18 @@ config :trinity, :skills, # user_dir at a temporary directory first. pending_dir: Path.join(System.tmp_dir!(), "trinity-test-pending-skills"), watch: true, + # The shipped poll is every second (lib/trinity/skills/registry.ex) and it compares mtimes at + # one-second resolution, so its worst case is about two seconds, which is the whole of slice + # 040 AC3's bound. With no margin, the test asserting that bound failed intermittently on a + # loaded runner while the watcher was behaving exactly as designed. A shorter interval here + # leaves the mechanism under test, the fs_poll backend over mtimes, and takes the runner's + # scheduler out of the measurement. The shipped default is unchanged. + # One backend everywhere, so slice 040 AC3 measures the same mechanism on every machine. The + # gate's runners have no `inotifywait` and already take this path; a developer box takes the + # native one, and takes it badly when the machine is at its inotify instance ceiling, where the + # watch is never established and no edit ever arrives. + poll: true, + poll_interval_ms: 100, index_tokens: 338 # Slice 060: no MCP client boots from rows in the suite (each test starts the clients it diff --git a/lib/trinity/skills/registry.ex b/lib/trinity/skills/registry.ex index db91771..64ebee9 100644 --- a/lib/trinity/skills/registry.ex +++ b/lib/trinity/skills/registry.ex @@ -304,13 +304,30 @@ defmodule Trinity.Skills.Registry do # has no native backend for, poll once a second (AC3's 2 s still holds; the poll compares # mtimes at one-second resolution). `TRINITY_SKILLS_POLL=1` forces the poll, so the # fallback can be exercised on a machine that has inotifywait. + # + # The interval is configurable because AC3's bound and this default leave no margin. A poll + # every second over mtimes at one-second resolution has a worst case of about two seconds all + # by itself, which is the whole of the criterion, so on a loaded shared runner the test that + # asserts it failed intermittently while the product was behaving exactly as designed. The + # default is unchanged and is what ships; the suite sets a shorter one so that what it measures + # is the watcher noticing rather than the runner's scheduler. defp backend do - poll = [backend: :fs_poll, interval: 1_000] + poll = [backend: :fs_poll, interval: poll_interval_ms()] cond do System.get_env("TRINITY_SKILLS_POLL") == "1" -> poll + # Same switch, from configuration rather than the environment, so the suite can pin one + # backend. Which backend the registry picks otherwise depends on the machine: a developer + # box has `inotifywait` and the gate's runners do not, so AC3's test measured a different + # mechanism in each place, and on a box already at its inotify **instance** ceiling + # (`/proc/sys/fs/inotify/max_user_instances`, 128 here with 119 in use) the native backend + # obtains no watch and fails silently, which the test could only report as an edit that + # never arrived. + Keyword.get(Application.get_env(:trinity, :skills, []), :poll, false) -> + poll + match?({:unix, :linux}, :os.type()) -> if System.find_executable("inotifywait"), do: [], else: poll @@ -327,6 +344,9 @@ defmodule Trinity.Skills.Registry do defp watch?, do: Application.get_env(:trinity, :skills, []) |> Keyword.get(:watch, true) + defp poll_interval_ms, + do: Application.get_env(:trinity, :skills, []) |> Keyword.get(:poll_interval_ms, 1_000) + ## Helpers defp ensure_project(nil), do: :ok diff --git a/test/support/boot_isolation.ex b/test/support/boot_isolation.ex new file mode 100644 index 0000000..5310dad --- /dev/null +++ b/test/support/boot_isolation.ex @@ -0,0 +1,120 @@ +# SPDX-FileCopyrightText: Sudo Apt Holdings LLC +# SPDX-License-Identifier: Apache-2.0 +defmodule Trinity.BootIsolation do + @moduledoc """ + Gives a spawned boot its own databases, whichever adapter is compiled in. + + `test/trinity/regulated_boot_node_test.exs` boots Trinity in a separate OS process, because + `start/2` cannot be re-run in a VM where the application is already up. Such a child needs + databases that are **not** the suite's own: it writes a boot receipt, and six children appending + to the chain the suite is using is not isolation. + + Two things make that awkward, and this module exists to hold both in one place. + + **The database is named differently per adapter.** Under SQLite it is a file path; under + `TRINITY_DB=postgres` it is a name inside a URL. `config/test.exs` fixes either one and derives + neither from `XDG_DATA_HOME`, so pointing a child at its own data directory does not move its + database. The config is rewritten here instead, in whichever shape the compiled adapter uses. + + **Migrations do not run in the child.** `Trinity.Application`'s `skip_migrations?/0` is + `Code.ensure_loaded?(Mix)`, so under `mix run` the `Ecto.Migrator` child is skipped and a fresh + database would boot with no tables. `isolate!/1` therefore creates the storage and runs the + migrations itself, before the application starts. + + The sandbox pool is removed as well. A child is a real boot, not a sandboxed test, and nothing + in it owns a sandbox connection. So is the data-directory lock's pinned path, for the same reason + the database is renamed: `config/test.exs` fixes it per test partition rather than per process. + """ + + @repos [Trinity.Repo, Trinity.Repo.Receipts] + + @doc """ + Called **in the child**, with `--no-start`, before `Application.ensure_all_started/1`. + + Rewrites both repos' configuration to a database named after `tag`, creates it, and migrates it. + """ + @spec isolate!(String.t()) :: :ok + def isolate!(tag) when is_binary(tag) and tag != "" do + # The data-directory lock is pinned by `config/test.exs` to one path per test partition, so the + # suite holds it and every child would refuse to boot with `{:data_dir_held, ...}`. Dropping the + # override returns `Trinity.Application`'s `lock_dir/0` to `Trinity.Paths.ensure_data_dir/0`, + # which reads `XDG_DATA_HOME` — the variable the caller already sets per case. Under + # `MIX_ENV=dev` there was no override and this was free; it is the same class of problem as the + # pinned database, and it belongs here beside it rather than in six call sites. + Application.delete_env(:trinity, Trinity.DataDir.Lock) + + Enum.each(@repos, fn repo -> + config = rewrite(repo, tag) + Application.put_env(:trinity, repo, config) + + # `:already_up` is not an error here: a tag is unique per case, but a retried run of the same + # case reuses it, and a database that exists is exactly what was wanted. + _ = repo.__adapter__().storage_up(config) + + Ecto.Migrator.with_repo(repo, &Ecto.Migrator.run(&1, :up, all: true), pool_size: 2) + end) + end + + @doc """ + Called **in the parent**, from `on_exit`, to drop what a child created. + + Matters most under Postgres, where a leaked database outlives the run on a developer's machine. + Under SQLite the files sit in the case's temporary directory and go with it. + """ + @spec drop!(String.t()) :: :ok + def drop!(tag) when is_binary(tag) do + Enum.each(@repos, fn repo -> + _ = repo.__adapter__().storage_down(rewrite(repo, tag)) + end) + end + + @doc "The tag a case uses, unique per case and short enough to be a Postgres database name." + @spec tag(String.t()) :: String.t() + def tag(name) do + "trinity_boot_#{name}_#{System.unique_integer([:positive])}" + |> String.replace(~r/[^a-z0-9_]/, "_") + |> String.slice(0, 50) + end + + defp rewrite(repo, tag) do + config = Application.get_env(:trinity, repo, []) + name = "#{tag}_#{repo |> Module.split() |> List.last() |> String.downcase()}" + + config + # A child is a real boot. Nothing in it owns a sandbox connection. + |> Keyword.delete(:pool) + |> name_database(name) + end + + # Postgres: the database is a name inside a URL. The URL is replaced with explicit keys rather + # than patched, so there is no second source of truth for the same field. + defp name_database(config, name) do + case Keyword.get(config, :url) do + url when is_binary(url) and url != "" -> + uri = URI.parse(url) + {username, password} = userinfo(uri) + + config + |> Keyword.delete(:url) + |> Keyword.merge( + username: username, + password: password, + hostname: uri.host || "localhost", + port: uri.port || 5432, + database: name + ) + + _ -> + Keyword.put(config, :database, Path.join(System.tmp_dir!(), name <> ".db")) + end + end + + defp userinfo(%URI{userinfo: info}) when is_binary(info) do + case String.split(info, ":", parts: 2) do + [user, pass] -> {user, pass} + [user] -> {user, nil} + end + end + + defp userinfo(_), do: {"postgres", nil} +end diff --git a/test/test_helper.exs b/test/test_helper.exs index b64be77..be84b45 100644 --- a/test/test_helper.exs +++ b/test/test_helper.exs @@ -7,14 +7,6 @@ # Slice 003: :fips (tests that need FIPS mode on) runs only where the leg declares itself with # TRINITY_FIPS_LEG=1; excluded by tag elsewhere, never skipped. test/fips/mode_test.exs runs # everywhere and is what makes a leg that failed to enter the mode red rather than quiet. -# Slice regulated-boot: :needs_dev_compile (tests that spawn a child OS process running -# `mix run` under MIX_ENV=dev) is excluded on the FIPS leg alone. The leg has no prebuilt dev -# tree, so the child compiles the dev dependency tree from scratch, and that reaches the network -# to fetch the `tokenizers` precompiled NIF. OTP's TLS client cannot complete that download -# (HelloRetryRequest middlebox assertion, erlang/otp#8470, docs/fips-leg.md finding 2 and slice -# 002), so the child dies at `mix compile` and every assertion about what it booted is really an -# assertion about a failed download. Excluded here rather than skipped by hand, and only here: -# gate and postgres run the file in full. # Slice 032: :local_model (the real embedder through the serving) runs where # TRINITY_LOCAL_MODEL_CACHE names a cache that already holds all-MiniLM-L6-v2 (no download, # no network: a missing model is a failure there, not a skip); excluded by tag elsewhere. @@ -24,8 +16,7 @@ local_model? = System.get_env("TRINITY_LOCAL_MODEL_CACHE") not in [nil, ""] ExUnit.start( exclude: [:live, :desktop, :eval] ++ - if(fips_leg?, do: [:needs_dev_compile], else: [:fips]) ++ - if(local_model?, do: [], else: [:local_model]) + if(fips_leg?, do: [], else: [:fips]) ++ if(local_model?, do: [], else: [:local_model]) ) # Slice 024's boot receipt is written by a transient Task after the tree starts; once the diff --git a/test/trinity/regulated_boot_node_test.exs b/test/trinity/regulated_boot_node_test.exs index 2c7a81b..6e22d60 100644 --- a/test/trinity/regulated_boot_node_test.exs +++ b/test/trinity/regulated_boot_node_test.exs @@ -15,54 +15,73 @@ defmodule Trinity.RegulatedBootNodeTest do own data directory, its own `Trinity.DataDir.Lock` and its own database files and cannot collide with this suite or with another case. - `MIX_ENV=dev`, deliberately, and not `test`. The test environment puts both repos on - `Ecto.Adapters.SQL.Sandbox`, and a child process with no sandbox owner blocks on checkout rather - than booting: that is the trap slice 126 was opened for and then retracted over. - - `mix run --no-start`, so the script can set application environment and define a fixture module - **before** `Application.ensure_all_started/1` runs the code under test. + `MIX_ENV=test`, so the child reuses the test tree that is already built everywhere and reaches no + network. This was `MIX_ENV=dev`, which meant the child compiled the whole dev dependency tree + from scratch. That cost nothing on a developer machine with a warm `_build`, and on CI it cost + everything: on the FIPS leg the compile could not finish at all, because fetching the + `tokenizers` NIF needs a TLS handshake OTP cannot complete (erlang/otp#8470), and on the `gate` + leg it merely took too long, until a loaded runner pushed one case past its 180 s timeout. The + dev compile was the defect; the FIPS leg was only where it failed loudly. + + `Trinity.BootIsolation` gives each child its own databases. That is the problem `MIX_ENV=test` + brings with it and the reason it was rejected once: `config/test.exs` fixes the database and does + not derive it from `XDG_DATA_HOME`, so without this the six children would write into the suite's + own chain. Measured before the helper existed, a child appended its boot receipt at `seq 360` on + the live chain. The helper rewrites both repos' configuration in whichever shape the compiled + adapter uses, creates the storage, runs the migrations the child would otherwise skip, and drops + the sandbox pool, since a child is a real boot and owns no sandbox connection. Nothing here touches `:persistent_term` in this VM. Every mutation happens in the child. - ## Why this file does not run on the FIPS leg - - `MIX_ENV=dev` means the child needs a dev build. On a developer machine and on the `gate` and - `postgres` jobs one either exists or is cheap to produce. The FIPS leg has **no prebuilt dev - tree**, so the child compiles the whole dev dependency tree from scratch, and that reaches the - network to fetch the `tokenizers` precompiled NIF. OTP's TLS client cannot complete that - download: it fails the HelloRetryRequest middlebox assertion (erlang/otp#8470, recorded in - `docs/fips-leg.md` finding 2 and owned by slice 002). The child then dies at `mix compile`, and - every assertion here about what a node booted becomes an assertion about a failed download. - - So the module carries `:needs_dev_compile`, which `test/test_helper.exs` excludes **only** when - `TRINITY_FIPS_LEG=1`. `gate` and `postgres` run all six cases unchanged. This is a limit of the - harness on one leg, not a gap in the profile: nothing here was red because a refusal failed. - - `MIX_ENV=test` was tried first, since the test tree is already built everywhere and needs no - network. The child boots (`test_helper.exs` never runs, so the sandbox stays in `:auto` mode and - nothing blocks on checkout), but the database path is fixed by `config/test.exs` and is not - derived from `XDG_DATA_HOME`, so six children write into the suite's own database: measured, a - child appended its boot receipt at `seq 360` on the live chain. Overriding both repos' paths and - migrating inside the child fixes that for SQLite, and was measured working in 1.45 s with 25 and - 4 tables created and the boot receipt back at `seq 1`. It does not carry to the `postgres` job, - where `:database` names a database rather than a file and each case would need one provisioned - and dropped. That is not written here, because it could not be tested here. + ## Every leg runs all six cases + + There is no longer a leg this file skips. It carried `:needs_dev_compile`, excluded where + `TRINITY_FIPS_LEG=1`, for as long as the child needed a dev build; that tag and its exclusion are + gone with the dev compile that caused them. """ use ExUnit.Case, async: false - # Excluded on the FIPS leg alone. See "Why this file does not run on the FIPS leg" above. - @moduletag :needs_dev_compile @moduletag timeout: 180_000 - # A signer failure the child can reproduce: the key file exists as a directory, so `File.read/1` - # answers `{:error, :eisdir}` and `KeyCustody.boot!/1` answers `{:error, {:key_file, :eisdir}}`. - # `keys_dir` itself is a real directory, so nothing raises on the way there. + # Prepended to every child script, ahead of the case's own setup: its databases exist and are + # migrated before the application is asked to start. + @isolate """ + Trinity.BootIsolation.isolate!(System.fetch_env!("TRINITY_BOOT_TAG")) + """ + + # A signer failure the child can reproduce, on whichever algorithm the leg selects. + # + # The key is `receipts-.key` (`KeyCustody.key_path/2`) and the algorithm is `:p384` + # whenever `:crypto.info_fips()` is `:enabled`, else `:ed25519` (`KeyCustody.select/0`). This used + # to sabotage the ed25519 name alone, so on the FIPS leg it aimed at a file the signer never + # opens: the node generated a p384 key, booted, and case 6 failed on a correct assertion while + # case 1 passed for the wrong reason. Every candidate name is taken instead. + # + # Each is a **directory**, which is deliberate and not interchangeable with the simpler sabotage + # of making `keys_dir` itself a file. A directory where the key belongs gives `File.read/1` + # `{:error, :eisdir}`, which `KeyCustody` returns as `{:error, {:key_file, :eisdir}}` — an error + # tuple, on the path the receipts supervisor is fail-open over. A `keys_dir` that is a regular + # file instead makes `File.mkdir_p!/1` **raise** `%File.Error{reason: :enotdir}`, which takes the + # boot down under `:default` too and so cannot express "the signer is down and a default node + # still starts". Measured, not reasoned: that is filed as its own finding. defp broken_keys_dir(root) do dir = Path.join(root, "broken-keys") - File.mkdir_p!(Path.join(dir, "receipts-ed25519.key")) + + for algorithm <- ["ed25519", "p384", "mldsa87"] do + File.mkdir_p!(Path.join(dir, "receipts-#{algorithm}.key")) + end + dir end + # A case's own data directory and its own database name, with the databases dropped afterwards. + # The drop matters under Postgres, where a leaked database outlives the run. + defp case_setup(name) do + tag = Trinity.BootIsolation.tag(name) + on_exit(fn -> Trinity.BootIsolation.drop!(tag) end) + {tmp!(name), tag} + end + defp tmp!(name) do dir = Path.join(System.tmp_dir!(), "regboot-#{name}-#{System.unique_integer([:positive])}") File.mkdir_p!(dir) @@ -74,8 +93,7 @@ defmodule Trinity.RegulatedBootNodeTest do # halts, so the status distinguishes the two without parsing prose. defp boot(env, script) do base = [ - {"MIX_ENV", "dev"}, - {"TRINITY_FAKE_PROVIDER", "1"}, + {"MIX_ENV", "test"}, # Never inherit this suite's values for anything the code under test reads. {"TRINITY_PROFILE", nil}, {"TRINITY_AUTHORITY", nil}, @@ -83,7 +101,7 @@ defmodule Trinity.RegulatedBootNodeTest do {"TRINITY_MCP_AUTH_PROFILE", nil} ] - System.cmd("mix", ["run", "--no-start", "-e", script], + System.cmd("mix", ["run", "--no-start", "-e", @isolate <> script], env: base ++ env, stderr_to_stdout: true ) @@ -147,7 +165,7 @@ defmodule Trinity.RegulatedBootNodeTest do describe "1. the default profile boots with the signer down" do test "a broken key store does not stop a default node" do - root = tmp!("default-signer-down") + {root, tag} = case_setup("default-signer-down") script = """ r = Application.get_env(:trinity, :receipts, []) @@ -155,7 +173,7 @@ defmodule Trinity.RegulatedBootNodeTest do #{@report} """ - {out, status} = boot([{"XDG_DATA_HOME", root}], script) + {out, status} = boot([{"XDG_DATA_HOME", root}, {"TRINITY_BOOT_TAG", tag}], script) assert status == 0, """ @@ -172,7 +190,7 @@ defmodule Trinity.RegulatedBootNodeTest do describe "2. regulated refuses an unset endpoint allow-list" do test "production mcp_auth and a non-Local authority, but no allow-list, does not start" do - root = tmp!("regulated-no-endpoints") + {root, tag} = case_setup("regulated-no-endpoints") script = """ #{@fixture} @@ -184,6 +202,7 @@ defmodule Trinity.RegulatedBootNodeTest do boot( [ {"XDG_DATA_HOME", root}, + {"TRINITY_BOOT_TAG", tag}, {"TRINITY_PROFILE", "regulated"}, {"TRINITY_AUTHORITY", "RegulatedBootFixture"} ], @@ -205,7 +224,7 @@ defmodule Trinity.RegulatedBootNodeTest do describe "3. regulated refuses the local authority" do test "everything else correct, but TRINITY_AUTHORITY unset, does not start" do - root = tmp!("regulated-local-authority") + {root, tag} = case_setup("regulated-local-authority") script = """ #{production_mcp_auth()} @@ -217,6 +236,7 @@ defmodule Trinity.RegulatedBootNodeTest do boot( [ {"XDG_DATA_HOME", root}, + {"TRINITY_BOOT_TAG", tag}, {"TRINITY_PROFILE", "regulated"}, {"TRINITY_REGULATED_LLM_ENDPOINTS", "https://models.internal"} ], @@ -234,7 +254,7 @@ defmodule Trinity.RegulatedBootNodeTest do describe "4. regulated boots when every condition is met" do test "production mcp_auth, a non-Local authority, an allowed endpoint and a signer" do - root = tmp!("regulated-good") + {root, tag} = case_setup("regulated-good") script = """ #{@fixture} @@ -247,6 +267,7 @@ defmodule Trinity.RegulatedBootNodeTest do boot( [ {"XDG_DATA_HOME", root}, + {"TRINITY_BOOT_TAG", tag}, {"TRINITY_PROFILE", "regulated"}, {"TRINITY_AUTHORITY", "RegulatedBootFixture"}, {"TRINITY_REGULATED_LLM_ENDPOINTS", "https://models.internal"} @@ -269,13 +290,15 @@ defmodule Trinity.RegulatedBootNodeTest do describe "5. regulated refuses an MCP authorization profile that is not production" do test "personal and local are both refused, where case 4's configuration is otherwise met" do for profile <- ["personal", "local"] do - root = tmp!("regulated-mcp-#{profile}") + {root, tag} = case_setup("regulated_mcp_#{profile}") - # No `Application.put_env` for :mcp_auth here, deliberately. The variable goes through - # `config/runtime.exs`, which is the path an operator actually uses, and which raises on a - # value that is not one of the three. MIX_ENV=dev, so that block runs (it is skipped in - # :test, where the suite sets :mcp_auth itself). + # Set here rather than through `TRINITY_MCP_AUTH_PROFILE`. That variable is mapped by + # `config/runtime.exs`, inside a block guarded by `config_env() != :test`, so under the + # child's `MIX_ENV=test` it is read by nothing. Passing it would have looked like a test of + # the operator's path while actually testing an unset profile, and the case would have gone + # green on a node that refused for the default reason instead of the one named. script = """ + Application.put_env(:trinity, :mcp_auth, profile: :#{profile}) #{@fixture} #{allowed_models()} #{@report} @@ -285,8 +308,8 @@ defmodule Trinity.RegulatedBootNodeTest do boot( [ {"XDG_DATA_HOME", root}, + {"TRINITY_BOOT_TAG", tag}, {"TRINITY_PROFILE", "regulated"}, - {"TRINITY_MCP_AUTH_PROFILE", profile}, {"TRINITY_AUTHORITY", "RegulatedBootFixture"}, {"TRINITY_REGULATED_LLM_ENDPOINTS", "https://models.internal"} ], @@ -316,7 +339,7 @@ defmodule Trinity.RegulatedBootNodeTest do describe "6. regulated refuses to keep running when receipts cannot be appended" do test "case 4's configuration with case 1's broken key store does not start" do - root = tmp!("regulated-signer-down") + {root, tag} = case_setup("regulated-signer-down") # Exactly case 4, plus the one break case 1 survives. Nothing else differs, so a refusal here # is attributable to the key store and to the profile, and to nothing else. @@ -333,6 +356,7 @@ defmodule Trinity.RegulatedBootNodeTest do boot( [ {"XDG_DATA_HOME", root}, + {"TRINITY_BOOT_TAG", tag}, {"TRINITY_PROFILE", "regulated"}, {"TRINITY_AUTHORITY", "RegulatedBootFixture"}, {"TRINITY_REGULATED_LLM_ENDPOINTS", "https://models.internal"} diff --git a/test/trinity/repo/sandbox_transaction_mode_test.exs b/test/trinity/repo/sandbox_transaction_mode_test.exs new file mode 100644 index 0000000..f5d3f9c --- /dev/null +++ b/test/trinity/repo/sandbox_transaction_mode_test.exs @@ -0,0 +1,68 @@ +# SPDX-FileCopyrightText: Sudo Apt Holdings LLC +# SPDX-License-Identifier: Apache-2.0 +defmodule Trinity.Repo.SandboxTransactionModeTest do + @moduledoc """ + R26's residual half: `default_transaction_mode: :immediate` does **not** reach a sandboxed test. + + Slice 005 reproduced R26 and demonstrated the fix, deliberately outside the sandbox, because + "the sandbox wraps each test in one connection's transaction and cannot express two contending + writers" (`test/trinity/repo/sqlite_transaction_mode_test.exs`). `config/test.exs` then sets + `default_transaction_mode: :immediate` on both test repos, which reads as though the suite were + covered too. + + It is not, and this file is here so that sentence cannot quietly stay wrong. + + `Ecto.Adapters.SQL.Sandbox.post_checkout/3` begins the per-test transaction with + `conn_mod.handle_begin([mode: :transaction] ++ opts, conn_state)`. `Exqlite.Connection` resolves + the mode as `Keyword.get(options, :mode, state.default_transaction_mode)`, so an explicit + `:mode` in the options **wins over the configured default**, and `:transaction` maps to a plain + `BEGIN TRANSACTION`, which is DEFERRED. + + So every sandboxed test runs in a DEFERRED transaction. A test whose first statements read and + then write is doing exactly what R26 describes: the read makes the transaction a reader, the + write tries to upgrade, and if another connection has written in between SQLite answers + `SQLITE_BUSY` **without consulting the busy handler**, so neither the 30 s `busy_timeout` nor + the `:immediate` setting can help. `Trinity.Gateways.IdentitiesTest` is such a test and has + failed this way on CI (gate run 36742943477, while the other run on the same commit passed). + + This is a fact about a dependency, so it is asserted against the dependency's source rather than + described. If an upgrade changes either line, this fails and the reasoning above gets revisited + instead of rotting. + """ + use ExUnit.Case, async: true + + @sandbox "deps/ecto_sql/lib/ecto/adapters/sql/sandbox.ex" + @exqlite "deps/exqlite/lib/exqlite/connection.ex" + + test "the sandbox begins its per-test transaction with an explicit mode, and that mode is DEFERRED" do + sandbox = File.read!(@sandbox) + + assert sandbox =~ "handle_begin([mode: :transaction] ++ opts", + """ + #{@sandbox} no longer begins the sandbox transaction with `mode: :transaction`. + + That is the line this project's understanding of R26-inside-the-suite rests on. Re-read + it: if the sandbox now begins IMMEDIATE, or takes the mode from the repo's + configuration, then `default_transaction_mode: :immediate` in config/test.exs finally + does reach sandboxed tests, and the moduledoc here and the comment in config/test.exs + should say so. + """ + end + + test "an explicit :mode option wins over the configured default_transaction_mode" do + exqlite = File.read!(@exqlite) + + assert exqlite =~ "Keyword.get(options, :mode, state.default_transaction_mode)", + """ + #{@exqlite} no longer resolves the transaction mode by preferring an explicit `:mode` + option over the configured default. If the precedence has reversed, the sandbox's + `mode: :transaction` no longer overrides `default_transaction_mode: :immediate`. + """ + + # And `:transaction` is the plain BEGIN, not the IMMEDIATE one: the two are separate clauses + # and only one of them takes the write lock up front. + assert exqlite =~ ~s|:transaction when transaction_status == :idle ->| + assert exqlite =~ ~s|handle_transaction(:begin, "BEGIN TRANSACTION", state)| + assert exqlite =~ ~s|handle_transaction(:begin, "BEGIN IMMEDIATE TRANSACTION", state)| + end +end diff --git a/test/trinity/skills/registry_test.exs b/test/trinity/skills/registry_test.exs index 96f284e..3110782 100644 --- a/test/trinity/skills/registry_test.exs +++ b/test/trinity/skills/registry_test.exs @@ -136,15 +136,28 @@ defmodule Trinity.Skills.RegistryTest do started = System.monotonic_time(:millisecond) - assert Enum.find_value(1..100, fn _ -> - Process.sleep(20) - - if Skills.get("watched").description == "After.", - do: System.monotonic_time(:millisecond) - started - end) - |> then(fn ms -> - IO.puts("\nAC3: the edit was in the registry after #{ms} ms") - ms - end) <= 2_000 + # The poll window and the criterion used to be the same 2 000 ms: `1..100` with a 20 ms sleep + # is a 2 s budget, asserted against a 2 s bound, so there was no way to tell "it never + # arrived" from "it arrived late", and a slow scheduler turned one into the other. They are + # separate now. The loop waits far longer than the criterion so that a failure says which of + # the two happened, and the criterion is then asserted against the time actually measured. + latency_ms = + Enum.find_value(1..600, fn _ -> + Process.sleep(25) + + if Skills.get("watched").description == "After.", + do: System.monotonic_time(:millisecond) - started + end) + + assert latency_ms, + "the edit never reached the registry at all, in 15 s of waiting, and no rescan was " <> + "called. That is not AC3's 2 s bound being missed; it is the watcher not working." + + IO.puts("\nAC3: the edit was in the registry after #{latency_ms} ms") + + assert latency_ms <= 2_000, + "AC3 gives 2 000 ms and the edit took #{latency_ms} ms. The suite runs the fs_poll " <> + "backend at the interval config/test.exs sets, so this is the watcher being slow " <> + "rather than the poll interval leaving no margin." end end