demo: idempotency tech-debt — falsy responses + Redis None handling#1
demo: idempotency tech-debt — falsy responses + Redis None handling#1sudsali wants to merge 2 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # 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": |
There was a problem hiding this comment.
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 useSTATUS_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.
| 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 |
There was a problem hiding this comment.
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 callssave_success(result=serialized_response); datarecord.py:98response_json_as_dict()returns None whenresponse_datais 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.
sudsali
force-pushed
the
demo-broken-idempotency
branch
from
e180869 to
de11b9b
Compare
| 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)), |
There was a problem hiding this comment.
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, sojson.loads("None")is invoked and fails. The PR also deletestest_item_to_datarecord_conversion_missing_optional_attributeswhich 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.
…es and Redis persistency (aws-powertools#8176)" This reverts commit 08c9921.
sudsali
force-pushed
the
demo-broken-idempotency
branch
from
de11b9b to
b7507b1
Compare
1 participant
Demo PR for the Shadow PR-review bot.
Why this PR exists
This is a reverted version of aws-powertools/powertools-lambda-python#8176 — a real merged fix from upstream Powertools. By opening it as a fresh PR against this fork's `develop`, the Shadow bot reviews the diff and (if the bot is working) flags the bugs the human reviewer also caught.
What the diff does
Re-introduces three known issues from the pre-aws-powertools#8176 codebase:
idempotency/base.py:170— replaces `STATUS_CONSTANTS["INPROGRESS"]` with the hardcoded literal `"INPROGRESS"`. Drift risk if the constants dict is renamed.idempotency/base.py:299— changes `response is not None` to plain `response` (truthy check). Functions returning `0`, `False`, `""`, `[]` will be persisted as `None`, defeating idempotency replay.idempotency/persistence/redis.py:338-339— wraps `item.get(...)` in `str(...)`, producing the literal string `"None"` when fields are missing instead of an empty string.Expected Shadow output
The bot should post inline comments calling out at least bugs 2 and 3 (real correctness regressions). Bug 1 may surface as DESIGN/NIT depending on Critic verdict.