Repository navigation
Conversation
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.
…on and update AGENTS.md #521 purge order
…ith loud partial-failure handling
…user is still present Both account-deletion catch blocks claimed 'the Cognito user is still present' as established fact. Two reachable paths never established it: delete_my_account re-runs the lookup when the pre-check found no username, and admin_purge_user_account can raise after admin_delete_user already landed. The wording now states only what the code knows - the sweep completed and the Cognito delete did not report success - while keeping INTERNAL_ERROR, exception chaining, the error_code field, and exc_info=True. Tests cover both named paths and assert the message does not claim the user is present.
…l accounts-row delete The Cognito-delete catch blocks in delete_my_account and admin_purge_user_account used a bare except Exception, rewrapping the typed AppError the Cognito helper already raises and silently downgrading any future retryable RESOURCE_BUSY from it. Narrow both catches to (ClientError, BotoCoreError, AppError) and re-raise AppError untouched so its code and message reach the caller intact. BotoCoreError stays because a transport failure after the request was sent is the unknown-state case the catch exists for. delete_all_user_data still let a failure of its final accounts-row delete propagate as a raw ClientError to the decorator's generic message. It now raises an AppError that names the partial state: all user data was swept, the account record and sign-in survive, retry to complete; throttling surfaces as retryable RESOURCE_BUSY per #291. Reported gap, not fixed here: admin_operations._delete_user_from_cognito still maps a throttled admin_delete_user to INTERNAL_ERROR instead of RESOURCE_BUSY; with the narrowed catch that AppError at least passes through intact.
…BUSY; widen pre-check catch
…rror in both deletion catches
… dictionary words
Owner
Author
|
Closing as a duplicate: this branch's first four commits are merged #673, and its remaining five were superseded by #677, which I merged today. Verified rather than assumed:
One residual is not in #677 and I am carrying it forward instead of dropping it here: this branch's Merging this branch would now regress #677's classification. Closing. |
Owner
Author
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
Stop the account-deletion error handling from swallowing typed errors and from leaving the most likely partial state ambiguous. Both new handlers catch bare
except Exception, which rewraps the Cognito helper's own AppError into a different message and silently downgrades any future typed code from that helper - notably the retryable RESOURCE_BUSY that this project mandates for throttling, so a throttled Cognito failure would surface as a generic internal error instead of a retryable one. Narrow the catch to the exception types the helper actually raises so AppError propagates with its code intact. Separately, delete_all_user_data runs eight delete sub-cascades and then the accounts-row delete, and a failure of that final delete - the most likely partial state in the handler - still propagates as a raw ClientError to the decorator and surfaces as the generic "Failed to delete account", which is exactly the ambiguity this change exists to remove. Make that failure say what was swept and what survived. Tests must assert on the message as well as the code, since a code-only assertion passes against wrong wording.What Changed
except Exceptioncatches indelete_my_accountandadmin_purge_user_accountto(ClientError, BotoCoreError, AppError): a typedAppErrorfrom the Cognito helper (e.g. the retryableRESOURCE_BUSY) now re-raises with its code and message intact, throttledadmin_delete_userfailures are classified asRESOURCE_BUSYinstead ofINTERNAL_ERROR, and a Cognito delete failure after a successful sweep reports that Cognito state is unknown and needs a retry rather than claiming the user survived.delete_all_user_datanow tracks the eight sub-cascades it sweeps and wraps the final accounts-row delete: a failure raises a typedAppError(retryableRESOURCE_BUSYwhen throttled) whose message and structured log state that all user data was deleted while the account record and Cognito sign-in survive.test_account_operations.py,test_admin_operations.py, andtest_account_deletion_cascade.pyasserting both the error code and the exact wording of the partial-state messages, plus pass-through of typed errors and retry convergence.Risk Assessment
Testing
Baseline targeted pytest (294 passed) plus a from-scratch disposable live stack — a local moto AWS behind a wire-level fault-injecting gateway — through which I invoked the real delete_my_account and admin_purge_user_account handlers with AppSync events across seven scenarios (happy path, Cognito throttle, transport land-and-drop, pre-check drop, and three flavors of final accounts-row delete failure), asserting the exact payload errorCode/message, persisted DynamoDB/S3/Cognito state, structured logs, and retry convergence; all 62 checks passed, the identical scenarios fail against the extracted pre-fix base commit, and a Node bridge through the real AppSync resolver modules confirms the typed codes land in extensions.errorCode. Evidence: run log, full JSON transcripts (fixed + base), proxy request trace, and bridge output in the evidence directory; no UI surface is involved (backend/Lambda-only change).
Evidence: Live run log — 7/7 scenarios PASS (post-fix)
Evidence: Full live transcript — payloads, state, structured logs, per-check results (post-fix)
Evidence: Regression run log — base commit fails 6/7 intent scenarios
~/.no-mistakes/evidence/01M417Z90ZGJNCECS9M38QA0QW/live_driver_base_transcript.json)Evidence: Real AppSync resolvers map live payloads to GraphQL errorType + extensions.errorCode
~/.no-mistakes/evidence/01M417Z90ZGJNCECS9M38QA0QW/fault_mode.txt.trace)~/.no-mistakes/evidence/01M417Z90ZGJNCECS9M38QA0QW/live_driver.py)Evidence: Wire-level fault-injecting gateway
Evidence: Resolver bridge script
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
src/handlers/admin_operations.py:1046- The admin purge's new narrowed catch cannot deliver the intent's mandated retryable RESOURCE_BUSY for a throttled Cognito failure, while its sibling in account_operations.py:132-148 now classifies raw ClientError via is_transient_client_error. Reachable sequence: AdminDeleteUser exhausts retry_on_transient_errors with TooManyRequestsException (src/utils/cognito.py:46-52 re-raises ClientError); admin _delete_user_from_cognito (src/handlers/admin_operations.py:837-841) rewraps that into AppError(INTERNAL_ERROR, 'Failed to delete user from Cognito'); in the new catch that AppError hits theif isinstance(e, AppError): raisebranch at line 1044 and propagates as a non-retryable INTERNAL_ERROR. The new catch's ClientError limb is accordingly dead in this path and the partial-state message is never emitted for the retryable case; a raw transport BotoCoreError (connected but unknown outcome) also gets the fixed INTERNAL_ERROR без classification. The throttle classification exists only via the missing classify step: adderror_code = ErrorCode.RESOURCE_BUSY if (isinstance(e, ClientError) and is_transient_client_error(e)) else ErrorCode.INTERNAL_ERRORand use the variable in both the log.kwargs and the AppError at lines 1047-1053. Sibling sites where the same 'protect the typed/default error, attribute raw faults' pattern must hold consistently: account_operations.py:142-148 (same helper, already supplied with RESOURCE_BUSY via tests), deletion_cascade.py:273 and deletion_cascade.py:251 (is_transient_client_error already carried there). A Prod AWS Cognito throttle is the trigger and the call path already exists; the exclusive divergence remains: 'rewraps the helper's own AppError' is fixed only by accident of the helper also having converted the throttle to INTERNAL first — the retryable signal intended for the caller is lost.src/handlers/account_operations.py:115- The pre-check lookup still catches only ClientError, so a transport BotoCoreError from _lookup_cognito_user_with_retry escapes the handler to the far-generic decorator wording ('Failed to delete account') — honest, but inconsistent with the new post-sweep block which deliberately added BotoCoreError to its catch for exactly the same transport class. A ConnectionError at the very first list_users yields the generic message while the rest of the flow now uses the precise pattern. Extending the pre-check except-branch to (ClientError, BotoCoreError) aligns the two. No masking risk: the existing branch already swallows only the message.src/handlers/admin_operations.py:1063- Pre-existing adjacent partial state (not introduced by this diff, observed while tracing the new code): the purge's accounts-row delete failure after the Cognito delete succeeded still raises AppError(INTERNAL_ERROR, 'Failed to delete account record') with no indication that the Cognito user is already gone — the same 'what survived is ambiguous' class the intent targets, in the step immediately following the changed block. The sweep and Cognito-delete ambiguity in the diff itself is addressed; this adjacent handler-owned wording was not. Informational only; the change is scoped to the sweep/Cognito boundary, not here.🔧 Fix applied.
3 issues (1 error, 2 infos) still open:
src/handlers/account_operations.py:115- The pre-check lookup still catches only ClientError, so a transport BotoCoreError from _lookup_cognito_user_with_retry escapes the handler to the far-generic decorator wording ('Failed to delete account') — honest, but inconsistent with the new post-sweep block which deliberately added BotoCoreError to its catch for exactly the same transport class. A ConnectionError at the very first list_users yields the generic message while the rest of the flow now uses the precise pattern. Extending the pre-check except-branch to (ClientError, BotoCoreError) aligns the two. No masking risk: the existing branch already swallows only the message.src/handlers/admin_operations.py:1063- Pre-existing adjacent partial state (not introduced by this diff, observed while tracing the new code): the purge's accounts-row delete failure after the Cognito delete succeeded still raises AppError(INTERNAL_ERROR, 'Failed to delete account record') with no indication that the Cognito user is already gone — the same 'what survived is ambiguous' class the intent targets, in the step immediately following the changed block. The sweep and Cognito-delete ambiguity in the diff itself is addressed; this adjacent handler-owned wording was not. Informational only; the change is scoped to the sweep/Cognito boundary, not here.src/handlers/account_operations.py:146- The post-sweep catch calls is_transient_client_error(e) unconditionally, but e can be a BotoCoreError, which has no.responseattribute (verified: botocore.exceptions.ConnectionError(...).hasattr('response') is False). Concrete reachable sequence: self-service delete_my_account completes the sweep, then admin_delete_user's transport fails (botocore raises ConnectionError/EndpointConnectionError, a BotoCoreError subclass, e.g. connection reset after the request was sent — precisely the unknown-state case the code comment says BotoCoreError is kept for); retry_on_transient_errors only catches ClientError (src/utils/cognito.py:43-52) so the BotoCoreError propagates raw into the handler'sexcept (ClientError, BotoCoreError, AppError); line 146 evaluatese.response-> AttributeError. The AttributeError escapes the handler to the decorator, which returns the generic INTERNAL_ERROR 'Failed to delete account' — the exact pre-change wording this change exists to remove — in the most ambiguous failure mode (delete may have landed). The admin purge path has a behavior test for exactly this scenario (test_cognito_delete_raising_after_success_does_not_claim_user_present) but the self-service path has none, so the crash is untested. Remedy:error_code = ErrorCode.RESOURCE_BUSY if isinstance(e, ClientError) and is_transient_client_error(e) else ErrorCode.INTERNAL_ERROR. Sibling site of the same transport-fault attribution class, left behind by the pre-round-1 commit 742ff9a in the new partial-state block: src/handlers/deletion_cascade.py:273 catches only ClientError at the final accounts-row delete, so a BotoCoreError there (transport fault on the intent's target failure) still propagates raw to the decorator's generic 'Failed to delete account' instead of the mandated swept/survived message — widen that catch to (ClientError, BotoCoreError) and classify only the ClientError limb via is_transient_client_error (BotoCoreError -> INTERNAL_ERROR), keeping the intent's message for both. Neither site should call is_transient_client_error with a non-ClientError.🔧 Fix applied.
1 info still open:
src/handlers/admin_operations.py:1063- Pre-existing adjacent partial state (not introduced by this diff, observed while tracing the new code): the purge's accounts-row delete failure after the Cognito delete succeeded still raises AppError(INTERNAL_ERROR, 'Failed to delete account record') with no indication that the Cognito user is already gone — the same 'what survived is ambiguous' class the intent targets, in the step immediately following the changed block. The sweep and Cognito-delete ambiguity in the diff itself is addressed; this adjacent handler-owned wording was not. Informational only; the change is scoped to the sweep/Cognito boundary, not here.✅ **Test** - passed
✅ No issues found.
Baseline targeted run:.venv/bin/pytest tests/unit/test_account_operations.py tests/unit/test_admin_operations.py tests/unit/test_account_deletion_cascade.py -q→ 294 passed (coverage-gate line fails only because it is a subset run)Built a disposable stack in the evidence dir:fault_proxy.py(HTTP fault gateway: injected Cognito throttle 400s, land-and-drop transport faults, injected DynamoDB DeleteItem throttle/access-denied/drop) in front of a localmoto_server, withAWS_ENDPOINT_URLrouting every product-side boto3 call through the gatewayRanlive_driver.py— 7 scenarios, 62 checks, all pass: happy-path self-service delete; self-service Cognito throttle → RESOURCE_BUSY + partial-state message + retry converges; admin purge throttle → helper's typed RESOURCE_BUSY passes through + retry converges; admin purge land-and-drop → unknown-state message, ground truth shows the delete actually landed; self-service transport fault after sweep → no AttributeError/generic wording; pre-check lookup drop → early typed failure with nothing swept; final accounts-row delete throttle/denied/transport → swept/survived message with RESOURCE_BUSY/INTERNAL_ERROR + retry convergesRegression: same harness against the extracted base commit (LIVE_DRIVER_SRC_ROOT=/tmp/kw673_base git archive b4134fd… src) → all 6 intent scenarios FAIL on their discriminating checks (generic 'Failed to delete account'/'Failed to purge user account', INTERNAL_ERROR instead of RESOURCE_BUSY) while the happy path passes; temp checkout removed afterwardsDrove the live error payloads through the reallambda_unit_resolver.js/lambda_passthrough_resolver.js(node --import …/register-loader.mjs resolver_bridge.mjs) showing errorType and extensions.errorCode = RESOURCE_BUSY/INTERNAL_ERROR reach the GraphQL boundaryVerified worktree untouched:git status --shortclean after all runs✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.