fix(locking-redis): order multi-key acquires, atomic releaseAll, same-owner reentrancy under awaitQueue by docloulou · Pull Request #16284 · medusajs/medusa

docloulou · 2026-08-02T11:12:06Z

Summary

What — What changes are introduced in this PR?

Three correctness fixes in the Redis locking provider (packages/modules/providers/locking-redis/src/services/redis-lock.ts), each reachable through the public locking module API:

Multi-key acquisition deduplicates and sorts the keys and takes them sequentially. This removes the ABBA deadlock between two callers that request the same keys in different orders under awaitQueue: true: previously each caller took the keys in its own argument order, in parallel, so two overlapping calls could each end up holding a key the other was waiting on and back off against each other until they timed out.
releaseAll issues the existing atomic releaseLock compare-and-delete per scanned key instead of a GET pipeline followed by a second UNLINK pipeline. It can no longer delete a lock that expired and was re-acquired by a different owner in the window between the read and the delete.
A named owner re-entering its own lock now succeeds under awaitQueue: true, as it already did with awaitQueue: false. The same-owner check was skipped entirely in the awaitQueue branch of the Lua script, so an owner backed off against its own lock until it timed out.

Why — Why are these changes relevant or necessary?

None of these require unusual configuration; a caller only has to use the documented options.

The deadlock needs nothing more than two concurrent calls passing an overlapping key set in different orders with awaitQueue: true, which is the option that is supposed to make contention wait rather than fail. Each call can take its first key and then stall on the second, and the failure mode is a timeout rather than a lock error, so it reads as a slow dependency rather than as contention. Sorting the keys inside the provider removes the cycle for the callers themselves, without any convention imposed on the caller.

releaseAll is documented as an owner-scoped sweep, but its read pipeline and its delete pipeline were separate round trips, and the gap between them grows with the number of scanned keys. Any key that expired and was re-acquired by another owner during that gap was deleted anyway, so a routine sweep by one owner could silently drop a lock another owner legitimately held. Reusing the existing releaseLock script makes the owner comparison and the delete happen at the same instant, on the server.

The re-entrancy bug turns awaitQueue: true into the opposite of its purpose for the one case where waiting is never correct. With awaitQueue: false, re-acquiring a lock you already own returns immediately and refreshes the TTL. With awaitQueue: true, the same call spun against its own lock until the key expired, or forever when no expire was passed. Passing an ownerId exists precisely so the provider can tell "my lock" from "someone else's lock", and the awaitQueue branch was the one place that ignored it.

How — How have these changes been implemented?

The Lua acquireLock script drops its awaitQueue argument. Whether to wait is a client-side decision; the script only reports the state of the key, and the same-owner branch now runs on every call instead of only when the caller opted out of waiting. The return contract is unchanged: 1 means the key was acquired, either freshly or as a same-owner re-entry, and 0 means it is held by someone else. A re-entry rewrites the value with SET ... XX, so it refreshes the TTL when the caller passed expire and drops any remaining TTL when it did not — the same thing develop already does when re-entering with awaitQueue: false. There is no new return code, and the anonymous owner "*" still cannot re-enter, since it is not an identity.

acquire_ iterates [...new Set(keys)].sort() instead of mapping over the caller's array in parallel. Each key gets its own backoff counter; previously a single retryDelay was shared and mutated by every key being acquired concurrently, so the delay a given key saw depended on how many siblings had already retried. The call to the script now passes three arguments instead of four. The error thrown on contention is unchanged, including the logical (unprefixed) key in its message.

releaseAll builds one pipeline of releaseLock calls, one per scanned key, and executes it: two Redis round trips per SCAN batch instead of three, since the SCAN is now followed by a single pipeline rather than by a read pipeline and then a delete pipeline. The owner check and the delete are fused inside each script invocation. The SCAN pattern, the batch size, and the "*" owner default are untouched. release is not changed at all; it already issued one atomic releaseLock per key, and running those in parallel is correct because each one is self-contained.

Testing — How have these changes been tested, or how can the reviewer test the feature?

Four unit tests added to src/services/__tests__/redis-lock.spec.ts, against a mocked Redis client:

a multi-key acquire deduplicates its keys, sorts them, and issues one three-argument acquireLock per key in that order;
a multi-key acquire stops at the first unavailable key and does not attempt the keys after it;
a first attempt that succeeds under awaitQueue: true resolves without any backoff delay;
releaseAll builds a single pipeline of releaseLock calls, one per scanned key, and executes it once.

Three integration tests added to integration-tests/__tests__/index.spec.ts, against a real Redis through the locking module:

an owner re-entering its own lock with awaitQueue: true resolves instead of running into the test timeout, which is what it runs into for as long as the awaitQueue branch skips the same-owner check;
releaseAll releases only the calling owner's keys and leaves another owner's lock intact;
two callers requesting the same two keys in opposite orders under awaitQueue: true both complete, and the same test deadlocks when the sorted acquisition is removed.

cd packages/modules/providers/locking-redis
yarn test
REDIS_URL=redis://localhost:6379 yarn test:integration

All pre-existing unit and integration tests pass unchanged.

Examples

Two callers asking for an overlapping key set in opposite orders, each releasing the set when it is done with it:

const run = async (keys: string[], ownerId: string) => {
  await locking.acquire(keys, { ownerId, expire: 10, awaitQueue: true })
  try {
    // work that needs both carts
  } finally {
    await locking.release(keys, { ownerId })
  }
}

await Promise.all([
  run(["cart_1", "cart_2"], "job-a"),
  run(["cart_2", "cart_1"], "job-b"),
])
// Before: job-a holds cart_1, job-b holds cart_2, and each backs off against
//         the key the other holds, so neither one ever reaches its release.
// After:  both callers take the set in the same sorted order, so one of them
//         runs and releases while the other waits, and both complete.

An owner re-entering a lock it already holds:

await locking.acquire("cart_1", { ownerId: "wf-1", expire: 60 })

// later, in the same unit of work
await locking.acquire("cart_1", { ownerId: "wf-1", expire: 60, awaitQueue: true })
// Before: backs off against its own lock until the timeout.
// After:  resolves immediately and refreshes the TTL, exactly like the
//         same call with awaitQueue: false always did.

Checklist

I have added a changeset for this PR
The changes are covered by relevant tests
I have verified the code works as intended locally
I have linked the related issue(s) if applicable

Additional Context

Known limitation, deliberately left in place

A multi-key acquire that fails does not release the keys it already took. They stay held until their TTL expires, or forever when the caller passed no expire, since the script then issues SET NX without EX. This is the current behaviour and this PR does not change it, in either direction.

It is worth being explicit about why it is not fixed here, because the obvious fix does not work. Rolling back by compare-and-deleting on the stored owner is unsafe: if one of the keys the call took expires, and a different call sharing the same ownerId acquires it before the rollback runs, the owner comparison succeeds against that second call's lease and the rollback deletes it. Sharing an ownerId across concurrent units of work is normal and supported, so this is not a corner case; the rollback would trade a leaked key for a lock silently taken away from a live holder, which is strictly worse. Telling the two acquisitions apart requires an exact per-acquisition token, and the value stored under a lock key today is a single owner string with nowhere to put one.

The larger design that does fix it

There is a design that closes this properly. It is a protocol change, so it belongs in front of maintainers as its own change rather than inside a bug fix.

Storing a per-call acquisition token alongside the owner makes the rollback exact: a call withdraws only the acquisition it performed itself, so it can never revoke a lease that expired and was re-acquired, by the same owner or anyone else. The same token additionally allows a commit step once every key in the set is held, which verifies that no lease was lost mid-flight and fails the call instead of letting it proceed believing it holds a set it no longer holds.

A full prototype of that protocol exposed the following compatibility and operational costs, which are what reviewers should weigh before asking for it:

a companion key per lock, carrying the per-call claims and the lock's lifetime, roughly doubling the number of keys the provider creates;
a new key namespace, so a Redis user restricted by a key-pattern ACL has to have that pattern widened before the provider works at all;
new provider-internal script commands and changed signatures for the existing ones, so any out-of-tree code driving redisClient.acquireLock or redisClient.releaseLock directly stops working;
rejection at construction of clients configured with an ioredis keyPrefix, because a client-side prefix breaks the slot alignment the scripts declare; for a deployment that sets one today, that is a boot failure rather than a degraded path;
rejection at acquisition of lock key shapes that cannot host a slot-aligned companion key, which is a new INVALID_ARGUMENT path reachable from any caller that passes a key it has not validated;
roughly three Redis exchanges per key acquired instead of one, arranged as N+1 latency waves for N keys, which is paid by exactly the multi-key callers it protects, including the inventory and reservation workflow steps;
no mixed-version safety: an old node's re-entry leaves no trace a new node can see, so old and new provider nodes cannot run against the same Redis, and rolling deploys are not supported.

That is a real protocol change with an operational story attached, not a patch. If maintainers want the rollback guarantee and consider those costs acceptable, I am happy to open it as a separate PR or write it up as an RFC first.

Fixes #16285

changeset-bot · 2026-08-02T11:12:12Z

🦋 Changeset detected

Latest commit: 088b073

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 79 packages

Name	Type
@medusajs/locking-redis	Patch
@medusajs/medusa	Patch
@medusajs/test-utils	Patch
@medusajs/loyalty-plugin	Patch
@medusajs/medusa-oas-cli	Patch
integration-tests-http	Patch
@medusajs/analytics	Patch
@medusajs/api-key	Patch
@medusajs/auth	Patch
@medusajs/caching	Patch
@medusajs/cart	Patch
@medusajs/currency	Patch
@medusajs/customer	Patch
@medusajs/file	Patch
@medusajs/fulfillment	Patch
@medusajs/index	Patch
@medusajs/inventory	Patch
@medusajs/link-modules	Patch
@medusajs/locking	Patch
@medusajs/notification	Patch
@medusajs/order	Patch
@medusajs/payment	Patch
@medusajs/pricing	Patch
@medusajs/product	Patch
@medusajs/promotion	Patch
@medusajs/rbac	Patch
@medusajs/region	Patch
@medusajs/sales-channel	Patch
@medusajs/settings	Patch
@medusajs/stock-location	Patch
@medusajs/store	Patch
@medusajs/tax	Patch
@medusajs/translation	Patch
@medusajs/user	Patch
@medusajs/workflow-engine-inmemory	Patch
@medusajs/workflow-engine-redis	Patch
@medusajs/draft-order	Patch
@medusajs/oas-github-ci	Patch
@medusajs/cache-inmemory	Patch
@medusajs/cache-redis	Patch
@medusajs/event-bus-local	Patch
@medusajs/event-bus-redis	Patch
@medusajs/analytics-local	Patch
@medusajs/analytics-posthog	Patch
@medusajs/auth-emailpass	Patch
@medusajs/auth-github	Patch
@medusajs/auth-google	Patch
@medusajs/caching-redis	Patch
@medusajs/file-local	Patch
@medusajs/file-s3	Patch
@medusajs/fulfillment-manual	Patch
@medusajs/locking-postgres	Patch
@medusajs/notification-local	Patch
@medusajs/notification-sendgrid	Patch
@medusajs/payment-stripe	Patch
@medusajs/core-flows	Patch
@medusajs/framework	Patch
@medusajs/js-sdk	Patch
@medusajs/modules-sdk	Patch
@medusajs/orchestration	Patch
@medusajs/query	Patch
@medusajs/types	Patch
@medusajs/utils	Patch
@medusajs/workflows-sdk	Patch
@medusajs/http-types-generator	Patch
@medusajs/cli	Patch
@medusajs/deps	Patch
@medusajs/eslint-plugin	Patch
@medusajs/telemetry	Patch
@medusajs/admin-bundler	Patch
@medusajs/admin-sdk	Patch
@medusajs/admin-shared	Patch
@medusajs/admin-vite-plugin	Patch
@medusajs/dashboard	Patch
@medusajs/icons	Patch
@medusajs/toolbox	Patch
@medusajs/ui-preset	Patch
create-medusa-app	Patch
@medusajs/ui	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

medusa-os-bot · 2026-08-02T11:20:05Z