[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]

Warning

Review limit reached

• Run on-demand review

_{This review includes 5 billable files and costs up to $1.25.}

Or wait 59 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: b6355a8
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 996b248a-53cc-424a-b2ce-c6586882f78d

Commits

Reviewing files that changed from the base of the PR and between b6355a8 and cad1a4b.

Files selected for processing: 5

• internal/auth/store_postgres.go
• internal/auth/store_postgres_admin_lock_db_test.go
• internal/auth/store_postgres_admin_lock_test.go
• internal/auth/store_postgres_credentials.go
• internal/auth/store_postgres_test.go

• 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.

[cristim]

VERDICT: APPROVE cad1a4b

PR #1202 (Closes #63), head cad1a4b, base 7e6ec71, single commit.
Merge-tree vs origin/main 6af022f: clean; merged tree builds (go 1.26.10), vet clean (default and -tags integration).

Findings (none blocking)

• nit (process) commit cad1a4b: 854 changed lines (689+, 165-), over the 400-line cap.
• nit internal/auth/store_postgres.go:385-405 vs :428-505 - CreateAdminIfNone keeps its own inline begin/lock/commit instead of using the new withAdminInvariantLock helper. Duplication the PR acknowledges (keeps the existing error strings). Fine to defer.
• note internal/auth/service_user.go:407,457 - the soft check checkLastAdminConstraint still runs outside the lock, so a concurrent demotion can still get past it. The deferred trigger catches it and maps it to the same ErrLastAdmin. The PR body says so; the issue does not ask for more.
• note: UpdateUserAdminFields and DeleteUser now serialize on one global advisory lock for every target user, admins and non-admins alike. These are rare admin actions, so this is acceptable.

Lock-order analysis (deadlock risk)

• Every holder of key 8059058058580001 in app code: CreateAdminIfNone (lock first, then INSERT), withAdminInvariantLock (lock first; used by UpdateUser :191, DeleteUser :258, UpdateUserAdminFields credentials.go:20), and the check_min_one_admin() trigger at commit (000065:53, 000074:90).
• A cycle needs a transaction that holds a users row and then waits on the key. Only the trigger does that, and its WHEN clauses (000065 trg_min_one_admin_update/delete) fire only when an active admin loses the group, is deactivated, or is deleted. Every app path that can do that is now converted. Other users writers (login :10/:28 in store_postgres_login.go, MFA, credentials, the ConsumePasswordResetToken and CompletePasswordReset pair (active is always true, service_password.go:429-434), the migrate.go:271 backfill that only adds the group, and the inserts) never fire it.
• DeleteUser cascades (sessions ON DELETE CASCADE, SET NULL FKs) now run while the key is held. No other key holder takes those rows, so this adds no cycle.
• The key does not collide with any other advisory key in internal/config (globalConfigLockKey, hashtextextended keys).

Results (merged tree, macOS)

• go test ./internal/auth/: 1021 passed.
• go test -race -tags integration -count=1 ./internal/auth/ (testcontainers PG, under heavy.lock): ok 54.0s.
• go test ./internal/api/ ./internal/server/ (AWS_EC2_METADATA_DISABLED=true): ok. Note: running -run 'User|Admin' ./internal/api/ fails the internal/api tests call real AWS Cost Explorer and probe IMDS #855 outbound-network guard on origin/main too, so that failure is environmental and comes from main, not this PR.

Mutation probes (scratch copy, reverted)

1. Removed the pg_advisory_xact_lock call from withAdminInvariantLock: the pgxmock tests TestPGXMock_AdminWrites_TakeLockBeforeWriteAndCommit, LockErrorSkipsWrite and NoRowRollsBack FAIL.
2. Left UpdateUserAdminFields unconverted (direct s.db.QueryRow) while the other paths keep the lock: LockBeforeRowLock fails both UpdateUserAdminFields subtests (55P03), and AdminWrites_NoDeadlock FAILs with 40P01 deadlock detected. This confirms the tests catch a partial conversion and that the deadlock mechanism is real.

CI (gh pr checks, on base 7e6ec71): all green (CI Success, Unit, Integration, E2E, pre-commit, lint, gosec, Trivy, TF validate x3, CodeRabbit); Snyk skipped. No red checks.
#1220 is stacked on this PR. Its tree was not reviewed here.

[cristim]

Local verification at cad1a4b6a7e8d036e959fc970bda5239119ad95c merged onto current main (6a4a506e; merge-tree b76b60c6; macOS):

• go build ./..., go vet -tags integration ./... and go test -race -short ./... pass (full suite).
• Nothing was applied to any cloud.

[cristim]

Marked ready and merged by the owner's agent after 2 independent reviews and local verification (see the review and verification comments above, all at cad1a4b6a7e8d036e959fc970bda5239119ad95c).

[IAmJSD]

Local verification at cad1a4b6a7e8d036e959fc970bda5239119ad95c merged onto current main (6a4a506e), together with #1140, #1170, #1197, #1201, #1202, #1204, #1205, #1208 and #1209 in that order (combined tree 8b907eb6, Linux, Go 1.26.10):

• go build ./..., go vet ./..., go vet -tags integration ./..., (cd tests/e2e && go vet -tags e2e ./...): clean.
• go test -race -short -count=1 ./...: 36 packages ok, no failures.
• go test -race -tags integration -count=1 on every package the nine PRs change (analytics, api, auth, config, database/postgres/migrations, mocks, server), against Docker Postgres: all ok.

CI on this head: CI Success and pre-commit green. cristim's independent review approved this head. Migrations in this batch (000136-000139) are applied in numeric order. Nothing was run against any cloud.

[IAmJSD]

Marked ready and merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), after cristim's independent review approving cad1a4b6 and the combined-tree verification above. Squash-merged at exactly cad1a4b6a7e8d036e959fc970bda5239119ad95c.