Keys the chart lock by store

changelog.d/sp-j6vh.md

@@ -0,0 +1,3 @@
+### Changed
+
+- `StatifierPersistence.Storage.Ecto` keys its per-chart advisory lock on Postgres by the store (the chart table under its prefix) as well as the content hash, so two stores in one database no longer wait on each other for a hash they share; every caller of one store still shares the lock, the first key is unchanged, and a host that held its own advisory locks against the old key sees no overlap it did not see before.

docs/adr/0012-retention-and-retirement.md

@@ -857,3 +857,78 @@ suffix derived from the using module's name
 (`lib/statifier_persistence/testing/storage_conformance.ex`,
 `@conformance_hash_suffix`), so two hosts running the suite in one
 database never retire the same hash.
+
+## Amendment (2026-09-29): the per-hash lock is keyed by the store, never by the tenant
+
+Status of this amendment: proposed (2026-09-29). The record above is
+accepted; this amendment does not change its status line.
+
+The 2026-09-28 Amendment keyed the per-hash advisory lock on
+`(namespace, hashtext(content_hash))`, and the 2026-09-29 Note on the
+lock-upgrade hazard records what that costs: no table, table prefix or
+schema is in the key, so two hosts, or two suites, that keep charts in
+different tables of one database wait on each other for every hash they
+have in common. This amendment scopes the key's second half by the
+store. It adds to that Note's sentence "The key is database-wide" and
+removes nothing from it: the sentence describes the key before this
+change. Ruled by the operator, 2026-09-29. The functions it names in
+`lib/` are the ones the change that carries it adds or edits; every
+other cite was read on `main` at `4ae883c`.
+
+**The key.** The lock is taken on the two-`int4` key
+`(namespace, hashtext(store <> " " <> content_hash))`
+(`lib/statifier_persistence/storage/ecto.ex`, `chart_lock/3`). The first
+key stays `@chart_lock_namespace`, so the first-key sentence of the
+2026-09-28 Amendment still holds. The store is the chart schema's
+`__schema__(:source)` under its `__schema__(:prefix)` when it has one,
+each written as a double-quoted identifier with any quote inside doubled
+and joined by a dot (`chart_store/1`); `supports_chart_retirement?/1`
+already reads the same two values to ask the store whether it can be
+tombstoned. Both modes, the shared read in `fetch_retired_info/2` and
+the exclusive retirement in `retire_chart/3`, take the one key, so
+every interleaving the 2026-09-28 Amendment closes stays closed within
+a store.
+
+**Its unit is the store.** Two stores in one database, two table names
+or one table name under two prefixes, take different keys for one hash
+and do not wait on each other, short of a collision in `hashtext`'s
+32 bits (the prefix-free edge below is the one other exception).
+Two host modules that name one physical table, the same source under the
+same prefix, are one store and keep sharing the key, because they share
+the rows the lock guards.
+
+**Why the unit is not the tenant.** Beside its primary key, the charts
+table's unique index is on `content_hash` alone
+(`lib/statifier_persistence/ecto/migrations/v01.ex`, `up/1`), and there
+is no scope column in the unique index: a host's `:leading_columns` may
+add a tenant column to the table, but not to the index. So a tombstone is one row per hash within a store,
+and two tenants that share a charts table race on that one row. A key
+per tenant would let a create for one tenant read the hash as not
+retired while a retirement for another tenant tombstones the same row,
+which is the window the 2026-09-28 Amendment closed.
+
+**The prefix-free edge.** A chart schema with no prefix is named by its
+table alone, and which schema that table resolves to is the connection's
+`search_path`. Two hosts with the same table name, no prefix, and
+different `search_path` settings therefore share a key though their rows
+are apart. They wait on each other as every store in one database did
+before this amendment; nothing that was safe becomes unsafe.
+
+**What a host sees.** No answer changes. A write or a retirement on one
+store no longer waits for a retirement or a tombstone read of the same
+hash on another store in the same database; within a store, the waits
+and the losing interleaving's answers are the 2026-09-28 Amendment's.
+The key is still a pair of `int4`s under the same first key, so a host
+whose own two-key advisory locks use a different first key meets this
+lock no more often than before, and one whose locks use the same first
+key meets it on a second key it cannot predict, as before.
+
+**Pinned by.** Two live Postgres cases in
+`test/statifier_persistence/ecto/retire_chart_race_test.exs`: "two
+stores in one database retire one hash without either waiting" retires
+one hash in the `scoped` and `scoped_shared_id` prefixes' charts tables,
+the second while the first holds its lock; "two host modules on one
+charts table still share the hash's lock" reads a hash's tombstone
+through one host module while a retirement through another holds the
+lock on the same table, and the read waits. The existing race cases in
+that file run unchanged.

lib/statifier_persistence/storage/ecto.ex

@@ -86,10 +86,12 @@ if Code.ensure_loaded?(Ecto) do
 
     # The first key of the per-hash advisory lock (ADR-0012's 2026-09-28
     # Amendment). The lock is taken in Postgres's two-`int4` form,
-    # `(namespace, hashtext(content_hash))`, which is a key space of its
-    # own: the per-execution lock of `lock_execution/3` is the one-`bigint`
-    # form, and a one-key lock never conflicts with a two-key one. The
-    # value is the four ASCII bytes "SPCH" read as an integer.
+    # `(namespace, hashtext(store <> " " <> content_hash))` with the store
+    # from `chart_store/1` (ADR-0012's 2026-09-29 Amendment), which is a
+    # key space of its own: the per-execution lock of `lock_execution/3`
+    # is the one-`bigint` form, and a one-key lock never conflicts with a
+    # two-key one. The value is the four ASCII bytes "SPCH" read as an
+    # integer.
     @chart_lock_namespace 0x53504348
 
     @doc """
@@ -249,7 +251,10 @@ if Code.ensure_loaded?(Ecto) do
     # The per-hash advisory lock (ADR-0012's 2026-09-28 Amendment):
     # shared for a writer that reads the hash's tombstone before putting
     # an execution on it, exclusive for a retirement. Transaction-scoped,
-    # Postgres only; on any other backend it takes nothing.
+    # Postgres only; on any other backend it takes nothing. The second key
+    # hashes the store with the content hash, so two stores in one
+    # database never wait on each other while every caller of one store
+    # still meets on one key per hash.
     @spec chart_lock(Adapter.opts(), Adapter.content_hash(), :shared | :exclusive) :: :ok
     defp chart_lock(opts, content_hash, mode) do
       repo = repo(opts)
@@ -264,13 +269,35 @@ if Code.ensure_loaded?(Ecto) do
         %{rows: [[_void]]} =
           repo.query!("SELECT #{function}($1::int4, hashtext($2::text))", [
             @chart_lock_namespace,
-            content_hash
+            chart_store(opts) <> " " <> content_hash
           ])
       end
 
       :ok
     end
 
+    # The store the chart lock is scoped to (ADR-0012's 2026-09-29
+    # Amendment): the chart schema's table under its prefix when it has
+    # one, read the way `supports_chart_retirement?/1` reads them, each
+    # part a double-quoted identifier with any quote inside doubled. The
+    # quoting makes the identity end at its last closing quote, so no two
+    # stores and hashes join to one text. A table with no prefix is named
+    # bare: which schema it resolves to is the connection's `search_path`,
+    # and that is not part of the key.
+    @spec chart_store(Adapter.opts()) :: String.t()
+    defp chart_store(opts) do
+      schema = chart_schema(opts)
+      table = quote_identifier(schema.__schema__(:source))
+
+      case schema.__schema__(:prefix) do
+        nil -> table
+        prefix -> quote_identifier(prefix) <> "." <> table
+      end
+    end
+
+    @spec quote_identifier(String.t()) :: String.t()
+    defp quote_identifier(name), do: ~s(") <> String.replace(name, ~s("), ~s("")) <> ~s(")
+
     # The tombstone on one hash, or nil for a hash that has none - which
     # includes a hash with no row at all, because "no row" is
     # `:chart_not_found`'s answer to give and not this function's.

test/statifier_persistence/ecto/retire_chart_race_test.exs

@@ -25,8 +25,11 @@ defmodule StatifierPersistence.Ecto.RetireChartRaceTest do
   closes that interleaving with a per-hash advisory lock: a create or a
   migration reads the tombstone under the hash's shared lock inside its
   own transaction, and a retirement takes the exclusive lock as its first
-  statement. The last cases below race the two on two connections, in
-  both orders.
+  statement. The cases after the first three race the two on two
+  connections, in both orders; the last two show, per ADR-0012's
+  2026-09-29 Amendment, that the lock is keyed by the store: two stores
+  in one database never wait on each other, and two host modules on one
+  charts table do.
   """
 
   # Its own rows, outside any sandbox: nothing here may run beside
@@ -37,7 +40,7 @@ defmodule StatifierPersistence.Ecto.RetireChartRaceTest do
 
   alias Ecto.Adapters.SQL.Sandbox
   alias Statifier.Machine
-  alias StatifierPersistence.EctoHosts.Default
+  alias StatifierPersistence.EctoHosts.{Bigserial, Default, Scoped, SharedIdScoped}
   alias StatifierPersistence.{Executions, Storage}
   alias StatifierPersistence.Migration.Plan
   alias StatifierPersistence.TestRepo
@@ -285,6 +288,73 @@ defmodule StatifierPersistence.Ecto.RetireChartRaceTest do
              Storage.fetch_execution(store, execution_id)
   end
 
+  # The lock's key is the store's (ADR-0012's 2026-09-29 Amendment):
+  # `Scoped` and `SharedIdScoped` keep charts in the same table name under
+  # two prefixes of one database, so this pair proves the prefix is in the
+  # key. The first retirement holds its exclusive lock uncommitted while
+  # the second retires the same hash in the other store.
+  #
+  # sabotage: in Storage.Ecto.chart_lock/3, pass content_hash alone as the
+  # second key's text (dropping chart_store(opts)) -> red, the second
+  # store's retirement waited on the first store's lock and Task.yield
+  # answered nil. Verified red, reverted from a copy.
+  test "two stores in one database retire one hash without either waiting", %{
+    content_hash: content_hash
+  } do
+    {:ok, scoped} = Storage.new(Storage.Ecto, persistence: Scoped)
+    {:ok, shared_id} = Storage.new(Storage.Ecto, persistence: SharedIdScoped)
+    save_chart(scoped, content_hash)
+    save_chart(shared_id, content_hash)
+
+    retire = hold_retirement(scoped, content_hash)
+    assert_receive {:retired_uncommitted, retire_pid, {:ok, _info}}, 5_000
+
+    other =
+      Task.async(fn ->
+        Storage.retire_chart(shared_id, content_hash, retired_by: "branch-desk")
+      end)
+
+    # Answered while the first store's retirement still holds its lock.
+    assert {:ok, {:ok, %{retired_by: "branch-desk"}}} = Task.yield(other, 2_000)
+
+    send(retire_pid, :commit)
+    assert {:ok, %{retired_by: "circulation-desk"}} = Task.await(retire, 5_000)
+  end
+
+  # Two host modules on one physical table are one store: `Default` and
+  # `Bigserial` name the same charts table with no prefix, so a tombstone
+  # read through one waits for a retirement through the other and then
+  # reads its tombstone. The read is the shared lock's own caller: a
+  # plain `SELECT` does not wait on the uncommitted row, so only the
+  # advisory lock makes it wait.
+  #
+  # sabotage: in Storage.Ecto.chart_store/1, append the chart schema's
+  # module name to a prefix-free identity (a key per host module rather
+  # than per table) -> red, the read did not wait and Task.yield answered
+  # {:ok, {:ok, nil}} instead of nil. Verified red, reverted from a copy.
+  test "two host modules on one charts table still share the hash's lock", %{
+    store: store,
+    content_hash: content_hash
+  } do
+    {:ok, bigserial} = Storage.new(Storage.Ecto, persistence: Bigserial)
+    save_chart(store, content_hash)
+
+    retire = hold_retirement(store, content_hash)
+    assert_receive {:retired_uncommitted, retire_pid, {:ok, info}}, 5_000
+
+    read =
+      Task.async(fn ->
+        bigserial.adapter.fetch_retired_info(bigserial.opts, content_hash)
+      end)
+
+    # Still waiting on the retirement's exclusive lock.
+    assert Task.yield(read, 300) == nil
+
+    send(retire_pid, :commit)
+    assert {:ok, ^info} = Task.await(retire, 5_000)
+    assert {:ok, ^info} = Task.await(read, 5_000)
+  end
+
   # A library loan chart of its own for each call: the final state's id
   # carries a fresh integer, so its content hash is one no other case
   # shares, and the row is deleted when the case ends.
@@ -369,5 +439,9 @@ defmodule StatifierPersistence.Ecto.RetireChartRaceTest do
   defp delete_prefixed do
     TestRepo.delete_all(from(e in Default.Execution, where: like(e.execution_id, ^"#{@prefix}%")))
     TestRepo.delete_all(from(c in Default.Chart, where: like(c.content_hash, ^"#{@prefix}%")))
+
+    for chart <- [Scoped.Chart, SharedIdScoped.Chart] do
+      TestRepo.delete_all(from(c in chart, where: like(c.content_hash, ^"#{@prefix}%")))
+    end
   end
 end