Introduce Clang Thread Safety Analysis, and apply it to two subsystems by moonchen · Pull Request #13310 · apache/trafficserver

moonchen · 2026-06-22T21:06:57Z

Introduce Clang Thread Safety Analysis, and apply it to two subsystems

Motivation

A lot of ATS's concurrency contracts live only in comments and DEBUG-only
ink_asserts — "must hold this mutex," "only call on this thread." The compiler
can't see them, so the easy path is the one that silently violates them, and the
result shows up in production as a data race.

Clang's Thread Safety Analysis (-Wthread-safety) lets us put those contracts in
the type system: annotate which mutex guards which data, and the compiler proves
every access holds the lock. The annotations are compile-time only — zero runtime
cost, and nothing on GCC.

To show this pays for itself rather than just adding ceremony, this PR also uses
the new analysis to find and fix an existing data race (see below).

What's in this PR

Annotation infrastructure (tsutil)
- tsutil/ts_thread_safety.h: TS_* macros wrapping the Clang attributes;
  they expand to nothing off Clang.
- Annotated lock types for the analysis to track: ts::mutex with
  ts::lock_guard (the annotated counterparts to std::mutex /
  std::lock_guard), and ts::shared_mutex with ts::write_guard /
  ts::read_guard.
- A unit test that is itself compiled with the analysis as a worked example.
- A CMake lane: -Wthread-safety is on by default for Clang as a warning.
Annotate SSLOriginSessionCache — its session map and queue are reached
from every thread; they're now marked guarded by the cache mutex, so the
locking discipline is compiler-enforced instead of conventional. The
hand-rolled lock witness on remove_oldest_session (a std::unique_lock
parameter checked with owns_lock()) becomes a compile-time TS_REQUIRES.
Fix an unsynchronized read race in Metrics::Storage — the analysis
caught it (details below).
Gate it in CI — the ci/branch presets set
THREAD_SAFETY_ANALYSIS_AS_ERROR=ON so findings are errors on the Clang
lanes and gate merges, while local and dev builds keep them warnings.

The bug the analysis caught

Metrics::Storage::create(), createSpan(), and current() access
_cur_blob / _cur_off / _blobs under _mutex, but valid(),
lookup(IdType), and name() read the same fields with no lock held
(rename() likewise read them before taking the lock). A single Storage is
shared by all threads; metrics registered at runtime — a plugin TSStatCreate,
or a config reload — advance those fields while live traffic reads them through
the plugin stat APIs. That's a data race / UB.

The fix marks the fields guarded by the mutex and takes it on every access, so
the analysis enforces the locking from here on. The reads that were missing a
lock take it exclusively, matching the existing locked paths; Storage keeps its
plain mutex (now ts::mutex), so its runtime locking is otherwise unchanged.

Why ATS-owned guards instead of `std::lock_guard`/`std::unique_lock`

Clang's analysis can only track lock state through types that carry its
annotations, and the standard RAII lock wrappers don't fit: std::unique_lock,
std::shared_lock, and std::scoped_lock are intentionally flexible — deferred
locking, move, release(), adopt — dynamic state the analysis can't follow, and
ATS doesn't need that flexibility (the common case is a plain scoped lock).

So ATS provides its own simple, scoped, non-movable guards — ts::lock_guard
over ts::mutex, and ts::write_guard / ts::read_guard over
ts::shared_mutex — which are exactly the shape the analysis can verify. Reach
for std::unique_lock/shared_lock where the dynamic features are genuinely
needed, accepting that those sites sit outside the analysis.

Developer impact

Default Clang builds: warnings only. -Wno-error=thread-safety keeps it a
warning even in -Werror builds, so an in-progress annotation never blocks a
local build.
CI (THREAD_SAFETY_ANALYSIS_AS_ERROR=ON, set in the presets): findings
are errors on the Clang lanes and gate the merge, which keeps master clean —
so the default warnings only ever flag a developer's own in-progress change.
GCC: unaffected. The flag is Clang-only on purpose (GCC doesn't know
-Wthread-safety and would itself error if passed it), and the macros compile
to nothing.
Going forward: prefer the annotated ts:: guards for scoped locking and
mark the data they protect; reach for std:: lock guards only where the
dynamic features are actually needed.

Testing

test_tsutil includes test_thread_safety.cc, compiled with
-Werror=thread-safety under Clang as a live example.
The annotated translation units build clean under clang -Wthread-safety and
under GCC; removing a lock from an annotated reader produces the expected
diagnostic.

Scope

This is a deliberately small seed: the infrastructure plus two annotated
subsystems, one of which surfaced a real fix. It is meant to establish the
pattern and the CI lane; further annotation (e.g. NetHandler/cache/HostDB state,
and enforcing "no blocking lock on ET_NET" via a thread-role capability) can
follow incrementally as each subsystem is brought to zero warnings.

Copilot

Pull request overview

This PR introduces Clang Thread Safety Analysis support to ATS via a small annotation/locking infrastructure in tsutil, then applies it to SSLOriginSessionCache and Metrics::Storage to make their locking contracts compiler-checkable (and fixes a real metrics read race in the process).

Changes:

Add TS_* thread-safety annotation macros plus annotated ts::shared_mutex and scoped reader/writer guards.
Annotate and refactor locking in SSLOriginSessionCache and Metrics::Storage to enforce guarded access under the compiler.
Enable -Wthread-safety for Clang builds (warnings by default; CI presets promote to errors) and add a compile-time unit-test TU to validate the annotation chain.

Reviewed changes

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

Show a summary per file

File	Description
src/tsutil/unit_tests/test_thread_safety.cc	New compile-time + runtime example exercising the annotated mutex/guards.
src/tsutil/Metrics.cc	Switch metrics storage locking to scoped reader/writer guards to fix unsynchronized reads.
src/tsutil/CMakeLists.txt	Add thread-safety unit test and per-source compile flags for Clang analysis.
src/iocore/net/SSLSessionCache.h	Add thread-safety annotations (guarded members + requires) to session cache.
src/iocore/net/SSLSessionCache.cc	Replace std lock wrappers with annotated `ts::scoped_*_lock` and remove runtime lock witness.
include/tsutil/TsSharedMutex.h	Mark `ts::shared_mutex` as a capability and add annotated scoped guards.
include/tsutil/ts_thread_safety.h	New header defining ATS-wide `TS_*` wrappers for Clang thread-safety attributes.
include/tsutil/Metrics.h	Make `Metrics::Storage` fields guarded-by a shared mutex and update locking.
CMakePresets.json	Enable analysis and promote to errors for CI/branch presets.
CMakeLists.txt	Add build options and wiring for Clang `-Wthread-safety` / error gating.

Copilot

Pull request overview

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

Copilot

Pull request overview

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

Add TS_* annotation macros (tsutil/ts_thread_safety.h) wrapping Clang's -Wthread-safety attributes, so a lock's contract -- which mutex guards which data, which lock a function requires its caller to hold -- can be expressed in the type system and proved at build time. They expand to nothing off Clang: no runtime cost, and a no-op for GCC. Add annotated lock types for the analysis to track: ts::mutex with ts::scoped_lock, and ts::shared_mutex (already ATS's own rwlock) marked as a capability with ts::scoped_writer_lock / ts::scoped_reader_lock. Annotated code takes its locks through these because the std:: RAII wrappers are too flexible -- deferred locking, move, adopt/release -- for the analysis to track, and ATS does not need that flexibility. Install the new headers with the rest of tsutil, and add a unit test that is itself compiled with the analysis enabled as a worked example.

Add the ENABLE_THREAD_SAFETY_ANALYSIS option (Clang-only, on by default as a warning) and THREAD_SAFETY_ANALYSIS_AS_ERROR, which the CI and branch presets enable so violations are errors that gate merges while local and dev builds stay warnings. GCC is unaffected -- the flag is Clang-only and GCC would error if passed it. Skip FreeBSD: its libc annotates the pthread primitives themselves, so -Wthread-safety there flags ATS's existing hand-rolled mutex wrappers (tscore/ink_mutex.h and others) tree-wide, not just newly annotated code. Bringing FreeBSD into the gate needs those legacy wrappers made analysis-clean first.

valid(), lookup(IdType) and name() read _cur_blob/_cur_off/_blobs with no lock held, while create()/createSpan()/current() access the same fields under the mutex (rename() likewise read them before locking). A single Storage is shared by all threads, so a metric registered at runtime -- a plugin TSStatCreate or a config reload -- advances those fields while live traffic reads them: a data race. Take the mutex on every access and mark the fields guarded by it, so the analysis enforces the locking from here on. The reads that were missing a lock take it exclusively, matching the existing locked paths; Storage keeps its plain mutex, now the annotated ts::mutex with ts::scoped_lock.

The origin session map and queue are reachable from every thread; mark them guarded by the cache mutex so the compiler enforces the locking that was previously only convention. Replace the hand-rolled lock witness on remove_oldest_session (a std::unique_lock parameter checked with owns_lock()) with a compile-time TS_REQUIRES precondition.

Rename the annotated scoped guards to ts::lock_guard, ts::write_guard, and ts::read_guard (formerly ts::scoped_lock, ts::scoped_writer_lock, and ts::scoped_reader_lock). The shorter names describe what these guards are -- rigid acquire-in-constructor / release-in-destructor RAII, closest to std::lock_guard -- and avoid implying the flexibility of std::unique_lock / std::shared_lock, which they deliberately do not provide. Pure rename; no change in behavior.

Copilot

Pull request overview

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

masaori335

Looks good. Thank you!

moonchen · 2026-06-24T02:19:46Z

[approve ci autest]

cmcfarlen · 2026-06-26T00:19:52Z

Cherry-picked to 10.2.x

#13310) Add TS_* annotation macros (tsutil/ts_thread_safety.h) wrapping Clang's -Wthread-safety attributes, so a lock's contract -- which mutex guards which data, which lock a function requires its caller to hold -- can be expressed in the type system and proved at build time. They expand to nothing off Clang: no runtime cost, and a no-op for GCC. Add annotated lock types for the analysis to track: ts::mutex with ts::lock_guard, and ts::shared_mutex (already ATS's own rwlock) marked as a capability with ts::write_guard / ts::read_guard. Annotated code takes its locks through these because the std:: RAII wrappers are too flexible -- deferred locking, move, adopt/release -- for the analysis to track, and ATS does not need that flexibility. The guard names mirror std::lock_guard's rigid acquire-in-constructor / release-in-destructor RAII rather than implying std::unique_lock's flexibility. Add the ENABLE_THREAD_SAFETY_ANALYSIS option (Clang-only, on by default as a warning) and THREAD_SAFETY_ANALYSIS_AS_ERROR, which the CI and branch presets enable so violations are errors that gate merges while local and dev builds stay warnings. Install the new headers with the rest of tsutil, and add a unit test compiled with the analysis enabled as a worked example. Skip FreeBSD: its libc annotates the pthread primitives themselves, so -Wthread-safety there flags ATS's existing hand-rolled mutex wrappers (tscore/ink_mutex.h and others) tree-wide; bringing FreeBSD into the gate needs those legacy wrappers made analysis-clean first. Apply the analysis to two subsystems: Metrics::Storage: valid(), lookup(IdType) and name() read _cur_blob/_cur_off/_blobs with no lock held, while create()/createSpan()/ current() access the same fields under the mutex (rename() likewise read them before locking). A single Storage is shared by all threads, so a metric registered at runtime -- a plugin TSStatCreate or a config reload -- advances those fields while live traffic reads them: a data race. Take the mutex on every access and mark the fields guarded by it; the reads that were missing a lock take it exclusively, matching the existing locked paths. SSLOriginSessionCache: the origin session map and queue are reachable from every thread; mark them guarded by the cache mutex so the compiler enforces the locking that was previously only convention. Replace the hand-rolled lock witness on remove_oldest_session (a std::unique_lock parameter checked with owns_lock()) with a compile-time TS_REQUIRES precondition. (cherry picked from commit 2b6dce0)

apache#13310) Add TS_* annotation macros (tsutil/ts_thread_safety.h) wrapping Clang's -Wthread-safety attributes, so a lock's contract -- which mutex guards which data, which lock a function requires its caller to hold -- can be expressed in the type system and proved at build time. They expand to nothing off Clang: no runtime cost, and a no-op for GCC. Add annotated lock types for the analysis to track: ts::mutex with ts::lock_guard, and ts::shared_mutex (already ATS's own rwlock) marked as a capability with ts::write_guard / ts::read_guard. Annotated code takes its locks through these because the std:: RAII wrappers are too flexible -- deferred locking, move, adopt/release -- for the analysis to track, and ATS does not need that flexibility. The guard names mirror std::lock_guard's rigid acquire-in-constructor / release-in-destructor RAII rather than implying std::unique_lock's flexibility. Add the ENABLE_THREAD_SAFETY_ANALYSIS option (Clang-only, on by default as a warning) and THREAD_SAFETY_ANALYSIS_AS_ERROR, which the CI and branch presets enable so violations are errors that gate merges while local and dev builds stay warnings. Install the new headers with the rest of tsutil, and add a unit test compiled with the analysis enabled as a worked example. Skip FreeBSD: its libc annotates the pthread primitives themselves, so -Wthread-safety there flags ATS's existing hand-rolled mutex wrappers (tscore/ink_mutex.h and others) tree-wide; bringing FreeBSD into the gate needs those legacy wrappers made analysis-clean first. Apply the analysis to two subsystems: Metrics::Storage: valid(), lookup(IdType) and name() read _cur_blob/_cur_off/_blobs with no lock held, while create()/createSpan()/ current() access the same fields under the mutex (rename() likewise read them before locking). A single Storage is shared by all threads, so a metric registered at runtime -- a plugin TSStatCreate or a config reload -- advances those fields while live traffic reads them: a data race. Take the mutex on every access and mark the fields guarded by it; the reads that were missing a lock take it exclusively, matching the existing locked paths. SSLOriginSessionCache: the origin session map and queue are reachable from every thread; mark them guarded by the cache mutex so the compiler enforces the locking that was previously only convention. Replace the hand-rolled lock witness on remove_oldest_session (a std::unique_lock parameter checked with owns_lock()) with a compile-time TS_REQUIRES precondition.

…vert (#13583) * Metrics: one gate for id validation, and fix the off-by-one valid(), lookup(IdType), name() and rename() each carried their own copy of the same range test, and the copies had drifted. valid() rejected an offset past MAX_SIZE; the other three did not. Since _splitID passes the low 16 bits of an id through unmasked and the offset check only applied when the id named the current blob, 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(), which rejects a negative id, an offset no _makeId could have produced, an unallocated blob, and a slot at or past the allocation point. That last comparison 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. * Metrics: publish the allocation point with release/acquire The lock removal in #13567 left the reader path reading _cur_blob, _cur_off and _blobs while a concurrent create() advances them, which is the data race #13310 took the mutex to close. Close it without the mutex instead. Making each counter atomic does not make the pair update atomically, and it does not need to. _cur_blob and _cur_off are publication points: each is written last, with a release store, after whatever it makes visible -- the blob pointer and the reset offset for _cur_blob, the slot's name for _cur_off. A reader acquires _cur_blob first, so observing a value for it also observes everything addBlob() wrote before releasing it. 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, so neither a packed word nor per-blob counters are needed. _blobs stays non-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, so there is no race to close. Writers all hold the mutex and load relaxed. 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. Verified with a TSAN harness running eight readers validating and resolving ids across the whole space while a writer creates 2600 metrics across several blob boundaries: three reported races before this change, none after. * Metrics: cover concurrent id lookup, and make _extractType total Add a test that resolves ids from several threads while another registers metrics across a few blob boundaries. Nothing single threaded exercises the publication order the previous commit relies on; under the tsan preset, making either allocation counter non-atomic again reports a data race here. The test cannot catch a downgrade of the release/acquire pairs to relaxed -- atomics are race free at any ordering -- and says so, so the memory orders are not mistaken for tested. _extractType 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 enough on its own: the sign bit sits above the type field, so NOT_FOUND still yields 4. Mask to the single bit _makeId writes, which makes the function total for any input. * Add a ts::Metrics micro benchmark Nothing in tree measured the metric read paths, which is why a global mutex on the hottest one went unnoticed until it showed up in a production profile. Four cases, scaled by thread count: increment(id) what TSStatIntIncrement does, the path that regressed increment(ptr) what core and cripts do, the floor lookup(id) the lock free id resolution alone lookup(name) the same resolution through the mutex guarded name map lookup(name) is deliberately included as a positive control. It still takes the lock, so it must degrade with thread count; if it ever stops doing so, the harness is not loading the machine and the other three numbers mean nothing. Built only with ENABLE_BENCHMARKS, as with the rest of tools/benchmark. * Metrics: trim comments to the invariants State what holds rather than how it came to hold. Drops the explanations of which write order a comparison compensates for, what a reader would have seen otherwise, and what each benchmark case is meant to prove. Also shortens the createSpan boundary test's preamble, which describes the bug it covers at more length than the assertion needs. * Metrics: probe real ids in the concurrent lookup test The reader swept ids as consecutive integers, but an id packs the blob index above the offset, so 0..N only ever named blob 0 and everything from MAX_SIZE up decoded to an offset that validation rejects. Earlier cases in this file leave blob 0 full, so the reader was walking settled slots while the writer worked in a blob it never named. Take ids from what the writer has registered instead, and assert the ids span more than one blob so a future change cannot quietly confine the sweep again. Also assert the readers resolved something, since every id being skipped would otherwise pass. * Metrics: make _is_allocated private, tidy createSpan's index handling _is_allocated is only called by Storage's own accessors, so it does not belong in the public section; valid() remains the public gate. createSpan loaded _cur_off twice and _cur_blob once for its two guards, then re-read both unconditionally in case addBlob() had moved them. Load the pair once and refresh it only in the branch that grows a blob, which drops two atomic loads from the common path. Re-reading rather than adjusting the locals by hand keeps the caller from restating what addBlob() sets. * Publish the next free slot as one packed value _cur_blob and _cur_off were two atomics, and readers need the pair to be coherent. addBlob() reset the offset then bumped the blob, so a reader between the two saw the old blob with a zero offset and rejected every id in the just-completed blob. With ENABLE_FAST_SDK=OFF that reaches _TSReleaseAssert through TSStatInt*, so it aborts rather than losing a count. Reversing the stores only trades it for accepting ids in a blob nothing has been written to; two atomics have no coherent pair either way. One atomic holding the blob index above the offset, packed as an id is packed, fixes it: crossing a blob is a single release store. The value only ever increases, so an id is allocated exactly when it packs below the bound, which reduces the gate on every id based accessor to one acquire load and one compare. Acquiring the bound also acquires the blob install, so the null blob check goes away. It is also the id of the next free slot, which is what iteration wants for its end bound. Drop createSpan with it. It has no callers outside the tests, and it was the only path that could leave a blob partly filled -- it skipped to a fresh blob when a span did not fit, abandoning tail slots that were never handed out and that the packed bound would count as allocated. Without it, blobs fill contiguously and "packs below the bound" means exactly "was handed out". * Make the concurrent lookup test check what it claims Two ways it could pass without testing anything. 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; it now publishes each resolution as it happens and the writer waits for one before stopping. And an id that lookup() clamps resolves to the reserved 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. * Include <limits> and stop binding an unused offset Dropping createSpan took swoc/MemSpan.h with it, and that was what supplied <limits> for NOT_FOUND's numeric_limits. The header still compiles, through some other transitive path, which is exactly what makes it worth declaring. addBlob() destructured the packed value but only ever used the blob half. * Take the lock before touching a slot's name in rename() The name is the key _lookups is indexed by, so replacing it belongs entirely inside the lock. Nothing read the string outside it before -- binding a reference to it does not touch its bytes -- but computing that reference outside the lock made the boundary look wider than it is, and there is no reason for anything here to sit outside. * Metrics: trim comments, and drop ones about a check that is gone Two comments in the malformed-offset test explained that the null blob check, not the offset test, would reject those ids with only one blob allocated. The packed bound removed that check, so the reasoning no longer applied. * Remove rename() It mutated a slot's name while name() and lookup(id, &out_name) read that same std::string without the mutex and hand out views into it, which moonchen reproduced as a TSAN race. Locking rename() does not fix it; the readers are the lock free paths this PR exists to keep. Giving names immutable storage with its own lifetime rules would, but nothing outside the tests calls rename(). Without it a name is written once before the store that publishes it and never changes, so those readers are correct by construction.

…vert (apache#13583) * Metrics: one gate for id validation, and fix the off-by-one valid(), lookup(IdType), name() and rename() each carried their own copy of the same range test, and the copies had drifted. valid() rejected an offset past MAX_SIZE; the other three did not. Since _splitID passes the low 16 bits of an id through unmasked and the offset check only applied when the id named the current blob, 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(), which rejects a negative id, an offset no _makeId could have produced, an unallocated blob, and a slot at or past the allocation point. That last comparison 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. * Metrics: publish the allocation point with release/acquire The lock removal in apache#13567 left the reader path reading _cur_blob, _cur_off and _blobs while a concurrent create() advances them, which is the data race apache#13310 took the mutex to close. Close it without the mutex instead. Making each counter atomic does not make the pair update atomically, and it does not need to. _cur_blob and _cur_off are publication points: each is written last, with a release store, after whatever it makes visible -- the blob pointer and the reset offset for _cur_blob, the slot's name for _cur_off. A reader acquires _cur_blob first, so observing a value for it also observes everything addBlob() wrote before releasing it. 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, so neither a packed word nor per-blob counters are needed. _blobs stays non-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, so there is no race to close. Writers all hold the mutex and load relaxed. 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. Verified with a TSAN harness running eight readers validating and resolving ids across the whole space while a writer creates 2600 metrics across several blob boundaries: three reported races before this change, none after. * Metrics: cover concurrent id lookup, and make _extractType total Add a test that resolves ids from several threads while another registers metrics across a few blob boundaries. Nothing single threaded exercises the publication order the previous commit relies on; under the tsan preset, making either allocation counter non-atomic again reports a data race here. The test cannot catch a downgrade of the release/acquire pairs to relaxed -- atomics are race free at any ordering -- and says so, so the memory orders are not mistaken for tested. _extractType 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 enough on its own: the sign bit sits above the type field, so NOT_FOUND still yields 4. Mask to the single bit _makeId writes, which makes the function total for any input. * Add a ts::Metrics micro benchmark Nothing in tree measured the metric read paths, which is why a global mutex on the hottest one went unnoticed until it showed up in a production profile. Four cases, scaled by thread count: increment(id) what TSStatIntIncrement does, the path that regressed increment(ptr) what core and cripts do, the floor lookup(id) the lock free id resolution alone lookup(name) the same resolution through the mutex guarded name map lookup(name) is deliberately included as a positive control. It still takes the lock, so it must degrade with thread count; if it ever stops doing so, the harness is not loading the machine and the other three numbers mean nothing. Built only with ENABLE_BENCHMARKS, as with the rest of tools/benchmark. * Metrics: trim comments to the invariants State what holds rather than how it came to hold. Drops the explanations of which write order a comparison compensates for, what a reader would have seen otherwise, and what each benchmark case is meant to prove. Also shortens the createSpan boundary test's preamble, which describes the bug it covers at more length than the assertion needs. * Metrics: probe real ids in the concurrent lookup test The reader swept ids as consecutive integers, but an id packs the blob index above the offset, so 0..N only ever named blob 0 and everything from MAX_SIZE up decoded to an offset that validation rejects. Earlier cases in this file leave blob 0 full, so the reader was walking settled slots while the writer worked in a blob it never named. Take ids from what the writer has registered instead, and assert the ids span more than one blob so a future change cannot quietly confine the sweep again. Also assert the readers resolved something, since every id being skipped would otherwise pass. * Metrics: make _is_allocated private, tidy createSpan's index handling _is_allocated is only called by Storage's own accessors, so it does not belong in the public section; valid() remains the public gate. createSpan loaded _cur_off twice and _cur_blob once for its two guards, then re-read both unconditionally in case addBlob() had moved them. Load the pair once and refresh it only in the branch that grows a blob, which drops two atomic loads from the common path. Re-reading rather than adjusting the locals by hand keeps the caller from restating what addBlob() sets. * Publish the next free slot as one packed value _cur_blob and _cur_off were two atomics, and readers need the pair to be coherent. addBlob() reset the offset then bumped the blob, so a reader between the two saw the old blob with a zero offset and rejected every id in the just-completed blob. With ENABLE_FAST_SDK=OFF that reaches _TSReleaseAssert through TSStatInt*, so it aborts rather than losing a count. Reversing the stores only trades it for accepting ids in a blob nothing has been written to; two atomics have no coherent pair either way. One atomic holding the blob index above the offset, packed as an id is packed, fixes it: crossing a blob is a single release store. The value only ever increases, so an id is allocated exactly when it packs below the bound, which reduces the gate on every id based accessor to one acquire load and one compare. Acquiring the bound also acquires the blob install, so the null blob check goes away. It is also the id of the next free slot, which is what iteration wants for its end bound. Drop createSpan with it. It has no callers outside the tests, and it was the only path that could leave a blob partly filled -- it skipped to a fresh blob when a span did not fit, abandoning tail slots that were never handed out and that the packed bound would count as allocated. Without it, blobs fill contiguously and "packs below the bound" means exactly "was handed out". * Make the concurrent lookup test check what it claims Two ways it could pass without testing anything. 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; it now publishes each resolution as it happens and the writer waits for one before stopping. And an id that lookup() clamps resolves to the reserved 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. * Include <limits> and stop binding an unused offset Dropping createSpan took swoc/MemSpan.h with it, and that was what supplied <limits> for NOT_FOUND's numeric_limits. The header still compiles, through some other transitive path, which is exactly what makes it worth declaring. addBlob() destructured the packed value but only ever used the blob half. * Take the lock before touching a slot's name in rename() The name is the key _lookups is indexed by, so replacing it belongs entirely inside the lock. Nothing read the string outside it before -- binding a reference to it does not touch its bytes -- but computing that reference outside the lock made the boundary look wider than it is, and there is no reason for anything here to sit outside. * Metrics: trim comments, and drop ones about a check that is gone Two comments in the malformed-offset test explained that the null blob check, not the offset test, would reject those ids with only one blob allocated. The packed bound removed that check, so the reasoning no longer applied. * Remove rename() It mutated a slot's name while name() and lookup(id, &out_name) read that same std::string without the mutex and hand out views into it, which moonchen reproduced as a TSAN race. Locking rename() does not fix it; the readers are the lock free paths this PR exists to keep. Giving names immutable storage with its own lifetime rules would, but nothing outside the tests calls rename(). Without it a name is written once before the store that publishes it and never changes, so those readers are correct by construction. (cherry picked from commit c7af2e3)

Copilot AI review requested due to automatic review settings June 22, 2026 21:06

Copilot started reviewing on behalf of moonchen June 22, 2026 21:07 View session

moonchen self-assigned this Jun 22, 2026

moonchen added Tests Threads labels Jun 22, 2026

moonchen added this to ATS v10.2.x Jun 22, 2026

moonchen added this to the 11.0.0 milestone Jun 22, 2026

Copilot AI reviewed Jun 22, 2026

View reviewed changes

Comment thread src/tsutil/CMakeLists.txt Outdated

Comment thread include/tsutil/TsSharedMutex.h

Copilot AI review requested due to automatic review settings June 22, 2026 21:59

Copilot started reviewing on behalf of moonchen June 22, 2026 21:59 View session

Copilot AI reviewed Jun 22, 2026

View reviewed changes

Comment thread src/tsutil/CMakeLists.txt Outdated

bryancall requested a review from masaori335 June 22, 2026 22:16

ezelkow1 requested a review from Copilot June 22, 2026 22:22

Copilot started reviewing on behalf of ezelkow1 June 22, 2026 22:22 View session

This comment was marked as off-topic.

Sign in to view

masaori335 reviewed Jun 22, 2026

View reviewed changes

Comment thread include/tsutil/Metrics.h Outdated

Copilot AI review requested due to automatic review settings June 23, 2026 03:02

Copilot started reviewing on behalf of moonchen June 23, 2026 03:02 View session

Copilot AI reviewed Jun 23, 2026

View reviewed changes

moonchen added 4 commits June 22, 2026 22:24

moonchen force-pushed the thread-safety-analysis branch from e6b242a to e520e0b Compare June 23, 2026 03:24

masaori335 reviewed Jun 23, 2026

View reviewed changes

Comment thread include/tsutil/TsSharedMutex.h

Copilot AI review requested due to automatic review settings June 23, 2026 23:09

Copilot started reviewing on behalf of moonchen June 23, 2026 23:09 View session

Copilot AI reviewed Jun 23, 2026

View reviewed changes

Comment thread include/tsutil/Metrics.h

masaori335 approved these changes Jun 23, 2026

View reviewed changes

moonchen merged commit 2b6dce0 into apache:master Jun 24, 2026
15 checks passed

github-project-automation Bot moved this to For v10.2.0 in ATS v10.2.x Jun 24, 2026

cmcfarlen moved this from For v10.2.0 to Picked v10.2.0 in ATS v10.2.x Jun 26, 2026

cmcfarlen modified the milestones: 11.0.0, 10.2.0 Jun 26, 2026

cmcfarlen mentioned this pull request Aug 21, 2026

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

Merged

Navigation Menu

Uh oh!

Introduce Clang Thread Safety Analysis, and apply it to two subsystems - #13310

Introduce Clang Thread Safety Analysis, and apply it to two subsystems#13310
moonchen merged 5 commits into
apache:masterfrom
moonchen:thread-safety-analysis

moonchen commented Jun 22, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

This comment was marked as off-topic.

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

masaori335 left a comment

Uh oh!

moonchen commented Jun 24, 2026

Uh oh!

Uh oh!

cmcfarlen commented Jun 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

moonchen commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!