From f33a0f31a5a718619df8fb7210efd9750bc3b2a6 Mon Sep 17 00:00:00 2001 From: derrickmehaffy Date: Mon, 12 Oct 2026 17:04:49 -0700 Subject: [PATCH] fix(provider-redis): cache anything at all on a Redis Cluster @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. --- .github/workflows/tests.yml | 40 ++++++ package.json | 3 +- .../src/RedisCacheProvider.ts | 66 ++++++++- scripts/redis-cluster-live.cjs | 133 ++++++++++++++++++ 4 files changed, 236 insertions(+), 6 deletions(-) create mode 100644 scripts/redis-cluster-live.cjs diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 8d50017d..7461128b 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -78,6 +78,46 @@ jobs: node-version: 24 - run: node scripts/provider-smoke.cjs + # A real three-master Redis Cluster. The recording client in test:cluster + # proves which commands the provider issues, but it cannot see a failure + # inside @keyv/redis: its cross-slot MULTI on every write was refused by a + # real cluster and swallowed by keyv, so nothing was ever cached there. + # Service containers cannot form a cluster, so the nodes are started here + # on the host network. See + # https://github.com/strapi-community/plugin-rest-cache/issues/100 + redis_cluster: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + - uses: pnpm/action-setup@0977fd99725f1db4007ccb2928dbb4e90d06cc86 # v6 + + - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7 + with: + node-version: 22 + + - name: Start a Redis Cluster + run: | + for port in 7000 7001 7002; do + docker run -d --rm --name "redis-$port" --network host redis:8.10 \ + redis-server --port "$port" --cluster-enabled yes \ + --cluster-config-file "nodes-$port.conf" --save "" --appendonly no + done + for port in 7000 7001 7002; do + until docker exec redis-7000 redis-cli -p "$port" ping >/dev/null 2>&1; do sleep 1; done + done + docker exec redis-7000 redis-cli --cluster create \ + 127.0.0.1:7000 127.0.0.1:7001 127.0.0.1:7002 --cluster-replicas 0 --cluster-yes + until docker exec redis-7000 redis-cli -p 7000 cluster info | grep -q 'cluster_state:ok'; do + sleep 1 + done + + - name: Install dependencies + run: pnpm install --frozen-lockfile + + - run: pnpm run build + + - run: pnpm run test:cluster:live + linters: runs-on: ubuntu-latest strategy: diff --git a/package.json b/package.json index 7dc043ae..9d615119 100644 --- a/package.json +++ b/package.json @@ -37,7 +37,8 @@ "test:admin:ui": "playwright test -c e2e/playwright.config.ts --ui", "docs:api": "typedoc", "docs:build:only": "vitepress build docs", - "build:providers": "pnpm --filter \"./packages/provider-*\" run build" + "build:providers": "pnpm --filter \"./packages/provider-*\" run build", + "test:cluster:live": "node scripts/redis-cluster-live.cjs" }, "license": "MIT", "devDependencies": { diff --git a/packages/provider-rest-cache-redis/src/RedisCacheProvider.ts b/packages/provider-rest-cache-redis/src/RedisCacheProvider.ts index 436feeb7..5ac26f55 100644 --- a/packages/provider-rest-cache-redis/src/RedisCacheProvider.ts +++ b/packages/provider-rest-cache-redis/src/RedisCacheProvider.ts @@ -71,6 +71,61 @@ interface KeyvRedisStore { const redisStoreOf = (cache: Cache): KeyvRedisStore => cache.stores[0].opts.store as unknown as KeyvRedisStore; +/** @keyv/redis' own read/write methods, which a cluster needs replaced. */ +interface KeyvRedisAdapter extends KeyvRedisStore { + redis: RedisClient & { + set(key: string, value: string, ...args: unknown[]): Promise; + sadd(key: string, ...members: string[]): Promise; + }; + _getKeyName(key: string): string; + set(key: string, value: string | undefined, ttl?: number): Promise; + delete(key: string): Promise; +} + +/** + * Make @keyv/redis' writes and deletes work on a Redis Cluster. + * + * @keyv/redis 3 writes an entry and adds it to the tracking set in one MULTI, + * and deletes them the same way. Those 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 the failure and only emits it, so on + * a cluster set() reported success and nothing was ever cached. + * + * Here the same two commands are issued separately, leaving the same keys, the + * same PX lifetime and 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 - never a tracked key with nothing + * behind it. Standalone servers keep the MULTI. + * + * See https://github.com/strapi-community/plugin-rest-cache/issues/100 + */ +function makeClusterSafe(store: KeyvRedisAdapter): void { + const { redis } = store; + + store.set = async (key, value, ttl) => { + if (value === undefined) return; + const name = store._getKeyName(key); + // ttl is milliseconds, as PX expects. Never converted. + if (typeof ttl === 'number') { + await redis.set(name, value, 'PX', ttl); + } else { + await redis.set(name, value); + } + if (store.opts?.useRedisSets !== false) { + await redis.sadd(store._getNamespace!(), name); + } + }; + + store.delete = async (key) => { + const name = store._getKeyName(key); + const removed = Number(await redis.unlink(name)); + if (store.opts?.useRedisSets !== false) { + await redis.srem(store._getNamespace!(), name); + } + return removed > 0; + }; +} + /** Everything accepted under `provider.options` in the plugin configuration. */ export interface RedisCacheProviderOptions { /** Name of a connection declared on the redis plugin. */ @@ -90,14 +145,15 @@ export class RedisCacheProvider extends CacheProvider { const { ttl, ...adapterOptions } = options; + const store = new KeyvRedis(client, adapterOptions); + if (client.isCluster) { + makeClusterSafe(store); + } + this.client = client; this.cache = createCache({ ttl, - stores: [ - new Keyv({ - store: new KeyvRedis(client, adapterOptions), - }), - ], + stores: [new Keyv({ store })], }); } diff --git a/scripts/redis-cluster-live.cjs b/scripts/redis-cluster-live.cjs new file mode 100644 index 00000000..bd184191 --- /dev/null +++ b/scripts/redis-cluster-live.cjs @@ -0,0 +1,133 @@ +#!/usr/bin/env node +'use strict'; + +/** + * Drives the redis provider against a real Redis Cluster. + * + * redis-cluster-check.cjs proves which commands the provider issues, using a + * recording client. That cannot see a failure inside a dependency: @keyv/redis + * writes an entry and its tracking-set member in one MULTI, two keys in + * different hash slots, which a cluster refuses with + * + * All keys in the pipeline should belong to the same slots allocation group + * + * Keyv catches that and emits it as an event, so set() reported success and + * nothing was ever cached on a cluster. Only a real cluster shows it. + * + * Needs a cluster to write to: REDIS_CLUSTER_NODES, comma-separated host:port + * (default 127.0.0.1:7000,127.0.0.1:7001,127.0.0.1:7002). + * + * @see https://github.com/strapi-community/plugin-rest-cache/issues/100 + */ + +const assert = require('assert'); +const path = require('path'); +const { createRequire } = require('module'); + +const ROOT = path.join(__dirname, '..'); +const PROVIDER = path.join(ROOT, 'packages/provider-rest-cache-redis'); +const requireFromProvider = createRequire(path.join(PROVIDER, 'package.json')); +// The ioredis the provider actually runs on is @keyv/redis's. +const Redis = createRequire(requireFromProvider.resolve('@keyv/redis'))('ioredis'); +const { RedisCacheProvider } = require(path.join(PROVIDER, 'dist/RedisCacheProvider.js')); + +const NODES = (process.env.REDIS_CLUSTER_NODES || '127.0.0.1:7000,127.0.0.1:7001,127.0.0.1:7002') + .split(',') + .map((node) => { + const [host, port] = node.split(':'); + return { host, port: Number(port) }; + }); + +// Spread across hash slots, as request paths are. +const KEYS = Array.from({ length: 30 }, (_, i) => `/api/articles?page=${i}&`); +const MAX_AGE = 600000; + +const results = []; +async function check(name, fn) { + try { + await fn(); + results.push([true, name, '']); + } catch (error) { + results.push([false, name, error.message.split('\n')[0]]); + } +} + +async function flushAll(cluster) { + await Promise.all(cluster.nodes('master').map((node) => node.flushdb())); +} + +/** Every key on every master. */ +async function allKeys(cluster) { + const per = await Promise.all(cluster.nodes('master').map((node) => node.keys('*'))); + return per.flat().sort(); +} + +(async () => { + const cluster = new Redis.Cluster(NODES); + await new Promise((resolve, reject) => { + cluster.once('ready', resolve); + cluster.once('error', reject); + }); + + const provider = new RedisCacheProvider(cluster, {}); + // Keyv reports store failures as events, not rejections. Collect them, so a + // swallowed CROSSSLOT fails a check instead of passing it. + const storeErrors = []; + provider.cache.stores[0].on('error', (error) => storeErrors.push(error.message)); + const noStoreErrors = () => + assert.deepStrictEqual(storeErrors.splice(0), [], 'the store reported an error'); + + await check('set() stores entries that get() returns', async () => { + await flushAll(cluster); + for (const key of KEYS) await provider.set(key, { key }, MAX_AGE); + noStoreErrors(); + for (const key of KEYS) assert.deepStrictEqual(await provider.get(key), { key }, key); + }); + + await check('set() tracks every entry, with its lifetime', async () => { + const tracked = (await cluster.smembers('namespace:keyv')).sort(); + assert.deepStrictEqual(tracked, KEYS.map((k) => `keyv:${k}`).sort()); + const ttl = await cluster.pttl(`keyv:${KEYS[0]}`); + assert.ok(ttl > MAX_AGE - 10000 && ttl <= MAX_AGE, `lifetime ${ttl}ms is not maxAge`); + }); + + await check('keys() lists every entry', async () => { + assert.deepStrictEqual((await provider.keys()).sort(), [...KEYS].sort()); + }); + + await check('del() removes the entry and its tracking', async () => { + await provider.del(KEYS[0]); + noStoreErrors(); + assert.ok((await provider.get(KEYS[0])) == null, 'entry survived del()'); + assert.strictEqual(await cluster.sismember('namespace:keyv', `keyv:${KEYS[0]}`), 0); + }); + + await check('delMany() removes entries across slots', async () => { + await provider.delMany(KEYS.slice(1, 15)); + noStoreErrors(); + for (const key of KEYS.slice(1, 15)) { + assert.ok((await provider.get(key)) == null, `${key} survived delMany()`); + } + assert.deepStrictEqual((await provider.keys()).sort(), KEYS.slice(15).sort()); + }); + + await check('clear() empties the cluster', async () => { + await provider.clear(); + noStoreErrors(); + assert.deepStrictEqual(await allKeys(cluster), []); + }); + + await flushAll(cluster); + cluster.disconnect(); + + console.log(`redis cluster (live) - ${NODES.length} nodes\n`); + for (const [ok, name, detail] of results) { + console.log(` ${ok ? 'PASS' : 'FAIL'} ${name}${detail ? `\n ${detail}` : ''}`); + } + const failed = results.filter(([ok]) => !ok).length; + console.log(`\n${results.length - failed}/${results.length} passed`); + process.exit(failed > 0 ? 1 : 0); +})().catch((error) => { + console.error(error); + process.exit(1); +});