From c7c86c6c9c46b513c302463ff36ec58cedf101da Mon Sep 17 00:00:00 2001 From: David Meiser Date: Mon, 28 Sep 2026 16:41:01 -0400 Subject: [PATCH 1/9] 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. --- src/handlers/account_operations.py | 30 ++++++++-- src/handlers/admin_operations.py | 82 ++++++++++++++++---------- tests/unit/test_account_operations.py | 69 ++++++++++++++++++++++ tests/unit/test_admin_operations.py | 83 +++++++++++++++++++++++++++ 4 files changed, 228 insertions(+), 36 deletions(-) diff --git a/src/handlers/account_operations.py b/src/handlers/account_operations.py index b8ad3d4e..3a06e791 100644 --- a/src/handlers/account_operations.py +++ b/src/handlers/account_operations.py @@ -12,7 +12,9 @@ # Sibling handler modules use a same-package relative import, which resolves both # in the Lambda zip (package `handlers`) and in unit tests (package `src.handlers`). -from .deletion_cascade import delete_all_user_data +# The delete_my_account data phase runs through this module-local name, the seam +# where a failing cascade is substituted in tests (#551). +from .deletion_cascade import delete_all_user_data as _delete_all_user_data # Handle both Lambda (absolute) and unit test (relative) imports try: # pragma: no cover @@ -84,9 +86,9 @@ def delete_my_account(event: Dict[str, Any], context: Any) -> bool: Delete the authenticated user's account and all associated data. This is a self-service account deletion that: - 1. Verifies Cognito credentials and connectivity before deleting DynamoDB data - 2. Deletes all user data from DynamoDB (profiles, campaigns, orders, shares, invites; catalogs are preserved) - 3. Deletes the user from Cognito User Pool with retry on transient errors + 1. Verifies Cognito credentials and connectivity before deleting data + 2. Deletes the user from Cognito User Pool with retry on transient errors (the commit point) + 3. Deletes all user data from DynamoDB (profiles, campaigns, orders, shares, invites; catalogs are preserved) Args: event: AppSync event with identity @@ -124,8 +126,26 @@ def delete_my_account(event: Dict[str, Any], context: Any) -> bool: logger.error("Cognito lookup failed before deletion", account_id=account_id, error=str(e), exc_info=True) raise AppError(ErrorCode.INTERNAL_ERROR, "Failed to delete account") - delete_all_user_data(account_id, logger) + # Delete the Cognito user FIRST so the Cognito call is the commit point + # (#551): a data-phase failure afterwards leaves records the user can no + # longer reach (no sign-in, so no post_authentication re-bootstrap) and a + # re-run of the data phase is safe; a Cognito failure leaves every record + # intact. Deleting data first would leave a sign-in-capable Cognito user + # whose records are gone, silently re-bootstrapped on next sign-in. _delete_user_from_cognito(cognito, user_pool_id, account_id, username, logger) + + try: + _delete_all_user_data(account_id, logger) + except Exception as e: + logger.error( + "Cognito user already deleted but user data cleanup failed; " + "leftover records are inert because the user can no longer sign in", + account_id=account_id, + error=str(e), + error_code=ErrorCode.INTERNAL_ERROR, + ) + raise + logger.info("Account deletion completed successfully") return True diff --git a/src/handlers/admin_operations.py b/src/handlers/admin_operations.py index 16c2c56a..eef08dae 100644 --- a/src/handlers/admin_operations.py +++ b/src/handlers/admin_operations.py @@ -3,19 +3,20 @@ Provides: - adminResetUserPassword: Send password reset email to user -- adminPurgeUserAccount: Sweep the client-unreachable residue, then delete the - accounts record and the Cognito user +- adminPurgeUserAccount: Delete the Cognito user (the commit point), then + sweep the client-unreachable residue and the accounts record - createManagedCatalog: Create an ADMIN_MANAGED global catalog User deletion is client-side by decision (#521). The client issues the per-entity ``adminDeleteUser*`` mutations it can reach, reads the profile IDs with ``adminGetUserProfiles`` before deleting the profile rows, and calls -``adminPurgeUserAccount`` with them as ``profileIds``. The purge sweeps the -four classes the browser cannot reach itself — invites for the account's -profiles, inbound shares on other owners' profiles, S3 report objects, and -payment-QR S3 objects — reusing the shared ``deletion_cascade`` helpers, then -deletes the ``accounts`` record and the Cognito user. Catalogs are never -deleted. +``adminPurgeUserAccount`` with them as ``profileIds``. After verifying the +client's cascade finished, the purge deletes the Cognito user first (the +commit point, #551), then sweeps the four classes the browser cannot reach +itself — invites for the account's profiles, inbound shares on other owners' +profiles, S3 report objects, and payment-QR S3 objects — reusing the shared +``deletion_cascade`` helpers, and finally the ``accounts`` record. Catalogs +are never deleted. Audit attribution (#507): AppSync logs at ERROR level only, so these handlers are the only record that an admin operation happened at all. Every admin audit @@ -965,12 +966,12 @@ def admin_purge_user_account(event: Dict[str, Any], context: Any) -> bool: BEFORE deleting the profile rows, as the ``profileIds`` argument. The purge first verifies each of them is actually gone with a strongly consistent read — a surviving profile means the client cascade has not completed, so - it refuses with CONFLICT and deletes nothing — then sweeps the four - classes the browser cannot reach, reusing the shared deletion_cascade - helpers: invites for the account's profiles, inbound shares on other - owners' profiles, S3 report objects, and payment-QR S3 objects. Invites - and reports are keyed by profileId, which survives the deleted profile - rows. + it refuses with CONFLICT and deletes nothing — then deletes the Cognito + user (the commit point, #551) and sweeps the four classes the browser + cannot reach, reusing the shared deletion_cascade helpers: invites for + the account's profiles, inbound shares on other owners' profiles, S3 + report objects, and payment-QR S3 objects. Invites and reports are keyed + by profileId, which survives the deleted profile rows. Call this after the client's per-entity deletes, so a failure there leaves the account intact rather than half-deleted. @@ -1013,27 +1014,46 @@ def admin_purge_user_account(event: Dict[str, Any], context: Any) -> bool: # has not completed, so nothing is deleted and the account stays intact. _assert_profiles_deleted(account_id, profile_ids, logger, actor_sub) - profiles_for_sweep = [{"profileId": profile_id} for profile_id in profile_ids] - delete_invites_for_owned_profiles(profiles_for_sweep, logger) - delete_inbound_shares(account_id, logger) - delete_user_s3_reports(profiles_for_sweep, logger) - delete_all_user_qr_codes(account_id, logger) - + # Delete the Cognito user FIRST so the Cognito call is the commit point (#551). + # A data-phase failure afterwards leaves records the user can no longer reach + # (no sign-in, so no post_authentication re-bootstrap) and a re-run of the + # data phase is safe; a Cognito failure leaves every record intact. Deleting + # data first would leave a sign-in-capable Cognito user whose records are + # gone, silently re-bootstrapped as an empty account on next sign-in. if username: _delete_user_from_cognito(cognito, user_pool_id, username, email or "", logger, actor_sub) else: - logger.info("Cognito user already absent", account_id=account_id, actor_sub=actor_sub) + logger.info("Cognito user already absent; proceeding with DynamoDB cleanup", account_id=account_id, actor_sub=actor_sub) - if account_exists: - db_account_id = normalize_account_id(account_id) - try: - tables.accounts.delete_item(Key={"accountId": db_account_id}) - logger.info("Deleted account record", account_id=db_account_id, actor_sub=actor_sub) - except ClientError as e: - logger.error("Failed to delete account record", error=str(e), account_id=db_account_id) - raise AppError(ErrorCode.INTERNAL_ERROR, "Failed to delete account record") from e - else: - logger.info("Account record already absent", account_id=account_id, actor_sub=actor_sub) + # Everything below runs after the Cognito commit: a failure here is logged + # loudly with an error_code-tagged field and re-raised, so the partial delete + # is detectable while the leftover records stay inert. + try: + profiles_for_sweep = [{"profileId": profile_id} for profile_id in profile_ids] + delete_invites_for_owned_profiles(profiles_for_sweep, logger) + delete_inbound_shares(account_id, logger) + delete_user_s3_reports(profiles_for_sweep, logger) + delete_all_user_qr_codes(account_id, logger) + + if account_exists: + db_account_id = normalize_account_id(account_id) + try: + tables.accounts.delete_item(Key={"accountId": db_account_id}) + logger.info("Deleted account record", account_id=db_account_id, actor_sub=actor_sub) + except ClientError as e: + logger.error("Failed to delete account record", error=str(e), account_id=db_account_id) + raise AppError(ErrorCode.INTERNAL_ERROR, "Failed to delete account record") from e + else: + logger.info("Account record already absent", account_id=account_id, actor_sub=actor_sub) + except Exception as e: + logger.error( + "Cognito user already deleted but user data cleanup failed; " + "leftover records are inert because the user can no longer sign in", + account_id=account_id, + error=str(e), + error_code=ErrorCode.INTERNAL_ERROR, + ) + raise logger.info("User account purged", account_id=account_id, email=mask_email(email), actor_sub=actor_sub) return True diff --git a/tests/unit/test_account_operations.py b/tests/unit/test_account_operations.py index 87773f66..cfda7359 100644 --- a/tests/unit/test_account_operations.py +++ b/tests/unit/test_account_operations.py @@ -772,6 +772,75 @@ def test_delete_account_cognito_admin_delete_error( assert "InternalError" in captured assert "AdminDeleteUser" in captured + # The data cascade must not have run: the account record survives so a + # retry converges (#551). + assert accounts_table.get_item(Key={"accountId": account_id_key}).get("Item") is not None + + def test_delete_account_data_failure_after_cognito_commit( + self, + dynamodb_table: Any, + sample_account_id: str, + appsync_event: Dict[str, Any], + lambda_context: Any, + monkeypatch: Any, + capsys: Any, + ) -> None: + """A data-phase failure after the Cognito commit is logged loudly (#551). + + The Cognito user is already gone, so leftover records are inert and a + re-run of the data phase is safe; the partial delete must be detectable in + post-incident review via the error_code-tagged log. + """ + from botocore.exceptions import ClientError + + from src.handlers.account_operations import delete_my_account + + monkeypatch.setenv("ACCOUNTS_TABLE_NAME", "kernelworx-accounts-ue1-dev") + monkeypatch.setenv("USER_POOL_ID", "us-east-1_test123") + + dynamodb = boto3.resource("dynamodb", region_name="us-east-1") + accounts_table = dynamodb.Table("kernelworx-accounts-ue1-dev") + account_id_key = f"ACCOUNT#{sample_account_id}" + + accounts_table.put_item( + Item={ + "accountId": account_id_key, + "email": "test@example.com", + "createdAt": datetime.now(timezone.utc).isoformat(), + } + ) + + with patch("boto3.client") as mock_boto_client: + mock_cognito = MagicMock() + mock_boto_client.return_value = mock_cognito + mock_cognito.list_users.return_value = { + "Users": [{"Username": "test-user", "Attributes": [{"Name": "sub", "Value": sample_account_id}]}] + } + + with patch( + "src.handlers.account_operations._delete_all_user_data", + side_effect=ClientError( + {"Error": {"Code": "InternalServerError", "Message": "DynamoDB exploded"}}, + "DeleteItem", + ), + ): + event = { + **appsync_event, + "identity": {"sub": sample_account_id}, + } + + result = delete_my_account(event, lambda_context) + + assert result["__isError"] is True + assert result["errorCode"] == ErrorCode.INTERNAL_ERROR + # The commit point was reached: the Cognito user is gone. + mock_cognito.admin_delete_user.assert_called_once_with( + UserPoolId="us-east-1_test123", Username="test-user" + ) + captured = capsys.readouterr().out + assert "data cleanup failed" in captured + assert "INTERNAL_ERROR" in captured + def test_delete_account_unexpected_exception( self, dynamodb_table: Any, diff --git a/tests/unit/test_admin_operations.py b/tests/unit/test_admin_operations.py index fc5425d9..3460a684 100644 --- a/tests/unit/test_admin_operations.py +++ b/tests/unit/test_admin_operations.py @@ -1854,6 +1854,89 @@ def test_purge_sweeps_inbound_shares( is not None ) + def test_cognito_failure_leaves_user_data_intact( + self, + dynamodb_table: Any, + invites_table: Any, + admin_appsync_event: Dict[str, Any], + lambda_context: Any, + monkeypatch: Any, + ) -> None: + """A Cognito delete failure must not sweep any user data (#551). + + The Cognito call is the commit point: if it fails, the data phase must + never run, so the half-deleted state (Cognito user alive, residue + destroyed) that the ordering fix targets cannot occur. + """ + monkeypatch.setenv("USER_POOL_ID", "test-pool-id") + monkeypatch.setenv("ACCOUNTS_TABLE_NAME", "kernelworx-accounts-ue1-dev") + + target_account_id = "target-user-123" + self._seed_account(target_account_id) + invites_table.put_item(Item={"inviteCode": "INV-OWNED", "profileId": "PROFILE#p1", "status": "PENDING"}) + + event = {**admin_appsync_event, "arguments": {"accountId": target_account_id, "profileIds": ["PROFILE#p1"]}} + + with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + mock_cognito = self._mock_cognito(target_account_id) + mock_cognito.admin_delete_user.side_effect = ClientError( + {"Error": {"Code": "NotAuthorizedException", "Message": "Access denied."}}, + "AdminDeleteUser", + ) + mock_get_client.return_value = mock_cognito + + result = admin_purge_user_account(event, lambda_context) + + assert result["__isError"] is True + assert result["errorCode"] == ErrorCode.INTERNAL_ERROR + # Neither the sweep nor the account-record delete ran: the residue and + # the accounts row survive so the admin retry converges. + assert invites_table.get_item(Key={"inviteCode": "INV-OWNED"}).get("Item") is not None + assert "Item" in get_accounts_table().get_item(Key={"accountId": f"ACCOUNT#{target_account_id}"}) + + def test_data_failure_after_cognito_commit( + self, + dynamodb_table: Any, + admin_appsync_event: Dict[str, Any], + lambda_context: Any, + monkeypatch: Any, + capsys: Any, + ) -> None: + """A data-phase failure after the Cognito commit is logged loudly (#551). + + The Cognito user is already gone, so leftover records are inert and a + re-run of the data phase is safe; the partial delete must be detectable + in post-incident review via the error_code-tagged log. + """ + monkeypatch.setenv("USER_POOL_ID", "test-pool-id") + monkeypatch.setenv("ACCOUNTS_TABLE_NAME", "kernelworx-accounts-ue1-dev") + + target_account_id = "target-user-123" + self._seed_account(target_account_id) + + event = {**admin_appsync_event, "arguments": {"accountId": target_account_id, "profileIds": []}} + + with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + mock_cognito = self._mock_cognito(target_account_id) + mock_get_client.return_value = mock_cognito + + with patch( + "src.handlers.admin_operations.delete_invites_for_owned_profiles", + side_effect=ClientError( + {"Error": {"Code": "InternalServerError", "Message": "DynamoDB exploded"}}, + "DeleteItem", + ), + ): + result = admin_purge_user_account(event, lambda_context) + + assert result["__isError"] is True + assert result["errorCode"] == ErrorCode.INTERNAL_ERROR + # The commit point was reached: the Cognito user is gone. + mock_cognito.admin_delete_user.assert_called_once_with(UserPoolId="test-pool-id", Username=target_account_id) + captured = capsys.readouterr().out + assert "data cleanup failed" in captured + assert "INTERNAL_ERROR" in captured + def test_purge_deletes_s3_reports_for_the_supplied_profiles( self, dynamodb_table: Any, From 7ac2b8fb9d1b70a17af4ccade00d2dae43dfa6a1 Mon Sep 17 00:00:00 2001 From: David Meiser Date: Thu, 1 Oct 2026 19:56:59 -0400 Subject: [PATCH 2/9] no-mistakes(review): Fix delete_my_account data-phase error attribution and update AGENTS.md #521 purge order --- AGENTS.md | 2 +- src/handlers/account_operations.py | 32 ++++++++++++++++-------------- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 433f6761..2f7c64cd 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -93,7 +93,7 @@ Chunk 1 (#351) established the pattern chunks 2-4 reused; chunk 5 (#355) retired ### User deletion is client-side, and catalogs are never deleted (#521) -Admin user deletion is driven from the client by decision, on cost grounds. The admin UI issues the per-entity `adminDeleteUserOrders` / `Campaigns` / `Shares` / `Profiles` mutations in order, then calls `adminPurgeUserAccount` last. The whole-user `adminDeleteUser` Lambda handler and its schema field, resolver, and dispatch-map entry were removed deliberately in #521 — do not reinstate them, and do not treat their absence as dead code to restore. `adminPurgeUserAccount` is NOT minimal beyond the browser boundary: the client reads the profile IDs via `adminGetUserProfiles` before deleting the profile rows and passes them as the required `profileIds` argument, and the purge (1) verifies with a strongly consistent per-profile get_item that every supplied profile is actually gone, refusing with `ErrorCode.CONFLICT` — deleting nothing — while any remains (a premature or partial purge fails loudly and the account stays recoverable), (2) sweeps the four classes the browser cannot reach by reusing the shared `deletion_cascade` helpers — `delete_invites_for_owned_profiles` and `delete_user_s3_reports` by the supplied `profileIds` (never by re-reading the profiles table, since the client already deleted those rows), plus `delete_inbound_shares` and `delete_all_user_qr_codes` by account — and only then (3) deletes the `accounts` record and the Cognito user, the two things no browser can do because it holds no AWS credentials and AppSync has no Cognito data source. It lives in the shared `admin-operations` Lambda (which still serves the other admin mutations), and it must stay last so a mid-cascade failure leaves the account intact rather than half-deleted. +Admin user deletion is driven from the client by decision, on cost grounds. The admin UI issues the per-entity `adminDeleteUserOrders` / `Campaigns` / `Shares` / `Profiles` mutations in order, then calls `adminPurgeUserAccount` last. The whole-user `adminDeleteUser` Lambda handler and its schema field, resolver, and dispatch-map entry were removed deliberately in #521 — do not reinstate them, and do not treat their absence as dead code to restore. `adminPurgeUserAccount` is NOT minimal beyond the browser boundary: the client reads the profile IDs via `adminGetUserProfiles` before deleting the profile rows and passes them as the required `profileIds` argument, and the purge (1) verifies with a strongly consistent per-profile get_item that every supplied profile is actually gone, refusing with `ErrorCode.CONFLICT` — deleting nothing — while any remains, (2) deletes the Cognito user FIRST as the commit point (#551), then (3) sweeps the four classes the browser cannot reach by reusing the shared `deletion_cascade` helpers — `delete_invites_for_owned_profiles` and `delete_user_s3_reports` by the supplied `profileIds` (never by re-reading the profiles table, since the client already deleted those rows), plus `delete_inbound_shares` and `delete_all_user_qr_codes` by account — and (4) deletes the `accounts` record, the things no browser can do because it holds no AWS credentials and AppSync has no Cognito data source. A Cognito failure leaves every record intact; a data-phase failure after the Cognito commit leaves only inert records the user can no longer reach, logged loudly with an error_code-tagged field. It lives in the shared `admin-operations` Lambda (which still serves the other admin mutations), and it must be called last so a mid-cascade failure leaves the account intact rather than half-deleted. Catalogs are never deleted, by product decision. The `adminDeleteUserCatalogs` mutation and its soft-delete helpers were removed in #521 for the same reason; there is no catalog step in the deletion path, and the admin confirmation dialog says catalogs are preserved. `tests/unit/test_admin_operations.py::TestAdminPurgeUserAccount::test_catalog_survives_a_purge` and the matching `AdminPage.interactions.test.tsx` case guard both halves. diff --git a/src/handlers/account_operations.py b/src/handlers/account_operations.py index 3a06e791..f9584f8c 100644 --- a/src/handlers/account_operations.py +++ b/src/handlers/account_operations.py @@ -134,21 +134,23 @@ def delete_my_account(event: Dict[str, Any], context: Any) -> bool: # whose records are gone, silently re-bootstrapped on next sign-in. _delete_user_from_cognito(cognito, user_pool_id, account_id, username, logger) - try: - _delete_all_user_data(account_id, logger) - except Exception as e: - logger.error( - "Cognito user already deleted but user data cleanup failed; " - "leftover records are inert because the user can no longer sign in", - account_id=account_id, - error=str(e), - error_code=ErrorCode.INTERNAL_ERROR, - ) - raise - - logger.info("Account deletion completed successfully") - return True - except ClientError as e: logger.error("Cognito error during account deletion", account_id=account_id, error=str(e), exc_info=True) raise AppError(ErrorCode.INTERNAL_ERROR, "Failed to delete account") + + # The data phase must not be caught by the Cognito-labeled except above; a + # post-commit DynamoDB/S3 failure carries its own error_code-tagged log. + try: + _delete_all_user_data(account_id, logger) + except Exception as e: + logger.error( + "Cognito user already deleted but user data cleanup failed; " + "leftover records are inert because the user can no longer sign in", + account_id=account_id, + error=str(e), + error_code=ErrorCode.INTERNAL_ERROR, + ) + raise + + logger.info("Account deletion completed successfully") + return True From ee3842601bbc7101b6b986baed53a992b35422f5 Mon Sep 17 00:00:00 2001 From: David Meiser Date: Thu, 1 Oct 2026 20:12:38 -0400 Subject: [PATCH 3/9] no-mistakes(review): Revert user deletion to data-sweep-first order with loud partial-failure handling --- AGENTS.md | 2 +- src/handlers/account_operations.py | 61 +++++++-------- src/handlers/admin_operations.py | 107 +++++++++++++------------- tests/unit/test_account_operations.py | 74 +++++++++--------- tests/unit/test_admin_operations.py | 63 ++++++++------- 5 files changed, 156 insertions(+), 151 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 2f7c64cd..433f6761 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -93,7 +93,7 @@ Chunk 1 (#351) established the pattern chunks 2-4 reused; chunk 5 (#355) retired ### User deletion is client-side, and catalogs are never deleted (#521) -Admin user deletion is driven from the client by decision, on cost grounds. The admin UI issues the per-entity `adminDeleteUserOrders` / `Campaigns` / `Shares` / `Profiles` mutations in order, then calls `adminPurgeUserAccount` last. The whole-user `adminDeleteUser` Lambda handler and its schema field, resolver, and dispatch-map entry were removed deliberately in #521 — do not reinstate them, and do not treat their absence as dead code to restore. `adminPurgeUserAccount` is NOT minimal beyond the browser boundary: the client reads the profile IDs via `adminGetUserProfiles` before deleting the profile rows and passes them as the required `profileIds` argument, and the purge (1) verifies with a strongly consistent per-profile get_item that every supplied profile is actually gone, refusing with `ErrorCode.CONFLICT` — deleting nothing — while any remains, (2) deletes the Cognito user FIRST as the commit point (#551), then (3) sweeps the four classes the browser cannot reach by reusing the shared `deletion_cascade` helpers — `delete_invites_for_owned_profiles` and `delete_user_s3_reports` by the supplied `profileIds` (never by re-reading the profiles table, since the client already deleted those rows), plus `delete_inbound_shares` and `delete_all_user_qr_codes` by account — and (4) deletes the `accounts` record, the things no browser can do because it holds no AWS credentials and AppSync has no Cognito data source. A Cognito failure leaves every record intact; a data-phase failure after the Cognito commit leaves only inert records the user can no longer reach, logged loudly with an error_code-tagged field. It lives in the shared `admin-operations` Lambda (which still serves the other admin mutations), and it must be called last so a mid-cascade failure leaves the account intact rather than half-deleted. +Admin user deletion is driven from the client by decision, on cost grounds. The admin UI issues the per-entity `adminDeleteUserOrders` / `Campaigns` / `Shares` / `Profiles` mutations in order, then calls `adminPurgeUserAccount` last. The whole-user `adminDeleteUser` Lambda handler and its schema field, resolver, and dispatch-map entry were removed deliberately in #521 — do not reinstate them, and do not treat their absence as dead code to restore. `adminPurgeUserAccount` is NOT minimal beyond the browser boundary: the client reads the profile IDs via `adminGetUserProfiles` before deleting the profile rows and passes them as the required `profileIds` argument, and the purge (1) verifies with a strongly consistent per-profile get_item that every supplied profile is actually gone, refusing with `ErrorCode.CONFLICT` — deleting nothing — while any remains (a premature or partial purge fails loudly and the account stays recoverable), (2) sweeps the four classes the browser cannot reach by reusing the shared `deletion_cascade` helpers — `delete_invites_for_owned_profiles` and `delete_user_s3_reports` by the supplied `profileIds` (never by re-reading the profiles table, since the client already deleted those rows), plus `delete_inbound_shares` and `delete_all_user_qr_codes` by account — and only then (3) deletes the `accounts` record and the Cognito user, the two things no browser can do because it holds no AWS credentials and AppSync has no Cognito data source. It lives in the shared `admin-operations` Lambda (which still serves the other admin mutations), and it must stay last so a mid-cascade failure leaves the account intact rather than half-deleted. Catalogs are never deleted, by product decision. The `adminDeleteUserCatalogs` mutation and its soft-delete helpers were removed in #521 for the same reason; there is no catalog step in the deletion path, and the admin confirmation dialog says catalogs are preserved. `tests/unit/test_admin_operations.py::TestAdminPurgeUserAccount::test_catalog_survives_a_purge` and the matching `AdminPage.interactions.test.tsx` case guard both halves. diff --git a/src/handlers/account_operations.py b/src/handlers/account_operations.py index f9584f8c..976ea34c 100644 --- a/src/handlers/account_operations.py +++ b/src/handlers/account_operations.py @@ -12,9 +12,7 @@ # Sibling handler modules use a same-package relative import, which resolves both # in the Lambda zip (package `handlers`) and in unit tests (package `src.handlers`). -# The delete_my_account data phase runs through this module-local name, the seam -# where a failing cascade is substituted in tests (#551). -from .deletion_cascade import delete_all_user_data as _delete_all_user_data +from .deletion_cascade import delete_all_user_data # Handle both Lambda (absolute) and unit test (relative) imports try: # pragma: no cover @@ -86,9 +84,9 @@ def delete_my_account(event: Dict[str, Any], context: Any) -> bool: Delete the authenticated user's account and all associated data. This is a self-service account deletion that: - 1. Verifies Cognito credentials and connectivity before deleting data - 2. Deletes the user from Cognito User Pool with retry on transient errors (the commit point) - 3. Deletes all user data from DynamoDB (profiles, campaigns, orders, shares, invites; catalogs are preserved) + 1. Verifies Cognito credentials and connectivity before deleting DynamoDB data + 2. Deletes all user data from DynamoDB (profiles, campaigns, orders, shares, invites; catalogs are preserved) + 3. Deletes the user from Cognito User Pool with retry on transient errors Args: event: AppSync event with identity @@ -98,7 +96,9 @@ def delete_my_account(event: Dict[str, Any], context: Any) -> bool: True if account was deleted successfully Raises: - AppError: If deletion error occurs + AppError: If deletion error occurs. When the data sweep succeeded but + the Cognito delete failed, the error names the partial state so + the caller knows the account needs a retry to complete. """ logger.info("delete_my_account handler invoked") @@ -113,44 +113,35 @@ def delete_my_account(event: Dict[str, Any], context: Any) -> bool: cognito = boto3.client("cognito-idp") - # Handler-specific Cognito error handling stays in this try block: the - # decorator only covers the generic unexpected-exception path, not typed - # ClientError mapping. + # Pre-check Cognito lookup to fail early on authorization/network/config issues try: - # Pre-check Cognito lookup to fail early on authorization/network/config issues - try: - username = _lookup_cognito_user_with_retry(cognito, user_pool_id, account_id) - if not username: - logger.warning(f"User not found in Cognito with sub: {account_id}") - except ClientError as e: - logger.error("Cognito lookup failed before deletion", account_id=account_id, error=str(e), exc_info=True) - raise AppError(ErrorCode.INTERNAL_ERROR, "Failed to delete account") - - # Delete the Cognito user FIRST so the Cognito call is the commit point - # (#551): a data-phase failure afterwards leaves records the user can no - # longer reach (no sign-in, so no post_authentication re-bootstrap) and a - # re-run of the data phase is safe; a Cognito failure leaves every record - # intact. Deleting data first would leave a sign-in-capable Cognito user - # whose records are gone, silently re-bootstrapped on next sign-in. - _delete_user_from_cognito(cognito, user_pool_id, account_id, username, logger) - + username = _lookup_cognito_user_with_retry(cognito, user_pool_id, account_id) + if not username: + logger.warning(f"User not found in Cognito with sub: {account_id}") except ClientError as e: - logger.error("Cognito error during account deletion", account_id=account_id, error=str(e), exc_info=True) + logger.error("Cognito lookup failed before deletion", account_id=account_id, error=str(e), exc_info=True) raise AppError(ErrorCode.INTERNAL_ERROR, "Failed to delete account") - # The data phase must not be caught by the Cognito-labeled except above; a - # post-commit DynamoDB/S3 failure carries its own error_code-tagged log. + # The data sweep runs before the Cognito delete and is safe to re-run, so a + # retry after any failure below converges; a sweep failure here leaves the + # Cognito user untouched. + delete_all_user_data(account_id, logger) + try: - _delete_all_user_data(account_id, logger) + _delete_user_from_cognito(cognito, user_pool_id, account_id, username, logger) except Exception as e: logger.error( - "Cognito user already deleted but user data cleanup failed; " - "leftover records are inert because the user can no longer sign in", + "Account data swept but the Cognito user is still present; " + "the account needs manual completion or a retry", account_id=account_id, error=str(e), - error_code=ErrorCode.INTERNAL_ERROR, + error_code=getattr(e, "error_code", None) or ErrorCode.INTERNAL_ERROR, + exc_info=True, ) - raise + raise AppError( + ErrorCode.INTERNAL_ERROR, + "Account data was deleted but the Cognito user is still present; retry to complete the deletion", + ) from e logger.info("Account deletion completed successfully") return True diff --git a/src/handlers/admin_operations.py b/src/handlers/admin_operations.py index eef08dae..0e140aeb 100644 --- a/src/handlers/admin_operations.py +++ b/src/handlers/admin_operations.py @@ -3,20 +3,19 @@ Provides: - adminResetUserPassword: Send password reset email to user -- adminPurgeUserAccount: Delete the Cognito user (the commit point), then - sweep the client-unreachable residue and the accounts record +- adminPurgeUserAccount: Sweep the client-unreachable residue, then delete the + accounts record and the Cognito user - createManagedCatalog: Create an ADMIN_MANAGED global catalog User deletion is client-side by decision (#521). The client issues the per-entity ``adminDeleteUser*`` mutations it can reach, reads the profile IDs with ``adminGetUserProfiles`` before deleting the profile rows, and calls -``adminPurgeUserAccount`` with them as ``profileIds``. After verifying the -client's cascade finished, the purge deletes the Cognito user first (the -commit point, #551), then sweeps the four classes the browser cannot reach -itself — invites for the account's profiles, inbound shares on other owners' -profiles, S3 report objects, and payment-QR S3 objects — reusing the shared -``deletion_cascade`` helpers, and finally the ``accounts`` record. Catalogs -are never deleted. +``adminPurgeUserAccount`` with them as ``profileIds``. The purge sweeps the +four classes the browser cannot reach itself — invites for the account's +profiles, inbound shares on other owners' profiles, S3 report objects, and +payment-QR S3 objects — reusing the shared ``deletion_cascade`` helpers, then +deletes the ``accounts`` record and the Cognito user. Catalogs are never +deleted. Audit attribution (#507): AppSync logs at ERROR level only, so these handlers are the only record that an admin operation happened at all. Every admin audit @@ -966,12 +965,12 @@ def admin_purge_user_account(event: Dict[str, Any], context: Any) -> bool: BEFORE deleting the profile rows, as the ``profileIds`` argument. The purge first verifies each of them is actually gone with a strongly consistent read — a surviving profile means the client cascade has not completed, so - it refuses with CONFLICT and deletes nothing — then deletes the Cognito - user (the commit point, #551) and sweeps the four classes the browser - cannot reach, reusing the shared deletion_cascade helpers: invites for - the account's profiles, inbound shares on other owners' profiles, S3 - report objects, and payment-QR S3 objects. Invites and reports are keyed - by profileId, which survives the deleted profile rows. + it refuses with CONFLICT and deletes nothing — then sweeps the four + classes the browser cannot reach, reusing the shared deletion_cascade + helpers: invites for the account's profiles, inbound shares on other + owners' profiles, S3 report objects, and payment-QR S3 objects. Invites + and reports are keyed by profileId, which survives the deleted profile + rows. Call this after the client's per-entity deletes, so a failure there leaves the account intact rather than half-deleted. @@ -987,7 +986,9 @@ def admin_purge_user_account(event: Dict[str, Any], context: Any) -> bool: AppError: If not admin, self-purge is attempted, the user does not exist, a supplied profile still exists (CONFLICT), or a deletion error occurs. An absent Cognito user is treated as idempotent - success. + success. When the sweep succeeded but the Cognito delete failed, + the error names the partial state so the operator knows the + account needs a retry to complete. """ logger = get_logger(__name__) @@ -1014,46 +1015,46 @@ def admin_purge_user_account(event: Dict[str, Any], context: Any) -> bool: # has not completed, so nothing is deleted and the account stays intact. _assert_profiles_deleted(account_id, profile_ids, logger, actor_sub) - # Delete the Cognito user FIRST so the Cognito call is the commit point (#551). - # A data-phase failure afterwards leaves records the user can no longer reach - # (no sign-in, so no post_authentication re-bootstrap) and a re-run of the - # data phase is safe; a Cognito failure leaves every record intact. Deleting - # data first would leave a sign-in-capable Cognito user whose records are - # gone, silently re-bootstrapped as an empty account on next sign-in. + profiles_for_sweep = [{"profileId": profile_id} for profile_id in profile_ids] + delete_invites_for_owned_profiles(profiles_for_sweep, logger) + delete_inbound_shares(account_id, logger) + delete_user_s3_reports(profiles_for_sweep, logger) + delete_all_user_qr_codes(account_id, logger) + + # The sweep runs before the Cognito delete and is safe to re-run, so a + # retry after a failure below converges; a failure here leaves the Cognito + # user untouched. When the sweep succeeded but the Cognito delete fails, + # the partial state (data swept, user still present) is logged loudly with + # an error_code-tagged field and surfaced honestly to the caller. if username: - _delete_user_from_cognito(cognito, user_pool_id, username, email or "", logger, actor_sub) + try: + _delete_user_from_cognito(cognito, user_pool_id, username, email or "", logger, actor_sub) + except Exception as e: + logger.error( + "Account data swept but the Cognito user is still present; " + "the account needs manual completion or a retry", + account_id=account_id, + error=str(e), + error_code=getattr(e, "error_code", None) or ErrorCode.INTERNAL_ERROR, + exc_info=True, + ) + raise AppError( + ErrorCode.INTERNAL_ERROR, + "Account data was deleted but the Cognito user is still present; retry the purge to complete the deletion", + ) from e else: - logger.info("Cognito user already absent; proceeding with DynamoDB cleanup", account_id=account_id, actor_sub=actor_sub) + logger.info("Cognito user already absent", account_id=account_id, actor_sub=actor_sub) - # Everything below runs after the Cognito commit: a failure here is logged - # loudly with an error_code-tagged field and re-raised, so the partial delete - # is detectable while the leftover records stay inert. - try: - profiles_for_sweep = [{"profileId": profile_id} for profile_id in profile_ids] - delete_invites_for_owned_profiles(profiles_for_sweep, logger) - delete_inbound_shares(account_id, logger) - delete_user_s3_reports(profiles_for_sweep, logger) - delete_all_user_qr_codes(account_id, logger) - - if account_exists: - db_account_id = normalize_account_id(account_id) - try: - tables.accounts.delete_item(Key={"accountId": db_account_id}) - logger.info("Deleted account record", account_id=db_account_id, actor_sub=actor_sub) - except ClientError as e: - logger.error("Failed to delete account record", error=str(e), account_id=db_account_id) - raise AppError(ErrorCode.INTERNAL_ERROR, "Failed to delete account record") from e - else: - logger.info("Account record already absent", account_id=account_id, actor_sub=actor_sub) - except Exception as e: - logger.error( - "Cognito user already deleted but user data cleanup failed; " - "leftover records are inert because the user can no longer sign in", - account_id=account_id, - error=str(e), - error_code=ErrorCode.INTERNAL_ERROR, - ) - raise + if account_exists: + db_account_id = normalize_account_id(account_id) + try: + tables.accounts.delete_item(Key={"accountId": db_account_id}) + logger.info("Deleted account record", account_id=db_account_id, actor_sub=actor_sub) + except ClientError as e: + logger.error("Failed to delete account record", error=str(e), account_id=db_account_id) + raise AppError(ErrorCode.INTERNAL_ERROR, "Failed to delete account record") from e + else: + logger.info("Account record already absent", account_id=account_id, actor_sub=actor_sub) logger.info("User account purged", account_id=account_id, email=mask_email(email), actor_sub=actor_sub) return True diff --git a/tests/unit/test_account_operations.py b/tests/unit/test_account_operations.py index cfda7359..1b1b1503 100644 --- a/tests/unit/test_account_operations.py +++ b/tests/unit/test_account_operations.py @@ -762,34 +762,36 @@ def test_delete_account_cognito_admin_delete_error( assert result["__isError"] is True assert result["errorCode"] == ErrorCode.INTERNAL_ERROR - assert result["message"] == "Failed to delete account" + assert result["message"] == ( + "Account data was deleted but the Cognito user is still present; retry to complete the deletion" + ) assert "InternalError" not in result["message"] assert "AdminDeleteUser" not in result["message"] - assert "Cognito" not in result["message"] captured = capsys.readouterr().out - assert "Cognito error during account deletion" in captured + assert "Account data swept but the Cognito user is still present" in captured + assert "INTERNAL_ERROR" in captured assert "InternalError" in captured assert "AdminDeleteUser" in captured - # The data cascade must not have run: the account record survives so a - # retry converges (#551). - assert accounts_table.get_item(Key={"accountId": account_id_key}).get("Item") is not None + # The sweep ran to completion before the Cognito failure: user data is + # gone while the Cognito user survives for the retry to remove. + assert accounts_table.get_item(Key={"accountId": account_id_key}).get("Item") is None - def test_delete_account_data_failure_after_cognito_commit( + def test_delete_account_cognito_failure_then_retry_completes( self, dynamodb_table: Any, sample_account_id: str, appsync_event: Dict[str, Any], lambda_context: Any, monkeypatch: Any, - capsys: Any, ) -> None: - """A data-phase failure after the Cognito commit is logged loudly (#551). + """A Cognito failure after a successful sweep is a partial delete; a retry completes it. - The Cognito user is already gone, so leftover records are inert and a - re-run of the data phase is safe; the partial delete must be detectable in - post-incident review via the error_code-tagged log. + First attempt: user data is swept, the Cognito delete fails, and the + caller gets the honest partial-state error. Second attempt: the sweep + no-ops over the already-deleted rows and the Cognito delete succeeds, + so the deletion completes cleanly. """ from botocore.exceptions import ClientError @@ -816,30 +818,31 @@ def test_delete_account_data_failure_after_cognito_commit( mock_cognito.list_users.return_value = { "Users": [{"Username": "test-user", "Attributes": [{"Name": "sub", "Value": sample_account_id}]}] } + mock_cognito.admin_delete_user.side_effect = [ + ClientError({"Error": {"Code": "InternalError", "Message": "Internal error"}}, "AdminDeleteUser"), + {}, + ] - with patch( - "src.handlers.account_operations._delete_all_user_data", - side_effect=ClientError( - {"Error": {"Code": "InternalServerError", "Message": "DynamoDB exploded"}}, - "DeleteItem", - ), - ): - event = { - **appsync_event, - "identity": {"sub": sample_account_id}, - } + event = { + **appsync_event, + "identity": {"sub": sample_account_id}, + } - result = delete_my_account(event, lambda_context) + first = delete_my_account(event, lambda_context) - assert result["__isError"] is True - assert result["errorCode"] == ErrorCode.INTERNAL_ERROR - # The commit point was reached: the Cognito user is gone. - mock_cognito.admin_delete_user.assert_called_once_with( - UserPoolId="us-east-1_test123", Username="test-user" - ) - captured = capsys.readouterr().out - assert "data cleanup failed" in captured - assert "INTERNAL_ERROR" in captured + assert first["__isError"] is True + assert first["errorCode"] == ErrorCode.INTERNAL_ERROR + assert first["message"] == ( + "Account data was deleted but the Cognito user is still present; retry to complete the deletion" + ) + + second = delete_my_account(event, lambda_context) + + assert second is True + assert mock_cognito.admin_delete_user.call_count == 2 + + # The retry completed the deletion: the account record is gone. + assert accounts_table.get_item(Key={"accountId": account_id_key}).get("Item") is None def test_delete_account_unexpected_exception( self, @@ -1041,10 +1044,11 @@ def test_delete_account_cognito_retry_exhausted( assert result["__isError"] is True assert result["errorCode"] == ErrorCode.INTERNAL_ERROR - assert result["message"] == "Failed to delete account" + assert result["message"] == ( + "Account data was deleted but the Cognito user is still present; retry to complete the deletion" + ) assert "TooManyRequestsException" not in result["message"] assert "AdminDeleteUser" not in result["message"] - assert "Cognito" not in result["message"] assert mock_sleep.call_count == 2 assert mock_cognito.admin_delete_user.call_count == 3 diff --git a/tests/unit/test_admin_operations.py b/tests/unit/test_admin_operations.py index 3460a684..f34dffde 100644 --- a/tests/unit/test_admin_operations.py +++ b/tests/unit/test_admin_operations.py @@ -1854,7 +1854,7 @@ def test_purge_sweeps_inbound_shares( is not None ) - def test_cognito_failure_leaves_user_data_intact( + def test_cognito_failure_after_sweep_then_retry_completes( self, dynamodb_table: Any, invites_table: Any, @@ -1862,11 +1862,13 @@ def test_cognito_failure_leaves_user_data_intact( lambda_context: Any, monkeypatch: Any, ) -> None: - """A Cognito delete failure must not sweep any user data (#551). + """A Cognito delete failure after the sweep is a partial delete; a retry completes it. - The Cognito call is the commit point: if it fails, the data phase must - never run, so the half-deleted state (Cognito user alive, residue - destroyed) that the ordering fix targets cannot occur. + First attempt: the sweep runs, the Cognito delete fails, and the caller + gets the honest partial-state error while the accounts row and the + Cognito user survive. Second attempt: the sweep no-ops over the + already-swept residue, the Cognito delete succeeds, and the accounts + record is removed. """ monkeypatch.setenv("USER_POOL_ID", "test-pool-id") monkeypatch.setenv("ACCOUNTS_TABLE_NAME", "kernelworx-accounts-ue1-dev") @@ -1879,34 +1881,42 @@ def test_cognito_failure_leaves_user_data_intact( with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: mock_cognito = self._mock_cognito(target_account_id) - mock_cognito.admin_delete_user.side_effect = ClientError( - {"Error": {"Code": "NotAuthorizedException", "Message": "Access denied."}}, - "AdminDeleteUser", - ) + mock_cognito.admin_delete_user.side_effect = [ + ClientError({"Error": {"Code": "InternalError", "Message": "Internal error"}}, "AdminDeleteUser"), + {}, + ] mock_get_client.return_value = mock_cognito - result = admin_purge_user_account(event, lambda_context) + first = admin_purge_user_account(event, lambda_context) - assert result["__isError"] is True - assert result["errorCode"] == ErrorCode.INTERNAL_ERROR - # Neither the sweep nor the account-record delete ran: the residue and - # the accounts row survive so the admin retry converges. - assert invites_table.get_item(Key={"inviteCode": "INV-OWNED"}).get("Item") is not None - assert "Item" in get_accounts_table().get_item(Key={"accountId": f"ACCOUNT#{target_account_id}"}) + assert first["__isError"] is True + assert first["errorCode"] == ErrorCode.INTERNAL_ERROR + assert first["message"] == ( + "Account data was deleted but the Cognito user is still present; " + "retry the purge to complete the deletion" + ) + # The sweep ran: the invite is gone, but the accounts row survives + # for the retry to remove. + assert invites_table.get_item(Key={"inviteCode": "INV-OWNED"}).get("Item") is None + assert "Item" in get_accounts_table().get_item(Key={"accountId": f"ACCOUNT#{target_account_id}"}) + + second = admin_purge_user_account(event, lambda_context) + + assert second is True + assert mock_cognito.admin_delete_user.call_count == 2 + assert "Item" not in get_accounts_table().get_item(Key={"accountId": f"ACCOUNT#{target_account_id}"}) - def test_data_failure_after_cognito_commit( + def test_data_sweep_failure_leaves_cognito_user_intact( self, dynamodb_table: Any, admin_appsync_event: Dict[str, Any], lambda_context: Any, monkeypatch: Any, - capsys: Any, ) -> None: - """A data-phase failure after the Cognito commit is logged loudly (#551). + """A sweep failure before the Cognito delete is not a partial delete. - The Cognito user is already gone, so leftover records are inert and a - re-run of the data phase is safe; the partial delete must be detectable - in post-incident review via the error_code-tagged log. + The Cognito user is untouched, so the retry converges once the + transient failure clears. """ monkeypatch.setenv("USER_POOL_ID", "test-pool-id") monkeypatch.setenv("ACCOUNTS_TABLE_NAME", "kernelworx-accounts-ue1-dev") @@ -1931,11 +1941,10 @@ def test_data_failure_after_cognito_commit( assert result["__isError"] is True assert result["errorCode"] == ErrorCode.INTERNAL_ERROR - # The commit point was reached: the Cognito user is gone. - mock_cognito.admin_delete_user.assert_called_once_with(UserPoolId="test-pool-id", Username=target_account_id) - captured = capsys.readouterr().out - assert "data cleanup failed" in captured - assert "INTERNAL_ERROR" in captured + assert result["message"] == "Failed to purge user account" + # The Cognito user was never touched: the delete never ran. + mock_cognito.admin_delete_user.assert_not_called() + assert "Item" in get_accounts_table().get_item(Key={"accountId": f"ACCOUNT#{target_account_id}"}) def test_purge_deletes_s3_reports_for_the_supplied_profiles( self, From 7e68c01b9fa589cd0748184ba57f4cea7cfdeae8 Mon Sep 17 00:00:00 2001 From: David Meiser Date: Thu, 1 Oct 2026 23:02:14 -0400 Subject: [PATCH 4/9] no-mistakes(document): Verified deletion docs current; reformatted one log string --- src/handlers/account_operations.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/handlers/account_operations.py b/src/handlers/account_operations.py index 976ea34c..c853665c 100644 --- a/src/handlers/account_operations.py +++ b/src/handlers/account_operations.py @@ -131,8 +131,7 @@ def delete_my_account(event: Dict[str, Any], context: Any) -> bool: _delete_user_from_cognito(cognito, user_pool_id, account_id, username, logger) except Exception as e: logger.error( - "Account data swept but the Cognito user is still present; " - "the account needs manual completion or a retry", + "Account data swept but the Cognito user is still present; the account needs manual completion or a retry", account_id=account_id, error=str(e), error_code=getattr(e, "error_code", None) or ErrorCode.INTERNAL_ERROR, From e07c226da2647b50de2ef69c09e1a31396ed3e70 Mon Sep 17 00:00:00 2001 From: David Meiser Date: Sat, 3 Oct 2026 08:08:27 -0400 Subject: [PATCH 5/9] fix(handlers): report unknown Cognito state instead of asserting the 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. --- src/handlers/account_operations.py | 10 +++- src/handlers/admin_operations.py | 15 +++--- tests/unit/test_account_operations.py | 75 +++++++++++++++++++++++++-- tests/unit/test_admin_operations.py | 53 ++++++++++++++++++- 4 files changed, 140 insertions(+), 13 deletions(-) diff --git a/src/handlers/account_operations.py b/src/handlers/account_operations.py index c853665c..ac68a067 100644 --- a/src/handlers/account_operations.py +++ b/src/handlers/account_operations.py @@ -130,8 +130,13 @@ def delete_my_account(event: Dict[str, Any], context: Any) -> bool: try: _delete_user_from_cognito(cognito, user_pool_id, account_id, username, logger) except Exception as e: + # Only two facts are established here: the sweep completed and the + # Cognito delete did not report success. Whether the user survives is + # unknown (the delete may have landed, and a failed re-lookup of an + # absent user also lands here), so the message says exactly that. logger.error( - "Account data swept but the Cognito user is still present; the account needs manual completion or a retry", + "Account data swept but the Cognito delete did not report success; " + "the account's Cognito state is unknown and needs a retry or manual completion", account_id=account_id, error=str(e), error_code=getattr(e, "error_code", None) or ErrorCode.INTERNAL_ERROR, @@ -139,7 +144,8 @@ def delete_my_account(event: Dict[str, Any], context: Any) -> bool: ) raise AppError( ErrorCode.INTERNAL_ERROR, - "Account data was deleted but the Cognito user is still present; retry to complete the deletion", + "Account data was deleted but the Cognito user could not be confirmed deleted; " + "retry to complete the deletion", ) from e logger.info("Account deletion completed successfully") diff --git a/src/handlers/admin_operations.py b/src/handlers/admin_operations.py index 0e140aeb..88933db5 100644 --- a/src/handlers/admin_operations.py +++ b/src/handlers/admin_operations.py @@ -1023,16 +1023,18 @@ def admin_purge_user_account(event: Dict[str, Any], context: Any) -> bool: # The sweep runs before the Cognito delete and is safe to re-run, so a # retry after a failure below converges; a failure here leaves the Cognito - # user untouched. When the sweep succeeded but the Cognito delete fails, - # the partial state (data swept, user still present) is logged loudly with - # an error_code-tagged field and surfaced honestly to the caller. + # user untouched. When the sweep succeeded but the Cognito delete does not + # report success, the partial state is logged loudly with an error_code + # field and surfaced honestly to the caller: the Cognito user's state is + # unknown, not asserted to survive, because the delete may have landed and + # still raised afterwards. if username: try: _delete_user_from_cognito(cognito, user_pool_id, username, email or "", logger, actor_sub) except Exception as e: logger.error( - "Account data swept but the Cognito user is still present; " - "the account needs manual completion or a retry", + "Account data swept but the Cognito delete did not report success; " + "the account's Cognito state is unknown and needs a retry or manual completion", account_id=account_id, error=str(e), error_code=getattr(e, "error_code", None) or ErrorCode.INTERNAL_ERROR, @@ -1040,7 +1042,8 @@ def admin_purge_user_account(event: Dict[str, Any], context: Any) -> bool: ) raise AppError( ErrorCode.INTERNAL_ERROR, - "Account data was deleted but the Cognito user is still present; retry the purge to complete the deletion", + "Account data was deleted but the Cognito user could not be confirmed deleted; " + "retry the purge to complete the deletion", ) from e else: logger.info("Cognito user already absent", account_id=account_id, actor_sub=actor_sub) diff --git a/tests/unit/test_account_operations.py b/tests/unit/test_account_operations.py index 1b1b1503..13f85957 100644 --- a/tests/unit/test_account_operations.py +++ b/tests/unit/test_account_operations.py @@ -763,13 +763,14 @@ def test_delete_account_cognito_admin_delete_error( assert result["__isError"] is True assert result["errorCode"] == ErrorCode.INTERNAL_ERROR assert result["message"] == ( - "Account data was deleted but the Cognito user is still present; retry to complete the deletion" + "Account data was deleted but the Cognito user could not be confirmed deleted; " + "retry to complete the deletion" ) assert "InternalError" not in result["message"] assert "AdminDeleteUser" not in result["message"] captured = capsys.readouterr().out - assert "Account data swept but the Cognito user is still present" in captured + assert "Account data swept but the Cognito delete did not report success" in captured assert "INTERNAL_ERROR" in captured assert "InternalError" in captured assert "AdminDeleteUser" in captured @@ -778,6 +779,70 @@ def test_delete_account_cognito_admin_delete_error( # gone while the Cognito user survives for the retry to remove. assert accounts_table.get_item(Key={"accountId": account_id_key}).get("Item") is None + def test_missing_username_relookup_failure_does_not_claim_user_present( + self, + dynamodb_table: Any, + sample_account_id: str, + appsync_event: Dict[str, Any], + lambda_context: Any, + monkeypatch: Any, + ) -> None: + """A failed re-lookup after an empty pre-check must not assert the user survived. + + When the pre-check found no username, the delete path re-runs the + lookup. A failure there says nothing about whether the Cognito user + exists, so the message may only report that the delete did not report + success - never that the user is still present. + """ + from botocore.exceptions import ClientError + + from src.handlers.account_operations import delete_my_account + + monkeypatch.setenv("ACCOUNTS_TABLE_NAME", "kernelworx-accounts-ue1-dev") + monkeypatch.setenv("USER_POOL_ID", "us-east-1_test123") + + dynamodb = boto3.resource("dynamodb", region_name="us-east-1") + accounts_table = dynamodb.Table("kernelworx-accounts-ue1-dev") + account_id_key = f"ACCOUNT#{sample_account_id}" + accounts_table.put_item( + Item={ + "accountId": account_id_key, + "email": "test@example.com", + "createdAt": datetime.now(timezone.utc).isoformat(), + } + ) + + with patch("boto3.client") as mock_boto_client: + mock_cognito = MagicMock() + mock_boto_client.return_value = mock_cognito + # Pre-check finds no user; the delete path's re-lookup then fails. + mock_cognito.list_users.side_effect = [ + {"Users": []}, + ClientError({"Error": {"Code": "AccessDenied", "Message": "Access Denied"}}, "ListUsers"), + ] + + event = { + **appsync_event, + "identity": {"sub": sample_account_id}, + } + + result = delete_my_account(event, lambda_context) + + assert result["__isError"] is True + assert result["errorCode"] == ErrorCode.INTERNAL_ERROR + assert result["message"] == ( + "Account data was deleted but the Cognito user could not be confirmed deleted; " + "retry to complete the deletion" + ) + # The handler never established that the user exists, so it must + # not report survival as fact. + assert "still present" not in result["message"] + assert mock_cognito.list_users.call_count == 2 + assert mock_cognito.admin_delete_user.call_count == 0 + + # The sweep ran to completion before the failed re-lookup. + assert accounts_table.get_item(Key={"accountId": account_id_key}).get("Item") is None + def test_delete_account_cognito_failure_then_retry_completes( self, dynamodb_table: Any, @@ -833,7 +898,8 @@ def test_delete_account_cognito_failure_then_retry_completes( assert first["__isError"] is True assert first["errorCode"] == ErrorCode.INTERNAL_ERROR assert first["message"] == ( - "Account data was deleted but the Cognito user is still present; retry to complete the deletion" + "Account data was deleted but the Cognito user could not be confirmed deleted; " + "retry to complete the deletion" ) second = delete_my_account(event, lambda_context) @@ -1045,7 +1111,8 @@ def test_delete_account_cognito_retry_exhausted( assert result["__isError"] is True assert result["errorCode"] == ErrorCode.INTERNAL_ERROR assert result["message"] == ( - "Account data was deleted but the Cognito user is still present; retry to complete the deletion" + "Account data was deleted but the Cognito user could not be confirmed deleted; " + "retry to complete the deletion" ) assert "TooManyRequestsException" not in result["message"] assert "AdminDeleteUser" not in result["message"] diff --git a/tests/unit/test_admin_operations.py b/tests/unit/test_admin_operations.py index f34dffde..16216f89 100644 --- a/tests/unit/test_admin_operations.py +++ b/tests/unit/test_admin_operations.py @@ -1892,7 +1892,7 @@ def test_cognito_failure_after_sweep_then_retry_completes( assert first["__isError"] is True assert first["errorCode"] == ErrorCode.INTERNAL_ERROR assert first["message"] == ( - "Account data was deleted but the Cognito user is still present; " + "Account data was deleted but the Cognito user could not be confirmed deleted; " "retry the purge to complete the deletion" ) # The sweep ran: the invite is gone, but the accounts row survives @@ -1906,6 +1906,57 @@ def test_cognito_failure_after_sweep_then_retry_completes( assert mock_cognito.admin_delete_user.call_count == 2 assert "Item" not in get_accounts_table().get_item(Key={"accountId": f"ACCOUNT#{target_account_id}"}) + def test_cognito_delete_raising_after_success_does_not_claim_user_present( + self, + dynamodb_table: Any, + invites_table: Any, + admin_appsync_event: Dict[str, Any], + lambda_context: Any, + monkeypatch: Any, + ) -> None: + """A failure raised after AdminDeleteUser landed must not assert the user survived. + + A transport-level failure can surface after the delete request has + already reached Cognito, so the handler knows only that the delete did + not report success. Reporting the user as still present would state a + fact the purge never checked. + """ + monkeypatch.setenv("USER_POOL_ID", "test-pool-id") + monkeypatch.setenv("ACCOUNTS_TABLE_NAME", "kernelworx-accounts-ue1-dev") + + target_account_id = "target-user-123" + self._seed_account(target_account_id) + invites_table.put_item(Item={"inviteCode": "INV-OWNED", "profileId": "PROFILE#p1", "status": "PENDING"}) + + event = {**admin_appsync_event, "arguments": {"accountId": target_account_id, "profileIds": ["PROFILE#p1"]}} + + with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + mock_cognito = self._mock_cognito(target_account_id) + + def _delete_then_fail(**kwargs: Any) -> Dict[str, Any]: + # The delete call itself is issued; the failure arrives on the + # way back, so the user's state is genuinely unknown. + raise ConnectionResetError("connection reset after the request was sent") + + mock_cognito.admin_delete_user.side_effect = _delete_then_fail + mock_get_client.return_value = mock_cognito + + result = admin_purge_user_account(event, lambda_context) + + assert result["__isError"] is True + assert result["errorCode"] == ErrorCode.INTERNAL_ERROR + assert result["message"] == ( + "Account data was deleted but the Cognito user could not be confirmed deleted; " + "retry the purge to complete the deletion" + ) + # The purge never re-read the user, so it must not claim survival. + assert "still present" not in result["message"] + assert mock_cognito.admin_delete_user.call_count == 1 + + # The sweep ran and the accounts row survives, so a retry converges. + assert invites_table.get_item(Key={"inviteCode": "INV-OWNED"}).get("Item") is None + assert "Item" in get_accounts_table().get_item(Key={"accountId": f"ACCOUNT#{target_account_id}"}) + def test_data_sweep_failure_leaves_cognito_user_intact( self, dynamodb_table: Any, From 742ff9a457bcba55c3aec4aadc578bede06a633b Mon Sep 17 00:00:00 2001 From: David Meiser Date: Sat, 3 Oct 2026 11:57:45 -0400 Subject: [PATCH 6/9] fix(handlers): narrow account-deletion catches and attribute the final 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. --- src/handlers/account_operations.py | 26 ++-- src/handlers/admin_operations.py | 13 +- src/handlers/deletion_cascade.py | 49 +++++-- tests/unit/test_account_deletion_cascade.py | 59 ++++++++- tests/unit/test_account_operations.py | 137 +++++++++++++++++++- tests/unit/test_admin_operations.py | 17 ++- 6 files changed, 265 insertions(+), 36 deletions(-) diff --git a/src/handlers/account_operations.py b/src/handlers/account_operations.py index ac68a067..a8eec53a 100644 --- a/src/handlers/account_operations.py +++ b/src/handlers/account_operations.py @@ -8,7 +8,7 @@ from typing import TYPE_CHECKING, Any, Dict import boto3 -from botocore.exceptions import ClientError +from botocore.exceptions import BotoCoreError, ClientError # Sibling handler modules use a same-package relative import, which resolves both # in the Lambda zip (package `handlers`) and in unit tests (package `src.handlers`). @@ -19,12 +19,14 @@ from utils.appsync_types import get_caller_id from utils.cognito import retry_on_transient_errors from utils.cognito_filters import cognito_user_filter + from utils.dynamodb import is_transient_client_error from utils.errors import AppError, ErrorCode from utils.logging import get_logger except ModuleNotFoundError: # pragma: no cover from ..utils.appsync_types import get_caller_id from ..utils.cognito import retry_on_transient_errors from ..utils.cognito_filters import cognito_user_filter + from ..utils.dynamodb import is_transient_client_error from ..utils.errors import AppError, ErrorCode from ..utils.logging import get_logger @@ -129,21 +131,29 @@ def delete_my_account(event: Dict[str, Any], context: Any) -> bool: try: _delete_user_from_cognito(cognito, user_pool_id, account_id, username, logger) - except Exception as e: - # Only two facts are established here: the sweep completed and the - # Cognito delete did not report success. Whether the user survives is - # unknown (the delete may have landed, and a failed re-lookup of an - # absent user also lands here), so the message says exactly that. + except (ClientError, BotoCoreError, AppError) as e: + # A typed AppError from the Cognito helper (or a future RESOURCE_BUSY) + # propagates with its own code and message intact; only a raw + # botocore fault is attributed here. BotoCoreError stays in the catch + # because a transport failure after the request was sent is precisely + # the unknown-state case below. Only two facts are then established: + # the sweep completed and the Cognito delete did not report success. + # Whether the user survives is unknown (the delete may have landed, + # and a failed re-lookup of an absent user also lands here), so the + # message says exactly that. + if isinstance(e, AppError): + raise + error_code = ErrorCode.RESOURCE_BUSY if is_transient_client_error(e) else ErrorCode.INTERNAL_ERROR logger.error( "Account data swept but the Cognito delete did not report success; " "the account's Cognito state is unknown and needs a retry or manual completion", account_id=account_id, error=str(e), - error_code=getattr(e, "error_code", None) or ErrorCode.INTERNAL_ERROR, + error_code=error_code, exc_info=True, ) raise AppError( - ErrorCode.INTERNAL_ERROR, + error_code, "Account data was deleted but the Cognito user could not be confirmed deleted; " "retry to complete the deletion", ) from e diff --git a/src/handlers/admin_operations.py b/src/handlers/admin_operations.py index 88933db5..221caed2 100644 --- a/src/handlers/admin_operations.py +++ b/src/handlers/admin_operations.py @@ -33,7 +33,7 @@ from decimal import Decimal, InvalidOperation from typing import TYPE_CHECKING, Any, Callable, Dict, NoReturn, Optional, cast -from botocore.exceptions import ClientError +from botocore.exceptions import BotoCoreError, ClientError # Sibling handler modules use a same-package relative import, which resolves both # in the Lambda zip (package `handlers`) and in unit tests (package `src.handlers`). @@ -1031,13 +1031,20 @@ def admin_purge_user_account(event: Dict[str, Any], context: Any) -> bool: if username: try: _delete_user_from_cognito(cognito, user_pool_id, username, email or "", logger, actor_sub) - except Exception as e: + except (ClientError, BotoCoreError, AppError) as e: + # A typed AppError from the Cognito helper propagates with its own + # code and message intact; only a raw botocore fault is attributed + # here. BotoCoreError stays in the catch because a transport + # failure after the request was sent is precisely the + # unknown-state case below. + if isinstance(e, AppError): + raise logger.error( "Account data swept but the Cognito delete did not report success; " "the account's Cognito state is unknown and needs a retry or manual completion", account_id=account_id, error=str(e), - error_code=getattr(e, "error_code", None) or ErrorCode.INTERNAL_ERROR, + error_code=ErrorCode.INTERNAL_ERROR, exc_info=True, ) raise AppError( diff --git a/src/handlers/deletion_cascade.py b/src/handlers/deletion_cascade.py index 7d92d4ce..2066cecc 100644 --- a/src/handlers/deletion_cascade.py +++ b/src/handlers/deletion_cascade.py @@ -32,11 +32,13 @@ # Handle both Lambda (absolute) and unit test (relative) imports try: # pragma: no cover - from utils.dynamodb import tables + from utils.dynamodb import is_transient_client_error, tables + from utils.errors import AppError, ErrorCode from utils.logging import get_logger from utils.payment_methods import delete_all_user_qr_codes except ModuleNotFoundError: # pragma: no cover - from ..utils.dynamodb import tables + from ..utils.dynamodb import is_transient_client_error, tables + from ..utils.errors import AppError, ErrorCode from ..utils.logging import get_logger from ..utils.payment_methods import delete_all_user_qr_codes @@ -252,22 +254,41 @@ def delete_all_user_data(account_id: str, logger: Any = None) -> None: profiles = get_user_profiles(normalize_account_id(account_id)) - delete_user_orders(profiles, log) - delete_user_campaigns(profiles, log) - delete_user_shares(profiles, log) - delete_invites_for_owned_profiles(profiles, log) - delete_inbound_shares(account_id, log) - delete_user_s3_reports(profiles, log) - delete_user_profiles(account_id, profiles, log) - # Catalogs are preserved per product design and should never be deleted. - # Delete payment method QR codes from S3 per captain decision - delete_all_user_qr_codes(account_id, log) + swept = { + "orders": delete_user_orders(profiles, log), + "campaigns": delete_user_campaigns(profiles, log), + "shares": delete_user_shares(profiles, log), + "invites": delete_invites_for_owned_profiles(profiles, log), + "inbound_shares": delete_inbound_shares(account_id, log), + "s3_reports": delete_user_s3_reports(profiles, log), + "profiles": delete_user_profiles(account_id, profiles, log), + # Catalogs are preserved per product design and should never be deleted. + # Delete payment method QR codes from S3 per captain decision + "qr_codes": delete_all_user_qr_codes(account_id, log), + } account_id_key = normalize_account_id(account_id) try: tables.accounts.delete_item(Key={"accountId": account_id_key}) log.info("Deleted account from DynamoDB", account_id=account_id_key) except ClientError as e: - log.error("Failed to delete account from DynamoDB", error=str(e), account_id=account_id_key) - raise + # Every sub-cascade above completed, so this is the most likely partial + # state of the whole cascade: all user data is gone but the account + # record — and the Cognito sign-in that outlives it in the self-service + # flow — survives. Say exactly that; a throttled failure is retryable. + error_code = ErrorCode.RESOURCE_BUSY if is_transient_client_error(e) else ErrorCode.INTERNAL_ERROR + log.error( + "All user data was swept but the account record could not be deleted; " + "the account record and Cognito sign-in survive and need a retry or manual completion", + account_id=account_id_key, + swept=swept, + error=str(e), + error_code=error_code, + exc_info=True, + ) + raise AppError( + error_code, + "All user data was deleted but the account record could not be removed, " + "so the sign-in remains active; retry to complete the deletion", + ) from e log.info("Deleted all user data from DynamoDB") diff --git a/tests/unit/test_account_deletion_cascade.py b/tests/unit/test_account_deletion_cascade.py index 9cf55e3a..363744ac 100644 --- a/tests/unit/test_account_deletion_cascade.py +++ b/tests/unit/test_account_deletion_cascade.py @@ -22,6 +22,8 @@ import botocore.client import pytest +from src.utils.errors import AppError, ErrorCode + ACCOUNTS_TABLE = "kernelworx-accounts-ue1-dev" PROFILES_TABLE = "kernelworx-profiles-v2-ue1-dev" CAMPAIGNS_TABLE = "kernelworx-campaigns-v2-ue1-dev" @@ -149,12 +151,16 @@ def _seed_s3_data() -> None: def _assert_everything_deleted() -> None: + _assert_all_user_data_deleted() + assert "Item" not in _table(ACCOUNTS_TABLE).get_item(Key={"accountId": OWNER_ACCOUNT_KEY}) + + +def _assert_all_user_data_deleted() -> None: assert _table(PROFILES_TABLE).scan()["Count"] == 0 assert _table(CAMPAIGNS_TABLE).scan()["Count"] == 0 assert _table(ORDERS_TABLE).scan()["Count"] == 0 assert _table(SHARES_TABLE).scan()["Count"] == 0 assert _table(INVITES_TABLE).scan()["Count"] == 0 - assert "Item" not in _table(ACCOUNTS_TABLE).get_item(Key={"accountId": OWNER_ACCOUNT_KEY}) class TestDeleteAllUserDataProfileSweep: @@ -207,6 +213,57 @@ def test_delete_all_user_data_removes_s3_reports_and_qr_codes( assert not [key for key in keys if key.startswith("payment-qr-codes/")] +class TestFinalAccountRecordDelete: + """The accounts-row delete is the cascade's most likely partial state.""" + + def _run_with_failing_account_delete(self, monkeypatch: pytest.MonkeyPatch, error_code: str) -> Any: + """Run the full cascade with the final accounts-row delete forced to fail.""" + from botocore.exceptions import ClientError + + from src.handlers.deletion_cascade import delete_all_user_data + from src.utils.dynamodb import tables + + _seed_user_data() + _seed_s3_data() + + def fail_delete_item(*args: Any, **kwargs: Any) -> Any: + raise ClientError({"Error": {"Code": error_code, "Message": "final delete failed"}}, "DeleteItem") + + monkeypatch.setattr(tables.accounts, "delete_item", fail_delete_item) + + with pytest.raises(AppError) as excinfo: + delete_all_user_data(OWNER_SUB) + return excinfo.value + + def test_throttled_final_delete_is_retryable_and_names_survivors( + self, dynamodb_table: Any, monkeypatch: pytest.MonkeyPatch + ) -> None: + """A throttled final delete surfaces as retryable RESOURCE_BUSY, not a generic error.""" + error = self._run_with_failing_account_delete(monkeypatch, "ThrottlingException") + + assert error.error_code == ErrorCode.RESOURCE_BUSY + assert error.message == ( + "All user data was deleted but the account record could not be removed, " + "so the sign-in remains active; retry to complete the deletion" + ) + + def test_final_delete_failure_names_swept_and_surviving_data( + self, dynamodb_table: Any, monkeypatch: pytest.MonkeyPatch + ) -> None: + """A permanent final-delete failure is INTERNAL_ERROR with the same honest attribution.""" + error = self._run_with_failing_account_delete(monkeypatch, "AccessDeniedException") + + assert error.error_code == ErrorCode.INTERNAL_ERROR + assert error.message == ( + "All user data was deleted but the account record could not be removed, " + "so the sign-in remains active; retry to complete the deletion" + ) + # The sweep completed before the failure: every user datum is gone and + # only the account record survives. + _assert_all_user_data_deleted() + assert "Item" in _table(ACCOUNTS_TABLE).get_item(Key={"accountId": OWNER_ACCOUNT_KEY}) + + class TestCascadeModuleGraph: """The cascade is a leaf of the handler import graph, not a cycle participant.""" diff --git a/tests/unit/test_account_operations.py b/tests/unit/test_account_operations.py index 13f85957..f6bfb4dd 100644 --- a/tests/unit/test_account_operations.py +++ b/tests/unit/test_account_operations.py @@ -910,6 +910,128 @@ def test_delete_account_cognito_failure_then_retry_completes( # The retry completed the deletion: the account record is gone. assert accounts_table.get_item(Key={"accountId": account_id_key}).get("Item") is None + def test_typed_app_error_from_cognito_helper_passes_through( + self, + dynamodb_table: Any, + sample_account_id: str, + appsync_event: Dict[str, Any], + lambda_context: Any, + monkeypatch: Any, + ) -> None: + """An AppError from the Cognito helper keeps its own code and message. + + The narrowed catch must not rewrap typed errors: a future retryable + RESOURCE_BUSY from the helper (issue #291) reaches the caller intact. + """ + from src.handlers import account_operations + from src.handlers.account_operations import delete_my_account + from src.utils.errors import AppError + + monkeypatch.setenv("ACCOUNTS_TABLE_NAME", "kernelworx-accounts-ue1-dev") + monkeypatch.setenv("USER_POOL_ID", "us-east-1_test123") + + dynamodb = boto3.resource("dynamodb", region_name="us-east-1") + accounts_table = dynamodb.Table("kernelworx-accounts-ue1-dev") + account_id_key = f"ACCOUNT#{sample_account_id}" + + accounts_table.put_item( + Item={ + "accountId": account_id_key, + "email": "test@example.com", + "createdAt": datetime.now(timezone.utc).isoformat(), + } + ) + + with patch("boto3.client") as mock_boto_client: + mock_cognito = MagicMock() + mock_boto_client.return_value = mock_cognito + mock_cognito.list_users.return_value = { + "Users": [{"Username": "test-user", "Attributes": [{"Name": "sub", "Value": sample_account_id}]}] + } + + def typed_failure(*args: Any, **kwargs: Any) -> None: + raise AppError(ErrorCode.RESOURCE_BUSY, "Cognito is throttled; retry") + + monkeypatch.setattr(account_operations, "_delete_user_from_cognito", typed_failure) + + event = { + **appsync_event, + "identity": {"sub": sample_account_id}, + } + + result = delete_my_account(event, lambda_context) + + assert result["__isError"] is True + assert result["errorCode"] == ErrorCode.RESOURCE_BUSY + assert result["message"] == "Cognito is throttled; retry" + + # The sweep ran to completion before the typed failure. + assert accounts_table.get_item(Key={"accountId": account_id_key}).get("Item") is None + + def test_final_accounts_row_delete_failure_is_honest( + self, + dynamodb_table: Any, + sample_account_id: str, + appsync_event: Dict[str, Any], + lambda_context: Any, + monkeypatch: Any, + ) -> None: + """A failure of the cascade's final accounts-row delete names the partial state. + + All eight sub-cascades completed, so the message must say the user data + is gone and the account record (and its sign-in) survived; a throttled + failure is retryable. + """ + from botocore.exceptions import ClientError + + from src.handlers.account_operations import delete_my_account + from src.utils.dynamodb import tables + + monkeypatch.setenv("ACCOUNTS_TABLE_NAME", "kernelworx-accounts-ue1-dev") + monkeypatch.setenv("USER_POOL_ID", "us-east-1_test123") + + dynamodb = boto3.resource("dynamodb", region_name="us-east-1") + accounts_table = dynamodb.Table("kernelworx-accounts-ue1-dev") + account_id_key = f"ACCOUNT#{sample_account_id}" + + accounts_table.put_item( + Item={ + "accountId": account_id_key, + "email": "test@example.com", + "createdAt": datetime.now(timezone.utc).isoformat(), + } + ) + + def fail_delete_item(*args: Any, **kwargs: Any) -> Any: + raise ClientError( + {"Error": {"Code": "ProvisionedThroughputExceededException", "Message": "throttled"}}, + "DeleteItem", + ) + + monkeypatch.setattr(tables.accounts, "delete_item", fail_delete_item) + + with patch("boto3.client") as mock_boto_client: + mock_cognito = MagicMock() + mock_boto_client.return_value = mock_cognito + mock_cognito.list_users.return_value = {"Users": []} + + event = { + **appsync_event, + "identity": {"sub": sample_account_id}, + } + + result = delete_my_account(event, lambda_context) + + assert result["__isError"] is True + assert result["errorCode"] == ErrorCode.RESOURCE_BUSY + assert result["message"] == ( + "All user data was deleted but the account record could not be removed, " + "so the sign-in remains active; retry to complete the deletion" + ) + + # The account record survived the failed final delete. + assert accounts_table.get_item(Key={"accountId": account_id_key}).get("Item") is not None + def test_delete_account_unexpected_exception( self, dynamodb_table: Any, @@ -1065,7 +1187,7 @@ def test_delete_account_cognito_retry_exhausted( lambda_context: Any, monkeypatch: Any, ) -> None: - """Test transient Cognito errors that exhaust retries raise AppError.""" + """Throttled Cognito errors that exhaust retries surface as retryable RESOURCE_BUSY (#291).""" from botocore.exceptions import ClientError from src.handlers.account_operations import delete_my_account @@ -1109,7 +1231,7 @@ def test_delete_account_cognito_retry_exhausted( result = delete_my_account(event, lambda_context) assert result["__isError"] is True - assert result["errorCode"] == ErrorCode.INTERNAL_ERROR + assert result["errorCode"] == ErrorCode.RESOURCE_BUSY assert result["message"] == ( "Account data was deleted but the Cognito user could not be confirmed deleted; " "retry to complete the deletion" @@ -1401,15 +1523,22 @@ def test_delete_all_user_data_prefixed(self, dynamodb_table: Any, s3_bucket: Any assert "Item" not in response def test_delete_all_user_data_client_error(self, dynamodb_table: Any, s3_bucket: Any) -> None: - """delete_all_user_data propagates ClientError on account delete.""" + """A permanent final accounts-row delete failure raises the honest AppError.""" from botocore.exceptions import ClientError from src.handlers.deletion_cascade import delete_all_user_data + from src.utils.errors import AppError with patch("src.handlers.deletion_cascade.tables.accounts.delete_item") as mock_delete: mock_delete.side_effect = ClientError( {"Error": {"Code": "ResourceNotFoundException", "Message": "Table not found"}}, "DeleteItem", ) - with pytest.raises(ClientError): + with pytest.raises(AppError) as excinfo: delete_all_user_data("test-user") + + assert excinfo.value.error_code == ErrorCode.INTERNAL_ERROR + assert excinfo.value.message == ( + "All user data was deleted but the account record could not be removed, " + "so the sign-in remains active; retry to complete the deletion" + ) diff --git a/tests/unit/test_admin_operations.py b/tests/unit/test_admin_operations.py index 16216f89..a48230ed 100644 --- a/tests/unit/test_admin_operations.py +++ b/tests/unit/test_admin_operations.py @@ -1890,11 +1890,11 @@ def test_cognito_failure_after_sweep_then_retry_completes( first = admin_purge_user_account(event, lambda_context) assert first["__isError"] is True + # The helper raises a typed AppError for the Cognito ClientError; + # the narrowed catch lets it pass through with its own message + # instead of rewrapping it into the partial-state wording. assert first["errorCode"] == ErrorCode.INTERNAL_ERROR - assert first["message"] == ( - "Account data was deleted but the Cognito user could not be confirmed deleted; " - "retry the purge to complete the deletion" - ) + assert first["message"] == "Failed to delete user from Cognito" # The sweep ran: the invite is gone, but the accounts row survives # for the retry to remove. assert invites_table.get_item(Key={"inviteCode": "INV-OWNED"}).get("Item") is None @@ -1935,8 +1935,13 @@ def test_cognito_delete_raising_after_success_does_not_claim_user_present( def _delete_then_fail(**kwargs: Any) -> Dict[str, Any]: # The delete call itself is issued; the failure arrives on the - # way back, so the user's state is genuinely unknown. - raise ConnectionResetError("connection reset after the request was sent") + # way back, so the user's state is genuinely unknown. Botocore + # raises ConnectionError for this class of transport failure. + from botocore.exceptions import ConnectionError as BotoConnectionError + + raise BotoConnectionError( + error="connection reset after the request was sent", operation_name="AdminDeleteUser" + ) mock_cognito.admin_delete_user.side_effect = _delete_then_fail mock_get_client.return_value = mock_cognito From abb39eec9aa8f785308aa667c0ac969431dcbafe Mon Sep 17 00:00:00 2001 From: David Meiser Date: Sat, 3 Oct 2026 12:29:27 -0400 Subject: [PATCH 7/9] no-mistakes(review): Type throttled admin Cognito delete as RESOURCE_BUSY; widen pre-check catch --- src/handlers/account_operations.py | 4 +- src/handlers/admin_operations.py | 5 +++ tests/unit/test_account_operations.py | 59 +++++++++++++++++++++++++++ tests/unit/test_admin_operations.py | 31 ++++++++------ 4 files changed, 85 insertions(+), 14 deletions(-) diff --git a/src/handlers/account_operations.py b/src/handlers/account_operations.py index a8eec53a..636e1885 100644 --- a/src/handlers/account_operations.py +++ b/src/handlers/account_operations.py @@ -120,9 +120,9 @@ def delete_my_account(event: Dict[str, Any], context: Any) -> bool: username = _lookup_cognito_user_with_retry(cognito, user_pool_id, account_id) if not username: logger.warning(f"User not found in Cognito with sub: {account_id}") - except ClientError as e: + except (ClientError, BotoCoreError) as e: logger.error("Cognito lookup failed before deletion", account_id=account_id, error=str(e), exc_info=True) - raise AppError(ErrorCode.INTERNAL_ERROR, "Failed to delete account") + raise AppError(ErrorCode.INTERNAL_ERROR, "Failed to delete account") from e # The data sweep runs before the Cognito delete and is safe to re-run, so a # retry after any failure below converges; a sweep failure here leaves the diff --git a/src/handlers/admin_operations.py b/src/handlers/admin_operations.py index 221caed2..deb4bb46 100644 --- a/src/handlers/admin_operations.py +++ b/src/handlers/admin_operations.py @@ -838,6 +838,11 @@ def _delete_user_from_cognito( actor_sub=actor_sub, ) return + if error_code in _THROTTLING_ERROR_CODES: + logger.warning("Cognito admin_delete_user throttled", error=str(e), error_code=error_code) + # Retryable at the purge boundary, which re-raises the typed + # error with its code intact. + raise AppError(ErrorCode.RESOURCE_BUSY, "Failed to delete user from Cognito") from e logger.error("Cognito admin_delete_user failed", error=str(e), error_code=error_code) raise AppError(ErrorCode.INTERNAL_ERROR, "Failed to delete user from Cognito") from e diff --git a/tests/unit/test_account_operations.py b/tests/unit/test_account_operations.py index f6bfb4dd..b65c72b0 100644 --- a/tests/unit/test_account_operations.py +++ b/tests/unit/test_account_operations.py @@ -702,6 +702,65 @@ def test_delete_account_cognito_client_error( # Account should NOT be deleted from DynamoDB if pre-check fails assert accounts_table.get_item(Key={"accountId": account_id_key}).get("Item") is not None + def test_delete_account_cognito_transport_error( + self, + dynamodb_table: Any, + sample_account_id: str, + appsync_event: Dict[str, Any], + lambda_context: Any, + monkeypatch: Any, + capsys: Any, + ) -> None: + """A transport-level lookup failure at the pre-check surfaces as the same typed INTERNAL_ERROR. + + The pre-check catch covers the same transport class as the post-sweep + Cognito-delete block: a BotoCoreError (e.g. a connection failure) + must not escape to the decorator's far-generic wording. + """ + from botocore.exceptions import ConnectionError as BotoConnectionError + + from src.handlers.account_operations import delete_my_account + + monkeypatch.setenv("ACCOUNTS_TABLE_NAME", "kernelworx-accounts-ue1-dev") + monkeypatch.setenv("USER_POOL_ID", "us-east-1_test123") + + dynamodb = boto3.resource("dynamodb", region_name="us-east-1") + accounts_table = dynamodb.Table("kernelworx-accounts-ue1-dev") + + account_id_key = f"ACCOUNT#{sample_account_id}" + + accounts_table.put_item( + Item={ + "accountId": account_id_key, + "email": "test@example.com", + "createdAt": datetime.now(timezone.utc).isoformat(), + "updatedAt": datetime.now(timezone.utc).isoformat(), + } + ) + + with patch("boto3.client") as mock_boto_client: + mock_cognito = MagicMock() + mock_boto_client.return_value = mock_cognito + mock_cognito.list_users.side_effect = BotoConnectionError( + error="connection reset", operation_name="ListUsers" + ) + + event = { + **appsync_event, + "identity": {"sub": sample_account_id}, + } + + result = delete_my_account(event, lambda_context) + + assert result["__isError"] is True + assert result["errorCode"] == ErrorCode.INTERNAL_ERROR + assert result["message"] == "Failed to delete account" + + captured = capsys.readouterr().out + assert "Cognito lookup failed before deletion" in captured + # The pre-check runs before any deletion, so the account row survives. + assert accounts_table.get_item(Key={"accountId": account_id_key}).get("Item") is not None + def test_delete_account_cognito_admin_delete_error( self, dynamodb_table: Any, diff --git a/tests/unit/test_admin_operations.py b/tests/unit/test_admin_operations.py index a48230ed..c32de050 100644 --- a/tests/unit/test_admin_operations.py +++ b/tests/unit/test_admin_operations.py @@ -1862,11 +1862,11 @@ def test_cognito_failure_after_sweep_then_retry_completes( lambda_context: Any, monkeypatch: Any, ) -> None: - """A Cognito delete failure after the sweep is a partial delete; a retry completes it. + """A throttled Cognito delete after the sweep is retryable; a retry completes it. - First attempt: the sweep runs, the Cognito delete fails, and the caller - gets the honest partial-state error while the accounts row and the - Cognito user survive. Second attempt: the sweep no-ops over the + First attempt: the sweep runs, the Cognito delete is throttled, and the + caller gets the mandated retryable RESOURCE_BUSY while the accounts row + and the Cognito user survive. Second attempt: the sweep no-ops over the already-swept residue, the Cognito delete succeeds, and the accounts record is removed. """ @@ -1881,25 +1881,32 @@ def test_cognito_failure_after_sweep_then_retry_completes( with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: mock_cognito = self._mock_cognito(target_account_id) - mock_cognito.admin_delete_user.side_effect = [ - ClientError({"Error": {"Code": "InternalError", "Message": "Internal error"}}, "AdminDeleteUser"), - {}, - ] + # First attempt: a throttled delete surfaces as the mandated + # retryable RESOURCE_BUSY. Second attempt: the delete succeeds, so + # the helper passes and the catch's raise branch is skipped. + throttle = ClientError( + {"Error": {"Code": "TooManyRequestsException", "Message": "Rate exceeded"}}, + "AdminDeleteUser", + ) + mock_cognito.admin_delete_user.side_effect = [throttle] mock_get_client.return_value = mock_cognito first = admin_purge_user_account(event, lambda_context) assert first["__isError"] is True - # The helper raises a typed AppError for the Cognito ClientError; - # the narrowed catch lets it pass through with its own message - # instead of rewrapping it into the partial-state wording. - assert first["errorCode"] == ErrorCode.INTERNAL_ERROR + # The throttled ClientError is typed at the helper boundary as the + # mandated retryable RESOURCE_BUSY, and it propagates through the + # narrowed catch with its code intact instead of surfacing as a + # non-retryable INTERNAL_ERROR from a generic rewrap. + assert first["errorCode"] == ErrorCode.RESOURCE_BUSY assert first["message"] == "Failed to delete user from Cognito" # The sweep ran: the invite is gone, but the accounts row survives # for the retry to remove. assert invites_table.get_item(Key={"inviteCode": "INV-OWNED"}).get("Item") is None assert "Item" in get_accounts_table().get_item(Key={"accountId": f"ACCOUNT#{target_account_id}"}) + mock_cognito.admin_delete_user.side_effect = None # type: ignore[assignment] + second = admin_purge_user_account(event, lambda_context) assert second is True From 363ef6cd013429616db69907b89d8855f49a40b6 Mon Sep 17 00:00:00 2001 From: David Meiser Date: Sat, 3 Oct 2026 12:45:29 -0400 Subject: [PATCH 8/9] no-mistakes(review): Guard transient classification against BotoCoreError in both deletion catches --- src/handlers/account_operations.py | 4 +- src/handlers/deletion_cascade.py | 13 +++- tests/unit/test_account_deletion_cascade.py | 43 ++++++++++++ tests/unit/test_account_operations.py | 72 +++++++++++++++++++++ 4 files changed, 128 insertions(+), 4 deletions(-) diff --git a/src/handlers/account_operations.py b/src/handlers/account_operations.py index 636e1885..edbc427a 100644 --- a/src/handlers/account_operations.py +++ b/src/handlers/account_operations.py @@ -143,7 +143,9 @@ def delete_my_account(event: Dict[str, Any], context: Any) -> bool: # message says exactly that. if isinstance(e, AppError): raise - error_code = ErrorCode.RESOURCE_BUSY if is_transient_client_error(e) else ErrorCode.INTERNAL_ERROR + error_code = ( + ErrorCode.RESOURCE_BUSY if isinstance(e, ClientError) and is_transient_client_error(e) else ErrorCode.INTERNAL_ERROR + ) logger.error( "Account data swept but the Cognito delete did not report success; " "the account's Cognito state is unknown and needs a retry or manual completion", diff --git a/src/handlers/deletion_cascade.py b/src/handlers/deletion_cascade.py index 2066cecc..a6802251 100644 --- a/src/handlers/deletion_cascade.py +++ b/src/handlers/deletion_cascade.py @@ -19,7 +19,7 @@ from typing import TYPE_CHECKING, Any, Dict -from botocore.exceptions import ClientError +from botocore.exceptions import BotoCoreError, ClientError # Sibling handler modules use a same-package relative import, which resolves both # in the Lambda zip (package `handlers`) and in unit tests (package `src.handlers`). @@ -271,12 +271,19 @@ def delete_all_user_data(account_id: str, logger: Any = None) -> None: try: tables.accounts.delete_item(Key={"accountId": account_id_key}) log.info("Deleted account from DynamoDB", account_id=account_id_key) - except ClientError as e: + except (ClientError, BotoCoreError) as e: # Every sub-cascade above completed, so this is the most likely partial # state of the whole cascade: all user data is gone but the account # record — and the Cognito sign-in that outlives it in the self-service # flow — survives. Say exactly that; a throttled failure is retryable. - error_code = ErrorCode.RESOURCE_BUSY if is_transient_client_error(e) else ErrorCode.INTERNAL_ERROR + # BotoCoreError joins the catch for the same transport-fault class: + # a failure after the request was sent leaves the same swept/survived + # split, and only a ClientError can be classified as throttling. + error_code = ( + ErrorCode.RESOURCE_BUSY + if isinstance(e, ClientError) and is_transient_client_error(e) + else ErrorCode.INTERNAL_ERROR + ) log.error( "All user data was swept but the account record could not be deleted; " "the account record and Cognito sign-in survive and need a retry or manual completion", diff --git a/tests/unit/test_account_deletion_cascade.py b/tests/unit/test_account_deletion_cascade.py index 363744ac..5b400ace 100644 --- a/tests/unit/test_account_deletion_cascade.py +++ b/tests/unit/test_account_deletion_cascade.py @@ -235,6 +235,30 @@ def fail_delete_item(*args: Any, **kwargs: Any) -> Any: delete_all_user_data(OWNER_SUB) return excinfo.value + def _run_with_transport_failing_account_delete(self, monkeypatch: pytest.MonkeyPatch) -> Any: + """Run the full cascade with the final accounts-row delete transport-failing. + + A BotoCoreError (no ``.response``) from the final delete carries the + same swept/survived split as a ClientError, so it must surface as the + same honest wording rather than escape raw to the decorator. + """ + from botocore.exceptions import ConnectionError as BotoConnectionError + + from src.handlers.deletion_cascade import delete_all_user_data + from src.utils.dynamodb import tables + + _seed_user_data() + _seed_s3_data() + + def fail_delete_item(*args: Any, **kwargs: Any) -> Any: + raise BotoConnectionError(error="connection reset", operation_name="DeleteItem") + + monkeypatch.setattr(tables.accounts, "delete_item", fail_delete_item) + + with pytest.raises(AppError) as excinfo: + delete_all_user_data(OWNER_SUB) + return excinfo.value + def test_throttled_final_delete_is_retryable_and_names_survivors( self, dynamodb_table: Any, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -263,6 +287,25 @@ def test_final_delete_failure_names_swept_and_surviving_data( _assert_all_user_data_deleted() assert "Item" in _table(ACCOUNTS_TABLE).get_item(Key={"accountId": OWNER_ACCOUNT_KEY}) + def test_transport_failed_final_delete_names_swept_and_surviving_data( + self, dynamodb_table: Any, monkeypatch: pytest.MonkeyPatch + ) -> None: + """A transport failure on the final delete keeps the honest attribution. + + A BotoCoreError has no ``.response``, so the classification must not + treat it as a ClientError; it surfaces as INTERNAL_ERROR with the same + swept/survived wording instead of escaping raw to the decorator. + """ + error = self._run_with_transport_failing_account_delete(monkeypatch) + + assert error.error_code == ErrorCode.INTERNAL_ERROR + assert error.message == ( + "All user data was deleted but the account record could not be removed, " + "so the sign-in remains active; retry to complete the deletion" + ) + _assert_all_user_data_deleted() + assert "Item" in _table(ACCOUNTS_TABLE).get_item(Key={"accountId": OWNER_ACCOUNT_KEY}) + class TestCascadeModuleGraph: """The cascade is a leaf of the handler import graph, not a cycle participant.""" diff --git a/tests/unit/test_account_operations.py b/tests/unit/test_account_operations.py index b65c72b0..2f3c1b50 100644 --- a/tests/unit/test_account_operations.py +++ b/tests/unit/test_account_operations.py @@ -838,6 +838,78 @@ def test_delete_account_cognito_admin_delete_error( # gone while the Cognito user survives for the retry to remove. assert accounts_table.get_item(Key={"accountId": account_id_key}).get("Item") is None + def test_delete_account_cognito_transport_error_after_sweep_does_not_crash( + self, + dynamodb_table: Any, + sample_account_id: str, + appsync_event: Dict[str, Any], + lambda_context: Any, + monkeypatch: Any, + capsys: Any, + ) -> None: + """A transport failure after the sweep yields the partial-state error, not a crash. + + A BotoCoreError (e.g. connection reset after the delete was sent) has + no ``.response``, so the classification must not touch it as a + ClientError: the failure surfaces as the unknown-state wording with + INTERNAL_ERROR instead of an AttributeError escaping to the decorator's + far-generic wording. + """ + from botocore.exceptions import ConnectionError as BotoConnectionError + + from src.handlers.account_operations import delete_my_account + + monkeypatch.setenv("ACCOUNTS_TABLE_NAME", "kernelworx-accounts-ue1-dev") + monkeypatch.setenv("USER_POOL_ID", "us-east-1_test123") + + dynamodb = boto3.resource("dynamodb", region_name="us-east-1") + accounts_table = dynamodb.Table("kernelworx-accounts-ue1-dev") + account_id_key = f"ACCOUNT#{sample_account_id}" + accounts_table.put_item( + Item={ + "accountId": account_id_key, + "email": "test@example.com", + "createdAt": datetime.now(timezone.utc).isoformat(), + } + ) + + with patch("boto3.client") as mock_boto_client: + mock_cognito = MagicMock() + mock_boto_client.return_value = mock_cognito + + mock_cognito.list_users.return_value = { + "Users": [ + { + "Username": "test-user", + "Attributes": [{"Name": "sub", "Value": sample_account_id}], + } + ] + } + mock_cognito.admin_delete_user.side_effect = BotoConnectionError( + error="connection reset after the request was sent", operation_name="AdminDeleteUser" + ) + + event = { + **appsync_event, + "identity": {"sub": sample_account_id}, + } + + result = delete_my_account(event, lambda_context) + + assert result["__isError"] is True + assert result["errorCode"] == ErrorCode.INTERNAL_ERROR + assert result["message"] == ( + "Account data was deleted but the Cognito user could not be confirmed deleted; " + "retry to complete the deletion" + ) + + captured = capsys.readouterr().out + assert "Account data swept but the Cognito delete did not report success" in captured + + # The sweep completed: user data is gone while the Cognito user's + # state is unknown, so a retry converges. + assert accounts_table.get_item(Key={"accountId": account_id_key}).get("Item") is None + def test_missing_username_relookup_failure_does_not_claim_user_present( self, dynamodb_table: Any, From 4fd2802c94baf789774f381e2af5480a4cd599f8 Mon Sep 17 00:00:00 2001 From: David Meiser Date: Sat, 3 Oct 2026 13:27:13 -0400 Subject: [PATCH 9/9] no-mistakes(document): Reformatted deletion handler, added spellcheck dictionary words --- .cspell/kernelworx-words.txt | 2 ++ src/handlers/account_operations.py | 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/.cspell/kernelworx-words.txt b/.cspell/kernelworx-words.txt index ab471d82..9d97c717 100644 --- a/.cspell/kernelworx-words.txt +++ b/.cspell/kernelworx-words.txt @@ -117,6 +117,7 @@ EEYQ esac esbuild eslint +excinfo exfiltration exportable falsey @@ -313,6 +314,7 @@ refetched regionAbbrev regularuser relogin +relookup renderer repo repr diff --git a/src/handlers/account_operations.py b/src/handlers/account_operations.py index edbc427a..5d62a57a 100644 --- a/src/handlers/account_operations.py +++ b/src/handlers/account_operations.py @@ -144,7 +144,9 @@ def delete_my_account(event: Dict[str, Any], context: Any) -> bool: if isinstance(e, AppError): raise error_code = ( - ErrorCode.RESOURCE_BUSY if isinstance(e, ClientError) and is_transient_client_error(e) else ErrorCode.INTERNAL_ERROR + ErrorCode.RESOURCE_BUSY + if isinstance(e, ClientError) and is_transient_client_error(e) + else ErrorCode.INTERNAL_ERROR ) logger.error( "Account data swept but the Cognito delete did not report success; "