ConduitIO / ConduitIO/conduit

embed: configureEmbeddedMetrics leaks a registry into pkg/foundation/metrics global bookkeeping on failed Register

Open
#2,669 0 comments 0 reactions 0 assignees View on GitHub
bug
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.