Uh oh!
There was an error while loading. Please reload this page.
V188 unique indexes, a shared test-helper DB preflight, and a docs cleanup - #795
Merged
Conversation
phoenix_kit_user_connections' three schemas each declare a unique_constraint/3 naming an index -- phoenix_kit_user_follows_unique_idx, phoenix_kit_user_blocks_unique_idx and phoenix_kit_user_connections_requester_recipient_uidx -- and none of the three has ever existed. unique_constraint/3 only translates a database violation into a changeset error, so with no index there was no violation: the constraints were inert and the module's read-then-write pre-check was the only guard. Two concurrent follows, blocks or requests both pass that check and both insert, leaving a duplicate relationship no code path produces deliberately and every count then double-reports. V188 removes existing duplicates and creates the three indexes under exactly the names the schemas name, so the module itself needs no change. De-duplication keeps the EARLIEST row of each set. All three tables have a UUIDv7 primary key, which is time-ordered, so `a.uuid > b.uuid` removes the later arrival and the relationship the user established first survives with its original timestamp; the module's *_history tables keep the full record either way. The connections index is on (requester_uuid, recipient_uuid) and deliberately does NOT normalise the pair. request_connection/2 auto-accepts when B requests while A->B is already pending, which needs B->A insertable; an order-independent index would change module behaviour rather than enforce what the module already claims. Manifest: three objects hand-declared in expected_schema.ex, shapes transcribed from a real database migrated through V188 (pg_get_indexdef, pg_index.indisunique, pg_opclass) rather than typed from the migration, and chain_hash restamped over the 54 shipped files. Verified on a scratch database through Squash.MigrationRunner, which can apply or undo a single version: fresh install creates 3 indexes; undoing V188 leaves 0, the state every existing install is in; seeding the duplicates only the missing index allowed and re-applying de-duplicates each table to one row, keeps the earliest uuid, keeps the reverse connection pair, refuses a fresh duplicate with 23505, and two further applications change nothing. The repair planner reports no V188 findings on a healthy database and flags V188 as the first incomplete version when one of the three is dropped. test/integration/user_connections_uniqueness_test.exs covers what the manifest test cannot: that the indexes enforce, and that the pair order is the one the module needs.
Correction to V188 from the review panel, and to my own reasoning behind it. The migration doc and PR said A->B and B->A must both stay insertable "because request_connection/2 auto-accepts a mutual pending request". That is wrong: the auto-accept UPDATES the existing row and never inserts a reverse one. Nothing in the module has ever needed both directions to exist at once. An ordered index therefore left the race that actually matters open. Two users connecting to each other at the same instant both pass request_connection/2's pre-check -- connected?/2 finds no accepted row, and each direction-specific pending lookup finds nothing -- so both insert. The result is not cosmetic: the next request auto-accepts one row and leaves the other as a live pending request between two already-connected users; remove_connection/2 then deletes only the accepted row and leaves that ghost; and get_accepted_connection/2 uses Repo.one/1, so a pair accepted twice raises Ecto.MultipleResultsError. Connections are undirected, so the index is now on (LEAST(requester_uuid, recipient_uuid), GREATEST(...)). Follows and blocks stay on the ordered pair -- those ARE directed, and their reverse is a different relationship. The expression index is still reported under its own name in a 23505, so the schema's unique_constraint/3 keeps working unchanged. The de-duplication had to grow with it. It only collapsed same-direction duplicates, which would have made CREATE UNIQUE INDEX fail on any install holding a cross-direction pair. It now collapses the unordered pair for connections, ranking "accepted" above "pending" before falling back to the earliest uuid, so a live connection is never dropped in favour of an older pending request. Verified on a scratch database: undo V188, seed same-direction AND cross-direction duplicates, re-apply -- one row per pair, accepted preserved, earliest kept, directed reverse follows untouched, cross-direction connection refused afterwards, re-application idempotent. The repair planner reports no V188 findings on a healthy database and flags V188 when an index is dropped. Manifest shape re-transcribed from a real database (the expression keys, not the column names); chain_hash restamped.
A sweep for usernames, personal paths and credentials across every AGENTS.md, CLAUDE.md and committed doc turned up a class of leak worth closing: absolute paths inside one developer's home directory, written into review documents by the AI review tools themselves (they cite files by absolute path) and into follow-up notes referencing a local agent-memory directory. They are rewritten to repo-relative paths, which is what a reader of a public repository can actually use. No content changed beyond the paths.
A wrong PGUSER did not look like a wrong PGUSER. These suites run through the
Ecto SQL sandbox, so a rejected login was queued and retried and surfaced
minutes later as a pool checkout timeout that reads like a flaky test -- nine
repos had ended up documenting that as a landmine in their AGENTS.md.
The default itself is fine and stays: falling back to $USER would break CI,
where the OS user is `runner` while the role is `postgres`. The defect is the
failure MODE, so this is a diagnosis fix, not a configuration one.
PhoenixKit.TestSupport.PostgresPreflight makes one bounded, classified
connection attempt and reports the reason -- credentials rejected, database
missing, nothing listening, no free slots -- naming the effective host,
database and username, and never the password. It replaces each helper's
`psql -lqt` listing, which asked the wrong question entirely: that ran as the
shell's user over a unix socket and said nothing about whether the CONFIGURED
role could connect over TCP, which is exactly how it reported "fine" right
before the suite failed to connect.
It ships in lib/ because the sibling packages depend on core through Hex,
where a test/support directory is unreachable; the precedent is
Ecto.Adapters.SQL.Sandbox.
Two implementation details are load-bearing and were established by measuring
against a live server, not from the documentation:
* It probes with Postgrex.Protocol.connect/1, not Postgrex.start_link/1.
start_link/1 returns {:ok, pid} for a bad role, a missing database AND a
closed port -- sync_connect: true does not change that -- and the failure
then happens inside the connection process, which is the original bug.
Asking that connection for SELECT 1 returns the generic "dropped from
queue after 4000ms". Ecto.Adapters.Postgres.storage_status/1 is no better:
a bad role gives {:error, {:error, %RuntimeError{message: "killed"}}}.
Protocol.connect/1 is undocumented, so the call is guarded and any
surprise degrades to "no opinion" rather than to a broken run.
* It whitelists connection keys off the repo config. Passing the config
through would carry pool: Ecto.Adapters.SQL.Sandbox and rebuild the very
pool whose timeout is being diagnosed.
check/1 never raises, because most suites here degrade to unit-only rather
than fail without a database; check!/1 is for a suite with no unit-only mode.
It is a connection preflight, not a "database ready" check.
Nine tests cover it against a real server, including that a rejected role is
reported in under a second rather than as a queue timeout, and that sandbox
settings in the config are stripped.
This was referenced Sep 8, 2026
Merged
This was referenced Sep 8, 2026
Merged
Merged
The standardisation pass trimmed the Commands block to a minimal set on the theory that standard Mix commands need no documenting. That was right for `mix format` / `mix credo` / `mix dialyzer` -- all reachable through the documented `mix precommit` -- and wrong for the aliases defined in THIS repo's mix.exs, which nobody can guess: `mix quality`, `mix quality.ci`, and where they exist `mix test.setup` / `mix test.reset`. Worse, the replacement told readers to run a bare `createdb` when the repo has a `mix test.setup` alias that does exactly that. Each bullet is generated from the alias's real command list in mix.exs, so it states what the alias actually runs rather than what it is assumed to run.
ddon pushed a commit
that referenced
this pull request
Sep 8, 2026
Reviewed PR #795 (V188 unique indexes for phoenix_kit_user_connections, the shared PostgresPreflight test helper, and a docs cleanup) — no bugs found, review doc at dev_docs/pull_requests/2026/795-user-connections-unique-indexes-and-preflight/CLAUDE_REVIEW.md. The migration's race-condition claims were verified against the actual phoenix_kit_user_connections sibling package, since that module was extracted out of this repo. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvBTdQ8tSq2Ev33q95ZKX3
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 freeto 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.
Three changes on
main. The V188 migration is the substantial one; the othertwo are follow-ups from the same pass.
1. V188 — unique indexes for user connections, follows and blocks
The defect
The three
phoenix_kit_user_connectionsschemas each declare aunique_constraint/3naming an index:phoenix_kit_user_follows_unique_idxphoenix_kit_user_blocks_unique_idxphoenix_kit_user_connections_requester_recipient_uidxNone of the three has ever existed.
unique_constraint/3only translates a database violation into a changeset error, so with no index there is no violation — the constraints were inert, and the module's read-then-write pre-check was the only guard. Two concurrent follows, blocks or requests both pass that check and both insert.Directed vs undirected — the part worth reviewing
Follows and blocks are directed: "A follows B" and "B follows A" are two different relationships, so those index the ordered pair.
Connections are undirected — one row is one relationship, stored in whichever direction it was asked — so that index is an expression index on
(LEAST(requester_uuid, recipient_uuid), GREATEST(...)).An ordered index there would leave the race that actually matters open. Two users clicking "connect" on each other at the same instant both pass
request_connection/2's pre-check (connected?/2finds no accepted row; each direction-specific pending lookup finds nothing) and both insert. The damage is not cosmetic:remove_connection/2then deletes only the accepted row and leaves that ghost behind;get_accepted_connection/2usesRepo.one/1, so a pair accepted twice raisesEcto.MultipleResultsError.Nothing legitimately needs both directions at once: a mutual request updates the existing row to
acceptedrather than inserting a reverse one, and a removed connection deletes its row, freeing the pair.The expression index is still reported under its own name in a
23505, so the schema's existingunique_constraint(name:)keeps working with no change — the field list only decides where the error is attached.De-duplication
Existing duplicates are removed first, or
CREATE UNIQUE INDEXfails. Follows and blocks keep the smallestuuid(UUIDv7 is time-ordered, so that is the row written first). Connections collapse the unordered pair and rankacceptedabovependingbefore falling back to that rule — the two can only coexist through the race being closed, and dropping the accepted row for an older pending request would disconnect two connected users. The*_historytables are untouched.Verification
On a scratch database through
PhoenixKit.Squash.MigrationRunner(it applies or undoes a single version, which the chain wrapper cannot):23505under the right indexManifest: three objects hand-declared in
expected_schema.ex, shapes transcribed from a real V188 database (pg_get_indexdef,pg_index.indisunique,pg_opclass) — including the expression keys rather than column names;chain_hashrestamped over the 54 shipped files.s7/s8skip here (they need a generated manifest, and the generator aborts on the pre-existing slug-function mismatch the V184 note records), so the equivalent was used: the repair planner reports no V188 findings on a freshly-migrated database, and after dropping one index reports "comment claims V188 but V188 is the first version whose objects are not all present".Gates:
mix phoenix_kit.release_checkpasses every migration check (current_version/0 == v188.ex, V135..V188 contiguous,chain_hashmatches 54 files).mix precommitclean. Full suite green.hand_declared_manifest_test.exscovers these objects structurally (it pins the manifest against a chain-built schema for V171+).test/integration/user_connections_uniqueness_test.exs(new, 4 tests) covers what it cannot: that the indexes enforce, that the connection pair is refused in either direction, and that follows and blocks keep both directions.Companion change
phoenix_kit_user_connectionsPR #9 handles the consequence: the losing insert now gets a23505instead of reaching the auto-accept branch, socreate_pending_connection/2surfaces:pair_existsandreconcile_pair_conflict/2re-reads the pair once. Verified with 250 concurrent mutual-click rounds — one row per pair every time, 250{:ok, "pending"}+ 250{:ok, "accepted"}, no caller raising.No version bump and no CHANGELOG entry — those are yours.
2.
PhoenixKit.TestSupport.PostgresPreflight— a shared connection checkA wrong
PGUSERdid not look like a wrongPGUSER. The module suites runthrough the Ecto SQL sandbox, so a rejected login was queued and retried and
surfaced minutes later as a pool checkout timeout that reads like a flaky
test — nine repos had ended up documenting that as a landmine in their
AGENTS.md.The default itself is fine and stays: falling back to
$USERwould break CI,where the OS user is
runnerwhile the role ispostgres. This fixes thefailure mode, not the configuration.
The helper makes one bounded, classified connection attempt and reports the
reason — credentials rejected, database missing, nothing listening, no free
slots — naming the effective host, database and username, and never the
password. It replaces each module's
psql -lqtlisting, which asked the wrongquestion entirely: that ran as the shell's user over a unix socket and said
nothing about whether the configured role could connect over TCP.
It ships in
lib/because the sibling packages depend on core through Hex,where a
test/supportdirectory is unreachable; the precedent isEcto.Adapters.SQL.Sandbox.Two implementation details are load-bearing, and both were established by
measuring against a live server rather than from the documentation:
Postgrex.Protocol.connect/1, notPostgrex.start_link/1.start_link/1returns{:ok, pid}for a bad role, a missing database anda closed port —
sync_connect: truedoes not change that — and the failurethen happens inside the connection process, which is the original bug. Asking
that connection for
SELECT 1returns the generic "dropped from queue after4000ms".
Ecto.Adapters.Postgres.storage_status/1is no better: a bad rolegives
{:error, {:error, %RuntimeError{message: "killed"}}}.Protocol.connect/1is undocumented, so the call is guarded and any surprisedegrades to "no opinion" rather than to a broken run.
through would carry
pool: Ecto.Adapters.SQL.Sandboxand rebuild the verypool whose timeout is being diagnosed.
check/1never raises (most suites degrade to unit-only);check!/1is for asuite with no unit-only mode. It is a connection preflight, not a "database
ready" check — it says nothing about migrations, privileges or sandbox
ownership.
Nine tests cover it against a real server, including that a rejected role is
reported in under a second rather than as a queue timeout, and that sandbox
settings in the config are stripped. The companion PRs wire it into each
module's
test_helper.exs.3. Remove local paths and personal identifiers from committed docs
Absolute paths inside one developer's home directory, written into review
documents by the AI review tools themselves (they cite files by absolute path)
and into follow-up notes referencing a local agent-memory directory. Rewritten
to repo-relative paths, which is what a reader of a public repository can
actually use. No content changed beyond the paths.
🤖 Generated with Claude Code