Skip to content

Conversation

@cjx-zar
Copy link
Contributor

@cjx-zar cjx-zar commented Jan 5, 2026

The HFE uses EXPIRY_NONE(-1) for fields without a TTL. A bug exists in HSETEX and RDB loading where expiry > 0 is used to check for an expiration. This is problematic because 0 might be treated as no expiry in import-mode, instead of an already expired timestamp, leading to incorrect behavior.

@cjx-zar cjx-zar force-pushed the fix-zero-expiry-in-HFE branch from 03dfb48 to bb3c851 Compare January 5, 2026 05:50
@codecov
Copy link

codecov bot commented Jan 5, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.34%. Comparing base (e4a3e9f) to head (6c84f99).
⚠️ Report is 6 commits behind head on unstable.

Additional details and impacted files
@@            Coverage Diff            @@
##           unstable    #3006   +/-   ##
=========================================
  Coverage     74.34%   74.34%           
=========================================
  Files           129      129           
  Lines         70908    70911    +3     
=========================================
+ Hits          52714    52718    +4     
+ Misses        18194    18193    -1     
Files with missing lines Coverage Δ
src/rdb.c 77.04% <100.00%> (-0.23%) ⬇️
src/t_hash.c 94.92% <100.00%> (ø)

... and 22 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.

Copy link
Member

@ranshid ranshid left a comment

Choose a reason for hiding this comment

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

LGTM. I think we should just move the test to hashexpire.tcl

@ranshid ranshid changed the title make zero a valid ttl in HFE HFE make zero a valid ttl during import mode and data loading Jan 5, 2026
@enjoy-binbin enjoy-binbin moved this to To be backported in Valkey 9.0 Jan 5, 2026
@ranshid ranshid added the bug Something isn't working label Jan 5, 2026
Signed-off-by: cjx-zar <[email protected]>
@ranshid ranshid added the release-notes This issue should get a line item in the release notes label Jan 5, 2026
@cjx-zar
Copy link
Contributor Author

cjx-zar commented Jan 5, 2026

By the way, I'd like to ask a question.
The documentation states that HSETEX returns "0 if none of the provided fields' values ​​and/or expiration times were set."
However, if the timestamp is expired, and the command contains duplicate fields(e.g.hsetex myhash ex 0 fields 2 f1 v1 f1 v2) or some fields that doesn't exist, the return value will also be 0.
This seems inconsistent with the description of "none of ... were set."
Also, the example in the documentation, HSETEX EX 0 myhash FIELDS 3 f1 v1 f2 v2 f3 v3, has a syntax error and should be corrected to HSETEX myhash EX 0 FIELDS 3 f1 v1 f2 v2 f3 v3

@ranshid
Copy link
Member

ranshid commented Jan 5, 2026

The documentation states that HSETEX returns "0 if none of the provided fields' values ​​and/or expiration times were set."
However, if the timestamp is expired, and the command contains duplicate fields(e.g.hsetex myhash ex 0 fields 2 f1 v1 f1 v2) or some fields that doesn't exist, the return value will also be 0.
This seems inconsistent with the description of "none of ... were set."

True. this is somewhat related to: #3001 since in the original implementation it was done as if to ignore these actions from ever being performed. However I agree that if we decide that we will report these keyspace events, we will probably have to report that in the result.

@ranshid ranshid merged commit 5a3fb04 into valkey-io:unstable Jan 6, 2026
57 checks passed
jdheyburn pushed a commit to jdheyburn/valkey that referenced this pull request Jan 8, 2026
…-io#3006)

The HFE uses EXPIRY_NONE(-1) for fields without a TTL. A bug exists in
`HSETEX` and RDB loading where `expiry > 0` is used to check for an
expiration. This is problematic because `0` might be treated as no
expiry in `import-mode`, instead of an already expired timestamp,
leading to incorrect behavior.

---------

Signed-off-by: cjx-zar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working release-notes This issue should get a line item in the release notes

Projects

Status: To be backported
Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants