fix: unregister metrics on reconciler stop to prevent panic on restart (#21647)

## Description

Fixes a panic that occurs when the prebuilds feature is toggled by
adding/removing a license. The `StoreReconciler` was not unregistering
the `reconciliationDuration` histogram, causing a "duplicate metrics
collector registration attempted" panic when a new reconciler was
created.

## Changes

* Unregister the `reconciliationDuration` histogram in `Stop()`
alongside the existing metrics collector
* Change log level when stopping the reconciler with a cause, since
"entitlements change" is not an error condition
* Add `TestReconcilerLifecycle` to verify the reconciler can be stopped
and recreated with the same prometheus registry

Related to internal slack thread:
https://codercom.slack.com/archives/C07GRNNRW03/p1769116582171379
This commit is contained in:
Susana Ferreira
2026-01-23 14:45:27 +00:00
committed by GitHub
parent 9843adb8c6
commit f5858c8a18
2 changed files with 52 additions and 3 deletions
+8 -3
View File
@@ -260,9 +260,9 @@ func (c *StoreReconciler) Stop(ctx context.Context, cause error) {
defer c.running.Store(false)
if cause != nil {
c.logger.Error(context.Background(), "stopping reconciler due to an error", slog.Error(cause))
c.logger.Info(context.Background(), "stopping reconciler", slog.F("cause", cause.Error()))
} else {
c.logger.Info(context.Background(), "gracefully stopping reconciler")
c.logger.Info(context.Background(), "stopping reconciler")
}
// If previously stopped (Swap returns previous value), then short-circuit.
@@ -272,7 +272,7 @@ func (c *StoreReconciler) Stop(ctx context.Context, cause error) {
return
}
// Unregister the metrics collector.
// Unregister prebuilds state and operational metrics.
if c.metrics != nil && c.registerer != nil {
if !c.registerer.Unregister(c.metrics) {
// The API doesn't allow us to know why the de-registration failed, but it's not very consequential.
@@ -281,6 +281,11 @@ func (c *StoreReconciler) Stop(ctx context.Context, cause error) {
// feature again. If the metrics cannot be registered, it'll log an error from NewStoreReconciler.
c.logger.Warn(context.Background(), "failed to unregister metrics collector")
}
if c.reconciliationDuration != nil {
if !c.registerer.Unregister(c.reconciliationDuration) {
c.logger.Warn(context.Background(), "failed to unregister reconciliation duration histogram")
}
}
}
// If the reconciler is not running, there's nothing else to do.
@@ -1401,6 +1401,50 @@ func TestRunLoop(t *testing.T) {
reconciler.Stop(ctx, nil)
}
// TestReconcilerLifecycle tests that a StoreReconciler can be stopped and a new one
// created to simulate the prebuilds feature being disabled and re-enabled.
func TestReconcilerLifecycle(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitLong)
logger := testutil.Logger(t)
db, ps := dbtestutil.NewDB(t)
cfg := codersdk.PrebuildsConfig{
ReconciliationInterval: serpent.Duration(testutil.WaitLong),
}
registry := prometheus.NewRegistry()
cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{})
// Given: a running reconciler (simulating the prebuilds feature being enabled)
reconciler := prebuilds.NewStoreReconciler(
db, ps, cache, cfg, logger,
quartz.NewMock(t),
registry,
newNoopEnqueuer(),
newNoopUsageCheckerPtr(),
noop.NewTracerProvider(),
10,
)
// When: the reconciler is stopped (simulating the prebuilds feature being disabled)
reconciler.Stop(ctx, xerrors.New("entitlements change"))
// Then: a new reconciler can be created without error
// (simulating the prebuilds feature being re-enabled)
reconciler = prebuilds.NewStoreReconciler(
db, ps, cache, cfg, logger,
quartz.NewMock(t),
registry,
newNoopEnqueuer(),
newNoopUsageCheckerPtr(),
noop.NewTracerProvider(),
10,
)
// Gracefully stop the reconciliation loop
reconciler.Stop(ctx, nil)
}
func TestFailedBuildBackoff(t *testing.T) {
t.Parallel()