From 479472813aa26a7a0d713845b2955e411bc2a557 Mon Sep 17 00:00:00 2001 From: bneradt Date: Tue, 11 Aug 2026 20:05:13 -0500 Subject: [PATCH 1/2] Fix records_yaml startup lookup race The records_yaml AuTest intermittently reported that proxy.config.diags.output.note was missing when a configuration callback ran while startup was still registering metrics. Metrics::find() returned the then-current end iterator for a miss, but RecLookupRecord() compared it with a new end iterator. An intervening registration made them differ and misread the missing string record as a numeric metric. This patch addresses the race by using the direct metrics lookup API, which reports a miss without comparing two moving end positions. It also adds a concurrent registration test that exercises string record lookups during metric creation. --- src/records/RecCore.cc | 20 +++++----- src/records/unit_tests/test_RecRegister.cc | 45 ++++++++++++++++++++++ 2 files changed, 56 insertions(+), 9 deletions(-) diff --git a/src/records/RecCore.cc b/src/records/RecCore.cc index c5daa449bf6..859620a5fa0 100644 --- a/src/records/RecCore.cc +++ b/src/records/RecCore.cc @@ -516,18 +516,20 @@ RecGetRecordCounter(const char *name, bool lock) RecErrT RecLookupRecord(const char *name, void (*callback)(const RecRecord *, void *), void *data, bool lock) { - RecErrT err = REC_ERR_FAIL; - ts::Metrics &metrics = ts::Metrics::instance(); - auto it = metrics.find(name); + RecErrT err = REC_ERR_FAIL; + ts::Metrics &metrics = ts::Metrics::instance(); + ts::Metrics::IdType metric_id; - if (it != metrics.end()) { - RecRecord r; - auto &&[name, type, val] = *it; + // A metric's storage is stable after creation. Avoid find()/end() here because end() is the current insertion position and + // can advance between those two calls while another thread registers a metric. + if (auto *metric = metrics.lookup(name, &metric_id); metric != nullptr) { + RecRecord r{}; r.rec_type = RECT_PLUGIN; - r.data_type = type == ts::Metrics::MetricType::COUNTER ? RECD_COUNTER : RECD_INT; - r.name = name.data(); - r.data.rec_int = val; + r.data_type = metrics.type(metric_id) == ts::Metrics::MetricType::COUNTER ? RECD_COUNTER : RECD_INT; + r.name = name; + r.data.rec_int = metric->load(); + r.registered = true; callback(&r, data); err = REC_ERR_OKAY; diff --git a/src/records/unit_tests/test_RecRegister.cc b/src/records/unit_tests/test_RecRegister.cc index 77d4c287625..0448d4779b4 100644 --- a/src/records/unit_tests/test_RecRegister.cc +++ b/src/records/unit_tests/test_RecRegister.cc @@ -17,11 +17,16 @@ or implied. See the License for the specific language governing permissions and limitations under the License. */ +#include +#include +#include + #include #include "records/RecCore.h" #include "iocore/eventsystem/EventSystem.h" #include "iocore/eventsystem/RecProcess.h" #include "tscore/Layout.h" +#include "tsutil/Metrics.h" #include "test_Diags.h" TEST_CASE("RecRegisterConfig - Type Dispatch", "[librecords][RecConfig]") @@ -87,3 +92,43 @@ TEST_CASE("RecRegisterStat - Type Dispatch", "[librecords][RecStat]") REQUIRE(value == 500); } } + +TEST_CASE("RecLookupRecord - Concurrent metric registration", "[librecords][RecLookup]") +{ + constexpr char record_name[] = "proxy.test.concurrent.string_value"; + constexpr char record_value[] = "stable"; + + REQUIRE(RecRegisterConfigString(RECT_CONFIG, record_name, record_value, RECU_DYNAMIC, RECC_NULL, nullptr, REC_SOURCE_NULL) == + REC_ERR_OKAY); + + std::atomic start{false}; + std::atomic finished{false}; + std::jthread register_metrics([&]() { + while (!start.load(std::memory_order_acquire)) { + std::this_thread::yield(); + } + + for (int i = 0; i < 100000; ++i) { + auto metric_name = std::string{"proxy.process.test.concurrent_metric_registration."} + std::to_string(i); + + ts::Metrics::Counter::create(metric_name); + } + finished.store(true, std::memory_order_release); + }); + + bool all_lookups_succeeded = true; + size_t lookup_count = 0; + + start.store(true, std::memory_order_release); + while (!finished.load(std::memory_order_acquire)) { + if (RecGetRecordStringAlloc(record_name) != record_value) { + all_lookups_succeeded = false; + break; + } + ++lookup_count; + } + register_metrics.join(); + + CHECK(lookup_count > 0); + CHECK(all_lookups_succeeded); +} From 23b72737c7bf7a0847f681551ed13daf4f6b2efa Mon Sep 17 00:00:00 2001 From: bneradt Date: Tue, 11 Aug 2026 21:14:42 -0500 Subject: [PATCH 2/2] Use std::thread in records lookup test The macOS CI toolchain does not provide std::jthread, which prevents test_records from compiling before the regression test can run. This uses std::thread instead. The test already joins the worker explicitly, so its behavior and lifetime remain unchanged. --- src/records/unit_tests/test_RecRegister.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/records/unit_tests/test_RecRegister.cc b/src/records/unit_tests/test_RecRegister.cc index 0448d4779b4..cf749aa7a4c 100644 --- a/src/records/unit_tests/test_RecRegister.cc +++ b/src/records/unit_tests/test_RecRegister.cc @@ -103,7 +103,7 @@ TEST_CASE("RecLookupRecord - Concurrent metric registration", "[librecords][RecL std::atomic start{false}; std::atomic finished{false}; - std::jthread register_metrics([&]() { + std::thread register_metrics([&]() { while (!start.load(std::memory_order_acquire)) { std::this_thread::yield(); }