fix(lock): preserve zero-timeout locks on renewal

[daleselaji-dev]

Problem

Lock(timeout=0) represents a lock that remains held until explicitly released, but renewal calls with a non-positive target TTL could invoke PEXPIRE with zero and delete the lock while reporting success.

Root Cause

The renewal Lua scripts did not consistently guard non-positive TTL values before calling PEXPIRE. The reacquire script always called PEXPIRE, and the extend path did not treat a zero replacement as a no-op.

Solution

Skip PEXPIRE when the requested or calculated TTL is non-positive, while still verifying that the caller owns the lock and returning success for a safe no-op.

Changes

• Apply the guard to synchronous and asyncio lock renewal scripts.
• Add synchronous and asyncio regression tests for zero-timeout extend(..., replace_ttl=True) and reacquire().

Testing

• py -3.10 -m pytest tests/test_lock.py tests/test_asyncio/test_lock.py --redis-url redis://localhost:16379/0 -q — 102 passed.
• New regression selection — 6 passed.
• py -3.10 -m ruff check redis/lock.py redis/asyncio/lock.py tests/test_lock.py tests/test_asyncio/test_lock.py — passed.
• py -3.10 -m compileall -q redis/lock.py redis/asyncio/lock.py — passed.
• git diff --check — passed.
• Ruff format check was not clean on the four existing files because it would reformat unrelated pre-existing content.
• Mypy was not clean because the repository baseline reports unrelated dependency and cross-package type errors.

Compatibility/Risk

The change preserves ownership checks and the existing positive-TTL behavior. It prevents destructive expiry operations for non-positive TTLs in both sync and asyncio locks.

Notes for Reviewer

The integration tests require Redis; this validation used the available Redis container on port 16379. The default localhost:6379 endpoint was unavailable in the local environment.

Linked Issue

Fixes #4279

---

Note

Medium Risk
Touches distributed lock renewal in Redis Lua; behavior change is narrowly scoped but affects correctness for zero-timeout and edge-case TTL renewals.

Overview
Fixes renewal accidentally dropping locks when extend or reacquire would set a non-positive TTL. Redis PEXPIRE with 0 removes the key, so Lock(timeout=0) and “replace TTL with 0” paths could report success while the lock was gone.

The sync and asyncio lock Lua scripts now no-op the expiry update when the computed or requested TTL is <= 0, after the usual ownership check, and still return success so the key and token stay intact. reacquire only calls PEXPIRE when the timeout in milliseconds is positive. extend returns early without PEXPIRE when the new TTL would be non-positive (including negative add-ons or zero replace_ttl on timed locks).

Docstrings for extend and reacquire describe this behavior. Matching regression tests were added in tests/test_lock.py and tests/test_asyncio/test_lock.py.

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

[Mukller]

I filed the underlying #4279 and reviewed the Lua changes line by line against the failure mode from my report (Lock(timeout=0) + renewal → PEXPIRE key 0 deletes the lock while reporting success):

1. REACQUIRE: expiration == -1 and ARGV[2] == "0" → return 1 — a no-expiry lock being renewed with timeout 0 now keeps its no-TTL state instead of receiving PEXPIRE 0. Correct: preserves the documented "remains locked until release()".
2. EXTEND (replace_ttl): tonumber(newttl) <= 0 → return 1 without pexpire — prevents deletion when the computed replacement TTL lands at ≤0. Safe-by-default choice: the lock is never silently destroyed, even for edge inputs beyond the exact timeout=0 case.
3. Third script's tonumber(ARGV[2]) > 0 guard around pexpire — blocks the raw PEXPIRE key 0 that constituted the original destruction path.

The added tests assert exactly the right invariants (token preserved, PTTL stays -1, key still exists after renewal) — they require a live Redis, so I could not execute them locally; CI green on this branch covers that leg. My client-side mock harness can only confirm the arguments still reach the scripts unchanged ([token, 0]), which is expected — the guard lives inside the script.

One docs nit, non-blocking: with this change, extend(0) on a TTL'd lock becomes a no-op returning True (previously it deleted the lock via PEXPIRE 0 — arguably also a bug, but different behavior). Worth one changelog sentence noting both interpretations are now "keep current state".

Approving — this restores the documented invariant that timeout=0 locks live until release().

[petyaslavova]

Hey @daleselaji-dev, thank you for your contribution!

This is the right shape for #4279: enforcing "a renewal must never call pexpire with a non-positive TTL" inside the scripts covers reacquire(), extend(..., replace_ttl=True), and negative or computed TTLs in one place, and the fix is mirrored in the async stack with tests on both sides.

Two things before we merge:

1. The tonumber(newttl) <= 0 guard is not covered by the new tests. Both extend tests use timeout=0, so they return early at the expiration == -1 and ARGV[2] == "0" branch and never reach it. Please add a regression test for the case that is actually destructive on master: a lock with a positive timeout (for example timeout=10) where extend(0, replace_ttl=True) must keep the key with its TTL intact instead of deleting it. A negative additional_time case would be welcome too, in both stacks.
2. The new early return checks ARGV[2] but not ARGV[3], so extend(0, replace_ttl=False) on a persistent lock now returns True where it previously raised LockNotOwnedError. Please either gate that branch on ARGV[3] == "1" or make the change deliberate and cover it with a test.

Minor: for consistency, tonumber(ARGV[2]) <= 0 in the first branch would match the numeric comparison used below it. Also note that ruff format --check is clean on these four files on master, so the formatting caveat in the description is likely a local tooling difference.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6d0d00b2db

ℹ️ 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]

Restore persistence when reacquiring zero-timeout locks

When a Lock(timeout=0) key has subsequently received a positive TTL—for example, another operation temporarily calls PEXPIRE—reacquire() should reset it to the configured no-expiry state, just as the existing positive-timeout test expects renewal to restore an externally shortened TTL. This guard instead skips every operation for zero, reports success, and leaves the key expiring; use PERSIST for the zero-timeout case while retaining the ownership check. The mirrored asyncio script has the same issue.

AGENTS.md reference: AGENTS.md:L159-L163

Useful? React with 👍 / 👎.

[petyaslavova]

Thanks for the follow-up. The tonumber(newttl) <= 0 guard and its two new timeout=10 tests are exactly right - that is the path that actually deletes a lock on master while returning True, and the tests fail there for the right reason. The reacquire guard is right too. Sync/async parity looks good.

One correction to my previous review, and the fault is mine. I offered two options for the expiration < 0 early return: gate it on ARGV[3], or make it deliberate and test it. Having traced the base behavior on a real server, the gate needs to go the other way and the branch should come out. On master, extend(anything, replace_ttl=True) against a persistent lock reads PTTL == -1, returns 0, and raises LockNotOwnedError - it never reaches pexpire, so nothing is deleted on that path. The claim in #4279 that extend(replace_ttl=True) destroys a timeout=0 lock came from a repro whose FakeScript returns 1 without executing the Lua. So the new branch does not fix a defect; it converts a raised LockNotOwnedError into True, which is a breaking change to released error semantics, and we are not taking those right now.

Please drop the three added lines inside the expiration < 0 block in both stacks, and turn test_extend_lock_zero_timeout_replace_ttl_keeps_lock around to assert that extend(0, replace_ttl=True) on a persistent lock still raises LockNotOwnedError and leaves the key in place with PTTL == -1. Your test_extend_lock_zero_timeout_without_replace_raises_error already pins the replace_ttl=False twin. Also please add one sentence to the extend() docstring in both stacks noting that a non-positive resulting TTL leaves the current expiry unchanged and returns True, since the replace_ttl text no longer holds for those values.

One suggestion, not blocking: in the reacquire script, persist rather than a bare skip in the non-positive branch would restore the configured no-expiry state if the key has picked up a TTL from elsewhere. test_reacquire_lock already asserts that behavior for positive timeouts, and persist is a no-op when there is no TTL. Happy to take that as a follow-up if you would rather keep this PR minimal.

Minor: the two new tests named ..._zero_timeout_... and ..._negative_timeout_... use timeout=10 with a zero or negative additional_time, so additional_time in the name would read better.

[petyaslavova]

Thanks for the update - this is now what we asked for. The expiration < 0 branch is out of both stacks, so extend(..., replace_ttl=True) against a persistent lock raises LockNotOwnedError again, and the reversed test pins that with PTTL == -1. The remaining tonumber(newttl) <= 0 and reacquire guards are the two paths that really delete a lock on master while returning True, and the sync/async mirrors match.

Two small things and I think we are done:

1. The additive path is still untested. With replace_ttl=False the script computes newttl = ARGV[2] + expiration, so a timeout=10 lock with extend(-20) sends PEXPIRE key -10000 on master and deletes it while returning True. test_extend_lock_zero_timeout_without_replace_raises_error uses timeout=0, so it exits at the expiration < 0 check and never reaches the guard. Please add one test per stack for that case.
2. Please add the same sentence you added to extend() to the reacquire() docstring in both stacks. It still says only that it resets the TTL back to the timeout value, but for a non-positive TTL it is now a successful no-op - and that is the case #4279 actually reported.

The test renaming to additional_time and the persist idea for the reacquire script are still non-blocking; I am happy to take persist as a follow-up. One more note: the workflows have not run on this branch (only Bugbot reported), so I will trigger CI once the two items above are in.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a37f19f697

ℹ️ 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]

Preserve expiration for non-positive extensions

When an expiring lock calls extend(0, replace_ttl=True) or supplies a negative delta that makes the computed TTL non-positive, this guard changes the longstanding behavior from executing PEXPIRE—which immediately expires the lock—to returning success while leaving the old TTL in place. Since additional_time publicly accepts any Number and previously forwarded these values without validation, callers using a negative extension to shorten or immediately expire a lock will now silently retain it; preserve the prior expiration behavior or explicitly reject such inputs instead of reporting a successful no-op.

AGENTS.md reference: AGENTS.md:L121-L125

Useful? React with 👍 / 👎.