From 6bb2a5476bc9ded26719cf623209304fb09d3e65 Mon Sep 17 00:00:00 2001 From: Ayla Croft Date: Wed, 30 Sep 2026 16:04:10 -0400 Subject: [PATCH 1/3] fix: slice 040 AC3 measures the watcher, not the machine The skills AC3 test was red on one machine and green on CI, and the reason was different in each place. Neither was the watcher misbehaving. On CI there is no inotifywait, so the registry takes the fs_poll backend: a poll every second over mtimes at one-second resolution, whose worst case is about two seconds. AC3's bound is two seconds. There was no margin at all, and a loaded shared runner closed it. Measured here, with the backend forced: interval 1000 ms (shipped) 1222 ms, 1223 ms interval 100 ms (suite) 339 ms, 312 ms, 338 ms The interval is now configurable and the suite sets 100 ms. The shipped default is unchanged: a desktop does not need a tenth-of-a-second poll, and the criterion it has to meet is the same either way. The suite also pins the backend. Which one the registry picks otherwise depends on the machine, so the test measured a different mechanism in each place. Worse, on a box already at its inotify instance ceiling (128 here, 119 in use) the native backend obtains no watch and fails silently: the test then fails for a reason that has nothing to do with the product, and the old assertion could only report it as AC3: the edit was in the registry after ms because the poll window and the criterion were the same 2000 ms budget and an exhausted loop was indistinguishable from a late arrival. They are separate now. The loop waits 15 s so a failure says which of the two happened, and the criterion is asserted against the latency actually measured. Verified on the machine where it was failing: three runs, 338, 338 and 312 ms, and a full gate at 951 passed. R26 inside the sandbox, which is the other test Gateways.IdentitiesTest fails on CI with Database busy on an insert, and config/test.exs sets default_transaction_mode: :immediate on both test repos, which reads as though slice 005 had covered the suite. It has not. 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 the configuration and :transaction is a plain DEFERRED BEGIN. Every sandboxed test is a DEFERRED transaction, and one that reads before it writes is on R26's unguarded upgrade path, where no busy_timeout applies. Identities.admit/3 reads and then inserts, with no transaction of its own. That is diagnosis, not a fix, and it is recorded as such: the comment in config/test.exs no longer implies the suite is covered, and test/trinity/repo/sandbox_transaction_mode_test.exs asserts both dependency lines so the paragraph fails rather than rots if an upgrade moves them. The identities test is not changed, because the concurrent writer that completes the mechanism is still unidentified and the failure does not reproduce here. Signed-off-by: Ayla Croft --- config/test.exs | 23 +++++++ lib/trinity/skills/registry.ex | 22 +++++- .../repo/sandbox_transaction_mode_test.exs | 68 +++++++++++++++++++ test/trinity/skills/registry_test.exs | 33 ++++++--- 4 files changed, 135 insertions(+), 11 deletions(-) create mode 100644 test/trinity/repo/sandbox_transaction_mode_test.exs 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/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 From 3618d5a2baa5a067af4ff47a31c210d72117e278 Mon Sep 17 00:00:00 2001 From: Ayla Croft Date: Wed, 7 Oct 2026 15:09:19 -0400 Subject: [PATCH 2/3] test: the spawned boot cases need no dev build, so every leg runs them The six cases ran a child under MIX_ENV=dev, which meant each child compiled the whole dev dependency tree from scratch. On a developer machine with a warm _build that cost nothing. On CI it cost everything, in two different ways, and only one of them was diagnosed at the time: the FIPS leg could not finish the compile at all, because fetching the tokenizers NIF needs a TLS handshake OTP cannot complete (erlang/otp#8470); the gate leg merely took too long, until a loaded runner pushed case 6 past its 180 s timeout and failed PR #113 while the other run of the same sha passed. The dev compile was the defect. The FIPS leg was where it failed loudly, and :needs_dev_compile treated that symptom by skipping the file on one leg. The tag and its exclusion in test_helper.exs are both gone. The child now runs MIX_ENV=test, reusing the tree the suite is already running from: no compile, no network, on every leg. Trinity.BootIsolation is what makes that safe, and it is why MIX_ENV=test was rejected once. config/test.exs pins both the database and the data-directory lock, and derives neither from XDG_DATA_HOME, so without it six children would write into the suite's own chain and then refuse to boot on a lock the suite holds. Measured before the helper existed: a child appended its boot receipt at seq 360 on the live chain. The helper renames both repos' databases in whichever shape the compiled adapter uses, creates the storage, runs the migrations the child would otherwise skip (skip_migrations?/0 is Code.ensure_loaded?(Mix)), drops the sandbox pool because a child is a real boot, and releases the pinned lock path so XDG_DATA_HOME isolation works as the moduledoc always claimed. The Postgres half is the part I previously declined to write because I could not test it. It is now tested, against pgvector/pgvector:pg17 on 5433 with a separate MIX_BUILD_ROOT=_build_pg tree: mix test test/trinity/regulated_boot_node_test.exs 6 passed, 9.9 s mix test --exclude sqlite 931 passed, exit 0 and the per-case databases are dropped afterwards rather than leaked: after the run, pg_database holds no trinity_boot% row and trinity_test is untouched. On SQLite: 6 passed in 8.0 s, and mix gate 951 passed, exit 0. Case 5 changed with the environment. It set TRINITY_MCP_AUTH_PROFILE and relied on config/runtime.exs to map it, but that block is guarded by config_env() != :test, so under the child's MIX_ENV=test the variable is read by nothing. Left as it was, the case would have gone green on a node refusing for the default reason rather than the one under test. It now sets :mcp_auth directly and says why. _build_pg is added to .gitignore: the adapter is chosen at compile time, so the Postgres build cannot share a tree with the SQLite one. Signed-off-by: Ayla Croft --- .gitignore | 4 + test/support/boot_isolation.ex | 120 ++++++++++++++++++++++ test/test_helper.exs | 11 +- test/trinity/regulated_boot_node_test.exs | 100 +++++++++--------- 4 files changed, 179 insertions(+), 56 deletions(-) create mode 100644 test/support/boot_isolation.ex 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/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..ee5a9d8 100644 --- a/test/trinity/regulated_boot_node_test.exs +++ b/test/trinity/regulated_boot_node_test.exs @@ -15,45 +15,40 @@ 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 + # 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: 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. @@ -63,6 +58,14 @@ defmodule Trinity.RegulatedBootNodeTest do 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 +77,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 +85,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 +149,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 +157,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 +174,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 +186,7 @@ defmodule Trinity.RegulatedBootNodeTest do boot( [ {"XDG_DATA_HOME", root}, + {"TRINITY_BOOT_TAG", tag}, {"TRINITY_PROFILE", "regulated"}, {"TRINITY_AUTHORITY", "RegulatedBootFixture"} ], @@ -205,7 +208,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 +220,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 +238,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 +251,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 +274,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 +292,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 +323,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 +340,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"} From 602e468dfa9af80e50c4ad9f963b588009a7d26d Mon Sep 17 00:00:00 2001 From: Ayla Croft Date: Wed, 7 Oct 2026 15:24:49 -0400 Subject: [PATCH 3/3] test: the signer sabotage was aimed at a file the FIPS signer never opens Removing the FIPS exclusion let that leg run these cases for the first time, and it immediately found a defect in the fixture rather than in the profile. 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). The fixture created a directory named receipts-ed25519.key, so on the FIPS leg it sabotaged a path the signer never opens: the node generated a p384 key, booted, and case 6 failed on an assertion that was correct. Case 1 passed there for the wrong reason, since it expects a boot either way. Every candidate name is now taken. Verified on both algorithms rather than reasoned about, by forcing config :trinity, :receipts, algorithm: and reading the refusal: selected: {:ok, :ed25519} BOOT_REFUSED {:regulated_boot_refused, {:regulated_requires_receipts, {:key_file, :eisdir}}} selected: {:ok, :p384} BOOT_REFUSED {:regulated_boot_refused, {:regulated_requires_receipts, {:key_file, :eisdir}}} Each candidate is a directory, which is not interchangeable with the simpler sabotage of making keys_dir itself a regular file. A directory where the key belongs gives File.read/1 {:error, :eisdir}, which KeyCustody returns as 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}, and that takes the boot down under :default as well, so it cannot express "the signer is down and a default node still starts". Measured while trying it: BOOT_REFUSED {:trinity, {{:shutdown, {:failed_to_start_child, Trinity.Receipts.Supervisor, {%File.Error{reason: :enotdir, ... That is a narrower fail-open guarantee than the supervisor's own documentation claims, it is a product observation rather than a test one, and it is filed as its own finding rather than fixed here. receipts/supervisor.ex is untouched. SQLite: 6 passed, and mix gate 951 passed, exit 0. Postgres: 6 passed, 10.7 s, against pgvector:pg17 on 5433. Signed-off-by: Ayla Croft --- test/trinity/regulated_boot_node_test.exs | 24 +++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/test/trinity/regulated_boot_node_test.exs b/test/trinity/regulated_boot_node_test.exs index ee5a9d8..6e22d60 100644 --- a/test/trinity/regulated_boot_node_test.exs +++ b/test/trinity/regulated_boot_node_test.exs @@ -49,12 +49,28 @@ defmodule Trinity.RegulatedBootNodeTest do Trinity.BootIsolation.isolate!(System.fetch_env!("TRINITY_BOOT_TAG")) """ - # 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. + # 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