Repository navigation
fix(backend): harden get_caller_id and classify transport faults on the deletion lookups - #678
Merged
Merged
Conversation
…he deletion lookups Carries forward the two non-duplicate pieces of the closed #671 and #676, neither of which made it into the merged work. get_caller_id (from #671, whose rest is a duplicate of merged #662): `event.get("identity") or {}` then `.get("sub")` assumed identity is a mapping. A present-but-non-mapping identity (a string, a number, a list) is truthy, so the read raised AttributeError instead of returning None, and the caller surfaced as the decorator's generic INTERNAL_ERROR rather than the typed UNAUTHORIZED the handlers raise for an absent caller. Guard with isinstance against Mapping. Cognito pre-check lookups (from #676, whose rest is merged #673 and #677): both `_lookup_cognito_user_for_deletion` and `_find_cognito_user_by_sub` caught ClientError only, so a BotoCoreError during the lookup escaped to the decorator with no attributed log line and no retry guidance -- even though the lookup runs before the sweep and the Cognito delete, so nothing has been mutated and a retry converges. Widen both to `(ClientError, BotoCoreError)`. To classify that without adding a second definition of "transient", `is_transient_cognito_error` now takes a BaseException and counts a bare BotoCoreError as retryable; the pre-delete catches just call it. The post-sweep Cognito delete in `run_deletion_steps` keeps its narrower `isinstance(error, ClientError)` guard, and the asymmetry is now documented on both sides: a transport fault *after* the delete request was sent leaves the Cognito state genuinely unknown, so it must not promise a retry, while the same fault *before* anything is mutated is plainly retryable. This keeps #677's tested unknown-state contract intact. Gates: `pytest tests/unit` 1575 passed / 2 skipped at 100% coverage, xenon, ruff, and mypy all clean.
The AGENTS.md #291 entry now names the isinstance narrowing that distinguishes the post-sweep Cognito delete from the pre-delete lookups; the word was not in the dictionary and failed the frontend job's Spell check step.
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Carries forward the two non-duplicate pieces of the now-closed #671 and #676. Neither is in
main; both are small and independently testable, so they are split out rather than folded into the already-merged deletion work.1.
get_caller_idassumedidentityis a mapping (from #671)event.get("identity") or {}then.get("sub")raisedAttributeErroron a present-but-non-mappingidentity— a string, a number, a list — because those are truthy. The caller then surfaced as the decorator's genericINTERNAL_ERRORrather than the typedUNAUTHORIZEDthe handlers raise for an absent caller.Everything else in #671 is already on
mainvia #662 (same 31 files, same outcome:responses.pygone, the dead symbols pruned, the AST guard and behavioural UNAUTHORIZED check both present), which is why that PR was closed rather than merged.2. The pre-delete Cognito lookups ignored transport faults (from #676)
Both
_lookup_cognito_user_for_deletionand_find_cognito_user_by_subcaughtClientErroronly, so aBotoCoreErrorduring the lookup escaped tolambda_handler— no attributed log line, no retry guidance. That is the wrong answer here specifically: the lookup runs before the sweep and before the Cognito delete, so nothing has been mutated and a retry converges. Both now catch(ClientError, BotoCoreError).To classify that without adding a second definition of "transient",
is_transient_cognito_errornow takes aBaseExceptionand counts a bareBotoCoreErroras retryable (a transport fault is not a service verdict); the pre-delete catches just call it.COGNITO_TRANSIENT_ERROR_CODESremains the single set shared withretry_on_transient_errors.The asymmetry this deliberately preserves
run_deletion_steps' post-sweep Cognito delete keeps its narrowerisinstance(error, ClientError)guard, so #677's tested unknown-state contract is untouched: there the delete request has already been sent, so a transport fault leaves the Cognito state genuinely unknown and must not promise a retry. Same fault, opposite correct answer, because of where it happens. Both sides are now commented in the code and in the AGENTS.md #291 entry.Tests
test_cognito_transient_classification.py(new): every code in the set, a permanent code, a bareBotoCoreError, non-botocore exceptions, aClientErrorwith noErrorblock, plus a guard that the retry wrapper and the classifier cannot drift onto separate sets.test_caller_id_helper.py: non-mapping identities returnNone.BotoCoreErrorlookup isRESOURCE_BUSYwith a retry promise on both the self-service and admin paths, and the Cognito delete is never attempted.Gates
pytest tests/unit— 1575 passed, 2 skipped, 100% coverage;xenon --max-average A --max-absolute B(the CI complexity gate),ruff check src/ tests/, andmypy src/all clean.