fix(auth): close admin bootstrap race and cover store SQL paths

[cristim]

Problem

Closes #1153 (review finding TEST-01, P2).

The auth PostgresStore's security-critical SQL paths had zero test coverage: CreateAdminIfNone (the admin bootstrap guard), CountGroupMembers (backs the last-admin protection), ListUsers, ListAPIKeysByUser, UpdateAPIKeyLastUsed and Ping were only ever exercised through interface mocks, so the 19-column bootstrap INSERT, the NOT EXISTS predicate and the Administrators-group forcing logic never actually ran in any test.

Writing the recommended DB-backed concurrency test then exposed a real bug: the comment on CreateAdminIfNone claimed the single INSERT ... WHERE NOT EXISTS statement is atomic, but under READ COMMITTED each statement snapshots at statement start, so two concurrent bootstrap calls can both observe "no admin" and both insert. The new integration test reproduced this against the unmodified code (2 winners, 2 admin rows).

Fix

• CreateAdminIfNone now runs the guarded insert in a transaction that first takes the transaction-scoped advisory lock already used by the min-one-admin trigger from migration 000065 (pg_advisory_xact_lock(8059058058580001), exposed as the documented constant adminInvariantAdvisoryLockKey). The second concurrent caller blocks until the first commits; its INSERT statement then takes a fresh snapshot, sees the committed admin, and the NOT EXISTS guard suppresses the duplicate. Commit failures are surfaced as errors instead of reporting an unconfirmed insert as created.
• DBConnection gains Begin(ctx) (pgx.Tx, error); database.Connection (production) and pgxmock already provide it, and the hand-rolled MockDBConnection got the method.

Tests

• internal/auth/store_postgres_pgxmock_test.go (new, runs in the normal unit suite): pins the actual SQL text and argument order for CreateAdminIfNone (19-column INSERT fed by SELECT $1..$19, NOT EXISTS predicate bound to $20, Administrators-group forcing and dedup, ErrEmailInUse mapping, begin/lock/commit error paths), AdminExists predicate agreement, CountGroupMembers, ListUsers (incl. NULL handling and row errors), ListAPIKeysByUser, UpdateAPIKeyLastUsed and Ping.
• internal/auth/store_postgres_db_test.go (new, integration build tag, testcontainers): proves bootstrap-insert-once semantics under two concurrent CreateAdminIfNone calls across 25 barrier-synced iterations, plus sequential no-op, inactive-admin-counts-as-no-admin (AdminExists agreement) and email-collision semantics against the real migrated schema.

Evidence:

• go test ./internal/auth/ (and with -race): 577 passed.
• go test -tags integration ./internal/auth/ -run TestIntegration_CreateAdminIfNone: 6 passed post-fix; the concurrent test failed with expected: 1, actual: 2 winners against the pre-fix code, confirming it catches the real race.
• Coverage for the previously-0% functions is now 85-100% (go tool cover -func).
• go build ./..., go vet ./...: clean.

Summary by CodeRabbit

Release Notes

• Bug Fixes & Improvements

  • Strengthened the initial administrator bootstrap to be fully atomic and safe under concurrent startups, ensuring only one bootstrap admin is created and administrator-group enforcement remains consistent.

• Tests

  • Added DB-backed integration tests covering bootstrap, no-op behavior when an active admin already exists, inactive-admin edge cases, and email-collision handling.
  • Added extensive PGXMock tests validating SQL behavior, error mapping, and predicate consistency for admin existence checks.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 11 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b3984d93-9c22-4bc7-87e8-8445586cf75a

📥 Commits

Reviewing files that changed from the base of the PR and between a5f6e69 and feaa190.

📒 Files selected for processing (4)

• internal/auth/store_postgres.go
• internal/auth/store_postgres_db_test.go
• internal/auth/store_postgres_pgxmock_test.go
• internal/auth/store_postgres_test.go

📝 Walkthrough

Walkthrough

This PR adds transaction-scoped advisory locking to CreateAdminIfNone, extends the auth store interface for transactions, and adds PostgreSQL-backed integration tests plus PGXMock coverage for the auth store’s SQL paths.

Changes

Admin Bootstrap Invariant with Advisory Lock and Test Coverage

Layer / File(s)	Summary
Advisory Lock Transaction for CreateAdminIfNone internal/auth/store_postgres.go	DBConnection gains Begin(ctx). adminInvariantAdvisoryLockKey is added. CreateAdminIfNone now begins a transaction, acquires pg_advisory_xact_lock, runs the guarded INSERT through the transaction, and commits with error handling.
DB Integration Tests for Bootstrap Concurrency internal/auth/store_postgres_db_test.go	Testcontainer-backed tests run migrations and validate first bootstrap insert, sequential no-op behavior, inactive-admin handling, email-collision handling, and concurrent bootstrap behavior across repeated iterations.
PGXMock Unit Tests for Store Methods internal/auth/store_postgres_pgxmock_test.go	PGXMock tests pin the bootstrap SQL and transaction prefix, cover CreateAdminIfNone success and error paths, verify AdminExists predicate consistency, and exercise CountGroupMembers, ListUsers, ListAPIKeysByUser, UpdateAPIKeyLastUsed, and Ping.
MockDBConnection Begin Method internal/auth/store_postgres_test.go	MockDBConnection adds a Begin(ctx) stub for transaction-based tests.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• LeanerCloud/CUDly#353: Introduced the CreateAdminIfNone bootstrap flow that this PR changes to use a transaction and advisory lock.

Suggested labels

bug

Poem

🐰 I hopped through locks and txns tonight,
To keep the first admin shining bright.
One bootstrap wins, no race in sight,
SQL hums softly, clean and tight.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 42.42% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	Clear, concise, and directly describes the main change: fixing the auth bootstrap race and adding SQL-path coverage.
Linked Issues check	✅ Passed	The PR adds pgxmock and DB-backed coverage for the requested PostgresStore paths and fixes the bootstrap race with transactional locking.
Out of Scope Changes check	✅ Passed	All code changes align with the issue scope: auth store SQL coverage, bootstrap concurrency fix, and supporting test/mock updates.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch test/test-01-auth-postgres-store-coverage

---

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[cristim]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[cristim]

Adversarial review (PR #1227)

Pushed one in-scope fix (commit a5f6e6934) that unblocks the failing Lint Code job. The bootstrap-race fix and the new test coverage hold up under the paranoid lens; the two remaining UNSTABLE checks (Integration Tests, Security Scanning) are pre-existing on main.

Fixed in this PR (commit a5f6e69)

• Lint Code (errcheck + govet/shadow on the new tx scaffolding):
  • defer func() { _ = tx.Rollback(ctx) }() did not satisfy errcheck because this project has check-blank: true in .golangci.yml (PR fix(lint): clear golangci-lint debt in files untouched by open PRs #1276's follow-up flagged the same pattern). Replaced with the project-standard defer tx.Rollback(ctx) //nolint:errcheck used in internal/config/store_postgres.go:{2684,2992}.
  • if _, err := tx.Exec(ctx, "SELECT pg_advisory_xact_lock($1)", ...) shadowed the outer err from Begin (govet shadow). Dropped the short-declaration so the lock-exec assigns to the outer variable.
  • 577 unit tests pass with -race; golangci-lint run ./internal/auth/store_postgres.go is clean on the touched lines.

Verified clean

• Bootstrap race semantics: pg_advisory_xact_lock($1) is taken inside a transaction before the guarded INSERT, with key adminInvariantAdvisoryLockKey = 8059058058580001. Cross-checked against migrations/000065_enforce_min_one_admin.up.sql:53 — exact match, so bootstrap inserts now serialize against each other AND against the deferred trg_min_one_admin_{delete,update} triggers. The lock auto-releases at COMMIT/ROLLBACK (xact-scoped), and the second blocked caller's INSERT re-snapshots after the first commit so the NOT EXISTS guard suppresses the duplicate. Note: the trigger does not fire on INSERT, which is exactly why the bootstrap path needs its own lock acquisition — that gap is what this PR closes.
• Idempotency / TOCTOU: NOT EXISTS predicate (group_ids @> ARRAY[$20::uuid] AND active = true) matches AdminExists's predicate exactly, and the pgxmock test TestPGXMock_AdminExists_PredicateMatchesBootstrapGuard pins both with separate regexes so future drift in either fails a test. The pgxmock test for "INSERT 0" reproduces the expected "lost-the-race" (created=false, err=nil) shape.
• Commit error path: TestPGXMock_CreateAdminIfNone_CommitError asserts the unconfirmed-insert case is reported as (false, err) — not silently (true, nil). Per memory feedback_no_silent_fallbacks.md, no fabricated success on a failed COMMIT. Good.
• Concurrent integration test (TestIntegration_CreateAdminIfNone_ConcurrentBootstrapOnce): two goroutines × 25 barrier-synced iterations; only the test goroutine calls require.* (workers send outcome{} over a channel) — passes feedback_require_in_goroutines.md. Uses close(start) as the barrier instead of time.Sleep — passes feedback_no_sleep_in_tests.md. PR body confirms the test reliably fails (2 winners) against the pre-fix code, so it gates the regression.
• Mock hygiene: newAuthPgxMock registers t.Cleanup(func(){ assert.NoError(t, mock.ExpectationsWereMet()) }) for every test — matches feedback_mock_assert_expectations.md so a missing expectation cannot ship green.
• MockDBConnection.Begin: nil-safe (returns nil tx if the test sets Return(nil, err)); matches the production database.Connection.Begin(ctx) shape from internal/database/connection.go:281.
• isEmailDuplicateError path still wins over a hypothetical 23505 from the advisory-lock acquire — but lock acquire on key 8059058058580001 is not a unique-constraint operation, so 23505 cannot fire there. Safe.
• Build clean (go build ./...), vet clean (go vet -tags integration ./internal/auth/...).

Pre-existing on main (not introduced here)

Verified against the latest main CI - Build & Test run, where they also fail:

• Integration Tests — pre-existing FK seed gap + a sibling panic on internal/config/store_postgres_test.go:313. The latter was the in-scope fix for PR fix(lint): clear golangci-lint debt in files untouched by open PRs #1276; both UNSTABLE on main until that lands.
• Security Scanning — pre-existing npm audit advisories from earlier sweep PRs; unrelated to this auth change.

Findings filed as follow-up issues (out-of-scope)

Two adjacent hardening opportunities that surfaced while reviewing the bootstrap path; both fully triaged and filed against main:

1. auth: acquire admin advisory lock symmetrically on demote/deactivate paths #1305 — Other admin-affecting writes do not acquire the bootstrap advisory lock. UpdateUser / direct demotion paths rely on the deferred constraint trigger (which takes the lock at commit time), but that means an in-flight bootstrap caller blocked on pg_advisory_xact_lock(8059058058580001) can be temporarily ordered after an update that doesn't pre-acquire it. Functionally the deferred trigger still enforces the floor, but acquiring the same key on the update path would give symmetric serialization and remove the "lock is asymmetric" mental footgun. Low priority — current code is correct, just not maximally symmetric.
2. auth: add high-fanout stress variant of bootstrap-race integration test #1306 — Concurrent stress is only 2 goroutines. The integration test runs 2 goroutines × 25 iterations (barrier-synced). The race could theoretically slip a wider window with N=10–100 goroutines pounding the same bootstrap call. The 2-caller scenario is what the production bootstrap UX produces, but a higher-concurrency variant would harden the regression net. Optional.

Verdict

In-scope lint failure is fixed and pushed. The race fix itself is sound: lock key matches the migration, predicate matches AdminExists, the pgxmock pins both SQL and arg order, the integration test reliably reproduces the pre-fix race and passes post-fix. UNSTABLE will persist until the pre-existing main-level Integration Tests + Security Scanning breakages clear (out-of-scope here, tracked elsewhere). LGTM after CR re-confirms.

@coderabbitai full review

[coderabbitai]

@cristim Thanks for the detailed adversarial pass. I’m re-running a full review of #1227 with your latest context in mind — especially the in-scope lint fix in commit a5f6e6934, the confirmation that the remaining UNSTABLE checks are pre-existing on main, and the out-of-scope follow-ups in #1305 and #1306.

(•ᴗ•)⟃🐇

---

✏️ Learnings added

Learnt from: cristim
URL: https://github.com/LeanerCloud/CUDly/pull/1227

Timestamp: 2026-06-26T16:17:10.346Z
Learning: In this repository's Go code, `.golangci.yml` enables errcheck with `check-blank: true`, so intentionally ignored transaction rollback errors should use the project-standard pattern `defer tx.Rollback(ctx) //nolint:errcheck` rather than `_ = tx.Rollback(ctx)`. This convention is already used in `internal/config/store_postgres.go`.

---

Learnt from: cristim
URL: https://github.com/LeanerCloud/CUDly/pull/1227

Timestamp: 2026-06-26T16:17:10.346Z
Learning: In this repository's Go tests, avoid calling `require.*` from worker goroutines. For concurrency tests, worker goroutines should send results back to the main test goroutine over channels, and synchronization should prefer explicit barriers such as `close(start)` instead of `time.Sleep`.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

✅ Action performed

Full review finished.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Merged to main after pre-merge adversarial review (verdict MERGE): the admin bootstrap race fix is real synchronization (pg_advisory_xact_lock with the key matching migrations 000065/000074, serializing with the min-one-admin trigger), verified by reverting the fix and reproducing the race (2 winners at iteration 1) then confirming the fix closes it (25 barrier-synced iterations, exactly 1 winner, clean under -race). Union with current main builds and tests green (585 auth tests + integration testcontainers run). Red CI jobs on the head are the repo-wide Lint/Security debt being fixed in #1364/#1363, not from this PR.