fix(storage): canonicalize lock and check identity keys

[Kiran01bm]

Fold lock and check identity keys Go-side at the store boundary as a backstop behind ingress canonicalization.

Why

MySQL's ai_ci collation deduplicates mixed-case identity keys through idx_locks_database and idx_checks_check_key; PostgreSQL compares byte-wise, so the same event stream can double-book a database lock or duplicate check rows, and release/cleanup predicates can miss existing rows. With canonical ingress these folds are no-ops in practice — they defend against future non-canonical callers.

What

• locks.go and checks.go: identity fields (repository, database name, database type, environment) folded on written values and query-predicate args in every method.
• The lock-intent guard (verifyExpectedLockIntent) folds its identity args the same way.
• Lock owner strings are deliberately not folded — they inherit the canonical repository at ingress.
• Folding is Go-side only; no SQL LOWER() (keeps indexes usable).
• Cross-dialect parity subtests: store mixed-case, query differently-cased, assert identical MySQL/PostgreSQL behavior.

Before / after

Before (PostgreSQL):
  Acquire("MyDB")  ──▶ row: MyDB
  Acquire("mydb")  ──▶ row: mydb        ← double-booked lock
  Release("MYDB")  ──▶ 0 rows matched   ← stuck lock

After (both dialects):
  Acquire("MyDB")  ──▶ row: mydb
  Acquire("mydb")  ──▶ conflict (held)
  Release("MYDB")  ──▶ releases mydb

[Copilot]

Pull request overview

This PR strengthens cross-dialect correctness by canonicalizing (lowercasing) storage identity keys at the SQL store boundary so MySQL’s case-insensitive collation behavior is matched by PostgreSQL’s byte-wise comparisons. It prevents duplicate rows and missed predicates for locks and stored check state when callers provide mixed-case repository/database/environment/type values.

Changes:

• Add storage.CanonicalKey() and apply it to lock/check identity fields on writes and on query predicate arguments in the SQL stores.
• Canonicalize lock intent verification predicates (verifyExpectedLockIntent) to avoid case-sensitive mismatches.
• Add cross-dialect storagetest subtests asserting mixed-case write + differently-cased read behaves identically on MySQL and PostgreSQL.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
pkg/storage/canonical.go	Introduces CanonicalKey helper for folding identity key strings.
pkg/storage/internal/sqlstore/locks.go	Canonicalizes lock identity fields on Acquire/Update and canonicalizes database/type/repo predicate args on reads/deletes.
pkg/storage/internal/sqlstore/checks.go	Canonicalizes check identity fields on upserts/state transitions and canonicalizes key predicate args on reads/queries/deletes.
pkg/storage/internal/sqlstore/applies.go	Canonicalizes lock-intent guard query args for database/type.
pkg/storage/storagetest/locks.go	Adds canonical-identity storagetest covering mixed-case lock write and case-insensitive lookup/release behavior.
pkg/storage/storagetest/checks.go	Adds canonical-identity storagetest covering mixed-case check upsert and case-insensitive lookup/query/delete behavior.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 Adversarial correctness review, requested by @aparajon and performed by their agent. Reviewed at head 85d3035f.

Verdict: folding at the store boundary is the right layer and the conformance tests are the right shape, but folding writes without folding what is already stored splits stored check state in two on PostgreSQL, and the one mutual-exclusion primitive that is byte-wise on both dialects is left unfolded two lines from a fold this PR adds. I reproduced the first against real MySQL and PostgreSQL. Reviewed alongside #1213 and #1214, which are independent branches off the same commit and ship a byte-identical copy of pkg/storage/canonical.go.

Findings

1. There is no backfill, so on PostgreSQL every stored check row written before this deploy goes invisible and the next upsert creates a second row for the same logical key. Upsert folds on write and Get/GetByDatabase/GetByPR fold on read, which is self-consistent for rows written after the deploy — but rows already in the table keep their as-delivered casing, and GitHub's repository.full_name preserves whatever case the org and repo were created with. Seeding one pre-upgrade row and then running the folding build against it:

	MySQL	PostgreSQL
Get finds the pre-upgrade row	yes	no
rows for that PR/environment/database after the next Upsert	1	2

idx_checks_check_key does not stop the second row, because the two differ in the repository column itself. MySQL is saved by utf8mb4_0900_ai_ci — the exact crutch CanonicalKey exists to remove — so the dialect this PR is written for is the one it breaks. The operator-facing shape matters more than the row count: if the orphaned row carried apply-owned blocking state, that state is now unreachable from every folded query, and the fresh row is created from whatever the next plan says. AGENTS.md's bar is that a started apply stays authoritative and the gate keeps blocking until an operator reconciles; an invisible blocking row transitions the gate open by cleanup alone. The repo already self-bootstraps storage through EnsureSchema, which is where a one-time fold of the identity columns belongs.

2. applyTargetLockName hashes the unfolded identity triple, and a hash is byte-wise on both dialects. applies.go builds the advisory lock name as sha256(database + "\x00" + dbType + "\x00" + environment). This PR edits verifyExpectedLockIntent a few hundred lines below to fold apply.Database and apply.DatabaseType before hitting the locks table, but leaves the hash alone — so two callers spelling the database differently derive two different lock names, both Acquire succeeds, and the guard that serializes creating an active apply for one target across instances does not run. No collation forgives this one: it is byte-wise on MySQL too. It is pre-existing rather than introduced here, but it is the strongest form of the problem this PR is about, in the same file, and the locks row it now folds is the weaker guard of the two.

3. The doc comment justifies excluding lock owners on a premise that is not true. canonical.go says lock owner strings are "audit metadata … compared byte-wise on both sides of every ownership check". They are the ownership predicate: acquireOnce returns ErrLockHeld on existing.Owner != lock.Owner, and Release, ReleaseIfPendingPlanID and verifyExpectedLockIntent all carry owner in the WHERE clause. The owner is also derived from the repository (fmt.Sprintf("%s#%d", repo, pr)), which #1213 folds — so the owner's value changes even though this PR declines to fold it, and a lock written before that deploy becomes unreleasable by its own PR on PostgreSQL. The reproduction and the transition options are in my review on #1213; I am not restating them here. Whatever the two PRs settle on, this comment should describe it accurately, because it is the file a future author will read before deciding whether a new boundary folds.

4. The new conformance cases hold Owner constant through every case permutation. TestLocks/CanonicalIdentityKey varies DatabaseName, DatabaseType and Repository across four spellings and asserts stored.Owner is unchanged — which pins the current behavior faithfully, but means the suite's ErrLockHeld assertion uses a genuinely different owner ("different-owner") rather than the same owner spelled differently. The case that actually moves in production is the one the suite does not cover.

5. Two of the eight tables that carry identity columns fold. applies, apply_target_locks, plan_comments, plans, tasks and webhook_events all have some combination of repository, database_name, database_type and environment; this PR folds checks and locks. Half-folded identity is worse than uniformly unfolded, because a folded value on one side of a comparison guarantees a miss against an unfolded value on the other, where before both sides drifted together. Either state the boundary explicitly in canonical.go — "the store folds checks and locks; the remaining tables rely on the ingress fold in pkg/webhook" — or extend it, but the current split reads as an oversight rather than a decision.

Action items

1. Add a one-time fold of the identity columns to EnsureSchema, or state the drain requirement, before this lands on any PostgreSQL deployment.
2. Fold the triple in applyTargetLockName, or say in a comment why the advisory lock is allowed to be case-sensitive when the lock row is not.
3. Correct the lock-owner paragraph in canonical.go to describe what owner is used for, and settle the owner question jointly with fix(webhook): canonicalize repository identity at ingress #1213.
4. Add a same-owner-different-spelling case to TestLocks/CanonicalIdentityKey.
5. State the intended table boundary in canonical.go, or extend the fold to the remaining identity-bearing stores.

Verified (tried to break, couldn't)

The fold placement inside the store is right and I could not find a read path that skips it: Get, GetByPR, GetByDatabase and DeleteByPRRetainingBlockingApplyOwned all fold their arguments, so a caller holding an unfolded string still resolves the folded row, and GetByCheckRunID correctly does not fold because it keys on an integer. canonicalizeCheck/canonicalizeLock nil-guard and mutate in place, and the interface comments in storage.go say so, which matters because callers keep using the struct after the call — the storagetest cases rely on exactly that and assert the folded values come back on the caller's pointer. Putting the fold in Acquire rather than acquireOnce is the correct single boundary: acquireOnce is re-entered by withLockRetry, so folding there would repeat harmlessly but would leave the retry loop comparing against a value the caller could still mutate between attempts; the comments added to acquireOnce and refreshPendingConfirmation say which side of that boundary they are on. verifyExpectedLockIntent folds into locals rather than mutating apply, so the caller's struct is not silently rewritten by a verification helper. strings.ToLower is not locale-sensitive in Go — there is no Turkish-dotless-I hazard here — and folding is idempotent, so a value that passes through two folding boundaries is unchanged. Deliberately unfolded values in the tests are the right ones: HeadSHA and PendingPlanID survive verbatim. Both new cases pass on both legs of the parity suite at head (TestStorageParity and TestPostgresStorageParity, Locks/CanonicalIdentityKey and Checks/CanonicalIdentityKey). Worth noting where their value sits: checks and locks are already utf8mb4_0900_ai_ci, so the MySQL side of every assertion would pass with or without the fold — the PostgreSQL leg is what makes them load-bearing.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Approving on @aparajon's behalf after the adversarial correctness review above. The findings there are yours to pick up as follow-ups — flagging them, not gating on them.

This stamp was left by Claude Code (claude-opus-5).

[Kiran01bm]

🤖 Review response — created by Kiran's code review agent (Amp, Claude Opus 4.5) — pull/1216, follow-up commit

Response to Armand's adversarial review. The lock-name hash fold is a genuine catch and is fixed here; the backfill remains sequenced as the series' final PR. Severity-ordered.

#	Finding	Status	Response
F2	applyTargetLockName hashes the unfolded database/type/environment triple — byte-sensitive on BOTH dialects, so case drift defeats mutual exclusion	fixed	All three components fold before hashing, with a unit test proving case-variant inputs derive the identical lock name.
F1	No backfill: pre-upgrade mixed-case rows are invisible to folded predicates on PostgreSQL	deferred	As dispositioned in the first round: a one-time lower() backfill is the final PR of the series, once every fold boundary is in place. PostgreSQL carries no production traffic yet; MySQL's collation forgives the drift in the interim.
F3	canonical.go's lock-owner paragraph rests on a false premise (owner IS the ownership predicate, not audit metadata)	fixed	Rewritten: the owner is the ownership predicate on acquire/release/intent checks, but the repository it derives from folds at ingress, so owners are canonical by construction rather than re-folded at the storage boundary.
F4	TestLocks/CanonicalIdentityKey lacks a same-owner-different-spelling case	fixed	Added: a differently-cased owner can neither re-acquire (ErrLockHeld) nor release (ErrLockNotOwned) — owners compare byte-wise by contract.
F5	Only 2 of 8 identity tables fold in this PR	reply	Deliberate tiering: applies/tasks fold in #1217 and plans/webhook-events in #1218, and the three backstop PRs merge adjacently so no release ships with a partial backstop.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.