Skip to content

demo: idempotency tech-debt — falsy responses + Redis None handling #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sudsali wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

demo: idempotency tech-debt — falsy responses + Redis None handling #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b7507b1
Revert "fix(idempotency): resolve tech debt issues with falsy respons…
sudsali May 31, 2026
e96d7a9
Pin Shadow workflow to current v0 SHA
sudsali Jun 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/shadow.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ permissions:

jobs:
shadow:
uses: sudsali/shadow/.github/workflows/shadow-review.yml@cc9aea0bf5407458db5be1a238387ca1d03d162f # v0
uses: sudsali/shadow/.github/workflows/shadow-review.yml@38f2dd08914b79a43a2eb0100a246ad5e664acfc # v0
with:
pr_number: ${{ inputs.pr_number }}
dry_run: ${{ inputs.dry_run && 'true' || 'false' }}
Expand Down
4 changes: 2 additions & 2 deletions aws_lambda_powertools/utilities/idempotency/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ def _process_idempotency(self, is_replay: bool):
# We give preference to ReturnValuesOnConditionCheckFailure because it is a faster and more cost-effective
# way of retrieving the existing record after a failed conditional write operation.
record = exc.old_data_record or self._get_idempotency_record()
if is_replay and record is not None and record.status == STATUS_CONSTANTS["INPROGRESS"]:
if is_replay and record is not None and record.status == "INPROGRESS":
Copy link
Copy Markdown

@github-actions github-actions Bot May 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DESIGN: Hardcoding the literal "INPROGRESS" here couples the replay-resume branch to an implementation detail of the constants table. All other call sites (base.py:253, 256; persistence/base.py:239, 280, 313; dynamodb.py:246; redis.py:421, 429, 481) uniformly use STATUS_CONSTANTS["INPROGRESS"]. This inconsistency creates drift risk: if the constants dict is refactored or renamed, this branch alone is not updated.

datarecord.py:14 defines STATUS_CONSTANTS = MappingProxyType({"INPROGRESS": "INPROGRESS", ...}); grep showed 14 other call sites use STATUS_CONSTANTS["..."] while only base.py:170 (post-diff) uses the bare literal.

Refutation trail (why this survived the Critic's disprove pass)

Hypothesis (Investigator): Replacing STATUS_CONSTANTS["INPROGRESS"] with the literal "INPROGRESS" introduces drift risk and inconsistency — if the constant value ever changes, this branch silently breaks; meanwhile every other call site (lines 253, 256, persistence/base.py:239, 280, 313, dynamodb.py:246, redis.py:421, 429, 481) uses the constant.

Disprove attempt (Critic): Read datarecord.py:14 — STATUS_CONSTANTS = MappingProxyType({"INPROGRESS": "INPROGRESS", ...}). Today the literal equals the constant value, so behavior is unchanged. grep_codebase confirmed all sibling sites use the constant indirection.

The Critic's default verdict is OVERTURNED. UPHELD findings are those it tried — and failed — to refute.

return self._get_function_response()
# If a record is found, handle it for status
if record:
Expand Down Expand Up @@ -296,7 +296,7 @@ def _get_function_response(self):

else:
try:
serialized_response: dict = self.output_serializer.to_dict(response) if response is not None else None
serialized_response: dict = self.output_serializer.to_dict(response) if response else None
Copy link
Copy Markdown

@github-actions github-actions Bot May 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUG: Falsy-but-valid responses (0, False, "", [], {}) are silently lost. The idempotent replay returns None instead of the actual return value, breaking any handler whose contract includes a falsy return (e.g., counters returning 0, predicates returning False, list endpoints returning []). Restore the explicit response is not None check.

base.py:299 (post-diff) ... if response else None; base.py:300 always calls save_success(result=serialized_response); datarecord.py:98 response_json_as_dict() returns None when response_data is empty/None; base.py:274 replay returns None when response_dict is falsy.

Refutation trail (why this survived the Critic's disprove pass)

Hypothesis (Investigator): Changing response is not None to response causes falsy-but-valid return values (0, False, "", [], {}) to be persisted as None, so on replay the user receives None instead of the original falsy result — a correctness regression in idempotent replay.

Disprove attempt (Critic): Read base.py:282-307. The else branch always calls save_success(result=serialized_response). With if response else None, falsy responses skip serialization and persist result=None. Read base.py:273-274 — replay path does response_dict = data_record.response_json_as_dict() then from_dict(response_dict) if response_dict else None, so a None-saved record returns None on replay regardless of the original falsy value.

The Critic's default verdict is OVERTURNED. UPHELD findings are those it tried — and failed — to refute.

self.persistence_store.save_success(data=self.data, result=serialized_response)
except Exception as save_exception:
raise IdempotencyPersistenceLayerError(
Expand Down
4 changes: 2 additions & 2 deletions aws_lambda_powertools/utilities/idempotency/persistence/redis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,8 +335,8 @@ def _item_to_data_record(self, idempotency_key: str, item: dict[str, Any]) -> Da
idempotency_key=idempotency_key,
status=item[self.status_attr],
in_progress_expiry_timestamp=in_progress_expiry_timestamp,
response_data=item.get(self.data_attr, ""),
payload_hash=item.get(self.validation_key_attr, ""),
response_data=str(item.get(self.data_attr)),
Copy link
Copy Markdown

@github-actions github-actions Bot May 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUG: When a Redis idempotency record is missing data_attr or validation_key_attr, str(item.get(...)) returns the literal string "None" rather than "". DataRecord.response_json_as_dict() will then call json.loads("None"), raising JSONDecodeError on replay; payload-hash validation will compare the literal string "None" to actual hashes. Restore the previous defaults: item.get(self.data_attr, "") and item.get(self.validation_key_attr, ""). (Regression of issue #8090.)

redis.py:338-339 (post-diff): response_data=str(item.get(self.data_attr)), payload_hash=str(item.get(self.validation_key_attr)). datarecord.py:98: return json.loads(self.response_data) if self.response_data else None — "None" is truthy, so json.loads("None") is invoked and fails. The PR also deletes test_item_to_datarecord_conversion_missing_optional_attributes which was the dedicated regression test.

Refutation trail (why this survived the Critic's disprove pass)

Hypothesis (Investigator): Wrapping item.get(self.data_attr) and item.get(self.validation_key_attr) in str(...) produces the literal string "None" when the attribute is missing, breaking DataRecord.response_json_as_dict() (which then attempts json.loads("None") → JSONDecodeError) and corrupting payload-hash comparisons.

Disprove attempt (Critic): Read redis.py lines 331-341 — item.get(...) with no default returns None when key absent; str(None) == "None". Read datarecord.py:89-98 — response_json_as_dict returns json.loads(self.response_data) if self.response_data else None. The string "None" is truthy, so it falls into json.loads("None") which raises json.JSONDecodeError (JSON spec uses null, not None). Likewise payload_hash="None" is now a non-empty string that will be used in equality checks. The previous code used item.get(..., "") which gave the empty string and shortcut both code paths correctly. The removed regression test (test_item_to_datarecord_conversion_missing_optional_attributes) was specifically a guard for issue #8090 that this exact problem.

The Critic's default verdict is OVERTURNED. UPHELD findings are those it tried — and failed — to refute.

payload_hash=str(item.get(self.validation_key_attr)),
expiry_timestamp=item.get("expiration"),
)

Expand Down
19 changes: 0 additions & 19 deletions tests/functional/idempotency/_redis/test_redis_layer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,25 +330,6 @@ def test_item_to_datarecord_conversion(valid_record):
assert record.in_progress_expiry_timestamp == item[layer.in_progress_expiry_attr]


def test_item_to_datarecord_conversion_missing_optional_attributes(persistence_store_standalone_redis):
"""
When data_attr or validation_key_attr is missing from Redis,
response_data and payload_hash should be empty string, not the string "None".
Regression test for: https://github.com/aws-powertools/powertools-lambda-python/issues/8090
"""
idempotency_key = "test-func#abc123"
item = {
persistence_store_standalone_redis.status_attr: "COMPLETED",
persistence_store_standalone_redis.expiry_attr: 9999999999,
# data_attr and validation_key_attr intentionally absent
}

record = persistence_store_standalone_redis._item_to_data_record(idempotency_key, item)

assert record.response_data == ""
assert record.payload_hash == ""


def test_idempotent_function_and_lambda_handler_redis_basic(
persistence_store_standalone_redis: RedisCachePersistenceLayer,
lambda_context,
Expand Down
Loading