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 _cur_off is the next free slot, and the old <= / > tests accepted
it. 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.

Publication, without a lock

_cur_blob and _cur_off become atomics, but the point is not that each is atomic — that alone
would not make the pair update atomically. They are publication points: each is release stored last,
after whatever it makes visible (the blob pointer and the reset offset for _cur_blob, the slot's
name for _cur_off), and a reader acquire loads _cur_blob first. Observing a value for it also
observes everything written before it was released, so the torn pair a reader could otherwise see —
a new blob index with the previous blob's stale offset — is unreachable rather than merely unlikely.
Neither a packed word nor per-blob counters are needed.

addBlob() also writes the blob pointer and resets _cur_off before advancing _cur_blob, so
_cur_blob alone publishes the blob. The blob comparison in _is_allocated is <= / < rather
than a test for "not the current blob" to match, since a blob past _cur_blob can be allocated but
unpublished. _blobs needs no atomic: it is only read at an index no greater than _cur_blob, and
that write is sequenced before the release store the reader acquired.

What remains is that a reader can observe an older _cur_blob with an already reset _cur_off and
reject an id naming the previous blob, which drops an increment rather than misattributing one.

_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.

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.

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.

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, an opaque handle API for
plugins, restructuring Storage around per-blob published counts, and removing rename() — were
either out of proportion to the measured benefit or need an upstream decision first.

createSpan's blob boundary and addBlob's bound assert were 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.