Metrics: close the id lookup race and bounds gaps left by the lock revert

[cmcfarlen]

Follow-ups to #13567, which reverted the locking that #13310 had added to the ts::Metrics::Storage
read paths. That revert restored the performance but left the data race #13310 was closing, plus
some pre-existing bounds problems in the same functions. This closes the race without a lock, and
fixes the bounds.

One gate for id validation

valid(), lookup(IdType), name() and rename() each carried their own copy of the same range
test, and the copies disagreed. valid() rejected an offset past MAX_SIZE; the other three did
not. _splitID passes the low 16 bits of an id through unmasked and the offset check only applied
when the id named the current blob, so an id such as 0x0000FFFF indexed well past the end of a
blob's 1024 entry arrays once a second blob existed. Ids reaching these accessors come from plugins
through the TSStat* API, so they are untrusted.

All four now go through Storage::_is_allocated(). It also fixes an off-by-one: create() returns
the id and then advances, so the next free slot was being accepted as allocated. An increment there
landed on the slot create() would hand out next, and since create() writes only the name and
never the value, the next plugin to call TSStatCreate() received a metric already carrying someone
else's count.

Nothing depended on the loose bound: end() builds an id at the allocation point that is compared
but never dereferenced, iterator::next() keeps the offset in range, and find() returns end()
on a miss.

One published position

_cur_blob and _cur_off are gone, replaced by a single atomic holding the blob index above the
offset, packed as _makeId packs one.

Two atomics cannot be read as a coherent pair. addBlob() reset the offset before advancing the
blob, so a reader between the two stores saw the old blob index with a zero offset and rejected
every id in the just-completed blob. That is not a dropped increment: with the default
ENABLE_FAST_SDK=OFF, TSStatInt* feeds the result to sdk_assert and aborts through
_TSReleaseAssert. Ordering the two stores the other way only trades it for accepting ids in a blob
nothing has been written to yet.

One value removes the window rather than relocating it: crossing a blob is a single release store,
sequenced after the blob pointer it publishes. The position only ever increases, since (N+1)<<16
exceeds N<<16 + offset for any offset < MAX_SIZE, so an id is allocated exactly when it packs
below the bound and _is_allocated() is one acquire load and one compare. Acquiring the bound
acquires the blob install, so _blobs needs no check of its own. The offset test stays: _splitID
takes the low 16 bits, so an id in an earlier blob can name an offset past MAX_SIZE and still pack
under the bound. The packed value is also the id of the next free slot, which is exactly what
end() wants, so iteration's bound stops being reconstructed from two fields.

Thanks to @moonchen for finding the rollover window and the rename() race in review.

createSpan and rename removed

It was the only path that could leave a blob partly filled: when a span did not fit it skipped to a
fresh blob, abandoning tail slots that were never handed out but that pack below the bound. With it
gone, blobs fill contiguously and "packs below the bound" means exactly "was handed out", with no
special case. It also has no callers outside the tests.

Two test consumers adapted: a span/rename section became rename-only, and test_RecRegister.cc was
using createSpan(1) as a cheap anonymous registration while hammering lookups, which create()
does as well. The test case covering createSpan's blob boundary goes with it.

rename() goes for a different reason: it mutated a slot's name while name() and
lookup(id, &out_name) read that same std::string without the mutex and hand out string_views
into it. Locking rename() does not close that — the readers are the lock free paths this PR exists
to keep — and giving names immutable storage with its own lifetime rules is a lot of machinery for a
function nothing outside the tests calls. Removing it leaves the useful invariant: a name is written
once, before the store that publishes it, and never changes, so the unlocked readers are correct by
construction and the string_view keys in _lookups are stable for the life of the process.

Both are removals from an installed header, so they belong in the 11.0.0 release notes. Neither has
a caller in tree, and createSpan handed out unnamed slots that only rename() could have named.

_extractType on a negative id

It shifted a signed IdType, so _extractType(NOT_FOUND) sign extended to -4 — a MetricType
outside its enumeration, returned by Metrics::type(). Shifting unsigned is not sufficient on its
own: the sign bit sits above the type field, so NOT_FOUND still yields 4. Masking to the single
bit _makeId writes makes the function total for any input.

Two smaller ones: rename() computed a reference into the name storage before taking the mutex,
which read nothing but made the critical section look wider than it is, and Metrics.h had been
relying on swoc/MemSpan.h for <limits>.

Testing

A new test resolves ids from several threads while another registers metrics across a few blob
boundaries. Under the tsan preset, making either allocation counter non-atomic again reports a data
race there. It does not catch a downgrade of the release/acquire pairs to relaxed — atomics are
race free at any ordering, so TSAN stays quiet and the assertions still hold. The memory orders are
reviewed, not tested, and the test says so.

Two ways that test could pass without testing anything, both found in review and both fixed. Readers
only published their tally on exit and nothing made them run before the writer finished, so on one
CPU every reader could see stop and resolve nothing while resolved > 0 still held; each
resolution is now published as it happens and the writer waits for one before stopping. And an id
that lookup() clamps resolves to the bad_id slot, whose name is not empty, so the name check
could not detect a clamp; it now compares against the name that id must have.

tools/benchmark/benchmark_Metrics.cc is new; nothing in tree measured these paths, which is how a
global mutex on the hottest one went unnoticed. Four cases scaled by thread count, on a 10 core
machine at 20k ops/thread:

threads	increment(ptr)	increment(id)	lookup(id)	lookup(name)
1	0.12 ms	1.81 ms	1.05 ms	2.34 ms
4	0.24 ms	2.20 ms	1.15 ms	37.6 ms
16	4.71 ms	7.04 ms	2.09 ms	80.4 ms
64	19.6 ms	24.0 ms	6.13 ms	317 ms

lookup(name) is a deliberate control: it still takes the mutex, so it must degrade with thread
count. It goes 2.3 ms to 317 ms while lookup(id) goes 1.05 to 6.13 ms, which is the evidence that
the harness loads the machine rather than the lock free numbers being flat for want of load. Above
10 threads the machine is oversubscribed, so treat the shape as meaningful and the magnitudes as
not.

Comparing a build with and without the atomics commit put every case within noise, the only
consistent signal being 4-8% on lookup(id) — two ldaprh rather than two ldrh on ARM64, and
plain loads on x86. Set against what the mutex costs, it is not a trade worth considering.

Those numbers predate the packed position, so the packing was measured separately, as an A/B of
bc333f401e (two atomics, two acquire loads and a null blob test in _is_allocated) against the tip
of this branch. Same binary, same machine, RelWithDebInfo, 20k ops/thread, seven runs per point,
mean +- sd. This is a 16 logical core machine, so these absolute numbers are not comparable with the
table above.

threads	case	two atomics	packed	delta
1	increment(ptr)	33.6 +- 2.1 us	32.7 +- 0.5 us	-2.9%
1	increment(id)	63.0 +- 2.3 us	50.2 +- 3.3 us	-20.2%
1	lookup(id)	47.1 +- 3.7 us	43.3 +- 3.5 us	-8.0%
1	lookup(name)	318.0 +- 18.0 us	305.3 +- 6.8 us	-4.0%
4	increment(ptr)	177.6 +- 9.5 us	175.9 +- 5.3 us	-1.0%
4	increment(id)	176.2 +- 9.0 us	154.4 +- 1.7 us	-12.4%
4	lookup(id)	81.3 +- 18.8 us	74.4 +- 1.7 us	-8.5%
4	lookup(name)	4396.7 +- 96.0 us	4457.0 +- 51.2 us	+1.4%

increment(ptr) and lookup(name) are the controls: neither calls _is_allocated, and both stay
within 4%, which sets the noise floor. The two id paths move well outside it, and by a ratio that
falls out of the code: the benchmark's increment(id) case is valid(id) ? increment(id, 1) : 0,
which enters _is_allocated twice, and its delta is about twice lookup(id)'s. So the saving is
localized to the check, which is what removing one acquire load and one null test should do.

At 16 and 64 threads every case lands within noise, controls included — contention on the metric
atomics dominates, with increment(ptr) going from 33 us to 19 ms. The packing is a win where the
check is measurable and free where it is not.

Provenance

The bounds and memory-order findings came out of a review of this code prompted by a production
perf profile, in which the #13310 locking accounted for roughly half of all CPU in futex
contention. I do not have a public link for that review to cite. The parts of it this PR does not
implement — deleting the sdk_assert from the TSStat* entry points, and an opaque handle API for
plugins — were either out of proportion to the measured benefit or need an upstream decision first.
Removing rename() was on that list too, and review showed it was not optional.

Per-blob published counts were the other candidate for the rollover window, and would additionally
have excluded createSpan's abandoned tail slots. Deleting createSpan achieves that instead, and
leaves the check at one load rather than two dependent ones.

addBlob's bound assert was part of the same review and landed earlier in #13505.

[Copilot]

Pull request overview

This PR hardens ts::Metrics::Storage against races and out-of-bounds id accesses (notably from untrusted plugin-provided TSStat* ids) while keeping the hot id-based read paths lock-free. It also adds targeted concurrency/bounds tests and a new Catch2 micro-benchmark to measure the affected metrics access patterns.

Changes:

• Introduce atomic publication for _cur_blob / _cur_off (release stores) and unify id validation via Storage::_is_allocated() across valid(), lookup(id), name(), and rename().
• Fix _extractType() to be total for all IdType inputs (including negative sentinel values like NOT_FOUND).
• Add a new concurrent-creation safety test and a new benchmark_Metrics executable to measure lookup/increment paths at different thread counts.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tools/benchmark/CMakeLists.txt	Adds the benchmark_Metrics target and links it against ts::tsutil and Catch2.
tools/benchmark/benchmark_Metrics.cc	New Catch2 benchmarking harness for increment(ptr), increment(id), lookup(id), and lookup(name) under configurable thread/op counts.
src/tsutil/unit_tests/test_Metrics.cc	Adds tests for malformed id offsets and concurrent id lookup during metric creation.
src/tsutil/Metrics.cc	Implements lock-free safe id lookup/name resolution via _is_allocated() and publishes allocation progress with atomic release stores.
include/tsutil/Metrics.h	Introduces atomic allocation counters, adds _is_allocated() gate, and fixes _extractType() masking.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

include/tsutil/Metrics.h:393

• Storage::_is_allocated() treats every slot in any blob with blob_ix < cur_blob as allocated (offset < MAX_SIZE), but createSpan() can advance to a new blob when a span does not fit, leaving unused tail slots in the previous blob. Those tail slots were never handed out by create()/createSpan(), yet valid() will accept manufactured ids that point into them (and lookup(id) will then return a non-null metric with an empty name). If ids from TSStat* are considered untrusted, this weakens the “allocated slot” gate for in-range-but-never-issued ids.

      // _cur_blob first: acquiring it also makes visible everything published under it.
      auto const cur_blob = _cur_blob.load(std::memory_order_acquire);
      auto const cur_off  = _cur_off.load(std::memory_order_acquire);

      // A non-null blob past cur_blob is allocated but not yet published, hence <= and < rather
      // than a test for "not the current blob".
      return offset < MAX_SIZE && blob_ix <= cur_blob && _blobs[blob_ix] != nullptr && (blob_ix < cur_blob || offset < cur_off);

src/tsutil/Metrics.cc:264

• Storage::rename() takes a reference to the slot's std::string before acquiring _mutex, but then uses the mutex to protect _lookups updates and the name mutation. This allows rename() callers to race with each other (and with create()/createSpan() writers) on the underlying std::string/_lookups state. Acquiring _mutex before reading/modifying the slot name keeps the rename operation internally consistent and matches how other writers protect _lookups.

  // We can only rename Metrics that are already allocated
  if (!_is_allocated(id)) {
    return false;
  }

  auto [blob_ix, offset]         = _splitID(id);
  Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get();

  std::string    &cur = std::get<0>(std::get<0>(*blob)[offset]);
  std::lock_guard lock(_mutex);

  if (cur.length() > 0) {
    _lookups.erase(cur);
  }
  cur = name;
  _lookups.emplace(cur, id);

  return true;

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

[moonchen]

I left a few inline comments on the blob-rollover state and the concurrency test coverage.

[cmcfarlen]

Both correct, both mine. Fixed in 580b4d1.

<limits> — dropping createSpan removed swoc/MemSpan.h, and that was what supplied <limits> for NOT_FOUND's numeric_limits. Worth being precise about the state: the header still compiles standalone today, through some other transitive path, so nothing is broken right now. That is exactly what makes it worth declaring rather than leaving to luck, since the path that happens to work is not one this header controls. Verified a translation unit including only tsutil/Metrics.h compiles before and after.

Unused cur_off — addBlob() destructured the packed value and used only the blob half. It now shifts out just the index. No warning locally, but there is no reason to bind a name that is never read.

While applying the second one I broke create(), which destructures the same expression and does need both halves; caught by the build, repaired before pushing. Full test_tsutil (15725 assertions) and test_records (294) pass, and the whole tree builds.

[Copilot]

🟡 Changes recommended

Metrics::Storage::rename() still dereferences the stored name string before taking _mutex, which can race with concurrent rename() calls and is easily fixed by locking earlier.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[cmcfarlen]

Moved in f0478d5.

Worth noting the mechanism as described is not quite what was happening. Binding std::string &cur = std::get<0>(std::get<0>(*blob)[offset]) is address computation; it does not access the string's bytes. The first actual read was cur.length(), which was already inside the lock, so there was no unsynchronized access to the string and no data race by the standard's definition.

The change is still worth making. Computing that reference outside the lock made the critical section look wider than it was, and nothing in this function has a reason to sit outside it — the name is the key _lookups is indexed by, so the whole replacement belongs in one place. The allocation check stays outside, since it reads only the published bound and is lock free like its neighbours.

Low risk either way: rename() has no callers outside the tests. LogObject::rename is an unrelated class.

Full test_tsutil and test_records pass; whole tree builds.

[Copilot]

🔵 Needs a closer look

The new concurrent unit test has unbounded spin-waits (hang risk) and one unit test now performs very heavy metric creation that can significantly inflate CI runtime/memory.

Review details

Suppressed comments (4)

Previously missed (3) — in code that hasn't changed since the last review.

src/tsutil/unit_tests/test_Metrics.cc:687

• This wait loop is unbounded; if a regression prevents readers from reaching the steady state (or scheduling is pathological), the unit test can hang indefinitely. Please bound the spin with a maximum iteration count (or a deadline) and fail if it isn’t reached.

This issue also appears on line 694 of the same file.
include/tsutil/Metrics.h:360

• The allocation counter stored in _next_free deliberately omits the type bits, so this function does not actually return “the id it will be given” for gauges. Tightening the comment would avoid confusion for future maintainers about what this value represents.
  src/records/unit_tests/test_RecRegister.cc:109
• This now creates 100,000 distinct metrics (string allocation + hash table insert) just to grow the store, which can significantly increase unit test runtime and memory use. Consider limiting the loop to a few blob boundaries’ worth of registrations (still exercises concurrent growth) to keep the test lightweight.

src/tsutil/unit_tests/test_Metrics.cc:699

• This wait loop is unbounded; if readers never manage to resolve an id (due to a bug or extreme scheduling), the test will hang. Please bound the spin and fail when the bound is exceeded so CI can terminate with a useful error.

  // Readers being in their loops is not enough to guarantee they did any work: on a single CPU the
  // writer can run to completion first, and every reader would then see stop and resolve nothing.
  // Wait for one actual resolution so the check below cannot pass vacuously.
  while (resolved.load(std::memory_order_relaxed) == 0) {
    std::this_thread::yield();
  }

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The changes materially alter lock-free concurrency and memory-ordering behavior in a hot metrics path, which warrants final human review despite only minor review notes.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[moonchen]

Requesting changes for the remaining name-storage race.

[moonchen]

The acquire on _next_free only publishes a slot's initial construction. This mutex protects the writer, but lookup(id, &out_name) and name(id) still read the same std::string without _mutex, and both expose a string_view into it. I reproduced a TSAN race between the assignment below and the read in lookup() at line 136. Please give metric names stable immutable storage with suitable synchronization/lifetime semantics, or remove rename().

[cmcfarlen]

Removed in 2d05898.

You are right that locking rename() cannot fix this. The readers you point at — name() and lookup(id, &out_name) — are unlocked on purpose; they are the paths this PR exists to keep off the mutex. So the choice was immutable name storage with its own lifetime rules, or dropping the function. For something with no caller outside the tests, an atomic name pointer per slot plus never reclaiming replaced strings is a lot of machinery to carry.

Dropping it leaves the invariant those readers actually need, which is worth stating rather than assuming: a slot's name is written once, before the release store that publishes it, and never changes afterwards. So the unlocked reads are correct by construction, and the string_view keys in _lookups stay valid for the life of the process. _lookups.erase disappeared with rename(), so that map only grows now.

That is the second removal from an installed header in this PR, after createSpan, and the two are related: createSpan handed out unnamed slots that only rename() could ever have named. Both are noted for the 11.0.0 release notes, and the description is updated — it had listed removing rename() among the things this PR deliberately did not do, which review has overtaken.

Full test_tsutil and test_records pass and the tree builds.

[Copilot]

🔵 Needs a closer look

It introduces subtle lock-free concurrency and publication-order changes in a hot metrics path where correctness depends on precise atomic ordering and invariants.

Review details

Suppressed comments (1)

include/tsutil/Metrics.h:321

• The comment describing _next_free says it is “packed as _makeId packs one”, but _next_free is stored using _pack() (i.e., without any type bit). This is equivalent to _makeId(..., MetricType::COUNTER) but not to _makeId in general, so the current wording is misleading for future maintainers.

    /* The next free slot, packed as @c _makeId packs one. A single value because a reader that
     * caught a new offset against an old blob index, or the reverse, would reject ids that exist
     * or accept ids that do not. Release stored last, after the blob pointer or the slot's name it
     * publishes. Only ever increases, so an id is allocated exactly when it packs below it.

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[moonchen]

The rename race is resolved: metric names are now write-once before publication, and the unsafe mutation API is gone. Verified the focused Metrics and records tests locally, including the Metrics suite under ThreadSanitizer.

[cmcfarlen]

Cherry-picked to the 10.2.x branch as c2c2f17 for the 10.2.1 release.