Add INCREX command for atomic increment with ttl and bounds#15045
Conversation
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
310432d to
95be3a9
Compare
|
@raffertyyu can I help you work on this PR? |
|
@sundb Sorry for the delay. I spent some time discussing with my colleagues what exactly need from this
This two cases will throw an error if the value or increment is not an integer.
We think this keeps command simple and efficient. Users should have a clear understanding of their data types. If they truly need floating-point support, they should explicitly use BYFLOAT rather than relying on the system to implicitly convert values, which could lead to a loss of precision. Please feel free to modify my PR as you see fit. I spent quite a bit of time debugging the refcount mechanism during increx command execution to ensure everything is handled correctly, and I’m open to any further optimizations. |
| if (value < lb) { | ||
| value = lb; | ||
| } else if (value > ub) { | ||
| value = ub; | ||
| } |
There was a problem hiding this comment.
if the old value is larger than UBOUND, now the old value will be reduced to UBOUND, i think the old value should remain unchanged.
CC @LiorKogan
127.0.0.1:6379> set mykey 1.5
OK
127.0.0.1:6379> increx mykey BYFLOAT 100 UBOUND 1
1) "1"
2) "-0.5"
There was a problem hiding this comment.
ok, let's keep the value unchanged on overflow - instead of increasing/decreasing up to the bound.
There was a problem hiding this comment.
@LiorKogan there is another case, for example:
upper = 100
old value = 200
What is the new value after increx key 100 BYINT, keep the original value(200) or 300?
There was a problem hiding this comment.
If this is what you mean:
For old value = 200, and INCREX key 100 BYINT UBOUND 100
I would expect the increment to be denied, hence the value should remain as is (200).
There was a problem hiding this comment.
For old value = 200, and INCREX key -50 BYINT UBOUND 100
How is it now? 200 or 150?
Personally, I find it all very confusing and it doesn't seem to work well.
@oranagra pls share your thoughts.
There was a problem hiding this comment.
Reject the request (which in this case means: don't create the key!)
do you mean replying to an error message?
There was a problem hiding this comment.
Another issue with the current implementation: if key doesn't exist and increment result is out-of-bounds, a zero-value key is still created with an TTL/expiration. I agree with not create new key
There was a problem hiding this comment.
do you mean replying to an error message?
Yes (if the key doesn't exist and we follow behavior 2.).
If the doesn't exist and we follow behavior 1 - we should create the key with the value bounded:
INCREX key LBOUND 100 UBOUND 200- create the key with value 100INCREX key BYINT 500 LBOUND 100 UBOUND 200- create the key with value 200
There was a problem hiding this comment.
the question i ask myself first, is if either of these cases can actually happen for user that properly uses this command for the use cases we aim to handle (in which case we need to make it useful), or is that just an edge case that in reality can be solved either way and it doesn't really matters.
i think it's clear that the case of an existing value being out of bound probably won't happen, unless maybe someone decides to change the bounds mid-run, in which case we should clamp.
the other case of a non-existing key and a zero being out of bounds is also probably not a real one, in which case we could error, but considering the other case needs to be solved by clamping, maybe we should align them.
WTYT?
There was a problem hiding this comment.
i think it's clear that the case of an existing value being out of bound probably won't happen, unless maybe someone decides to change the bounds mid-run, in which case we should clamp.
Agreeing to this approach and keeping the original value unchanged if out of bound has already confused me while implementing it.
|
@LiorKogan I suddenly realized that we have a similar syntax Should we synchronize ONBOUND with it? |
@sundb yes, definitely. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Reviewed by Cursor Bugbot for commit e96bd89. Configure here.
Follow #15045 ## Summary Simplify INCREX's out-of-bounds policy: The original INCREX shipped with three out-of-bounds policies — OVERFLOW FAIL, OVERFLOW SAT, OVERFLOW REJECT — but FAIL and REJECT are functionally redundant: both leave the key untouched when the result is out of bounds. They differ only in how the caller is notified (error reply vs. [current_value, 0] array reply), which forces the user to make a stylistic choice with no real semantic difference. This PR collapses the three policies into one clear behavior: * Default: the operation is rejected; the key value and TTL are left unchanged, and the reply is [current_value, 0]. Callers detect non-application by checking the applied-increment field; no error-handling branch is required. * SATURATE: the result is saturated to UBOUND / LBOUND, or to the type limits (LLONG_MAX/MIN for BYINT, ±LDBL_MAX for BYFLOAT) when no explicit bound is given. New syntax: INCREX <key> [BYFLOAT increment | BYINT increment] [LBOUND lowerbound] [UBOUND upperbound] [SATURATE] [EX seconds | PX milliseconds | EXAT seconds-timestamp | PXAT milliseconds-timestamp | PERSIST] [ENX] --------- Co-authored-by: Ozan Tezcan <ozantezcan@gmail.com>

Close #14278
Overview
Rate limiters, sliding windows, request counters, and numerous other network-facing patterns share a common primitive: atomically increment a counter and set its expiration. Achieving this in Redis requires either multiple round-trips or a Lua script that bundles
INCR/INCRBY/INCRBYFLOATwithEXPIRE/PEXPIRE.We propose a new command,
INCREX, that collapses this two-step pattern into a single, native, O(1) command.INCREXatomically:Use Cases
Basic Usage
Use Case: Rate Limiter
Before (Lua script):
Client invocation:
After (INCREX):
ENXmeans: set expiration only if the key doesn't already have an expiration. This ensures the sliding window's TTL is only set on the first request.Use Case: Token Bucket Refill
Refill tokens periodically up to a capacity ceiling, saturating at the cap instead of erroring:
Tokens cannot exceed 100, and the key auto-expires after inactivity.
Use Case: Countdown / Resource Consumption
Decrement a resource counter down to zero, saturating at the floor:
When credits are exhausted,
OVERFLOW SATprevents negative balances without client-side checks.Parameter Reference
Syntax
Parameters
key0if it does not exist.BYFLOAT incrementBYINT incrementLBOUND lowerboundLLONG_MIN(integer) or-LDBL_MAX(float).UBOUND upperboundLLONG_MAX(integer) orLDBL_MAX(float).OVERFLOW <FAIL | SAT | REJECT>FAILrejects the operation with an error (default).SATsaturates the result to the bound.REJECTleaves the key and its TTL untouched and replies with the current value and a zero delta.EX secondssecondsseconds.PX millisecondsmillisecondsmilliseconds.EXAT unix-time-secondsPXAT unix-time-millisecondsPERSISTENXIf neither
BYINTnorBYFLOATis specified, the increment defaults to integer1.Return Value
An array of two elements:
OVERFLOW REJECT).OVERFLOW SATsaturates the result to a bound, and is always0whenOVERFLOW REJECTskipped the operation.BYINT): both elements are integers.BYFLOAT): both elements are bulk strings representing the float values on RESP2, and RESP3 Doubles on RESP3.Overflow Policy (FAIL vs. SAT vs. REJECT)
Controlled by the optional
OVERFLOWargument. A bound violation includes both exceeding an explicitLBOUND/UBOUNDand overflowing the type limits when no explicit bound is given. The same policy also applies when the stored value is already out of bounds before the increment is applied (e.g. the key was set directly viaSET, or the bounds have been tightened since the previous write).OVERFLOW FAIL(default): if the computed result would violate a bound — or if the existing value is already out of bounds — the command returns an error and the key is left unchanged. This matches the existing semantics ofINCRBY/INCRBYFLOATon overflow.OVERFLOW SAT: the result is silently capped atUBOUND/ floored atLBOUND(or saturated to the type limits when no explicit bound is given). If the stored value is already out of bounds, it is still saturated to the nearest bound (LBOUNDorUBOUND) — the second element of the reply reflects the actual delta that was applied to reach the bound, which may be larger in magnitude than the requested increment. If the delta cannot be represented as a 64-bit signed integer (default orBYINT), or would produce Infinity (BYFLOAT), an error is returned.OVERFLOW REJECT: the operation is silently skipped — the key value and its TTL are left unchanged, no keyspace notification is fired, and nothing is replicated. This also applies when the stored value is already out of bounds: the original value is preserved untouched. The reply is[current_value, 0], allowing the caller to detect the rejection without handling an error.Notes
INCR).ENXrequires one ofEX/PX/EXAT/PXAT.OVERFLOW SAT, the expiration is still applied as specified.OVERFLOW REJECTthe expiration option is ignored on the rejected branch — TTL is preserved exactly as it was before the call.BYINTrequires an integer-typed existing value;BYFLOATaccepts both.Integers can be promoted to floats losslessly, but a stored float (e.g.
"1.5") cannot be parsed back as an integer. This is consistent withINCR/INCRBY(integer-only) andINCRBYFLOAT(accepts both).Note
Medium Risk
Adds a new write command with non-trivial numeric/TTL semantics and replication rewriting, and slightly refactors shared expiration parsing used by existing string commands, so regressions could affect counter or TTL behavior.
Overview
Adds a new
INCREXstring command that atomically increments a numeric key (integer or float) and can optionally apply bounds (LBOUND/UBOUND) with selectable overflow behavior (FAIL,SAT,REJECT) while also updating, preserving, or removing TTL (EX/PX/EXAT/PXAT/PERSISTplusENX).Updates the command tables/docs (
commands.def, newcommands/increx.json) and exposesincrexCommandinserver.h, implements the command int_string.cincluding deterministic replication rewrite toSET(orDEL/UNLINKfor already-elapsed expirations), and adds portable integer overflow helpers inutil.hplus an extensive new unit test suite (tests/unit/type/increx.tcl).Refactors
getExpireMillisecondsOrReplyto take an explicit relative vs absolute TTL boolean and updates call sites to avoid misinterpretingEXAT/PXATas relative expirations.Reviewed by Cursor Bugbot for commit 74f84be. Bugbot is set up for automated code reviews on this repo. Configure here.