Repository navigation
Conversation
TestAPIKeyVerifierUpgrade asked the planner for a plan with enable_seqscan off and expected an index scan on key_hash. The verifier migration rewrites api_key while its unique constraint is dropped, so those updates are HOT, and the constraint it adds afterwards gets a B-tree with indcheckxmin set. The planner ignores that index while any transaction older than the migration is still open. In the database contract run another package's test often holds one, so the test failed there and passed alone. Read pg_index for a valid, unique, non-partial B-tree on key_hash instead. The test still fails when the migration leaves key_hash without that index.
silo-triage
Bot
added
the
size/S
Silo Kody — review completeReview finished. Check the inline comments for findings and verify each suggestion against the code and tests. Reviewing changes in Silo
Review settingsReview OptionsThe following review options are enabled or disabled:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly identifies the main change: the auth test now checks the API key verifier index in pg_index instead of using an EXPLAIN plan. Linked Issues check Issue #2814requires TestAPIKeyVerifierUpgrade to pass reliably in the database contract run and to fail when the verifier lookup lacks its index. The test now checks pg_index for a valid, unique, non…Out of Scope Changes check The diff changes only internal/auth/api_key_storage_test.go. The change supports issue #2814by replacing the intermittent planner check with direct catalog validation. No production behavior or unrel…Description check The pull request description directly explains the test failure, the change from planner-based validation to pg_index inspection, and the reported validation results. It matches the changeset. Full details: Linked Issues check
-
Explanation
Issue
#2814requires TestAPIKeyVerifierUpgrade to pass reliably in the database contract run and to fail when the verifier lookup lacks its index. The test now checks pg_index for a valid, unique, non-partial, single-column B-tree index on api_keys.key_hash. It removes the planner-dependent EXPLAIN check. The migration remains unchanged.
Full details: Out of Scope Changes check
-
Explanation
The diff changes only internal/auth/api_key_storage_test.go. The change supports issue
#2814by replacing the intermittent planner check with direct catalog validation. No production behavior or unrelated scope is changed.
-
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
silo-triage
Bot
added
bug
1 participant
Problem
Closes #2814
Related issue: N/A
Validation tasks: none
TestAPIKeyVerifierUpgradefails theci / Go DB pinsjob whenever another package's test has a write transaction open at the moment it asks for a plan, and passes when run alone. Nothing is wrong with the index. It failed twice in a row on #2822, where the Go version change leaves the build cache cold and shifts when packages start.The test runs the verifier migration and then expects
EXPLAINof thekey_hashlookup, withenable_seqscanoff, to use an index. The migration dropsapi_keys_api_key_key, rewritesapi_key, and adds the constraint back onkey_hash. With no index on the column during the rewrite, those updates are HOT, so the rebuilt index hasindcheckxminset. Postgres then won't use it in any transaction whose snapshot is older than the migration's own transaction, so while an older transaction is open anywhere on the server, the plan is a disabled sequential scan. In a deployment this is harmless and clears as soon as those older transactions end.Approach
Check
pg_indexinstead of a plan:api_keysmust have a valid, unique, non-partial B-tree whose only key column iskey_hash. That keeps the test's intent (the existing unique B-tree still supports a bounded lookup) without depending on what else the database server is doing. The migration is unchanged.Validation
On a local PostgreSQL 18.6 database, running only
TestAPIKeyVerifierUpgrade:verifier lookup has no usable index(the same line as in CI) while another session holds a transaction that calledtxid_current()before the test started.ADD CONSTRAINT api_keys_api_key_key UNIQUE (key_hash)line is removed.pg_index.indcheckxminis true, the plan isSeq ScanwithDisabled: truewhile an older write transaction is open, andIndex Scan using api_keys_api_key_keyonce it ends.gofmtandgo vet ./internal/auth/are clean.a53074d, includingci / Go DB pins. ci: use the latest Go 1.26 patch, not the runner's cached one #2822 carries the same commit so its own DB pins job can pass.Evidence
Evidence: none, no user-visible change
Risks
None identified. Only the test changes.
Checklist
AI Disclosure