HIPCTF2/.kilo/plans/867.md

Implementation Plan: Hardened User Deletion Cleanup + Concurrency-Safe Last-Admin Lock

1. Architectural Reconnaissance

• Codebase style & conventions: TypeScript NestJS 10 backend over better-sqlite3 via TypeORM 0.3. All admin mutations live in AdminService (backend/src/modules/admin/admin.service.ts:109-119) and call a shared UsersService.applyLastAdminSafeMutation wrapper (backend/src/modules/users/users.service.ts:63-95). The wrapper currently uses dataSource.transaction('SERIALIZABLE', ...) which on better-sqlite3 still emits a plain BEGIN TRANSACTION (deferred) and only escalates the SQLite write lock at the first write. Two concurrent transactions can therefore both pass the precondition count > 1 before either commits, racing the invariant.
• Data Layer: user, solve, and refresh_token are the only tables with a user_id column. blog_post has no author_id, and awards are persisted as rows in solve itself (points_awarded, is_first/second/third). The new AddUserDeleteCascades1700000000700 migration already adds ON DELETE CASCADE on solve.user_id and refresh_token.user_id, so a plain manager.delete(UserEntity, ...) would cascade in practice — but the user request asks us to perform the dependent cleanup explicitly inside the same transaction so the operation is observable to business code, logging, and the post-condition count.
• Test Framework & Structure: Jest 29 multi-project config (tests/jest.config.js); existing tests/backend/last-admin-invariant.spec.ts and tests/backend/admin-players.spec.ts already exercise the safe-mutation wrapper and the Players endpoints with an in-memory SQLite database. All new tests will be appended to those files (no new top-level test files required) and run via npm run test:backend.
• Required Tools & Dependencies: No new package or global CLI is required. The plan uses the existing TypeORM QueryRunner, the in-repo ApiError / ERROR_CODES envelope, and SQLite's native BEGIN IMMEDIATE semantics. If a future deployment moves to Postgres/MySQL, the locking strategy below degrades gracefully: setLock('pessimistic_write') is a no-op for read paths and a real row lock on databases that support it, while BEGIN IMMEDIATE is harmless outside SQLite.

2. Impacted Files

• To Modify:
  • backend/src/modules/admin/admin.service.ts — perform explicit solve, refresh_token, and any other user-owned table deletion inside the same transaction as the user row delete; report per-table affected counts.
  • backend/src/modules/users/users.service.ts — replace the read/count-only SERIALIZABLE wrapper with a QueryRunner-driven BEGIN IMMEDIATE transaction that performs row-level pessimistic locking where supported, retries on SQLITE_BUSY/serialization failures, and rolls back with 409 LAST_ADMIN if the post-mutation admin count is zero.
  • backend/src/common/errors/error-codes.ts — add a stable CONFLICT_RETRY/CONFLICT_BUSY code (only if the retry path needs to surface a distinct code; otherwise reuse LAST_ADMIN and rely on logs).
  • tests/backend/last-admin-invariant.spec.ts — extend with explicit dependent-cleanup and SQLITE_BUSY retry coverage; add a concurrent Promise.all assertion that at most one writer can drive the admin count to zero.
  • tests/backend/admin-players.spec.ts — add a delete-cascade assertion (no orphan solve/refresh_token rows after deletion) and a LAST_ADMIN rejection when the sole remaining admin is deleted.
• To Create: None. The dependent table set is already known (solve + refresh_token) and the existing migration already enforces ON DELETE CASCADE; the request explicitly asks us to delete the rows imperatively in the same transaction rather than rely solely on the cascade, so no new migration is required.

3. Proposed Changes

1. Backend Logic & APIs — Dependent cleanup on delete (admin.service.ts:109-119):
   • Inside the existing applyLastAdminSafeMutation callback (which already supplies a TypeORM EntityManager), issue explicit deletions in this order:
     1. manager.delete(SolveEntity, { userId: targetId }) (capture affected).
     2. manager.delete(RefreshTokenEntity, { userId: targetId }) (capture affected).
     3. manager.delete(UserEntity, { id: targetId }) — this remains the actual user delete and is the only one that returns 404 when the row is missing; dependent tables short-circuit harmlessly when there are no rows.
   • Sum the affected counts and return them from the mutate closure so the applyLastAdminSafeMutation can log them at info level (NEVER include passwordHash, token_hash, or the plaintext password). The cascade is retained as a safety net; the imperative delete is what the request mandates.
   • Treat any user-owned table that is added in the future as opt-in: introduce a small registry (USER_OWNED_TABLES = [SolveEntity, RefreshTokenEntity]) iterated by the delete path so we don't have to add new call sites each time a new userId column appears. The plan keeps this registry co-located with the service; no new file.
2. Backend Logic & APIs — Concurrency-safe last-admin wrapper (users.service.ts:63-95):
   • Replace the dataSource.transaction('SERIALIZABLE', ...) call with a dataSource.createQueryRunner()-driven flow:
     1. await queryRunner.connect() then await queryRunner.query('BEGIN IMMEDIATE') so the very first statement acquires the SQLite RESERVED lock, preventing a second writer from racing through the precondition.
     2. Acquire a row-level lock on the target user via repo.createQueryBuilder('u').setLock('pessimistic_write').where('u.id = :id', { id: targetId }).getOne(). This is a no-op for SQLite (the database-level lock is what matters) but a real SELECT ... FOR UPDATE on Postgres/MySQL if the deployment is ever migrated.
     3. Acquire a second pessimistic_write lock on the full set of admin rows by selecting all user.id rows where role = 'admin' with setLock('pessimistic_write') (a no-op for SQLite; equivalent to a per-row FOR UPDATE on databases that support it). The aim is to serialize all concurrent admin mutations across the cluster.
     4. Count admins under the lock, fail fast with 409 LAST_ADMIN if count <= 1 and the operation is destructive, otherwise apply the mutation, re-count under the lock, and roll back with 409 LAST_ADMIN if the post-condition is count <= 0.
     5. Commit; in the catch, attempt ROLLBACK and rethrow. Translate SQLITE_BUSY / serialization failures (error code SQLITE_BUSY from better-sqlite3 and the TypeORM LockNotSupportedError/TransactionAlreadyStartedError chain) into a small retry loop (maxRetries = 5, exponential backoff 10ms / 20ms / 40ms / 80ms / 160ms). If retries are exhausted and the last failure still has the system in an indeterminate state, throw ApiError.conflict(ERROR_CODES.LAST_ADMIN, ...) so the caller always sees the existing 409 contract.
   • Add a database-level safety net: a forward-only migration that adds a partial unique index on the user table. The current user schema has no is_last_admin column to enforce a CHECK on, so the simplest correct approach is to add a server-side trigger that raises on UPDATE/DELETE that would reduce the admin count to zero. SQL: CREATE TRIGGER trg_user_last_admin BEFORE UPDATE OF role ON user WHEN NOT EXISTS (SELECT 1 FROM user WHERE role = 'admin' AND id <> NEW.id) AND OLD.role = 'admin' AND NEW.role <> 'admin' BEGIN SELECT RAISE(ABORT, 'LAST_ADMIN'); END; plus a sibling BEFORE DELETE trigger with the same condition. The trigger is the deployment-wide safety net so even a buggy caller that bypasses the service wrapper cannot commit a zero-admin state. The service still rolls back to 409 in normal operation; the trigger ensures the database itself rejects the offending statement if it ever does slip through (e.g. raw SQL or a future parallel module that forgets the wrapper).
3. Backend Logic & APIs — Error code & messaging:
   • The existing ApiError.conflict(ERROR_CODES.LAST_ADMIN, message) shape is preserved for every public path. Add a one-line clarifying message in the wrapper: 'Cannot delete or demote the last admin; the system must always retain at least one enabled user with role "admin".'. Plaintext, hashes, and tokens are never included.
4. Frontend Integration:
   • No frontend code change is required; the LAST_ADMIN toast, modal handling, and row updates already map the existing 409 response. The plan only strengthens the server contract; the existing tests in tests/frontend/admin-players.pure.spec.ts and tests/frontend/change-password-modal.spec.ts already assert the LAST_ADMIN error shape, so no new frontend test is required.
5. Test Strategy (covered in tests/backend/last-admin-invariant.spec.ts and tests/backend/admin-players.spec.ts):
   • Append to applyLastAdminSafeMutation describe block:
     • demote is rejected with 409 LAST_ADMIN when adminCount would reach zero after recount — seed two admins, then in a single transaction demote one to a player role while a second concurrent transaction simultaneously deletes the other; assert at most one of the two requests succeeds and the other receives 409 LAST_ADMIN (use Promise.all with two applyLastAdminSafeMutation calls).
     • delete is rejected with 409 LAST_ADMIN when adminCount would reach zero after recount — same pattern but with delete instead of demote.
     • concurrent role-toggle + delete cannot reduce admin count below 1 — exactly the multi-instance invariant the user requested; run two applyLastAdminSafeMutation calls in parallel (one role-toggle, one delete) against a two-admin seed and assert final admin count is 1 and at least one response is 409 LAST_ADMIN.
   • Append to admin-players describe block:
     • deletePlayer cascades/cleans dependent solve + refresh_token rows — create an admin, create a player, manually insert a solve row tied to the player, perform DELETE /api/v1/admin/players/:id, then assert the solve and refresh_token tables no longer contain rows for the deleted user.
     • sole-admin delete returns 409 LAST_ADMIN with no row removal — assert the user row count is unchanged after the 409.
   • Use the existing in-memory better-sqlite3 + Nest Test.createTestingModule setup; no new infrastructure. The retry path is exercised by directly invoking the service and stubbing the inner dataSource.createQueryRunner() to throw SQLITE_BUSY once before succeeding (use a tiny jest.spyOn on the DataSource) — kept narrow so the test stays fast and deterministic.

4. Test Strategy

• Target Unit Test File: tests/backend/last-admin-invariant.spec.ts (service-level) and tests/backend/admin-players.spec.ts (HTTP-level).
• Mocking Strategy: Real in-memory SQLite + real Nest application, no DB mocks. The retry path uses a one-line jest.spyOn(dataSource, 'createQueryRunner') that throws a SQLITE_BUSY-shaped Error on the first call and returns the real runner on the second; this keeps the assertion deterministic without simulating the SQLite lock manager. No new test helpers and no shared state — each test clears user, solve, and refresh_token rows in beforeEach to remain isolated.