Skip to content

fix(provider-redis): cache anything at all on a Redis Cluster - #244

Merged
derrickmehaffy merged 1 commit into
mainfrom
fix/redis-cluster-writes
Oct 13, 2026
Merged

fix(provider-redis): cache anything at all on a Redis Cluster#244
derrickmehaffy merged 1 commit into
mainfrom
fix/redis-cluster-writes

Conversation

@derrickmehaffy

@derrickmehaffy derrickmehaffy commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

What changed and why

On a Redis Cluster, the redis provider on plugin-redis v2 cached nothing, and nothing reported it.

@keyv/redis 3 writes an entry and adds it to the namespace:keyv tracking set in a single MULTI. Deletes work the same way. The entry and the set are two keys in different hash slots, and a cluster refuses that:

All keys in the pipeline should belong to the same slots allocation group

Keyv catches the failure and only emits it as an error event, so:

  • set() resolved and nothing was stored, so every request was a miss;
  • del() failed the same way.

The #100 work made purges slot-safe, but it was verified with a recording client (test:cluster), which can't see a failure inside a dependency. I reproduced this on a real three-master cluster: on main, set() returns, get() returns undefined, and no node holds a single key.

Fix: when the client reports isCluster, the store's set and delete issue the same two commands separately:

  • The keys, the PX lifetime (milliseconds, unconverted) and the tracking set stay the same, so existing entries and purges are unaffected.
  • The entry is written before it's tracked and untracked after it's removed. An interruption leaves at worst an untracked entry that expires, never a tracked key with nothing behind it.
  • Standalone servers keep the MULTI and see no change.

Nothing else changes: same keys, same commands on standalone, and no new configuration.

Tests

  • New scripts/redis-cluster-live.cjs (pnpm run test:cluster:live) drives the provider against a real cluster: set/get, tracking and lifetime, keys(), del(), delMany() across slots, and clear(). It treats keyv's swallowed error events as failures.
    • On main, 5 of 6 checks fail. clear() only passes because nothing was ever stored.
    • With the fix, 6 of 6 pass.
  • New CI job redis_cluster starts three redis:8.10 masters on the host network and runs the script. Service containers can't form a cluster.

Locally:

  • test:lint green, test:smoke 3/3, test:cluster 5/5, test:deps ok.
  • test:e2e:redis 135/135 on standalone.

Checklist

Notes for the reviewer

@derrickmehaffy
fix(provider-redis): cache anything at all on a Redis Cluster
f33a0f3 
@keyv/redis 3 writes an entry and adds it to the tracking set in one MULTI,
and deletes them the same way. The entry and the set are two keys in
different hash slots, which a cluster refuses:

  All keys in the pipeline should belong to the same slots allocation group

Keyv catches that and only emits it as an event, so set() resolved, nothing
was stored, and every request on a cluster was a miss. del() failed the
same way. The #100 work made purges slot-safe, but it was checked with a
recording client, which cannot see a failure inside a dependency.

On a cluster the store now issues the same two commands separately: the same
keys, the same PX lifetime, the same tracking set. The entry is written
before it is tracked and untracked after it is removed, so an interruption
leaves at worst an untracked entry that expires. Standalone servers keep the
MULTI and are unaffected.

scripts/redis-cluster-live.cjs drives the provider against a real
three-master cluster and treats keyv's error events as failures; on main
five of its six checks fail. CI now starts such a cluster and runs it.
@derrickmehaffy derrickmehaffy mentioned this pull request Oct 13, 2026
Merged
4 tasks done
@derrickmehaffy
derrickmehaffy merged commit e7016f7 into main Oct 13, 2026
13 checks passed
@derrickmehaffy
derrickmehaffy deleted the fix/redis-cluster-writes branch October 13, 2026 00:20
@github-actions github-actions Bot mentioned this pull request Oct 13, 2026
Open
derrickmehaffy added a commit that referenced this pull request Oct 13, 2026
@derrickmehaffy
Merge origin/main into dev/plugin-redis-v3
16f8d48 
Brings in the redis provider fixes from #244, #245 and #249. The plugin-redis
v2 path is main's as merged: makeClusterSafe, the SREM-before-UNLINK cluster
delete, the background tracking-set prune, and keys() scanning every master
when useRedisSets is off.

The constructor now builds @keyv/redis only when there is no node-redis
connection, as this branch did before, so the v3 path never wraps the ioredis
shim. The v3 store itself is unchanged here; porting the same fixes to it
follows separately so each one can be shown failing first. Until then
test:continuity fails its two useRedisSets: false checks, because v2 now
enumerates those entries and v3 does not.

The lockfile is main's plus this branch's two devDependencies.
derrickmehaffy added a commit that referenced this pull request Oct 13, 2026
@derrickmehaffy
test(provider-redis): hold the v3 store to the #244, #245 and #249 ch…
3afb156 
…ecks

The merged fixes have live and recording-client checks for the plugin-redis
v2 path only. The node-redis store this branch adds has the same three
problems, and nothing exercised them:

- on a cluster its delete() and unlinkTracked() UNLINK before they SREM, so a
  purge racing a write leaves a live entry outside the tracking set (#244);
- nothing prunes its tracking set, so expired members are listed by keys()
  and the set grows for as long as the cache runs (#249);
- with useRedisSets off, keys() reads the empty tracking set, so pattern
  purges match nothing (#245).

redis-standalone-live and redis-cluster-live now also drive the provider on a
real node-redis client (createClient, createCluster), constructed the way
init() does on v3. The cluster harness runs every check against both
backends. redis-cluster-check's node-redis recorder gains EXISTS, SSCAN,
per-master clients and an SREM hook, for the command-order, prune and
client-swap assertions. redis-key-continuity now requires keys() to list
every entry the other backend wrote, not only to agree with it, and runs
against a cluster when REDIS_CLUSTER_NODES is set, which CI's redis_cluster
job now does.

The new v3 checks fail at this commit; the fixes follow. The prune-race
checks also assert that a SREM was issued, since a prune that does nothing
passes them otherwise.
derrickmehaffy added a commit that referenced this pull request Oct 13, 2026
@derrickmehaffy
fix(provider-redis): untrack before unlinking on a cluster in the v3 …
22472fe 
…store

#244 changed the plugin-redis v2 path to SREM before UNLINK on a cluster,
where a delete is two commands rather than one MULTI. NodeRedisStore still
unlinked first, in both delete() and unlinkTracked(), so a purge racing a
response being cached for the same key could run UNLINK (purge) -> SET, SADD
(write) -> SREM (purge) and leave a live entry outside the tracking set.
keys() cannot list such an entry, so no purge reaches it and it is served
stale until its maxAge. Against a real three-node cluster, the race checks
left 16 and 22 such entries before this change and none after.

The trade-off is the one makeClusterSafe documents: a failure between the two
commands now leaves an untracked entry until its maxAge, where it used to
leave only a dead member.
@derrickmehaffy derrickmehaffy mentioned this pull request Oct 13, 2026
Draft
2 of 7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@derrickmehaffy