Fix TTL=0 race: start new window at 1 atomically in Lua (#218)

- Update the increment script to handle expiration and reset conditions more effectively.
- Introduce parameters for window duration and reset behavior, enhancing script functionality.
- Add Redis 2.6.12 minimum version requirement to README

Author: mt-ks

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)`                        |

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.

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<RedisReply> => {
+			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)
+	})
 })