[stefan-ernst]

Summary

• Run combined profile/status patches in one transaction, locking the target before reading SCIM/offboarded guards.
• Reuse the shared deactivation cascade for owned agents, API tokens, personal workspaces, and Todoist sync. Reject offboarded reactivation; preserve existing self-deactivation and SCIM denial contracts.
• Inject production token/session/permission invalidators and invoke them only after commit; preserve existing session semantics (cache eviction, not permanent session deletion).
• Audit cascaded agent deactivations and token revocations. No broad lifecycle consolidation.

Validation

• Reproduced failing service regressions before the fix.
• SQLite: lifecycle/admin/SCIM suites plus complete services, repository, auth, and v2 suites passed.
• Disposable PostgreSQL 17: lifecycle/admin/SCIM suites passed.
• Focused lifecycle service + HTTP race tests passed on SQLite and PostgreSQL.
• Full golangci-lint, go build ./..., and pre-commit architecture/OpenAPI checks passed.

Companion tests: Windshiftapp/core-tests-internal branch wi-2109-user-lifecycle (PR linked below).

WI-2109 — ready for review; not auto-merged.

[stefan-ernst]

Companion regression-test PR: https://github.com/Windshiftapp/core-tests-internal/pull/47. Both branches are wi-2109-user-lifecycle; overlay the test branch onto this branch for validation.

[stefan-ernst]

Solid direction — committing the profile + lifecycle write together under a lock and reusing the shared cascade is the right shape. A few things before merge.

1. v1 admin PUT now 500s when reactivating an offboarded user (blocking)

UpdateAdmin now returns services.ErrUserOffboarded, but the v1 handler's error switch doesn't map it:

• internal/restapi/v1/handlers/admin_users.go:161 calls h.userSvc.UpdateAdmin(...)
• the switch at :164-173 handles NotFound / ManagedExternally / EmailInvalid / EmailExists / UsernameExists, then default: RespondInternalError → HTTP 500

So PUT /rest/api/v1/admin/users/{id} with is_active: true on an offboarded account now returns 500 (before this change it silently reactivated). Add a services.ErrUserOffboarded case returning 409 restapi.ErrCodeConflict, mirroring the v2 mapping in internal/restapi/v2/admin.go:455.

2. The v1 path silently gets no-op security invalidators (blocking)

NewUserReadService(db) (variadic lifecycle omitted) builds NewUserDeactivationService(db, UserDeactivationInvalidators{}) — internal/services/user_read_service.go:63-68. The v1 admin handler constructs its service exactly that way (internal/restapi/v1/handlers/admin_users.go:24). So a v1 deactivation now:

• deletes api_tokens rows but never evicts the token validation cache (tokens stay accepted until the 30s TTL per auth.TokenManager.InvalidateTokens) and never evicts session/permission caches;
• emits no user.deactivate / agent.deactivate / api_token.auto_revoke audit rows (the v1 handler ignores result.Cascade).

The variadic default makes a security dependency easy to omit. Options:

• drop the variadic and require the lifecycle service, threading the real userDeactivationService into v1.RegisterRoutes (e.g. via restapi.Deps); or
• if v1 must stay untouched, make the omission impossible to miss (separate read-only constructor, or a required non-nil parameter) instead of defaulting to no-ops.

The new service tests all pass an explicit lifecycle service, so they won't catch this; a test that constructs NewUserReadService(db) (as production v1 does), or a v1 HTTP test, would.

3. Repeated is_active=false now writes and bumps updated_at (non-blocking)

The branch is if update.IsActive != nil && !*update.IsActive { DeactivateUserTx }, so a PATCH setting is_active=false on an already-inactive user runs the full cascade and UPDATE users ... updated_at = CURRENT_TIMESTAMP. Previously that was a no-op. The cascade is idempotent so it's correct — but if the intent is to heal partial state, say so in the comment; otherwise gate on before.IsActive to avoid the needless write.

4. Minor

• LockUpdateProfileSnapshot's SQLite branch (UPDATE users SET id = id) is a write-lock idiom; a one-line comment would help, and it will fire any future UPDATE triggers on users.
• ErrUserOffboarded is checked after SCIMManaged, so a SCIM-managed + offboarded user gets 403 rather than 409. Probably intended, but confirm.
• DeactivateUserTx's "caller commits and then calls InvalidateDeprovisionCaches" contract is easy to forget; returning a pending-invalidation value would let the compiler enforce it.
• The concurrency test relies on SQLite's single write connection (writeConns=1); on Postgres the SELECT ... FOR UPDATE in LockUpdateProfileSnapshot carries the serialization — worth a comment so the SQLite dependency isn't mistaken for a general guarantee.

[stefan-ernst]

Review fixes pushed in 2b2c3b6; companion regressions in Windshiftapp/core-tests-internal#47 (da78863e).

1. v1 offboarded reactivation now returns 409 / CONFLICT / User has been offboarded and cannot be reactivated, without changing the profile. The documented v1 OpenAPI response was regenerated.
2. Removed the variadic/no-op mutation default. NewUserReadService is read-only and rejects UpdateAdmin; NewAdminUserService requires a non-nil lifecycle service. Both v1 (via required restapi.Deps.UserDeactivationService) and v2 receive the production service with real token/session/permission hooks. v1 now emits lifecycle and per-agent/per-token cascade audits, and rejects self-deactivation.
3. Documented intentional repeated deactivation: rerun the idempotent cascade to heal partially deactivated dependants.
4. Documented SQLite writer-lock/no-op UPDATE trigger behavior and PostgreSQL row-lock serialization in the concurrency test. External ownership intentionally precedes the offboarded guard, now pinned by a regression. Kept the explicit transaction-scoped DeactivateUserTx/post-commit contract rather than expanding the lifecycle API further; commit/rollback hook tests cover the boundary.

Validation:

../core-tests/overlay.sh . -- -tags=test -race -ldflags=-w -p=1 -timeout=180s -count=1 -run "Test(AdminUserLifecycle|V[12]AdminUserLifecycle|AdminUserManagement)" ./internal/services ./tests
# Same command with TEST_DB_TYPE=postgres and TEST_POSTGRES_DSN pointing to disposable PostgreSQL 17
../core-tests/overlay.sh . -- -tags=test -ldflags=-w -p=1 -timeout=10m -count=1 ./internal/restapi/v1/... ./internal/restapi/v2/... ./internal/services/...
go build -ldflags=-w -p=1 ./...
golangci-lint run ./...

All passed, along with pre-commit architecture and OpenAPI checks. Initial parallel validation hit local disk exhaustion; serial runs with DWARF disabled completed successfully. The HTTP credential/audit scenario now runs through both v1 PUT and v2 PATCH. Not merged.