diff --git a/src/metric.cpp b/src/metric.cpp index a5eb903..1bd815b 100644 --- a/src/metric.cpp +++ b/src/metric.cpp @@ -217,6 +217,9 @@ struct Metric { static Counter create_counter_instance( Family *family, const std::vector> &labels) { + // Force thread_local initialization + (void)thread_init; + std::unique_lock _{mutex}; LabelsKey key{labels}; @@ -262,6 +265,9 @@ struct Metric { static Histogram create_histogram_instance( Family *family, const std::vector> &labels) { + // Force thread_local initialization + (void)thread_init; + std::unique_lock _{mutex}; LabelsKey key{labels}; auto &ptr = diff --git a/tests/test_metric.cpp b/tests/test_metric.cpp index 6513761..3074a49 100644 --- a/tests/test_metric.cpp +++ b/tests/test_metric.cpp @@ -483,6 +483,113 @@ TEST_CASE("thread safety") { } } +TEST_CASE("thread counter cleanup bug") { + SUBCASE( + "counter and histogram values should persist after thread destruction") { + auto counter_family = metric::create_counter( + "thread_cleanup_counter", "Counter for thread cleanup test"); + auto histogram_family = metric::create_histogram( + "thread_cleanup_histogram", "Histogram for thread cleanup test", + metric::linear_buckets(0.0, 1.0, 5)); // buckets: 0, 1, 2, 3, 4 + + // Variables to collect actual values from worker thread + double counter_value_in_thread = 0; + double histogram_sum_in_thread = 0; + + // Create thread that increments metrics and then exits + std::thread worker([&]() { + auto counter = counter_family.create({{"worker", "cleanup_test"}}); + auto histogram = histogram_family.create({{"worker", "cleanup_test"}}); + + counter.inc(1.0); + histogram.observe(1.5); // Should contribute to sum + + // Measure actual values from within the thread (before ThreadInit + // destructor runs) + ArenaAllocator thread_arena; + auto thread_output = metric::render(thread_arena); + + for (const auto &line : thread_output) { + if (line.find("thread_cleanup_counter{worker=\"cleanup_test\"}") != + std::string_view::npos) { + auto space_pos = line.rfind(' '); + if (space_pos != std::string_view::npos) { + auto value_str = line.substr(space_pos + 1); + if (value_str.back() == '\n') { + value_str.remove_suffix(1); + } + counter_value_in_thread = std::stod(std::string(value_str)); + } + } + if (line.find( + "thread_cleanup_histogram_sum{worker=\"cleanup_test\"}") != + std::string_view::npos) { + auto space_pos = line.rfind(' '); + if (space_pos != std::string_view::npos) { + auto value_str = line.substr(space_pos + 1); + if (value_str.back() == '\n') { + value_str.remove_suffix(1); + } + histogram_sum_in_thread = std::stod(std::string(value_str)); + } + } + } + }); + + // Wait for thread to complete and destroy (triggering ThreadInit + // destructor) + worker.join(); + + // Measure values after thread cleanup + ArenaAllocator arena; + auto output = metric::render(arena); + + double counter_value_after = 0; + double histogram_sum_after = 0; + + for (const auto &line : output) { + if (line.find("thread_cleanup_counter{worker=\"cleanup_test\"}") != + std::string_view::npos) { + auto space_pos = line.rfind(' '); + if (space_pos != std::string_view::npos) { + auto value_str = line.substr(space_pos + 1); + if (value_str.back() == '\n') { + value_str.remove_suffix(1); + } + counter_value_after = std::stod(std::string(value_str)); + } + } + if (line.find("thread_cleanup_histogram_sum{worker=\"cleanup_test\"}") != + std::string_view::npos) { + auto space_pos = line.rfind(' '); + if (space_pos != std::string_view::npos) { + auto value_str = line.substr(space_pos + 1); + if (value_str.back() == '\n') { + value_str.remove_suffix(1); + } + histogram_sum_after = std::stod(std::string(value_str)); + } + } + } + + // Values should have been captured correctly within the thread + CHECK(counter_value_in_thread == 1.0); + CHECK(histogram_sum_in_thread == 1.5); + + // The bug: These values should persist after thread cleanup but will be + // lost because ThreadInit destructor erases per-thread state without + // accumulating values + CHECK(counter_value_after == 1.0); + CHECK(histogram_sum_after == 1.5); + + // The bug: After thread destruction, the counter and histogram values are + // lost because ThreadInit::~ThreadInit() calls + // family->perThreadState.erase(thread_id) without accumulating the values + // into global storage first. This causes counter values to "go backwards" + // when threads are destroyed, violating the monotonic property of counters. + } +} + TEST_CASE("error conditions") { SUBCASE("counter negative increment") { auto counter_family = metric::create_counter("error_counter", "Error test");