Skip to content

fix(ttl): report -2 when a container key has become empty - #3770

Open
weim0000 wants to merge 7 commits into
apache:unstablefrom
weim0000:fix_ttl_cmd
Open

weim0000 wants to merge 7 commits into
apache:unstablefrom
weim0000:fix_ttl_cmd

Conversation

@weim0000

Copy link
Copy Markdown
Contributor

Issue

closes #3676

Description

After HDEL removes the last field of a hash, EXISTS returns 0 but TTL/PTTL
still return -1, while Redis returns -2 (key does not exist).

Redis removes the whole key once the last field is deleted, so a non-existent key
reports TTL -2:

# Redis 7.x
127.0.0.1:6379> HSET k f v
(integer) 1
127.0.0.1:6379> HDEL k f
(integer) 1
127.0.0.1:6379> EXISTS k
(integer) 0
127.0.0.1:6379> TTL k
(integer) -2
127.0.0.1:6379> PTTL k
(integer) -2

Kvrocks currently reports -1 for the same sequence, which is the bug:

# Kvrocks (before this fix)
127.0.0.1:6666> HSET k f v
(integer) 1
127.0.0.1:6666> HDEL k f
(integer) 1
127.0.0.1:6666> EXISTS k
(integer) 0
127.0.0.1:6666> TTL k
(integer) -1   # expected -2, as in Redis
127.0.0.1:6666> PTTL k
(integer) -1   # expected -2, as in Redis

The root cause is that Hash::Delete decrements the metadata size to 0 but keeps
the metadata instead of deleting it. EXISTS and TYPE rely on
Metadata::Expired(), which already treats a stored-but-empty container as
expired, so they report the key as gone. Database::TTL, however, decoded the
metadata and returned metadata.TTL() directly, leaking -1 from the leftover
expire == 0.

This change makes Database::TTL check Metadata::Expired() first and return -2,
keeping TTL/PTTL consistent with EXISTS and TYPE. Since it is applied on the
shared read path, it also covers any other container whose metadata is retained
after becoming empty.

The fix is on the read path only — no storage format, encoding, or replication
change, and no new column family.

Side effects

None. The change only aligns the reported TTL for an already-non-existent key.

Test

  • Added a C++ unit test RedisHashTest.TTLReportsKeyGoneAfterDeletingLastField
    asserting TTL is -1 while a field is present and -2 after deleting the last
    field.
  • Extended the Go integration test TestHash ("HDEL - hash becomes empty ...")
    to assert TTL/PTTL return -2 once the last field is deleted.
  • Ran the full TestExpire suite to confirm no regression for the other types on
    the shared Database::TTL path.

AI Usage Disclosure

  • The bug fix was implemented manually.
  • AI tools(Qoder) were used to assist in generating unit test scaffolding; all tests were reviewed and verified by the author.

@weim0000 weim0000 changed the title fix(hash): report TTL as -2 after HDEL removes the last field fix(ttl): report -2 when a container key has become empty Nov 10, 2026
@jihuayu
jihuayu requested a balanced review from Copilot November 10, 2026 14:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

No active review issues remain for the change aligning TTL and PTTL behavior for empty containers treated as expired.

No open issues 🎉

Review effort: Balanced

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.

TTL reports an existing key after HDEL removes the last field

4 participants