v42.1.0 — a dimension prefix filter compares BYTES: two backends disagreed, and every dimension read scanned the corpus (#817, #818) - #820
Conversation
… disagreed, and every dimension read scanned the corpus Closes #817 Closes #818 #818 — the same `AttestationFilter` returned different rows per backend. `dimension_prefixes` compiled to `LIKE` on both SQL backends; sqlite's LIKE is case-INsensitive for ASCII, postgres' is case-sensitive, and the memory backend folds with `str::starts_with`. v42.0.0 ratified CC 3.1.7 R3 — a dimension is a case-sensitive byte string, enforced at the write door by `check_dimension_case_rule` — so sqlite was the one that was wrong, matching `approach:GOALX:v1` for an `approach:goal` prefix. Reachable, not theoretical: R3 exempts Value/External/Wildcard segments from the lowercase rule because they carry caller data, and 32 catalogued families have such a segment. The witnesses use `approach:goalx:v1` vs `approach:GOALX:v1`, both admitted by the current door. The defect was on TWO handles with different backend counts: `list_scores` (three backends, sqlite the outlier) and `list_attestations` (two — the memory backend returns `memory_read_unsupported`). Fixing only the handle #817 named would have left half of it live. #817 — `list_attestations` compiled both dimension axes to a per-row `json_extract(attestation_envelope, '$.dimension')`, so a read for one dimension JSON-parsed every row its attester had ever written. V106 had already added a generated `dimension` column for exactly this and nothing ever used it. V137 indexes it; no table is rebuilt on either dialect because the column was already there. TWO TRAPS, either of which would have shipped a silent wrong answer ------------------------------------------------------------------ 1. Postgres `>=`/`<` use the DATABASE collation, not byte order. Under `en_US.utf8` the naive port of the sqlite range returns NOTHING AT ALL — not a few rows short, empty — because 'config:' and 'config;' collate adjacently when punctuation is weighted weakly. The predicate says `COLLATE "C"` and V137 indexes that same expression. They must agree in both directions: a default-collation predicate against the collated index is an index scan returning WRONG ROWS. 2. Routing through `attestation_subjects` — the approach #817 proposed — is a correctness regression. That projection is subject-keyed by construction (V106's backfill emits nothing for an empty `subject_key_ids`), while `list_attestations` is not subject-keyed and subjectless attestations are routine. A JOIN would have silently dropped every one of them. WITNESSES — six, three mutation-verified against the pre-fix builders -------------------------------------------------------------------- A witness for this class that stays green on the old code is measuring the wrong thing, so each was run against it: sqlite_dimension_prefix_is_case_sensitive_818 LIKE mutation -> ["l","u"] vs ["l"] sqlite_scores_dimension_prefix_is_case_sensitive_818 same, scores handle pg_dimension_prefix_is_case_sensitive_818 COLLATE mutation -> left: [] memory_scores_dimension_prefix_is_case_sensitive_818 pins the already-correct backend sqlite_dimension_filters_are_index_served_817 EXPLAIN plan, not a timing assertion dimension_prefix_bounds_tests (4) the range-is-the-prefix-set property `LIKE` is no longer used for a dimension prefix on either backend. Do not reintroduce it, and do not "fix" the case axis with a COLLATE NOCASE index — that encodes the R3 violation into the schema. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QnaPX9k2t3SeLKXqEXtTGj
…— and the failure could not say so
CI went red on `linux-x86_64 (cirisaudit)` for this branch with:
could not provision a per-process database (execute: db error).
SQL: CREATE DATABASE "ciris_t_11560_…" TEMPLATE "ciris_t_template_449d35e4185d59a2"
Two separate defects, and the second is why the first took a log dig to reach.
1. EXISTENCE ARRIVED BEFORE READINESS
-------------------------------------
`ensure_template` created the template UNDER ITS FINAL NAME and migrated it
afterwards, so for the length of a full migration run the name existed while the
schema did not. Its own fast path tests existence, so every other process in
that window skipped the advisory lock entirely and issued
`CREATE DATABASE … TEMPLATE <half-built>`.
What CI SAW was postgres refusing the copy because the builder was still
connected — a red leg, which is the LUCKY outcome. The unlucky one is the copy
succeeding against a partially migrated template: every per-test database then
carries a schema the tree does not describe and the suite goes green on it.
That is precisely the silent-wrong-answer the fingerprinted template name exists
to prevent (it bit both directions while landing V121) — reintroduced one level
up, in the lifecycle rather than in the name.
Fixed by building under a scratch name and RENAMING into place, so the name
means "fully migrated" by construction rather than by timing. The scratch is
`ciris_t_<pid>_tpl_<hash>`, deliberately shaped like a per-process database:
`reap_dead` parses the segment after `ciris_t_` as a PID, so a dead builder's
scratch is swept, while `ciris_t_template_<hash>` fails that parse and correctly
survives as the cache it is.
I got that naming backwards on the first pass (`<template>_bld_<pid>`, which
`reap_dead` can never match) and wrote a comment claiming the reaper handled it.
`scratch_and_template_names_sort_correctly_for_the_reaper` now pins both halves.
WHY THIS BRANCH AND NOT ANOTHER: the hazard is latent on every migration-adding
PR. V137 changed the migration fingerprint, which invalidates the cached
template and forces every CI process to race a fresh build. Nothing about the
dimension work is involved.
REPRODUCTION — STATED HONESTLY: I could not reproduce this locally. Sixteen
staggered followers across two rounds, against a deliberately dropped template,
all passed on the OLD code. The local migration window is ~0.6s; CI's, under a
2992-test parallel leg, is far wider. So the fix is argued from the ordering and
the error signature, NOT from a local repro that went red and then green.
2. THE FAILURE COULD NOT NAME ITS CAUSE
---------------------------------------
`tokio_postgres::Error`'s Display is the literal string "db error". The server's
message, SQLSTATE, detail and hint all live on the `DbError` behind
`as_db_error()` and were being thrown away, so the panic named the SQL that
failed and not one word about why. That is what made (1) a log-archaeology job,
and it is why the repro above matters less than it should.
`describe_pg_error` now renders, e.g.:
execute: database "template1" already exists [SQLSTATE 42P04]
Witnessed by `a_refused_statement_names_its_reason_not_just_db_error`, which
drives a real refusal rather than asserting on a constructed string.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QnaPX9k2t3SeLKXqEXtTGj
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c200322bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| assert!( | ||
| err.contains("already exists"), | ||
| "provisioning error must carry the server's message, got: {err}" |
There was a problem hiding this comment.
When the PostgreSQL test server uses a non-English lc_messages, the duplicate-database error is localized, so the operation still correctly returns SQLSTATE 42P04 but this assertion fails because the message no longer contains already exists. Assert the specific SQLSTATE already included by describe_pg_error rather than an English rendering so the postgres suite remains portable across server locales.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted and fixed in 127fbe9. You are right: that assertion tests postgres's locale, not our code.
The test now asserts the SQLSTATE, which is the stable half and the one describe_pg_error already carries:
assert!(err.contains("42P04"), "... got: {err}");I kept a second, weaker assertion that some server message came through — a length floor past "execute: db error" plus the SQLSTATE — because carrying the server's message is the whole point of the change, and asserting only the code would let a regression that drops the message go green. But the message's content is the server's to choose, so nothing asserts on its wording any more.
| let scratch = scratch_template_name(); | ||
| let _ = run_sql(admin, &format!("DROP DATABASE IF EXISTS \"{scratch}\"")); | ||
| run_sql(admin, &format!("CREATE DATABASE \"{scratch}\""))?; |
There was a problem hiding this comment.
On a cold parallel nextest start, each process can enter this scratch creation and run the full migration set concurrently: with_advisory_lock acquires the session lock in its spawned thread, then returns from the async block and drops that session before invoking f at line 329. Because these new scratch names are PID-specific, the builders no longer collide early, so all initial processes can perform 137 migrations, losers can leave renamed-failure scratch databases behind, and the test cluster can be overwhelmed by the exact migration and disk load the shared template is meant to avoid. Keep the lock-holding client alive until the scratch migration and final rename finish.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted, reproduced, and fixed in 127fbe9. This is the right call and it is worse than described, because my own previous commit is what turned it live.
You are right that the lock was never held. with_advisory_lock took pg_advisory_lock on a connection owned by a thread that then returned; the session closed and the lock dropped before f() was ever invoked. The function's own comment said so and argued the window was harmless because f re-checks existence.
That argument held only by accident, and I removed the accident. Previously every builder raced CREATE DATABASE "<final template name>" — postgres let exactly one win and the losers fell back to the slow-but-correct path. The PID-unique scratch name I introduced to fix the readiness race means every CREATE DATABASE now succeeds, so on a cold parallel start every process runs the full migration set at once, and the rename losers leave migrated scratch databases standing. Exactly your two consequences.
So the two fixes are coupled in a way I had missed: build-then-rename is only safe if the lock is real.
Fix. The lock-holding connection stays open until f has finished and the rename has landed. One detail worth naming: the release signal is a tokio::sync::oneshot that the lock thread .awaits, deliberately not a blocking recv — the current-thread runtime has to keep polling the connection future or the client stops being driven while the caller runs 137 migrations on the other side of the channel.
Measured rather than argued. the_lock_is_held_for_the_whole_closure_not_just_acquired sends four threads through one lock and records the peak occupancy of the critical section:
| shape | peak |
|---|---|
| with the fix | 1 |
| release-early (the original) | 4 — left: 4, right: 1 |
That mutation is the evidence the preceding commit lacked. I could not reproduce the template readiness race locally (16 staggered followers, all green on the old code — the local migration window is ~0.6s against CI's under a 2992-test leg), and I said so in the PR. This one reproduces on demand, which is a much better place to be.
Also closed the loser-path gap you named: a failed RENAME now drops its scratch rather than leaving a migrated database for reap_dead to collect only after the process exits.
Cold-start verified end to end — template dropped, six concurrent builders, 6/6 pass, zero leftover scratch databases.
Thank you. This was a real hole, and the fact that the broken lock had been serializing nothing since it was written — with a comment explaining why that was fine — is the part I would not have gone looking for.
…s meant to guard — Codex review P1/P2 Both findings from Codex's review of PR #820 accepted, reproduced, and fixed. The P1 is the more serious of the two, and my own previous commit is what turned it from dormant to live. P1 — `with_advisory_lock` serialized NOTHING -------------------------------------------- It took `pg_advisory_lock` on a connection owned by a thread that then RETURNED. Advisory locks are session-scoped, so the session closed and the lock dropped before `f()` was ever called. The function's own comment stated this and argued the window was harmless because `f` re-checks existence. That argument held only by accident. Every builder raced `CREATE DATABASE "<final template name>"`; postgres let exactly one win and the losers took the slow-but-correct path. Giving each builder a PID-unique scratch name — the previous commit's fix for the readiness race — removed that accidental serialization. Every `CREATE DATABASE` then succeeded, so on a cold parallel start EVERY process ran the full migration set simultaneously, which is exactly the load the shared template exists to prevent, and the losers of the rename left migrated scratch databases standing. So the two fixes are coupled: build-then-rename is only safe if the lock is real. The lock-holding connection now stays open — and its runtime keeps POLLING it, via `oneshot` + `.await` rather than a blocking recv, or the client stops being driven while the caller runs 137 migrations — until `f` has finished and the rename has landed. MEASURED, not argued. `the_lock_is_held_for_the_whole_closure_not_just_acquired` runs four threads through one lock and records the peak occupancy of the critical section: with the fix: peak = 1 release-early shape: peak = 4 (left: 4, right: 1) That is the mutation check, and it is the evidence the previous commit lacked: I could not reproduce the template race locally, but this one reproduces on demand. Also closed the gap Codex named in the loser path: a failed RENAME now drops its scratch instead of leaving a migrated database for `reap_dead` to collect only after this process exits. P2 — an assertion on English prose is a portability bug ------------------------------------------------------- `a_refused_statement_names_its_reason_not_just_db_error` asserted the rendered error contains "already exists". A server with a non-English `lc_messages` localizes that string while still returning SQLSTATE 42P04, so the test would fail on a correct server. It now asserts the SQLSTATE, plus a length floor that still proves a server message came through — the CONTENT of that message is the server's to choose, so asserting on it was testing postgres's locale, not our code. Cold-start verified end to end: template dropped, six concurrent builders, 6/6 pass, zero leftover scratch databases. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QnaPX9k2t3SeLKXqEXtTGj
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Both gates green on
|
1 participant
Closes #817
Closes #818
Two issues, one edit. A dimension prefix filter now compares bytes on every backend, which is simultaneously the correctness fix (#818) and the performance fix (#817).
#818 — the same filter returned different rows per backend
dimension_prefixescompiled toLIKEon both SQL backends. SQLite'sLIKEis case-INsensitive for ASCII; Postgres' is case-sensitive; the memory backend folds withstr::starts_with. So oneAttestationFilteranswered differently depending on what was underneath.v42.0.0 ratified CC 3.1.7 R3 — a dimension is a case-sensitive byte string, enforced at the write door by
check_dimension_case_rule. So sqlite was the one that was wrong: it admittedapproach:goalx:v1andapproach:GOALX:v1as distinct rows and then matched them as if they were the same.Reachable, not theoretical. R3 exempts
Value/External/Wildcardsegments from the lowercase rule because they carry caller data, and 32 catalogued families have such a segment —approach:{goal_id},delivery_receipt:{stream_id},bond_posted:{currency},content_rating:{scheme}:{rating}, and more. The witnesses use a pair both admitted by the current door rather than a constructed one.I corrected the issue body: my original
CONFIG:Xexample would be refused by the v42 write door (uppercase family stem), so it was a bad illustration of a real bug.The defect was on two handles with different backend counts, and fixing only the one #817 named would have left half of it live:
list_scoresstarts_with✓LIKE✗ outlier of 3LIKE✓list_attestationsLIKE✗LIKE✓#817 — dimension reads were O(rows the node authored)
list_attestationscompiled both dimension axes to a per-rowjson_extract(attestation_envelope, '$.dimension'), so a read for one dimension JSON-parsed every row its attester had ever written. CIRISServer#557 measured 334kpread64in 25 s and 20 s of a core, per poll cycle.V106 had already added a generated
dimensioncolumn for exactly this, and nothing ever used it. So there is no rebuild on either dialect — V137 is oneCREATE INDEXand the builders now read the column. Pinned by anEXPLAINplan assertion rather than a timing one:LIKEnever got the index anyway: sqlite categorically declines its LIKE-to-range optimization when anESCAPEclause is present, and our builders always emitted one.Two traps, either of which would have shipped a silent wrong answer
1. Postgres
>=/<are not byte order. They use the database collation; ours isen_US.utf8, which is linguistic. The naive port of the sqlite range makes the prefix filter return nothing at all — not a few rows short, empty — because'config:'and'config;'collate adjacently when punctuation is weighted weakly. The predicate saysCOLLATE "C"and V137 indexes that same expression. They must agree in both directions: a collated predicate against a default-collated index is correct-but-unindexed; a default-collated predicate against the collated index is an index scan returning wrong rows.2. Routing through
attestation_subjects— the approach #817 proposed — is a correctness regression. That projection is subject-keyed by construction (V106's backfill emits nothing for an emptysubject_key_ids), whilelist_attestationsis not subject-keyed and subjectless attestations are routine. AJOINwould have silently dropped every one of them.list_scoresmay use the projection;list_attestationsmay not.Witnesses — six, three mutation-verified
A witness for this class that stays green on the old builder is measuring the wrong thing, so each was run against the pre-fix code:
sqlite_dimension_prefix_is_case_sensitive_818LIKE→["l","u"]vs["l"]redsqlite_scores_dimension_prefix_is_case_sensitive_818LIKE→ both ids redpg_dimension_prefix_is_case_sensitive_818COLLATE "C"→left: []redmemory_scores_dimension_prefix_is_case_sensitive_818sqlite_dimension_filters_are_index_served_817EXPLAINplan assertiondimension_prefix_bounds_tests(4)Test state — stated plainly
evidence/cc_impl.tsvrows; re-stamped and verified. The other six are Six postgres quorum tests pass alone and fail together — the strict-majority denominator N is the process-wide steward roster, so the fixture's 2-of-2 becomes 2-of-33 #819, pre-existing: they pass individually and fail together because the strict-majority denominatorNis the process-wide steward roster, so a fixture's 2-of-2 becomes 2-of-33. Reproduced on a tree byte-identical tomain, and the affected path contains no dimension filter at all.RC=2on the co-tenant guard — two other repos are building against the shared 32 cores and the single postgres cluster. I am not overriding that guard; it exists because a suite once went red for a reason that was not in the tree. Queued for a clear window, and I will post the verdict here.Not claiming a performance number until it is measured on a corpus shaped like the status node's.
🤖 Generated with Claude Code
https://claude.ai/code/session_01QnaPX9k2t3SeLKXqEXtTGj