From 5cb900bfad0c364bd25cede9d9bb8977e8f2ff90 Mon Sep 17 00:00:00 2001 From: nikitapogromsky <129324283+nikitapogromsky@users.noreply.github.com> Date: Mon, 7 Sep 2026 12:03:39 +0300 Subject: [PATCH 1/2] logger: make AddSystemTarget idempotent Subscribe re-registered the console target on every console-log subscription, producing duplicate minio_logger_webhook_* series on each /minio/metrics/v3 scrape. Fixes #150 Signed-off-by: nikitapogromsky <129324283+nikitapogromsky@users.noreply.github.com> --- internal/logger/targets.go | 36 ++++++++++- internal/logger/targets_test.go | 105 ++++++++++++++++++++++++++++++++ 2 files changed, 138 insertions(+), 3 deletions(-) create mode 100644 internal/logger/targets_test.go diff --git a/internal/logger/targets.go b/internal/logger/targets.go index 774237e1d..9fcca6b14 100644 --- a/internal/logger/targets.go +++ b/internal/logger/targets.go @@ -59,11 +59,35 @@ func (tl *targetsList) get() []Target { return tl.list } -func (tl *targetsList) add(t Target) { +// contains reports whether t is already registered. +func (tl *targetsList) contains(t Target) bool { + tl.mu.RLock() + defer tl.mu.RUnlock() + + return tl.indexOf(t) >= 0 +} + +// addIfAbsent appends t unless it is already registered. +// Returns true if t was added. +func (tl *targetsList) addIfAbsent(t Target) bool { tl.mu.Lock() defer tl.mu.Unlock() + if tl.indexOf(t) >= 0 { + return false + } tl.list = append(tl.list, t) + return true +} + +// indexOf must be called with tl.mu held. +func (tl *targetsList) indexOf(t Target) int { + for i, existing := range tl.list { + if existing == t { + return i + } + } + return -1 } func (tl *targetsList) set(tgts []Target) { @@ -125,8 +149,14 @@ func CurrentStats() map[string]types.TargetStats { } // AddSystemTarget adds a new logger target to the -// list of enabled loggers +// list of enabled loggers. Adding a target that is already +// registered is a no-op, so callers may safely re-register +// long-lived targets such as the console logger. func AddSystemTarget(ctx context.Context, t Target) error { + if systemTargets.contains(t) { + return nil + } + if err := t.Init(ctx); err != nil { return err } @@ -137,7 +167,7 @@ func AddSystemTarget(ctx context.Context, t Target) error { } } - systemTargets.add(t) + systemTargets.addIfAbsent(t) return nil } diff --git a/internal/logger/targets_test.go b/internal/logger/targets_test.go new file mode 100644 index 000000000..5552eb734 --- /dev/null +++ b/internal/logger/targets_test.go @@ -0,0 +1,105 @@ +// Copyright (c) 2015-2026 MinIO, Inc. +// +// This file is part of MinIO Object Storage stack +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package logger + +import ( + "context" + "sync" + "sync/atomic" + "testing" + + types "github.com/minio/minio/internal/logger/target/loggertypes" +) + +// fakeTarget is a minimal Target used to exercise the registry. +type fakeTarget struct { + name string + kind types.TargetType + inits atomic.Int32 +} + +func (f *fakeTarget) String() string { return f.name } +func (f *fakeTarget) Endpoint() string { return "" } +func (f *fakeTarget) Stats() types.TargetStats { return types.TargetStats{} } +func (f *fakeTarget) Init(context.Context) error { f.inits.Add(1); return nil } +func (f *fakeTarget) IsOnline(context.Context) bool { return true } +func (f *fakeTarget) Cancel() {} +func (f *fakeTarget) Send(context.Context, any) error { return nil } +func (f *fakeTarget) Type() types.TargetType { return f.kind } + +// swapSystemTargets isolates the package-level registry for a test. +func swapSystemTargets(t *testing.T) { + t.Helper() + prevTargets, prevConsole := systemTargets, consoleTgt + systemTargets, consoleTgt = newTargetsList(), nil + t.Cleanup(func() { + systemTargets, consoleTgt = prevTargets, prevConsole + }) +} + +// AddSystemTarget must not register the same target twice: the console +// logger is re-added on every console-log subscription, and duplicates +// surface as identical series in the /logger/webhook metrics collector. +func TestAddSystemTargetIdempotent(t *testing.T) { + swapSystemTargets(t) + ctx := context.Background() + + console := &fakeTarget{name: "console+http", kind: types.TargetConsole} + for range 3 { + if err := AddSystemTarget(ctx, console); err != nil { + t.Fatalf("AddSystemTarget: %v", err) + } + } + if got := len(SystemTargets()); got != 1 { + t.Fatalf("expected 1 system target after repeated add, got %d", got) + } + if got := console.inits.Load(); got != 1 { + t.Fatalf("expected Init to run once, ran %d times", got) + } + if consoleTgt != console { + t.Fatalf("consoleTgt not set to the console target") + } + + other := &fakeTarget{name: "other", kind: types.TargetHTTP} + if err := AddSystemTarget(ctx, other); err != nil { + t.Fatalf("AddSystemTarget: %v", err) + } + if got := len(SystemTargets()); got != 2 { + t.Fatalf("expected 2 distinct system targets, got %d", got) + } +} + +func TestAddSystemTargetConcurrent(t *testing.T) { + swapSystemTargets(t) + ctx := context.Background() + + console := &fakeTarget{name: "console+http", kind: types.TargetConsole} + var wg sync.WaitGroup + for range 32 { + wg.Add(1) + go func() { + defer wg.Done() + _ = AddSystemTarget(ctx, console) + }() + } + wg.Wait() + + if got := len(SystemTargets()); got != 1 { + t.Fatalf("expected 1 system target after concurrent adds, got %d", got) + } +} From 079ebb19261d5649074e5b1d275a89f721d7cd37 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Tue, 8 Sep 2026 12:58:21 +0800 Subject: [PATCH 2/2] fix: serialize logger initialization and publish the console target safely Signed-off-by: Feng Ruohang --- internal/logger/logger.go | 8 ++--- internal/logger/targets.go | 15 +++++--- internal/logger/targets_test.go | 64 ++++++++++++++++++++++++++++----- 3 files changed, 69 insertions(+), 18 deletions(-) diff --git a/internal/logger/logger.go b/internal/logger/logger.go index 09573ab49..335ce88fd 100644 --- a/internal/logger/logger.go +++ b/internal/logger/logger.go @@ -395,9 +395,9 @@ func consoleLogIf(ctx context.Context, subsystem string, err error, errKind ...a if err == nil { return } - if consoleTgt != nil { + if console := consoleTgt.Load(); console != nil { entry := errToEntry(ctx, subsystem, err, errKind...) - consoleTgt.Send(ctx, entry) + (*console).Send(ctx, entry) } } @@ -423,8 +423,8 @@ func sendLog(ctx context.Context, entry log.Entry) { // Iterate over all logger targets to send the log entry for _, t := range systemTgts { if err := t.Send(ctx, entry); err != nil { - if consoleTgt != nil { // Sending to the console never fails - consoleTgt.Send(ctx, errToEntry(ctx, "logging", fmt.Errorf("unable to send log event to Logger target (%s): %v", t.String(), err), entry.Level)) + if console := consoleTgt.Load(); console != nil { // Sending to the console never fails + (*console).Send(ctx, errToEntry(ctx, "logging", fmt.Errorf("unable to send log event to Logger target (%s): %v", t.String(), err), entry.Level)) } } } diff --git a/internal/logger/targets.go b/internal/logger/targets.go index 9fcca6b14..0b2d20525 100644 --- a/internal/logger/targets.go +++ b/internal/logger/targets.go @@ -22,6 +22,7 @@ import ( "fmt" "strings" "sync" + "sync/atomic" "github.com/minio/minio/internal/logger/target/http" "github.com/minio/minio/internal/logger/target/kafka" @@ -106,7 +107,10 @@ var ( auditTargets = newTargetsList() // This is always set represent /dev/console target - consoleTgt Target + consoleTgt atomic.Pointer[Target] + + // Init may emit logs, so serialize registration without holding the list lock. + systemTargetInitMu sync.Mutex ) // SystemTargets returns active targets. @@ -153,6 +157,9 @@ func CurrentStats() map[string]types.TargetStats { // registered is a no-op, so callers may safely re-register // long-lived targets such as the console logger. func AddSystemTarget(ctx context.Context, t Target) error { + systemTargetInitMu.Lock() + defer systemTargetInitMu.Unlock() + if systemTargets.contains(t) { return nil } @@ -161,10 +168,8 @@ func AddSystemTarget(ctx context.Context, t Target) error { return err } - if consoleTgt == nil { - if t.Type() == types.TargetConsole { - consoleTgt = t - } + if t.Type() == types.TargetConsole { + consoleTgt.CompareAndSwap(nil, &t) } systemTargets.addIfAbsent(t) diff --git a/internal/logger/targets_test.go b/internal/logger/targets_test.go index 5552eb734..e1075df86 100644 --- a/internal/logger/targets_test.go +++ b/internal/logger/targets_test.go @@ -19,6 +19,7 @@ package logger import ( "context" + "errors" "sync" "sync/atomic" "testing" @@ -31,12 +32,19 @@ type fakeTarget struct { name string kind types.TargetType inits atomic.Int32 + init func() error } -func (f *fakeTarget) String() string { return f.name } -func (f *fakeTarget) Endpoint() string { return "" } -func (f *fakeTarget) Stats() types.TargetStats { return types.TargetStats{} } -func (f *fakeTarget) Init(context.Context) error { f.inits.Add(1); return nil } +func (f *fakeTarget) String() string { return f.name } +func (f *fakeTarget) Endpoint() string { return "" } +func (f *fakeTarget) Stats() types.TargetStats { return types.TargetStats{} } +func (f *fakeTarget) Init(context.Context) error { + f.inits.Add(1) + if f.init != nil { + return f.init() + } + return nil +} func (f *fakeTarget) IsOnline(context.Context) bool { return true } func (f *fakeTarget) Cancel() {} func (f *fakeTarget) Send(context.Context, any) error { return nil } @@ -45,10 +53,12 @@ func (f *fakeTarget) Type() types.TargetType { return f.kind } // swapSystemTargets isolates the package-level registry for a test. func swapSystemTargets(t *testing.T) { t.Helper() - prevTargets, prevConsole := systemTargets, consoleTgt - systemTargets, consoleTgt = newTargetsList(), nil + prevTargets, prevConsole := systemTargets, consoleTgt.Load() + systemTargets = newTargetsList() + consoleTgt.Store(nil) t.Cleanup(func() { - systemTargets, consoleTgt = prevTargets, prevConsole + systemTargets = prevTargets + consoleTgt.Store(prevConsole) }) } @@ -71,7 +81,7 @@ func TestAddSystemTargetIdempotent(t *testing.T) { if got := console.inits.Load(); got != 1 { t.Fatalf("expected Init to run once, ran %d times", got) } - if consoleTgt != console { + if got := consoleTgt.Load(); got == nil || *got != console { t.Fatalf("consoleTgt not set to the console target") } @@ -89,17 +99,53 @@ func TestAddSystemTargetConcurrent(t *testing.T) { ctx := context.Background() console := &fakeTarget{name: "console+http", kind: types.TargetConsole} + console.init = func() error { + // Initialization is allowed to inspect or write to the existing logger. + _ = SystemTargets() + consoleLogIf(ctx, "test", errors.New("initializing logger")) + return nil + } var wg sync.WaitGroup + start := make(chan struct{}) for range 32 { wg.Add(1) go func() { defer wg.Done() - _ = AddSystemTarget(ctx, console) + <-start + if err := AddSystemTarget(ctx, console); err != nil { + t.Errorf("AddSystemTarget: %v", err) + } + consoleLogIf(ctx, "test", errors.New("concurrent logger")) }() } + close(start) wg.Wait() if got := len(SystemTargets()); got != 1 { t.Fatalf("expected 1 system target after concurrent adds, got %d", got) } + if got := console.inits.Load(); got != 1 { + t.Fatalf("expected one concurrent initialization, got %d", got) + } +} + +func TestAddSystemTargetRetriesFailedInit(t *testing.T) { + swapSystemTargets(t) + ctx := context.Background() + initErr := errors.New("target unavailable") + target := &fakeTarget{name: "console", kind: types.TargetConsole} + target.init = func() error { return initErr } + if err := AddSystemTarget(ctx, target); !errors.Is(err, initErr) { + t.Fatalf("expected initialization error, got %v", err) + } + if len(SystemTargets()) != 0 || consoleTgt.Load() != nil { + t.Fatal("failed initialization published a target") + } + target.init = nil + if err := AddSystemTarget(ctx, target); err != nil { + t.Fatal(err) + } + if got := len(SystemTargets()); got != 1 || target.inits.Load() != 2 { + t.Fatalf("retry: targets=%d, initializations=%d", got, target.inits.Load()) + } }