metrics: allocation failures can terminate construction/teardown or leave stale registrations
Hi,
I found this while working on my own project, Kwaque, a Seastar-native distributed log.
Kwaque currently uses Redpanda’s Seastar fork, pinned at:
a6ac2ff6190a4a9dce5059991355703e1073d11f
While implementing Kwaque’s observability layer, I added allocation-failure tests around the lifecycle of native seastar::metrics::metric_groups. The goal was to verify this invariant:
After any failed metric registration attempt, the registry must be indistinguishable from its state before the attempt, and destroying a metric group must not allocate or leave callbacks behind.
The tests initially terminated inside Seastar rather than propagating std::bad_alloc. Continuing through construction, registration, rollback, and destruction exposed several related exception-safety gaps.
I first found these in Redpanda’s fork, but I then checked current ScyllaDB Seastar at:
9b95b250cc64af11b84fa5ea3d677f76b37fdef0
The same fundamental behavior is present there.
1. Allocating constructors are declared noexcept
The default metric_groups constructor is declared and defined as noexcept, but it calls create_metric_groups(), which allocates a metric_groups_impl using std::make_unique.
Consequently, an allocation failure calls std::terminate instead of propagating std::bad_alloc.
metric_group() inherits the same behavior through its base constructor.
Relevant code:
- https://github.com/scylladb/seastar/blob/9b95b250cc64af11b84fa5ea3d677f76b37fdef0/include/seastar/core/metrics_registration.hh#L90-L102
- https://github.com/scylladb/seastar/blob/9b95b250cc64af11b84fa5ea3d677f76b37fdef0/include/seastar/core/metrics_registration.hh#L151-L169 https://github.com/scylladb/seastar/blob/9b95b250cc64af11b84fa5ea3d677f76b37fdef0/src/core/metrics.cc#L43-L50
- https://github.com/scylladb/seastar/blob/9b95b250cc64af11b84fa5ea3d677f76b37fdef0/src/core/metrics.cc#L332-L334
The initializer-list constructor and metric_groups::clear() perform the same underlying allocation without being declared noexcept, which makes the default constructor’s specification appear accidental.
2. metric_definition also allocates under noexcept
The constructor that creates a public metric_definition from metric_definition_impl is declared noexcept, but its implementation uses std::make_unique.
This is reached through metric factory functions such as make_gauge() and make_counter(). An injected allocation failure therefore terminates instead of propagating.
Relevant code:
- https://github.com/scylladb/seastar/blob/9b95b250cc64af11b84fa5ea3d677f76b37fdef0/include/seastar/core/metrics_registration.hh#L63-L70
- https://github.com/scylladb/seastar/blob/9b95b250cc64af11b84fa5ea3d677f76b37fdef0/src/core/metrics.cc#L74-L82
metric_definition_impl::aggregate() has the same problem: it is declared noexcept, but reserves vector capacity and copies label-name strings.
Relevant code:
- https://github.com/scylladb/seastar/blob/9b95b250cc64af11b84fa5ea3d677f76b37fdef0/include/seastar/core/metrics.hh#L405-L424
- https://github.com/scylladb/seastar/blob/9b95b250cc64af11b84fa5ea3d677f76b37fdef0/src/core/metrics.cc#L315-L324
3. Registry publication precedes ownership bookkeeping
metric_groups_impl::add_metric() registers the metric in the global shard-local registry and only afterward pushes the returned registration reference into _registration.
The effective order is:
impl::add_registration(...)
_registration.push_back(...)The destructor can unregister only the entries that reached _registration.
If the vector push needs to grow and throws:
- The callback has already been inserted into the global registry.
- The owning
metric_groups_impldoes not record it. - Construction/startup unwinds.
- The group destructor cannot unregister that metric.
- A later registration can fail with
double_registration. - More seriously, the registry may retain a callback referring to an object whose construction was unwound.
Relevant code:
- https://github.com/scylladb/seastar/blob/9b95b250cc64af11b84fa5ea3d677f76b37fdef0/include/seastar/core/metrics_api.hh#L269-L284
- https://github.com/scylladb/seastar/blob/9b95b250cc64af11b84fa5ea3d677f76b37fdef0/src/core/metrics.cc#L336-L374
4. add_registration() can leave partial registry state
Reserving _registration before calling add_registration() closes the ownership-vector gap, but it does not provide a complete exception guarantee by itself.
For a new family, _value_map[name] creates the family before all metadata and the metric instance have been initialized.
For an existing family, the metric instance is inserted before all label bookkeeping completes.
Allocations can therefore fail after some registry mutation has already happened, leaving a partially initialized family or series.
Relabeling also matters here: rollback must identify the published series using the metric’s post-relabel ID and labels, rather than assuming the original ID is still the registry key.
Relevant code:
5. Metric-group destruction itself allocates
After fixing the constructor and registration paths locally, the allocation-failure test exposed another termination during teardown.
metric_groups_impl::~metric_groups_impl() calls unregister_metric() for each retained registration. impl::remove_registration() then calls metric_id::full_name() to find the family in _value_map.
full_name() constructs and sanitizes _group + "_" + _name, which can allocate.
The effective destruction path is:
metric_groups::~metric_groups()
metric_groups_impl::~metric_groups_impl()
unregister_metric()
impl::remove_registration()
metric_id::full_name()
allocation failure
std::terminateBecause destructors are implicitly non-throwing, an injected std::bad_alloc during full_name() terminates the process.
Relevant code:
- https://github.com/scylladb/seastar/blob/9b95b250cc64af11b84fa5ea3d677f76b37fdef0/src/core/metrics.cc#L336-L342
- https://github.com/scylladb/seastar/blob/9b95b250cc64af11b84fa5ea3d677f76b37fdef0/src/core/metrics.cc#L390-L398
- https://github.com/scylladb/seastar/blob/9b95b250cc64af11b84fa5ea3d677f76b37fdef0/src/core/metrics.cc#L408-L429
How I reproduced it
Kwaque has a small owner-local probe containing:
std::optional<seastar::metrics::metric_groups> metrics;Startup emplaces the group and registers a gauge whose callback captures the owner. Rollback and shutdown destroy the optional.
I exercised both startup and destruction with Seastar’s native memory::with_allocation_failures() helper.
The checks require that:
- Every allocation failure propagates to the test rather than terminating.
- No failed attempt leaves the metric family registered.
- Registering the same family after a failed attempt succeeds.
- Destroying the group removes every callback.
- Destruction makes no allocation.
- Repeated start/stop cycles leave the registry clean.
The first allocation budget exposed the noexcept construction problem. Once that was corrected locally, later budgets exposed the registration bookkeeping and partial-mutation problems. Testing destruction independently then exposed the allocating full_name() lookup.
Downstream workaround
I currently carry a narrow patch against Kwaque’s pinned Redpanda Seastar revision.
The downstream patch:
- Removes
noexceptfrom the allocating group and metric-definition constructors. - Removes
noexceptfrom aggregate-label construction. - Reserves unregister bookkeeping before registry publication.
- Rolls back a partially inserted family or series when registration fails.
- Uses the post-relabel metric identity for rollback.
- Computes the sanitized family name before publication.
- Stores that family name with the registration handle.
- Reuses the stored name during destruction so teardown performs only lookups and erases.
The focused allocation-failure lifecycle test now passes, and the broader Kwaque debug, sanitizer, and release test suites pass as well.
I do not want to present the downstream patch as directly mergeable upstream without discussion. Redpanda’s fork has handle and metric-replication extensions that differ from current ScyllaDB Seastar, although the underlying construction, registration-order, partial-mutation, and destructor-allocation problems are shared.
Expected behavior
When allocation failures are configured to throw rather than abort:
- Allocating metric construction should propagate
std::bad_alloc. - A failed
add_group()should leave the registry unchanged. - A failed attempt should not retain a callback or block a retry with
double_registration. - Rollback should use the effective post-relabel metric identity.
- Destroying
metric_groupsshould not allocate or throw. - No metric callback should survive its owning group.
Would the maintainers be interested in a PR adding allocation-failure tests and the corresponding exception-safety changes?
Source: scylladb/seastar