This is an automated email from the ASF dual-hosted git repository.
git-hulk pushed a commit to branch unstable
in repository https://gitbox.apache.org/repos/asf/kvrocks.git
The following commit(s) were added to refs/heads/unstable by this push:
new 6f1c45fdd fix(tests): prevent RocksDB races during test cleanup (#3671)
6f1c45fdd is described below
commit 6f1c45fdd9ca0253ad7f7a5fb96f173be64de611
Author: hulk <[email protected]>
AuthorDate: Wed Oct 14 10:00:59 2026 +0800
fix(tests): prevent RocksDB races during test cleanup (#3671)
RDB, storage, and compaction tests can remove database directories while
RocksDB is still open.
RDBTest::TearDown runs before fixture destruction. Other affected tests
remove directories before their local storage objects are destroyed.
Background purging can delete a file after remove_all discovers its
directory entry, causing intermittent filesystem errors.
The [original CI
job](https://github.com/apache/kvrocks/actions/runs/38224574981/job/114729326234)
failed during RDB teardown:
```text
Value of: clearDBDir(config_.db_dir)
Actual: false
Expected: true
```
The [follow-up CI
job](https://github.com/apache/kvrocks/actions/runs/38361167392/job/115176472110?pr=3671)
failed in Storage.ReplDataManagerFileExistsCRC:
```text
Value of: ec
Actual: true
Expected: false
```
Ensure test readers and contexts are destroyed first, then close RocksDB
and wait for background work before removing the database directories.
Assistant-By GPT-6
---
tests/cppunit/compact_test.cc | 43 +++++++------------------------------------
tests/cppunit/rdb_test.cc | 13 -------------
tests/cppunit/storage_test.cc | 20 +++++++-------------
tests/cppunit/test_base.h | 11 +++++++++++
4 files changed, 25 insertions(+), 62 deletions(-)
diff --git a/tests/cppunit/compact_test.cc b/tests/cppunit/compact_test.cc
index 1d0dc3789..2f2c634c8 100644
--- a/tests/cppunit/compact_test.cc
+++ b/tests/cppunit/compact_test.cc
@@ -31,6 +31,7 @@
#include "storage/compact_filter.h"
#include "storage/redis_metadata.h"
#include "storage/storage.h"
+#include "test_base.h"
#include "time_util.h"
#include "types/redis_hash.h"
#include "types/redis_timeseries.h"
@@ -135,6 +136,7 @@ TEST(Compact, Filter) {
config.db_dir = "compactdb";
config.slot_id_encoded = false;
+ auto cleanup = MakeDBDirCleanup(config.db_dir);
auto storage = std::make_unique<engine::Storage>(&config);
Status s = storage->Open();
assert(s.IsOK());
@@ -229,11 +231,6 @@ TEST(Compact, Filter) {
}
db->ReleaseSnapshot(read_options.snapshot);
- std::error_code ec;
- std::filesystem::remove_all(config.db_dir, ec);
- if (ec) {
- std::cout << "Encounter filesystem error: " << ec << std::endl;
- }
}
TEST_F(CompactHashFieldExpirationTest,
DropsExpiredTTLSubkeyWithoutChangingMetadata) {
@@ -363,6 +360,7 @@ TEST(Compact, KeepsLegacyHashSubkeyWithTTLLikePrefix) {
config.rocks_db.write_buffer_size = 1;
config.rocks_db.block_size = 100;
+ auto cleanup = MakeDBDirCleanup(config.db_dir);
auto storage = std::make_unique<engine::Storage>(&config);
Status s = storage->Open();
ASSERT_TRUE(s.IsOK()) << s.Msg();
@@ -400,13 +398,6 @@ TEST(Compact, KeepsLegacyHashSubkeyWithTTLLikePrefix) {
rs = storage->Get(ctx, ctx.GetReadOptions(), sub_key, &raw_value);
EXPECT_TRUE(rs.ok()) << rs.ToString();
EXPECT_EQ(raw_value, value);
-
- storage.reset();
- std::error_code ec;
- std::filesystem::remove_all(config.db_dir, ec);
- if (ec) {
- std::cout << "Encounter filesystem error: " << ec << std::endl;
- }
}
TEST_F(CompactHashFieldExpirationTest,
KeepsMalformedFieldExpirationSubkeyValue) {
@@ -446,6 +437,7 @@ TEST(Compact, SearchFilter) {
config.db_dir = "compactdb";
config.slot_id_encoded = false;
+ auto cleanup = MakeDBDirCleanup(config.db_dir);
auto storage = std::make_unique<engine::Storage>(&config);
auto s = storage->Open();
assert(s.IsOK());
@@ -503,12 +495,6 @@ TEST(Compact, SearchFilter) {
sg = storage->Get(ctx, rocksdb::ReadOptions(),
storage->GetCFHandle(ColumnFamilyID::Search), num_search_key,
&search_value);
ASSERT_TRUE(sg.IsNotFound());
-
- std::error_code ec;
- std::filesystem::remove_all(config.db_dir, ec);
- if (ec) {
- std::cout << "Encounter filesystem error: " << ec << std::endl;
- }
}
TEST(Compact, IndexFilter) {
@@ -516,6 +502,7 @@ TEST(Compact, IndexFilter) {
config.db_dir = "compactdb";
config.slot_id_encoded = false;
+ auto cleanup = MakeDBDirCleanup(config.db_dir);
auto storage = std::make_unique<engine::Storage>(&config);
auto s = storage->Open();
assert(s.IsOK());
@@ -553,12 +540,6 @@ TEST(Compact, IndexFilter) {
ASSERT_TRUE(timeseries->MGet(ctx, mget_option, false, &mget_result).ok());
ASSERT_EQ(mget_result.size(), 1);
ASSERT_EQ(mget_result[0].name, ts_keep_key);
-
- std::error_code ec;
- std::filesystem::remove_all(config.db_dir, ec);
- if (ec) {
- std::cout << "Encounter filesystem error: " << ec << std::endl;
- }
}
TEST(Compact, TSRetention) {
@@ -566,6 +547,7 @@ TEST(Compact, TSRetention) {
config.db_dir = "compactdb_tsretention";
config.slot_id_encoded = false;
+ auto cleanup = MakeDBDirCleanup(config.db_dir);
auto storage = std::make_unique<engine::Storage>(&config);
auto s = storage->Open();
assert(s.IsOK());
@@ -626,12 +608,6 @@ TEST(Compact, TSRetention) {
ASSERT_EQ(range_result.size(), 2);
ASSERT_EQ(range_result[0].ts, 10);
ASSERT_EQ(range_result[1].ts, 110);
-
- std::error_code ec;
- std::filesystem::remove_all(config.db_dir, ec);
- if (ec) {
- std::cout << "Encounter filesystem error: " << ec << std::endl;
- }
}
TEST(Compact, TSDownstreamSubKey) {
@@ -639,6 +615,7 @@ TEST(Compact, TSDownstreamSubKey) {
config.db_dir = "compactdb_tsdownstream";
config.slot_id_encoded = false;
+ auto cleanup = MakeDBDirCleanup(config.db_dir);
auto storage = std::make_unique<engine::Storage>(&config);
auto s = storage->Open();
assert(s.IsOK());
@@ -690,10 +667,4 @@ TEST(Compact, TSDownstreamSubKey) {
ds_keys = get_all_ds_key();
ASSERT_EQ(ds_keys.size(), 1);
ASSERT_EQ(ds_keys[0], dst_key2);
-
- std::error_code ec;
- std::filesystem::remove_all(config.db_dir, ec);
- if (ec) {
- std::cout << "Encounter filesystem error: " << ec << std::endl;
- }
}
diff --git a/tests/cppunit/rdb_test.cc b/tests/cppunit/rdb_test.cc
index 5c8e6dbd0..74ae3dc67 100644
--- a/tests/cppunit/rdb_test.cc
+++ b/tests/cppunit/rdb_test.cc
@@ -20,7 +20,6 @@
#include "storage/rdb/rdb.h"
#include <cmath>
-#include <filesystem>
#include <map>
#include <memory>
#include <string>
@@ -48,8 +47,6 @@ class RDBTest : public TestBase {
~RDBTest() override = default;
void SetUp() override { crc64_init(); }
- void TearDown() override { ASSERT_TRUE(clearDBDir(config_.db_dir)); }
-
void loadRdb(const std::string &path) {
auto stream_ptr = std::make_unique<RdbFileStream>(path);
auto s = stream_ptr->Open();
@@ -130,16 +127,6 @@ class RDBTest : public TestBase {
std::string ns_;
std::string tmp_rdb_;
-
- private:
- static bool clearDBDir(const std::string &path) {
- try {
- std::filesystem::remove_all(path);
- } catch (std::filesystem::filesystem_error &e) {
- return false;
- }
- return true;
- }
};
void RDBTest::encodingDataCheck() {
diff --git a/tests/cppunit/storage_test.cc b/tests/cppunit/storage_test.cc
index 881f8418b..0add8c185 100644
--- a/tests/cppunit/storage_test.cc
+++ b/tests/cppunit/storage_test.cc
@@ -30,6 +30,8 @@
#include <string>
#include <vector>
+#include "test_base.h"
+
TEST(Storage, CreateBackup) {
std::error_code ec;
@@ -40,6 +42,7 @@ TEST(Storage, CreateBackup) {
std::filesystem::remove_all(config.db_dir, ec);
ASSERT_TRUE(!ec);
+ auto cleanup = MakeDBDirCleanup(config.db_dir);
auto storage = std::make_unique<engine::Storage>(&config);
auto s = storage->Open();
ASSERT_TRUE(s.IsOK());
@@ -59,9 +62,6 @@ TEST(Storage, CreateBackup) {
// check if backup success without caring about the sequence number
s = storage->CreateBackup();
ASSERT_TRUE(s.IsOK());
-
- std::filesystem::remove_all(config.db_dir, ec);
- ASSERT_TRUE(!ec);
}
TEST(Storage, ReadOnlyTransactions) {
@@ -186,6 +186,7 @@ TEST(Storage, GetFullReplDataInfoRejectsEmptyCheckpoint) {
std::filesystem::create_directory(test_dir, ec);
ASSERT_FALSE(ec);
+ auto cleanup = MakeDBDirCleanup(test_dir);
auto storage = std::make_unique<engine::Storage>(&config);
auto s = storage->Open();
ASSERT_TRUE(s.IsOK()) << s.Msg();
@@ -208,9 +209,6 @@ TEST(Storage, GetFullReplDataInfoRejectsEmptyCheckpoint) {
s = engine::Storage::ReplDataManager::GetFullReplDataInfo(storage.get(),
&files);
EXPECT_FALSE(s.IsOK());
EXPECT_TRUE(files.empty());
-
- std::filesystem::remove_all(test_dir, ec);
- ASSERT_FALSE(ec);
}
TEST(Storage, TryPurgeCheckpoint) {
@@ -227,6 +225,7 @@ TEST(Storage, TryPurgeCheckpoint) {
std::filesystem::create_directory(test_dir, ec);
ASSERT_FALSE(ec);
+ auto cleanup = MakeDBDirCleanup(test_dir);
auto storage = std::make_unique<engine::Storage>(&config);
auto s = storage->Open();
ASSERT_TRUE(s.IsOK()) << s.Msg();
@@ -257,9 +256,6 @@ TEST(Storage, TryPurgeCheckpoint) {
s = engine::Storage::ReplDataManager::GetFullReplDataInfo(storage.get(),
&files);
ASSERT_TRUE(s.IsOK()) << s.Msg();
EXPECT_TRUE(storage->ExistCheckpoint());
-
- std::filesystem::remove_all(test_dir, ec);
- ASSERT_FALSE(ec);
}
TEST(Storage, ReplDataManagerFileExistsCRC) {
@@ -276,6 +272,7 @@ TEST(Storage, ReplDataManagerFileExistsCRC) {
std::filesystem::create_directories(test_dir + "/sync", ec);
ASSERT_FALSE(ec);
+ auto cleanup = MakeDBDirCleanup(test_dir);
auto storage = std::make_unique<engine::Storage>(&config);
auto s = storage->Open();
ASSERT_TRUE(s.IsOK()) << s.Msg();
@@ -335,7 +332,4 @@ TEST(Storage, ReplDataManagerFileExistsCRC) {
std::string file_name = "large_file";
EXPECT_TRUE(engine::Storage::ReplDataManager::FileExists(storage.get(),
test_dir + "/sync", file_name, 0));
}
-
- std::filesystem::remove_all(test_dir, ec);
- ASSERT_FALSE(ec);
-}
\ No newline at end of file
+}
diff --git a/tests/cppunit/test_base.h b/tests/cppunit/test_base.h
index fd691e59b..16ae9472d 100644
--- a/tests/cppunit/test_base.h
+++ b/tests/cppunit/test_base.h
@@ -24,10 +24,21 @@
#include <filesystem>
#include <fstream>
+#include <string>
+#include "common/scope_exit.h"
#include "storage/redis_db.h"
#include "types/redis_hash.h"
+// Declare before storage so the database is closed before removing its
directory.
+inline auto MakeDBDirCleanup(const std::string &path) {
+ return MakeScopeExit([path] {
+ std::error_code ec;
+ std::filesystem::remove_all(path, ec);
+ ASSERT_FALSE(ec) << "Failed to remove database directory " << path << ": "
<< ec.message();
+ });
+}
+
class TestFixture { // NOLINT
public:
TestFixture(TestFixture &&) = delete;
