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 b8ad3d4e..5d62a57a 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 @@ -96,7 +98,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") @@ -111,24 +115,52 @@ 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_all_user_data(account_id, logger) - _delete_user_from_cognito(cognito, user_pool_id, account_id, username, logger) - logger.info("Account deletion completed successfully") - return True + 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, 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") from e - 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 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_user_from_cognito(cognito, user_pool_id, account_id, username, logger) + 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 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", + account_id=account_id, + error=str(e), + error_code=error_code, + exc_info=True, + ) + raise AppError( + error_code, + "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") + return True diff --git a/src/handlers/admin_operations.py b/src/handlers/admin_operations.py index 16c2c56a..deb4bb46 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`). @@ -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 @@ -986,7 +991,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__) @@ -1019,8 +1026,37 @@ def admin_purge_user_account(event: Dict[str, Any], context: Any) -> bool: 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 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: - _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 (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=ErrorCode.INTERNAL_ERROR, + exc_info=True, + ) + raise AppError( + ErrorCode.INTERNAL_ERROR, + "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/src/handlers/deletion_cascade.py b/src/handlers/deletion_cascade.py index 7d92d4ce..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`). @@ -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,48 @@ 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 + 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. + # 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", + 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..5b400ace 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,100 @@ 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 _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: + """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}) + + 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 87773f66..2f3c1b50 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, @@ -762,16 +821,348 @@ 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 could not be confirmed deleted; " + "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 delete did not report success" in captured + assert "INTERNAL_ERROR" in captured assert "InternalError" in captured assert "AdminDeleteUser" in captured + # 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_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, + 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, + sample_account_id: str, + appsync_event: Dict[str, Any], + lambda_context: Any, + monkeypatch: Any, + ) -> None: + """A Cognito failure after a successful sweep is a partial delete; a retry completes it. + + 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 + + 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 = [ + ClientError({"Error": {"Code": "InternalError", "Message": "Internal error"}}, "AdminDeleteUser"), + {}, + ] + + event = { + **appsync_event, + "identity": {"sub": sample_account_id}, + } + + first = delete_my_account(event, lambda_context) + + assert first["__isError"] is True + assert first["errorCode"] == ErrorCode.INTERNAL_ERROR + assert first["message"] == ( + "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) + + 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_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, @@ -927,7 +1318,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 @@ -971,11 +1362,13 @@ 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["message"] == "Failed to delete account" + 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" + ) 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 @@ -1261,15 +1654,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 fc5425d9..c32de050 100644 --- a/tests/unit/test_admin_operations.py +++ b/tests/unit/test_admin_operations.py @@ -1854,6 +1854,161 @@ def test_purge_sweeps_inbound_shares( is not None ) + def test_cognito_failure_after_sweep_then_retry_completes( + self, + dynamodb_table: Any, + invites_table: Any, + admin_appsync_event: Dict[str, Any], + lambda_context: Any, + monkeypatch: Any, + ) -> None: + """A throttled Cognito delete after the sweep is retryable; a retry completes it. + + 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. + """ + 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) + # 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 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 + 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. 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 + + 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, + admin_appsync_event: Dict[str, Any], + lambda_context: Any, + monkeypatch: Any, + ) -> None: + """A sweep failure before the Cognito delete is not a partial delete. + + 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") + + 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 + 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, dynamodb_table: Any,