Bug: Idempotency cache persistence overwrites live records when recovering an orphan

[svozza]

Expected Behavior

When the Redis/Valkey persistence layer recovers what it believes is an orphaned in-progress record, it should only replace the record if it is still that orphan. If another invocation has completed the operation or acquired the key in the meantime, the recovering invocation should receive the current record and either replay the stored result or be rejected as a concurrent execution.

Current Behavior

CachePersistenceLayer._putRecord() reads the existing record after a failed SET NX, classifies it, and if it looks orphaned acquires a lock and then writes the new in-progress record with an unconditional SET. The classification and the write both trust the snapshot from the earlier GET. Nothing checks that the record is unchanged at write time.

Two consequences follow, both confirmed with a TLA+ model of the record lifecycle and reproduced in a unit test with two persistence instances sharing one client:

• Caller B's GET returns caller A's in-progress record. A completes and A's deadline passes before B classifies the snapshot. B treats the stale snapshot as an orphan, takes the lock, and overwrites A's completed result with a new in-progress record. A subsequent retry re-executes the operation.
• With three callers: A fails and deletes its record, C acquires the key with a plain SET NX, and B's takeover overwrites C's record while C is still running. Two invocations execute the operation concurrently.

The recovery lock does not prevent either case. It only serialises other orphan recoverers. First-time acquirers never take it, and the lock is taken after the snapshot that drives the decision.

The DynamoDB persistence layer is not affected: its conditional PutItem evaluates the expiry and deadline against the current item atomically.

Code snippet

import assert from 'node:assert/strict';
import { randomUUID } from 'node:crypto';
import {
  IdempotencyConfig,
  IdempotencyItemAlreadyExistsError,
} from '@aws-lambda-powertools/idempotency';
import { CachePersistenceLayer } from '@aws-lambda-powertools/idempotency/cache';
import { createClient } from '@redis/client';

// A slow client for B: delivers every GET response late so A can complete meanwhile.
const fast = await createClient({ url: 'redis://localhost:6379' }).connect();
const slow = await createClient({ url: 'redis://localhost:6379' }).connect();
const delayedGet = slow.get.bind(slow);
slow.get = async (key) => {
  const value = await delayedGet(key); // read now
  await new Promise((r) => setTimeout(r, 2500)); // deliver late
  return value;
};

const config = new IdempotencyConfig({ expiresAfterSeconds: 3600 });
const keyPrefix = `stale-takeover-${randomUUID()}`;
const callerA = new CachePersistenceLayer({ client: fast });
const callerB = new CachePersistenceLayer({ client: slow });
callerA.configure({ config, keyPrefix });
callerB.configure({ config, keyPrefix });
const event = { id: 'order-1' };

try {
  await callerA.saveInProgress(event, 2000); // deadline in 2s
  const acquisition = callerB.saveInProgress(event, 2000); // GET answered after 2.5s
  await callerA.saveSuccess(event, { processed: true });

  // B should be told the record exists. Currently resolves and overwrites it.
  await assert.rejects(acquisition, IdempotencyItemAlreadyExistsError);
  const [recordKey] = await fast.keys(`${keyPrefix}#*`);
  const stored = JSON.parse((await fast.get(recordKey)) ?? '{}');
  assert.equal(stored.status, 'COMPLETED');
} finally {
  await callerA.deleteRecord(event);
  await fast.close();
  await slow.close();
}

Steps to Reproduce

1. Start a local Redis or Valkey instance and install the idempotency package and @redis/client.
2. Run the snippet. Caller A acquires the key with a two-second deadline, caller B's SET NX fails and its GET is delayed past that deadline, and A completes in between.
3. Observe that B's acquisition resolves instead of throwing, and the stored record is now INPROGRESS owned by B. A's completed result is gone.
4. As a control, remove the delay from the slow client. B's acquisition correctly throws IdempotencyItemAlreadyExistsError with A's record.

Possible Solution

Two options both verified against the TLA+ model of the lifecycle:

• Remove the takeover path. Give in-progress records a TTL equal to their execution deadline instead of the overall expiry, using PX. Redis then removes an orphan at exactly the moment its owner is guaranteed to have been killed, and the next SET NX acquires it atomically. saveSuccess() already rewrites the key with the full expiry, and records without a deadline keep the current behaviour. The lock, orphan classification, and unconditional overwrite can be deleted. This relies on the same assumption as the DynamoDB condition, that an invocation cannot act after its deadline.
• Make the takeover atomic. Replace the unconditional SET with a server-side script that writes only if the current value still equals the observed orphan, and returns the current record otherwise so the handler can classify it. Re-reading after taking the lock is not sufficient, since a first-time acquirer can SET NX between the re-read and the write.

The first option is smaller and removes code, so it is the recommended one.

Powertools for AWS Lambda (TypeScript) version

2.35.0

AWS Lambda function runtime

22.x

Packaging format used

npm

[vishwakt]

@svozza is this open for community contribution?

[svozza]

I've already started this one but I have another issue I'm going to file that you can pick up. I will tag you when I post it.