Keep an active system administrator when revocation and deactivation overlap

[Floating-Y]

Two administrators can both pass AuthUser, then revoke or deactivate one another. Revocation previously had no last-admin check, and deactivation counted administrators without serializing competing transactions, so both operations could succeed and leave their organization with no active system administrator. Follow-up to #1234.

Both store entry points now acquire an exclusive FOR NO KEY UPDATE lock on the target's existing organization row before reading account state. The last-active-admin check and mutation run in that same transaction. Under READ COMMITTED, a waiting operation sees the first committed removal and refuses to remove the remaining administrator. Grants use the same locking protocol. This lock mode remains compatible with the FOR KEY SHARE locks used by foreign-key checks, so the organization lock does not unnecessarily block registration or workspace inserts.

Self-revocation/deactivation, missing and deactivated targets, repeated deactivation attribution, and successful-operation audits retain their existing semantics. An already-admitted request may execute after its actor loses permissions, but cannot remove the last active administrator. No migrations, dependencies, or public interfaces change.

The regression tests call the real store functions for mutual revocation, mutual deactivation, and mixed revocation/deactivation. A controller locks the target user rows, observes both worker PIDs blocked through pg_blocking_pids, then releases them. The original reproduction failed all three cases with zero active administrators before the production fix; the fixed implementation permits exactly one removal and retains one active administrator. The stale-actor last-admin case is retained. Tests that merely re-proved unchanged HTTP behavior and the unnecessary pre-race account lookup have been removed.

Validation of fd78ef0 on Windows, Rust 1.98.1, against a dedicated PostgreSQL 18/pgvector instance in WSL with all 101 migrations applied:

• Eight focused account tests passed, including all three deterministic concurrency cases.
• cargo fmt --all --check passed.
• cargo clippy --workspace --all-targets -- -D warnings passed.
• cargo test --workspace passed: 1,565 tests passed, zero failed, six existing opt-in/live-service tests ignored. This includes 653 server tests, 82 store unit tests, and all 313 combined store integration tests.
• Database tests were required with UTOPIA_DATABASE_URL and UTOPIA_TEST_REQUIRE_DB=1; PDF tests used UTOPIA_TEST_REQUIRE_PDFTOTEXT=1. The final run used RUST_TEST_THREADS=1, NO_PROXY=127.0.0.1,localhost,::1, and sslmode=disable on the isolated loopback database connection. Concurrency inside the regression tests remains enabled.

Environment notes: Docker Desktop could not start, so validation used a newly created WSL database. The first full attempt stalled in local HTTP tests with the Windows system proxy enabled; setting process-local NO_PROXY resolved it. A two-thread run encountered an existing DROP TRIGGER deadlock between chat persistence tests. A serial SSL run hit the existing pool-cancellation test's two-second timeout; disabling SSL for this local test connection made it pass. The complete final run above passed without modifying those tests or weakening assertions. The unchanged web app's frozen-lockfile install and production build passed in the initial revision; they were not rerun for this Rust-only review update.

[WaylandYang]

Thank you, this is a real hole. Revocation had no last-admin check, and deactivation's count wasn't serialised, so two admins removing each other could both succeed. Locking the organization row before reading the target, in both set_system_admin and deactivate_user, is the right shape: under READ COMMITTED the waiter's count then sees the first removal. Users are soft-deleted and there is no other is_admin write, so these two paths are all of them. The concurrency tests are excellent; waiting on pg_blocking_pids instead of sleeping makes them deterministic, and they fail without the fix.

Before merge:

1. Please revert the changes to system_admin_tests.rs. That a revoked or deactivated session gets 403/401 on its next request, and that the routes write audits, are existing behaviour this PR doesn't change, and CONTRIBUTING asks that tests not re-prove what the change leaves alone. Also drop the find_user_by_id pre-check loop at the top of concurrent_admin_removal; the race doesn't depend on it. Keep the stale-actor last-admin case in an_admin_grants_system_admin.rs, which is new behaviour.
2. Consider FOR NO KEY UPDATE instead of FOR UPDATE. FOR UPDATE also conflicts with the FOR KEY SHARE lock that foreign-key checks take, so a registration or workspace insert into the org waits on an admin change, and it leaves a small chance of deadlock with anything that locks a user row and then inserts under the org. NO KEY UPDATE still serialises revoke against deactivate. (The description calls the lock "shared"; it is exclusive either way.)

Once the tests are trimmed this is ready to go.

[Floating-Y]

@WaylandYang Thank you — addressed in signed-off commit fd78ef0.

• Reverted all changes to system_admin_tests.rs and removed the find_user_by_id pre-check loop from the concurrency helper.
• Kept the three deterministic store concurrency cases and the stale-actor last-admin regression.
• Changed the organization lock to FOR NO KEY UPDATE, retaining serialization between removals while allowing FK KEY SHARE checks.
• Updated the description to describe the exclusive organization-row lock accurately.

The eight focused account tests, cargo fmt --all --check, and cargo clippy --workspace --all-targets -- -D warnings pass. The final cargo test --workspace run passed 1,565 tests with zero failures and six existing ignored tests, against a dedicated migrated PostgreSQL 18/pgvector database with UTOPIA_TEST_REQUIRE_DB=1.

The PR description records the local environment adjustments and earlier failed attempts: loopback NO_PROXY, serial test-harness execution to avoid an existing DROP TRIGGER deadlock, and SSL disabled only for the isolated local test connection. Internal concurrency in the regression tests is unchanged.

[WaylandYang]

Thank you, both points are addressed. The system_admin_tests.rs changes and the pre-check loop are gone, the concurrency tests still wait on real lock waits, and the stale-actor last-admin case is kept. FOR NO KEY UPDATE still serialises revocation against deactivation without making registrations and workspace inserts wait on an admin change. A small follow-up if you like: with the org lock held, the rows_affected() == 0 branch in set_system_admin can no longer be reached. Merging.