Skip to content

Commit 8ee7639

Browse files
committed
fix(idempotency): Redis persistence layer reclaims live in-progress records as orphans, allowing concurrent double-execution
RedisCachePersistenceLayer._put_in_progress_record() only guarded against a competing invocation when the existing record's in_progress_expiry_timestamp was set AND still in the future. When that field is None -- which is the normal case for idempotent_function, since it never calls config.register_lambda_context(), unlike the idempotent() handler decorator -- the guard was skipped entirely and a genuinely still-running invocation's record fell through to the "orphan record" branch, which unconditionally overwrites the record with no NX guard. A second concurrent invocation with the same idempotency key would then proceed to execute the underlying function too, defeating idempotency (e.g. double-charging a customer). This is exactly what persistence/base.py's own warning at that call site flags ("Couldn't determine the remaining time left. Did you call register_lambda_context on IdempotencyConfig?") -- it warns, but nothing downstream actually failed closed on it. The DynamoDB persistence layer does not have this gap: its conditional expression requires attribute_exists(#in_progress_expiry) before allowing an expired-in-progress reclaim, so a missing attribute correctly blocks the competing writer instead of granting a reclaim. Fix: when status is INPROGRESS and in_progress_expiry_timestamp is None, treat the record as still in progress (fail closed) instead of falling through to the orphan-reclaim path, mirroring the DynamoDB layer's behavior.
1 parent 8db13c7 commit 8ee7639

2 files changed

Lines changed: 45 additions & 4 deletions

File tree

aws_lambda_powertools/utilities/idempotency/persistence/redis.py

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -425,10 +425,17 @@ def _put_in_progress_record(self, data_record: DataRecord) -> None:
425425
# (meaning the timestamp is greater than the current timestamp in milliseconds), then we have encountered
426426
# a valid in-progress record. This indicates that another process is currently handling the request, and
427427
# to maintain idempotency, we raise an error to prevent concurrent processing of the same request.
428-
if (
429-
idempotency_record.status == STATUS_CONSTANTS["INPROGRESS"]
430-
and idempotency_record.in_progress_expiry_timestamp
431-
and idempotency_record.in_progress_expiry_timestamp > int(now.timestamp() * 1000)
428+
#
429+
# If the record is INPROGRESS but in_progress_expiry_timestamp was never set (e.g. the caller never
430+
# invoked config.register_lambda_context(), so remaining_time_in_millis was None when the record was
431+
# created), we cannot determine whether the in-progress invocation has actually timed out. Fail closed
432+
# and treat it as still in progress, rather than reclaiming it as an "orphan" below -- otherwise a
433+
# second concurrent invocation would wrongly conclude the first one has expired and execute the
434+
# function a second time. This mirrors the DynamoDB persistence layer, which requires
435+
# attribute_exists(#in_progress_expiry) before allowing an expired-in-progress reclaim.
436+
if idempotency_record.status == STATUS_CONSTANTS["INPROGRESS"] and (
437+
idempotency_record.in_progress_expiry_timestamp is None
438+
or idempotency_record.in_progress_expiry_timestamp > int(now.timestamp() * 1000)
432439
):
433440
raise IdempotencyItemAlreadyExistsError
434441

tests/functional/idempotency/_redis/test_redis_layer.py

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,18 @@ def valid_record():
198198
)
199199

200200

201+
@pytest.fixture
202+
def in_progress_record_missing_expiry():
203+
# Simulates a record created via idempotent_function without register_lambda_context()
204+
# having been called: in_progress_expiry_timestamp was never set. This record represents
205+
# a genuinely still-running invocation, NOT an orphan.
206+
return DataRecord(
207+
idempotency_key="test_orphan_key",
208+
status=STATUS_CONSTANTS["INPROGRESS"],
209+
in_progress_expiry_timestamp=None,
210+
)
211+
212+
201213
@mock.patch("aws_lambda_powertools.utilities.idempotency.persistence.redis.redis", MockRedis())
202214
def test_redis_connection_standalone():
203215
# when RedisCachePersistenceLayer is init with the following params
@@ -303,6 +315,28 @@ def test_redis_orphan_record_lock(orphan_record, valid_record):
303315
)
304316

305317

318+
@mock.patch("aws_lambda_powertools.utilities.idempotency.persistence.redis.redis", MockRedis())
319+
def test_redis_in_progress_record_missing_expiry_is_not_treated_as_orphan(in_progress_record_missing_expiry):
320+
"""Regression test: an INPROGRESS record whose in_progress_expiry_timestamp is None (e.g. because
321+
idempotent_function was used without register_lambda_context()) must NOT be reclaimed as an orphan.
322+
Doing so lets a second concurrent invocation execute the underlying function while the first is
323+
still genuinely running, defeating idempotency (e.g. double-charging a customer).
324+
"""
325+
layer = RedisCachePersistenceLayer(host="host")
326+
# Given a genuinely still-running in-progress record with no expiry info
327+
layer._put_in_progress_record(in_progress_record_missing_expiry)
328+
329+
# When a second, concurrent invocation tries to claim the same idempotency key
330+
# Then it must be rejected as "already in progress", not treated as an orphan and overwritten
331+
with pytest.raises(IdempotencyItemAlreadyExistsError):
332+
layer._put_in_progress_record(in_progress_record_missing_expiry)
333+
334+
# And the original record must remain untouched
335+
assert layer._get_record(in_progress_record_missing_expiry.idempotency_key).status == STATUS_CONSTANTS[
336+
"INPROGRESS"
337+
]
338+
339+
306340
@mock.patch("aws_lambda_powertools.utilities.idempotency.persistence.redis.redis", MockRedis())
307341
def test_redis_error_in_progress(valid_record):
308342
layer = RedisCachePersistenceLayer(host="host", mode="standalone")

0 commit comments

Comments
 (0)