Skip to content

Commit 771e63f

Browse files
Takuka0311Takuka0311
committed
fix(checkpoint): keep active file checkpoint on dump key collision
Fake rotation (a new file reusing a deleted file's inode) makes two readers dump checkpoints under the same (dev, inode, configName) key. Last-writer-wins let a stale reader of an already-deleted file overwrite the active reader's offset, causing re-collection from the beginning and duplicate logs. Apply active-priority in AddCheckPoint: on key collision, keep the checkpoint whose file still exists (probing host and real path). Fall back to legacy last-writer-wins when neither or both files exist, so no state is lost. Order-independent and only stats on collision. Fixes #2629
1 parent d052b5f commit 771e63f

2 files changed

Lines changed: 164 additions & 5 deletions

File tree

‎core/file_server/checkpoint/CheckPointManager.cpp‎

Lines changed: 36 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -47,13 +47,44 @@ bool CheckPointManager::CheckVersion() {
4747
return (mLoadVersion == NO_CHECKPOINT_VERSION) || (mLoadVersion / 10000 == INT32_FLAG(check_point_version) / 10000);
4848
}
4949

50+
namespace {
51+
// A checkpoint's physical file still exists when its path currently resolves to the
52+
// same (dev, inode). After a fake rotation the deleted file's path is gone or points
53+
// to a different inode, so such a checkpoint is treated as stale. Both the host path
54+
// and the resolved real path are probed so a rotated-but-present file is not misjudged.
55+
bool CheckPointFileStillExists(const CheckPoint& checkPoint) {
56+
if (checkPoint.mDevInode == GetFileDevInode(checkPoint.mFileName)) {
57+
return true;
58+
}
59+
if (!checkPoint.mRealFileName.empty() && checkPoint.mDevInode == GetFileDevInode(checkPoint.mRealFileName)) {
60+
return true;
61+
}
62+
return false;
63+
}
64+
} // namespace
65+
5066
void CheckPointManager::AddCheckPoint(CheckPoint* checkPointPtr) {
51-
DevInodeCheckPointHashMap::iterator it
52-
= mDevInodeCheckPointPtrMap.find(CheckPointKey(checkPointPtr->mDevInode, checkPointPtr->mConfigName));
53-
if (it != mDevInodeCheckPointPtrMap.end())
67+
CheckPointPtr newCheckPoint(checkPointPtr);
68+
CheckPointKey key(newCheckPoint->mDevInode, newCheckPoint->mConfigName);
69+
DevInodeCheckPointHashMap::iterator it = mDevInodeCheckPointPtrMap.find(key);
70+
if (it != mDevInodeCheckPointPtrMap.end()) {
71+
// Active-priority on key collision: the key (dev, inode, configName) cannot tell apart
72+
// two physical files that reuse the same inode (fake rotation). Keep the checkpoint whose
73+
// file still exists, so a stale reader of an already-deleted file cannot overwrite the
74+
// active reader's offset and cause re-collection from the beginning. When neither or both
75+
// files exist, fall back to the legacy last-writer-wins behavior to avoid losing state.
76+
const CheckPointPtr& oldCheckPoint = it->second;
77+
if (!CheckPointFileStillExists(*newCheckPoint) && CheckPointFileStillExists(*oldCheckPoint)) {
78+
LOG_INFO(
79+
sLogger,
80+
("skip stale checkpoint", "keep active file checkpoint")("config", newCheckPoint->mConfigName)(
81+
"dev", ToString(newCheckPoint->mDevInode.dev))("inode", ToString(newCheckPoint->mDevInode.inode))(
82+
"stale file", newCheckPoint->mFileName)("active file", oldCheckPoint->mFileName));
83+
return;
84+
}
5485
mDevInodeCheckPointPtrMap.erase(it);
55-
mDevInodeCheckPointPtrMap.insert(std::make_pair<CheckPointKey, CheckPointPtr>(
56-
CheckPointKey(checkPointPtr->mDevInode, checkPointPtr->mConfigName), CheckPointPtr(checkPointPtr)));
86+
}
87+
mDevInodeCheckPointPtrMap.insert(std::make_pair(key, newCheckPoint));
5788
}
5889

5990
void CheckPointManager::DeleteCheckPoint(DevInode devInode, const std::string& configName) {

‎core/unittest/checkpoint/CheckpointManagerUnittest.cpp‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,37 @@ class CheckpointManagerUnittest : public ::testing::Test {
3535
static void TearDownTestCase() { bfs::remove_all(kTestRootDir); }
3636

3737
void TestSearchFilePathByDevInodeInDirectory();
38+
void TestAddCheckPointStaleFileNotOverwriteActive();
39+
void TestAddCheckPointActiveOverwriteStale();
40+
void TestAddCheckPointBothExistLastWriterWins();
41+
void TestAddCheckPointNeitherExistLastWriterWins();
42+
43+
private:
44+
static CheckPoint* MakeCheckPoint(const std::string& fileName,
45+
const DevInode& devInode,
46+
int64_t offset,
47+
const std::string& configName,
48+
const std::string& realFileName = "") {
49+
return new CheckPoint(fileName,
50+
"" /* resolvedFileName */,
51+
offset,
52+
0 /* signatureSize */,
53+
0 /* signatureHash */,
54+
devInode,
55+
configName,
56+
realFileName,
57+
false /* fileOpenFlag */,
58+
false /* containerStopped */,
59+
"" /* containerID */,
60+
false /* lastForceRead */);
61+
}
3862
};
3963

4064
UNIT_TEST_CASE(CheckpointManagerUnittest, TestSearchFilePathByDevInodeInDirectory);
65+
UNIT_TEST_CASE(CheckpointManagerUnittest, TestAddCheckPointStaleFileNotOverwriteActive);
66+
UNIT_TEST_CASE(CheckpointManagerUnittest, TestAddCheckPointActiveOverwriteStale);
67+
UNIT_TEST_CASE(CheckpointManagerUnittest, TestAddCheckPointBothExistLastWriterWins);
68+
UNIT_TEST_CASE(CheckpointManagerUnittest, TestAddCheckPointNeitherExistLastWriterWins);
4169

4270
void CheckpointManagerUnittest::TestSearchFilePathByDevInodeInDirectory() {
4371
const std::string kRotateFileName = "test.log.5";
@@ -96,6 +124,106 @@ void CheckpointManagerUnittest::TestSearchFilePathByDevInodeInDirectory() {
96124
}
97125
}
98126

127+
// Fake rotation: a new file B reuses the inode of a deleted file A. Both readers dump a
128+
// checkpoint under the same (dev, inode, config) key. The stale checkpoint of the deleted
129+
// file A must not overwrite the active checkpoint of the still-existing file B.
130+
void CheckpointManagerUnittest::TestAddCheckPointStaleFileNotOverwriteActive() {
131+
CheckPointManager::Instance()->RemoveAllCheckPoint();
132+
const std::string configName = "test-config";
133+
const std::string activeFile = (bfs::path(kTestRootDir) / "active.log").string();
134+
const std::string deletedFile = (bfs::path(kTestRootDir) / "deleted.log").string();
135+
std::ofstream(activeFile) << "active";
136+
// deletedFile is never created on disk -> its checkpoint is stale.
137+
138+
fsutil::PathStat ps;
139+
EXPECT_TRUE(fsutil::PathStat::stat(activeFile, ps));
140+
const DevInode reusedDevInode = ps.GetDevInode();
141+
142+
// Active reader dumps first, then the stale reader of the deleted file (same inode).
143+
CheckPointManager::Instance()->AddCheckPoint(MakeCheckPoint(activeFile, reusedDevInode, 1000, configName));
144+
CheckPointManager::Instance()->AddCheckPoint(MakeCheckPoint(deletedFile, reusedDevInode, 0, configName));
145+
146+
CheckPointPtr cpt;
147+
EXPECT_TRUE(CheckPointManager::Instance()->GetCheckPoint(reusedDevInode, configName, cpt));
148+
EXPECT_EQ(cpt->mFileName, activeFile);
149+
EXPECT_EQ(cpt->mOffset, 1000);
150+
EXPECT_EQ(CheckPointManager::Instance()->GetAllFileCheckPoint().size(), 1UL);
151+
152+
CheckPointManager::Instance()->RemoveAllCheckPoint();
153+
bfs::remove(activeFile);
154+
}
155+
156+
// Order independence: even when the stale checkpoint is added first, the later active
157+
// checkpoint of the still-existing file must win.
158+
void CheckpointManagerUnittest::TestAddCheckPointActiveOverwriteStale() {
159+
CheckPointManager::Instance()->RemoveAllCheckPoint();
160+
const std::string configName = "test-config";
161+
const std::string activeFile = (bfs::path(kTestRootDir) / "active2.log").string();
162+
const std::string deletedFile = (bfs::path(kTestRootDir) / "deleted2.log").string();
163+
std::ofstream(activeFile) << "active";
164+
165+
fsutil::PathStat ps;
166+
EXPECT_TRUE(fsutil::PathStat::stat(activeFile, ps));
167+
const DevInode reusedDevInode = ps.GetDevInode();
168+
169+
CheckPointManager::Instance()->AddCheckPoint(MakeCheckPoint(deletedFile, reusedDevInode, 0, configName));
170+
CheckPointManager::Instance()->AddCheckPoint(MakeCheckPoint(activeFile, reusedDevInode, 2000, configName));
171+
172+
CheckPointPtr cpt;
173+
EXPECT_TRUE(CheckPointManager::Instance()->GetCheckPoint(reusedDevInode, configName, cpt));
174+
EXPECT_EQ(cpt->mFileName, activeFile);
175+
EXPECT_EQ(cpt->mOffset, 2000);
176+
EXPECT_EQ(CheckPointManager::Instance()->GetAllFileCheckPoint().size(), 1UL);
177+
178+
CheckPointManager::Instance()->RemoveAllCheckPoint();
179+
bfs::remove(activeFile);
180+
}
181+
182+
// Both files exist (e.g. hard links sharing an inode): legacy last-writer-wins is preserved.
183+
void CheckpointManagerUnittest::TestAddCheckPointBothExistLastWriterWins() {
184+
CheckPointManager::Instance()->RemoveAllCheckPoint();
185+
const std::string configName = "test-config";
186+
const std::string file1 = (bfs::path(kTestRootDir) / "link1.log").string();
187+
const std::string file2 = (bfs::path(kTestRootDir) / "link2.log").string();
188+
std::ofstream(file1) << "data";
189+
bfs::create_hard_link(file1, file2);
190+
191+
fsutil::PathStat ps;
192+
EXPECT_TRUE(fsutil::PathStat::stat(file1, ps));
193+
const DevInode devInode = ps.GetDevInode();
194+
195+
CheckPointManager::Instance()->AddCheckPoint(MakeCheckPoint(file1, devInode, 100, configName));
196+
CheckPointManager::Instance()->AddCheckPoint(MakeCheckPoint(file2, devInode, 200, configName));
197+
198+
CheckPointPtr cpt;
199+
EXPECT_TRUE(CheckPointManager::Instance()->GetCheckPoint(devInode, configName, cpt));
200+
EXPECT_EQ(cpt->mFileName, file2);
201+
EXPECT_EQ(cpt->mOffset, 200);
202+
203+
CheckPointManager::Instance()->RemoveAllCheckPoint();
204+
bfs::remove(file1);
205+
bfs::remove(file2);
206+
}
207+
208+
// Neither file exists (both stale): fall back to legacy last-writer-wins, no state dropped.
209+
void CheckpointManagerUnittest::TestAddCheckPointNeitherExistLastWriterWins() {
210+
CheckPointManager::Instance()->RemoveAllCheckPoint();
211+
const std::string configName = "test-config";
212+
const std::string gone1 = (bfs::path(kTestRootDir) / "gone1.log").string();
213+
const std::string gone2 = (bfs::path(kTestRootDir) / "gone2.log").string();
214+
const DevInode devInode(12345, 67890); // does not match any real file
215+
216+
CheckPointManager::Instance()->AddCheckPoint(MakeCheckPoint(gone1, devInode, 100, configName));
217+
CheckPointManager::Instance()->AddCheckPoint(MakeCheckPoint(gone2, devInode, 200, configName));
218+
219+
CheckPointPtr cpt;
220+
EXPECT_TRUE(CheckPointManager::Instance()->GetCheckPoint(devInode, configName, cpt));
221+
EXPECT_EQ(cpt->mFileName, gone2);
222+
EXPECT_EQ(cpt->mOffset, 200);
223+
224+
CheckPointManager::Instance()->RemoveAllCheckPoint();
225+
}
226+
99227
} // namespace logtail
100228

101229
UNIT_TEST_MAIN

0 commit comments

Comments
 (0)