Skip to content

Conversation

@mt-ks
Copy link
Collaborator

@mt-ks mt-ks commented Oct 20, 2025

  • Problem: At TTL=0, INCR→PTTL ordering could let the counter continue instead of starting a new window.
  • Fix: Lua increment now checks PTTL first; if TTL<=0, SET ... PX windowMs initializes the window at 1; otherwise INCR (and PEXPIRE when resetExpiryOnChange is true).
  • Tests: Added unit test that stubs “key exists with PTTL=0”; existing tests pass.

Issue SS

ss

mt-ks added 2 commits October 20, 2025 16:35
- Update the increment script to handle expiration and reset conditions more effectively.
- Introduce parameters for window duration and reset behavior, enhancing script functionality.
Copy link
Member

@nfriedly nfriedly left a comment

Choose a reason for hiding this comment

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

Thank you, this is great work!

Comment on lines +10 to +13
local windowMs = tonumber(ARGV[2])
local resetOnChange = ARGV[1] == "1"
return { totalHits, timeToExpire }
local timeToExpire = redis.call("PTTL", KEYS[1])
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for making these named variables, we should have done that a while ago.

@nfriedly
Copy link
Member

This does look like a much better fix than the original one, and it looks like I was wrong with my initial thinking that this was an unrelated issue.

I think using the SET PX argument bumps our minimum redis version to 2.6.12, up from 2.6.0 previously. I thought we had that documented somewhere, but I'm not seeing it now. Would you like to add a note to the readme calling that out? Maybe on the same line where it says the minimum node.js version?

Lastly, the linter is complaining about formatting - run npm run format to fix that automatically.

Then I can merge it in a publish a new release.

- Add Redis 2.6.12 minimum version requirement to README
- Fix prettier formatting issues in table
@mt-ks
Copy link
Collaborator Author

mt-ks commented Oct 20, 2025

74208c0
Thx @nfriedly <3
Fix: 74208c0

@nfriedly nfriedly merged commit 428475d into express-rate-limit:main Oct 20, 2025
8 checks passed
@nfriedly
Copy link
Member

Ok, CI should publish v4.2.3 to NPM shortly.

Thanks, again!

@mt-ks
Copy link
Collaborator Author

mt-ks commented Oct 20, 2025

Feedback;
Tested, works perfect ✅

Screenshot 2025-10-20 at 20 24 23

@nfriedly
Copy link
Member

nfriedly commented Oct 23, 2025

Hey @mt-ks you did an awesome job on this PR; would you like to be added to this repo as a co-maintainer with @gamemaker1 and myself? (And @wyattjoh who is the original author but no longer as involved in the project.)

We have a couple of changes planned (include a default redis library, & deprecate resetExpiryOnChange), and you'd be welcome but not obligated to take that on. My thinking is more that it would be beneficial to have someone involved who is currently using the library, in order to better anticipate and respond to issues.

@mt-ks
Copy link
Collaborator Author

mt-ks commented Oct 23, 2025

Hi @nfriedly First of all, thank you
Absolutely, it’d be exciting to be part of this, and I’m looking forward to contributing and supporting the project! 🙌

@nfriedly
Copy link
Member

Awesome, I just sent you an invite

@gamemaker1
Copy link
Member

gamemaker1 commented Oct 24, 2025

Welcome on board @mt-ks! Looking forward to working with you :)

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.

3 participants