fix(lock): reacquire must not delete a persistent (timeout=0) lock

[Mukller]

Fixes #4279

Root cause

A Lock(timeout=0) acquires via SET key token NX without PX — the key is persistent by design ("hold until released"). But LUA_REACQUIRE_SCRIPT unconditionally ran redis.call('pexpire', KEYS[1], ARGV[2]), and do_reacquire() passed timeout = int(0 * 1000) == 0. PEXPIRE key 0 deletes the key immediately, so reacquire() returned True while silently destroying the lock.

(The extend() path turned out to be already protected: its Lua script reads PTTL first and bails with 0 when it's negative, which surfaces as a loud LockNotOwnedError instead of deleting anything — covered by an added regression test regardless.)

Changes

• LUA_REACQUIRE_SCRIPT: skip the PEXPIRE when the requested TTL is "0" — reacquiring a persistent lock now only re-verifies ownership and keeps the key without expiry.

Testing

Two regression tests mirroring the issue repro:

• test_reacquire_lock_zero_timeout_keeps_lock_persistent — reacquire must keep the key alive and persistent (PTTL == -1), and release() must still fully remove it.
• test_extend_replace_ttl_on_zero_timeout_lock_raises_not_delete — pins the existing safe behavior of extend(..., replace_ttl=True) against regressions.

These live in tests/test_lock.py alongside the rest of the lock suite (they need the redis test service, same as the other cases there).

---

Note

Medium Risk
Corrects distributed lock TTL handling so reacquire no longer deletes persistent locks; a small Lua change with high impact if wrong.

Overview
Fixes reacquire() deleting persistent timeout=0 locks. LUA_REACQUIRE_SCRIPT used to always call PEXPIRE with the timeout, and PEXPIRE 0 immediately removes the key while still returning success.

Reacquire now skips expiry when the TTL argument is "0", so ownership is re-checked and the key stays without TTL.

Adds regression tests for that reacquire path and for extend(..., replace_ttl=True) on a zero-timeout lock (already raises, must not delete). Note: those tests were added on TestLockClassSelection, which has no get_lock helper (that lives on TestLock).

^{Reviewed by Cursor Bugbot for commit c55e6f3. Bugbot is set up for automated code reviews on this repo. Configure here.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c55e6f3812

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Put the new tests in a class with get_lock

These new test methods are under TestLockClassSelection, which does not define or inherit the get_lock helper from TestLock; when pytest reaches this line it raises AttributeError before the regression is exercised. Move both zero-timeout tests into TestLock or instantiate with r.lock(..., lock_class=Lock) so the suite can pass and the coverage is effective.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Mirror the zero-timeout fix into asyncio

This patch fixes only the synchronous lock script; redis/asyncio/lock.py still calls pexpire unconditionally in its LUA_REACQUIRE_SCRIPT, so await client.lock("foo", timeout=0).reacquire() will still delete a persistent lock while returning success for asyncio users. Please apply the same guard and add the async regression coverage to keep the mirrored lock implementations aligned.

AGENTS.md reference: AGENTS.md:L146-L149

Useful? React with 👍 / 👎.

[cursor]

Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.

^{Reviewed by Cursor Bugbot for commit c55e6f3. Configure here.}

[cursor]

Async reacquire still deletes locks

High Severity

The sync LUA_REACQUIRE_SCRIPT now skips pexpire when the TTL is 0, but the async lock still always runs pexpire. Calling reacquire() on an asyncio Lock with timeout=0 still deletes the key while returning success—the same failure this PR aims to fix. Project guidance requires sync and async lock behavior to stay aligned.

 

^{Reviewed by Cursor Bugbot for commit c55e6f3. Configure here.}

[cursor]

Regression tests call missing helper

Medium Severity

The new regression tests live on TestLockClassSelection and call self.get_lock, but that helper exists only on TestLock. These cases fail with AttributeError instead of exercising the fix, so the claimed coverage for issue #4279 never actually runs.

 

^{Reviewed by Cursor Bugbot for commit c55e6f3. Configure here.}

[petyaslavova]

Hey @Mukller, thank you for your contribution!

Thanks for both the report in #4279 and this patch. We are going to move forward with #4282 for this fix, which you have already approved. It enforces the same invariant inside the scripts but covers more of it: extend() as well as reacquire(), negative and computed non-positive TTLs rather than only the literal "0", and both the sync and async stacks, which is a requirement here since redis/asyncio/lock.py carries the same script.

One note for the record: the two tests added here were appended to TestLockClassSelection, which has no get_lock helper, so they fail with AttributeError rather than verifying the fix.

Your extend(10, replace_ttl=True) case is still worth keeping - it pins that a positive replace-TTL extend on a persistent lock keeps raising. If you would like to add it to #4282 or open a small follow-up with it, that would be very welcome.

Closing this one in favor of #4282.