Fix how hash is handling overriding of expired fields overwrite by ranshid · Pull Request #3060 · valkey-io/valkey

ranshid · 2026-01-13T18:34:49Z

There are currently several issues with the existing hash field expiration mechanism:

HINCRBY is propagated to the replica "as-is". It mean it relies on the fact that the state of the hash is the same on the primary and the replica. HFE did change this assumption as the field might be expired only when the replica will handle the propagated hincrby. the problem is that the replica does not "expire" fields by it's own. it needs to respect the request from the primary and always try to use the existing field. This can lead to either miss-alignment with the value on the primary and the replica AND even a disconnection since the replica might hold and "expired" field which is not in "integer" format...
HINCRBYFLOAT is currently ALWAYS propagating hset - this means that the expiration time of an entry will always be removed on the replica side (it needs to propagate HSETEX when expiration time needs to be maintained)
Currently all hash write commands which are mutating values might overwrite an expired field. In such cases the existing implementation will "silently" do so. The problem is that the user will not get any key-space-notificaiton explaining the reason for the behavior. For example, when hincrby is issued overwriting an expired field which was not yet "cleaned" by active-expiration it will reset the counter to '0' before incrementing it. this means that the user might ask: why is the value '1' and not bigger, "I did not see any notification that the old value expired"...
HSETEX with KEEPTTL suffers from a "somewhat" similar problem as #(1). the replica will receive the propagated command, but will not know if the primary "replaced" the entry which is expired now but might not have been expired when the primary applied it.

There are 2 options for a solution:

we could propagate hdel for every entry we are "overwritting" (batch them if we can)
propagate the commands "by effect". For example - have hincrby always propagate either HSET or HSETEX. This will not solve the '#(4)' problem above though, for which we might HAVE to propagate hdel

I tend to go with the second option. The reason is that it is expected to have less impact on replication stream and should include less processing time on the replicas and network traffic. Specifically for HSETEX with KEEPTTL we will have to propagate the hdel in case we overwritten an expired field, but that would help limit the impact of this propagation.

Signed-off-by: Ran Shidlansik <ranshid@amazon.com>

src/t_hash.c

codecov · 2026-01-13T18:55:38Z

Codecov Report

❌ Patch coverage is 96.90722% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.38%. Comparing base (f0d9810) to head (0ea52ac).
⚠️ Report is 30 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/t_hash.c	97.82%	2 Missing ⚠️
src/module.c	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #3060      +/-   ##
============================================
+ Coverage     74.25%   74.38%   +0.13%     
============================================
  Files           129      129              
  Lines         70988    71124     +136     
============================================
+ Hits          52712    52907     +195     
+ Misses        18276    18217      -59

Files with missing lines	Coverage Δ
src/db.c	`94.33% <100.00%> (+0.15%)`	⬆️
src/server.c	`89.57% <100.00%> (+0.11%)`	⬆️
src/server.h	`100.00% <ø> (ø)`
src/module.c	`26.50% <0.00%> (-0.01%)`	⬇️
src/t_hash.c	`95.38% <97.82%> (+0.46%)`	⬆️

... and 28 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

src/t_hash.c

Signed-off-by: Ran Shidlansik <ranshid@amazon.com>

src/t_hash.c

cjx-zar · 2026-01-14T14:58:57Z

Since we have import-mode, I think this solution is feasible.

…cation Signed-off-by: Ran Shidlansik <ranshid@amazon.com>

ranshid · 2026-01-15T10:04:59Z

@cjx-zar I updated the PR with some relevant tests. @frostzt is working to add the hdel propagation when the primary overwrites an expired field/s (which is why the relevant test should fail now). would be happy if you could TAL

…ided Signed-off-by: Ran Shidlansik <ranshid@amazon.com>

Signed-off-by: Ran Shidlansik <ranshid@amazon.com>

cjx-zar · 2026-01-16T11:52:02Z

Sorry, I'm busy with other things today. I'll check tomorrow.

src/server.h

Signed-off-by: Ran Shidlansik <ranshid@amazon.com>

src/t_hash.c

src/db.c

Signed-off-by: Ran Shidlansik <ranshid@amazon.com>

cjx-zar

LGTM：）

src/t_hash.c

enjoy-binbin

Top comment LGTM, i scanned the code (but didn't do a very thorough review), and i trust your changes in HFE part.

src/t_hash.c

Co-authored-by: Binbin <binloveplay1314@qq.com> Signed-off-by: Ran Shidlansik <ranshid@amazon.com>

src/t_hash.c

tests/unit/hashexpire.tcl

src/t_hash.c

Signed-off-by: Ran Shidlansik <ranshid@amazon.com>

…ey-io#3060) There are currently several issues with the existing hash field expiration mechanism: 1. `HINCRBY` is propagated to the replica "as-is". It mean it relies on the fact that the state of the hash is the same on the primary and the replica. HFE did change this assumption as the field might be expired only when the replica will handle the propagated `hincrby`. the problem is that the replica does not "expire" fields by it's own. it needs to respect the request from the primary and always try to use the existing field. This can lead to either miss-alignment with the value on the primary and the replica AND even a disconnection since the replica might hold and "expired" field which is not in "integer" format... 2. HINCRBYFLOAT is currently ALWAYS propagating `hset` - this means that the expiration time of an entry will always be removed on the replica side (it needs to propagate HSETEX when expiration time needs to be maintained) 3. Currently all hash write commands which are mutating values might overwrite an expired field. In such cases the existing implementation will "silently" do so. The problem is that the user will not get any key-space-notificaiton explaining the reason for the behavior. For example, when `hincrby` is issued overwriting an expired field which was not yet "cleaned" by active-expiration it will reset the counter to '0' before incrementing it. this means that the user might ask: why is the value '1' and not bigger, "I did not see any notification that the old value expired"... 4. HSETEX with KEEPTTL suffers from a "somewhat" similar problem as if the primary "replaced" the entry which is expired now but might not have been expired when the primary applied it. There are 2 options for a solution: 1. we could propagate `hdel` for every entry we are "overwritting" (batch them if we can) 2. propagate the commands "by effect". For example - have `hincrby` always propagate either HSET or HSETEX. This will not solve the '#(4)' problem above though, for which we might HAVE to propagate `hdel` I tend to go with the second option. The reason is that it is expected to have less impact on replication stream and should include less processing time on the replicas and network traffic. Specifically for HSETEX with KEEPTTL we will have to propagate the `hdel` in case we overwritten an expired field, but that would help limit the impact of this propagation. --------- Signed-off-by: Ran Shidlansik <ranshid@amazon.com> Co-authored-by: Sourav Singh Rawat <aidenfrostbite@gmail.com> Co-authored-by: Binbin <binloveplay1314@qq.com>

tests/unit/hashexpire.tcl

…ey-io#3060) There are currently several issues with the existing hash field expiration mechanism: 1. `HINCRBY` is propagated to the replica "as-is". It mean it relies on the fact that the state of the hash is the same on the primary and the replica. HFE did change this assumption as the field might be expired only when the replica will handle the propagated `hincrby`. the problem is that the replica does not "expire" fields by it's own. it needs to respect the request from the primary and always try to use the existing field. This can lead to either miss-alignment with the value on the primary and the replica AND even a disconnection since the replica might hold and "expired" field which is not in "integer" format... 2. HINCRBYFLOAT is currently ALWAYS propagating `hset` - this means that the expiration time of an entry will always be removed on the replica side (it needs to propagate HSETEX when expiration time needs to be maintained) 3. Currently all hash write commands which are mutating values might overwrite an expired field. In such cases the existing implementation will "silently" do so. The problem is that the user will not get any key-space-notificaiton explaining the reason for the behavior. For example, when `hincrby` is issued overwriting an expired field which was not yet "cleaned" by active-expiration it will reset the counter to '0' before incrementing it. this means that the user might ask: why is the value '1' and not bigger, "I did not see any notification that the old value expired"... 4. HSETEX with KEEPTTL suffers from a "somewhat" similar problem as if the primary "replaced" the entry which is expired now but might not have been expired when the primary applied it. There are 2 options for a solution: 1. we could propagate `hdel` for every entry we are "overwritting" (batch them if we can) 2. propagate the commands "by effect". For example - have `hincrby` always propagate either HSET or HSETEX. This will not solve the '#(4)' problem above though, for which we might HAVE to propagate `hdel` I tend to go with the second option. The reason is that it is expected to have less impact on replication stream and should include less processing time on the replicas and network traffic. Specifically for HSETEX with KEEPTTL we will have to propagate the `hdel` in case we overwritten an expired field, but that would help limit the impact of this propagation. --------- Signed-off-by: Ran Shidlansik <ranshid@amazon.com> Co-authored-by: Sourav Singh Rawat <aidenfrostbite@gmail.com> Co-authored-by: Binbin <binloveplay1314@qq.com>

…ey-io#3060) There are currently several issues with the existing hash field expiration mechanism: 1. `HINCRBY` is propagated to the replica "as-is". It mean it relies on the fact that the state of the hash is the same on the primary and the replica. HFE did change this assumption as the field might be expired only when the replica will handle the propagated `hincrby`. the problem is that the replica does not "expire" fields by it's own. it needs to respect the request from the primary and always try to use the existing field. This can lead to either miss-alignment with the value on the primary and the replica AND even a disconnection since the replica might hold and "expired" field which is not in "integer" format... 2. HINCRBYFLOAT is currently ALWAYS propagating `hset` - this means that the expiration time of an entry will always be removed on the replica side (it needs to propagate HSETEX when expiration time needs to be maintained) 3. Currently all hash write commands which are mutating values might overwrite an expired field. In such cases the existing implementation will "silently" do so. The problem is that the user will not get any key-space-notificaiton explaining the reason for the behavior. For example, when `hincrby` is issued overwriting an expired field which was not yet "cleaned" by active-expiration it will reset the counter to '0' before incrementing it. this means that the user might ask: why is the value '1' and not bigger, "I did not see any notification that the old value expired"... 4. HSETEX with KEEPTTL suffers from a "somewhat" similar problem as if the primary "replaced" the entry which is expired now but might not have been expired when the primary applied it. There are 2 options for a solution: 1. we could propagate `hdel` for every entry we are "overwritting" (batch them if we can) 2. propagate the commands "by effect". For example - have `hincrby` always propagate either HSET or HSETEX. This will not solve the '#(4)' problem above though, for which we might HAVE to propagate `hdel` I tend to go with the second option. The reason is that it is expected to have less impact on replication stream and should include less processing time on the replicas and network traffic. Specifically for HSETEX with KEEPTTL we will have to propagate the `hdel` in case we overwritten an expired field, but that would help limit the impact of this propagation. --------- Signed-off-by: Ran Shidlansik <ranshid@amazon.com> Co-authored-by: Sourav Singh Rawat <aidenfrostbite@gmail.com> Co-authored-by: Binbin <binloveplay1314@qq.com> Signed-off-by: Ran Shidlansik <ranshid@amazon.com>

There are currently several issues with the existing hash field expiration mechanism: 1. `HINCRBY` is propagated to the replica "as-is". It mean it relies on the fact that the state of the hash is the same on the primary and the replica. HFE did change this assumption as the field might be expired only when the replica will handle the propagated `hincrby`. the problem is that the replica does not "expire" fields by it's own. it needs to respect the request from the primary and always try to use the existing field. This can lead to either miss-alignment with the value on the primary and the replica AND even a disconnection since the replica might hold and "expired" field which is not in "integer" format... 2. HINCRBYFLOAT is currently ALWAYS propagating `hset` - this means that the expiration time of an entry will always be removed on the replica side (it needs to propagate HSETEX when expiration time needs to be maintained) 3. Currently all hash write commands which are mutating values might overwrite an expired field. In such cases the existing implementation will "silently" do so. The problem is that the user will not get any key-space-notificaiton explaining the reason for the behavior. For example, when `hincrby` is issued overwriting an expired field which was not yet "cleaned" by active-expiration it will reset the counter to '0' before incrementing it. this means that the user might ask: why is the value '1' and not bigger, "I did not see any notification that the old value expired"... 4. HSETEX with KEEPTTL suffers from a "somewhat" similar problem as if the primary "replaced" the entry which is expired now but might not have been expired when the primary applied it. There are 2 options for a solution: 1. we could propagate `hdel` for every entry we are "overwritting" (batch them if we can) 2. propagate the commands "by effect". For example - have `hincrby` always propagate either HSET or HSETEX. This will not solve the '#(4)' problem above though, for which we might HAVE to propagate `hdel` I tend to go with the second option. The reason is that it is expected to have less impact on replication stream and should include less processing time on the replicas and network traffic. Specifically for HSETEX with KEEPTTL we will have to propagate the `hdel` in case we overwritten an expired field, but that would help limit the impact of this propagation. --------- Signed-off-by: Ran Shidlansik <ranshid@amazon.com> Co-authored-by: Sourav Singh Rawat <aidenfrostbite@gmail.com> Co-authored-by: Binbin <binloveplay1314@qq.com> Signed-off-by: Ran Shidlansik <ranshid@amazon.com>

…ey-io#3060) There are currently several issues with the existing hash field expiration mechanism: 1. `HINCRBY` is propagated to the replica "as-is". It mean it relies on the fact that the state of the hash is the same on the primary and the replica. HFE did change this assumption as the field might be expired only when the replica will handle the propagated `hincrby`. the problem is that the replica does not "expire" fields by it's own. it needs to respect the request from the primary and always try to use the existing field. This can lead to either miss-alignment with the value on the primary and the replica AND even a disconnection since the replica might hold and "expired" field which is not in "integer" format... 2. HINCRBYFLOAT is currently ALWAYS propagating `hset` - this means that the expiration time of an entry will always be removed on the replica side (it needs to propagate HSETEX when expiration time needs to be maintained) 3. Currently all hash write commands which are mutating values might overwrite an expired field. In such cases the existing implementation will "silently" do so. The problem is that the user will not get any key-space-notificaiton explaining the reason for the behavior. For example, when `hincrby` is issued overwriting an expired field which was not yet "cleaned" by active-expiration it will reset the counter to '0' before incrementing it. this means that the user might ask: why is the value '1' and not bigger, "I did not see any notification that the old value expired"... 4. HSETEX with KEEPTTL suffers from a "somewhat" similar problem as #(1). the replica will receive the propagated command, but will not know if the primary "replaced" the entry which is expired now but might not have been expired when the primary applied it. There are 2 options for a solution: 1. we could propagate `hdel` for every entry we are "overwritting" (batch them if we can) 2. propagate the commands "by effect". For example - have `hincrby` always propagate either HSET or HSETEX. This will not solve the '#(4)' problem above though, for which we might HAVE to propagate `hdel` I tend to go with the second option. The reason is that it is expected to have less impact on replication stream and should include less processing time on the replicas and network traffic. Specifically for HSETEX with KEEPTTL we will have to propagate the `hdel` in case we overwritten an expired field, but that would help limit the impact of this propagation. --------- Signed-off-by: Ran Shidlansik <ranshid@amazon.com> Co-authored-by: Sourav Singh Rawat <aidenfrostbite@gmail.com> Co-authored-by: Binbin <binloveplay1314@qq.com>

propagate hdel when primary overrides expired hash field

e9138be

Signed-off-by: Ran Shidlansik <ranshid@amazon.com>

github-actions bot assigned ranshid Jan 13, 2026

frostzt reviewed Jan 13, 2026

View reviewed changes

src/t_hash.c Outdated Show resolved Hide resolved

frostzt reviewed Jan 13, 2026

View reviewed changes

src/t_hash.c Show resolved Hide resolved

frostzt reviewed Jan 13, 2026

View reviewed changes

src/t_hash.c Outdated Show resolved Hide resolved

ranshid added 2 commits January 14, 2026 10:10

add KSN verification to tests

37fe3e3

Signed-off-by: Ran Shidlansik <ranshid@amazon.com>

do not propagate anymore. replicate by effect when needed

f30fcc1

Signed-off-by: Ran Shidlansik <ranshid@amazon.com>

ranshid changed the title ~~propagate hdel when primary overrides expired hash field~~ Fix how hash is handling overriding of expired fields overwrite Jan 14, 2026

ranshid mentioned this pull request Jan 14, 2026

[BUG] HSETEX with KEEPTTL option may cause inconsistency between primary and replica #3036

Closed

cjx-zar reviewed Jan 14, 2026

View reviewed changes

src/t_hash.c Show resolved Hide resolved

ranshid mentioned this pull request Jan 14, 2026

propagate changes for hsetex with PXAT absolute timestamps ranshid/valkey#8

Merged

add tests for HSETEX with KEEPTTL and change hashTypeSet time verifi…

eea4361

…cation Signed-off-by: Ran Shidlansik <ranshid@amazon.com>

ranshid added bug Something isn't working release-notes This issue should get a line item in the release notes labels Jan 15, 2026

ranshid added this to Valkey 9.1 and Valkey 9.0 Jan 15, 2026

frostzt and others added 2 commits January 15, 2026 12:55

hsetex propagate hdel for overwritten expired fields and KEEPTTL prov…

b4da7a3

…ided Signed-off-by: Ran Shidlansik <ranshid@amazon.com>

refactor a bit

6af3175

Signed-off-by: Ran Shidlansik <ranshid@amazon.com>

ranshid force-pushed the fix-propagate-hdel-when-override-expired-element branch from 7faa9a2 to 6af3175 Compare January 15, 2026 11:14

ranshid marked this pull request as ready for review January 15, 2026 11:26

fix typo

cc26839

Signed-off-by: Ran Shidlansik <ranshid@amazon.com>

ranshid requested a review from enjoy-binbin January 15, 2026 12:00

cjx-zar reviewed Jan 17, 2026

View reviewed changes

src/server.h Outdated Show resolved Hide resolved

change argument name

81d9f35

Signed-off-by: Ran Shidlansik <ranshid@amazon.com>

madolson requested a review from murphyjacob4 January 19, 2026 17:27

murphyjacob4 reviewed Jan 19, 2026

View reviewed changes

src/t_hash.c Show resolved Hide resolved

cjx-zar reviewed Jan 21, 2026

View reviewed changes

src/db.c Outdated Show resolved Hide resolved

fix bound check

36eeac7

Signed-off-by: Ran Shidlansik <ranshid@amazon.com>

ranshid requested review from cjx-zar and murphyjacob4 January 22, 2026 05:56

cjx-zar approved these changes Jan 22, 2026

View reviewed changes

murphyjacob4 reviewed Jan 24, 2026

View reviewed changes

src/t_hash.c Show resolved Hide resolved

enjoy-binbin approved these changes Jan 25, 2026

View reviewed changes

src/t_hash.c Outdated Show resolved Hide resolved

src/t_hash.c Show resolved Hide resolved

src/t_hash.c Show resolved Hide resolved

ranshid and others added 2 commits January 25, 2026 09:12

Update src/t_hash.c

1380126

Co-authored-by: Binbin <binloveplay1314@qq.com> Signed-off-by: Ran Shidlansik <ranshid@amazon.com>

Update src/t_hash.c

ba1efbf

Co-authored-by: Binbin <binloveplay1314@qq.com> Signed-off-by: Ran Shidlansik <ranshid@amazon.com>

murphyjacob4 reviewed Jan 25, 2026

View reviewed changes

fix typos and improve test

1f426fb

Signed-off-by: Ran Shidlansik <ranshid@amazon.com>

murphyjacob4 approved these changes Jan 26, 2026

View reviewed changes

fix comment

0ea52ac

Signed-off-by: Ran Shidlansik <ranshid@amazon.com>

ranshid merged commit 94edae4 into valkey-io:unstable Jan 26, 2026
24 checks passed

github-project-automation bot moved this to To be backported in Valkey 9.0 Jan 26, 2026

github-project-automation bot moved this to Done in Valkey 9.1 Jan 26, 2026

zuiderkwast mentioned this pull request Jan 27, 2026

Backport hfe fixes to 9.0 #3111

Merged

cjx-zar reviewed Jan 28, 2026

View reviewed changes

tests/unit/hashexpire.tcl Show resolved Hide resolved

zuiderkwast moved this from To be backported to 9.0.2 WIP in Valkey 9.0 Jan 28, 2026

Comments

Conversation

ranshid commented Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

cjx-zar commented Jan 14, 2026

Uh oh!

ranshid commented Jan 15, 2026

Uh oh!

cjx-zar commented Jan 16, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cjx-zar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

enjoy-binbin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ranshid commented Jan 13, 2026 •

edited

Loading

codecov bot commented Jan 13, 2026 •

edited

Loading