Repository navigation
fix(handlers): attribute partial account-deletion failures and emit retryable codes - #677
Merged
Merged
Conversation
…on both paths A failure after the data sweep leaves a half-deleted account; every log line and error on that path must say who acted, which account, what survives, and whether a retry can help. admin_purge_user_account: - The partial-failure error log now carries actor_sub like every other line in the handler. - The Cognito helper no longer logs the failure itself (its line and the handler's line were double noise, neither with the actor); it propagates the raw ClientError so the single log line where the context is added can extract the real AWS error code (aws_error_code) instead of the constant error_code field that could only ever be INTERNAL_ERROR. - A throttled AdminDeleteUser is classified with the canonical transient classifier and raised as retryable RESOURCE_BUSY (#291), paired with a message that promises the retry; other failures stay INTERNAL_ERROR with a manual-completion message. - The message names what survives on the admin path: the accounts record (deleted after the Cognito step) and possibly the Cognito user, so an operator cannot conclude the record is gone and skip the retry. delete_my_account: - The Cognito delete after the sweep gets the same loud partial-failure handling: one error line with the account id, transient -> RESOURCE_BUSY with a retry message, otherwise INTERNAL_ERROR with manual completion. - A data-sweep failure is logged with the account id (the removed outer except was the only attributed sweep log) and leaves the Cognito user untouched. Tests assert actor attribution, single logging, real AWS codes, message/ code agreement, and per-path surviving resources.
…d sweep attribution on both paths
…nt codes, split purge lookup
…AGENTS.md, spellcheck token
…t comments at it (#669) * docs(scripts): correct run-id provenance list and point script comments at the single contract (#595) * no-mistakes(document): Run-id contract docs verified; shellcheck warning fixed * no-mistakes(ci): Ephemeral tests for PR failed on one integration test: resolvers/campaignQueries.integration.test.ts 'should return zero for totalOrders and totalRevenue when campaign has no orders' threw TypeError on data.getCampaign.totalOrders because getCampaign was read immediately after createCampaign. getCampaign's pipeline resolves the campaign via the eventually-consistent campaignId-index GSI (query_campaign_fn.js), so a straight read can return null for an existing campaign (Bug #21); the test was the only positive read-after-create site in the file with no consistency poll, unlike its siblings. Fix: that test and its four sibling sites in the same file (campaignId/all-fields, shared-user-READ, no-endDate, both-dates) now poll getCampaign through the file's existing waitForGSIConsistency(query, len, 10, 1000) helper before asserting, exactly as the four call sites that already did so. Assertions were rewritten against campaigns[0] (visibleCampaigns in the no-endDate test, which already declares campaigns for its polled create) with identical field/value expectations; cleanups and timeouts unchanged. Negative tests (expect null) and the totalOrders=2 test (guarded by two successful createOrder calls that read the same GSI) were correctly left alone. The PR's own diff (docs/scripts/README.md + shell comments) is unrelated to the failure. Verified the edited file parses/links (module load reaches only ERR_MODULE_NOT_FOUND for uninstalled deps; the method was proven to catch duplicate declarations, which it caught before the shadowing rename); the integration suite cannot run locally (no .env/AWS credentials; it provisions cloud resources), so CI re-run is the definitive check
…ree-wide (#670) * refactor(appsync): route ID-prefix normalization through lib/ids.js tree-wide (#534) Every hand-rolled value.startsWith('PREFIX#') ternary and unconditional 'PREFIX#' + id concatenation in the js-resolvers now routes through the shared lib/ids.js helpers (normalizeId / normalizeIdOrPrefix, plus a new stripIdPrefix inverse for the API-boundary prefix strips). The three verify_profile_owner_for_* resolvers - which had drifted into two different owner-key implementations, one of them double-prefixing an already-prefixed sub - now share lib/owner_key.js (expectedOwnerKey + ownerGetItemRequest) and differ only in their FORBIDDEN message, per the Behavior-preserving for every schema-valid input. Two deliberate tolerances are kept rather than normalized away: update_campaign_fn still passes catalogId null through as null (a deliberate clear-catalog update), and check_existing_share_fn still passes a missing profileId / targetAccountId through untouched. The share owner-verifier no longer double-prefixes a prefixed sub - the divergence the issue flags as the rot hazard - and identity-sub key building is now idempotent everywhere (a Cognito sub is a bare UUID today; that assumption is no longer baked into 20+ resolvers). tests/unit/check_id_prefix_normalization.test.ts now sweeps every non-test resolver for all four table prefixes (ACCOUNT/PROFILE/CATALOG/ CAMPAIGN), with a documented autoId() minting exemption; a re-inlined ternary fails the guard. lib/verify_profile_owner_cases.js asserts the consolidated owner-key behavior once for all three families, and id_prefix_contract.test.js pins emitted DynamoDB requests for prefixed, unprefixed, foreign-prefixed, and null inputs across the converted patterns. * no-mistakes(review): Add missing normalizeIdOrPrefix import to listMySharedCampaigns resolver * test(appsync): pin verify_profile_read_access GetItem key bytes after the #508/#534 merge The rebase onto main merged #508's consistent-GetItem rewrite of verify_profile_read_access_fn.js with #534's routing of its two ID-prefix constructions through lib/ids.js. The composite GetItem key IS the authorization signal, so pin it: a bare Cognito sub must still produce exactly ACCOUNT#<sub> and PROFILE#<id>, byte-identical to the pre-#534 hand-rolled ternaries. * test(appsync): align update_campaign catalogId null contract with #659 in the #534 sweep The #534 id_prefix_contract test pinned the pre-#659 tolerance that an explicit null catalogId passes through as a NULL attribute. The base this branch rebases onto (#659) made Campaign.catalogId ID! non-nullable and rejects an explicit null with INVALID_INPUT, so the rebased contract test now pins the rejection instead. Also correct the #534 comment in normalizeCatalogId that claimed null must survive as null.
…t profileIds input (#672) * fix(frontend): regenerate GraphQL types to match adminPurgeUserAccount profileIds list input The committed generated types were stale from the client-side account-purge work: the schema accepts a single ID or a list for profileIds, but GqlAdminPurgeUserAccountMutationVariables still declared only the array form. Regenerated with the lockfile-pinned toolchain (@graphql-codegen/cli 6.3.1, @graphql-codegen/typescript 5.0.10) via npm run codegen. * ci(frontend): fail when the generated GraphQL types drift from the schema frontend/src/types/graphql-generated.ts is committed generated output that no build or test step regenerates, so it can go stale against tofu/application/schema/schema.graphql without anything noticing: the adminPurgeUserAccount profileIds argument landed in #641 and the generated file stayed behind until it was regenerated later. Add a frontend CI step that runs the repo's lockfile-pinned codegen (npm run codegen) and fails on any diff in the generated file, with an error annotation naming the fix command. Verified locally: the step passes on the current tree, and fails (exit 1) when the generated file is hand-edited and committed - the regenerated union line shows up in the diff. * no-mistakes(document): Documented the codegen sync CI gate in owner docs
…ls at Cognito (#673) * fix(handlers): make Cognito the commit point in user deletion (#551) Delete the Cognito user before running the shared data cascade in both admin_delete_user and delete_my_account. A Cognito failure now leaves every record intact, and a data-phase failure afterwards leaves only inert records the user can no longer reach (no sign-in, so no post_authentication re-bootstrap); a re-run of the data phase is safe in both directions. A data-phase failure after the Cognito commit is logged loudly with an error_code-tagged field so a partial delete is detectable in post-incident review rather than inferred from a client error. * no-mistakes(review): Fix delete_my_account data-phase error attribution and update AGENTS.md #521 purge order * no-mistakes(review): Revert user deletion to data-sweep-first order with loud partial-failure handling * no-mistakes(document): Verified deletion docs current; reformatted one log string
Both KW-673 branches were cut from b4134fd and reworked the same deletion paths, so they collided with the merged #673 (6ae5f4c). Resolved in favour of this branch's implementation, which supersedes #673's classification: - account_operations / admin_operations: the sweep -> Cognito delete sequence now goes through the shared `run_deletion_steps`, so both paths classify a failure once and identically. #673's `except Exception` around the Cognito delete is replaced by that helper. - #521 sweep-first ordering is preserved and verified: `run_deletion_steps` runs the sweep before the Cognito delete, and the admin purge still deletes the accounts record last, after `run_deletion_steps`. - utils/cognito.py single-homes the Cognito transient set as the public `COGNITO_TRANSIENT_ERROR_CODES` and adds `is_transient_cognito_error`, so the retry wrapper and the exhausted-retry classification cannot diverge. Three #673-era tests asserted the messages this branch replaces. Updated to the new contract (a permanent fault no longer promises a retry; the sweep failure names what survived). One of them, the partial-state post-state assertion, was orphaned onto the wrong test by the merge; moved back to `test_delete_account_cognito_admin_delete_error`, which owns the seed. Gates: `pytest tests/unit` 1563 passed / 2 skipped at 100% coverage, xenon (max-average A, max-absolute B), ruff, and mypy all clean.
This was referenced Oct 4, 2026
dmeiser
added a commit
that referenced
this pull request
Oct 4, 2026
…he deletion lookups (#678) * fix(backend): harden get_caller_id and classify transport faults on the deletion lookups Carries forward the two non-duplicate pieces of the closed #671 and #676, neither of which made it into the merged work. get_caller_id (from #671, whose rest is a duplicate of merged #662): `event.get("identity") or {}` then `.get("sub")` assumed identity is a mapping. A present-but-non-mapping identity (a string, a number, a list) is truthy, so the read raised AttributeError instead of returning None, and the caller surfaced as the decorator's generic INTERNAL_ERROR rather than the typed UNAUTHORIZED the handlers raise for an absent caller. Guard with isinstance against Mapping. Cognito pre-check lookups (from #676, whose rest is merged #673 and #677): both `_lookup_cognito_user_for_deletion` and `_find_cognito_user_by_sub` caught ClientError only, so a BotoCoreError during the lookup escaped to the decorator with no attributed log line and no retry guidance -- even though the lookup runs before the sweep and the Cognito delete, so nothing has been mutated and a retry converges. Widen both to `(ClientError, BotoCoreError)`. To classify that without adding a second definition of "transient", `is_transient_cognito_error` now takes a BaseException and counts a bare BotoCoreError as retryable; the pre-delete catches just call it. The post-sweep Cognito delete in `run_deletion_steps` keeps its narrower `isinstance(error, ClientError)` guard, and the asymmetry is now documented on both sides: a transport fault *after* the delete request was sent leaves the Cognito state genuinely unknown, so it must not promise a retry, while the same fault *before* anything is mutated is plainly retryable. This keeps #677's tested unknown-state contract intact. Gates: `pytest tests/unit` 1575 passed / 2 skipped at 100% coverage, xenon, ruff, and mypy all clean. * chore(spelling): add 'isinstance' to the project cspell dictionary The AGENTS.md #291 entry now names the isinstance narrowing that distinguishes the post-sweep Cognito delete from the pre-delete lookups; the word was not in the dictionary and failed the frontend job's Spell check step.
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Make the account-deletion partial-state failure report honestly and attribute it to the actor. On the admin path the new logger.error omits actor_sub, which every other line in admin_purge_user_account carries - so the single most audit-worthy event in an admin-triggered destructive path is the one with no actor attribution. The Cognito helper already logs the failure with the real AWS code, so the handler logs it twice and neither line identifies the operator. The removed outer except ClientError was the only place a sweep failure was logged with account_id, so sweep failures now reach the decorator whose log line does not say which account was affected. The message instructs the caller to retry but emits INTERNAL_ERROR, the catch-all the frontend maps to a generic failure, when this project mandates RESOURCE_BUSY for a transient or throttled Cognito failure. The error_code field is a constant that can never carry anything else - it is either the fallback or the helper own INTERNAL_ERROR - and it collides with four other sites that use error_code for the AWS code. And the message says the account data was deleted, which is true for self-service but false for the admin path where the accounts row is deleted after the Cognito step and survives the raise, so an operator could conclude the record is gone and skip the retry, orphaning it permanently. Log once with full context, attribute every line to the actor and the account, emit the code that matches the retry guidance, and name the resources that actually survive per path.
What Changed
run_deletion_stepshelper insrc/handlers/deletion_cascade.pythat bothdelete_my_accountandadmin_purge_user_accountuse to run the data sweep then the Cognito delete, logging each failure exactly once with per-path context (account_idon the self-service path,account_idplusactor_subon the admin path) and naming the resources that actually survive each failure phase.RESOURCE_BUSYwith retry-guidance messages instead ofINTERNAL_ERROR, using a single-homeCOGNITO_TRANSIENT_ERROR_CODESset andis_transient_cognito_errorclassifier insrc/utils/cognito.py; sweep failures are likewise classified viais_transient_client_error, and the admin Cognito delete propagates its real AWS error code to the one attributed log line._sweep_purge_residue, with new unit tests intest_account_operations.pyandtest_admin_operations.pycovering attribution, single-log, transient classification, and per-path surviving-resource messages; AGENTS.md's transient-classification entry was corrected to reflect the separate Cognito classifier.Risk Assessment
Testing
I stood up the handlers the way AppSync invokes them (real decorated entry points, AppSync-shaped events) against a disposable local AWS instance — moto's HTTP server fronted by per-service fault-injecting proxies so botocore parses genuine AWS error responses — pointed there exclusively through the product's own endpoint overrides, and drove 10 scenarios covering every failure path the intent enumerates (attribution, single log line, retryable code vs. guidance, per-path survival naming) with post-state read back from the local instance and retry counts observed at the proxy; a base-commit copy of src/ reproduced the reported defect under the same driver, proving sensitivity. All target scenarios passed, the regression reproduction matched the reported failure, the change's own unit tests (287) pass, and the worktree was left clean.
Evidence: Live driver transcript — target commit (10 scenarios, payloads, log lines, state, checks)
~/.no-mistakes/evidence/01M419PA797T446TEJZTAN0NDS/target_results.json)Evidence: Live driver transcript — base-commit regression reproduction
Evidence: Live driver raw results — base commit
~/.no-mistakes/evidence/01M419PA797T446TEJZTAN0NDS/live_driver.py)Pipeline
Updates from git push no-mistakes
... (5 earlier update rounds omitted to keep the PR body within GitHub's 65536-char limit; full history is in the run log.)
🔧 Fix applied.
6 issues (3 warnings, 3 infos) still open:
src/handlers/admin_operations.py:896- Transient classification of Cognito failures is misaligned with the repo's own retry policy, so a genuinely transient Cognito code still surfaces as INTERNAL_ERROR with 'complete the deletion manually' instead of the mandated retryable RESOURCE_BUSY. Concrete sequence (admin path): Cognito backend unstable →admin_delete_userraises ClientError codeInternalErrorException→is_transient_client_error(e)checksTRANSIENT_ERROR_CODES(src/utils/dynamodb.py:54-56 = {ProvisionedThroughputExceededException, ThrottlingException, TooManyRequestsException}) which omitsInternalErrorException→ falls into the INTERNAL_ERROR branch with 'the accounts record still exists and the Cognito user may also survive — complete the deletion manually', telling an operator to do manual console work for a failure the project itself defines as retryable (_RETRYABLE_CODESin src/utils/cognito.py:12-14 includes InternalErrorException). Self path (src/handlers/account_operations.py:158) is worse: the internalretry_on_transient_errors(account_operations.py:74) retries InternalErrorException twice, then the catch classifies it as permanent — same code, two different transient-definitions on one call chain. This is the same code-vs-retry-guidance contradiction the change was written to eliminate, surviving for one member of the Cognito transient set. Same invariant is violated at: account_operations.py:158 (same check, same gap for InternalErrorException); account_operations.py:133-140 (sweep catch maps ANY ClientError to INTERNAL_ERROR with no transient split — a throttledquery_all_items/accounts-rowdelete_itemfromdelete_all_user_data(raw ClientError re-raised at src/handlers/deletion_cascade.py:268-272) gets 'Failed to delete account data' / INTERNAL_ERROR, while the identical throttle raised bybatch_delete_keyson the same sweep becomes retryable RESOURCE_BUSY via_raise_delete_error, src/handlers/campaign_operations.py:90-98 — retry converges for both, so RESOURCE_BUSY matches the guidance in both); account_operations.py:124-126 (lookup catch, same no-split shape). Mechanical remedy contained to this change: classify Cognito delete failures against the Cognito retryable set (union with the canonical set or a sharedis_transient_cognito_error), and apply the same transient→RESOURCE_BUSY split in the sweep/lookup catches instead of blanket INTERNAL_ERROR.src/handlers/admin_operations.py:1098- Residual attribution gap inside the same destructive purge path, unchanged by this diff: the accounts-record deletion failure logs 'Failed to delete account record' with error and account_id but no actor_sub (src/handlers/admin_operations.py:1098-1099), breaking the admin_operations.py never logs the acting admin; _validate_admin_and_get_account_id returns the target, not the actor #507 invariant ('every admin audit line downstream records both') that the newly added lines now honor; the shared cascade helper's failure line 'Failed to delete account from DynamoDB' (src/handlers/deletion_cascade.py:271) likewise lacks actor_sub. Noting for a follow-up pass; not a blocker for this change.src/handlers/account_operations.py:170- The unconditional success narration 'Deleted account from DynamoDB' inside delete_all_user_data (src/handlers/deletion_cascade.py:269, unchanged) is ordered last in the sweep, so a sweep failure before that point combined with the change's honest 'retry converges' framing is consistent — the narration is only emitted on actual success. No defect found on the ordering; noting the two paths' resource-naming claims were verified: self-service deletes the accounts row inside the sweep (deletion_cascade.py:268) so 'Account data was deleted' is true; admin deletes it after the Cognito step (admin_operations.py:1090-1096) so 'the accounts record still exists' is true. No action needed.src/utils/dynamodb_exceptions.py:15- Fix-round change:COGNITO_TRANSIENT_ERROR_CODESduplicates_RETRYABLE_CODES(src/utils/cognito.py:12-14) with only a comment claiming parity — a second hand-maintained definition of the same concept, unenforced. If a code is later added to the retry wrapper but not here (or vice versa), the exhausted-retry classification (is_transient_cognito_errorat account_operations.py:130/185, admin_operations.py:900) diverges from what actually gets retried — the same code-vs-retry-guidance contradiction F1 targeted, reintroduced structurally. Remedy contained to the change: make the set single-homed (export a public name from utils.cognito and buildCOGNITO_TRANSIENT_ERROR_CODESfrom it, or vice versa). This is also the smallest honest remedy under the simplification pass: the parallel copy of the rule is not required by the intent, which only requires one consistent classification.src/handlers/admin_operations.py:828- Sibling left behind by the fix round:_find_cognito_user_by_sub(called by admin_purge_user_account at admin_operations.py:1058) maps ANY ClientError — transient Cognito codes included — to INTERNAL_ERROR 'Failed to look up Cognito user' with no retry wrapper, while the self path's identical lookup now emits retryable RESOURCE_BUSY when the same fault exhausts its retries (src/handlers/account_operations.py:126-138, the fix round's own work). Concrete sequence on the purge path: Cognito list_users throttled (TooManyRequestsException/InternalErrorException) → aborts the purge with the catch-all the frontend maps to a generic failure, contradicting the intent mandate that a transient or throttled Cognito failure carry retry guidance. Note the purge path uses this lookup with NO retry wrapper at all, so the throttle aborts on the first attempt. Remedy contained: classify againstis_transient_cognito_errorhere like the sibling sites (the missing actor_sub in this log line is the F3 gap the user chose to ignore — not part of this finding).src/handlers/admin_operations.py:885- Round-2 fix (e23e66f) copied the self-service path's failure-provenance comment into the admin wrapper's docstring: '_delete_cognito_user_after_sweep' claims a 'failed re-lookup of an absent user lands here too', but the admin path's _delete_user_from_cognito (admin_operations.py:839) takes an already-resolved username from _find_cognito_user_by_sub and performs no re-lookup — that clause describes only the self path (account_operations.py:174, where the fallback lookup inside the account _delete_user_from_cognito can fail). Minor doc inaccuracy on the destructive purge path; drop the clause.🔧 Fix applied.
7 issues (3 warnings, 4 infos) still open:
src/handlers/admin_operations.py:896- Transient classification of Cognito failures is misaligned with the repo's own retry policy, so a genuinely transient Cognito code still surfaces as INTERNAL_ERROR with 'complete the deletion manually' instead of the mandated retryable RESOURCE_BUSY. Concrete sequence (admin path): Cognito backend unstable →admin_delete_userraises ClientError codeInternalErrorException→is_transient_client_error(e)checksTRANSIENT_ERROR_CODES(src/utils/dynamodb.py:54-56 = {ProvisionedThroughputExceededException, ThrottlingException, TooManyRequestsException}) which omitsInternalErrorException→ falls into the INTERNAL_ERROR branch with 'the accounts record still exists and the Cognito user may also survive — complete the deletion manually', telling an operator to do manual console work for a failure the project itself defines as retryable (_RETRYABLE_CODESin src/utils/cognito.py:12-14 includes InternalErrorException). Self path (src/handlers/account_operations.py:158) is worse: the internalretry_on_transient_errors(account_operations.py:74) retries InternalErrorException twice, then the catch classifies it as permanent — same code, two different transient-definitions on one call chain. This is the same code-vs-retry-guidance contradiction the change was written to eliminate, surviving for one member of the Cognito transient set. Same invariant is violated at: account_operations.py:158 (same check, same gap for InternalErrorException); account_operations.py:133-140 (sweep catch maps ANY ClientError to INTERNAL_ERROR with no transient split — a throttledquery_all_items/accounts-rowdelete_itemfromdelete_all_user_data(raw ClientError re-raised at src/handlers/deletion_cascade.py:268-272) gets 'Failed to delete account data' / INTERNAL_ERROR, while the identical throttle raised bybatch_delete_keyson the same sweep becomes retryable RESOURCE_BUSY via_raise_delete_error, src/handlers/campaign_operations.py:90-98 — retry converges for both, so RESOURCE_BUSY matches the guidance in both); account_operations.py:124-126 (lookup catch, same no-split shape). Mechanical remedy contained to this change: classify Cognito delete failures against the Cognito retryable set (union with the canonical set or a sharedis_transient_cognito_error), and apply the same transient→RESOURCE_BUSY split in the sweep/lookup catches instead of blanket INTERNAL_ERROR.src/handlers/admin_operations.py:1098- Residual attribution gap inside the same destructive purge path, unchanged by this diff: the accounts-record deletion failure logs 'Failed to delete account record' with error and account_id but no actor_sub (src/handlers/admin_operations.py:1098-1099), breaking the admin_operations.py never logs the acting admin; _validate_admin_and_get_account_id returns the target, not the actor #507 invariant ('every admin audit line downstream records both') that the newly added lines now honor; the shared cascade helper's failure line 'Failed to delete account from DynamoDB' (src/handlers/deletion_cascade.py:271) likewise lacks actor_sub. Noting for a follow-up pass; not a blocker for this change.src/handlers/account_operations.py:170- The unconditional success narration 'Deleted account from DynamoDB' inside delete_all_user_data (src/handlers/deletion_cascade.py:269, unchanged) is ordered last in the sweep, so a sweep failure before that point combined with the change's honest 'retry converges' framing is consistent — the narration is only emitted on actual success. No defect found on the ordering; noting the two paths' resource-naming claims were verified: self-service deletes the accounts row inside the sweep (deletion_cascade.py:268) so 'Account data was deleted' is true; admin deletes it after the Cognito step (admin_operations.py:1090-1096) so 'the accounts record still exists' is true. No action needed.src/utils/dynamodb_exceptions.py:15- Fix-round change:COGNITO_TRANSIENT_ERROR_CODESduplicates_RETRYABLE_CODES(src/utils/cognito.py:12-14) with only a comment claiming parity — a second hand-maintained definition of the same concept, unenforced. If a code is later added to the retry wrapper but not here (or vice versa), the exhausted-retry classification (is_transient_cognito_errorat account_operations.py:130/185, admin_operations.py:900) diverges from what actually gets retried — the same code-vs-retry-guidance contradiction F1 targeted, reintroduced structurally. Remedy contained to the change: make the set single-homed (export a public name from utils.cognito and buildCOGNITO_TRANSIENT_ERROR_CODESfrom it, or vice versa). This is also the smallest honest remedy under the simplification pass: the parallel copy of the rule is not required by the intent, which only requires one consistent classification.src/handlers/admin_operations.py:828- Sibling left behind by the fix round:_find_cognito_user_by_sub(called by admin_purge_user_account at admin_operations.py:1058) maps ANY ClientError — transient Cognito codes included — to INTERNAL_ERROR 'Failed to look up Cognito user' with no retry wrapper, while the self path's identical lookup now emits retryable RESOURCE_BUSY when the same fault exhausts its retries (src/handlers/account_operations.py:126-138, the fix round's own work). Concrete sequence on the purge path: Cognito list_users throttled (TooManyRequestsException/InternalErrorException) → aborts the purge with the catch-all the frontend maps to a generic failure, contradicting the intent mandate that a transient or throttled Cognito failure carry retry guidance. Note the purge path uses this lookup with NO retry wrapper at all, so the throttle aborts on the first attempt. Remedy contained: classify againstis_transient_cognito_errorhere like the sibling sites (the missing actor_sub in this log line is the F3 gap the user chose to ignore — not part of this finding).src/handlers/admin_operations.py:885- Round-2 fix (e23e66f) copied the self-service path's failure-provenance comment into the admin wrapper's docstring: '_delete_cognito_user_after_sweep' claims a 'failed re-lookup of an absent user lands here too', but the admin path's _delete_user_from_cognito (admin_operations.py:839) takes an already-resolved username from _find_cognito_user_by_sub and performs no re-lookup — that clause describes only the self path (account_operations.py:174, where the fallback lookup inside the account _delete_user_from_cognito can fail). Minor doc inaccuracy on the destructive purge path; drop the clause.src/handlers/admin_operations.py:919- Cosmetic path-dependent wording mismatch within this change: the admin partial-state message reads 'Account data was swept but the Cognito user could not be confirmed deleted' (singular 'Account data') while its log narration (admin_operations.py:894) and the analogous self-path message ('Account data was deleted', account_operations.py:190) use different phrasings for the same distinction. Every intent requirement (honest survival naming, actor attribution, code/retry alignment) is met on both paths and verified against the source: the sweep (deletion_cascade.py:268) deletes the accounts row on the self path before the Cognito delete; the admin path deletes it after (admin_operations.py:1129-1136) so 'the accounts record still exists' is accurate there. Purely a narration-wording nuance; no action needed.🔧 No changes applied.
7 issues (3 warnings, 4 infos) still open:
src/handlers/admin_operations.py:896- Transient classification of Cognito failures is misaligned with the repo's own retry policy, so a genuinely transient Cognito code still surfaces as INTERNAL_ERROR with 'complete the deletion manually' instead of the mandated retryable RESOURCE_BUSY. Concrete sequence (admin path): Cognito backend unstable →admin_delete_userraises ClientError codeInternalErrorException→is_transient_client_error(e)checksTRANSIENT_ERROR_CODES(src/utils/dynamodb.py:54-56 = {ProvisionedThroughputExceededException, ThrottlingException, TooManyRequestsException}) which omitsInternalErrorException→ falls into the INTERNAL_ERROR branch with 'the accounts record still exists and the Cognito user may also survive — complete the deletion manually', telling an operator to do manual console work for a failure the project itself defines as retryable (_RETRYABLE_CODESin src/utils/cognito.py:12-14 includes InternalErrorException). Self path (src/handlers/account_operations.py:158) is worse: the internalretry_on_transient_errors(account_operations.py:74) retries InternalErrorException twice, then the catch classifies it as permanent — same code, two different transient-definitions on one call chain. This is the same code-vs-retry-guidance contradiction the change was written to eliminate, surviving for one member of the Cognito transient set. Same invariant is violated at: account_operations.py:158 (same check, same gap for InternalErrorException); account_operations.py:133-140 (sweep catch maps ANY ClientError to INTERNAL_ERROR with no transient split — a throttledquery_all_items/accounts-rowdelete_itemfromdelete_all_user_data(raw ClientError re-raised at src/handlers/deletion_cascade.py:268-272) gets 'Failed to delete account data' / INTERNAL_ERROR, while the identical throttle raised bybatch_delete_keyson the same sweep becomes retryable RESOURCE_BUSY via_raise_delete_error, src/handlers/campaign_operations.py:90-98 — retry converges for both, so RESOURCE_BUSY matches the guidance in both); account_operations.py:124-126 (lookup catch, same no-split shape). Mechanical remedy contained to this change: classify Cognito delete failures against the Cognito retryable set (union with the canonical set or a sharedis_transient_cognito_error), and apply the same transient→RESOURCE_BUSY split in the sweep/lookup catches instead of blanket INTERNAL_ERROR.src/handlers/admin_operations.py:1098- Residual attribution gap inside the same destructive purge path, unchanged by this diff: the accounts-record deletion failure logs 'Failed to delete account record' with error and account_id but no actor_sub (src/handlers/admin_operations.py:1098-1099), breaking the admin_operations.py never logs the acting admin; _validate_admin_and_get_account_id returns the target, not the actor #507 invariant ('every admin audit line downstream records both') that the newly added lines now honor; the shared cascade helper's failure line 'Failed to delete account from DynamoDB' (src/handlers/deletion_cascade.py:271) likewise lacks actor_sub. Noting for a follow-up pass; not a blocker for this change.src/handlers/account_operations.py:170- The unconditional success narration 'Deleted account from DynamoDB' inside delete_all_user_data (src/handlers/deletion_cascade.py:269, unchanged) is ordered last in the sweep, so a sweep failure before that point combined with the change's honest 'retry converges' framing is consistent — the narration is only emitted on actual success. No defect found on the ordering; noting the two paths' resource-naming claims were verified: self-service deletes the accounts row inside the sweep (deletion_cascade.py:268) so 'Account data was deleted' is true; admin deletes it after the Cognito step (admin_operations.py:1090-1096) so 'the accounts record still exists' is true. No action needed.src/utils/dynamodb_exceptions.py:15- Fix-round change:COGNITO_TRANSIENT_ERROR_CODESduplicates_RETRYABLE_CODES(src/utils/cognito.py:12-14) with only a comment claiming parity — a second hand-maintained definition of the same concept, unenforced. If a code is later added to the retry wrapper but not here (or vice versa), the exhausted-retry classification (is_transient_cognito_errorat account_operations.py:130/185, admin_operations.py:900) diverges from what actually gets retried — the same code-vs-retry-guidance contradiction F1 targeted, reintroduced structurally. Remedy contained to the change: make the set single-homed (export a public name from utils.cognito and buildCOGNITO_TRANSIENT_ERROR_CODESfrom it, or vice versa). This is also the smallest honest remedy under the simplification pass: the parallel copy of the rule is not required by the intent, which only requires one consistent classification.src/handlers/admin_operations.py:828- Sibling left behind by the fix round:_find_cognito_user_by_sub(called by admin_purge_user_account at admin_operations.py:1058) maps ANY ClientError — transient Cognito codes included — to INTERNAL_ERROR 'Failed to look up Cognito user' with no retry wrapper, while the self path's identical lookup now emits retryable RESOURCE_BUSY when the same fault exhausts its retries (src/handlers/account_operations.py:126-138, the fix round's own work). Concrete sequence on the purge path: Cognito list_users throttled (TooManyRequestsException/InternalErrorException) → aborts the purge with the catch-all the frontend maps to a generic failure, contradicting the intent mandate that a transient or throttled Cognito failure carry retry guidance. Note the purge path uses this lookup with NO retry wrapper at all, so the throttle aborts on the first attempt. Remedy contained: classify againstis_transient_cognito_errorhere like the sibling sites (the missing actor_sub in this log line is the F3 gap the user chose to ignore — not part of this finding).src/handlers/admin_operations.py:885- Round-2 fix (e23e66f) copied the self-service path's failure-provenance comment into the admin wrapper's docstring: '_delete_cognito_user_after_sweep' claims a 'failed re-lookup of an absent user lands here too', but the admin path's _delete_user_from_cognito (admin_operations.py:839) takes an already-resolved username from _find_cognito_user_by_sub and performs no re-lookup — that clause describes only the self path (account_operations.py:174, where the fallback lookup inside the account _delete_user_from_cognito can fail). Minor doc inaccuracy on the destructive purge path; drop the clause.src/handlers/admin_operations.py:919- Cosmetic path-dependent wording mismatch within this change: the admin partial-state message reads 'Account data was swept but the Cognito user could not be confirmed deleted' (singular 'Account data') while its log narration (admin_operations.py:894) and the analogous self-path message ('Account data was deleted', account_operations.py:190) use different phrasings for the same distinction. Every intent requirement (honest survival naming, actor attribution, code/retry alignment) is met on both paths and verified against the source: the sweep (deletion_cascade.py:268) deletes the accounts row on the self path before the Cognito delete; the admin path deletes it after (admin_operations.py:1129-1136) so 'the accounts record still exists' is accurate there. Purely a narration-wording nuance; no action needed.✅ **Test** - passed
✅ No issues found.
live_driver.py --mode target: 10 scenarios driving the real handlers end-to-end against a local moto ThreadedMotoServer with fault-injecting HTTP proxies (target_results.json/.md)admin purge, transient Cognito AdminDeleteUser fault (InternalErrorException): RESOURCE_BUSY payload, one ERROR line with actor_sub + account_id + aws_error_code, accounts row + Cognito user verified surviving, no legacy helper lineadmin purge, permanent Cognito fault (NotAuthorizedException): INTERNAL_ERROR with 'complete the deletion manually' and no retry promise, single attributed line, state verifiedadmin purge, DynamoDB sweep throttle (Query on shares table): RESOURCE_BUSY with 'Cognito user and the accounts record are untouched' — both verified untouched, AdminDeleteUser never attemptedadmin purge, S3 AccessDenied during QR sweep: AppError re-raised unchanged ('Failed to purge payment QR codes from S3') with attributed sweep-failure line (error_code field), nothing deletedadmin purge, transient ListUsers lookup throttle: RESOURCE_BUSY 'Retry the purge', nothing deletedadmin purge success: returns True, accounts row and Cognito user actually gone, 'User account purged' audit line carries actor_sub + account_idself-service delete, transient Cognito fault after sweep: RESOURCE_BUSY, 'Account data was deleted' verified true (accounts row gone), Cognito user survives, 3 real retry attempts observed at the proxyself-service delete, permanent Cognito fault: INTERNAL_ERROR 'complete the deletion manually in Cognito', zero pointless retries, accounts row gone / Cognito user survivesself-service delete, sweep throttle: RESOURCE_BUSY, 'the Cognito user is untouched' verified, 4 throttled Query attempts observedself-service delete, transient lookup throttle: RESOURCE_BUSY after exactly 3 project-wrapper attemptslive_driver.py --mode baseline (git archive b4134fd src/): reproduced the pre-fix defect — InternalErrorException surfaced as INTERNAL_ERROR 'Failed to delete user from Cognito' with an unattributed helper error line (baseline_results.json/.md)targeted unit validation: .venv/bin/python -m pytest tests/unit/test_admin_operations.py tests/unit/test_account_operations.py -q (287 passed)✅ **Document** - passed
✅ No issues found.
🔧 **Lint** - 2 issues found → auto-fixed ✅
src/handlers/account_operations.py:84- The CI complexity gate now fails on this change:uv run xenon --max-average A --max-absolute B --exclude '*payment_methods.py,*logging.py' src/exits 1 withblock "src/handlers/account_operations.py:84 delete_my_account" has a rank of C.delete_my_accountgrew from cyclomatic complexity 6 (rank B) at base commit b4134fd to 14 (rank C, >10) after the change's three new try/except classification blocks. AGENTS.md documents that this xenon command is the enforced CI gate (Grade A average, no block above Grade B), so the backend CI job will fail. Unresolved here because the fix requires restructuring functional code on the destructive deletion path (extracting the lookup/sweep/Cognito-delete classification blocks into helpers), which cannot be safely verified without running the test suite, and tests must not be changed in this phase.src/handlers/admin_operations.py:1027- Same CI complexity gate breach as L1:block "src/handlers/admin_operations.py:1027 admin_purge_user_account" has a rank of C. The function grew from complexity 8 (rank B) at base to 11 (rank C, just over the B ceiling of 10) after the change added the sweeptry/except AppError/except ClientErrorclassification and the_delete_cognito_user_after_sweepcall site. Same constraint as L1: a behavior-preserving extraction of the sweep-failure classification into a helper would clear it, but that is a functional refactor needing test verification, which this documentation/lint phase may not run or may not apply to tests.🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Push** - passed
✅ No issues found.