Repository navigation
feat: orchestrate explicit full customer account deletion - #666
Conversation
|
Claude finished @mashkovd's task in 6m 26s —— View job Review complete — changes requested
Reviewed commit P1
P2
P3
What reads wellThe state machine is carefully built: the frozen manifest surviving tenant and principal removal, external effects strictly after the local disable, idempotent retries that never repeat destructive work, the consistent unobserved-vs-absent distinction (503 vs 409), and the ArgoCD missing-array hardening. I did not run |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| func customerDeletionPending(store *principals.Store) func(context.Context, string) (bool, error) { | ||
| return func(ctx context.Context, id string) (bool, error) { | ||
| if store == nil { |
There was a problem hiding this comment.
P1 — a nil principal store now breaks every new customer signup, even with CUSTOMER_DELETION_ENABLED off.
customerDeletionPending always returns a non-nil closure, so zitadelhook.Config.CreationPending is always non-nil at cmd/api/main.go:982. principalStore is legitimately nil in three non-fatal paths that main.go:303-320 deliberately logs and continues past:
PRINCIPALS_DISABLEDkill switch on- no
SURFACE_IDENTITY_DB_URLand noAUDIT_DB_URL - principal store init failure
In all three, this closure returns errors.New("customer deletion store unavailable") on every call, so Hook.Intent returns failure("intent_deletion_unreadable") (HTTP 503) and gateCreateUser returns failure("gate_deletion_unreadable") for every customer create — not just ones with a pending deletion. zitadelHookOptions only depends on CustomersOrgID, so the hook stays enabled while the store is absent, and this fencing is not behind CUSTOMER_DELETION_ENABLED, so the regression ships with the feature flag off. It also means PRINCIPALS_DISABLED, which exists as a rollback switch, now additionally kills customer self-registration.
Fix: only wire the probe when the store exists.
func customerDeletionPending(store *principals.Store) func(context.Context, string) (bool, error) {
if store == nil {
return nil
}
return store.CustomerDeletionPending
}Intent/gateCreateUser already no-op on a nil CreationPending, so fail-open here is the pre-existing behaviour rather than a new hole: a deletion cannot be in progress if no store can hold a manifest.
Code review in progress
|
| approvalGrantAuditParams(auditParams, user, consumed) | ||
| } | ||
|
|
||
| if opName == "delete-tenant" && !h.prepareCustomerAccountDeletion(w, r, input) { |
There was a problem hiding this comment.
P2 — this gate is inserted after the approval grant is spent, breaking the invariant stated 10 lines above.
handlers_write.go:425-427 says: "Spend the grant last, after every check that could still refuse, so a refused request leaves the approval usable." prepareCustomerAccountDeletion has six refusal paths (403 not a direct human admin, 400 invalid/protected principal, 400 customer_principal_id without the flag, 503 not configured / state unreadable / portal freeze failed, 409 ownership unverified). Each of them now runs after h.spendApprovalGrant, so the grant is consumed and the operator has to obtain a fresh approval to retry — exactly the failure the existing comment was written to prevent, and the most likely case in practice while the private cleanup endpoints are still rolling out (the 503 portal-freeze path).
Move the block above if grantAuthorized {.
| // delegated-human route. The target comes from the frozen approval manifest. | ||
| func (h *Handlers) AdvanceCustomerDeletion(w http.ResponseWriter, r *http.Request) { | ||
| u := auth.UserFromContext(r.Context()) | ||
| if u == nil || !u.IsService() { |
There was a problem hiding this comment.
P2 — bare u.IsService() admits relayed surface callers to the irreversible stages.
The repo already has the helper for "the service principal acting for itself": isDirectService in handlers_action_approvals.go:57-60, which is u.IsService() && !relayed. handlers_work_item_snapshots.go:74 applies the same rule to its workflow callback (if _, relayed := user.RelaySurface(); relayed || !user.IsService()). The doc comment here says "not a surface or delegated-human route", but the check does not enforce that half: a request carrying the service token through a surface relay has IsService() == true and passes.
That matters because the complete stage behind this gate performs the destructive, unrecoverable work (ZITADEL user deletion, session revocation, portal purge, principal removal). Note isHumanAdmin, used in prepareCustomerAccountDeletion, correctly excludes both relayed and surface callers — so the prepare side is stricter than the side that actually deletes.
Use !isDirectService(u) here.
| return nil | ||
| } | ||
| p := &customerdelete.Providers{ | ||
| Issuer: "https://auth.mctl.ai", CustomerOrg: cfg.ZitadelHook.CustomersOrgID, GitHubIDP: cfg.ZitadelHook.GitHubIdPID, |
There was a problem hiding this comment.
P2 — the ZITADEL issuer is hardcoded instead of taken from the configured provider entry.
Everything else on this line comes from cfg/env, but Issuer is pinned to the production hostname. The runtime ZITADEL issuer is cfg.oauthZitadelProvider().Issuer — the MCTL_OIDC_PROVIDERS entry named by OAUTH_ZITADEL_PROVIDER (main.go:1844-1860), which is also the issuer that linking writes identity rows under.
Two consequences when the two diverge (staging, a tenanted ZITADEL host, or just a trailing slash):
o.Issueris passed as theissuerargument toPrepareCustomerDeletion, which matches identities withprovider == "zitadel" && iss == issuer(internal/principals/customer_deletion.go:126). A mismatch falls into thedefaultbranch and returnsErrInvalid: principal has an unrelated identity, so the handler answers a permanent 409 "exclusive customer identity and ownership could not be verified" — a confusing failure for something that is purely a config mismatch, and the opposite of theerrCustomerDeletionUnobserved→ 503 signal the rest of the code is careful to distinguish.Providers.Issuerdoubles as the ZITADEL API base (/oauth/v2/token,/v2/users/...), so the lifecycle machine credential would be sent to the wrong host.
Derive it from cfg.oauthZitadelProvider() and return nil (with the existing slog.Error) if that entry cannot be read. SiteBase: "https://mctl.ai" on line 47 has the same hardcoding problem, though only the portal/site reachability depends on it.
| // ErrUnresolved. Answers are cached like ResolvePrincipal's, under their own | ||
| // key, so Forget drops them with the rest. | ||
| func (r *Resolver) CheckPrincipal(ctx context.Context, principalID string) error { | ||
| if err := r.checkCustomerDeletion(ctx, principalID); err != nil { |
There was a problem hiding this comment.
P2 — this puts an uncached database round trip on the per-request authentication path.
CheckPrincipal is called from auth.checkTokenPrincipal (internal/auth/principal.go:219-230) during token authentication, i.e. once per request for every local token carrying a pid. Until now the whole answer came from cachedLookup. The new tombstone probe is deliberately uncached, so every such request now also performs a SELECT EXISTS(... customer_deletions ...); ResolvePrincipal does two uncached reads for customer identities (CustomerSubjectDeleted before the cache plus checkCustomerDeletion after).
Two effects worth confirming are intended:
- One extra DB round trip per authenticated request, at the latency floor of the whole API.
checkCustomerDeletionmaps any read error toErrUnresolved, so the cache no longer absorbs a transient store failure: a brief DB blip now refuses authentication for every principal-mode caller.OAuthServer.checkMintPrincipalwidens that to token minting (IssueJWT/IssueRefreshToken/IssueCode).
The reasoning in the comment above — a warm replica must not outlive another replica beginning a deletion — is right, but it is satisfiable without an unconditional read: a short-TTL (a few seconds) negative cache, or a revocation epoch/counter bumped by BeginCustomerDeletion and read once per cache window, gives the same cross-replica bound at a fraction of the cost. The lifecycle test asserts correctness of the invalidation but nothing covers the added per-request cost.
| if s.TokenPrincipalChecker == nil || pid == "" { | ||
| return nil | ||
| } | ||
| ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) |
There was a problem hiding this comment.
P3 — context.Background() discards the caller context on a path that has one.
IssueCode already takes a ctx and passes it no further than its own body, so the mint check runs on a detached 3s deadline: client cancellation and any upstream deadline are lost. CLAUDE.md asks for context propagation on all I/O. Taking a ctx parameter in checkMintPrincipal and threading ctx from IssueCode (with context.Background() only for IssueJWT/IssueRefreshToken, which genuinely have none) keeps the intent explicit.
| } | ||
| res, err := client.Do(req) | ||
| if err != nil { | ||
| return 0, nil, fmt.Errorf("provider request failed") |
There was a problem hiding this comment.
P3 — the transport error is dropped, so retry triage loses its only signal.
CLAUDE.md: return errors with fmt.Errorf("context: %w", err). Every provider failure surfaces to the operator as a 503 with just {"stage": "zitadel"}, and this is the branch that distinguishes DNS failure from TLS rejection from connection timeout from the redirect refusal that CheckRedirect installs. fmt.Errorf("provider request failed: %w", err) costs nothing here: the URL and credentials are not in err for a *url.Error from Do beyond the request URL, which is not secret.
| return failure("gate_deletion_unreadable") | ||
| } | ||
| if pending { | ||
| return no("gate_deletion_in_progress") |
There was a problem hiding this comment.
P3 — the gate half of the signup fence is untested.
TestIntentBlocksCreationDuringDeletion covers all three outcomes on the Intent path, but nothing exercises gate_deletion_in_progress / gate_deletion_unreadable here. The two paths differ in more than the reason string — this one returns no(...), whose operator-facing message is the fixed "Customer accounts are created only through GitHub sign-in.", which is misleading for a pending deletion, whereas Intent uses msgNotAllowed. A table case mirroring the intent test over CreateGate would pin both the refusal and the message choice.
| "strings" | ||
|
|
||
| "github.com/go-chi/chi/v5" | ||
| "github.com/jackc/pgx/v5" |
There was a problem hiding this comment.
P3 — importing pgx into the HTTP layer to compare one sentinel.
errors.Is(readErr, pgx.ErrNoRows) at line 150 is the only use, and it makes the CustomerDeletionStore interface — which exists so the handler is testable without a database — carry an implicit pgx contract that only the comment conveys (the test fake has to know to return pgx.ErrNoRows). A principals.ErrNotFound-style sentinel translated at the store boundary would keep internal/api free of the driver, consistent with the interface-based design CLAUDE.md calls for.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 1 P1, 4 P2, 3 P3.
P1: customerDeletionPending always returns a non-nil closure, so the ZITADEL signup fence is wired even when principalStore is nil (PRINCIPALS_DISABLED, no principal DB URL, or store init failure — all non-fatal in main.go). Every customer create then answers 503/403 regardless of whether a deletion is pending, and this is not behind CUSTOMER_DELETION_ENABLED, so it ships with the flag off.
P2: the new gate in ExecuteOperation runs after spendApprovalGrant, burning the grant on every refusal path and contradicting the invariant documented 10 lines above; AdvanceCustomerDeletion authorizes with bare IsService() instead of the existing isDirectService, admitting relayed surface callers to the irreversible stages; the ZITADEL Issuer is hardcoded rather than read from the OAUTH_ZITADEL_PROVIDER entry, which both mismatches stored identity rows and aims the lifecycle credential at a fixed host; the tombstone probe adds an uncached DB read to CheckPrincipal, which runs per request in token authentication.
The state machine itself reads well: the frozen manifest, the prepare/begin/complete ordering with external effects after the local disable, idempotent retries, and the strict unobserved-vs-absent distinction are all handled carefully, and the test coverage for those invariants is genuinely good.
|
Claude finished @mashkovd's task in 8m 21s —— View job Code review complete — changes requested
1 P2, 5 P3 — posted as inline comments on Previous round resolvedAll four blocking findings from the review of
The uncached tombstone read on the authentication path is now an explicitly argued contract in Remaining P2 (new)
P3
What reads wellThe state machine is careful: the frozen manifest with a partial unique index on pending Note: |
| } | ||
| d, err := o.Store.CustomerDeletion(r.Context(), tenant, pid) | ||
| if err != nil { | ||
| writeError(w, 409, "approved customer deletion manifest unavailable") |
There was a problem hiding this comment.
P2 — a read failure here answers 409. Store.CustomerDeletion returns the raw pgx error, so an unreachable or slow Postgres is reported as a conflict instead of 503 — exactly the conflation prepareCustomerAccountDeletion is careful to avoid at line 130. The deletion workflow drives these stages by retrying, and a terminal 4xx on a transient read can strand a half-torn-down account (principal already disabled, portal frozen, IdP user still present). Distinguish pgx.ErrNoRows (409) from any other error (503), as the prepare path does.
| approvalGrantAuditParams(auditParams, user, consumed) | ||
| } | ||
|
|
||
| if opName == "delete-tenant" && !h.prepareCustomerAccountDeletion(w, r, input) { |
There was a problem hiding this comment.
P2 — prepare has durable side effects before anything is submitted: the frozen manifest row, the signup fence (CustomerDeletionPending is true for any status other than complete, and the partial unique index on github_id blocks a second manifest) and the portal freeze. If Executor.Submit just below fails, the handler returns 500 and nothing releases them, and this PR adds no cancel or abort stage — so the customer stays portal-frozen and unable to re-register until an operator edits customer_deletions by hand. Either release the freeze and drop the prepared manifest when Submit fails, or add an explicit cancel stage to AdvanceCustomerDeletion.
| } | ||
| u := auth.UserFromContext(r.Context()) | ||
| if u == nil || !isHumanAdmin(u) || u.IsAgent() || u.ViaPrincipalID() != "" || u.PrincipalID() == "" { | ||
| writeError(w, 403, "complete customer deletion requires a direct human platform administrator") |
There was a problem hiding this comment.
P2 — only the success path is audited (line 174). Every refusal in this function (403 non-admin, 400 protected principal, 409 ambiguous ownership, 503 unreadable) returns before Submit, so an attempt by a non-admin, or against a protected principal, leaves no audit record at all. ExecuteOperation audits its denials for exactly this reason — see the create-tenant and approver-relay branches in this file, where a previous review raised the same gap. Please audit these refusals with Status: "denied".
| for _, query := range []string{ | ||
| `DELETE FROM oauth_refresh_tokens WHERE principal_id=$1`, | ||
| `DELETE FROM external_identities WHERE principal_id=$1`, | ||
| `DELETE FROM principals WHERE id=$1 AND status='disabled' AND kind='human'`, |
There was a problem hiding this comment.
P2 — the result of this DELETE is never checked, unlike BeginCustomerDeletion, which refuses when RowsAffected() != 1. If the principal was re-activated between begin and finish (Store.SetStatus permits active), the status=disabled predicate matches nothing, the row survives — yet the manifest is still flipped to complete two statements later and the handler reports a clean teardown. For a flow whose premise is strict postconditions, require exactly one affected row here (or re-assert the status under the lock before deleting).
|
|
||
| // WithCustomerDeletionLock serializes external effects across API replicas. | ||
| // Its lock is separate from the short store transactions used by each stage. | ||
| func (s *Store) WithCustomerDeletionLock(ctx context.Context, tenant, pid string, fn func() error) error { |
There was a problem hiding this comment.
P2 — this holds an open transaction, and its pooled connection, with pg_advisory_xact_lock for the entire teardown: ZITADEL verify plus delete, site session revoke, portal cleanup and release, each up to 15s. Two consequences:
BeginCustomerDeletionandFinishCustomerDeletioncallwithTxfrom inside the callback, so each stage needs a second pooled connection while the first is parked idle-in-transaction. Concurrent retries (the workflow retries by design) can hold every pool connection on an outer transaction and then all block acquiring the inner one until the 30s request timeout.- A Postgres
idle_in_transaction_session_timeoutwould kill the outer transaction and silently release the advisory lock while the external effects are still in flight — losing precisely the cross-replica exclusion this function exists to provide.
A lease column in customer_deletions (or a session-level advisory lock on a dedicated connection with an explicit renewal) avoids both.
| // ErrUnresolved. Answers are cached like ResolvePrincipal's, under their own | ||
| // key, so Forget drops them with the rest. | ||
| func (r *Resolver) CheckPrincipal(ctx context.Context, principalID string) error { | ||
| if err := r.checkCustomerDeletion(ctx, principalID); err != nil { |
There was a problem hiding this comment.
P2 — CheckPrincipal sits on the per-request authentication path (auth.checkTokenPrincipal runs for every local token carrying a pid claim), and this tombstone read is deliberately uncached. So every authenticated request now costs a Postgres round-trip and can block up to ResolveTimeout (3s). More importantly, a store blip used to be absorbed by the 5-minute cache; now it returns ErrUnresolved, which clears the principal and makes principal mode refuse the request — for every caller, not just customers under deletion. The CustomerSubjectDeleted read at line 143 has the same shape on ResolvePrincipal.
Caching only the negative answer ("not deleted") for a few seconds would keep the revocation window tight (Forget plus the DB tombstone still bound it) while restoring the warm path.
| return nil | ||
| } | ||
| p := &customerdelete.Providers{ | ||
| Issuer: "https://auth.mctl.ai", CustomerOrg: cfg.ZitadelHook.CustomersOrgID, GitHubIDP: cfg.ZitadelHook.GitHubIdPID, |
There was a problem hiding this comment.
P2 — the ZITADEL issuer and the site base are hardcoded to production. The issuer is not only a provider base URL: it is frozen onto the manifest and PrepareCustomerDeletion matches it against the stored identity (provider == "zitadel" && iss == issuer). So in any deployment whose issuer is not https://auth.mctl.ai, every target is refused as "an unrelated identity", while the provider HTTP calls would be aimed at the production IdP and the production site from a non-production cluster. The configured value already exists — cfg.oauthZitadelProvider().Issuer, used by oauthZitadelOptions in main.go — please take it from there, and make SiteBase configurable alongside PortalBase.
| // delegated-human route. The target comes from the frozen approval manifest. | ||
| func (h *Handlers) AdvanceCustomerDeletion(w http.ResponseWriter, r *http.Request) { | ||
| u := auth.UserFromContext(r.Context()) | ||
| if u == nil || !u.IsService() { |
There was a problem hiding this comment.
P3 — u.IsService() alone admits every service principal, not only the platform deletion workflow, to a route that disables a principal and revokes its sessions. The blast radius is bounded (an admin-approved manifest must already exist), but this package has isDirectService (handlers_action_approvals.go), which also excludes a relayed surface principal, and the router has the narrower precedent of usageWriterGate / ciPrincipalGate for single-route principals. Scoping this to the lifecycle principal would match them.
| if s.TokenPrincipalChecker == nil || pid == "" { | ||
| return nil | ||
| } | ||
| ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) |
There was a problem hiding this comment.
P3 — context.Background() discards the caller context; IssueCode has a ctx it could pass down (CLAUDE.md asks for context propagation on all I/O), and only IssueJWT/IssueRefreshToken genuinely lack one. Worth noting too that ExchangeCode now makes two checker round-trips (IssueJWT and IssueRefreshToken) after the authorization code has already been consumed, so a transient store failure burns the code and forces a full re-login rather than a retry.
| } | ||
| res, err := client.Do(req) | ||
| if err != nil { | ||
| return 0, nil, fmt.Errorf("provider request failed") |
There was a problem hiding this comment.
P3 — the transport error is discarded: fmt.Errorf("provider request failed") keeps no cause, so a DNS failure, a TLS failure, a context cancellation and a timeout are indistinguishable in logs during an operation that is expected to be retried until it succeeds. CLAUDE.md asks for fmt.Errorf("context: %w", err). If the concern is leaking the URL into a caller-visible message, wrap err here and redact at the HTTP boundary instead.
| // WithCustomerDeletionLock serializes external effects across API replicas. | ||
| // Its lock is separate from the short store transactions used by each stage. | ||
| func (s *Store) WithCustomerDeletionLock(ctx context.Context, tenant, pid string, fn func() error) error { | ||
| return s.withTx(ctx, "customer-deletion-run|"+tenant+"|"+pid, func(pgx.Tx) error { return fn() }) |
There was a problem hiding this comment.
P2 — the cross-replica deletion lock holds a Postgres transaction open across the entire external teardown, and acquires further pool connections from inside it.
withTx (internal/principals/store.go:362-378) opens a transaction, takes pg_advisory_xact_lock(hashtext(key)), then runs fn. Here fn is the whole run() closure in the handler (internal/api/handlers_customer_deletion.go:214-270): o.Store.CustomerDeletion (a separate pool.QueryRow), BeginCustomerDeletion / FinishCustomerDeletion (each its own pool.Begin), and every provider call — CheckPlatformGone (two namespace GETs plus an Argo list), DeleteCustomer (token + GET + DELETE + GET), RevokeSiteSessions, CleanPortal, ReleasePortal — each on the 15s client timeout in customerdelete.Providers.request.
Three consequences:
- One connection sits idle-in-transaction for the full teardown, plausibly a minute or more when a provider is slow. That is the same pool the new tombstone probes use on the per-request authentication path (
internal/principals/resolver.go:165-180, reached fromCheckPrincipalfor every principal-mode request), so a stuck teardown consumes capacity exactly where latency matters most. - The nested
pool.Begininside the outer transaction is a pool-exhaustion deadlock vector: the outer transaction holds one connection and then waits for another. The advisory lock serializes only per(tenant, principal), so concurrent teardowns of different targets, plus ordinary traffic, can hold every connection while each waits for an inner one until the context deadline. pg_advisory_xact_lockis released when the session ends. If a pooler oridle_in_transaction_session_timeoutkills the session mid-run, the lock is silently gone, another replica may enter the same irreversible stages concurrently, and this run discovers it only whentx.Commitfails — after every external effect has already happened.
A session-level pg_try_advisory_lock on one dedicated connection (released in a defer), or a lease row with a heartbeat, gives the same cross-replica exclusion without holding a transaction across provider I/O.
Same pattern at lines 86 and 141: PrepareCustomerDeletion runs the validate callbacks supplied by the handler inside its transaction while holding both the deletion lock and lockPrincipalTx(pid). Those callbacks are customerMemberships (a full GitOps checkout read) and o.VerifyCustomer (three ZITADEL round trips), so every other principal operation for that principal blocks for their duration.
| "strings" | ||
|
|
||
| "github.com/go-chi/chi/v5" | ||
| "github.com/jackc/pgx/v5" |
There was a problem hiding this comment.
P3 — carried findings from earlier rounds, still open on this commit, plus one small gap new in d0bb009.
- The new
previous.Cancelled409 has no HTTP test. Line 200 adds a distinct code and message ("prepared cancellation is awaiting portal acknowledgment; retry cancellation") for preparing against an unacknowledged receipt. The store-level equivalent is pinned by the real-Postgres test (customer_deletion_test.go:318, "cancel receipt reused as a new deletion intent"), so the behaviour is covered — but the handler's distinct, operator-facing message is not, and it is the one an administrator actually sees. pgxin the HTTP layer (line 26).errors.Is(..., pgx.ErrNoRows)at lines 196, 281 and 450 are the only uses, and they makeCustomerDeletionStore— an interface that exists so the handler is testable without a database — carry an implicit pgx contract that only the comments convey. Aprincipals.ErrNotFound-style sentinel translated at the store boundary would keepinternal/apifree of the driver, per the interface-based design in CLAUDE.md.- Detached context in the mint path.
checkMintPrincipal(internal/auth/oauth_server.go:1458) builds its 3s deadline fromcontext.Background()whileIssueCodehas a realctxto thread; CLAUDE.md asks for context propagation on all I/O. Related: inIssueCodethe check runs beforeValidPrincipalID, so a malformed id costs a store lookup and returns the generic refusal instead ofmalformed principal id %q; andExchangeCodemakes two checker round trips after the authorization code is consumed, so a transient store failure burns the code and forces a full re-login. - The
resourcesreplay answers 409 before the lock and 503 inside it (lines 279 and 394), for an operation that can never succeed; the in-lock branch is the one the re-read exists for and still has no test. Same mechanism as the cancelled-manifest finding above. - The
resourcesstage deletes nothing yet is audited ascustomer.delete.resources succeededand holds one of only two per-replicadeletionSlotsplus a dedicated Postgres connection and the tenant-wide advisory lock — now shared with prepare and cancel, so three different customers' operations contend for two slots. cmd/api/customer_deletion.go:111—customerDeletionAppRemainsdropsProject, whichinternal/api/service_listing.go:163treats as an ownership signal and which, unlike a name prefix, is an exact match that cannot capture a sibling workspace.internal/mcp/server.go:1157—regexp.MustCompileper call, where every other pattern in the package is a package-level var andinternal/apialready holds this exact expression; the new entries inannotations_test.goanddocs/portal-allowlist.jsonare also out of alphabetical order.- A lock failure after the inner refusal writes a second
deniedaudit entry. InprepareCustomerAccountDeletion(line 151), whenranis true andallowedis false the inner function has already emitted its deferreddeniedentry, so this adds a second row for the same attempt with a differentMessage; gating on!rankeeps one entry per attempt. Line 140 also restates the full authorization predicate from lines 177-186 to decide whether to take the lock. internal/zitadelhook/hook.go:824— thegateCreateUserhalf of the signup fence is still untested and still returnsno(...), whose fixed message ("Customer accounts are created only through GitHub sign-in.") is wrong for a pending deletion, whereIntentcorrectly usesmsgNotAllowed.
Resolved in d0bb009, for the record: the retried cancellation no longer sends an empty login (the fabricated manifest is gone, so portalLifecycle always carries the verified identity), and WithCustomerDeletionLock's comment now states the tenant-only granularity and why pid is still accepted.
|
Claude finished @mashkovd's task in 7m 41s —— View job Code review complete — changes requested
2 P2, 2 P3 — posted as inline comments on Resolved in
|
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 1 P2, 3 P3.
Resolved in d0bb009. The P2 from the review of 1efa8a3 is fixed the way the release direction already worked, which is the right symmetry. CancelCustomerDeletion now stamps cancelled_at instead of deleting, FinishCustomerCancellation removes the receipt only after CancelPortal succeeds, and CustomerTenantDeletionPending deliberately does not exclude cancelled rows — so the name fence survives a failed acknowledgment while CustomerDeletionPending, CustomerSubjectDeleted, CustomerPrincipalDeleted, BeginCustomerDeletion and ResolveCustomer all gained AND cancelled_at IS NULL and restore the customer immediately. That split is exactly the one the finding asked for: deny fences lift at once, the name fence waits for the portal.
It also closes the two P3s from that round on its own terms. The fabricated &CustomerDeletion{Tenant, PrincipalID} is gone, so portalLifecycle always carries the verified login and an absent pair performs no portal mutation at all; the invariant "receipt absent implies portal acknowledged" makes the old fabrication unnecessary rather than merely risky. WithCustomerDeletionLock's comment now states the tenant-only granularity and why pid is still accepted. Coverage is in the right places too — the handler test walks local-write failure, portal failure, a refused begin against the cancelled receipt, a failed durable acknowledgment that keeps both the receipt and the name freeze, then the successful retry; and the real-Postgres test asserts the receipt keeps its exact login, that signup is unfenced while the name stays frozen, that the receipt cannot be reused as a new intent, and that only FinishCustomerCancellation releases the name.
Remaining P2. customer_deletion_pending_github is still UNIQUE (github_id) WHERE status <> 'complete' and did not get the cancelled_at IS NULL clause the five predicates above did. Because a cancelled receipt keeps status='prepared', it now holds that unique slot until the portal acknowledges — where the previous DELETE freed it immediately. A customer restored by a cancellation whose portal call failed can create a replacement workspace, and preparing a deletion for that workspace then fails the INSERT on the stale receipt and surfaces as 409 "exclusive customer identity and ownership could not be verified", pointing the operator at identity and GitOps membership rather than at the pending cancellation. It fails closed and retrying the cancellation clears it, but this commit gives the same-tenant version of that situation a precise, actionable 409, so the other-tenant version answering an identity error reads as an oversight. One clause on the index predicate (plus a DROP INDEX IF EXISTS, since CREATE ... IF NOT EXISTS will not redefine it) restores the pre-commit behaviour.
P3s: a cancelled manifest is terminal 409 at the pre-lock read but 503 "retry" at the in-lock re-read, which is the same shape as the open resources replay inconsistency and equally untested; the absent-pair no-op returns 200 and audits "prepared deletion cancelled", contradicting the documented stance that it is not evidence any such deletion existed, and the now-dead else hides that the branch no longer falls through to the portal; plus the carried bundle (untested handler-level cancelled 409, pgx in the HTTP layer, the mint-path context, the resources replay and its no-op slot consumption, customerDeletionAppRemains dropping Project, regexp.MustCompile per call in the MCP tool, the duplicate denied entry in the prepare wrapper, the untested gateCreateUser fence).
Note that the retained receipt makes an unacknowledged cancellation freeze the tenant name with no local override — but that is the deliberate, symmetric cost of the released_at contract this PR already documents and accepts, and the customer themselves is not fenced, so it is not raised as a finding.
Note: go build / go vet / go test could not be run in this job (commands not permitted), so this is a static review of the pinned diff 0ab7605...d0bb009.
Antigravity (agy) reviewagy returned a result this workflow could not use; see the workflow log. Treated as a failed review. |
| if err != nil { | ||
| return err | ||
| } | ||
| if total != 2 || matching != 2 { |
There was a problem hiding this comment.
P2 — the exactly-two-identity invariant this recheck enforces counts mirrored surface identities, so a customer who has ever linked a surface can never be fully deleted, and a redeem landing in the prepare→begin window strands the manifest.
Every live surface link is mirrored into external_identities as (provider=<surface>, issuer='', subject=<external id>) — internal/principals/mirror.go:102-104, called unconditionally from the redeem path (internal/surfaceid/store.go:449,469 via mirrorLink), with the mirror installed whenever a principal store exists (cmd/api/main.go:658-662). That is the same condition under which customerDeletionOptions can be configured at all, so the two features always coexist.
Two consequences:
-
Prepare is permanently impossible for such a customer.
PrepareCustomerDeletionreads all identity rows for the principal (line 171, no provider filter) and refuses anything that is not GitHub-with-empty-issuer or ZITADEL-at-the-configured-issuer: lines 195-198,ErrInvalid: principal has an unrelated identity. A customer with a live Telegram link has three rows, so it is refused. Worse,MirrorRevokeonly stampsrevoked_atand leaves the row in place (mirror.go:130-131), and prepare also refuses any revoked row (lines 185-188,revoked identity needs reconciliation) — so a customer who linked and later unlinked a surface is refused forever. The handler maps both to409 "exclusive customer identity and ownership could not be verified"(internal/api/handlers_customer_deletion.go:250), pointing the administrator at the customer's GitOps memberships and IdP identity rather than at the surface mirror row that is the actual cause. -
This new recheck extends it to
begin. A challenge created before prepare can be redeemed by the surface after it — redeem is driven by the surface with the possession code and the principal was fixed at challenge creation, so it needs no customer authentication, which matters because90a42efalready fences the customer out of the API atprepared. The redeem writes a mirror row,totalbecomes 3, andbeginrefuses on every retry. The manifest is then stuck atprepareduntil an administrator cancels, and because the mirror row is never removed — only revoked — re-preparing afterwards hits case (1).
The strongest evidence that this is unintended rather than chosen: FinishCustomerDeletion goes out of its way to clean up surface state, with a comment explaining the legacy keying (lines 347-353: surface_identity_links and surface_identity_challenges, by pid and by github:<login>). For live links that code is unreachable, because no principal holding one can be prepared. docs/principals.md describes the exactly-two rule as "never a staff/service account or a mixed principal" and says nothing about surface links.
It is also why no test catches it: TestCustomerDeletionRemovesOldIdentityAndPermitsFreshSubject (internal/principals/customer_deletion_test.go:50-56) inserts into surface_identity_links / surface_identity_challenges directly with s.pool.Exec, bypassing MirrorLink. The fixture therefore builds a state the running system cannot produce — surface links with no mirrored identity — and proves the cleanup works on exactly the case that can never reach it.
This fails closed, so no wrong target is ever deleted. What needs deciding before the flag goes on is whether surface-linked customers are in scope. If they are, both the prepare classification and this recheck should treat non-reserved-provider mirror rows as part of the customer, and FinishCustomerDeletion should remove them alongside the link rows it already deletes. If they are not, prepare should say so in its own refusal instead of answering an ownership error, and the unreachable cleanup should go.
| ALTER TABLE customer_deletions ADD COLUMN IF NOT EXISTS released_at TIMESTAMPTZ; | ||
| ALTER TABLE customer_deletions ADD COLUMN IF NOT EXISTS cancelled_at TIMESTAMPTZ; | ||
| CREATE UNIQUE INDEX IF NOT EXISTS customer_deletion_pending_github | ||
| ON customer_deletions(github_id) WHERE status <> 'complete'; |
There was a problem hiding this comment.
P2 (raised on d0bb009, still open) — the partial unique index on github_id did not get the cancelled_at exclusion that the five fence predicates got, so a retained cancellation receipt blocks the next preparation for the same customer with a misleading error.
customer_deletion_pending_github is UNIQUE (github_id) WHERE status <> 'complete'. Since d0bb009, cancellation no longer deletes the row: CancelCustomerDeletion (line 376) stamps cancelled_at and leaves status='prepared' until FinishCustomerCancellation runs after a successful CancelPortal. A cancelled-but-unacknowledged receipt therefore still occupies the unique slot, where before that commit the DELETE freed it immediately.
Meanwhile the same commit added AND cancelled_at IS NULL to CustomerDeletionPending (262), CustomerSubjectDeleted (268), CustomerPrincipalDeleted (274), BeginCustomerDeletion (286) and ResolveCustomer (internal/principals/link.go:462) — correctly, since a cancelled intent is not a pending deletion. This index is the one remaining place where it still counts as one.
Concretely, and this is the expected rollout state rather than an exotic one, since the PR describes the portal's cancellation release as the last component to deploy:
- Cancel
(acme, pidX).cancelled_atis stamped,ForgetPrincipalsruns,CancelPortalreturns non-200, the handler answers 503. The receipt stays atstatus='prepared'. - The customer is fully restored by design — all three deny predicates exclude cancelled — so they sign in and create a replacement workspace
acme2;create-tenantisRoleAuthenticated, and only the nameacmeis fenced. - An administrator prepares
(acme2, pidX).PrepareCustomerDeletionfinds no row for that key — theCancelledguard at line 154 is keyed on(tenant, pid)and does not see theacmereceipt — builds the manifest, and theINSERTat line 233 violatescustomer_deletion_pending_githubagainst the stale(acme, pidX)row. - Nothing in this package maps
23505(eight call sites elsewhere in the repository do), so the raw unique-violation error is neithererrCustomerDeletionUnobservednorErrInvalid, andinternal/api/handlers_customer_deletion.go:250answers409 "exclusive customer identity and ownership could not be verified"— pointing the operator at the customer's identity and GitOps memberships rather than at the pending cancellation receipt that is the actual cause.
It fails closed and is recoverable by retrying the cancellation, so no deletion runs against a wrong target. What makes it worth fixing is that d0bb009 handles the same-tenant version of this situation with a precise, actionable 409 — "prepared cancellation is awaiting portal acknowledgment; retry cancellation" (internal/api/handlers_customer_deletion.go:200) — so the other-tenant version answering a misleading ownership error reads as an oversight rather than a choice.
Aligning the index with the predicates is one clause:
CREATE UNIQUE INDEX IF NOT EXISTS customer_deletion_pending_github
ON customer_deletions(github_id) WHERE status <> 'complete' AND cancelled_at IS NULL;CREATE UNIQUE INDEX IF NOT EXISTS will not redefine an index that already exists under the old predicate, so a cluster that already ran an earlier commit of this branch needs a DROP INDEX IF EXISTS customer_deletion_pending_github; ahead of it. Harmless to add, given the flag has never been enabled in production.
| return err | ||
| } | ||
| if total != 2 || matching != 2 { | ||
| return fmt.Errorf("%w: approved customer identities changed before begin", ErrInvalid) |
There was a problem hiding this comment.
P3 (new in 33f66bb) — a permanent identity-drift refusal is reported to the workflow as a retryable 503.
ErrInvalid propagates out of BeginCustomerDeletion into run(), and the tail of AdvanceCustomerDeletion maps every error to writeErrorCode(w, 503, "customer_deletion_incomplete", "Customer deletion incomplete; retry", ...) (internal/api/handlers_customer_deletion.go:410-418). An administrator merge is not transient — the identities now belong to another principal and nothing in this flow moves them back — so the lifecycle workflow is told to retry something that can never succeed, and the audit Message an operator reads says "incomplete; retry" alongside "approved customer identities changed before begin".
The correct recovery is the one the new doc paragraph describes (cancel the prepared intent, then re-approve against the surviving principal), and a 409 is what every sibling terminal condition in this handler answers: the cancelled manifest at line 283, the protected principal at line 301, the completed-replay resources at line 294. This is the third instance of the same shape (the cancelled and resources in-lock re-reads are already open), so whatever mechanism is chosen there — a sentinel the tail maps to 409, or the conflict flag CancelCustomerDeletion already uses — would cover this one too.
Two smaller notes on the new query, neither a defect:
total = count(*)deliberately counts revoked rows, which makes the check strictly stricter without false positives, because prepare already refuses any principal holding a revoked identity (line 185-188). Worth a half-sentence in the comment, since the asymmetry between the unfilteredcount(*)and therevoked_at IS NULLfilter otherwise reads as an oversight.matching = 2cannot be reached by two rows satisfying the same clause:external_identitiesisUNIQUE (provider, issuer, subject)(internal/principals/store.go:99), so the check really does mean "precisely the two approved identities, both live". That is a stronger invariant thanPrepareCustomerDeletionitself enforces, which is the right direction for a gate in front of irreversible work.
The doc claim that "existing identity-mutation rules prevent moving its live identities" once disabled checks out: mergePrincipals is the only path that reassigns principal_id for an authentication provider (internal/principals/link.go:334) and refuses a disabled source holding live identities (lines 308-318), while LinkIdentity refuses a non-active principal (line 131) and an identity owned elsewhere (line 139). mirrorLink's ON CONFLICT ... DO UPDATE SET principal_id (internal/principals/mirror.go:96-104) also repoints ownership, but reservedProvider keeps it off github/dex/service/dev, so it cannot move either approved identity.
| "strings" | ||
|
|
||
| "github.com/go-chi/chi/v5" | ||
| "github.com/jackc/pgx/v5" |
There was a problem hiding this comment.
P3 — carried findings from earlier rounds, still open on this commit.
- The no-op cancellation is audited as a cancellation that happened. The
pgx.ErrNoRowsbranch returns nil (line 462), so the tail at lines 492-493 setsstatus, message = "succeeded", "prepared deletion cancelled"and the body says{"status": "cancelled"}.docs/principals.mdstates the opposite — "An absent pair performs no portal mutation: it confirms the desired final state, not evidence that such a deletion previously existed" — yet the only durable record asserts a prepared deletion was cancelled, so an operator cannot distinguish a real cancellation from a mistyped pair that matched nothing. A distinct message (and ideally a distinctstatusin the body) keeps the trail honest. The relatedelseat line 463 is also dead structure now: every path in the no-rows branch returns, so the sharedCancelPortal+FinishCustomerCancellationtail is reachable only from the second branch, and an early return would make "an absent pair performs no portal mutation" visible in the control flow rather than inferred. pgxin the HTTP layer (line 26) —errors.Is(..., pgx.ErrNoRows)at lines 196, 281 and 451 are the only uses, and they makeCustomerDeletionStore(an interface that exists so the handler is testable without a database) carry an implicit pgx contract that only the comments convey. Aprincipals.ErrNotFound-style sentinel translated at the store boundary would keepinternal/apifree of the driver, per the interface-based design in CLAUDE.md.- Terminal-vs-retryable split across the two manifest reads.
resourceson a completed deletion is a terminal 409 before the lock (line 294) but a 503 "retry" from the in-lock re-read (line 394); the same is true of the cancelled manifest (409 at line 283, 503 from line 392). The in-lock branches are the ones the re-read exists for and neither has a test. - A lock failure after the inner refusal writes a second
deniedaudit entry. InprepareCustomerAccountDeletion(line 151), whenranis true andallowedis false the inner function has already emitted its deferreddeniedentry, so this adds a second row for the same attempt with a differentMessage; gating on!rankeeps one entry per attempt, which is the distinction the response handling one line below already draws. Line 140 also restates the authorization predicate from lines 177-186 to decide whether to take the lock, where a drift in one direction would skip the lock for a request that goes on to insert a manifest. - Detached context in the mint path —
checkMintPrincipal(internal/auth/oauth_server.go:1458) builds its 3s deadline fromcontext.Background()whileIssueCodehas a realctxto thread; CLAUDE.md asks for context propagation on all I/O. Related: the check runs beforeValidPrincipalID, so a malformed id costs a store lookup and returns the generic refusal instead ofmalformed principal id %q, andExchangeCodemakes two checker round trips after the authorization code is consumed, so a transient store failure burns the code. - The
resourcesstage deletes nothing yet is audited ascustomer.delete.resources succeededand holds one of only two per-replicadeletionSlotsplus a dedicated Postgres connection and the tenant-wide advisory lock, now shared with prepare. cmd/api/customer_deletion.go:111—customerDeletionAppRemainsdropsProject, whichinternal/api/service_listing.go:163treats as an ownership signal and which, unlike a prefix, is an exact match that cannot capture a sibling workspace.internal/principals/customer_deletion.go:77—WithCustomerDeletionLockstill takes apidit no longer reads; the comment now explains why, butunparam/reviveare not enabled, so nothing will catch a later change that starts relying on the parameter.internal/mcp/server.go:1157—regexp.MustCompileper call, where every other pattern in the package is a package-level var andinternal/apialready holds this exact expression; the new entries inannotations_test.goanddocs/portal-allowlist.jsonare also out of alphabetical order.internal/zitadelhook/hook.go:824— thegateCreateUserhalf of the signup fence is still untested and still returnsno(...), whose fixed message ("Customer accounts are created only through GitHub sign-in.") is wrong for a pending deletion, whereIntentcorrectly usesmsgNotAllowed.
Resolved since the d0bb009 review, for the record: the empty-login retry is gone. A retained receipt has status='prepared', so a retried cancellation takes the second branch, re-stamps cancelled_at idempotently via COALESCE, and calls CancelPortal with the full manifest including the verified login.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 2 P2, 2 P3.
Resolved in 33f66bb. The pre-begin identity recheck is a real strengthening and it is placed correctly: under the same principal lock as the status read and the disable, before any credential is touched, and prepared-only, which matches where VerifyCustomer already runs. The predicate is exactly right — external_identities is UNIQUE (provider, issuer, subject), so matching == 2 can only mean both frozen identities are live on this principal, and total == 2 excludes any extra. That makes it stricter than PrepareCustomerDeletion itself, which is the right direction for a gate in front of irreversible work. The real-Postgres test is the one worth having: it performs an actual MergePrincipals, asserts begin refuses, asserts the manifest stays prepared, asserts the merge target is untouched, and then proves the intent is still cancellable end to end. The doc claim that a disabled principal's live identities cannot be moved also checks out — mergePrincipals is the only reassignment path for an authentication provider and it refuses a disabled source holding live identities, LinkIdentity refuses a non-active principal, and mirrorLink's repointing upsert is kept off reserved providers.
P2 (new). The exactly-two-identity invariant — in PrepareCustomerDeletion and now in this recheck — counts mirrored surface identities. Every live surface link is written into external_identities as (provider=<surface>, issuer='', subject=<external id>) by mirrorLink, unconditionally on the redeem path, with the mirror installed in exactly the deployments where this feature can be configured. So a customer with a Telegram link has three rows and prepare refuses; and because MirrorRevoke only stamps revoked_at, a customer who linked and later unlinked is refused forever — reported as 409 "exclusive customer identity and ownership could not be verified", which points the administrator at GitOps memberships rather than at the mirror row. A redeem landing between prepare and begin strands the manifest at prepared the same way. The clearest sign this is unintended: FinishCustomerDeletion carefully cleans surface_identity_links for both the pid and legacy github:<login> forms, and for live links that code is unreachable; the test that covers it builds the state with raw pool.Exec inserts, bypassing MirrorLink, so it proves the cleanup on a state the running system cannot produce.
P2 (carried from d0bb009, unchanged). The partial unique index customer_deletion_pending_github is still WHERE status <> 'complete' while five fence predicates gained AND cancelled_at IS NULL. A retained cancellation receipt therefore still occupies the unique slot, so after a failed CancelPortal the restored customer's next preparation on a different tenant hits a raw 23505 that no code in the package maps, surfacing as the same misleading ownership 409 — in contrast to the precise "prepared cancellation is awaiting portal acknowledgment; retry cancellation" that d0bb009 added for the same-tenant case.
P3s: the new drift refusal is reported to the workflow as a retryable 503 although a merge is permanent, which is the third instance of the terminal-vs-retryable split already open for the cancelled and resources in-lock re-reads; plus the carried bundle (the no-op cancellation audited as "prepared deletion cancelled" with its now-dead else, pgx in the HTTP layer, the duplicate denied entry and restated predicate in the prepare wrapper, the mint-path context, the no-op resources stage holding a deletion slot, customerDeletionAppRemains dropping Project, the ignored pid on the tenant-keyed lock, regexp.MustCompile per call in the MCP tool, and the untested gateCreateUser fence with the wrong message). The empty-login retry raised last round is resolved: a retained receipt is still prepared, so the retry re-stamps idempotently and calls the portal with the full manifest.
Note: go build / go vet / go test could not be run in this job (commands not permitted), so this is a static review of the pinned diff 0ab7605...33f66bb.
Antigravity (agy) reviewagy returned a result this workflow could not use; see the workflow log. Treated as a failed review. |
|
Claude finished @mashkovd's task in 7m 55s —— View job Code review complete — changes requested
1 P2, 3 P3 — posted as inline comments on Resolved in
|
| } | ||
| // Older surface links were keyed by the verified GitHub login rather | ||
| // than the immutable principal. Remove both forms before name reuse. | ||
| for _, table := range []string{"surface_identity_links", "surface_identity_challenges"} { |
There was a problem hiding this comment.
P2 (sharpened by ecb15d0) — these three deletes are unconditional while the identity reader added in the same commit guards the same table with to_regclass, so in two supported configurations complete fails permanently at its last local step, after every irreversible external effect.
customerDeletionIdentityTx (line 371) now probes to_regclass('public.surface_identity_links') and falls back to a surface-free query, and BeginCustomerDeletion (line 282) does the same for agent_run_tokens. FinishCustomerDeletion does not: DELETE FROM surface_identity_links, DELETE FROM surface_identity_challenges (line 315) and DELETE FROM oauth_refresh_tokens (line 320) assume the tables exist in the principals database. When one does not, Postgres answers 42P01 undefined_table, the whole transaction rolls back, and nothing in the stage can make progress.
Two configurations reach it, both supported by cmd/api/main.go:
- The surface identity kill switch.
surfaceid.NewStore— the only creator ofsurface_identity_links/surface_identity_challenges— is skipped onSURFACE_IDENTITY_DISABLED, on an invalidSURFACE_LINK_TTL, and on store-init failure (main.go:654-666), whileprincipalStoreis built independently from the same URL resolution (main.go:312-328).customerDeletionOptionsrequiresstore,cache,oauthandargo, never the surface store, so full-account deletion stays configurable with those tables absent. - A split OAuth database.
oauth_refresh_tokensis created byrefreshstore.NewPostgresStore(ctx, oauthDBURL)whereoauthDBURL = OAUTH_DB_URL else AUDIT_DB_URL(main.go:231-233), whereas the principals pool isSURFACE_IDENTITY_DB_URL else AUDIT_DB_URL. PointingSURFACE_IDENTITY_DB_URLat one database and leaving refresh tokens in another is exactly what the two separate knobs are for, and then line 320 runs against a database that has no such table. (In that configuration the delete is also redundant:o.RevokeSessionsalready revoked the real rows in the other database.)
The damage is worse than an ordinary retryable stage failure because of where it sits. phase = "principal" runs after DeleteCustomer removed the ZITADEL user, after RevokeSiteSessions, and after CleanPortal purged the portal record. The handler answers 503 customer_deletion_incomplete; retry, every retry fails identically, and the manifest is stranded at deleting: the principal and its identities survive locally, CustomerTenantDeletionPending keeps the tenant name frozen for every caller, and CustomerDeletionPending keeps the GitHub signup fence on — so the customer can neither be deleted nor re-register. Recovery is manual SQL.
The guard already in the file is the fix:
for _, table := range []string{"surface_identity_links", "surface_identity_challenges", "oauth_refresh_tokens"} {
var present bool
if err := tx.QueryRow(ctx, `SELECT to_regclass('public.'||$1) IS NOT NULL`, table).Scan(&present); err != nil {
return err
}
if !present {
continue
}
...
}No test can see this today: TestCustomerDeletionRemovesOldIdentityAndPermitsFreshSubject builds all three stores from TEST_DATABASE_URL, so every table always exists.
Antigravity (agy) reviewagy returned a result this workflow could not use; see the workflow log. Treated as a failed review. |
| query := `SELECT provider,issuer,subject,display,revoked_at IS NOT NULL,false FROM external_identities WHERE principal_id=$1` | ||
| if hasSurface { | ||
| query = `SELECT e.provider,e.issuer,e.subject,e.display,e.revoked_at IS NOT NULL, | ||
| EXISTS(SELECT 1 FROM surface_identity_links l WHERE l.surface=e.provider AND l.external_id=e.subject AND |
There was a problem hiding this comment.
P3 (new in ecb15d0) — the mirror proof only accepts the current GitHub alias, so a legacy-form link made under a previous login leaves the same customer undeletable that this commit set out to rescue.
The alias branch is lower(l.principal)='github:'||lower(g.display), evaluated against the live GitHub row's display now. surface_identity_links.principal is frozen at challenge time, and display is deliberately mutable — the schema comment in store.go says it "is kept unique among live GitHub rows by moving it to whichever id GitHub last reported for it". So:
- The customer links a surface while
humanPrincipalreturns thegithub:<login>form (handlers_surface_identity.go:388-390) — the form this commit's own test exercises (redeem("github:delete-customer", ...)) and the oneFinishCustomerDeletioncalls "older surface links … keyed by the verified GitHub login". - GitHub login is renamed; the next sign-in moves
displayto the new spelling. - An administrator prepares the deletion. The mirror row is this principal's (
e.principal_id=$1), but neither proof branch matches:l.principalisgithub:oldlogin, not the prn, and notgithub:+ the current display. countbecomes 3, socustomerDeletionIdentityTxrefuses withErrInvalid: unrelated, revoked or unproven customer authentication identity, and the handler answers409 "exclusive customer identity and ownership could not be verified"— pointing the administrator at GitOps memberships and IdP identity rather than at a stale surface link. There is no administrator-facing way out, because nothing can repair the frozenl.principal.
Same failure class as the P2 this commit fixes, just narrower: it needs a rename after a legacy-form link. Worth closing while the reader is fresh, because the condition is invisible from the error.
Two framings that keep the proof meaningful:
- Treat a non-reserved, empty-issuer row as owned data when no other principal's link claims that
(surface, external_id)— the row is alreadye.principal_id=$1, so the proof is only guarding against ownership drift, and "nobody else claims it" expresses that directly without depending on a mutable display. - Or keep the current predicate and refuse with a distinct error (
surface link ownership could not be proven) that the handler surfaces separately, so the administrator at least learns where to look.
Worth noting too that FinishCustomerDeletion's link cleanup has the mirror-image of this gap: it deletes by principal=$1 OR lower(principal)='github:'||lower($login) using the frozen manifest login, so a link recorded under a different alias would survive a completed deletion and keep its (surface, external_id) slot occupied against a relink. Unreachable today only because prepare refuses first.
| ALTER TABLE customer_deletions ADD COLUMN IF NOT EXISTS cancelled_at TIMESTAMPTZ; | ||
| CREATE UNIQUE INDEX IF NOT EXISTS customer_deletion_active_github | ||
| ON customer_deletions(github_id) WHERE status <> 'complete' AND cancelled_at IS NULL; | ||
| DROP INDEX IF EXISTS customer_deletion_pending_github; |
There was a problem hiding this comment.
P3 (new in ecb15d0) — the index replacement is right, but two details around it.
The DROP INDEX runs on every startup, so a rollback resurrects the old predicate. CREATE UNIQUE INDEX IF NOT EXISTS customer_deletion_active_github ... AND cancelled_at IS NULL plus DROP INDEX IF EXISTS customer_deletion_pending_github is the correct migration, and wrapping both scripts in one transaction under pg_advisory_xact_lock(hashtext('principals-schema')) (store.go:126-137) correctly stops concurrent replicas racing the catalog — initCtx carries storeInitBudget, so the lock wait is bounded. But any pod still running or restarting on a pre-ecb15d0 image re-creates customer_deletion_pending_github, and from then on both indexes exist and the old one re-blocks a cancelled receipt exactly as before. During a rolling deploy that window is short and fails closed; after a deployment rollback it is permanent and silent. One line in TestCancelledReceiptDoesNotOccupyActiveGitHubIndex asserting customer_deletion_pending_github is absent after the upgrade would at least pin the intent, and the ordering dependency is worth a sentence in docs/principals.md next to the migration note.
The new 23505 classification never reaches the operator. customerDeletionStorageError maps a unique violation to ErrInvalid: another customer deletion is pending, which is the right diagnosis and exactly the gap raised on d0bb009 — but the handler discards it. prepareCustomerAccountDeletionLocked writes the fixed "exclusive customer identity and ownership could not be verified" (line 250), and the deferred denied audit entry carries the fixed "Customer deletion preparation did not submit a workflow", so neither the response nor audit_events records that the cause was a concurrent deletion for the same GitHub account on another tenant. Compare the precise message d0bb009 added one branch earlier for the same-tenant case ("prepared cancellation is awaiting portal acknowledgment; retry cancellation"). Threading err.Error() into the audit Message on the 409/503 paths — which AdvanceCustomerDeletion already does — would make the new classification visible at no cost.
The ErrUnresolved half is a genuine improvement and is pinned on both sides: TestCustomerDeletionPrepareStorageFailureIsUnknown drives a cancelled context through the real store, and TestCustomerDeletionUnknownStorageIsNotOwnershipConflict asserts 503 with no portal freeze versus 409 for an observed conflict. Good pair.
| "strings" | ||
|
|
||
| "github.com/go-chi/chi/v5" | ||
| "github.com/jackc/pgx/v5" |
There was a problem hiding this comment.
P3 — carried findings from earlier rounds, still open on this commit.
- The no-op cancellation is audited as a cancellation that happened. The
pgx.ErrNoRowsbranch returns nil (line 462), so the tail setsstatus, message = "succeeded", "prepared deletion cancelled"and the body says{"status": "cancelled"}, whiledocs/principals.mdstates the opposite — "it confirms the desired final state, not evidence that such a deletion previously existed". An operator cannot distinguish a real cancellation from a mistyped pair that matched nothing. Theelseat line 463 is also dead structure now: every path in the no-rows branch returns, so an early return would make "an absent pair performs no portal mutation" visible in the control flow. pgxin the HTTP layer (line 26) —errors.Is(..., pgx.ErrNoRows)at lines 196, 281 and 451 are the only uses, and they makeCustomerDeletionStore(an interface that exists so the handler is testable without a database) carry an implicit pgx contract that only the comments convey. Aprincipals.ErrNotFound-style sentinel translated at the store boundary would keepinternal/apifree of the driver, per the interface-based design in CLAUDE.md. This commit took a step the other way by addingprincipals.ErrUnresolvedto the same branch, which is the right classification but a second driver-adjacent coupling.- Terminal conditions reported as retryable.
resourceson a completed deletion is a terminal 409 before the lock (line 294) but a 503 "retry" from the in-lock re-read (line 394); the cancelled manifest is the same (409 at line 283, 503 from line 392); andBeginCustomerDeletion's identity-drift refusal — now also the unprovable-mirror refusal fromcustomerDeletionIdentityTx— reaches the workflow as 503 although neither can ever succeed. One sentinel the tail maps to 409, or theconflictflagCancelCustomerDeletionalready uses, covers all four. The in-lock branches are the ones the re-read exists for and none has a test. - A lock failure after the inner refusal writes a second
deniedaudit entry. InprepareCustomerAccountDeletion(line 151), whenranis true andallowedis false the inner function has already emitted its deferreddeniedentry, so this adds a second row for the same attempt with a differentMessage; gating on!rankeeps one entry per attempt, which is the distinction the response handling one line below already draws. Line 140 also restates the authorization predicate from lines 177-186 to decide whether to take the lock. - Detached context in the mint path —
checkMintPrincipal(internal/auth/oauth_server.go:1458) builds its 3s deadline fromcontext.Background()whileIssueCodehas a realctxto thread; CLAUDE.md asks for context propagation on all I/O. Related: the check runs beforeValidPrincipalID, so a malformed id costs a store lookup and returns the generic refusal instead ofmalformed principal id %q, andExchangeCodemakes two checker round trips after the authorization code is consumed, so a transient store failure burns the code. - The
resourcesstage deletes nothing yet is audited ascustomer.delete.resources succeededand holds one of only two per-replicadeletionSlotsplus a dedicated Postgres connection and the tenant-wide advisory lock, now shared with prepare and cancel. cmd/api/customer_deletion.go:111—customerDeletionAppRemainsdropsProject, whichinternal/api/service_listing.go:163treats as an ownership signal and which, unlike a prefix, is an exact match that cannot capture a sibling workspace.internal/principals/customer_deletion.go:77—WithCustomerDeletionLockstill takes apidit no longer reads; the comment explains why, butunparam/reviveare not enabled, so nothing catches a later change that starts relying on it.customerDeletionTx(line 363) is likewise a method whose receiver is unused.internal/mcp/server.go:1157—regexp.MustCompileper call, where every other pattern in the package is a package-level var andinternal/apialready holds this exact expression; the new entries inannotations_test.goanddocs/portal-allowlist.jsonare out of alphabetical order.internal/zitadelhook/hook.go:824— thegateCreateUserhalf of the signup fence is still untested and still returnsno(...), whose fixed message ("Customer accounts are created only through GitHub sign-in.") is wrong for a pending deletion, whereIntentcorrectly usesmsgNotAllowed.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 1 P2, 3 P3.
Resolved in ecb15d0. Both P2s from the review of 33f66bb are properly fixed, and both at the level the findings pointed at rather than at the symptom.
The surface-mirror exclusion is the right model: customerDeletionIdentityTx is now one reader shared by prepare and the pre-begin recheck, so the two can no longer disagree, and it skips only rows that are empty-issuer, non-reserved-provider and corroborated by surface_identity_links for the exact principal or its verified GitHub alias. That is safe by construction — reservedProvider covers github/dex/service/dev and mirrorLink refuses a surface named like any of them, while a real customer ZITADEL identity carries a non-empty issuer, so no authentication identity can be hidden behind a mirror. Accepting revoked mirrors is correct too, since MirrorRevoke only stamps revoked_at and surfaceid never deletes link rows, so the proof stays durable. The test rework is the part that matters most: it drives real CreateChallenge/Redeem/Revoke instead of raw inserts, covers a redeem landing between prepare and begin, asserts an unrelated owner's two identities survive, and proves the erased surface id can be relinked after a fresh signup — which is exactly the state the previous fixture could not produce.
The index fix is complete rather than partial: customer_deletion_active_github carries AND cancelled_at IS NULL, the old index is dropped, and NewStore now applies both schema scripts in one transaction under pg_advisory_xact_lock(hashtext('principals-schema')) so concurrent replicas cannot race the replacement. TestCancelledReceiptDoesNotOccupyActiveGitHubIndex recreates the pre-migration index and exercises the real startup path, then checks the new intent's fences are intact and only the acknowledged name is released. The ErrUnresolved/23505 classification added alongside it closes the unobserved-vs-observed gap at prepare, pinned from both ends by a cancelled-context store test and a handler test asserting 503 with no portal freeze versus 409.
Remaining P2. FinishCustomerDeletion deletes from surface_identity_links, surface_identity_challenges and oauth_refresh_tokens unconditionally, while the reader added in this same commit guards the same table with to_regclass and BeginCustomerDeletion already guards agent_run_tokens that way. Those tables are created by other stores: surfaceid.NewStore is skipped on SURFACE_IDENTITY_DISABLED, an invalid SURFACE_LINK_TTL or an init failure, and oauth_refresh_tokens lives in OAUTH_DB_URL else AUDIT_DB_URL, which is a separate knob from the principals pool's SURFACE_IDENTITY_DB_URL else AUDIT_DB_URL. customerDeletionOptions requires neither store, so full-account deletion stays configurable with the tables absent, and then 42P01 rolls back the final stage — at phase = "principal", after the ZITADEL user is deleted, site sessions revoked and the portal purged. Every retry fails identically, leaving a deleting manifest whose principal survives locally while the tenant name and GitHub signup stay frozen, recoverable only by hand. No test can see it, because the fixture builds all three stores from one TEST_DATABASE_URL.
P3s: the mirror proof matches only the current GitHub display, so a legacy github:<login>-form link made before a rename is unprovable and refuses deletion permanently with the generic ownership 409 — the same class this commit fixes, just narrower; the DROP INDEX runs on every startup, so a pre-ecb15d0 pod restarting or a deployment rollback silently resurrects the old predicate, and the new 23505 diagnosis is discarded by the handler's fixed message and fixed audit Message; plus the carried bundle (no-op cancellation audited as a real cancellation with its dead else, pgx in the HTTP layer, four terminal conditions reported as retryable 503, the duplicate denied entry and restated predicate in the prepare wrapper, the mint-path context, the no-op resources stage holding a deletion slot, customerDeletionAppRemains dropping Project, the unused pid/receiver on the lock helpers, regexp.MustCompile per call in the MCP tool, and the untested gateCreateUser fence with the wrong message).
Note: go build / go vet / go test could not be run in this job (commands not permitted), so this is a static review of the pinned diff f2d4484...7913dd9. The merge commit brought no changes outside the feature.
|
Claude finished @mashkovd's task in 5m 9s —— View job Code review complete — approved
No P1/P2, 3 P3 — posted as inline comments on Resolved in
|
| continue | ||
| } | ||
| args := []any{pid} | ||
| if cleanup.table != "oauth_refresh_tokens" { |
There was a problem hiding this comment.
P3 (new in 40dd6fe) — the guard is the right fix; two notes on how the loop expresses it.
The absence probe matches the shape BeginCustomerDeletion already uses for agent_run_tokens and customerDeletionIdentityTx uses for surface_identity_links, and it keeps the distinction that matters: only a documented absence (to_regclass → NULL) skips a table, while a QueryRow failure or any Exec error still fails the transaction. That is exactly what the previous finding asked for, and the seven-combination test proves completion, replay and a fresh signup in every case.
Two small things:
-
The parameter arity is keyed off the table name.
if cleanup.table != "oauth_refresh_tokens"decides whether the alias argument is appended, so the struct no longer describes its own call. A fourth single-argument table added to the list would silently get two arguments and fail at runtime withbind message supplies 2 parameters, but prepared statement requires 1— and only in the configuration where that table is present. Carrying the arguments in the literal removes the coupling:alias := "github:" + strings.ToLower(login) for _, cleanup := range []struct { table, query string args []any }{ {"surface_identity_links", "DELETE FROM surface_identity_links WHERE principal=$1 OR lower(principal)=$2", []any{pid, alias}}, {"surface_identity_challenges", "DELETE FROM surface_identity_challenges WHERE principal=$1 OR lower(principal)=$2", []any{pid, alias}}, {"oauth_refresh_tokens", "DELETE FROM oauth_refresh_tokens WHERE principal_id=$1", []any{pid}}, } {
-
The probe qualifies
public.while theDELETEresolves throughsearch_path. If the pool ever runs with a non-publicsearch_paththe two disagree: the probe answers absent and the cleanup is skipped silently, where theDELETEwould have succeeded. The existingagent_run_tokensguard makes the same assumption, so this is consistency rather than a regression — worth knowing it is an assumption and not a check.
On coverage: the mixed masks do implicitly pin the arity, because a wrong argument count against a present table errors and fails the subtest. What they do not assert is that the present tables actually lost the principal's rows — TestCustomerDeletionRemovesOldIdentityAndPermitsFreshSubject covers that only for the all-present case. Inserting one refresh token and one surface link before FinishCustomerDeletion and asserting both are gone would make each combination prove cleanup as well as survival.
| for _, cleanup := range []struct{ table, query string }{ | ||
| {"surface_identity_links", "DELETE FROM surface_identity_links WHERE principal=$1 OR lower(principal)=$2"}, | ||
| {"surface_identity_challenges", "DELETE FROM surface_identity_challenges WHERE principal=$1 OR lower(principal)=$2"}, | ||
| {"oauth_refresh_tokens", "DELETE FROM oauth_refresh_tokens WHERE principal_id=$1"}, |
There was a problem hiding this comment.
P3 (new in 40dd6fe) — skipping this table is the right call, but it changes what full deletion means depending on one environment variable, and nothing surfaces the difference.
With OAUTH_DB_URL unset the refresh store shares the principals database, this DELETE runs, and the customer's refresh rows are gone when the manifest flips to complete. With OAUTH_DB_URL pointing elsewhere the table is absent here, the probe skips it, and the rows survive in the other database — revoked, not removed. The comment's claim is accurate as far as it goes: RevokePrincipalSessions → PostgresStore.RevokePrincipal is UPDATE oauth_refresh_tokens SET revoked_at=$1, revoked_reason=$2 ... WHERE revoked_at IS NULL (internal/auth/refreshstore/postgres.go:410-418), so the credentials are dead either way. But principal_id and the token hash remain, for a principal row that no longer exists, until DELETE FROM oauth_refresh_tokens WHERE expires_at < $1 (line 436) sweeps them at expiry.
So the residue is bounded by the refresh TTL rather than permanent, which is why this is a nit and not a blocker. It is still worth stating explicitly, because the PR's contract is precise everywhere else about what survives a completed deletion ("API/Argo/Scaffolder execution audit history is deliberately retained") and this is an undocumented fourth category whose presence depends on deployment topology. One line in docs/principals.md next to the retained-history paragraph — that a split OAuth database leaves revoked refresh rows to expire rather than purging them at completion — would close it.
Worth noting too that principalStore and surfaceid.NewStore both resolve SURFACE_IDENTITY_DB_URL else AUDIT_DB_URL (cmd/api/main.go:312 and :649), so the surface tables can only ever be absent from this pool when no surface store has run against it — in which case there are no mirror rows for customerDeletionIdentityTx to prove either, and the two halves of the feature stay consistent. oauth_refresh_tokens is the only one of the three that can genuinely live in a different database, which is exactly why it is the one with a retention consequence.
| "strings" | ||
|
|
||
| "github.com/go-chi/chi/v5" | ||
| "github.com/jackc/pgx/v5" |
There was a problem hiding this comment.
P3 — carried findings from earlier rounds, consolidated; none of these block the merge.
In internal/principals/
customer_deletion.go:390— the mirror proof matches only the current GitHub display (lower(l.principal)='github:'||lower(g.display)), whilesurface_identity_links.principalis frozen at challenge time anddisplayis deliberately mutable. A legacygithub:<login>-form link made before a rename is therefore unprovable, so the row counts as an extra identity and the customer can never be prepared — surfaced as the generic409 "exclusive customer identity and ownership could not be verified".FinishCustomerDeletion's link cleanup has the mirror image of the same gap (it deletes by the frozen manifest login), unreachable today only because prepare refuses first.customer_deletion.go:56—DROP INDEX IF EXISTS customer_deletion_pending_githubruns on every startup, so a pod still on a pre-ecb15d0image re-creates the old predicate and both indexes then exist; after a deployment rollback that is permanent and silent. One assertion inTestCancelledReceiptDoesNotOccupyActiveGitHubIndexthat the old name is absent after the upgrade would pin the intent.customer_deletion.go:443—customerDeletionStorageErrormaps23505toanother customer deletion is pending, which is the right diagnosis, but the handler discards it:prepareCustomerAccountDeletionLockedwrites the fixed"exclusive customer identity and ownership could not be verified"and the deferreddeniedentry carries a fixedMessage, so neither the response noraudit_eventsrecords the real cause.AdvanceCustomerDeletionalready threadserr.Error()into the auditMessage; doing the same here makes the new classification visible.customer_deletion.go:77—WithCustomerDeletionLockstill takes apidit no longer reads (documented, butunparam/reviveare not enabled so nothing guards it), andcustomerDeletionTxis a method whose receiver is unused.
In internal/api/handlers_customer_deletion.go
- The no-op cancellation returns
{"status": "cancelled"}and audits"prepared deletion cancelled"(line 492), contradicting the documented stance that an absent pair "confirms the desired final state, not evidence that such a deletion previously existed"; theelseat line 463 is also dead structure now that every path in the no-rows branch returns. pgxis imported into the HTTP layer for threeerrors.Is(..., pgx.ErrNoRows)comparisons, soCustomerDeletionStorecarries an implicit driver contract that only the comments convey.- Four terminal conditions reach the workflow as a retryable
503:resourceson a completed deletion and a cancelled manifest are both409before the lock but503from the in-lock re-read, and the identity-drift and unprovable-mirror refusals fromBeginCustomerDeletioncan never succeed. One sentinel the tail maps to409, or theconflictflagCancelCustomerDeletionalready uses, covers all four; none of the in-lock branches has a test. - A lock failure after an inner refusal writes a second
deniedaudit row for the same attempt (line 151); gating on!rankeeps one entry per attempt. Line 140 also restates the authorization predicate it gates on. - The
resourcesstage deletes nothing yet is audited assucceededand holds one of only two per-replicadeletionSlotsplus a dedicated connection and the tenant-wide advisory lock, now shared with prepare and cancel.
Elsewhere
internal/auth/oauth_server.go:1458—checkMintPrincipalbuilds its 3s deadline fromcontext.Background()whileIssueCodehas a realctx; the check also runs beforeValidPrincipalID, andExchangeCodemakes two checker round trips after the authorization code is consumed.cmd/api/customer_deletion.go:111—customerDeletionAppRemainsdropsProject, whichinternal/api/service_listing.go:163treats as an ownership signal and which, unlike a prefix, cannot capture a sibling workspace.internal/mcp/server.go:1157—regexp.MustCompileper call where every other pattern in the package is a package-level var; the new entries inannotations_test.goanddocs/portal-allowlist.jsonare out of alphabetical order.internal/zitadelhook/hook.go:824— thegateCreateUserhalf of the signup fence is still untested and still returnsno(...), whose fixed message ("Customer accounts are created only through GitHub sign-in.") is wrong for a pending deletion, whereIntentcorrectly usesmsgNotAllowed.internal/principals/customer_deletion_test.go:477— the refresh store built per subtest is never closed, so the seven subtests leak seven pools. Consistent with the existing test at line 38, so pre-existing rather than new.
There was a problem hiding this comment.
No P1/P2 findings (3 P3). Good to merge.
40dd6fe closes the last blocking finding, and it closes it at the level the finding pointed at rather than at the symptom. FinishCustomerDeletion now probes to_regclass for each of the three tables that belong to independently configured stores, skipping only a documented absence while a failed probe or a failed DELETE still fails the transaction — the same unobserved-versus-absent discipline the rest of this design applies, and the same shape BeginCustomerDeletion already used for agent_run_tokens and customerDeletionIdentityTx for surface_identity_links. The stage can no longer strand a deleting manifest at phase = "principal", after the ZITADEL user, site sessions and portal record are already gone, in a deployment that simply disabled the surface store or split OAUTH_DB_URL.
The test is the right one: it renames each subset of the three tables away and drives real PrepareCustomerDeletion → BeginCustomerDeletion → FinishCustomerDeletion across all seven combinations, asserting completion, an idempotent replay, and that a fresh signup gets a new principal. Restoring the names from t.Cleanup keeps the shared serial fixture intact even when an assertion fails, and the LIFO ordering against newStoreForTest's own wipe is correct. The surface and principal stores resolve the same SURFACE_IDENTITY_DB_URL else AUDIT_DB_URL, so the mirror proof and this cleanup can never disagree about whether the surface tables exist.
The three remaining P3s are posted inline: the cleanup loop keys its parameter arity off a table-name string comparison and the probe assumes public. while the DELETE resolves through search_path; a split OAuth database now leaves revoked refresh rows to expire rather than purging them at completion, which is an undocumented fourth retention category; and one consolidated comment carries the nits still open from earlier rounds (mirror proof pinned to the current GitHub display, the DROP INDEX that a rollback resurrects, the discarded 23505 diagnosis, four terminal conditions reported as retryable 503, the no-op cancellation audited as a real one, and the usual small ones).
Note: go build / go vet / go test could not be run in this job (commands not permitted), so this is a static review of the pinned diff f2d4484...40dd6fe.
Antigravity (agy) reviewagy returned a result this workflow could not use; see the workflow log. Treated as a failed review. |
Problem and behavior
Workspace deletion currently leaves the customer ZITADEL identity, renewable credentials and private portal records behind. Add explicit
delete_customer_account=trueplus the exactcustomer_principal_idtomctl_delete_tenant; ordinary workspace deletion is preserved. Full-account deletion requires a directly authenticated human platform administrator and refuses self/protected principals, unrelated identities, other memberships and unreadable observations.Lifecycle
mctl_cancel_customer_deletionwith exact tenant/principal and explicit confirmation for a direct human administrator. It stampscancelled_atonly on a prepared intent, under the tenant run lock, before unfreezing the portal. This restores principal/subject authentication and makes queued begin refuse. The exact identity receipt and name fence remain durable until portal acknowledgment and final local cleanup succeed. Retry uses the same stored identity fields. Deleting/complete manifests cannot be cancelled; no independently removed resources are restored.CUSTOMER_DELETION_ENABLEDgates new preparations. Already approved callbacks and prepared cancellation stay recoverable when it is switched off. The cancellation tool is explicitly disabled on the shared MCP aggregate; direct API authorization remains authoritative.Providers and retained history
Use the dedicated customer-org lifecycle machine and pinned Worker audience; do not mount the instance-wide IaC key or a Cloudflare account token. Portal cleanup revokes PID-bound Backstage sessions, membership/contact/ready-email and per-user notification data while preserving staff sessions with matching display logins. Legacy OIDC admission is staff-only and has a tested exact-key membership invariant. API/Argo/Scaffolder execution audit history is deliberately retained, as documented in the canonical plan and portal contract.
Uncached indexed tombstone probes intentionally enforce immediate cross-replica revocation; a negative cache would weaken this contract. No completed rows are silently backfilled as released: completion previously preceded the external acknowledgment, and no version of this feature has been enabled in production.
Surface identity mirrors are accepted only with matching stored ownership provenance; unrelated or additional authentication identities still refuse deletion. Both preparation and begin use the same reader and propagate unreadable observations as unknown. Schema migration replaces the active GitHub uniqueness index transactionally, excluding cancelled receipts while retaining their name fence.
Validation
go test -p 1, matching CI shared-fixture isolation); current-head CI also runs the full repository suite.Rollout dependencies
Worker #164/#165 and GitOps bootstrap #1951 are deployed. Portal #185/#189/#191 are released as 4.25.1; release-deploy 39116804503 succeeded and the new pod is ready. A real API service-token request to workspace cleanup returned HTTP 200 with cleaned=true and scope=workspace, verifying the installed Backstage static-token subject contract. Full customer-scoped acceptance remains pending.
After the API releases/deploys, merge/sync mctl-gitops#1952 and activate separately via draft mctl-gitops#1961. Public signup remains restricted. Production create/full-delete/retry/recreate acceptance is still pending.