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 774237e1d..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" @@ -59,11 +60,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) { @@ -82,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. @@ -125,19 +153,26 @@ 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 { + systemTargetInitMu.Lock() + defer systemTargetInitMu.Unlock() + + if systemTargets.contains(t) { + return nil + } + if err := t.Init(ctx); err != nil { return err } - if consoleTgt == nil { - if t.Type() == types.TargetConsole { - consoleTgt = t - } + if t.Type() == types.TargetConsole { + consoleTgt.CompareAndSwap(nil, &t) } - 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..e1075df86 --- /dev/null +++ b/internal/logger/targets_test.go @@ -0,0 +1,151 @@ +// 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" + "errors" + "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 + 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) + 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 } +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.Load() + systemTargets = newTargetsList() + consoleTgt.Store(nil) + t.Cleanup(func() { + systemTargets = prevTargets + consoleTgt.Store(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 got := consoleTgt.Load(); got == nil || *got != 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} + 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() + <-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()) + } +}