Repository navigation
fix(idempotency): clear the in progress record when the response can't be serialized - #8740
Open
sumedh-aerram wants to merge 3 commits into
Conversation
…t be serialized When the decorated function returned a value the output serializer or json.dumps could not convert (datetime, set, bytes, ...), the invocation failed with IdempotencyPersistenceLayerError but the INPROGRESS record stayed. Every retry with the same payload then raised IdempotencyAlreadyInProgressError until the in progress expiry, or for expires_after_seconds (one hour by default) without register_lambda_context. Serialization fails before anything is written, so the record can be deleted safely, the same way it is when the function raises. Clear it at both serialization points: the output serializer's to_dict in the handler, and json.dumps in BasePersistenceLayer.save_success. The error type and message are unchanged. A failure while writing the COMPLETED record still keeps the record: that write may have landed, and deleting it would let a retry run the function twice.
Add public decorator tests showing a failed write of the COMPLETED record keeps the record, both when the write landed (the next call replays the saved response) and when it did not (the next call stays in progress), with and without the local cache. This guards the cleanup from widening to write failures. Say in the docs that the in progress record is only deleted if cleanup succeeds, otherwise retries wait for it to expire, and that write failures keep the record. Mention serialization cleanup in the exceptions example.
Keeps the idempotency handler module within the complexity rank checked by make complexity-baseline.
|
This branch has not been 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.



Issue number:
closes #8721
Summary
Changes
When the decorated function returns a value that the output serializer or
json.dumpscan't convert, delete the in progress record before raisingIdempotencyPersistenceLayerError, the same way the record is deleted when the function itself raises. Cleanup runs at both conversion points: the output serializer'sto_dictin the handler, andjson.dumpsinBasePersistenceLayer.save_success. Both run before anything is written. The error type and message are unchanged.A failure while writing the COMPLETED record still keeps the record, because the write may have landed and deleting it would let a retry run the function twice. New tests guard that boundary.
The "Handling exceptions" docs section and its example now describe the new failure mode. They say cleanup is attempted, retries wait for expiry if the deletion itself fails, and write failures keep the record.
User experience
Before: returning e.g. a
datetime,setorbytesfailed the invocation, but left the record INPROGRESS. Every retry with the same payload raisedIdempotencyAlreadyInProgressErroruntil the in progress expiry, or forexpires_after_seconds(one hour by default) withoutregister_lambda_context.After: the invocation still fails with the same error, but the next call with the same payload runs the function again.
Known limitation, not introduced here: deletion is by key, without an ownership check. If a function outlives
expires_after_secondsand another invocation reacquires the key, a cleanup can remove the other invocation's record. Upstream's existing cleanup for function exceptions already has this behaviour. I reproduced it on unchangeddevelopwith a scripted replacement record (fake Redis client, public decorator API). This PR adds a new trigger to that existing cleanup, but no new mechanism. An ownership-aware delete (DynamoDB conditional delete, Redis compare-and-delete) seems worth a separate issue. I'm happy to open one if you agree.Verification
AI assistants (Claude and Codex) helped implement and review this change. I ran the commands below and read the output.
make -k pr tyexited 0: 4012 passed, 73 skipped, 10 deselected, plus all 10 performance tests. Ruff, mypy (949 source files), ty, docs lint, security and complexity checks passed. An earlier gate flagged the handler module at complexity rank B; the serializer cleanup moved into a small helper and the gate was rerun clean.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.