diff --git a/changelog.d/315.fixed.md b/changelog.d/315.fixed.md new file mode 100644 index 0000000..b9dba6b --- /dev/null +++ b/changelog.d/315.fixed.md @@ -0,0 +1,7 @@ +**`MemcachedBackend.increment()` no longer fails when a new short-lived counter expires mid-call.** +Creating a counter takes an `ADD` and then an `INCR`, and Memcached keeps time +in whole seconds, so a counter created with `ttl=1` could expire between the +two and `increment()` raised `CacheXError("Counter vanished between ADD and +INCR")`. The `ADD` + `INCR` pair is now retried up to 16 times in the same +worker call, starting a new window at `delta`; `CacheXError` is raised only if +the counter vanishes on every attempt. diff --git a/docs/BACKENDS.md b/docs/BACKENDS.md index a5ed325..7dc5ea2 100644 --- a/docs/BACKENDS.md +++ b/docs/BACKENDS.md @@ -243,8 +243,11 @@ if await backend.set_if_absent(f"stream:{user_id}", owner, ttl=300): - `increment(key, delta=1, ttl=None) -> int` — Memory does the read-modify-write under its lock, Redis runs a Lua script (`EXISTS` + `INCRBY` + `EXPIRE`) and - Memcached uses `ADD` + `INCR`/`DECR` (Memcached counters stop at 0). The - counter is visible through `get()` as a `CacheEntry` with fingerprint + Memcached uses `ADD` + `INCR`/`DECR` (Memcached counters stop at 0). + Memcached keeps time in whole seconds, so a new counter with a short `ttl` + can expire between its `ADD` and the `INCR`; Memcached then retries `ADD` + + `INCR`, starting a new window at `delta`, and raises `CacheXError` only if + the counter vanishes on all 16 attempts. The counter is visible through `get()` as a `CacheEntry` with fingerprint `COUNTER_FINGERPRINT` and the decimal value as content, so `delete`/`clear*` and the monitoring routes treat it like any other entry. Incrementing a key that holds anything else raises `CacheXError` on every backend, even a cached diff --git a/fastapi_cachex/backends/memcached.py b/fastapi_cachex/backends/memcached.py index 0edc2c3..50b01ee 100644 --- a/fastapi_cachex/backends/memcached.py +++ b/fastapi_cachex/backends/memcached.py @@ -316,20 +316,42 @@ def _add_delta(self, prefixed_key: str, delta: int) -> int | None: return None if result is None else int(result) def _increment(self, prefixed_key: str, delta: int, exptime: int) -> int | None: - """Run ``increment``'s INCR, ADD and retried INCR in one worker thread.""" + """Run ``increment``'s INCR, ADD and retried INCR in one worker thread. + + Returns ``None`` only when the counter vanished after every one of + ``_CAS_MAX_RETRIES`` ADD + INCR attempts. + """ value = self._add_delta(prefixed_key, delta) - if value is None: + if value is not None: + return value + for _ in range(_CAS_MAX_RETRIES): # No counter yet: ADD is atomic and a no-op when a concurrent - # call created it first, so the retry always finds a counter. + # call created it first. self.client.add(prefixed_key, b"0", exptime, noreply=False) value = self._add_delta(prefixed_key, delta) - return value + if value is not None: + return value + # Memcached keeps time in whole seconds, so a counter created with + # a short ttl can expire before the INCR that follows its ADD. It + # expired inside its own window, so the next window starts again + # at ``delta``. + logger.debug("Memcached INCREMENT RETRY; key=%s", prefixed_key) + return None async def increment(self, key: str, delta: int = 1, ttl: int | None = None) -> int: """Atomically add ``delta`` to the counter at ``key`` (see base class). Memcached counters are unsigned, so a negative ``delta`` uses DECR, which stops at 0 instead of going negative. + + Creating a counter takes an ADD and then an INCR. If the new counter + expires in between (Memcached's clock has one-second resolution, so a + ``ttl=1`` counter can live for well under a second), the ADD + INCR + pair is retried, starting a new window at ``delta``. + + Raises: + CacheXError: If the key holds a value that is not a counter, or + the counter vanished after every ADD + INCR attempt. """ validate_delta(delta) validate_ttl(ttl) @@ -348,7 +370,7 @@ async def increment(self, key: str, delta: int = 1, ttl: int | None = None) -> i msg = "Cache key holds a value that is not a counter" raise CacheXError(msg) from e if value is None: - msg = "Counter vanished between ADD and INCR" + msg = f"Counter vanished between ADD and INCR on each of {_CAS_MAX_RETRIES} attempts" raise CacheXError(msg) logger.debug("Memcached INCREMENT; key=%s value=%s ttl=%s", key, value, ttl) return value diff --git a/i18n/zh-TW/docs/BACKENDS.md b/i18n/zh-TW/docs/BACKENDS.md index 21bad85..0223c7d 100644 --- a/i18n/zh-TW/docs/BACKENDS.md +++ b/i18n/zh-TW/docs/BACKENDS.md @@ -176,7 +176,7 @@ if await backend.set_if_absent(f"stream:{user_id}", owner, ttl=300): await backend.delete_if_equals(f"stream:{user_id}", owner) ``` -- `increment(key, delta=1, ttl=None) -> int`:記憶體後端在鎖內執行讀取—修改—寫入,Redis 執行 Lua 腳本(`EXISTS` + `INCRBY` + `EXPIRE`),Memcached 則使用 `ADD` + `INCR`/`DECR`(Memcached 的計數器最低停在 0)。計數器可透過 `get()` 讀到,形式為 fingerprint 為 `COUNTER_FINGERPRINT`、內容為十進位數值的 `CacheEntry`,因此 `delete`/`clear*` 與監控路由都會把它當成一般項目處理。對存放其他內容的鍵執行 increment,在每個後端上都會拋出 `CacheXError`,即使是本文剛好是數字的快取回應也一樣。以 `set(key, counter_entry(n))` 寫入的計數器在每個後端上都可以 increment,唯一的例外是 Memcached 的計數器沒有正負號:在它上面 `n` 必須介於 0 到 2**64 - 1 之間,對負數的計數器執行 increment 會拋出 `CacheXError`。`delta` 必須是 signed 64 位元範圍內的 `int`,否則會在存取後端之前拋出 `TypeError` 或 `ValueError`。 +- `increment(key, delta=1, ttl=None) -> int`:記憶體後端在鎖內執行讀取—修改—寫入,Redis 執行 Lua 腳本(`EXISTS` + `INCRBY` + `EXPIRE`),Memcached 則使用 `ADD` + `INCR`/`DECR`(Memcached 的計數器最低停在 0)。Memcached 以整秒計時,因此 `ttl` 很短的新計數器可能在 `ADD` 與 `INCR` 之間就過期;此時 Memcached 會重試 `ADD` + `INCR`,從 `delta` 開始新的時間窗,只有連續 16 次嘗試計數器都消失時才拋出 `CacheXError`。計數器可透過 `get()` 讀到,形式為 fingerprint 為 `COUNTER_FINGERPRINT`、內容為十進位數值的 `CacheEntry`,因此 `delete`/`clear*` 與監控路由都會把它當成一般項目處理。對存放其他內容的鍵執行 increment,在每個後端上都會拋出 `CacheXError`,即使是本文剛好是數字的快取回應也一樣。以 `set(key, counter_entry(n))` 寫入的計數器在每個後端上都可以 increment,唯一的例外是 Memcached 的計數器沒有正負號:在它上面 `n` 必須介於 0 到 2**64 - 1 之間,對負數的計數器執行 increment 會拋出 `CacheXError`。`delta` 必須是 signed 64 位元範圍內的 `int`,否則會在存取後端之前拋出 `TypeError` 或 `ValueError`。 - `get_and_delete(key) -> CacheEntry | None`:記憶體後端在鎖內 pop,Redis 使用 `GETDEL`(伺服器 6.2 以上),Memcached 使用 `GETS` + `exptime=-1` 的 `CAS` 寫入(若中間有其他寫入者替換了值則會重試;連續 16 次都被替換時會拋出 `CacheXError`,而不是當成鍵不存在)。`StateManager.consume_state`、`StateManager.delete_state`、`CacheManager.delete` 與 `invalidate()` 都建立在它之上。 - `set_if_absent(key, value, ttl=None) -> bool`:只在 `key` 不存在時儲存 `value`(已過期的鍵視為不存在),並回報是否有寫入。記憶體後端在鎖內檢查,Redis 使用 `SET NX EX`,Memcached 使用 `ADD`。 - `delete_if_equals(key, expected) -> bool`:只在 `key` 仍存放 `expected` 時才移除它,因此項目已過期的持有者無法釋放已被他人取得的鎖。請在你儲存的項目中放入唯一的權杖,並以同一個項目釋放。記憶體後端在鎖內比較,Redis 透過 Lua 腳本刪除,並在腳本中重新檢查先前比較過的值,Memcached 則使用 `GETS` + 一個讓項目立即過期的 `CAS` 寫入(傳統協定的 `DELETE` 不接受 CAS 權杖)。 diff --git a/tests/backends/test_memcached.py b/tests/backends/test_memcached.py index d062432..0fea524 100644 --- a/tests/backends/test_memcached.py +++ b/tests/backends/test_memcached.py @@ -438,12 +438,23 @@ async def test_memcached_increment_decrement_stops_at_zero( async def test_memcached_increment_honors_ttl( memcached_backend: MemcachedBackend, ) -> None: - await memcached_backend.increment("window", ttl=1) - await memcached_backend.increment("window", ttl=3600) - await asyncio.sleep(2) + """The ttl of the first call sets the window; later ttls do not extend it. - assert await memcached_backend.get("window") is None - assert await memcached_backend.increment("window", ttl=1) == 1 + Memcached keeps time in whole seconds, so an item stored with exptime N + lives somewhere between N - 1 and N + 1 seconds. ttl=2 keeps the counter + alive across the next call, and polling with a deadline waits for expiry + without depending on where the clock tick lands. + """ + assert await memcached_backend.increment("window", ttl=2) == 1 + assert await memcached_backend.increment("window", ttl=3600) == 2 + + loop = asyncio.get_running_loop() + deadline = loop.time() + 5 + while await memcached_backend.get("window") is not None: + assert loop.time() < deadline, "counter outlived its 2 s window" + await asyncio.sleep(0.2) + + assert await memcached_backend.increment("window", ttl=2) == 1 async def test_memcached_increment_only_maps_non_numeric_errors_to_not_a_counter() -> ( @@ -722,17 +733,42 @@ async def spaced() -> dict[str, str]: async def test_increment_reports_a_counter_that_vanished_mid_call() -> None: """ADD then INCR is two round-trips; the entry can expire in between. - Memcached has no way to make the pair atomic, so `increment` has to - surface the loss instead of returning `None` as if it were a count. + Memcached has no way to make the pair atomic, so `increment` retries a + bounded number of times and then surfaces the loss instead of returning + `None` as if it were a count. """ backend = stubbed_backend() - # INCR keeps missing: the key is gone again by the time ADD's retry runs. + # INCR keeps missing: the key is gone again by the time each retry runs. backend.client.incr.return_value = None - with pytest.raises(CacheXError, match="Counter vanished between ADD and INCR"): - await backend.increment("k") + with pytest.raises( + CacheXError, + match=f"Counter vanished between ADD and INCR on each of {_CAS_MAX_RETRIES} attempts", + ): + await backend.increment("k", ttl=1) + + assert backend.client.add.call_count == _CAS_MAX_RETRIES + # The first INCR, then one after every ADD. + assert backend.client.incr.call_count == _CAS_MAX_RETRIES + 1 + + +async def test_increment_retries_when_the_new_counter_expires_before_incr() -> None: + """Issue #315: a ttl=1 counter can expire between its ADD and the INCR. + + Memcached's clock has one-second resolution, so the tick can land between + the two. The counter expired inside its own window, so the retry starts + a new window at `delta`. + """ + backend = stubbed_backend() + # Miss (no counter), miss (expired right after ADD), then the retry lands. + backend.client.incr.side_effect = [None, None, 3] + + assert await backend.increment("k", 3, ttl=1) == 3 - assert backend.client.add.call_count == 1 + assert backend.client.add.call_count == 2 + for call in backend.client.add.call_args_list: + assert call.args == ("fastapi_cachex:k", b"0", 1) + assert backend.client.incr.call_count == 3 @requires_memcached