diff --git a/readme.md b/readme.md index 1adf295..2fb4d7a 100644 --- a/readme.md +++ b/readme.md @@ -48,7 +48,7 @@ Replace `{version}` with the version of the package that you want to use, e.g.: This library is provided in ESM as well as CJS forms, and works with both Javascript and Typescript projects. -**This package requires you to use Node 16 or above.** +**This package requires you to use Node 16 or above and Redis 2.6.12 or above.** Import it in a CommonJS project (`type: commonjs` or no `type` field in `package.json`) as follows: @@ -140,7 +140,7 @@ below: | Library | Function | | ------------------------------------------------------------------ | ----------------------------------------------------------------------------- | | [`node-redis`](https://github.com/redis/node-redis) | `async (...args: string[]) => client.sendCommand(args)` | -| [`node-redis`](https://github.com/redis/node-redis) (cluster) | `async (...args: string[]) => cluster.sendCommand(args[1], false, args)` | +| [`node-redis`](https://github.com/redis/node-redis) (cluster) | `async (...args: string[]) => cluster.sendCommand(args[1], false, args)` | | [`ioredis`](https://github.com/luin/ioredis) | `async (command: string, ...args: string[]) => client.call(command, ...args)` | | [`handy-redis`](https://github.com/mmkal/handy-redis) | `async (...args: string[]) => client.nodeRedis.sendCommand(args)` | | [`tedis`](https://github.com/silkjs/tedis) | `async (...args: string[]) => client.command(...args)` | diff --git a/source/scripts.ts b/source/scripts.ts index a883c2d..94a8da3 100644 --- a/source/scripts.ts +++ b/source/scripts.ts @@ -7,15 +7,24 @@ */ const scripts = { increment: ` - local totalHits = redis.call("INCR", KEYS[1]) - local timeToExpire = redis.call("PTTL", KEYS[1]) - if timeToExpire < 0 or ARGV[1] == "1" - then - redis.call("PEXPIRE", KEYS[1], tonumber(ARGV[2])) - timeToExpire = tonumber(ARGV[2]) - end + local windowMs = tonumber(ARGV[2]) + local resetOnChange = ARGV[1] == "1" - return { totalHits, timeToExpire } + local timeToExpire = redis.call("PTTL", KEYS[1]) + + if timeToExpire <= 0 then + redis.call("SET", KEYS[1], 1, "PX", windowMs) + return { 1, windowMs } + end + + local totalHits = redis.call("INCR", KEYS[1]) + + if resetOnChange then + redis.call("PEXPIRE", KEYS[1], windowMs) + timeToExpire = windowMs + end + + return { totalHits, timeToExpire } ` // Ensure that code changes that affect whitespace do not affect // the script contents. diff --git a/test/store-test.ts b/test/store-test.ts index 9191084..e1b5958 100644 --- a/test/store-test.ts +++ b/test/store-test.ts @@ -200,6 +200,28 @@ describe('redis store test', () => { expect(await client.get('rl:test-store-two')).toEqual(null) }) + it('starts new window with count 1 when TTL expired (race fix)', async () => { + const store = new RedisStore({ sendCommand }) + const windowMs = 50 + store.init({ windowMs } as Options) + + const key = 'test-expired-window' + + // First hit in first window + const first = await store.increment(key) + expect(first.totalHits).toEqual(1) + expect(Number(await client.pttl(`rl:${key}`))).toEqual(windowMs) + + // Advance beyond expiry so Redis reports expired (-2) + jest.advanceTimersByTime(windowMs + 1) + + // Next increment should start a fresh window with count=1 and TTL reset + const afterExpiry = await store.increment(key) + expect(afterExpiry.totalHits).toEqual(1) + expect(Number(await client.get(`rl:${key}`))).toEqual(1) + expect(Number(await client.pttl(`rl:${key}`))).toEqual(windowMs) + }) + it.skip('do not reset the expiration when the ttl is very close to 0', async () => { const store = new RedisStore({ sendCommand }) const windowMs = 60 @@ -236,4 +258,91 @@ describe('redis store test', () => { expect(Number(await client.get('rl:test-store'))).toEqual(1) expect(Number(await client.pttl('rl:test-store'))).toEqual(10) }) + + it('unit: when PTTL==0 but key exists, script starts new window with 1', async () => { + // In-memory fake Redis state for a single key + const state: { value: number; ttl: number } = { value: 0, ttl: -2 } + let loadedIncrementScript = '' + + // Stub that simulates Redis primitives and applies logic depending on script order + const sendCommandStub = async (...args: string[]): Promise => { + const [command, ...rest] = args + if (command === 'SCRIPT' && rest[0] === 'LOAD') { + const scriptBody = rest[1] + if (scriptBody.includes('INCR')) loadedIncrementScript = scriptBody + // Return a fake sha + return 'sha-' + (scriptBody.includes('INCR') ? 'incr' : 'get') + } + + if (command === 'EVALSHA') { + const sha = rest[0] + const numberKeys = rest[1] + const key = rest[2] + const resetOnChange = rest[3] === '1' + const windowMs = Number(rest[4]) + if (sha.endsWith('incr') && numberKeys === '1' && key) { + // Determine algorithm: does the script read PTTL before INCR? + const pttlIndex = loadedIncrementScript.indexOf('PTTL') + const incrIndex = loadedIncrementScript.indexOf('INCR') + + let totalHits = 0 + let { ttl } = state + + if (pttlIndex > -1 && incrIndex > -1 && pttlIndex < incrIndex) { + // NEW script: check ttl first + if (ttl <= 0) { + state.value = 1 + state.ttl = windowMs + totalHits = 1 + ttl = windowMs + } else { + state.value += 1 + totalHits = state.value + if (resetOnChange) state.ttl = windowMs + ttl = state.ttl + } + } else { + // OLD script: INCR first, then PTTL; only resets when ttl<0 + state.value += 1 + totalHits = state.value + // Read pttl after incr; here ttl is 0 (exists but expired) + if (ttl < 0 || resetOnChange) { + state.ttl = windowMs + ttl = windowMs + } else { + // Ttl == 0 branch + ttl = 0 + } + } + + return [totalHits as unknown as number, ttl as unknown as number] + } + } + + // Fallback for get script + if (command === 'EVALSHA' && args[0].endsWith('get')) { + return [ + state.value as unknown as number, + state.ttl as unknown as number, + ] + } + + return -99 + } + + const store = new RedisStore({ sendCommand: sendCommandStub }) + store.init({ windowMs: 60 } as Options) + + const key = 'pttl-zero-exists' + + // First increment to create key with value=1 and ttl=60 + await store.increment(key) + state.value = 1 + state.ttl = 0 // Simulate edge: key still exists but PTTL==0 exactly + + const result = await store.increment(key) + + // With NEW script we expect a fresh window: hits=1 and ttl reset + expect(result.totalHits).toEqual(1) + }) })