embed: configureEmbeddedMetrics leaks a registry into pkg/foundation/metrics global bookkeeping on failed Register
- Dominant language
- Go
- Stars
- 610
- Forks
- 63
- Avg merge
- 12h 28m
- Merged PRs (30d)
- 57
Description
## Context
`pkg/conduit/runtime.go`'s `configureEmbeddedMetrics` (used by the `libconduit` embed API, see
`conduit.New`/`WithMetricsRegisterer`) builds a fresh `foundation/metrics/prometheus.Registry`
per-Runtime and calls `metrics.Register(reg)` *before* attempting
`registerer.Register(reg)`/`registerer.Register(statsHandler)`:
```go
func configureEmbeddedMetrics(registerer promclient.Registerer) (*promgrpc.StatsHandler, error) {
reg := prometheus.NewRegistry(nil)
metrics.Register(reg) // documented cross-talk limitation, see WithMetricsRegisterer's doc
if err := registerer.Register(reg); err != nil {
...
return nil, conduiterr.Wrap(...)
}
statsHandler := promgrpc.ServerStatsHandler()
if err := registerer.Register(statsHandler); err != nil {
...
return nil, conduiterr.Wrap(...)
}
return statsHandler, nil
}
```
`metrics.Register(reg)` appends `reg` to `pkg/foundation/metrics`'s process-global bookkeeping
(the `global` slices that every `metrics.NewCounter`-style constructor fans values out to — the
same known cross-talk limitation already documented for the happy path). If either
`registerer.Register` call fails (e.g. a caller-reused `Registerer` with a metric-name
collision — see `TestNew_MetricNameCollision_ReturnsCodedError`), `configureEmbeddedMetrics`
returns an error and `conduit.New` fails, but `reg` was already registered into the process
global and is never unregistered. That registry — and any metric values fanned into it — leaks
for the process lifetime.
## Impact
- A failed `conduit.New` call (metric collision, or any other `Register` failure) leaves a
dangling registry in `pkg/foundation/metrics`' global bookkeeping.
- In a long-running host process that retries `conduit.New` after a fixable misconfiguration
(e.g. it swaps `MetricsRegisterer`), each failed attempt adds another leaked registry.
- Not a data-path/correctness issue (Invariants 1-7 unaffected) — this is a resource/observability
leak scoped to the embed metrics seam.
## Accepted as a v1 ("B1") limitation
Tracked, not blocking the current libconduit embed API PR (#2667) — the existing metrics
cross-talk limitation (two Engines observing each other's metric values,
`TestTwoEngines_MetricsCrossTalk_KnownLimitation`) is the umbrella known issue; this is a
sharper instance of the same root cause (process-global metric definitions in
`pkg/foundation/metrics`). Fixing it properly likely means `metrics.Register` supporting
unregister-on-failure or `configureEmbeddedMetrics` validating both `Register` calls before
calling `metrics.Register`.
## Suggested fix direction
- Reorder: attempt `registerer.Register(reg)` and `registerer.Register(statsHandler)` first
(against a throwaway/no-op collector, or by registering `statsHandler` before calling
`metrics.Register`), only calling `metrics.Register(reg)` once both registrations are known to
succeed; or
- Add an unregister path so a failed `configureEmbeddedMetrics` call cleans up its own
process-global registration before returning the error.
Contributor guide
Research direction
Start in pkg/conduit/runtime.go at configureEmbeddedMetrics and inspect pkg/foundation/metrics global bookkeeping, then run TestNew_MetricNameCollision_ReturnsCodedError and TestTwoEngines_MetricsCrossTalk_KnownLimitation. Compare the registration failure paths and determine how a failed conduit.New can avoid retaining its registry. Done means failed registration leaves no dangling registry or metric fan-out state.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, prometheus
- Domain
- observability
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100