Skip to content

Fence Redis cache fills after invalidation#76

Closed
ChiragAgg5k wants to merge 3 commits into
mainfrom
fix/redis-fenced-cache-fills
Closed

Fence Redis cache fills after invalidation#76
ChiragAgg5k wants to merge 3 commits into
mainfrom
fix/redis-fenced-cache-fills

Conversation

@ChiragAgg5k

@ChiragAgg5k ChiragAgg5k commented Jun 23, 2026

Copy link
Copy Markdown
Member

What

Fixes stale Redis cache fills after invalidation by fencing Redis-backed cache misses internally.

The problematic sequence is:

  1. A reader calls load() and misses.
  2. Another flow writes newer backing data and invalidates the cache with purge() or flush().
  3. The original reader finishes its stale read and calls save().
  4. Without fencing, that stale value can repopulate Redis after invalidation.

This PR keeps the public cache API unchanged. Callers still use load(), save(), purge(), and flush() normally; the cache layer tracks an internal token for Redis miss fills and Redis only accepts the follow-up save when that token is still valid.

Why this is narrow

This intentionally keeps the fix Redis-focused:

  • Only concrete Redis adapters create and validate fenced fill tokens: Redis, RedisCluster, and Redis\Multiplexing.
  • Non-Redis storage adapters are unchanged.
  • Adapter remains unchanged; the new behavior is behind a small Feature\FencedFill capability.
  • Pool, Sharding, and CircuitBreaker only forward FencedFill when their inner adapter supports it, so Redis-backed wrapper deployments keep the same protection.
  • Public method signatures and return types are unchanged.

Changes

  • Adds internal Token and Feature\FencedFill types.
  • Updates Cache to remember fill tokens only when the top-level adapter supports FencedFill.
  • Stores pending tokens by normalized key/hash and bounds them to 1024 entries.
  • Uses Swoole coroutine context for pending tokens when running inside a coroutine, so concurrent coroutine requests do not consume each other’s fill tokens.
  • Clears matching pending tokens after successful same-instance purge(), including all known hashes for key-level purge.
  • Adds Redis Lua scripts for atomic load/token checks, fenced saves, tombstone writes on purge, and tombstone cleanup on normal save.
  • Uses short-lived Redis tombstones to reject stale fills after hash-level or key-level purges.
  • Writes a short-lived Redis flush marker from Redis-family flush() implementations so pre-flush absent tokens cannot save after a flush.
  • Expires absent fill tokens using the same TTL window as tombstones and flush markers.
  • Keeps Redis Cluster tombstone keys hash-tagged to the same slot as the cache key. Redis Cluster checks the flush marker outside the per-key Lua script to avoid cross-slot Lua access, with a post-write cleanup check if a flush wins the race.
  • Adds focused unit coverage for cache token bookkeeping and wrapper forwarding, plus Redis-only E2E coverage for purge and flush fencing scenarios.

Redis behavior

  • Cache hits decode and return existing values.
  • Plain adapter load() still returns false on misses.
  • Cache::load() receives an internal token on Redis misses and returns false to callers.
  • Cache::save() consumes the matching token and calls saveFenced() only for Redis fenced fills.
  • Normal Redis saves still work without requiring a token and atomically clear the field tombstone.
  • A purge writes a short-lived tombstone, so older miss tokens from other requests/processes cannot repopulate a purged value.
  • A successful same-instance purge clears that instance’s pending fill token, so an explicit invalidate-then-save refresh flow can repopulate normally.
  • A flush writes a short-lived marker, so absent tokens issued before the flush are rejected for TOKEN_TTL seconds.

Greptile follow-up

Latest Greptile P1s are now handled:

  • Fence Redis flushes: Redis::flush() and Redis\Multiplexing::flush() now perform FLUSHDB and marker write in one Lua script. RedisCluster::flush() writes a marker after flushing masters and saveFenced() checks that marker before and after the per-key fenced save.
  • Clear purge tokens: successful Cache::purge() now clears pending local fill tokens for the purged key/hash, or all known hashes for key-level purge.

Earlier Greptile notes:

  • Wrapper forwarding: fixed by forwarding FencedFill through Pool, Sharding, and CircuitBreaker when supported by the inner adapter.
  • Absent token lifetime: fixed by expiring absent fill tokens with TOKEN_TTL during fenced saves.
  • Coroutine flush tokens: not changed; flush() increments the shared token generation, so old coroutine-context tokens are no longer looked up by later saves.
  • PHP 8.2 parse error on typed constants: not changed; this repo requires PHP >=8.3 in composer.json.

Validation

Passed locally:

  • php -l on all touched PHP files
  • vendor/bin/phpunit --configuration phpunit.xml tests/Cache/Unit
  • vendor/bin/phpstan analyse --level max src tests --memory-limit 2G
  • vendor/bin/pint --test ... on touched files
  • Temporary local Redis 7 Docker check for standalone Redis flush fencing:
    • verified FLUSHDB is accepted inside EVAL
    • verified the flush marker survives the flush
    • verified a pre-flush miss token is rejected after another cache instance flushes

Could not run the repo Redis E2E files from this host because the expected Docker DNS names are not resolvable here:

  • redis
  • redis-cluster

The Redis E2E attempt failed in setup/connection before assertions ran. Pint passes, but PHP 8.5 emits deprecation noise from Pint’s bundled dependencies.

Risk

Risk is limited to Redis-backed cache behavior and wrappers around Redis-backed adapters. The main compatibility surface is that Redis load(), fenced save(), purge(), and standalone flush() now execute Lua scripts, and Redis invalidations keep short-lived tombstone/flush markers for TOKEN_TTL seconds.

@greptile-apps

greptile-apps Bot commented Jun 23, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds a Redis-level fencing mechanism to prevent stale cache fills after purge() or flush(): load() misses now return an internal Token, and save() routes through an atomic Lua saveFenced() that only writes if no tombstone or flush marker has appeared since the token was issued. The implementation is well-structured — Token, Feature\FencedFill, and Lua scripts for atomic load/save/purge/flush operations land cleanly, and the Pool, Sharding, and CircuitBreaker wrappers correctly forward the capability.

  • The standalone Redis and RedisCluster adapters implement fencing correctly, including flush-marker writes and token TTL expiry checks inside the Lua scripts.
  • The Multiplexing adapter's saveFenced() passes numkeys='3' to EVAL but the Lua script (copied verbatim from Redis.php) accesses KEYS[4] for the flush marker — causing a Lua runtime error on every fenced save and silently making all post-miss fills fail for Multiplexing-backed caches.
  • Cache::flush() unconditionally increments the token generation and clears local tokens regardless of whether the underlying flush() call succeeded; on a failed flush, pending fill tokens are discarded, so the next save() falls through to the plain (unfenced) path.

Confidence Score: 4/5

Safe to merge for Redis and RedisCluster deployments, but Multiplexing-backed caches will have fencing entirely disabled until the numkeys fix lands.

The fencing logic in Redis.php and RedisCluster.php is sound, and the wrapper forwarding is correct. However, the Multiplexing adapter's saveFenced() has a one-character off-by-one in the EVAL numkeys declaration that causes a Lua runtime error on every invocation — meaning any production deployment that uses the Multiplexing adapter will silently fall through to false on every post-miss save, defeating the entire fencing guarantee for that adapter.

src/Cache/Adapter/Redis/Multiplexing.php — the saveFenced() method needs numkeys changed from '3' to '4' before this is safe for Multiplexing-backed deployments.

Important Files Changed

Filename Overview
src/Cache/Adapter/Redis/Multiplexing.php saveFenced() passes numkeys='3' to EVAL but the Lua script accesses KEYS[4] (flush key), causing a Lua error on every fenced save in the Multiplexing adapter.
src/Cache/Adapter/Redis.php Fenced fill correctly implemented — loadOrToken, saveFenced, purge and flush Lua scripts all use the right key/arg offsets (numkeys=4 for saveFenced).
src/Cache/Adapter/RedisCluster.php saveFenced uses numkeys=3 correctly (no in-script flush check); two-phase flush guard (pre + post Lua) handles the cluster cross-slot constraint with an accepted race window.
src/Cache/Cache.php Token bookkeeping (store, consume, forget, prune, coroutine context, purge cleanup, flush generation) is correct; tokenGeneration++ and clearTokens() run unconditionally even on a failed flush (minor edge case).
src/Cache/Token.php New simple value-object wrapping the opaque fill-token string; correct and complete.
src/Cache/Feature/FencedFill.php New interface with loadFenced and saveFenced; signatures match all implementing adapters.
src/Cache/Adapter/Pool.php Correctly delegates loadFenced/saveFenced to the inner adapter when it supports FencedFill, falls back gracefully otherwise.
src/Cache/Adapter/Sharding.php Correctly routes loadFenced/saveFenced to the selected shard adapter when supported.
src/Cache/Adapter/CircuitBreaker.php Correctly forwards loadFenced/saveFenced through the circuit breaker delegate when inner adapter supports FencedFill.
tests/Cache/Unit/FencedFillTest.php Comprehensive unit coverage for token bookkeeping, bounding, purge clearing, and wrapper forwarding via fake adapters.

Reviews (4): Last reviewed commit: "Handle Redis purge and flush fences" | Re-trigger Greptile

Comment thread src/Cache/Cache.php
Comment thread src/Cache/Adapter/Redis.php
@ChiragAgg5k

Copy link
Copy Markdown
Member Author

Follow-up on the two Greptile notes that did not result in code changes:

  • Coroutine flush tokens: I do not think this is a valid stale-fill path in the current implementation. Cache::flush() increments the shared tokenGeneration, and token lookup keys include that generation. A token stored in another Swoole coroutine context under the old generation will not be looked up by a later save() after flush(), even if that coroutine-local array still physically contains the old token.

  • PHP 8.2 parse error for typed constants: not changing this. The package requires PHP >=8.3 in composer.json, and typed class constants are valid for the supported runtime. The local validation for this PR is running on PHP 8.5.

@ChiragAgg5k

Copy link
Copy Markdown
Member Author

Follow-up on the latest Greptile notes that are intentionally not changing this PR:

  • Fence flush invalidations: agreed this is a theoretical distributed race for a request/process that already holds an absent token while another request calls flush(). I am leaving this out of scope for this PR because flush() is a coarse cache-clear/admin/test path, while this PR is scoped to the normal Redis invalidation path via purge(). Fixing distributed flush() fencing cleanly, especially for Redis Cluster hash-slot constraints, would add a broader global invalidation mechanism that I do not want to fold into this narrow stale-fill fix.

  • Invalidate purge tokens: also intentionally not changing. In the same Cache instance, load() -> purge() -> save() returning false is acceptable because the pending save is still tied to the pre-purge miss. A caller that wants to write after an explicit purge can perform a normal save without first carrying a stale miss token, or reload after the purge. The distributed tombstone behavior is the important guarantee here: a pre-purge fill should not silently become an unfenced post-purge write.

@ChiragAgg5k

Copy link
Copy Markdown
Member Author

@greptileai

@ChiragAgg5k

Copy link
Copy Markdown
Member Author

Update after commit 091d544: the previous note saying the flush and purge-token Greptile P1s were intentionally out of scope is now obsolete. Both are handled in this PR now: Redis-family flushes write a short-lived flush fence, and successful same-instance purges clear matching pending fill tokens before the next save.

@ChiragAgg5k

Copy link
Copy Markdown
Member Author

@greptileai

Comment on lines +232 to +245
$result = $this->command([
'EVAL',
$script,
'3',
$key,
$this->getTombstoneKey($key, $hash),
$this->getTombstoneKey($key, '*'),
$this->getFlushKey(),
$hash,
$token->value,
$value,
(string) self::TOKEN_TTL,
(string) \time(),
]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 saveFenced passes numkeys='3' to EVAL but the Lua script reads KEYS[4] for the flush key. In Redis EVAL, elements beyond numkeys are treated as ARGV, so getFlushKey() lands in ARGV[1] instead of KEYS[4], and KEYS[4] is nil. Calling redis.call('GET', nil) raises a Lua error, which causes EVAL to fail on every fenced-save call through the Multiplexing adapter — silently breaking all post-miss saves. Compare with Redis::saveFenced(), which correctly passes numkeys=4.

Suggested change
$result = $this->command([
'EVAL',
$script,
'3',
$key,
$this->getTombstoneKey($key, $hash),
$this->getTombstoneKey($key, '*'),
$this->getFlushKey(),
$hash,
$token->value,
$value,
(string) self::TOKEN_TTL,
(string) \time(),
]);
$result = $this->command([
'EVAL',
$script,
'4',
$key,
$this->getTombstoneKey($key, $hash),
$this->getTombstoneKey($key, '*'),
$this->getFlushKey(),
$hash,
$token->value,
$value,
(string) self::TOKEN_TTL,
(string) \time(),
]);

Comment on lines +232 to +245
$result = $this->command([
'EVAL',
$script,
'3',
$key,
$this->getTombstoneKey($key, $hash),
$this->getTombstoneKey($key, '*'),
$this->getFlushKey(),
$hash,
$token->value,
$value,
(string) self::TOKEN_TTL,
(string) \time(),
]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 The EVAL call declares numkeys='3', but the Lua script reads KEYS[4] to fetch the flush marker (local lastFlush = redis.call('GET', KEYS[4])). With only 3 keys declared, KEYS[4] is nil in Lua, and redis.call('GET', nil) raises a Lua runtime error. As a result, every call to saveFenced() on the Multiplexing adapter fails unconditionally — all post-miss fills go to the false branch — silently disabling fencing for this adapter. The standalone Redis adapter correctly passes 4 here, and the same fix is needed for Multiplexing.

Suggested change
$result = $this->command([
'EVAL',
$script,
'3',
$key,
$this->getTombstoneKey($key, $hash),
$this->getTombstoneKey($key, '*'),
$this->getFlushKey(),
$hash,
$token->value,
$value,
(string) self::TOKEN_TTL,
(string) \time(),
]);
$result = $this->command([
'EVAL',
$script,
'4',
$key,
$this->getTombstoneKey($key, $hash),
$this->getTombstoneKey($key, '*'),
$this->getFlushKey(),
$hash,
$token->value,
$value,
(string) self::TOKEN_TTL,
(string) \time(),
]);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant