From cfcaef88e3dec908efb348c70912e9fe0e432af4 Mon Sep 17 00:00:00 2001 From: Moshe Immerman Date: Fri, 24 Apr 2026 14:25:20 +0300 Subject: [PATCH] feat(logger): Add SetOutput and GetOutput for dynamic log redirection Implement atomic-backed output redirection for all loggers (root and named) without requiring handler reconstruction. Uses sync/atomic.Value to wrap io.Writer in outputBox, and a sharedOutput indirection writer that all handlers capture. This allows SetOutput to retarget existing loggers immediately and safely for concurrent callers. Adds comprehensive test coverage including new logger redirection, existing logger retargeting, restore semantics, nil handling, and concurrent safety. Also adds .gavel.yaml configuration to ignore false-positive secret detections in test fixtures and certificates. --- .gavel.yaml | 24 ++++++++ logger/slog.go | 58 ++++++++++++++++++- logger/slog_setoutput_test.go | 106 ++++++++++++++++++++++++++++++++++ 3 files changed, 187 insertions(+), 1 deletion(-) create mode 100644 .gavel.yaml create mode 100644 logger/slog_setoutput_test.go diff --git a/.gavel.yaml b/.gavel.yaml new file mode 100644 index 0000000..1101f16 --- /dev/null +++ b/.gavel.yaml @@ -0,0 +1,24 @@ +commit: + compatibility: {} + linkedDeps: {} + precommit: {} +fixtures: {} +lint: + ignore: + - file: certs/fixtures/k8s-ca-key.pem + rule: private-key + source: betterleaks + - file: cmd/hx/test.out + rule: generic-api-key + source: betterleaks + - file: certs/fixtures/literal.yml + rule: private-key + source: betterleaks +secrets: {} +ssh: {} +verify: + checks: + disabled: null + disabledCategories: null + model: "" + prompt: "" diff --git a/logger/slog.go b/logger/slog.go index 8065ba8..ffc1296 100644 --- a/logger/slog.go +++ b/logger/slog.go @@ -9,6 +9,7 @@ import ( "os" "strconv" "strings" + "sync/atomic" "time" "unicode" @@ -29,6 +30,58 @@ const rootName = "root" var namedLoggers cmap.Map[string, *SlogLogger] var todo = context.TODO() +// outputBox wraps the current io.Writer so all atomic stores carry the same +// concrete type (*outputBox) — a hard requirement of sync/atomic.Value. +// Without this, swapping from *os.File to *bytes.Buffer panics at runtime. +type outputBox struct{ w io.Writer } + +// currentOutput holds a *outputBox pointing at the io.Writer every slog +// handler writes to. Handlers constructed via New / NewWithWriter receive a +// thin indirection writer (sharedOutput) that reads this atomic on every +// Write, so SetOutput swaps take effect immediately for every logger — named +// or not, existing or newly created — without rebuilding handlers. +var currentOutput atomic.Value // *outputBox + +// sharedOutput is the single writer every non-explicit handler captures. +// Its Write delegates to whatever currentOutput holds, making SetOutput a +// per-call dispatch decision rather than a capture-at-handler-build one. +type sharedOutput struct{} + +func (sharedOutput) Write(p []byte) (int, error) { + box, _ := currentOutput.Load().(*outputBox) + if box == nil || box.w == nil { + return os.Stderr.Write(p) + } + return box.w.Write(p) +} + +var sharedWriter io.Writer = sharedOutput{} + +func init() { + currentOutput.Store(&outputBox{w: os.Stderr}) +} + +// SetOutput redirects log output for all loggers — root and every named +// logger, existing or future — to w. Pair with a prior GetOutput() + deferred +// SetOutput(old) when you want to restore the previous writer. Safe for +// concurrent callers; the swap is a single atomic store. +func SetOutput(w io.Writer) { + if w == nil { + w = os.Stderr + } + currentOutput.Store(&outputBox{w: w}) +} + +// GetOutput returns the writer SetOutput most recently installed (or os.Stderr +// if none has been set). Returns the writer itself, not the internal +// indirection, so callers can wrap it or save it for restore. +func GetOutput() io.Writer { + if box, ok := currentOutput.Load().(*outputBox); ok && box != nil && box.w != nil { + return box.w + } + return os.Stderr +} + func GetNamedLoggingLevels() (levels map[string]string) { levels = make(map[string]string) namedLoggers.Range(func(key string, value *SlogLogger) bool { @@ -99,7 +152,10 @@ func New(prefix string) *SlogLogger { } namedLevel := properties.String(rootLevel, "log.level."+prefix) - destination := os.Stderr + // Handlers write through sharedWriter (a thin indirection over + // currentOutput) so a later SetOutput retargets every existing logger, + // not just those constructed after the swap. + destination := sharedWriter if logJson { flags.color = false flags.jsonLogs = true diff --git a/logger/slog_setoutput_test.go b/logger/slog_setoutput_test.go new file mode 100644 index 0000000..6ca9bf5 --- /dev/null +++ b/logger/slog_setoutput_test.go @@ -0,0 +1,106 @@ +package logger + +import ( + "bytes" + "io" + "os" + "strings" + "sync" + "testing" +) + +// TestSetOutput_RedirectsNewLogger verifies that a logger constructed after +// SetOutput writes to the new destination. +func TestSetOutput_RedirectsNewLogger(t *testing.T) { + original := GetOutput() + t.Cleanup(func() { SetOutput(original) }) + + var buf bytes.Buffer + SetOutput(&buf) + + lg := New("setoutput-new") + lg.Infof("hello %s", "world") + + if !strings.Contains(buf.String(), "hello world") { + t.Fatalf("expected buffer to contain log line, got %q", buf.String()) + } +} + +// TestSetOutput_RedirectsExistingLogger verifies that a logger constructed +// BEFORE SetOutput still honors the swap on its next write. This is the +// important guarantee — handlers capture the indirect writer, not the +// concrete fd, so a later swap retargets them without rebuilding. +func TestSetOutput_RedirectsExistingLogger(t *testing.T) { + original := GetOutput() + t.Cleanup(func() { SetOutput(original) }) + + lg := New("setoutput-existing") + + var buf bytes.Buffer + SetOutput(&buf) + + lg.Infof("retargeted %d", 42) + + if !strings.Contains(buf.String(), "retargeted 42") { + t.Fatalf("existing logger did not retarget on SetOutput, buf=%q", buf.String()) + } +} + +// TestSetOutput_RestoreSwap verifies that restoring the original writer +// via GetOutput + SetOutput stops sending to the intermediate buffer. +func TestSetOutput_RestoreSwap(t *testing.T) { + original := GetOutput() + t.Cleanup(func() { SetOutput(original) }) + + lg := New("setoutput-restore") + + var buf bytes.Buffer + saved := GetOutput() + SetOutput(&buf) + lg.Infof("recorded") + SetOutput(saved) + lg.Infof("not recorded") + + if !strings.Contains(buf.String(), "recorded") { + t.Fatalf("expected first write in buf, got %q", buf.String()) + } + if strings.Contains(buf.String(), "not recorded") { + t.Fatalf("buf contains post-restore write, swap didn't restore, got %q", buf.String()) + } +} + +// TestSetOutput_NilRestoresStderr verifies that SetOutput(nil) falls back +// to os.Stderr rather than panicking on every log write. +func TestSetOutput_NilRestoresStderr(t *testing.T) { + original := GetOutput() + t.Cleanup(func() { SetOutput(original) }) + + SetOutput(nil) + got := GetOutput() + if got != io.Writer(os.Stderr) { + t.Fatalf("SetOutput(nil) did not reset to os.Stderr, got %T", got) + } +} + +// TestSetOutput_ConcurrentSwaps verifies that a flood of concurrent SetOutput +// + log calls does not race or deadlock. Run with -race. +func TestSetOutput_ConcurrentSwaps(t *testing.T) { + original := GetOutput() + t.Cleanup(func() { SetOutput(original) }) + + lg := New("setoutput-concurrent") + + var wg sync.WaitGroup + for i := 0; i < 8; i++ { + wg.Add(1) + go func() { + defer wg.Done() + for j := 0; j < 100; j++ { + var b bytes.Buffer + SetOutput(&b) + lg.Infof("n=%d", j) + } + }() + } + wg.Wait() +}