[IAmJSD]

Stacked on #1202 (branch fix/issue-63, head cad1a4b6). This branch starts from #1202's head, so the diff against main includes #1202's commit. Review only the commits after cad1a4b6: IAmJSD/cloud-commitments-platform@cad1a4b...fix/issue-583

Merge #1202 first. Once it lands, this PR's diff against main shrinks to the single commit below.

Root cause

After #581, every service path writes users through a column-scoped compare-and-set writer (UpdateUserAdminFields, UpdateUserCredentials, SetPasswordResetToken, the MFA writers and the login bookkeeping writers). The whole-row PostgresStore.UpdateUser(ctx, *User), which rewrites 19 columns from whatever the caller read, had no production caller left. It was still on StoreInterface and every mock, so a future caller could bring back the stale whole-row save that #581 removed. #1202 had also wrapped it in withAdminInvariantLock and given it its own lock-order test cases.

Before changing anything I confirmed there is no production caller, on both main and cad1a4b6:

• git grep -nw UpdateUser <ref> -- '*.go' | grep -v _test.go: the only hits are the definition, the interface method, the two mocks, and Service.UpdateUser(ctx, actor, target, req). That last one is the unrelated service-level method, and it writes through UpdateUserAdminFields.
• The only call sites of the store method are test fixtures and the fix(auth): take the admin invariant lock before the row on demote, deactivate and delete #1202 lock tests.
• gh pr list --state open --search UpdateUser, plus a scan of every open PR touching interfaces.go, store_postgres.go, test_helpers.go, internal/mocks/stores.go or internal/server/health_test.go: no open PR adds a store-level UpdateUser call.

Fix (commit after cad1a4b6)

• Removed UpdateUser(ctx, *User) from StoreInterface, from PostgresStore, and from MockStore (internal/auth/test_helpers.go), MockAuthStore (internal/mocks/stores.go) and mockAuthStoreForHealth (internal/server/health_test.go).
• fix(auth): take the admin invariant lock before the row on demote, deactivate and delete #1202's lock tests: removed only the UpdateUser cases. These are the UpdateUser entry in adminLockedWrites() (pgxmock), its updateUserSQLRegex, and the "UpdateUser deactivate" entry in adminDemotingWrites() (real Postgres). The UpdateUserAdminFields (deactivate and drop-group) and DeleteUser cases are unchanged. They are now the only store writes that can lower the active-admin count, and they still run through:
  • all six pgxmock lock-order tests
  • TestIntegration_AdminDemotingWrites_LockBeforeRowLock (NOWAIT)
  • TestIntegration_ConcurrentDemotionOfLastTwoAdmins and TestIntegration_ConcurrentBootstrapVersusDemotion (service-level, unchanged)
  • TestIntegration_AdminWrites_NoDeadlock. Its mixed workload still sends two writes per path and spreads them over all three admins.
• Fixtures that used UpdateUser as the concurrent writer:
  • service_credentials_db_test.go (4 sites: two email changes, two deactivations): these now go through a new credentialRaceFixture.adminEdit helper, which calls UpdateUserAdminFields, the writer an admin's email or active change really uses. The fields set are the same as before.
  • service_login_db_test.go: the competing change here covers 14 columns (credentials, MFA, reset token, history, groups, active), more than any one store writer touches. It now goes through a test-only overwriteSecurityColumns statement that writes exactly those columns. Because it changes group_ids and active, it runs under withAdminInvariantLock. The function is in a _test.go file with the integration tag, so production code can't call it.
• AssertNotCalled(t, "UpdateUser", ...): these could no longer fail once the method was gone, and the repo's vacuous-assertion guard would flag them.
  • In the MFA wrong-password and empty-code tests, they now name the narrow writer each path would use: SetPendingMFASecret, DisableMFA or ReplaceMFARecoveryCodes, with the matching arity.
  • In the two lockout tests they were dropped. Those mocks already panic on any unstubbed write, and the whole-row write they guarded against no longer exists.
• Comments that named the store UpdateUser now name the real writer: two in production code (service_mfa.go, service_password.go) and several in tests. Comments about the service-level UpdateUser were left as they are.
• Regression pin: TestStore_HasNoWholeRowUserWriter asserts by reflection that neither StoreInterface nor *PostgresStore has an UpdateUser method.

No behavior change.

Closes #583

Verification

All commands run from the repo root at c1d39b0c, with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null. The local ~/.aws/config default profile is invalid, which breaks some unrelated tests.

Fail before / pass after (unit):

• go test ./internal/auth/ -run TestStore_HasNoWholeRowUserWriter -count=1
  • Before the removal it FAILS: auth.StoreInterface still has the whole-row UpdateUser and *auth.PostgresStore still has the whole-row UpdateUser.
  • After the removal it PASSES.

Builds, vet and tests:

• go build ./... passes.
• go vet ./... and go vet -tags integration ./... are clean.
• go test ./internal/auth/ -count=1: ok.
• go test -tags integration ./internal/auth/ -count=1 -timeout 30m (real PostgreSQL via testcontainers): ok, 121s. This includes the four lock-order and race tests named above, TestIntegration_LoginPreservesSecurityChanges, TestIntegration_CredentialWritesRejectStaleCredentials and TestIntegration_ResetConfirmRejectsStaleRead.
• go test ./internal/mocks/ -count=1: ok. This includes TestNoUnfailableMockAssertions.
• go test ./internal/server/ -count=1 and go test ./internal/api/ -count=1: ok. Each package was run on its own.

Lint and other CI checks:

• golangci-lint v2.10.1, run with --allow-parallel-runners on ./internal/auth/... ./internal/mocks/... ./internal/server/..., with and without --build-tags integration: 0 issues.
• gocyclo -over 10 -ignore "_test\.go" internal/auth internal/mocks internal/server: silent.
• bash scripts/gosec-hook.sh <changed .go files>: clean.
• git diff --check: clean.
• git-secrets --scan over all tracked files in a fresh git init repo with an isolated HOME: clean, with both this branch's and origin/main's setup-git-secrets.sh and .gitallowed (main changed the setup in fix(scripts): detect bare AWS secret access keys in git-secrets setup #1125).
• scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL.

Mutation probes (local only, reverted, not committed):

• I made UpdateUserAdminFields write without withAdminInvariantLock. TestIntegration_AdminDemotingWrites_LockBeforeRowLock then fails on both of its UpdateUserAdminFields subtests ("the write locked the users row before the admin-invariant advisory lock"), and TestPGXMock_AdminWrites_TakeLockBeforeWriteAndCommit/UpdateUserAdminFields fails too. So removing the UpdateUser cases did not weaken lock-order coverage of the paths that remain.
• I added a field-by-field assertion inside the login fixture. It confirmed that overwriteSecurityColumns lands all 14 changed values (the 11 scalar or slice fields plus the three timestamps) in all four login scenarios, the same fields the old UpdateUser call changed.

Evidence type and gaps: this is unit, pgxmock and real-PostgreSQL (testcontainers) evidence. The change is internal to the store, has no cloud surface, and needs no real-account verification.

Out of scope

• CompletePasswordReset writes active = $7 from the caller's struct without the admin-invariant lock. Every caller only ever raises active (an invite activates, and the WHERE clause excludes deactivated rows), so it cannot lower the admin count. I left it as fix(auth): take the admin invariant lock before the row on demote, deactivate and delete #1202 left it.
• This branch is not merged with main, so the stack stays reviewable. main's three newer commits don't touch these files.

Labels

A maintainer please mirror #583's triage labels: impact/internal, type/chore, priority/p3, severity/low, urgency/eventually, effort/s.

🤖 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 c1d39b0

Context: this PR is stacked on #1202. The parent of c1d39b0 is cad1a4b, #1202's head, so the 25-file diff against 7e6ec71 includes all of #1202. This PR's own commit is 23 files, +98/-146.

Findings:

• [non-blocking, merge order] This PR must land after fix(auth): take the admin invariant lock before the row on demote, deactivate and delete #1202 or together with it. If fix(auth): take the admin invariant lock before the row on demote, deactivate and delete #1202 is squash-merged, rebase this onto main.
  • I simulated that: fix(auth): take the admin invariant lock before the row on demote, deactivate and delete #1202 squashed onto origin/main, then this commit replayed on top.
  • It is clean and gives exactly the tree I tested.
• No other findings.

Attacked:

1. The method is unused. I grepped origin/main, cad1a4b and the merged tree for .UpdateUser(ctx, x), "UpdateUser" and UpdateUser(ctx context.Context, user.
   • Before the PR, the only references were tests, mocks and fakes:
     • the interface (interfaces.go:14);
     • MockStore (test_helpers.go:48);
     • MockAuthStore (mocks/stores.go:1106);
     • the health fake (server/health_test.go:31).
   • There were no production callers.
   • Main's store.UpdateUser uses in store_postgres_test.go are removed by fix(auth): take the admin invariant lock before the row on demote, deactivate and delete #1202.
   • What remains named UpdateUser is Service.UpdateUser (service_user.go:322), plus comments and docs.
   • The AssertNotCalled replacements (SetPendingMFASecret, DisableMFA, ReplaceMFARecoveryCodes) match the MockStore arities, so they cannot pass vacuously.
2. Interaction with fix(auth): take the admin invariant lock before the row on demote, deactivate and delete #1202:
   • merge-tree of fix(auth): take the admin invariant lock before the row on demote, deactivate and delete #1202's head with this head is clean and gives this PR's own tree.
   • After both, the only store writes that can lower the active-admin count are UpdateUserAdminFields (store_postgres_credentials.go:21) and DeleteUser (store_postgres.go:195).
   • Both go through withAdminInvariantLock, which takes pg_advisory_xact_lock first.
   • Both are also covered by trg_min_one_admin_update and trg_min_one_admin_delete (migration 000074:110-130).
   • The other UPDATE users writers do not lower the count:
     • CompletePasswordReset only ever sets active to true (service_password.go:431).
     • migrate.go:271 only adds the admin group.
     • The credential, MFA and login writers touch neither group_ids nor active.
   • The replaced fixtures use UpdateUserAdminFields, which is locked, and the login test's overwriteSecurityColumns takes the lock.
3. Mutation probes on a scratch copy, each restored and confirmed identical to the head blobs with cmp. All 4 were caught:
   • P1, PostgresStore.UpdateUser re-added: TestStore_HasNoWholeRowUserWriter FAILS.
   • P2, AND email = $2 dropped from the UpdateUserCredentials compare-and-set: the profile-after-concurrent-email-change subtest FAILS. So the new fixture still drives that race.
   • P3, the advisory lock replaced with SELECT $1::bigint: TestPGXMock_AdminWrites_TakeLockBeforeWriteAndCommit and TestIntegration_AdminDemotingWrites_LockBeforeRowLock FAIL.
   • P4, RecordFailedLogin also sets mfa_enabled = false: TestIntegration_LoginPreservesSecurityChanges FAILS.
4. Fail-before: TestStore_HasNoWholeRowUserWriter on base 7e6ec71 FAILS. The other test edits only change fixtures.

Verified locally on the tree merged with origin/main, with GOTOOLCHAIN=go1.26.10 and no cloud credentials, under the heavy lock:

• merge-tree --write-tree origin/main c1d39b0c: clean.
• go build ./... and go vet on auth, api, server and mocks, with and without -tags integration: OK. gofmt -l: clean.
• go test -count=1 ./internal/{auth,api,server,mocks}/...: ok.
• go test -tags integration -v ./internal/auth/ (testcontainers Postgres): ok, with 1134 PASS, 0 SKIP and 0 FAIL.
  • That includes the four admin-lock DB tests (LockBeforeRowLock, ConcurrentDemotionOfLastTwoAdmins, ConcurrentBootstrapVersusDemotion and NoDeadlock) and the pgxmock order tests.
• No migration.

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: REQUEST_CHANGES c1d39b0

Reviewed only the PR's own delta (cad1a4b..c1d39b0, commit c1d39b0) since #1202 merged to main as d53eb3f. The content is correct; the only blocker is mergeability.

Findings:

• blocking (merge state) branch history - c1d39b0 still sits on the pre-squash fix(auth): take the admin invariant lock before the row on demote, deactivate and delete #1202 commit cad1a4b, which is not an ancestor of origin/main (8f186a8). A plain 3-way merge against main conflicts in internal/auth/store_postgres.go, store_postgres_admin_lock_test.go and store_postgres_admin_lock_db_test.go, and GitHub reports CONFLICTING/DIRTY. Replaying only c1d39b0 onto origin/main (merge-tree with base cad1a4b) is conflict-free, so git rebase --onto origin/main cad1a4b6 should be mechanical. A rebase whose git range-diff against c1d39b0 is empty needs only that confirmation, not a fresh content review.
• nit internal/auth/service_mfa_test.go:238,367 - the new AssertNotCalled("SetPendingMFASecret"/"DisableMFA") can fail in principle (method names and arity match interfaces.go:20,23), but a wrong-password regression already fails first through the unstubbed-call panic in MockStore (probe 2), so they are belt-and-braces only. Fine to keep.

Issue #583 check: no production caller of the store-level UpdateUser(ctx, *User) remains on the replayed tree (git grep over *.go: only the service-level Service.UpdateUser and comments). The interface, PostgresStore and all three mocks lose the method, and TestStore_HasNoWholeRowUserWriter pins it. The remaining admin-count-lowering store writes (UpdateUserAdminFields, DeleteUser) keep their lock-order cases. Fixes the issue as stated.

Attacked:

• Replayed c1d39b0 onto origin/main 8f186a8 in a scratch export and ran there: go build ./...; go vet on auth, mocks, server (and auth with -tags integration); go test ./internal/auth/ ./internal/mocks/ ./internal/server/: all ok.
• Integration (testcontainers Postgres) on the replayed tree: TestIntegration_LoginPreservesSecurityChanges, CredentialWritesRejectStaleCredentials, ResetConfirmRejectsStaleRead, AdminDemotingWrites*, AdminWrites_NoDeadlock, ConcurrentDemotion*, ConcurrentBootstrap*: ok, 21 PASS lines with -v (not skipped).
• New UpdateUser callers on main since cad1a4b (sec(ci): refuse authoritative GCP IAM policies in the secret scope guard #1207, fix(db): remove ReplaceRecommendations and its unscoped DELETE #1209, fix(ci): verify the rollback image in AWS_ACCOUNT_ID's registry #1112 and other auth changes): none.
• overwriteSecurityColumns: checked mfa_recovery_codes is NOT NULL (migration 000052) and the fixture always sets a non-nil slice; takes withAdminInvariantLock because it writes group_ids/active; lives in an integration-tagged _test.go file.
• adminEdit fixture: uses the real CAS writer with values read just before the edit, so it cannot fail on its own read.
• Probe 1: re-added a PostgresStore.UpdateUser method -> TestStore_HasNoWholeRowUserWriter FAILS ("*auth.PostgresStore still has the whole-row UpdateUser"). Restored.
• Probe 2: bypassed the password check in MFASetup/MFADisable -> TestMFASetup_WrongPassword FAILS (unstubbed SetPendingMFASecret panic). Restored.
• Probe 3: made adminEdit skip its UpdateUserAdminFields write -> CredentialWritesRejectStaleCredentials (3 subtests) and ResetConfirmRejectsStaleRead/after-concurrent-deactivation FAIL, so the replacement fixtures still produce the race. Restored.
• Probe 4: made the login test skip overwriteSecurityColumns -> still passes. Expected: with no concurrent change there is nothing for a stale save to clobber. This shows only that the probe removes the stimulus, not that the fixture is weak. The overwritten column set matches the removed UpdateUser's, minus the bookkeeping columns the test already excludes.
• CI on c1d39b0: 24 pass, 1 skipping.