diff --git a/README.md b/README.md index c62af20..78e989d 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,40 @@ crash semantics. This package is that loop, packaged. ## Status -Nothing is implemented yet. This repository holds the scaffold only. +Pre-release, under active development. The storage-adapter behaviour with +its identity guard, the in-memory reference adapter, the run lifecycle, +and the Ecto layer (configurable keys/tables, versioned migrations, and +the Postgres adapter below) exist; nothing is published to Hex yet. + +## The Ecto adapter + +Configure a persistence module on your own repo once, and migrate: + + defmodule MyApp.Persistence do + use StatifierPersistence.Ecto, repo: MyApp.Repo + end + + defmodule MyApp.Repo.Migrations.AddStatifierPersistence do + use Ecto.Migration + def up, do: StatifierPersistence.Ecto.Migrations.up(for: MyApp.Persistence) + def down, do: StatifierPersistence.Ecto.Migrations.down(for: MyApp.Persistence) + end + +then build the guarded store the rest of the package works through: + + {:ok, store} = + StatifierPersistence.Storage.new( + StatifierPersistence.Storage.Ecto, + persistence: MyApp.Persistence + ) + +The adapter passes the same conformance suite the in-memory reference +does (`StatifierPersistence.Testing.StorageConformance` - point it at +your own adapter to hold it to the identical bar), stores engine +identities verbatim, and implements the optional per-run `lock_run/3` +as a transaction-scoped advisory-plus-row lock (ADR-0004 as amended). +In your test suite, pass `sandbox: true` so each test runs in its own +`Ecto.Adapters.SQL.Sandbox` checkout via the adapter's `isolate/1`. ## Running the tests diff --git a/changelog.d/sp-4an.3.1.md b/changelog.d/sp-4an.3.1.md new file mode 100644 index 0000000..6623fc2 --- /dev/null +++ b/changelog.d/sp-4an.3.1.md @@ -0,0 +1,17 @@ +# sp-4an.3.1 + +## Added + +- `StatifierPersistence.Storage.Ecto`: the Postgres storage adapter over + the schemas a host generates with `use StatifierPersistence.Ecto` + (`Storage.new(Storage.Ecto, persistence: MyApp.Persistence)`). Passes + the same conformance suite as the in-memory reference adapter; engine + identities stored verbatim; `:run_exists` enforced atomically by the + unique index. +- `Storage.Ecto.isolate/1`: with `sandbox: true`, wraps each test in its + own `Ecto.Adapters.SQL.Sandbox` checkout - the hook host test suites + (and this package's conformance suite) isolate through. +- `Storage.Ecto.lock_run/3`: per-run mutual exclusion as a + transaction-scoped `pg_advisory_xact_lock` plus a `SELECT ... FOR + UPDATE` row lock (ADR-0004 as amended), consumed by + `Serialization.AdapterLock`. diff --git a/docs/adr/0004-run-lifecycle-executor-seam-and-serialization.md b/docs/adr/0004-run-lifecycle-executor-seam-and-serialization.md index 4638f81..3cf11d7 100644 --- a/docs/adr/0004-run-lifecycle-executor-seam-and-serialization.md +++ b/docs/adr/0004-run-lifecycle-executor-seam-and-serialization.md @@ -177,3 +177,22 @@ a host decision about the run, not a chart transition. - The Ecto adapter (sp-4an.3) inherits three run callbacks, two error arms, and an optional `lock_run/3` to implement as a transaction-scoped row lock, all conformance-tested through the sp-4an.1 suite. + +## Amendment (2026-08-22, sp-4an.3.1): the Ecto lock is advisory plus row + +Decision 5 (and the Consequences bullet above) named the Ecto adapter's +`lock_run/3` a transaction-scoped row lock. Implementing it showed the +row lock alone cannot honor the callback's contract: `SELECT ... FOR +UPDATE` excludes nothing when no run row matches, and the contract (with +the conformance suite's lock tests) requires mutual exclusion for a +`run_id` that has not been inserted yet. + +So the Ecto adapter's transaction takes +`pg_advisory_xact_lock(hashtextextended(run_id, 0))` first - +unconditional per-run exclusion, row or no row - and then locks the run +row with `SELECT ... FOR UPDATE` when it exists, keeping this decision's +ordering against the row itself. Both are transaction-scoped, so any +exit from `fun` (a raise included) releases them with the transaction. +The rowless hole and the fix are pinned by a live two-connection test +outside the SQL sandbox, whose single shared connection would otherwise +serialize the callers by ownership and mask a broken lock. diff --git a/docs/plans/260822-sp-4an.3.1-ecto-storage-adapter.md b/docs/plans/260822-sp-4an.3.1-ecto-storage-adapter.md new file mode 100644 index 0000000..284b0e0 --- /dev/null +++ b/docs/plans/260822-sp-4an.3.1-ecto-storage-adapter.md @@ -0,0 +1,226 @@ +# Ecto storage adapter (sp-4an.3.1) Implementation Plan + +## Overview + +Implement `StatifierPersistence.Storage.Ecto`: the +`StatifierPersistence.Storage.Adapter` behaviour over the schemas a host +generates with `use StatifierPersistence.Ecto` (sp-02x, merged as PR #13), +passing the identical conformance suite `InMemory` passes, against real +Postgres per ADR-0005. Bead: sp-4an.3.1 (second task of the sp-4an.3 epic). + +## Current State Analysis + +- The adapter contract is fixed and conformance-tested: + `lib/statifier_persistence/storage/adapter.ex` (seven required callbacks, + optional `isolate/1` and `lock_run/3`), + `lib/statifier_persistence/testing/storage_conformance.ex` (the suite, + lock tests generated only when the adapter exports `lock_run/3`), + `lib/statifier_persistence/storage/in_memory.ex` (the reference). +- The Ecto layer under it is on main: `use StatifierPersistence.Ecto` + generates `Chart`/`Position`/`Run` schema modules from a resolved + `Config`; `StatifierPersistence.Ecto.Migrations` V01 creates the tables + with unique indexes on `content_hash` / `session_id` / `run_id` + (`lib/statifier_persistence/ecto/migrations/v01.ex`). Primary keys + autogenerate through the schemas, so `Repo.insert/2` works under every + key scheme, `bigserial` included (`read_after_writes`). +- The Postgres harness is live (ADR-0005): `TestRepo` on the SQL sandbox in + `:manual` mode, `docker compose up -d db`, storage_up in + `test/test_helper.exs`. `mix quality` requires the reachable server. +- No table exists for the `EctoHosts.Default` host yet at test start - the + live migration tests create and drop only their own `kx_*` tables. + +### Key Discoveries + +- `runs.status` is a `text` column; the contract's `run_status` is an atom + (`:active | :completed | :failed`). The adapter owns an explicit two-way + mapping - no `String.to_atom` on database bytes. +- `insert_run/2`'s `:run_exists` refusal must be atomic with the write + (adapter.ex contract). The V01 unique index on `run_id` is the mechanism; + a changeset `unique_constraint` (index name `_run_id_index`, the + `unique_index/2` default) maps the violation to `{:error, :run_exists}` + without a raise and without check-then-insert. +- `update_run/2` must refuse a missing `run_id` atomically too: + `Repo.update_all/3` on `run_id` returns the match count - `{0, _}` is + `:run_not_found`, no separate existence read. +- The conformance suite locks run_ids that were **never inserted** + ("run-conformance-lock"). A pure `SELECT ... FOR UPDATE` row lock excludes + nothing when no row matches, so ADR-0004's row-lock spelling alone cannot + satisfy the unconditional mutual-exclusion contract in + `adapter.ex` (`lock_run/3`: "no interleaving ... for the same run_id"). + Resolution (this repo owns locking per the cross-repo ownership table): + inside the transaction, first `pg_advisory_xact_lock(hashtextextended( + run_id, 0))`, then `SELECT ... FOR UPDATE` on the run row when it exists. + The advisory lock makes exclusion unconditional; the row lock keeps + ADR-0004's ordering against the run row itself. Both are + transaction-scoped, so any exit from `fun` - raise included - releases + them with the transaction. Recorded as a dated amendment to ADR-0004. +- Under the SQL sandbox both conformance lock tests pass through connection + ownership alone (both tasks share the owner's one connection, and + `$callers` grants the allowance), which proves nothing about the SQL. + The real semantics need a live test in `:auto` mode with two pooled + connections - the same pattern `migrations_test.exs` already uses. + +## Desired End State + +```elixir +{:ok, store} = + StatifierPersistence.Storage.new( + StatifierPersistence.Storage.Ecto, + persistence: MyApp.Persistence + ) +``` + +behaves exactly like the same call with `InMemory`: every facade path +(save/load chart and position, insert/fetch/update run, +`update_run_status/4`, `load_run_position/3`) works over Postgres, engine +identities verbatim, blobs byte-identical, identity guard on every load, +and `Serialization.AdapterLock` gets real per-run mutual exclusion from +`lock_run/3`. The conformance suite runs green against it in this repo's +own gate, lock tests included. + +## What We're NOT Doing + +- **No stepper/lifecycle changes.** `Runs`, `Executor`, `Serialization` + land untouched; the adapter slots under them through the existing seams. +- **No new facade surface.** `update_run_status/4` already rides + `fetch_run` + `update_run`; the adapter only implements the behaviour. +- **No listing/querying API** (runs by status, etc.) - no consumer asks for + it yet; unexercised contract. +- **No tenancy columns, no session_id population on runs** - unchanged + from sp-02x's scope cuts. +- **No `.quality.exs` edits** (campaign consent excludes them). +- **No backend-failure taxonomy.** Like `InMemory`, `{:adapter, term()}` + is produced where init can observe a failure; a dead database mid-call + raises (DBConnection's own exception), which the loop above treats as a + crash, not a value. Wrapping every call is scope for a later bead if a + host asks. + +## Implementation Approach + +Three phases, each independently gate-green and committable. Phase 1 is +the adapter's CRUD surface plus `isolate/1`, passing the full conformance +suite minus the lock tests (they generate only once `lock_run/3` exists). +Phase 2 adds `lock_run/3`, which makes the conformance lock tests +generate, plus the live two-connection proof and the ADR-0004 amendment. +Phase 3 is user-facing docs and the changelog fragment. + +### Phase 1: adapter CRUD, isolate, conformance green + +**Files:** +- `lib/statifier_persistence/storage/ecto.ex` (new, inside + `if Code.ensure_loaded?(Ecto)`): `@behaviour Storage.Adapter`. + - `init/1`: requires `persistence:` (a module exporting + `__statifier_persistence__/1`); refusal is + `{:error, {:adapter, {:not_a_persistence_host, module}}}`. Resolves + and stores `repo`, the three schema modules, and the runs table name + (for the constraint name) into the returned opts. Optional + `sandbox: true` opts flag consumed by `isolate/1`. + - `save_chart/2`: `Repo.insert(struct, on_conflict: :nothing, + conflict_target: [:content_hash])` - idempotent by construction. + - `save_position/2`: upsert, + `on_conflict: {:replace, [:content_hash, :identity_blob, + :position_blob, :updated_at]}, conflict_target: [:session_id]`. + - `fetch_chart/2`, `fetch_position/2`, `fetch_run/2`: `Repo.get_by/3`, + `nil` mapped to the contract's not-found arm, struct mapped to the + plain record map (status decoded through the explicit mapping). + - `insert_run/2`: `Ecto.Changeset.change/1` + + `unique_constraint(:run_id, name: "_run_id_index")`; + `{:error, changeset}` with that constraint error becomes + `{:error, :run_exists}`. + - `update_run/2`: one `Repo.update_all/3` setting every contract field + plus `updated_at`; `{0, _}` is `{:error, :run_not_found}`. + - `isolate/1`: `Ecto.Adapters.SQL.Sandbox.checkout(repo)` when + `sandbox: true` (tolerating `{:already, _}`), no-op `:ok` otherwise. +- `test/test_helper.exs`: after `start_link`, before `:manual`, run the + V01 migration for `EctoHosts.Default` **and** `EctoHosts.Overridden` + through `Ecto.Migrator` (fixed versions, idempotent on `:already_up`) + so the default `statifier_*` tables and the prefixed + `workflows.wf_*`/`workflows.workflow_runs` tables exist for the whole + suite. +- `test/statifier_persistence/storage/ecto_conformance_test.exs` (new): + `use StatifierPersistence.Testing.StorageConformance, + adapter: Storage.Ecto, opts: [persistence: EctoHosts.Default, + sandbox: true], async: true`. +- `test/statifier_persistence/storage/ecto_test.exs` (new): adapter-level + unit tests the conformance suite does not cover - `init/1` refusing a + non-host module, all three statuses round-tripping, `save_chart` + repeated write not bumping the row count, and the same CRUD mechanisms + (`save_chart`/`insert_run` upsert, uniqueness, `update_run`) exercised + once against `EctoHosts.Overridden` so the `@schema_prefix`/renamed- + table path (`workflows.workflow_runs`) is covered, not only the + zero-config host. Each with a sabotage note (mutation verified red, + reverted). + +**Success criteria (automated):** full `mix quality` green (the 22 +non-lock conformance tests run against the Ecto adapter); `mix +gate.verify`. +**Manual:** none. + +### Phase 2: lock_run/3, live proof, ADR-0004 amendment + +**Files:** +- `lib/statifier_persistence/storage/ecto.ex`: `lock_run/3` - + `repo.transaction(fn -> advisory xact lock on + hashtextextended(run_id, 0); SELECT ... FOR UPDATE on the run row (zero + rows is fine); {:ok tail} fun.() end)`; result `{:ok, fun_result}`; a + raising `fun` rolls back and propagates (locks release with the + transaction). +- `test/statifier_persistence/storage/ecto_conformance_test.exs`: the two + lock tests now generate automatically - no edit, but the run count + changes. The generation gate is a compile-time + `function_exported?(adapter, :lock_run, 3)` inside the `use` expansion, + and nothing in this repo has yet proven Mix recompiles the test file + when the adapter gains the export. **Verify the tests actually appear** + (`mix test test/statifier_persistence/storage/ecto_conformance_test.exs + --trace` and see both "lock_run/3" tests listed); if they do not, + force the regeneration (`touch` the test file or `mix compile + --force`) before treating the phase as green - a silently smaller + suite still passes. +- `test/statifier_persistence/storage/ecto_live_lock_test.exs` (new, + `async: false`, sandbox `:auto` for the module like + `migrations_test.exs`): with two real pooled connections, (a) two + concurrent `lock_run/3` bodies on an **inserted** run never overlap + (the FOR UPDATE row path), (b) same for a run_id **never inserted** + (the advisory path), (c) a raising `fun` releases the lock and a fresh + `lock_run/3` reacquires. Rows cleaned up after the module. Sabotage + notes on each. +- `docs/adr/0004-run-lifecycle-executor-seam-and-serialization.md`: dated + amendment under decision 5 recording the advisory-lock-plus-row-lock + shape and why (unconditional exclusion for a run_id with no row yet). +- `lib/statifier_persistence/storage/adapter.ex`: the `lock_run/3` doc's + "implements it as a transaction-scoped row lock" sentence gains the + advisory clause so the doc and the amendment agree. + +**Success criteria (automated):** full `mix quality` green (conformance +now 24 tests against Ecto, the two lock tests confirmed present per the +verification note above; live lock tests green); `mix gate.verify`. +**Manual:** none. + +### Phase 3: docs and changelog + +**Files:** +- `README.md`: an "Ecto adapter" subsection - the + `Storage.new(Storage.Ecto, persistence: MyApp.Persistence)` call, the + migration prerequisite, the sandbox note for host test suites. +- `changelog.d/sp-4an.3.1.md`: Added - `StatifierPersistence.Storage.Ecto` + (conformance-passing Postgres adapter; `isolate/1` sandbox hook; + `lock_run/3` transaction-scoped locking behind + `Serialization.AdapterLock`). + +**Success criteria:** docs-only diff, reviewed; gate untouched by content +(run the full gate anyway before the final commit per campaign rules). + +## Testing Strategy + +The conformance suite is the bar and is not edited (except that lock +tests self-generate). New tests: adapter unit tests (Phase 1) and the +live two-connection lock tests (Phase 2), each sabotage-verified per the +repo convention. The live tests exist precisely because the sandbox +serializes on one connection and would mask a broken lock. + +## References + +- ADR-0003 (behaviour + identity guard), ADR-0004 (runs vocabulary, lock), + ADR-0005 (Postgres harness), ADR-0002 (keys/tables). +- `docs/plans/260822-sp-02x-keys-tables-migrations.md` - phase/gate + discipline mirrored here. diff --git a/lib/statifier_persistence/storage/adapter.ex b/lib/statifier_persistence/storage/adapter.ex index effe4b1..ba97d0f 100644 --- a/lib/statifier_persistence/storage/adapter.ex +++ b/lib/statifier_persistence/storage/adapter.ex @@ -238,9 +238,12 @@ defmodule StatifierPersistence.Storage.Adapter do This is the callback the default serialization strategy (`StatifierPersistence.Serialization.AdapterLock`) delegates to; an adapter that does not export it makes that strategy refuse with - `{:error, {:serialization, :not_supported}}`. An Ecto adapter implements - it as a transaction-scoped row lock - `SELECT ... FOR UPDATE` on the run - row inside a transaction that spans `fun` (sp-4an.3). + `{:error, {:serialization, :not_supported}}`. The Ecto adapter + implements it as a transaction-scoped advisory lock plus a + `SELECT ... FOR UPDATE` row lock inside a transaction that spans `fun` + (ADR-0004 decision 5 as amended 2026-08-22) - the advisory half exists + because a row lock alone excludes nothing for a `run_id` whose run has + not been inserted yet. """ @callback lock_run(opts(), run_id(), (-> result)) :: {:ok, result} | {:error, error()} diff --git a/lib/statifier_persistence/storage/ecto.ex b/lib/statifier_persistence/storage/ecto.ex new file mode 100644 index 0000000..5ffcf31 --- /dev/null +++ b/lib/statifier_persistence/storage/ecto.ex @@ -0,0 +1,330 @@ +if Code.ensure_loaded?(Ecto) do + defmodule StatifierPersistence.Storage.Ecto do + @moduledoc """ + The Ecto `StatifierPersistence.Storage.Adapter`: the storage contract + over the schemas a host generates with `use StatifierPersistence.Ecto` + (ADR-0002), against the tables the versioned migrations helper + creates. Requires the optional `ecto_sql` dependency (ADR-0005). + + defmodule MyApp.Persistence do + use StatifierPersistence.Ecto, repo: MyApp.Repo + end + + {:ok, store} = + StatifierPersistence.Storage.new( + StatifierPersistence.Storage.Ecto, + persistence: MyApp.Persistence + ) + + Options `init/1` accepts: + + * `:persistence` - required, a module that called + `use StatifierPersistence.Ecto`. The repo, the schema modules, + and the table names all come from its resolved configuration, + so this adapter adds no knobs of its own (ADR-0002 decision 3). + * `:sandbox` - when `true`, `isolate/1` checks out an + `Ecto.Adapters.SQL.Sandbox` connection: the hook a test suite + (this package's conformance suite included) uses to wrap each + test in its own transaction. Default `false`, and `isolate/1` + is then a no-op. + + Engine identities (`content_hash`, `session_id`, `run_id`) are + stored verbatim in `text` columns and blobs in `bytea` columns, so + both round-trip byte-identically (ADR-0002 decision 1, ADR-0003 + decision 1). The identity guard lives in + `StatifierPersistence.Storage`, above this adapter like above every + other one (ADR-0003 decision 2); nothing here decodes a blob. + + `insert_run/2`'s `:run_exists` refusal rides the V01 unique index on + `run_id` - one atomic insert, never a check-then-insert. A backend + failure a callback cannot observe as a value (the database down, a + timeout) raises the driver's own exception rather than being + flattened into a default (this package's errors-are-events rule). + """ + + @behaviour StatifierPersistence.Storage.Adapter + + import Ecto.Query, only: [from: 2] + + alias Ecto.Adapters.SQL.Sandbox + alias Ecto.Changeset + alias StatifierPersistence.Ecto.Config + alias StatifierPersistence.Storage.Adapter + + # The runs.status column vocabulary (ADR-0004 decision 2), mapped + # explicitly in both directions - never String.to_atom on database + # bytes, and an unknown stored status fails loudly on a clause. + @statuses [active: "active", completed: "completed", failed: "failed"] + + @doc """ + Resolves the `:persistence` host module into the handle every other + callback takes: the host's repo, its three generated schema modules, + and its runs table name (for the unique-constraint mapping). + + Refuses a module that never called `use StatifierPersistence.Ecto` + with `{:error, {:adapter, {:not_a_persistence_host, module}}}`. + Makes no database call: reachability surfaces on first use, per + call site. + """ + @impl Adapter + @spec init(Adapter.opts()) :: {:ok, Adapter.opts()} | {:error, Adapter.error()} + def init(opts) do + host = Keyword.get(opts, :persistence) + + if persistence_host?(host) do + config = host.__statifier_persistence__(:config) + + {:ok, + Keyword.merge(opts, + repo: config.repo, + chart_schema: Module.concat(host, Chart), + position_schema: Module.concat(host, Position), + run_schema: Module.concat(host, Run), + runs_table: Config.table(config, :runs) + )} + else + {:error, {:adapter, {:not_a_persistence_host, host}}} + end + end + + @doc """ + Stores `chart_record`, idempotent on its `content_hash`: an insert + with `on_conflict: :nothing` against the unique index, so a repeated + save of the same hash neither duplicates the row nor rewrites it. + """ + @impl Adapter + @spec save_chart(Adapter.opts(), Adapter.chart_record()) :: :ok | {:error, Adapter.error()} + def save_chart(opts, chart_record) do + {:ok, _row} = + repo(opts).insert(struct(chart_schema(opts), chart_record), + on_conflict: :nothing, + conflict_target: [:content_hash] + ) + + :ok + end + + @doc """ + Fetches the chart stored under `content_hash`, or `:chart_not_found`. + """ + @impl Adapter + @spec fetch_chart(Adapter.opts(), Adapter.content_hash()) :: + {:ok, Adapter.chart_record()} | {:error, Adapter.error()} + def fetch_chart(opts, content_hash) do + case repo(opts).get_by(chart_schema(opts), content_hash: content_hash) do + nil -> + {:error, :chart_not_found} + + row -> + {:ok, + %{ + content_hash: row.content_hash, + identity_blob: row.identity_blob, + chart_blob: row.chart_blob + }} + end + end + + @doc """ + Stores `position_record` under its `session_id`, overwriting any + position already stored for that session: an upsert replacing the + record columns (and `updated_at`) on the unique index. + """ + @impl Adapter + @spec save_position(Adapter.opts(), Adapter.position_record()) :: + :ok | {:error, Adapter.error()} + def save_position(opts, position_record) do + {:ok, _row} = + repo(opts).insert(struct(position_schema(opts), position_record), + on_conflict: {:replace, [:content_hash, :identity_blob, :position_blob, :updated_at]}, + conflict_target: [:session_id] + ) + + :ok + end + + @doc """ + Fetches the position stored for `session_id`, or + `:position_not_found`. + """ + @impl Adapter + @spec fetch_position(Adapter.opts(), Adapter.session_id()) :: + {:ok, Adapter.position_record()} | {:error, Adapter.error()} + def fetch_position(opts, session_id) do + case repo(opts).get_by(position_schema(opts), session_id: session_id) do + nil -> + {:error, :position_not_found} + + row -> + {:ok, + %{ + session_id: row.session_id, + content_hash: row.content_hash, + identity_blob: row.identity_blob, + position_blob: row.position_blob + }} + end + end + + @doc """ + Inserts `run_record`, refusing a duplicate `run_id` with + `{:error, :run_exists}`. + + The refusal is the V01 unique index on `run_id` speaking: the insert + carries a `unique_constraint/3` on that index's name, so two + concurrent inserts of one `run_id` cannot both return `:ok` and no + separate existence check ever runs. + """ + @impl Adapter + @spec insert_run(Adapter.opts(), Adapter.run_record()) :: :ok | {:error, Adapter.error()} + def insert_run(opts, run_record) do + row = struct(run_schema(opts), %{run_record | status: encode_status(run_record.status)}) + + changeset = + row + |> Changeset.change() + |> Changeset.unique_constraint(:run_id, + name: "#{Keyword.fetch!(opts, :runs_table)}_run_id_index" + ) + + case repo(opts).insert(changeset) do + {:ok, _row} -> :ok + {:error, %Changeset{}} -> {:error, :run_exists} + end + end + + @doc """ + Fetches the run stored under `run_id`, or `:run_not_found`. + """ + @impl Adapter + @spec fetch_run(Adapter.opts(), Adapter.run_id()) :: + {:ok, Adapter.run_record()} | {:error, Adapter.error()} + def fetch_run(opts, run_id) do + case repo(opts).get_by(run_schema(opts), run_id: run_id) do + nil -> + {:error, :run_not_found} + + row -> + {:ok, + %{ + run_id: row.run_id, + status: decode_status(row.status), + content_hash: row.content_hash, + identity_blob: row.identity_blob, + position_blob: row.position_blob, + failure: row.failure + }} + end + end + + @doc """ + Overwrites the run stored under `run_record`'s `run_id` with the + full record, or refuses with `:run_not_found`. + + One `update_all/3` keyed on `run_id`: the match count is the + existence check, so refusal and overwrite are a single statement. + """ + @impl Adapter + @spec update_run(Adapter.opts(), Adapter.run_record()) :: :ok | {:error, Adapter.error()} + def update_run(opts, %{run_id: run_id} = run_record) do + query = from(r in run_schema(opts), where: r.run_id == ^run_id) + + updates = [ + status: encode_status(run_record.status), + content_hash: run_record.content_hash, + identity_blob: run_record.identity_blob, + position_blob: run_record.position_blob, + failure: run_record.failure, + updated_at: DateTime.utc_now() + ] + + case repo(opts).update_all(query, set: updates) do + {1, _returned} -> :ok + {0, _returned} -> {:error, :run_not_found} + end + end + + @doc """ + Per-test isolation (the optional + `c:StatifierPersistence.Storage.Adapter.isolate/1`): checks out an + `Ecto.Adapters.SQL.Sandbox` connection when this handle was built + with `sandbox: true`, and is a no-op otherwise. + """ + @impl Adapter + @spec isolate(Adapter.opts()) :: :ok | {:error, Adapter.error()} + def isolate(opts) do + if Keyword.get(opts, :sandbox, false) do + case Sandbox.checkout(repo(opts)) do + :ok -> :ok + {:already, _owner_or_allowed} -> :ok + end + else + :ok + end + end + + @doc """ + Runs `fun` under per-run mutual exclusion for `run_id` (the optional + `c:StatifierPersistence.Storage.Adapter.lock_run/3`, ADR-0004 + decision 5 as amended 2026-08-22). + + Everything happens inside one transaction that spans `fun`. It takes + `pg_advisory_xact_lock(hashtextextended(run_id, 0))` first - + unconditional per-run exclusion whether or not the run row exists + yet - and then `SELECT ... FOR UPDATE` on the run row when it does, + keeping the row itself locked against every other writer for the + rest of the transaction. Both locks are transaction-scoped, so any + exit from `fun` releases them: a normal return commits, and a raise + rolls back and propagates to the caller with nothing leaked. + """ + @impl Adapter + @spec lock_run(Adapter.opts(), Adapter.run_id(), (-> result)) :: + {:ok, result} | {:error, Adapter.error()} + when result: term() + def lock_run(opts, run_id, fun) do + repo = repo(opts) + schema = run_schema(opts) + + transaction = + repo.transaction(fn -> + %{rows: [[_void]]} = + repo.query!("SELECT pg_advisory_xact_lock(hashtextextended($1::text, 0))", [run_id]) + + _row_locked = + repo.all( + from(r in schema, where: r.run_id == ^run_id, select: r.id, lock: "FOR UPDATE") + ) + + fun.() + end) + + case transaction do + {:ok, result} -> {:ok, result} + {:error, reason} -> {:error, {:adapter, reason}} + end + end + + @spec repo(Adapter.opts()) :: module() + defp repo(opts), do: Keyword.fetch!(opts, :repo) + + @spec chart_schema(Adapter.opts()) :: module() + defp chart_schema(opts), do: Keyword.fetch!(opts, :chart_schema) + + @spec position_schema(Adapter.opts()) :: module() + defp position_schema(opts), do: Keyword.fetch!(opts, :position_schema) + + @spec run_schema(Adapter.opts()) :: module() + defp run_schema(opts), do: Keyword.fetch!(opts, :run_schema) + + @spec persistence_host?(term()) :: boolean() + defp persistence_host?(host) do + is_atom(host) and not is_nil(host) and Code.ensure_loaded?(host) and + function_exported?(host, :__statifier_persistence__, 1) + end + + for {atom, string} <- @statuses do + defp encode_status(unquote(atom)), do: unquote(string) + defp decode_status(unquote(string)), do: unquote(atom) + end + end +end diff --git a/lib/statifier_persistence/storage/in_memory.ex b/lib/statifier_persistence/storage/in_memory.ex index 0e434c1..ef1d02e 100644 --- a/lib/statifier_persistence/storage/in_memory.ex +++ b/lib/statifier_persistence/storage/in_memory.ex @@ -153,8 +153,9 @@ defmodule StatifierPersistence.Storage.InMemory do releases it; the raise itself propagates to the caller. Simple and honest for a reference adapter. A production adapter should - prefer its backend's native lock - the Ecto adapter implements this as a - transaction-scoped row lock (sp-4an.3). + prefer its backend's native lock - the Ecto adapter implements this as + a transaction-scoped advisory-plus-row lock (ADR-0004 decision 5 as + amended 2026-08-22). """ @impl Adapter @spec lock_run(Adapter.opts(), Adapter.run_id(), (-> result)) :: diff --git a/test/statifier_persistence/storage/ecto_conformance_test.exs b/test/statifier_persistence/storage/ecto_conformance_test.exs new file mode 100644 index 0000000..ac28866 --- /dev/null +++ b/test/statifier_persistence/storage/ecto_conformance_test.exs @@ -0,0 +1,17 @@ +defmodule StatifierPersistence.Storage.EctoConformanceTest do + @moduledoc """ + The lib-shipped conformance suite (ADR-0003 decision 5) run against + `StatifierPersistence.Storage.Ecto` over the zero-config Default host - + the identical suite `InMemory` passes, against real Postgres + (ADR-0005). Isolation comes from the adapter's own `isolate/1` + sandbox checkout, which the suite's setup calls after `init/1`. + + Every test here is generated by the case template, sabotage notes + included; this module adds none of its own. + """ + + use StatifierPersistence.Testing.StorageConformance, + async: true, + adapter: StatifierPersistence.Storage.Ecto, + opts: [persistence: StatifierPersistence.EctoHosts.Default, sandbox: true] +end diff --git a/test/statifier_persistence/storage/ecto_live_lock_test.exs b/test/statifier_persistence/storage/ecto_live_lock_test.exs new file mode 100644 index 0000000..389fa48 --- /dev/null +++ b/test/statifier_persistence/storage/ecto_live_lock_test.exs @@ -0,0 +1,101 @@ +defmodule StatifierPersistence.Storage.EctoLiveLockTest do + # Live lock tests run outside the SQL sandbox, like the live migration + # tests: the sandbox funnels every caller through one shared + # connection, which serializes transactions by ownership alone and + # would mask a broken lock. Here each task takes its own pooled + # connection, so the exclusion observed is the database's - the + # advisory lock and the row lock - not connection scheduling. + use ExUnit.Case, async: false + + alias Ecto.Adapters.SQL.Sandbox + alias StatifierPersistence.EctoHosts.Default + alias StatifierPersistence.Storage + alias StatifierPersistence.TestRepo + + setup_all do + Sandbox.mode(TestRepo, :auto) + + on_exit(fn -> + TestRepo.delete_all(Default.Run) + Sandbox.mode(TestRepo, :manual) + end) + + {:ok, opts} = Storage.Ecto.init(persistence: Default) + %{opts: opts} + end + + defp assert_no_overlap(opts, run_id) do + {:ok, events} = Agent.start_link(fn -> [] end) + + body = fn tag -> + fn -> + Agent.update(events, &[{:enter, tag} | &1]) + Process.sleep(50) + Agent.update(events, &[{:exit, tag} | &1]) + tag + end + end + + tasks = + for tag <- [:first, :second] do + Task.async(fn -> Storage.Ecto.lock_run(opts, run_id, body.(tag)) end) + end + + assert [{:ok, _tag_a}, {:ok, _tag_b}] = Task.await_many(tasks, 10_000) + + recorded = events |> Agent.get(& &1) |> Enum.reverse() + assert [{:enter, one}, {:exit, one}, {:enter, other}, {:exit, other}] = recorded + assert one != other + end + + # sabotage: replaced lock_run/3's transaction body with a bare + # {:ok, fun.()} (no advisory lock, no row lock, no transaction) -> + # red, the two sleeping bodies interleaved on separate pool + # connections and the paired enter/exit pattern broke. Verified red + # (this test and the rowless one below under the one mutation), + # reverted. + test "two connections never overlap on an inserted run's lock", %{opts: opts} do + :ok = + Storage.Ecto.insert_run(opts, %{ + run_id: "run-live-lock-row", + status: :active, + content_hash: "sha256:live-lock", + identity_blob: <<1>>, + position_blob: nil, + failure: nil + }) + + assert_no_overlap(opts, "run-live-lock-row") + end + + # sabotage: dropped the pg_advisory_xact_lock query, leaving only the + # SELECT ... FOR UPDATE row lock -> red on this test alone: with no + # row to lock, the two bodies interleaved (while the inserted-run + # test above stayed green on its row lock). The exact hole the + # ADR-0004 amendment exists for. Verified red, reverted. + test "two connections never overlap on a rowless run_id", %{opts: opts} do + assert_no_overlap(opts, "run-live-lock-rowless") + end + + # sabotage: same bare {:ok, fun.()} mutation as above -> this test + # alone stayed green (nothing held means nothing leaks), which is why + # it exists alongside the overlap tests, not instead of them: it pins + # the release-on-raise contract while they pin the exclusion. Under + # the real implementation a leaked transaction-scoped lock would park + # the reacquisition until the pool's checkout timeout instead of + # answering within the yield window. + test "a raising fun releases the lock for the next caller", %{opts: opts} do + assert_raise RuntimeError, "live lock boom", fn -> + Storage.Ecto.lock_run(opts, "run-live-lock-raise", fn -> + raise "live lock boom" + end) + end + + task = + Task.async(fn -> + Storage.Ecto.lock_run(opts, "run-live-lock-raise", fn -> :reacquired end) + end) + + assert {:ok, {:ok, :reacquired}} = Task.yield(task, 5_000) || Task.shutdown(task) + end +end diff --git a/test/statifier_persistence/storage/ecto_test.exs b/test/statifier_persistence/storage/ecto_test.exs new file mode 100644 index 0000000..b863f98 --- /dev/null +++ b/test/statifier_persistence/storage/ecto_test.exs @@ -0,0 +1,149 @@ +defmodule StatifierPersistence.Storage.EctoTest do + @moduledoc """ + Adapter-level tests the conformance suite does not cover: `init/1`'s + host refusal, the full status vocabulary round-tripping, row-count + idempotence, and the same CRUD mechanisms against the schema-prefixed + `Overridden` host (`workflows.wf_*`, `workflows.workflow_runs`) so + none of them is proven only against the zero-config host. + """ + + use ExUnit.Case, async: true + + alias Ecto.Adapters.SQL.Sandbox + alias StatifierPersistence.EctoHosts.{Default, Overridden} + alias StatifierPersistence.Storage + alias StatifierPersistence.TestRepo + + setup do + :ok = Sandbox.checkout(TestRepo) + + {:ok, default} = Storage.Ecto.init(persistence: Default) + {:ok, overridden} = Storage.Ecto.init(persistence: Overridden) + + %{default: default, overridden: overridden} + end + + defp run_record(run_id, overrides \\ %{}) do + Map.merge( + %{ + run_id: run_id, + status: :active, + content_hash: "sha256:ecto-test-chart", + identity_blob: <<1, 2, 3>>, + position_blob: <<7, 8, 9>>, + failure: nil + }, + overrides + ) + end + + describe "init/1" do + # sabotage: made persistence_host?/1 return true unconditionally -> + # red, both refusals below returned {:ok, _} instead of the + # {:adapter, {:not_a_persistence_host, _}} arm. Verified red, + # reverted. + test "refuses a module that never used StatifierPersistence.Ecto" do + assert {:error, {:adapter, {:not_a_persistence_host, Enum}}} = + Storage.Ecto.init(persistence: Enum) + + assert {:error, {:adapter, {:not_a_persistence_host, nil}}} = + Storage.Ecto.init([]) + end + end + + describe "run status vocabulary" do + # sabotage: crossed the @statuses mapping (completed -> "failed", + # failed -> "failed2") -> the round trip alone stayed green (a + # self-consistent swap is invisible to it), so this test also pins + # the raw stored strings, which went red under the same mutation. + # Verified red, reverted. + test "all three statuses round-trip and store ADR-0004's strings", %{default: opts} do + for {status, stored, run_id} <- [ + {:active, "active", "run-ecto-status-active"}, + {:completed, "completed", "run-ecto-status-completed"}, + {:failed, "failed", "run-ecto-status-failed"} + ] do + :ok = Storage.Ecto.insert_run(opts, run_record(run_id, %{status: status})) + assert {:ok, %{status: ^status}} = Storage.Ecto.fetch_run(opts, run_id) + assert TestRepo.get_by(Default.Run, run_id: run_id).status == stored + end + end + end + + describe "row-count idempotence" do + # sabotage: dropped the on_conflict/conflict_target options from + # save_chart/2 -> red, the second save raised Ecto.ConstraintError + # instead of returning :ok with one row. Verified red, reverted. + test "a repeated save_chart/2 leaves exactly one row", %{default: opts} do + chart_record = %{ + content_hash: "sha256:ecto-test-idempotent", + identity_blob: <<1>>, + chart_blob: <<2>> + } + + :ok = Storage.Ecto.save_chart(opts, chart_record) + :ok = Storage.Ecto.save_chart(opts, chart_record) + + assert TestRepo.aggregate(Default.Chart, :count) == 1 + end + end + + describe "the schema-prefixed Overridden host" do + # sabotage: hardcoded init/1's runs_table to "statifier_runs" -> red, + # the duplicate insert below raised Ecto.ConstraintError (constraint + # workflow_runs_run_id_index not declared under the wrong name) + # instead of returning :run_exists. Verified red, reverted. + test "insert_run/2 maps the renamed table's unique index to :run_exists", %{ + overridden: opts + } do + :ok = Storage.Ecto.insert_run(opts, run_record("run-ecto-overridden-dup")) + + assert {:error, :run_exists} = + Storage.Ecto.insert_run( + opts, + run_record("run-ecto-overridden-dup", %{status: :failed}) + ) + end + + # sabotage: dropped the on_conflict/conflict_target options from + # save_position/2 -> red, the second save below raised + # Ecto.ConstraintError instead of overwriting. Verified red, + # reverted (one mutation covering this test and the conformance + # overwrite test together). + test "chart, position, and run CRUD work under the workflows schema", %{ + overridden: opts + } do + chart_record = %{ + content_hash: "sha256:ecto-overridden-chart", + identity_blob: <<1>>, + chart_blob: <<2>> + } + + :ok = Storage.Ecto.save_chart(opts, chart_record) + :ok = Storage.Ecto.save_chart(opts, chart_record) + assert {:ok, ^chart_record} = Storage.Ecto.fetch_chart(opts, chart_record.content_hash) + assert TestRepo.aggregate(Overridden.Chart, :count) == 1 + + first = %{ + session_id: "sess_ecto_overridden", + content_hash: "sha256:ecto-overridden-chart", + identity_blob: <<1>>, + position_blob: <<3>> + } + + second = %{first | position_blob: <<4>>} + :ok = Storage.Ecto.save_position(opts, first) + :ok = Storage.Ecto.save_position(opts, second) + assert {:ok, ^second} = Storage.Ecto.fetch_position(opts, "sess_ecto_overridden") + + inserted = run_record("run-ecto-overridden-crud") + updated = %{inserted | status: :completed, position_blob: <<9, 9>>} + :ok = Storage.Ecto.insert_run(opts, inserted) + :ok = Storage.Ecto.update_run(opts, updated) + assert {:ok, ^updated} = Storage.Ecto.fetch_run(opts, "run-ecto-overridden-crud") + + assert {:error, :run_not_found} = + Storage.Ecto.update_run(opts, run_record("run-ecto-overridden-missing")) + end + end +end diff --git a/test/support/bootstrap_migrations.ex b/test/support/bootstrap_migrations.ex new file mode 100644 index 0000000..c7e9943 --- /dev/null +++ b/test/support/bootstrap_migrations.ex @@ -0,0 +1,53 @@ +defmodule StatifierPersistence.BootstrapMigrations do + @moduledoc """ + The suite-wide DDL bootstrap: V01 tables for the fixture hosts the + Ecto adapter tests run against, applied once by `test/test_helper.exs` + through `Ecto.Migrator` (idempotent on `:already_up`) and left in + place - the SQL sandbox rolls each test's rows back, so only the DDL + persists between runs. + + The `Kx*` hosts are not bootstrapped here: the live migration tests + own their DDL end to end, up and down, and prove the helper itself. + Test-only support code. + """ + + @migrations [ + {20_260_822_000_101, __MODULE__.DefaultTables}, + {20_260_822_000_102, __MODULE__.OverriddenTables} + ] + + defmodule DefaultTables do + @moduledoc false + use Ecto.Migration + + alias StatifierPersistence.Ecto.Migrations + alias StatifierPersistence.EctoHosts + + def up, do: Migrations.up(for: EctoHosts.Default) + def down, do: Migrations.down(for: EctoHosts.Default) + end + + defmodule OverriddenTables do + @moduledoc false + use Ecto.Migration + + alias StatifierPersistence.Ecto.Migrations + alias StatifierPersistence.EctoHosts + + def up, do: Migrations.up(for: EctoHosts.Overridden) + def down, do: Migrations.down(for: EctoHosts.Overridden) + end + + @doc "Applies every bootstrap migration, tolerating `:already_up`." + @spec up(module()) :: :ok + def up(repo) do + for {version, module} <- @migrations do + case Ecto.Migrator.up(repo, version, module, log: false) do + :ok -> :ok + :already_up -> :ok + end + end + + :ok + end +end diff --git a/test/test_helper.exs b/test/test_helper.exs index f8c3630..11d76cb 100644 --- a/test/test_helper.exs +++ b/test/test_helper.exs @@ -12,6 +12,11 @@ end {:ok, _pid} = StatifierPersistence.TestRepo.start_link() +# The Ecto adapter tests (conformance and unit) run against the Default +# and Overridden fixture hosts' tables; create them once for the whole +# suite, idempotently. Only DDL persists - the sandbox rolls rows back. +:ok = StatifierPersistence.BootstrapMigrations.up(StatifierPersistence.TestRepo) + # The sandbox stays :manual for everything except the live migration tests # (migrations_test.exs, async: false), which manage their own DDL and # inserts: they switch the repo to :auto for the duration of their