Retires only per-host chart hashes in the conformance suite - #244
Merged
Merged
Conversation
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The shipped conformance suite (
StatifierPersistence.Testing.StorageConformance, inlib/) retired chart hashes that were the same for every module that used it. On Postgres the per-hash retirement lock is keyed on(namespace, hashtext(content_hash))with no table, prefix or schema in it, so two async modules running the suite against one database met on one key. The tombstone-check case and the create-retired-after-first-check case each read a tombstone (shared lock) and then retire the same hash (exclusive lock) inside the test-long sandbox transaction; two copies of either case upgrade at once and Postgres answers40P01. That is the flake the tombstone-check case showed on CI.Classification: test sharing, not an adapter lock-order defect.
retire_chart/3takes the exclusive lock as the first statement of its transaction and holds nothing before it;fetch_retired_info/2is the only shared-lock site. The deadlock needs a caller that composes a tombstone read and a retirement of one hash in one transaction of its own.Change
@conformance_hash_suffix(a module attribute inside theusingblock): the first 16 hex characters of the SHA-256 of the using module's name. Private; no new public function, option or callback.own_chart_a/0, private). Deadlocked before.@retire_hash, and the never-stored miss case's hash: these save a chart row and then retire it, and a row insert racing a shared read and a retirement on one key can also deadlock, so they no longer share a key across modules.Charts.chart_a/0orchart_b/0without retiring (shared locks only; now that no case retires chart "a", no exclusive lock is taken on its hash).docs/adr/0012-retention-and-retirement.md, after its last Note. It states the upgrade hazard for a caller that reads, creates or migrates onto a hash and then retires it in one transaction, that no package path does this on its own, and that the lock key is database-wide. A Note decides nothing: no Status line, zero removed lines underdocs/adr/.changelog.d/sp-gbyk.mdunder### Fixed: the suite ships inlib/and a host running it asynchronously could hit the same flake.Evidence
Seeded
mix testloops against local Postgres 17:ecto_conformance_test.exs,ecto_blob_type_conformance_test.exs,ecto_scoped_conformance_test.exs), seeds 1..15: 7 seeds failed, 11 failures, each a40P01in the tombstone-check case or the create case.@conformance_hash_suffixredefined to one constant string for every module, the three modules together, seeds 1..30: 17 seeds failed, 25 failures, every one a40P01in the tombstone-check case or the create case. Restored from a copy (byte-equal), then touched and force-recompiled.No new test: the change is to the suite's own fixtures, and the evidence is the repeated runs above and the sabotage row, which fails when the suffix stops differing per module.
Gate
Full local
mix quality: green (format, compile with warnings as errors, credo, docs, doc links, dependencies, tests 1,347 of 1,347 at 95.8% coverage, dialyzer). The commit's tree is byte-identical to the tree that gate ran on.Provenance
Refs: sp-gbyk