Skip to content

Commit f4bb2c9

Browse files
generatedunixname26860585020226206meta-codesync[bot]
authored andcommitted
Fix flaky mcrouter PreprocessedConfigDumpTest ms-timestamp collision
Summary: `mcrouter:preprocessed_config_dump_test` flaked after dumping five configs in one tight loop. Backup filenames use millisecond timestamps, so sub-ms dumps collided and the test sometimes kept 2 files instead of 3. Prod dumps are naturally spaced; the bug is test-only. This injects deterministic timestamps in the test while leaving prod on wall clock. Sentinel-Harness: claude *Modify your team agent prompt, check stats, and leave feedback: https://www.internalfb.com/sentinel_agent/rotations/cacheclient* Model used: Claude Opus 4.8 Reviewed By: ghostonhuang Differential Revision: D112753398 fbshipit-source-id: 5a3858ec4230d33db77e8e9d02dcec1d056fd406
1 parent d78e00b commit f4bb2c9

3 files changed

Lines changed: 20 additions & 6 deletions

File tree

mcrouter/CarbonRouterInstanceBase.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -345,7 +345,8 @@ int32_t CarbonRouterInstanceBase::getStatsEnabledPoolIndex(
345345

346346
struct PreprocessedConfigDumpTag;
347347

348-
void CarbonRouterInstanceBase::dumpPreprocessedConfigToDisk() {
348+
void CarbonRouterInstanceBase::dumpPreprocessedConfigToDisk(
349+
std::optional<int64_t> timestampMsOverride) {
349350
if (!isDumpPreprocessedConfigEnabled(opts_)) {
350351
return;
351352
}
@@ -383,10 +384,10 @@ void CarbonRouterInstanceBase::dumpPreprocessedConfigToDisk() {
383384
// Create timestamped backup only if max_preprocessed_config_history > 0
384385
if (opts_.max_preprocessed_config_history > 0) {
385386
try {
386-
auto timestampMs =
387+
auto timestampMs = timestampMsOverride.value_or(
387388
std::chrono::duration_cast<std::chrono::milliseconds>(
388389
std::chrono::system_clock::now().time_since_epoch())
389-
.count();
390+
.count());
390391
auto timestampFilename = getPpcFilename(
391392
opts_.service_name, opts_.flavor_name, timestampMs);
392393
auto timestampFilePath = (directory / timestampFilename).string();

mcrouter/CarbonRouterInstanceBase.h

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,9 @@
99

1010
#include <atomic>
1111
#include <chrono>
12+
#include <cstdint>
1213
#include <memory>
14+
#include <optional>
1315
#include <unordered_map>
1416

1517
#include <folly/Synchronized.h>
@@ -283,8 +285,10 @@ class CarbonRouterInstanceBase {
283285
*/
284286
void deregisterForStatsUpdates();
285287

286-
// Dumps preprocessed config to disk on reconfiguration
287-
void dumpPreprocessedConfigToDisk();
288+
// Dumps preprocessed config to disk on reconfiguration. timestampMsOverride
289+
// is a test-only seam to fix the backup filename timestamp; unset in prod.
290+
void dumpPreprocessedConfigToDisk(
291+
std::optional<int64_t> timestampMsOverride = std::nullopt);
288292

289293
const McrouterOptions opts_;
290294
const pid_t pid_;
@@ -351,6 +355,10 @@ class CarbonRouterInstanceBase {
351355
dumpPreprocessedConfigToDisk();
352356
}
353357

358+
void dumpPreprocessedConfigToDiskForTesting(int64_t timestampMs) {
359+
dumpPreprocessedConfigToDisk(timestampMs);
360+
}
361+
354362
private:
355363
// Track preprocessed config files to avoid unnecessary rewrites
356364
std::string preprocessedConfigFileMD5Hash_;

mcrouter/test/PreprocessedConfigDumpTest.cpp

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -486,6 +486,11 @@ TEST_F(PreprocessedConfigDumpTest, TimestampedFilesCleanupWorksCorrectly) {
486486

487487
TestableRouterInstance instance(std::move(opts), std::move(mockConfigApi));
488488

489+
// Backups are named ppc_<ms>.json. Pass an explicit, increasing timestamp per
490+
// dump so each writes a distinct file deterministically, without relying on
491+
// wall-clock spacing between rapid dumps.
492+
constexpr int64_t kBaseTimestampMs = 1700000000000;
493+
489494
// Create 5 different configs to trigger 5 timestamped files
490495
for (int i = 0; i < 5; ++i) {
491496
std::string config = fmt::format(
@@ -508,7 +513,7 @@ TEST_F(PreprocessedConfigDumpTest, TimestampedFilesCleanupWorksCorrectly) {
508513
::testing::SetArgReferee<1>("test_config_path"),
509514
::testing::Return(true)));
510515

511-
instance.dumpPreprocessedConfigToDiskForTesting();
516+
instance.dumpPreprocessedConfigToDiskForTesting(kBaseTimestampMs + i);
512517
}
513518

514519
// Should only have 3 timestamped files (oldest 2 should be deleted)

0 commit comments

Comments
 (0)