[IAmJSD]

Summary

Every store write that can lower the active-admin count now takes adminInvariantAdvisoryLockKey as the first statement of its transaction. That is the same point CreateAdminIfNone takes it: before any users row lock.

Main was already safe. The deferred check_min_one_admin() trigger (migration 000065) already serializes the final count, and the new race tests pass on unmodified main. This PR is defense in depth and makes the lock order consistent, as the issue asks. The one test that fails before the fix is the lock-order test.

Root cause

• Bootstrap: BEGIN → pg_advisory_xact_lock(key) → INSERT → COMMIT
• Demote/deactivate/delete: UPDATE/DELETE (row lock) → COMMIT → trigger takes pg_advisory_xact_lock(key)

So admin-demoting writes held the row lock before the advisory lock, which is the opposite order from the bootstrap. Today nothing deadlocks. The bootstrap only inserts and never waits on an existing admin row, and every demoting path uses the same row→lock order. But a fix that converted only some paths would deadlock. The mutation probe below shows this (40P01 in round 0).

Fix

• New helper PostgresStore.withAdminInvariantLock(ctx, op, fn): BEGIN → pg_advisory_xact_lock(adminInvariantAdvisoryLockKey) → fn(tx) → COMMIT, with a deferred rollback. It reuses the existing constant. The trigger takes the key again at commit, which is a no-op re-acquire.
• Converted every admin-affecting write path on current main:
  • UpdateUserAdminFields: the fix(auth): write only the admin-editable columns from UpdateUser #581 narrow replacement, used by Service.UpdateUser for both deactivate and group removal.
  • UpdateUser: the whole-row update. It has no production caller left, but it is still on StoreInterface and can flip active/group_ids.
  • DeleteUser.
• The comment above adminInvariantAdvisoryLockKey now states the convention: any transaction that may demote, deactivate or delete an Administrators-group member, or create the first admin, must take this lock as its first statement. It also explains the deadlock rule.
• The trigger error now surfaces from the explicit COMMIT, wrapped with %w. isLastAdminConstraintViolation still finds the PgError, and a unit test pins this, so the service still maps it to ErrLastAdmin / 409.

Paths checked and deliberately not locked

These never take admin standing away, so the trigger never fires and they only ever hold the row lock. They cannot complete a cycle with a lock holder:

• RecordSuccessfulLogin and RecordFailedLogin.
• The MFA writes (SetPendingMFASecret, EnableMFA, DisableMFA, ReplaceMFARecoveryCodes, ConsumeMFARecoveryCode, ClaimTOTPCounter, ClearPendingMFASecret).
• UpdateUserCredentials, SetPasswordResetToken and ConsumePasswordResetToken.
• CompletePasswordReset. Its active = $7 is always true from its only caller (ConfirmPasswordReset activates invites and returns early for deactivated accounts).
• CreateUser. A plain INSERT is not watched by the trigger.
• DeleteGroup. It refuses system-managed groups, Administrators included, and does not touch users.group_ids.
• The migration-time admin-group backfill in migrate.go. It only adds the group.

Verification

Run locally on macOS. Integration tests use a real PostgreSQL 16 testcontainer. Tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid. No cloud APIs are involved.

New real-Postgres tests (internal/auth/store_postgres_admin_lock_db_test.go, integration tag):

Test	What it proves	Main (before)	This PR
TestIntegration_AdminDemotingWrites_LockBeforeRowLock	For each of the four write paths (UpdateUserAdminFields deactivate, UpdateUserAdminFields drop the group, UpdateUser deactivate, DeleteUser), a test tx holds the advisory lock the way a bootstrap would. The write must queue on that lock (checked in pg_locks) before it locks the row, so the holder's SELECT … FOR UPDATE NOWAIT on the target row succeeds	FAIL in all 4 subtests: could not obtain lock on row in relation "users" (SQLSTATE 55P03)	PASS
TestIntegration_ConcurrentDemotionOfLastTwoAdmins	Every pair of {deactivate, drop group, delete} through Service, on the last two admins, 15 barrier-synced rounds per pair. Exactly one succeeds, the other gets ErrLastAdmin, and exactly one active admin remains	PASS (the trigger already enforces this)	PASS
TestIntegration_ConcurrentBootstrapVersusDemotion	CreateAdminIfNone racing each demotion, with 1 or 2 existing admins, 15 rounds each. The bootstrap never inserts. The demotion gets ErrLastAdmin with 1 admin and succeeds with 2. Exactly one active admin remains	PASS	PASS
TestIntegration_AdminWrites_NoDeadlock	60 rounds of 17 concurrent writes on 3 admins: all four demoting paths, both login-counter writes and a bootstrap. They run on a pool with lock_timeout=10s. The test asserts no 40P01 and no 55P03, only expected outcomes (trigger refusal, ErrUserChanged, user not found), and ≥1 active admin after every round	PASS	PASS

Mutation probe (partial fix: the lock removed from DeleteUser only, so it uses the old row→lock order while the other paths use lock→row): TestIntegration_AdminWrites_NoDeadlock fails in round 0 with failed to update user: ERROR: deadlock detected (SQLSTATE 40P01). The lock-order test fails on its DeleteUser subtest. The fix has to cover all paths, and the tests catch one that is missed.

New pgxmock unit tests (internal/auth/store_postgres_admin_lock_test.go) replace the three testify MockDBConnection tests for UpdateUser/DeleteUser, which mocked db.Exec directly and cannot express a transaction. For each of the 3 methods they check:

• BEGIN → lock with the exact key → the write → COMMIT, in that order.
• A begin error is reported.
• A lock error rolls back without running the write.
• No row rolls back, returning user not found or ErrUserChanged.
• A write error rolls back.
• A trigger PgError raised at COMMIT still satisfies isLastAdminConstraintViolation.
• UpdateUserAdminFields still returns the stored updated_at.

All 7 fail against main's store code and pass with this PR. Unit/mock evidence.

Commands run:

go build ./...
go vet ./internal/auth/ && go vet -tags integration ./internal/auth/
go test -count=1 ./internal/auth/ ./internal/mocks/ ./internal/api/ ./internal/server/          # all ok
go test -race -tags integration -count=1 ./internal/auth/                                     # whole package ok (94s)
go test -race -tags integration -count=3 -run '<the 4 new tests>' ./internal/auth/            # ok
go test -tags integration -run 'TestAdminUpdateUserHTTPKeepsConcurrentMFA|TestGlobalFiltersCollectedRecommendationsAPI|TestMarketplacePermissionConstraintsHTTP' ./internal/api/     # ok
go test -tags integration -run TestConfigPermissionConstraintsHTTP ./internal/server/         # ok
golangci-lint v2.10.1 run ./internal/auth/...  (and with --build-tags integration)            # 0 issues
gocyclo -over 10 -ignore "_test\.go" internal/auth/                                           # silent
bash scripts/gosec-hook.sh <changed files>                                                    # exit 0
bash scripts/test-git-secrets-allowlist.sh                                                    # 42 PASS, 0 FAIL (includes a scan of all tracked files)

Behavior notes / out of scope

• Under single-writer load nothing changes. With concurrent writers, admin edits (UpdateUserAdminFields, any target) and user deletes now serialize on one global advisory lock for the length of a single-row write. These are rare admin actions. Login, MFA and password writes are not affected.
• The service-level soft check (checkLastAdminConstraint / CountGroupMembers) still runs before and outside the lock, so it can still miss a concurrent demotion. The deferred trigger remains the hard backstop and maps to the same ErrLastAdmin. Moving the soft check under the lock would mean restructuring the service, and the issue does not ask for that.
• CreateAdminIfNone keeps its own inline begin/lock/commit, which is already in the right order. I left it unchanged to keep its existing error messages and tests.
• Raw SQL run outside the store (for example by an operator in psql) still uses the trigger-only order. That is out of scope for application code.
• This PR overlaps fix(auth): reject sessions issued before the user's last password change #1197 in internal/auth/store_postgres.go, in different functions (sessions). Expect at most a trivial merge.

Maintainer: please mirror the issue's triage labels on this PR: impact/internal, type/chore, triaged, priority/p3, severity/low, urgency/eventually, effort/s.

Closes #63

🤖 Generated with Claude Code

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE cad1a4b

Findings (none blocking):

• [non-blocking] internal/auth/store_postgres.go:474-489 - CreateAdminIfNone still opens its own transaction and takes the lock inline, duplicating the new withAdminInvariantLock (store_postgres.go:385). The body says this is deliberate, to keep the existing error messages. Harmless today, but there are now two places that take this lock.
• [non-blocking] internal/auth/store_postgres_admin_lock_db_test.go:232-277 (ConcurrentBootstrapVersusDemotion) - In the 1-admin case, the service soft check (CountGroupMembers) usually rejects the demotion before the store is called, so the lock path is rarely reached.
  • The test passes on base too, as the PR admits.
  • The lock-order proof comes from the LockBeforeRowLock and NoDeadlock tests.
• [non-blocking, documented] UpdateUserAdminFields and DeleteUser now take one global advisory lock even for non-admin targets, so all admin edits and user deletes run one at a time. These are rare admin actions.
• [non-blocking, pre-existing, worth a known-issues entry] service_user.go:731 deletes the user's sessions before DeleteUser. If the delete then fails with ErrLastAdmin, the sessions are already gone.

Completeness: every UPDATE/DELETE/INSERT users in the head Go code was checked.

• The only writes that can lower the admin count are UpdateUser, UpdateUserAdminFields and DeleteUser, and all three are converted.
• CompletePasswordReset only ever sets active to true (service_password.go:414-415).
• The migrate.go backfill only adds a group.
• FK cascades onto users are SET NULL or CASCADE from child tables, and none of those holders takes the advisory lock, so there is no lock cycle.

Error mapping:

• The trigger error now comes back from COMMIT, wrapped with %w.
• isLastAdminConstraintViolation still finds it through errors.As.
• Both callers still map it (service_user.go:365 and :740).

Attacked:

• Mutation probes on a scratch copy, each restored and confirmed identical with cmp:
  • M1, the lock skipped for delete only: the unit DeleteUser subtests FAIL, and the integration AdminWrites_NoDeadlock FAILS with a 40P01 deadlock in round 1.
  • M2, the lock taken after the write instead of before: the unit order expectations FAIL, and integration FAILS.
  • M3, the commit wrap changed from %w to %v:
    • unit CommitTriggerViolationStaysDetectable FAILS for all 3 writes;
    • integration ConcurrentDemotionOfLastTwoAdmins FAILS ("delete vs *" lost ErrLastAdmin);
    • NoDeadlock FAILS.
  • M4, DeleteUser's zero-rows check dropped: NoRowRollsBack/DeleteUser FAILS.
• Fail-before, with both new test files on base:
  • All TestPGXMock_* unit tests FAIL, because base opens no transaction.
  • Integration LockBeforeRowLock FAILS in all 4 subtests with 55P03.
  • NoDeadlock, ConcurrentDemotion and BootstrapVersusDemotion pass on base.
  • This matches the PR's claim exactly.
• Merge: merge-tree with origin/main 7eac792 is clean.
  • Main changed internal/auth since the base (dec9ad7), so I built and tested the merged tree.
  • The PR's production files are identical in head and the merged tree.
• No migration.

Verified locally, with no cloud credentials, under the heavy lock, using the repo's testcontainers Postgres:

• Head:
  • go build ./...: ok.
  • go vet ./internal/auth/, plain and with -tags integration: ok. gofmt -l on the 5 changed files: clean.
  • go test -count=1 on auth, mocks, api and server: ok.
  • go test -race -tags integration -count=1 ./internal/auth/: ok.
• Merged tree:
  • Build and vet with both tag sets: ok.
  • go test -count=1 ./internal/auth/ ./internal/server/: ok.
  • go test -tags integration -count=1 ./internal/auth/: ok.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.