From e626f63ba8633f1d907dd66a27f58822567387ba Mon Sep 17 00:00:00 2001 From: JohnnyT Date: Tue, 29 Sep 2026 19:07:19 -0600 Subject: [PATCH] Retires only per-host hashes in conformance The conformance suite's retiring cases used literal chart hashes, so two async modules running it against one Postgres database met on the same database-wide advisory lock key. A case that reads a tombstone and then retires its hash upgrades its shared lock inside the sandbox transaction, and two copies of that case deadlocked (40P01). Every hash a generated case retires now carries a suffix derived from the using module's name: the adapter retirement block, the capability and tombstone-read cases, the facade create and migration sources, and the tombstone-check case's own copy of chart "a". Adds a dated Note to the retention record stating the upgrade hazard for a caller that reads and retires one hash in one transaction, and that the lock key is database-wide, plus a Fixed changelog fragment. Refs: sp-gbyk --- changelog.d/sp-gbyk.md | 3 ++ docs/adr/0012-retention-and-retirement.md | 47 ++++++++++++++++++ .../testing/storage_conformance.ex | 48 +++++++++++++++++-- 3 files changed, 93 insertions(+), 5 deletions(-) create mode 100644 changelog.d/sp-gbyk.md diff --git a/changelog.d/sp-gbyk.md b/changelog.d/sp-gbyk.md new file mode 100644 index 0000000..4219c6a --- /dev/null +++ b/changelog.d/sp-gbyk.md @@ -0,0 +1,3 @@ +### Fixed + +- `StatifierPersistence.Testing.StorageConformance` retires only chart hashes derived from the using module's name, so two or more modules running it asynchronously against one Postgres database no longer deadlock (`40P01`) in the tombstone-check case or in the cases that retire a hash after a create's or a migration's first check. diff --git a/docs/adr/0012-retention-and-retirement.md b/docs/adr/0012-retention-and-retirement.md index 56baf3f..39561d0 100644 --- a/docs/adr/0012-retention-and-retirement.md +++ b/docs/adr/0012-retention-and-retirement.md @@ -810,3 +810,50 @@ What was re-read before the flip: `{:chart_retired, info}` answer. - **The changelog.** The 0.23.0 section of `CHANGELOG.md` names the race under Changed and the two conformance cases under Added. + +## Note (2026-09-29, sp-gbyk): a caller that reads a hash's tombstone and then retires that hash in one transaction upgrades the hash's lock, and the lock's key is database-wide + +This Note decides nothing; it states a hazard the sp-3v2c Amendment's +lock carries, and what the conformance suite now does about it. The +`lib/` cites were read on `main` at `b11af5d`, except the suite's, which +the change that carries this Note edits. + +**The upgrade.** On the Ecto adapter over Postgres a tombstone read +takes the hash's shared lock (`lib/statifier_persistence/storage/ecto.ex`, +`fetch_retired_info/2`), and a retirement takes the exclusive one +(`retire_chart/3`). Both locks are transaction-scoped. A caller whose +own transaction first reads a hash's tombstone, or creates or migrates +an execution onto that hash, and then retires the same hash asks for +the exclusive lock while it still holds the shared one. One such +transaction alone is granted the upgrade. Two such transactions on one +hash each hold the shared lock and each wait for the other to release +it, and Postgres ends one of them with `deadlock_detected` (SQLSTATE +`40P01`). + +**No path the package offers does this on its own.** `retire_chart/3` +takes the exclusive lock as the first statement of its transaction and +holds nothing on the hash before it; `create/4`, `migrate/4`, +`migrate_tree/4` and `StatifierPersistence.Storage.check_chart_retired/2` +take only the shared lock; and +`StatifierPersistence.Executions.retire_chart/4` asks for the active +executions and the pin sources' counts before the retirement, with no +tombstone read. The upgrade is reached only when a caller composes a +tombstone read and a retirement of one hash inside a transaction of +its own. A test that runs in a sandbox transaction lasting the whole +test, and reads a hash's tombstone before it retires that hash, is such +a caller. + +**The key is database-wide.** The lock's key is +`(@chart_lock_namespace, hashtext(content_hash))` (`chart_lock/3`). No +table, table prefix or schema is part of it, so two hosts, or two +suites, that store charts in different tables of one database still +share the lock of every hash they have in common. + +**What the conformance suite does.** Its three Ecto modules in this +package ran the same suite asynchronously against one database, and the +cases that read a tombstone and then retire its hash met on one key and +deadlocked. Every chart hash a generated case retires now carries a +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. diff --git a/lib/statifier_persistence/testing/storage_conformance.ex b/lib/statifier_persistence/testing/storage_conformance.ex index 319a6d5..1cfa6c8 100644 --- a/lib/statifier_persistence/testing/storage_conformance.ex +++ b/lib/statifier_persistence/testing/storage_conformance.ex @@ -147,6 +147,22 @@ defmodule StatifierPersistence.Testing.StorageConformance do @conformance_adapter_opts conformance_adapter_opts @conformance_prune_scope conformance_prune_scope + # Every chart hash a generated case retires is this host module's + # own. On Postgres a retirement takes the hash's advisory lock + # exclusive, the tombstone read takes it shared, and the key is + # database-wide: no table, prefix or schema is in it. Cases in one + # module run one at a time, but two async modules running this + # suite against one database run the same case at once, each in a + # sandbox transaction that lasts the whole test; with one shared + # hash, a case that reads a tombstone and then retires the hash + # upgrades its shared lock while the other module's copy holds + # the same shared lock, and Postgres answers 40P01. A suffix + # derived from the module name keeps each host on keys of its own. + @conformance_hash_suffix :sha256 + |> :crypto.hash(inspect(__MODULE__)) + |> Base.encode16(case: :lower) + |> binary_part(0, 16) + setup do {:ok, store} = Storage.new(@conformance_adapter, @conformance_adapter_opts) @@ -1274,7 +1290,7 @@ defmodule StatifierPersistence.Testing.StorageConformance do if Code.ensure_loaded?(conformance_adapter) and function_exported?(conformance_adapter, :retire_chart, 3) do - @retire_hash "sha256:conformance-retire" + @retire_hash "sha256:conformance-retire-" <> @conformance_hash_suffix # sabotage: drop the adapter under test's guard on the :active # arm - the `Adapter.pinned?(counts) -> ...` cond clause in the @@ -1608,7 +1624,7 @@ defmodule StatifierPersistence.Testing.StorageConformance do assert {:error, :chart_not_found} = @conformance_adapter.retire_chart( store.opts, - "sha256:conformance-retire-never-stored", + "sha256:conformance-retire-never-stored-" <> @conformance_hash_suffix, retirement() ) end @@ -1696,6 +1712,7 @@ defmodule StatifierPersistence.Testing.StorageConformance do + """ @@ -1706,6 +1723,7 @@ defmodule StatifierPersistence.Testing.StorageConformance do + """ @@ -2090,7 +2108,9 @@ defmodule StatifierPersistence.Testing.StorageConformance do # Reverted from a copy. test "facade: a retirement either runs or is declined at open", %{store: store} do answer = - Storage.retire_chart(store, "sha256:conformance-retire-capability", + Storage.retire_chart( + store, + "sha256:conformance-retire-capability-" <> @conformance_hash_suffix, retired_by: "conformance-operator" ) @@ -2150,7 +2170,7 @@ defmodule StatifierPersistence.Testing.StorageConformance do # from a copy. test "adapter: the tombstone read answers what the retired arm of fetch_chart/2 carries", %{store: store} do - hash = "sha256:conformance-tombstone-retired" + hash = "sha256:conformance-tombstone-retired-" <> @conformance_hash_suffix at = DateTime.from_naive!(~N[2026-09-23 09:00:00.000000], "Etc/UTC") assert :ok = @@ -3234,7 +3254,7 @@ defmodule StatifierPersistence.Testing.StorageConformance do # reverted from a copy. test "facade: the tombstone check refuses a retired chart and lets a live one through", %{store: store} do - {source, machine} = Charts.chart_a() + {source, machine} = own_chart_a() content_hash = Machine.identity(machine).content_hash assert :ok = Storage.check_chart_retired(store, machine) @@ -3304,6 +3324,24 @@ defmodule StatifierPersistence.Testing.StorageConformance do if self() == pid, do: send(pid, {:adapter_call, metadata.callback}) end + # Chart "a" with a comment naming this host module's hash suffix, + # so its content hash is this module's own: the case above + # retires it, and a retirement must never meet another async + # module's copy of the case on one lock key. + defp own_chart_a do + {source, _machine} = Charts.chart_a() + + own_source = + String.replace( + source, + "", + " \n" + ) + + {:ok, machine} = Statifier.compile(own_source) + {own_source, machine} + end + defp narrow_read_declared?(store) do if function_exported?(@conformance_adapter, :supports_retired_info?, 1) and function_exported?(@conformance_adapter, :fetch_retired_info, 2) do