Use CAS deletion in get_and_delete to prevent race conditions

[ShivanshShukla]

Summary

Fixes the race condition in MemcachedBackend.get_and_delete() where a concurrent writer could store a new value between get and delete, causing get_and_delete() to delete the new value from the cache while returning the stale old value.

Part of #119.

Changes

• Atomic Deletion via CAS: Refactored MemcachedBackend.get_and_delete() from a naive get + delete(noreply=False) sequence to gets followed by cas(..., -1, noreply=False) (mirroring delete_if_equals).
• Differentiated CAS Outcome Handling:
  • True: CAS won and atomically expired the item immediately; returns decode_entry(raw).
  • None: Key was deleted or expired concurrently before CAS; returns None.
  • False: A concurrent writer replaced the value since gets. Retries gets + cas in a bounded loop (_CAS_MAX_RETRIES = 16) to retrieve and delete the latest value, ensuring behavior parity with Redis GETDEL.
• Focused Scope: Kept strictly to get_and_delete() without touching increment() or multi-step worker consolidation (Memcached: run each multi-step operation in one worker call #176).
• Tests:
  • Unit tests verifying:
    • Retrying and successfully returning the updated value when another writer updates the key between gets and cas.
    • Returning None when the key is deleted before cas.
    • Bounded termination returning None if writes continue past _CAS_MAX_RETRIES.
  • Live server regression test simulating concurrent write between gets and cas on Memcached.
• Documentation & Changelog:
  • Added entry under ### Fixed in CHANGELOG.md.
  • Updated documentation in docs/BACKENDS.md, docs/STATE.md, CLAUDE.md, and Traditional Chinese translations.

Verification

• Ran unit & regression tests: pytest tests/backends/test_memcached.py -k "get_and_delete" (all passed).
• Ran full test suite: pytest (704 passed, 0 failed).
• Validated linter and formatting: ruff check and ruff format --check (clean).
• Validated type annotations: mypy (0 errors across 82 files).

[allen0099]

Thanks @ShivanshShukla, this is a solid change. I checked it locally, merged into current master against live Redis and Memcached. The full suite passes. Your tests also fail when I revert get_and_delete to the old get + delete, and when I drop the retry on False.

A few things before merging:

1. Raise when the retries run out, instead of returning None

When all 16 attempts lose to a concurrent writer, the key still holds a value and was not deleted. Returning None tells the caller "there was nothing there", and every caller acts on that:

• CacheManager.delete() returns False ("key did not exist"), but the key is still there.
• invalidate() treats the entry as already gone, so the stale response keeps being served.
• StateManager.consume_state() / delete_state() report the state as missing, but it stays consumable.

We just removed this kind of made-up answer from the Memcached backend in #216 (#197): when it cannot do what was asked, it raises. Please raise CacheXError here too, with a message that names the key and the number of attempts, and keep the bound so the call cannot spin forever. test_memcached_get_and_delete_exhausts_retries_when_writes_continue would then assert the raise (pytest.raises(CacheXError)) instead of None. The logger.warning can go, since the exception carries the same information.

2. Rebase onto master

The PR conflicts with #216, which added _DEAD_TIMEOUT next to your _CAS_MAX_RETRIES and an import at the same place in tests/backends/test_memcached.py. Keep both sides. Afterwards, ruff check will ask you to re-sort the imports in the test file.

3. Link the right issue

• In the PR description, replace "Part of #119" with Closes #175, so the issue closes on merge.
• The CHANGELOG entry links #119. Please point it at #175.
• Please move the entry to the end of the ### Fixed list rather than the top; new entries are appended there. Its wording also needs to mention the raise from point 1.

4. Nit: use monkeypatch in the live test

test_memcached_get_and_delete_live_concurrent_write_regression assigns memcached_backend.client.gets directly. The fixture is per-test, so nothing leaks, but monkeypatch.setattr(memcached_backend.client, "gets", ...) matches the other tests in the file.

Thanks again!

[ShivanshShukla]

Here is the updated summary:

Summary

Fixes the race condition in MemcachedBackend.get_and_delete() where a concurrent writer could store a new value between get and delete, causing get_and_delete() to delete the new value from the cache while returning the stale old value.

Closes #175.

Changes

• Atomic Deletion via CAS: Refactored MemcachedBackend.get_and_delete() from a naive get + delete(noreply=False) sequence to gets followed by cas(..., -1, noreply=False) (mirroring delete_if_equals).
• Differentiated CAS Outcome Handling:
  • True: CAS won and atomically expired the item immediately; returns decode_entry(raw).
  • None: Key was deleted or expired concurrently before CAS; returns None.
  • False: A concurrent writer replaced the value since gets. Retries gets + cas in a bounded loop (_CAS_MAX_RETRIES = 16) to retrieve and delete the latest value, ensuring behavior parity with Redis GETDEL.
  • Exhaustion: Raises CacheXError naming the key and attempt count if retries run out, ensuring caller logic is not misled by a false None.
• Focused Scope: Kept strictly to get_and_delete() without touching increment() or multi-step worker consolidation (Memcached: run each multi-step operation in one worker call #176).
• Tests:
  • Unit tests verifying:
    • Retrying and returning the updated value when another writer updates the key between gets and cas.
    • Returning None when the key is deleted before cas.
    • Raising CacheXError when writes continuously fail CAS after _CAS_MAX_RETRIES attempts.
  • Live server regression test with monkeypatch simulating concurrent writes between gets and cas on Memcached.
• Documentation & Changelog:
  • Appended entry under ### Fixed in CHANGELOG.md linking Memcached get_and_delete can return a value it did not delete #175.
  • Updated documentation in docs/BACKENDS.md, docs/STATE.md, CLAUDE.md, and Traditional Chinese translations.

Verification

• Rebased cleanly onto latest upstream/master.
• pytest tests/backends/test_memcached.py tests/test_changelog_release.py: 50 passed, 40 skipped.
• ruff check and ruff format --check: all checks passed, formatted cleanly.
• mypy: 0 errors across 82 source files.

[allen0099]

Thanks, all points addressed.