Allow a metric to be unlisted

[cmcfarlen]

Problem

ts::Metrics::Storage has no removal path. create() allocates a slot and a name and nothing ever undoes either, so a metric name lives for the life of the process.

Any code that decides whether to publish a name based on a runtime changeable input therefore makes a permanent commitment the first time it publishes. The decision is latched at first creation and can never be revisited.

The case that surfaced this is the per upstream server connection metrics from #13506. proxy.config.http.per_server.connection.metric_aggregate is RECU_DYNAMIC and overridable, and at value 2 the per group <fqdn>.<ip>:<port> metrics are supposed to stay hidden while only the per hostname aggregates are published. On a box that ran for a while at 0 before being switched to 2, both shapes are present in traffic_ctl metric match per_server, and no reload can remove the first set. The config change took effect correctly for everything created after it; the names created before it simply cannot be withdrawn.

What this does

Lets a metric be taken out of the store's listing.

auto &m = ts::Metrics::instance();

m.unlist(id);                       // by id
m.unlist("proxy.process.example");  // or by name
m.relist(id);                       // put it back

if (m.listed(id)) { ... }

An unlisted metric:

• is skipped by enumeration, so it disappears from traffic_ctl metric match, the JSONRPC record lookup and stats_over_http with no change in any of those consumers;
• still resolves by exact name through lookup(), so RecLookupRecord, LogAccess field resolution and TSStatFindName keep working;
• keeps its atomic, which may still be read and written, so a Derived aggregate sourcing from it is unaffected;
• is relisted by create() on the same name, returning the same id with its accumulated value intact.

An unlisted phone number is the analogy: not in the directory, but it still rings if you know it. This is a publication policy, not a lifetime — any IdType or AtomicType * a caller already holds stays valid across an unlist and relist.

This PR adds the mechanism only. Nothing in the tree calls it, so every existing metric enumerates exactly as before. The ConnectionTracker fix is a follow up.

Why enumeration had to be refactored

Unlisting means enumeration skips slots. Once it skips, a position in the store is no longer a meaningful place in the sequence — and a public iterator is nothing but a position a caller can hold onto.

Earlier revisions of this PR kept the iterator and tried to make positions safe under skipping. That does not work, and review found two ways it fails, both reproduced by @bneradt:

• find() checked listed(id) and then the positional constructor skipped forward from it. An unlist between the two makes the returned iterator dereference to the next listed metric rather than the one named.
• A saved subrange endpoint that is later unlisted is stepped over: the walk visits metrics past the intended end, then exhausts and spins. It never compares equal to the saved endpoint, ++ cannot make progress, and it dereferences one past the end of the allocated slots.

The two fixes pulled against each other. Copilot flagged that the positional constructor not skipping let a caller name an unlisted slot, which enumeration must never visit; adding the skip to satisfy that is exactly what created the find() defect above. Skipping enumeration and positional bounds are in tension and you cannot have both.

So enumeration is now the whole store or nothing:

ts::Metrics::instance().for_each([](std::string_view name, ts::Metrics::MetricType type, int64_t value) {
  // ...
});

With no cursor to hand out, there is nothing to outlive a walk, nothing to name a slot the walk would skip, and no invalidation rule to define or enforce. Metrics::iterator, begin(), end(), find(), the snapshot bound, the shared bound equality rule and the id type bit normalization all go away with the defects they carried. lookup() is the way to reach a single metric by name.

Nothing wanted the removed surface. There are three enumeration sites in the tree, all in RecCore.cc, and all are full passes that filter per element. find() had no callers at all outside its own tests — not in src, include, plugins, example or tools.

Net effect of the refactor is 170 insertions against 344 deletions.

On the naming

This started out called tombstone, which was wrong twice over. A tombstone elsewhere is a record that something was deleted, and this codebase already uses it that way — CacheShm tombstones a slot to mark it dead and reusable. Nothing is deleted here.

hide / publish would read best in isolation but both words are already load bearing for a different mechanism in this same class: the two separate stores, hidden_instance() and createHiddenPtr() versus the published store. An unlisted metric in the published store would have been "published but not published".

listed collides with neither, and says the useful part out loud.

Implementation notes

Storage. A parallel FlagStorage array in the blob, rather than a member of NameAndId: an std::atomic member would make that tuple neither copyable nor movable, and the slot is written with a tuple assignment. Blobs are already built with make_unique, which value initializes, so flags start zero with no change to addBlob(). Reads are lock free at relaxed ordering, matching the rest of the class. Cost is 1 KiB per 1024 slots against a blob that is already about 48 KiB. The single UNLISTED bit is set and cleared with fetch_or / fetch_and rather than a whole word store, so a flag added later is not clobbered.

The walk. Storage::for_each reads next_free_id() once. That load is the sequence: acquiring it acquires every slot below it, which is what lets the walk read names and values without the mutex — a slot's name is written before the release store that publishes it and never changes afterwards. It then scans blob by blob up to that bound, skips any slot with UNLISTED set, and takes each metric's type from the slot's own stored id rather than from its position, so a gauge is always reported as a gauge. A metric created while a walk runs is simply below no bound it agreed to visit.

Metrics::BAD_ID_NAME names the reserved slot 0, which both the Storage constructor and RecCore now use. The hidden store enumeration used to skip that slot positionally, with begin(); ++it;; it now skips by name, which is what it was actually expressing.

Id validation. Storage::_is_allocated() gates both entry points. It is deliberately stricter than the existing valid(): the offset is the low 16 bits of an id and so can name a slot past MAX_SIZE in a blob that is full, and the next free slot is not allocated yet. That second case is not theoretical — a test that marked the free slot caused an unrelated metric created later in the same run to come out invisible, because create() only clears the flag when it finds the name already present, not when it allocates a fresh slot.

A static_assert now ties MAX_BLOBS and MAX_SIZE to the widths of the blob and offset fields in an id. That relationship is what keeps every _blobs[] subscript in this class in range without an explicit check, and nothing previously enforced it.

API change

Metrics::iterator, begin(), end() and find() are removed. Metrics.h is in TSUTIL_PUBLIC_HEADERS and is installed, so this is a source level break for anything enumerating a store directly or constructing an iterator.

Nothing in tree does either, and find() had no callers even in tests before this PR added some. Worth noting find() predated this PR and was safe there — the positional constructor did not skip, so equality was a plain position compare and no bound was involved. Unlisting is what made it unsound, which is the argument for removing it here rather than in a separate change.

Worth a line in the 11.0.0 release notes alongside the createSpan and rename removals from #13583.

Tests

test_Metrics.cc, in TEST_CASE("Metrics unlisting"): skipped by enumeration; relisted by create() with its value intact; unlist and relist by name; still resolvable by name and id while unlisted; for_each skipping an unlisted first slot; an unlisted run at the end of the store; the reported type matching the type each metric was created with; three rejected id shapes (an unallocated blob, an offset past MAX_SIZE, and the next free slot); and independence between the published and hidden stores.

TEST_CASE("Metrics") covers for_each itself: the reserved slot first, creation order, and the count moving by one when a metric is created.

test_RecHiddenMetricLookup.cc: an unlisted metric is not enumerated by RecLookupMatchingRecords, is still found by RecLookupRecord, and returns to enumeration when relisted.

Documented in doc/developer-guide/internal-libraries/Metrics.en.rst, which now has an Enumerating metrics section stating that for_each is the only way to enumerate and why.

[Copilot]

🟡 Changes recommended

The new tombstone and iterator code paths need additional defensive validation and invariant enforcement to avoid incorrect behavior or potential out-of-bounds access on manufactured/invalid IDs.

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

Pull request overview

This PR adds a “tombstone” mechanism to ts::Metrics so a metric can be withdrawn from publication (hidden from iteration/enum-based consumers) while remaining resolvable by exact name / id and keeping its backing atomic/value.

Changes:

• Extend ts::Metrics::Storage with a per-slot flag array and public tombstone() / tombstoned() APIs.
• Update ts::Metrics iteration semantics to skip tombstoned slots and to use a snapshot bound captured at iterator construction.
• Add unit tests covering tombstoning behavior across both ts::Metrics and records lookup, plus documentation updates.

File summaries

File	Description
src/tsutil/Metrics.cc	Implements tombstone flagging and iterator behavior changes (snapshot bound + skip).
include/tsutil/Metrics.h	Exposes the tombstone API, adds per-slot flag storage, and updates iterator semantics/contracts.
src/tsutil/unit_tests/test_Metrics.cc	Adds coverage for tombstone behavior, iteration skipping, resurrection, and edge cases.
src/records/unit_tests/test_RecHiddenMetricLookup.cc	Verifies record lookup behavior with tombstoned metrics (enumeration vs exact lookup).
doc/developer-guide/internal-libraries/Metrics.en.rst	Documents the tombstone feature and its interaction with find()/iteration.

Review details

Suppressed comments (2)

src/tsutil/Metrics.cc:288

• Storage::tombstoned() indexes the per-slot flag array with offset without validating that offset < MAX_SIZE (or that the slot is allocated in the current blob). A manufactured/invalid IdType with a large offset can trigger out-of-bounds access; it should safely return false for non-allocated/non-sensical IDs.

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

  if (!blob) {
    return false;
  }

  return (std::get<2>(*blob)[offset].load(MEMORY_ORDER) & TOMBSTONE) != 0;
}

src/tsutil/Metrics.cc:296

• The positional iterator ctor iterator(const Metrics&, IdType) does not call skip_tombstoned(). That allows external callers to construct an iterator that points at a tombstoned slot (contradicting the intended "iteration never visits marked slots" invariant) and reintroduces the non-terminating range-walk risk if such an iterator is used as a bound.

Metrics::iterator::iterator(const Metrics &m, IdType pos) : _metrics(m), _it(pos), _bound(m._storage->current_id()) {}

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

---

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

[cmcfarlen]

Thanks — all three findings were real, including the two that were filed as suppressed comments. Fixed in 9ed3ee8.

tombstone() / tombstoned() accepting ids that name no allocated slot. The bound check was blob_ix == _cur_blob && offset > _cur_off, which only constrains the current blob. For any blob below it the offset is the low 16 bits of the id and so can reach 65535 against a 1024-entry array, and tombstone() writes through it. Both now go through a new Storage::allocated():

bool
allocated(IdType id) const
{
  auto [blob, entry] = _splitID(id);

  if (id < 0 || entry >= MAX_SIZE || !_blobs[blob]) {
    return false;
  }

  return blob < _cur_blob || (blob == _cur_blob && entry < _cur_off);
}

This is deliberately stricter than the existing valid() in the second way you noted as well: _cur_off is the next free slot, so valid() accepts one slot that does not exist yet. That mattered more than I expected. A new test marked the free slot, and an unrelated metric created in a later test section landed in it and came out invisible, because create() only clears the flag when it finds the name already present, not when it allocates a fresh slot. So the "harmless" case was a real way to lose a metric.

I left valid() alone rather than tightening it here — other callers depend on its current semantics and that is a separate change.

The positional iterator constructor. You are right that it let a caller rest an iterator on a tombstoned slot, which reintroduces the non-terminating range-walk. Rather than only skipping, the three constructors are now private with friend class Metrics, so begin(), end() and find() are the only ways to obtain one. find() already resolves a tombstoned name to end(). The positional constructor also skips now, so the invariant holds for any future in-class use.

New tests covering each case: an offset past the end of a full blob, the next free slot, and a blob index that was never allocated.

[cmcfarlen]

Follow-up on a review question: allocated() indexes _blobs with a blob index taken from a caller-supplied id and does not range check it.

That is in fact safe, but only by coincidence. _splitID masks the blob index with METRIC_TYPE_MASK (0x1FFF), and MAX_BLOBS is 8192, so the index is always a valid _blobs subscript by construction rather than by a check. Nothing in the code tied those two constants together, and every other accessor that splits an id — lookup, name, rename, valid — depends on the same relationship.

b4fee22 makes it explicit:

static_assert(MAX_BLOBS == METRIC_TYPE_MASK + 1, "a masked blob index must always be a valid _blobs index");

Verified it fires: dropping MAX_BLOBS to 4096 fails the build rather than silently producing out-of-range subscripts throughout the class.

Also added a test for the largest possible id, which exercises the other half — the offset is not masked to the blob size, so it needs the explicit entry >= MAX_SIZE check to avoid running off the end of a blob.

Worth noting for a possible follow-up, out of scope here: METRIC_TYPE_MASK is misnamed. It has exactly one use, masking the blob index in _splitID, and has nothing to do with the metric type, which lives at METRIC_TYPE_BITS. Renaming it would be a one-line change but it is a public constant, so I left it alone.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

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

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

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

[Copilot]

🔵 Needs a closer look

It changes core metrics storage/iteration semantics used broadly across ATS, and needs human validation with full CI results and concurrency/compatibility scrutiny.

Review details

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

[cmcfarlen]

Went through all six. Two were already addressed, three are the same point and are now fixed, and one is incorrect — details below.

Fixed in ef97aa6

create() reimplementing the flag clear, and the duplicated ~UNLISTED mask (three of the comments): create() now calls set_listed(it->second, true), so the mask appears in exactly one place and there is one code path for relisting. On the null-blob concern: a name present in _lookups always names an allocated slot, so neither the old inline check nor the new allocated() gate can fail there — but routing through one function means the failure modes cannot drift apart, which was the substance of the comment.

current() returning std::pair<int16_t, int16_t> built from two uint16_t members is now std::pair<uint16_t, uint16_t>. The values are small enough that the narrowing never mattered, but see below for why it was worth removing.

Not a defect: the negative-offset concern

entry comes from _splitID(id) as an int16_t (low 16 bits). If those low bits are >= 0x8000, entry becomes negative and will pass entry >= MAX_SIZE

_splitID returns std::tuple<uint16_t, uint16_t>, so entry is unsigned. Low bits of 0xFFFF give entry == 65535, which fails entry >= MAX_SIZE and is rejected. There is no signed value and no negative subscript. The existing test for std::numeric_limits<IdType>::max() covers exactly this input — its offset bits are 0xFFFF, high bit set — and asserts both unlist and listed return false.

The int16_t in the comment is real, though, just in a different function: current(), which is what I changed above. That is very likely where the reading came from, so removing the narrowing is worth it even though allocated() was never affected.

Already addressed

The Storage::tombstone() allocated-slot comment was fixed in 9ed3ee8, which is what introduced allocated().

The PR description mismatch was real when filed; the description now documents unlist/relist/listed and includes a section on why the name changed. No compatibility alias: tombstone was never released, and it is the wrong word for this — a tombstone marks something deleted and reusable, which is how CacheShm in this same tree uses it. Nothing is deleted here.

[Copilot]

🟡 Changes recommended

The new per-slot atomic flag storage needs explicit initialization to avoid nondeterministic “unlisted” state, and one unit-test section can dereference begin() after making the store empty (undefined behavior).

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

Review details

Suppressed comments (1)

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

src/tsutil/unit_tests/test_Metrics.cc:740

• This section dereferences *m.begin() after unlisting id 0; if this TEST_CASE path runs before any other metric is created, begin()==end() and dereferencing is undefined. Create a guaranteed-listed metric first so begin() is always safe to dereference.

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

[cmcfarlen]

FlagStorage uses std::atomic<uint8_t>, whose default constructor does not initialize the stored value.

That was true through C++17, but P0883 changed std::atomic's default constructor to value-initialize the contained value in C++20, and this tree builds -std=c++20.

It also does not depend on that change. addBlob() allocates with std::make_unique<Metrics::NamesAndAtomics>(), which is new T() — value-initialization — and that recurses through the tuple and the std::array to every element. Under the pre-C++20 rules the atomic's defaulted default constructor was trivial, so value-initialization zero-initialized its storage anyway. Both readings give a zero flag byte, so a new slot starts listed.

I would rather not argue that from the standard, so 02d30f9 asserts it instead. The blob growth test already creates MAX_SIZE + 100 metrics, which spans a blob boundary; it now also requires listed(id) for every one of them, so a full blob's worth of freshly allocated slots is checked.

Confirmed the assertion is not vacuous. Storing 0xFF into the flag array immediately after the allocation fails it:

test_Metrics.cc:600: FAILED:
  REQUIRE( h.listed(id) )

That matters more than the standard argument, because reading uninitialized heap frequently does return zero — fresh pages are zero-filled — so this class of bug hides well and a test that only samples a metric or two would not catch it.

I did not add an explicit initialization loop. It would be dead work on every blob, and the real risk is not today's behavior but a future change to something like make_unique_for_overwrite; the test catches that, and addBlob() now says so at the allocation.

[Copilot]

🟡 Changes recommended

Metrics::iterator can incorrectly treat listed GAUGE metrics as “end” due to type-bit contamination in find() positional iterators when using the new _bound numeric comparison.

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

Review details

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

[cmcfarlen]

This one is a real bug, and mine. Fixed in 7b19905.

find() returned an iterator whose _it was the stored id, type bits and all, while _bound comes from current_id() and is built with COUNTER type bits. For a GAUGE the type bit at METRIC_TYPE_BITS puts _it above any realistic bound, so at_end() was immediately true and the iterator compared equal to end(). find() was therefore broken for every gauge in the store, not just unlisted ones. advance() already normalized the position; the positional constructor did not.

It is a regression from the bound comparison I introduced with unlisting — before that, operator== compared raw ids and the type bits cancelled out. No production code calls find(), so nothing was broken in the field, but every test I wrote for it happened to use a counter, which is why it got through.

Fixed as suggested, by keeping only the blob and offset:

auto [blob, offset] = _metrics._splitID(pos);

_it = _makeId(blob, offset, MetricType::COUNTER);

Dereferencing is unaffected: Storage::lookup(id, ...) deliberately reads the type from the slot rather than the id, precisely because iterators manufacture positions.

Test added first and watched fail on REQUIRE(g != m.end()), now covering both a gauge and a counter through find(), including that the dereferenced type comes back as GAUGE.

Separately, I have dropped the commit I added earlier about FlagStorage initialization. That comment was incorrect and a reply should have been the whole response; the extra commit was noise.

[Copilot]

🟡 Changes recommended

The new allocated()-based checks introduce unsynchronized reads of shared storage state that can be exercised by the new public APIs, creating a C++ data race risk under concurrent metric registration.

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

Review details

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

[cmcfarlen]

Correct as stated, but deliberately out of scope here.

allocated() reads _blobs[blob], _cur_blob and _cur_off without the mutex because that is what every other id-taking accessor in this class already does:

• Storage::valid() — the same two fields, unlocked
• Storage::lookup(IdType, ...) — !blob || (blob_ix == _cur_blob && offset > _cur_off), unlocked
• Storage::name(IdType) — the same guard, unlocked
• Storage::rename() — the same guard, before it takes the lock

So this is not a race introduced by unlisting; it is the existing synchronization model of Storage, and allocated() was written to match it rather than to invent a second convention in the same class. Making just this one function lock while its neighbours do not would be misleading about the guarantees, and taking _mutex in allocated() would also put a lock in the iterator's skip path, which is currently lock free by design.

The synchronization of this class is being addressed directly in #13583, "Metrics: close the id lookup race and bounds gaps left by the lock revert". That is the right place for it — it is a property of the whole store, not of this feature, and fixing it in two PRs at once would just produce conflicts.

Whichever of the two lands second should extend the fix to cover the other's accessors: if #13583 goes first, allocated() needs the same treatment; if this one goes first, #13583 picks it up along with valid(), lookup() and name().

[cmcfarlen]

Five comments, one real bug. Taking them in order of consequence.

The subrange across snapshots — real, fixed in e776b65

This is a genuine defect and reachable through the public API. Two find() calls with a metric created between them is enough:

auto start = m.find("a");        // captures bound B1
Metrics::Counter::create("z");   // store grows
auto stop  = m.find("z");        // bound B2 > B1, and its position is >= B1

for (auto it = start; it != stop; ++it) { ... }   // never terminates

it passes B1, becomes exhausted against its own bound, stop is still live against B2, so they never compare equal and operator++ cannot make progress. I wrote the test first, with a step cap so it fails rather than hanging, and watched it fail.

Fixed by judging exhaustion between two positional iterators against the earlier of the two bounds, so such a subrange ends at the earlier snapshot. The sentinel keeps its own answer, since its bound is meaningless — folding it into the minimum would make every other iterator compare exhausted immediately.

My earlier subrange test missed this because it created both endpoints after all the metrics, so both held the same bound.

The count > 0 coupling — fair, fixed in 1f0f4ca

Filed twice, and right both times: that assertion did depend on the shared store holding at least one listed metric. In practice slot 0 is always listed so it could not actually fail, but the coupling is real and pointless. The section now creates a listed metric of its own and asserts it is observed.

One correction to the suggestion: an anchor placed before the unlisted tail proves the loop ran, not that iteration reached the tail. The "none of the tail names appear" assertion is what covers the tail. The comment says only the former.

Structured bindings and -Werror — not an issue

The concern is that binding [name, type, value] and using only name trips an unused-variable warning under -Werror. Neither clang nor gcc warns per element of a structured binding, and I can show it rather than argue it: the exact translation unit compiles with

-Wall -Werror -Wextra -Wsuggest-override -Wthread-safety ...

and is clean. The pattern is also already in the tree — two occurrences in this same file before this PR, three in RecCore.cc. Rewriting to std::get<0>(entry) would diverge from that for no benefit.

listed() const-correctness — declining, but happy to be overruled

Correct that NamesAndAtomics const * would be tighter in a const method. I left it because lookup(IdType, ...) and name() immediately above it do exactly the same thing, and changing one of three makes the file less consistent, not more. It is a worthwhile sweep across all of them as its own change; say the word if you would rather I just do the one here.

Both commits build and pass test_tsutil individually.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

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

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

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

Suppressed comments (1)

src/tsutil/Metrics.cc:1

• iterator(const Metrics&, IdType) normalizes _it to MetricType::COUNTER, but Storage::type() derives the type from the id bits (_extractType(id)). If the iterator id is coerced to COUNTER, iteration and find() dereference can report the wrong metric type (e.g., GAUGE/COUNTER mismatches) and potentially break any logic that depends on the encoded type. A robust fix is to keep _it as the canonical id (with its real type bits) and compare exhaustion/ordering using a separate positional value (e.g., blob+offset linear index), or to fetch the type for dereference from the slot’s stored id rather than from _it.

/** @file

[cmcfarlen]

Two to answer, since the FlagStorage one is a repeat and already addressed above.

Iterator equality is not an equivalence relation — correct, documented in 03452ff

The counterexample is real and I can be precise about it. With bounds B1 < B2 and an iterator a holding B1, b holding B2, and b positioned in [B1, B2): a == b (both past the shared minimum), a == end() (past its own bound), but b != end(). Transitivity fails.

The cause is structural rather than a slip in the comparison: exhaustion is a property of each iterator's own snapshot, so any iterator-versus-end test is per-iterator, and no comparison rule over a single type can paper over that.

I looked at the sentinel design you suggest. It does fix it properly — iterator-versus-iterator becomes pure position equality, iterator-versus-sentinel carries the end test, and the two are different relations so nothing is required to hold across them. The cost is that end()'s return type changes in an installed header and std::distance(m.begin(), m.end()) stops compiling; there are two such lines in this file already, predating this PR.

We decided against it for now and documented the limitation instead. These are input_iterator_tag, the only uses in tree are range-for over the whole store, std::distance(begin, end), and find() != end(), and none of them depends on transitivity. Trading a public type change for a property nothing relies on did not seem the right call inside this PR. The note on operator== now says plainly that only same snapshot comparisons and comparison against end() are meaningful, and that these should not be handed to a generic algorithm — so the next person meets the constraint at the point of use rather than deriving it. If you would rather have the sentinel, it is a contained change and I am happy to do it.

Worth adding: the min-bound rule this replaced a hang, not correct behaviour. Before it, that same subrange did not terminate at all.

Many SECTIONs mutating the singleton store — declining

The remedy does not do what it is meant to here. Splitting into separate TEST_CASEs would only reduce accumulation if the shared body registered metrics on each re-run, and this one does not:

TEST_CASE("Metrics unlisting", "[libtsapi][Metrics]")
{
  auto &m = Metrics::instance();

  SECTION(...)

The body is a reference binding. Every registration happens inside a SECTION, so it happens exactly once whichever way the cases are split, and the total is identical. Splitting also would not reset anything — it is the same process and the same singleton either way, and there is deliberately no reset hook, since the store never frees a slot.

On volume: the largest fill in this test case is 8 metrics, and it adds a few dozen in total against a store that holds 8M. The pattern of many sections over Metrics::instance() is also what the rest of this file already does.

The one place this was a real problem was an assertion that depended on the store holding at least one listed metric from elsewhere, which is fixed in 1f0f4ca.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

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

[cmcfarlen]

Both of these arrived twice; answering once each.

input_iterator_tag versus the note — fair, and my note caused it. Reframed in 0d16232

The objection lands on the previous wording rather than on the code: declaring std::input_iterator_tag and then saying "not iterators to hand to a generic algorithm" advertises a contract and denies it in the same breath. That was mine and it was the wrong framing.

The framing that fits what the code does: a snapshot is the sequence. begin() fixes the extent to be walked; iterators from two different snapshots are no more comparable than iterators into two different containers, and the standard already treats that as outside the domain rather than as a broken relation. Within one snapshot every iterator shares a bound, so equality is exactly the equivalence relation an input iterator requires, and the tag is honest.

The min-bound rule then has a narrower job than the old note implied: it makes the out-of-domain case terminate instead of hang. Before it, that comparison did not terminate at all, which is a worse kind of unspecified.

On the two suggested remedies: the sentinel design does fix the relation properly and I priced it out — end()'s return type changes in an installed header and std::distance(m.begin(), m.end()) stops compiling, which is two lines in this file that predate the PR. We decided that was not a trade to make inside this PR. It remains a contained change if a maintainer prefers it.

Private iterator constructors are a source break — correct, and now documented

Accurate: iterator(const Metrics &, IdType) was public and is not any more.

I am keeping it private. Nothing in tree constructs an iterator directly, and there is no sensible reason to — a caller holding an id wants lookup, not an iterator positioned at one slot. More to the point, that public constructor is where both iterator defects found in this review came from: a gauge id whose type bits put it past the bound, and a subrange built from two snapshots that never terminated. Both required naming an arbitrary position. The suggested compromise, keeping it public but normalising unlisted positions to end(), closes neither.

What was missing was disclosure, so the PR description now has an API change section recording it, for the 11.0.0 release notes alongside the createSpan and rename removals from #13583.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

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

[cmcfarlen]

Follow-up #13666 opens the consumer of unlist: the per server connection metrics retract their published names when metric_aggregate changes. It is a draft while this PR is open, since it carries this branch's commit until then.

[bneradt]

Two correctness concerns with changes to listing state during iterator use. I validated both using the iterator/find implementation from this revision in an isolated C++20 harness with a mock store; I did not run the full ATS test suite. The retained metric storage is an intentional, documented limitation and is not a finding.

[bneradt]

[P2] Keep find() from returning a different metric after concurrent unlisting

There is a check/use gap between listed(id) above and the positional constructor's skip_unlisted(). With adjacent listed metrics a and b, find("a") can observe listed(a) == true, another thread can unlist a, and the constructor then advances to b. The returned iterator is non-end but dereferences to a metric whose name does not match the query. I reproduced that interleaving by triggering the unlist when the constructor acquires its bound. Please make the find path retain the requested position or return end if that position is skipped, rather than accepting the next listed metric, and add a regression test for this interleaving.

[cmcfarlen]

Confirmed the check/use gap: listed(id) here and skip_unlisted() in the positional constructor are two separate decisions, so an unlist in between makes the returned iterator dereference to the next listed metric instead of the one named.

Rather than make the find path retain the requested position, the positional constructor and find() are both gone as of f340f14. Enumeration is now Metrics::for_each(func) and there is no public iterator at all, so there is no position for a caller to name and nothing to race against. lookup() is the way to reach a single metric by name, which is what RecLookupRecord, LogAccess and TSStatFindName already used.

Worth noting find() predated this PR and was safe there -- the constructor did not skip, and equality was a plain position compare. This PR is what made it unsafe, which is why removing it here rather than in a separate change seemed right. It has no callers in src, include, plugins, example or tools; the only ones were the tests added by this PR. Do you know of out-of-tree consumers? Metrics.h is in TSUTIL_PUBLIC_HEADERS so this is a source-breaking removal, and I would rather hear about users now than after 11.0.0.

[bneradt]

[P2] Account for a saved subrange endpoint becoming unlisted

The shared-bound rule does not cover listing changes within the same allocation snapshot. Create a, b, and c, obtain start = m.find("a") and stop = m.find("b"), then call m.unlist("b") without creating any metrics. A walk for (auto it = start; it != stop; ++it) skips b, visits c outside the intended range, and passes its end without ever comparing equal to stop: the saved stop remains below the shared bound while the walking iterator is exhausted. This also arises when another thread unlists the endpoint during traversal. The current subrange test only unlists interior slots before obtaining the iterators. Please either support this endpoint transition or explicitly define and enforce the iterator invalidation/synchronization requirement for unlisting, with a regression test; snapshotting only the allocation bound does not preserve the endpoint.

[cmcfarlen]

Reproduced, and it goes one step further than described. With a, b, c listed, start = find("a"), stop = find("b"), then unlist("b"): the walk visits a, skips the unlisted b and lands on c, compares live-vs-live so it visits c outside the range, then exhausts. At that point at_end() is true for the walker and false for the saved stop, so a && b is false and != stays true while ++ cannot make progress -- it spins, dereferencing _it == _bound. Storage::lookup guards with offset > _cur_off, so one-past-end slips past the redirect and yields an empty name rather than bad_id. So: out-of-range visit followed by a non-terminating loop.

Your reading of the coverage was right too -- the subrange test only unlisted interior slots, and only before obtaining the iterators. Neither test unlisted an endpoint after capturing it.

I went with the second of your two options, but by construction rather than by contract: as of f340f14 there is no way to form a subrange. Metrics::for_each(func) is the only enumeration, always the whole store, so there is no cursor to save across a listing change and no invalidation rule to define or enforce. _bound, the shared-bound comparison and the equality rules are all gone with the iterator. The two subrange tests are deleted rather than extended, since what they covered can no longer be expressed.

Net effect on the PR is 170 insertions against 344 deletions.

[Copilot]

🟡 Changes recommended

Add the direct <utility> include and document the public API removals and migration path.

Get a fresh assessment by requesting another Copilot review.

Review details

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