fix(hset): delete key if empty after HEXPIRE #6503
Conversation
Previously, HEXPIRE could leave an empty hash in the database if all fields were expired immediately. This "zombie" key caused the RDB serializer to crash during SAVE/BGSAVE operations. This change detects when the hash becomes empty inside OpHExpire and removes the key using DelMutable. DelMutable is required here to transfer ownership of the iterator and safely handle the AutoUpdater lifecycle. Signed-off-by: Gil Levkovich <[email protected]>
glevkovich
force-pushed
the
glevkovich/fix_save_entry_fatal_error_on_empty_hash
branch
from
f27908a to
6e92baf
Compare
🤖 Augment PR SummarySummary: Prevents HEXPIRE from leaving an empty ("zombie") hash key behind, which could crash RDB SAVE/BGSAVE. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Fixes a bug where HEXPIRE with an immediate expiration could leave an empty (“zombie”) hash key behind, which could crash RDB serialization during SAVE/BGSAVE.
Changes:
- Update
OpHExpireto delete the hash key when expiring fields makes it empty (usingDbSlice::DelMutable). - Add a regression test covering
HEXPIRE ... 0 ...ensuring the key is removed and an RDB save succeeds.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/server/hset_family.cc |
Deletes the hash key when HEXPIRE removes the last remaining field, preventing empty-hash serialization crashes. |
src/server/hset_family_test.cc |
Adds a test verifying HEXPIRE with TTL 0 deletes the key and that SAVE RDB succeeds. |
| // if the hash becomes empty. If the key remains (zombie), saving the RDB will crash | ||
| // and EXISTS |
There was a problem hiding this comment.
The comment is incomplete/unclear (line ends with “and EXISTS”). Please reword or remove this fragment so the intent is unambiguous.
| // if the hash becomes empty. If the key remains (zombie), saving the RDB will crash | |
| // and EXISTS | |
| // if the hash becomes empty. If the key remains (zombie), saving the RDB or running | |
| // commands like EXISTS against it may lead to crashes or other incorrect behavior. |
| // If it is a hash which became empty after expiring fields, we must delete the key safely. | ||
| // We use DelMutable which consumes the iterator/updater to prevent the crash. | ||
| if (pv->Encoding() == kEncodingStrMap2) { | ||
| auto* sm = static_cast<StringMap*>(pv->RObjPtr()); | ||
| if (sm->UpperBoundSize() == 0) { | ||
| db_slice.DelMutable(op_args.db_cntx, std::move(*op_res)); | ||
| } | ||
| } |
There was a problem hiding this comment.
The indentation in this newly added block doesn’t match the surrounding style in this file (2-space indent). Please reformat this block (e.g., clang-format) so the inner statements align consistently.
| Run({"HSET", "zombie", "f", "v"}); | ||
| auto resp = Run({"HEXPIRE", "zombie", "0", "FIELDS", "1", "f"}); | ||
| EXPECT_THAT(resp, IntArg(2)); | ||
| EXPECT_EQ(0, CheckedInt({"EXISTS", "zombie"})); | ||
| EXPECT_EQ(Run({"SAVE", "RDB", "zombie_test.rdb"}), "OK"); | ||
| } |
There was a problem hiding this comment.
This test writes an RDB snapshot to a hard-coded filename ("zombie_test.rdb"). In this fixture, FLAGS_dbfilename is empty and BaseFamilyTest::CleanupSnapshots() won’t remove this file, so it can leak artifacts into the build/test working directory and cause conflicts across runs. Consider calling InitWithDbFilename() and using SAVE RDB without an explicit basename (or use a basename that starts with the configured dbfilename), or explicitly delete the file at the end of the test.
romange
left a comment
romange
left a comment
There was a problem hiding this comment.
Lgtm but please address the comments
| auto resp = Run({"HEXPIRE", "zombie", "0", "FIELDS", "1", "f"}); | ||
| EXPECT_THAT(resp, IntArg(2)); | ||
| EXPECT_EQ(0, CheckedInt({"EXISTS", "zombie"})); | ||
| EXPECT_EQ(Run({"SAVE", "RDB", "zombie_test.rdb"}), "OK"); |
Previously, HEXPIRE could leave an empty hash in the database if all fields were expired immediately. This "zombie" key caused the RDB serializer to crash during SAVE/BGSAVE operations.
This change detects when the hash becomes empty inside OpHExpire and removes the key using DelMutable. DelMutable is required here to transfer ownership of the iterator and safely handle the AutoUpdater lifecycle.