From c1090e8babe02e986236f1f05fa4a236e98750b0 Mon Sep 17 00:00:00 2001 From: Nelson Parente Date: Thu, 27 Aug 2026 16:05:37 +0100 Subject: [PATCH 01/11] feat: add --log-file-tee to write logs to both file and console When --log-file is set the logger currently replaces the console writer, so logs stop appearing in kubectl logs. --log-file-tee keeps both destinations via io.MultiWriter. The console writer is listed first because io.MultiWriter stops at the first failed writer: this way console output survives file write failures such as a full disk. Default is false, so existing behaviour is unchanged. Signed-off-by: Nelson Parente --- logger/options.go | 45 +++++++++++++++++++++------- logger/options_test.go | 68 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+), 10 deletions(-) diff --git a/logger/options.go b/logger/options.go index e84d3de..c8409bf 100644 --- a/logger/options.go +++ b/logger/options.go @@ -25,6 +25,7 @@ const ( defaultJSONOutput = false defaultOutputLevel = "info" defaultTimestampFormat = time.RFC3339Nano + defaultOutputFileTee = false undefinedAppID = "" ) @@ -32,6 +33,11 @@ var ( // logOutputMu protects logOutputFile from concurrent access. logOutputMu sync.Mutex logOutputFile *os.File + + // consoleWriter is the console log destination. It is a variable rather + // than a direct os.Stdout reference so that tests can capture console + // output. + consoleWriter io.Writer = os.Stdout ) // Options defines the sets of options for Dapr logging. @@ -52,6 +58,11 @@ type Options struct { // Go time layout. An empty value means the default (RFC3339 with // nanoseconds). TimestampFormat string + + // OutputFileTee, when true and OutputFile is set, writes logs to both the + // file and the console instead of the file only. It has no effect when + // OutputFile is unset. + OutputFileTee bool } // SetOutputLevel sets the log output level. @@ -99,6 +110,11 @@ func (o *Options) AttachCmdFlags( "log-as-json", defaultJSONOutput, "print log as JSON (default false)") + boolVar( + &o.OutputFileTee, + "log-file-tee", + defaultOutputFileTee, + "When --log-file is set, also keep writing logs to the console. No effect without --log-file (default false)") } } @@ -110,6 +126,7 @@ func DefaultOptions() Options { OutputLevel: defaultOutputLevel, OutputFile: "", TimestampFormat: "", + OutputFileTee: defaultOutputFileTee, } } @@ -142,7 +159,7 @@ func ApplyOptionsToLoggers(options *Options) error { v.SetOutputLevel(daprLogLevel) } - err := setLogOutput(options.OutputFile, internalLoggers) + err := setLogOutput(options, internalLoggers) if err != nil { return err } @@ -150,28 +167,36 @@ func ApplyOptionsToLoggers(options *Options) error { return nil } -// setLogOutput configures log output destination. If path is non-empty, logs -// are written to the file at that path. If empty, output reverts to stdout. -// The new file is opened before closing the previous one so that loggers are -// never left pointing at a closed file descriptor. -func setLogOutput(path string, loggers map[string]Logger) error { +// setLogOutput configures log output destination. If options.OutputFile is +// non-empty, logs are written to the file at that path, and additionally to the +// console when options.OutputFileTee is set. If empty, output reverts to the +// console. The new file is opened before closing the previous one so that +// loggers are never left pointing at a closed file descriptor. +func setLogOutput(options *Options, loggers map[string]Logger) error { logOutputMu.Lock() defer logOutputMu.Unlock() var ( - out io.Writer = os.Stdout + out io.Writer = consoleWriter newFile *os.File ) - if path != "" { + if options.OutputFile != "" { var err error - newFile, err = os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644) + newFile, err = os.OpenFile(options.OutputFile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644) if err != nil { - return fmt.Errorf("failed to open log file %q: %w", path, err) + return fmt.Errorf("failed to open log file %q: %w", options.OutputFile, err) } out = newFile + + if options.OutputFileTee { + // Console first: io.MultiWriter stops at the first failed writer, + // so this ordering keeps console output alive even when file + // writes start failing (e.g. disk full). + out = io.MultiWriter(consoleWriter, newFile) + } } // Switch all loggers to the new output before closing the old file. diff --git a/logger/options_test.go b/logger/options_test.go index b4c95ce..60c603b 100644 --- a/logger/options_test.go +++ b/logger/options_test.go @@ -14,6 +14,7 @@ limitations under the License. package logger import ( + "bytes" "os" "path/filepath" "testing" @@ -29,6 +30,7 @@ func TestOptions(t *testing.T) { assert.Equal(t, undefinedAppID, o.appID) assert.Equal(t, defaultOutputLevel, o.OutputLevel) assert.Empty(t, o.OutputFile) + assert.Equal(t, defaultOutputFileTee, o.OutputFileTee) }) t.Run("set dapr ID", func(t *testing.T) { @@ -60,10 +62,15 @@ func TestOptions(t *testing.T) { } logAsJSONAsserted := false + logFileTeeAsserted := false testBoolVarFn := func(p *bool, name string, value bool, usage string) { if name == "log-as-json" && value == defaultJSONOutput { logAsJSONAsserted = true } + + if name == "log-file-tee" && value == defaultOutputFileTee { + logFileTeeAsserted = true + } } o.AttachCmdFlags(testStringVarFn, testBoolVarFn) @@ -73,6 +80,7 @@ func TestOptions(t *testing.T) { assert.True(t, logFileAsserted) assert.True(t, logTimestampFormatAsserted) assert.True(t, logAsJSONAsserted) + assert.True(t, logFileTeeAsserted) }) } @@ -145,6 +153,66 @@ func TestApplyOptionsToLoggersFileOutput(t *testing.T) { assert.Contains(t, string(b), msg) } +func TestLogFileTee(t *testing.T) { + var console bytes.Buffer + + consoleWriter = &console + t.Cleanup(func() { + consoleWriter = os.Stdout + // Re-point all registered loggers back at the real stdout. Doing this + // before restoring consoleWriter would leave them aimed at the dead + // test buffer. + o := DefaultOptions() + require.NoError(t, ApplyOptionsToLoggers(&o)) + }) + + logPath := filepath.Join(t.TempDir(), "dapr.log") + + o := DefaultOptions() + o.OutputFile = logPath + o.OutputFileTee = true + + l := NewLogger("testLoggerTee") + require.NoError(t, ApplyOptionsToLoggers(&o)) + + msg := "hello-tee" + l.Info(msg) + + b, err := os.ReadFile(logPath) + require.NoError(t, err) + assert.Contains(t, string(b), msg, "message should reach the file") + assert.Contains(t, console.String(), msg, "message should also reach the console") +} + +func TestLogFileTeeDisabledKeepsFileOnly(t *testing.T) { + var console bytes.Buffer + + consoleWriter = &console + t.Cleanup(func() { + consoleWriter = os.Stdout + o := DefaultOptions() + require.NoError(t, ApplyOptionsToLoggers(&o)) + }) + + logPath := filepath.Join(t.TempDir(), "dapr.log") + + o := DefaultOptions() + o.OutputFile = logPath + // OutputFileTee deliberately left false — this is the pre-existing + // behaviour and must not change. + + l := NewLogger("testLoggerTeeDisabled") + require.NoError(t, ApplyOptionsToLoggers(&o)) + + msg := "file-only" + l.Info(msg) + + b, err := os.ReadFile(logPath) + require.NoError(t, err) + assert.Contains(t, string(b), msg) + assert.NotContains(t, console.String(), msg, "console must stay silent when tee is off") +} + func TestApplyOptionsToLoggersFileOutputReapply(t *testing.T) { dir := t.TempDir() logPath1 := filepath.Join(dir, "dapr1.log") From 3b65989f40b0e2d8d3aebd9694c7ad65e9c27110 Mon Sep 17 00:00:00 2001 From: Nelson Parente Date: Thu, 27 Aug 2026 16:10:26 +0100 Subject: [PATCH 02/11] feat: add size/age-based rotation for --log-file output --log-file opens the file in append mode and never rolls it, so long-lived components grow it without bound. This adds rotation via lumberjack behind four new flags: --log-file-max-size megabytes before rotation --log-file-max-backups rotated files to keep --log-file-max-age days to retain rotated files --log-file-compress gzip rotated files When none of them is set the writer stays a plain append-mode *os.File, so existing behaviour is byte-for-byte unchanged. The three numeric options are string-typed and parsed at apply time so that AttachCmdFlags keeps its (stringVar, boolVar) signature. That keeps the kit bump non-breaking and surfaces the flags on every binary with no caller changes. Signed-off-by: Nelson Parente --- go.mod | 1 + go.sum | 40 +++--------- logger/options.go | 144 ++++++++++++++++++++++++++++++++++------- logger/options_test.go | 121 ++++++++++++++++++++++++++++++++++ 4 files changed, 249 insertions(+), 57 deletions(-) diff --git a/go.mod b/go.mod index ebb0c43..70e3610 100644 --- a/go.mod +++ b/go.mod @@ -22,6 +22,7 @@ require ( google.golang.org/grpc v1.82.1 google.golang.org/grpc/examples v0.0.0-20250407062114-b368379ef8f6 google.golang.org/protobuf v1.36.11 + gopkg.in/natefinch/lumberjack.v2 v2.2.1 k8s.io/apimachinery v0.26.9 k8s.io/utils v0.0.0-20230726121419-3b25d923346b ) diff --git a/go.sum b/go.sum index 452fa5b..e584318 100644 --- a/go.sum +++ b/go.sum @@ -15,8 +15,6 @@ github.com/frankban/quicktest v1.14.4 h1:g2rn0vABPOOXmZUj+vbmUp0lPoXEMuhTpIluN0X github.com/frankban/quicktest v1.14.4/go.mod h1:4ptaffx2x8+WTWXmUCuVU6aPUX1/Mz7zb5vbUoiM6w0= github.com/fsnotify/fsnotify v1.7.0 h1:8JEhPFa5W2WU7YfeZzPNqzMP6Lwt7L2715Ggo0nosvA= github.com/fsnotify/fsnotify v1.7.0/go.mod h1:40Bi/Hjc2AVfZrqy+aj+yEI+/bRxZnMJyTJwOpGvigM= -github.com/go-jose/go-jose/v4 v4.1.3 h1:CVLmWDhDVRa6Mi/IgCgaopNosCaHz7zrMeF9MlZRkrs= -github.com/go-jose/go-jose/v4 v4.1.3/go.mod h1:x4oUasVrzR7071A4TnHLGSPpNOm2a21K9Kf04k1rs08= github.com/go-jose/go-jose/v4 v4.1.4 h1:moDMcTHmvE6Groj34emNPLs/qtYXRVcd6S7NHbHz3kA= github.com/go-jose/go-jose/v4 v4.1.4/go.mod h1:x4oUasVrzR7071A4TnHLGSPpNOm2a21K9Kf04k1rs08= github.com/go-logr/logr v1.4.3 h1:CjnDlHq8ikf6E492q6eKboGOC0T8CDaOvkHCIg8idEI= @@ -83,86 +81,62 @@ github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9de github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= go.opentelemetry.io/auto/sdk v1.2.1 h1:jXsnJ4Lmnqd11kwkBV2LgLoFMZKizbCi5fNZ/ipaZ64= go.opentelemetry.io/auto/sdk v1.2.1/go.mod h1:KRTj+aOaElaLi+wW1kO/DZRXwkF4C5xPbEe3ZiIhN7Y= -go.opentelemetry.io/otel v1.39.0 h1:8yPrr/S0ND9QEfTfdP9V+SiwT4E0G7Y5MO7p85nis48= -go.opentelemetry.io/otel v1.39.0/go.mod h1:kLlFTywNWrFyEdH0oj2xK0bFYZtHRYUdv1NklR/tgc8= go.opentelemetry.io/otel v1.43.0 h1:mYIM03dnh5zfN7HautFE4ieIig9amkNANT+xcVxAj9I= -go.opentelemetry.io/otel/metric v1.39.0 h1:d1UzonvEZriVfpNKEVmHXbdf909uGTOQjA0HF0Ls5Q0= -go.opentelemetry.io/otel/metric v1.39.0/go.mod h1:jrZSWL33sD7bBxg1xjrqyDjnuzTUB0x1nBERXd7Ftcs= +go.opentelemetry.io/otel v1.43.0/go.mod h1:JuG+u74mvjvcm8vj8pI5XiHy1zDeoCS2LB1spIq7Ay0= go.opentelemetry.io/otel/metric v1.43.0 h1:d7638QeInOnuwOONPp4JAOGfbCEpYb+K6DVWvdxGzgM= -go.opentelemetry.io/otel/sdk v1.39.0 h1:nMLYcjVsvdui1B/4FRkwjzoRVsMK8uL/cj0OyhKzt18= -go.opentelemetry.io/otel/sdk v1.39.0/go.mod h1:vDojkC4/jsTJsE+kh+LXYQlbL8CgrEcwmt1ENZszdJE= +go.opentelemetry.io/otel/metric v1.43.0/go.mod h1:RDnPtIxvqlgO8GRW18W6Z/4P462ldprJtfxHxyKd2PY= go.opentelemetry.io/otel/sdk v1.43.0 h1:pi5mE86i5rTeLXqoF/hhiBtUNcrAGHLKQdhg4h4V9Dg= -go.opentelemetry.io/otel/sdk/metric v1.39.0 h1:cXMVVFVgsIf2YL6QkRF4Urbr/aMInf+2WKg+sEJTtB8= -go.opentelemetry.io/otel/sdk/metric v1.39.0/go.mod h1:xq9HEVH7qeX69/JnwEfp6fVq5wosJsY1mt4lLfYdVew= +go.opentelemetry.io/otel/sdk v1.43.0/go.mod h1:P+IkVU3iWukmiit/Yf9AWvpyRDlUeBaRg6Y+C58QHzg= go.opentelemetry.io/otel/sdk/metric v1.43.0 h1:S88dyqXjJkuBNLeMcVPRFXpRw2fuwdvfCGLEo89fDkw= -go.opentelemetry.io/otel/trace v1.39.0 h1:2d2vfpEDmCJ5zVYz7ijaJdOF59xLomrvj7bjt6/qCJI= -go.opentelemetry.io/otel/trace v1.39.0/go.mod h1:88w4/PnZSazkGzz/w84VHpQafiU4EtqqlVdxWy+rNOA= +go.opentelemetry.io/otel/sdk/metric v1.43.0/go.mod h1:C/RJtwSEJ5hzTiUz5pXF1kILHStzb9zFlIEe85bhj6A= go.opentelemetry.io/otel/trace v1.43.0 h1:BkNrHpup+4k4w+ZZ86CZoHHEkohws8AY+WTX09nk+3A= +go.opentelemetry.io/otel/trace v1.43.0/go.mod h1:/QJhyVBUUswCphDVxq+8mld+AvhXZLhe+8WVFxiFff0= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= -golang.org/x/crypto v0.49.0 h1:+Ng2ULVvLHnJ/ZFEq4KdcDd/cfjrrjjNSXNzxg0Y4U4= -golang.org/x/crypto v0.49.0/go.mod h1:ErX4dUh2UM+CFYiXZRTcMpEcN8b/1gxEuv3nODoYtCA= golang.org/x/crypto v0.54.0 h1:YLIA59K4fiNzHzjnZt2tUJQjQtUWfWbeHBqKtk3eScw= golang.org/x/crypto v0.54.0/go.mod h1:KWL8ny2AZdGR2cWmzeHrp2azQPGogOv+HeQaVEXC2dk= golang.org/x/exp v0.0.0-20231006140011-7918f672742d h1:jtJma62tbqLibJ5sFQz8bKtEM8rJBtfilJ2qTU199MI= golang.org/x/exp v0.0.0-20231006140011-7918f672742d/go.mod h1:ldy0pHrwJyGW56pPQzzkH36rKxoZW1tw7ZJpeKx+hdo= golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= -golang.org/x/mod v0.33.0 h1:tHFzIWbBifEmbwtGz65eaWyGiGZatSrT9prnU8DbVL8= -golang.org/x/mod v0.33.0/go.mod h1:swjeQEj+6r7fODbD2cqrnje9PnziFuw4bmLbBZFrQ5w= golang.org/x/mod v0.37.0 h1:vF1DjpVEshcIqoEaauuHebaLk1O1forxjxBaVn884JQ= golang.org/x/mod v0.37.0/go.mod h1:m8S8VeM9r4dzDwjrKO0a1sZP3YjeMamRRlD+fmR2Q/0= golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20200226121028-0de0cce0169b/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20201021035429-f5854403a974/go.mod h1:sp8m0HH+o8qH0wwXwYZr8TS3Oi6o0r6Gce1SSxlDquU= -golang.org/x/net v0.52.0 h1:He/TN1l0e4mmR3QqHMT2Xab3Aj3L9qjbhRm78/6jrW0= -golang.org/x/net v0.52.0/go.mod h1:R1MAz7uMZxVMualyPXb+VaqGSa3LIaUqk0eEt3w36Sw= golang.org/x/net v0.57.0 h1:K5+3DljvIuDG9/Jv9rvyMywYNFCQ9RSUY6OOTTkT+tE= golang.org/x/net v0.57.0/go.mod h1:KpXc8iv+r3XplLAG/f7Jsf9RPszJzdR0f58q9vGOuEU= golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= -golang.org/x/sync v0.20.0 h1:e0PTpb7pjO8GAtTs2dQ6jYa5BWYlMuX047Dco/pItO4= -golang.org/x/sync v0.20.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= golang.org/x/sync v0.22.0 h1:SZjpbeLmrCk4xhRSZFNZW5gFUeCeFgjekvI/+gfScek= golang.org/x/sync v0.22.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20220715151400-c0bba94af5f8/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.42.0 h1:omrd2nAlyT5ESRdCLYdm3+fMfNFE/+Rf4bDIQImRJeo= -golang.org/x/sys v0.42.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs= golang.org/x/sys v0.47.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= -golang.org/x/text v0.35.0 h1:JOVx6vVDFokkpaq1AEptVzLTpDe9KGpj5tR4/X+ybL8= -golang.org/x/text v0.35.0/go.mod h1:khi/HExzZJ2pGnjenulevKNX1W67CUy0AsXcNubPGCA= golang.org/x/text v0.40.0 h1:Ub2Z6/xjgF1WrYQz2nuITOEegKFtiIy+rieRJ5lHZKs= golang.org/x/text v0.40.0/go.mod h1:hpnzDAfGV753zIKo+wk3u1bVKCGPbrnF7+7LBF/UHVY= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= golang.org/x/tools v0.0.0-20200619180055-7c47624df98f/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= golang.org/x/tools v0.0.0-20210106214847-113979e3529a/go.mod h1:emZCQorbCU4vsT4fOWvOPXz4eW1wZW4PmDk9uLelYpA= -golang.org/x/tools v0.42.0 h1:uNgphsn75Tdz5Ji2q36v/nsFSfR/9BRFvqhGBaJGd5k= -golang.org/x/tools v0.42.0/go.mod h1:Ma6lCIwGZvHK6XtgbswSoWroEkhugApmsXyrUmBhfr0= golang.org/x/tools v0.47.0 h1:7Kn5x/d1svx/PzryTsqeoZN4TZwqeH5pGWjefhLi/1Q= golang.org/x/tools v0.47.0/go.mod h1:dFHnyTvFWY212G+h7ZY4Vsp/K3U4/7W9TyVaAul8uCA= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= -gonum.org/v1/gonum v0.16.0 h1:5+ul4Swaf3ESvrOnidPp4GZbzf0mxVQpDCYUQE7OJfk= -gonum.org/v1/gonum v0.16.0/go.mod h1:fef3am4MQ93R2HHpKnLk4/Tbh/s0+wqD5nfa6Pnwy4E= gonum.org/v1/gonum v0.17.0 h1:VbpOemQlsSMrYmn7T2OUvQ4dqxQXU+ouZFQsZOx50z4= -google.golang.org/genproto/googleapis/rpc v0.0.0-20260316180232-0b37fe3546d5 h1:aJmi6DVGGIStN9Mobk/tZOOQUBbj0BPjZjjnOdoZKts= -google.golang.org/genproto/googleapis/rpc v0.0.0-20260316180232-0b37fe3546d5/go.mod h1:4Hqkh8ycfw05ld/3BWL7rJOSfebL2Q+DVDeRgYgxUU8= +gonum.org/v1/gonum v0.17.0/go.mod h1:El3tOrEuMpv2UdMrbNlKEh9vd86bmQ6vqIcDwxEOc1E= google.golang.org/genproto/googleapis/rpc v0.0.0-20260727163830-6c54dddc4772 h1:zuslGE3FGxH0hC6veLvSLME3TZzun9MQzjYwo1CBN+k= google.golang.org/genproto/googleapis/rpc v0.0.0-20260727163830-6c54dddc4772/go.mod h1:4Hqkh8ycfw05ld/3BWL7rJOSfebL2Q+DVDeRgYgxUU8= -google.golang.org/grpc v1.79.3 h1:sybAEdRIEtvcD68Gx7dmnwjZKlyfuc61Dyo9pGXXkKE= -google.golang.org/grpc v1.79.3/go.mod h1:KmT0Kjez+0dde/v2j9vzwoAScgEPx/Bw1CYChhHLrHQ= google.golang.org/grpc v1.82.1 h1:NnAxzGRA0677vCa4BUkOAnO5+FfQqVl9iUXeD0IqcGE= google.golang.org/grpc v1.82.1/go.mod h1:yzTZ1TB1Z3SG+LIYaI+WiE8D5+PZ3ArnrSp8zF3+/ZA= google.golang.org/grpc/examples v0.0.0-20250407062114-b368379ef8f6 h1:ExN12ndbJ608cboPYflpTny6mXSzPrDLh0iTaVrRrds= @@ -174,6 +148,8 @@ gopkg.in/check.v1 v1.0.0-20200227125254-8fa46927fb4f h1:BLraFXnmrev5lT+xlilqcH8X gopkg.in/check.v1 v1.0.0-20200227125254-8fa46927fb4f/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/inf.v0 v0.9.1 h1:73M5CoZyi3ZLMOyDlQh031Cx6N9NDJ2Vvfl76EDAgDc= gopkg.in/inf.v0 v0.9.1/go.mod h1:cWUDdTG/fYaXco+Dcufb5Vnc6Gp2YChqWtbxRZE0mXw= +gopkg.in/natefinch/lumberjack.v2 v2.2.1 h1:bBRl1b0OH9s/DuPhuXpNl+VtCaJXFZ5/uEFST95x9zc= +gopkg.in/natefinch/lumberjack.v2 v2.2.1/go.mod h1:YD8tP3GAjkrDg1eZH7EGmyESg/lsYskCTPBJVb9jqSc= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/logger/options.go b/logger/options.go index c8409bf..cc8719c 100644 --- a/logger/options.go +++ b/logger/options.go @@ -17,22 +17,26 @@ import ( "fmt" "io" "os" + "strconv" "sync" "time" + + lumberjack "gopkg.in/natefinch/lumberjack.v2" ) const ( - defaultJSONOutput = false - defaultOutputLevel = "info" - defaultTimestampFormat = time.RFC3339Nano - defaultOutputFileTee = false - undefinedAppID = "" + defaultJSONOutput = false + defaultOutputLevel = "info" + defaultTimestampFormat = time.RFC3339Nano + defaultOutputFileTee = false + defaultOutputFileCompress = false + undefinedAppID = "" ) var ( - // logOutputMu protects logOutputFile from concurrent access. - logOutputMu sync.Mutex - logOutputFile *os.File + // logOutputMu protects logOutputCloser from concurrent access. + logOutputMu sync.Mutex + logOutputCloser io.Closer // consoleWriter is the console log destination. It is a variable rather // than a direct os.Stdout reference so that tests can capture console @@ -63,6 +67,21 @@ type Options struct { // file and the console instead of the file only. It has no effect when // OutputFile is unset. OutputFileTee bool + + // OutputFileMaxSize is the maximum size in megabytes of the log file + // before it gets rotated. Empty or "0" disables size-based rotation. + OutputFileMaxSize string + + // OutputFileMaxBackups is the maximum number of rotated log files to keep. + // Empty or "0" keeps all files, subject to OutputFileMaxAge. + OutputFileMaxBackups string + + // OutputFileMaxAge is the maximum number of days to retain rotated log + // files. Empty or "0" disables age-based deletion. + OutputFileMaxAge string + + // OutputFileCompress, when true, gzip-compresses rotated log files. + OutputFileCompress bool } // SetOutputLevel sets the log output level. @@ -102,6 +121,21 @@ func (o *Options) AttachCmdFlags( "log-timestamp-format", "", "Format for log timestamps, expressed as a Go time layout, e.g. '2006/01/02 15:04:05.000' (default RFC3339 with nanoseconds)") + stringVar( + &o.OutputFileMaxSize, + "log-file-max-size", + "", + "Maximum size in megabytes of the log file before it gets rotated. 0 disables size-based rotation. No effect without --log-file") + stringVar( + &o.OutputFileMaxBackups, + "log-file-max-backups", + "", + "Maximum number of rotated log files to keep. 0 keeps all files. No effect without --log-file") + stringVar( + &o.OutputFileMaxAge, + "log-file-max-age", + "", + "Maximum number of days to retain rotated log files. 0 disables age-based deletion. No effect without --log-file") } if boolVar != nil { @@ -115,18 +149,24 @@ func (o *Options) AttachCmdFlags( "log-file-tee", defaultOutputFileTee, "When --log-file is set, also keep writing logs to the console. No effect without --log-file (default false)") + boolVar( + &o.OutputFileCompress, + "log-file-compress", + defaultOutputFileCompress, + "Gzip-compress rotated log files. No effect without --log-file (default false)") } } // DefaultOptions returns default values of Options. func DefaultOptions() Options { return Options{ - JSONFormatEnabled: defaultJSONOutput, - appID: undefinedAppID, - OutputLevel: defaultOutputLevel, - OutputFile: "", - TimestampFormat: "", - OutputFileTee: defaultOutputFileTee, + JSONFormatEnabled: defaultJSONOutput, + appID: undefinedAppID, + OutputLevel: defaultOutputLevel, + OutputFile: "", + TimestampFormat: "", + OutputFileTee: defaultOutputFileTee, + OutputFileCompress: defaultOutputFileCompress, } } @@ -177,25 +217,24 @@ func setLogOutput(options *Options, loggers map[string]Logger) error { defer logOutputMu.Unlock() var ( - out io.Writer = consoleWriter - newFile *os.File + out = consoleWriter + newCloser io.Closer ) if options.OutputFile != "" { - var err error - - newFile, err = os.OpenFile(options.OutputFile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644) + fileOut, closer, err := newFileWriter(options) if err != nil { - return fmt.Errorf("failed to open log file %q: %w", options.OutputFile, err) + return err } - out = newFile + newCloser = closer + out = fileOut if options.OutputFileTee { // Console first: io.MultiWriter stops at the first failed writer, // so this ordering keeps console output alive even when file // writes start failing (e.g. disk full). - out = io.MultiWriter(consoleWriter, newFile) + out = io.MultiWriter(consoleWriter, fileOut) } } @@ -205,11 +244,66 @@ func setLogOutput(options *Options, loggers map[string]Logger) error { } // Close the previous log file after loggers have been redirected. - if logOutputFile != nil { - logOutputFile.Close() + if logOutputCloser != nil { + logOutputCloser.Close() } - logOutputFile = newFile + logOutputCloser = newCloser return nil } + +// newFileWriter returns the file-backed writer for the options: a plain +// append-mode file when no rotation option is set, or a rotating (lumberjack) +// writer when any rotation option is enabled. The returned io.Closer releases +// the underlying file. +func newFileWriter(options *Options) (io.Writer, io.Closer, error) { + maxSize, err := parseRotationValue("log-file-max-size", options.OutputFileMaxSize) + if err != nil { + return nil, nil, err + } + + maxBackups, err := parseRotationValue("log-file-max-backups", options.OutputFileMaxBackups) + if err != nil { + return nil, nil, err + } + + maxAge, err := parseRotationValue("log-file-max-age", options.OutputFileMaxAge) + if err != nil { + return nil, nil, err + } + + if maxSize == 0 && maxBackups == 0 && maxAge == 0 && !options.OutputFileCompress { + f, ferr := os.OpenFile(options.OutputFile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644) + if ferr != nil { + return nil, nil, fmt.Errorf("failed to open log file %q: %w", options.OutputFile, ferr) + } + + return f, f, nil + } + + lj := &lumberjack.Logger{ + Filename: options.OutputFile, + MaxSize: maxSize, // megabytes; lumberjack defaults to 100 when 0 + MaxBackups: maxBackups, // number of rotated files retained + MaxAge: maxAge, // days + Compress: options.OutputFileCompress, + } + + return lj, lj, nil +} + +// parseRotationValue parses a rotation flag value as a non-negative integer. +// An empty value means 0, which disables the corresponding limit. +func parseRotationValue(name, value string) (int, error) { + if value == "" { + return 0, nil + } + + n, err := strconv.Atoi(value) + if err != nil || n < 0 { + return 0, fmt.Errorf("invalid value for --%s: %q (must be a non-negative integer)", name, value) + } + + return n, nil +} diff --git a/logger/options_test.go b/logger/options_test.go index 60c603b..f765bae 100644 --- a/logger/options_test.go +++ b/logger/options_test.go @@ -21,6 +21,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + lumberjack "gopkg.in/natefinch/lumberjack.v2" ) func TestOptions(t *testing.T) { @@ -157,6 +158,7 @@ func TestLogFileTee(t *testing.T) { var console bytes.Buffer consoleWriter = &console + t.Cleanup(func() { consoleWriter = os.Stdout // Re-point all registered loggers back at the real stdout. Doing this @@ -173,6 +175,7 @@ func TestLogFileTee(t *testing.T) { o.OutputFileTee = true l := NewLogger("testLoggerTee") + require.NoError(t, ApplyOptionsToLoggers(&o)) msg := "hello-tee" @@ -188,6 +191,7 @@ func TestLogFileTeeDisabledKeepsFileOnly(t *testing.T) { var console bytes.Buffer consoleWriter = &console + t.Cleanup(func() { consoleWriter = os.Stdout o := DefaultOptions() @@ -202,6 +206,7 @@ func TestLogFileTeeDisabledKeepsFileOnly(t *testing.T) { // behaviour and must not change. l := NewLogger("testLoggerTeeDisabled") + require.NoError(t, ApplyOptionsToLoggers(&o)) msg := "file-only" @@ -213,6 +218,122 @@ func TestLogFileTeeDisabledKeepsFileOnly(t *testing.T) { assert.NotContains(t, console.String(), msg, "console must stay silent when tee is off") } +func TestNewFileWriter(t *testing.T) { + t.Run("rotation options build a rotating writer", func(t *testing.T) { + o := DefaultOptions() + o.OutputFile = filepath.Join(t.TempDir(), "dapr.log") + o.OutputFileMaxSize = "1" + o.OutputFileMaxBackups = "3" + o.OutputFileMaxAge = "7" + o.OutputFileCompress = true + + w, c, err := newFileWriter(&o) + require.NoError(t, err) + + lj, ok := w.(*lumberjack.Logger) + require.True(t, ok, "expected a lumberjack writer") + assert.Equal(t, o.OutputFile, lj.Filename) + assert.Equal(t, 1, lj.MaxSize) + assert.Equal(t, 3, lj.MaxBackups) + assert.Equal(t, 7, lj.MaxAge) + assert.True(t, lj.Compress) + + require.NoError(t, c.Close()) + }) + + t.Run("no rotation options keeps a plain append file", func(t *testing.T) { + o := DefaultOptions() + o.OutputFile = filepath.Join(t.TempDir(), "dapr.log") + + w, c, err := newFileWriter(&o) + require.NoError(t, err) + + _, ok := w.(*os.File) + assert.True(t, ok, "expected a plain *os.File when no rotation option is set") + + require.NoError(t, c.Close()) + }) + + t.Run("compress alone is enough to enable rotation", func(t *testing.T) { + o := DefaultOptions() + o.OutputFile = filepath.Join(t.TempDir(), "dapr.log") + o.OutputFileCompress = true + + w, c, err := newFileWriter(&o) + require.NoError(t, err) + + _, ok := w.(*lumberjack.Logger) + assert.True(t, ok) + + require.NoError(t, c.Close()) + }) + + t.Run("invalid rotation value errors", func(t *testing.T) { + for _, tc := range []struct { + name string + apply func(o *Options) + }{ + {"max-size not a number", func(o *Options) { o.OutputFileMaxSize = "not-a-number" }}, + {"max-backups negative", func(o *Options) { o.OutputFileMaxBackups = "-1" }}, + {"max-age not a number", func(o *Options) { o.OutputFileMaxAge = "7d" }}, + } { + t.Run(tc.name, func(t *testing.T) { + o := DefaultOptions() + o.OutputFile = filepath.Join(t.TempDir(), "dapr.log") + tc.apply(&o) + + _, _, err := newFileWriter(&o) + require.Error(t, err) + }) + } + }) +} + +func TestParseRotationValue(t *testing.T) { + t.Run("empty means disabled", func(t *testing.T) { + got, err := parseRotationValue("log-file-max-size", "") + require.NoError(t, err) + assert.Equal(t, 0, got) + }) + + t.Run("parses a non-negative integer", func(t *testing.T) { + got, err := parseRotationValue("log-file-max-size", "42") + require.NoError(t, err) + assert.Equal(t, 42, got) + }) + + t.Run("rejects negatives and non-numbers", func(t *testing.T) { + for _, v := range []string{"-1", "abc", "1.5", " 1"} { + _, err := parseRotationValue("log-file-max-size", v) + require.Error(t, err, "value %q should be rejected", v) + } + }) +} + +func TestApplyOptionsToLoggersRotation(t *testing.T) { + logPath := filepath.Join(t.TempDir(), "dapr.log") + + o := DefaultOptions() + o.OutputFile = logPath + o.OutputFileMaxSize = "1" + + l := NewLogger("testLoggerRotation") + + require.NoError(t, ApplyOptionsToLoggers(&o)) + + t.Cleanup(func() { + d := DefaultOptions() + require.NoError(t, ApplyOptionsToLoggers(&d)) + }) + + msg := "rotating-message" + l.Info(msg) + + b, err := os.ReadFile(logPath) + require.NoError(t, err) + assert.Contains(t, string(b), msg) +} + func TestApplyOptionsToLoggersFileOutputReapply(t *testing.T) { dir := t.TempDir() logPath1 := filepath.Join(dir, "dapr1.log") From 0f87d2c36d37ef86cf9b9872ab3e8693edd3f325 Mon Sep 17 00:00:00 2001 From: Nelson Parente Date: Thu, 27 Aug 2026 16:16:03 +0100 Subject: [PATCH 03/11] fix(test): call t.TempDir before registering the tee cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit t.Cleanup runs LIFO. The tee tests registered their cleanup before calling t.TempDir, so TempDir's RemoveAll ran first — while the log file was still open — and Windows cannot delete an open file: TempDir RemoveAll cleanup: unlinkat ...\dapr.log: The process cannot access the file because it is being used by another process. Calling t.TempDir first registers its cleanup first, so it runs last, after the cleanup that closes the file. The rotation test already had this ordering, which is why only the two tee tests failed. Signed-off-by: Nelson Parente --- logger/options_test.go | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/logger/options_test.go b/logger/options_test.go index f765bae..79084d6 100644 --- a/logger/options_test.go +++ b/logger/options_test.go @@ -155,21 +155,25 @@ func TestApplyOptionsToLoggersFileOutput(t *testing.T) { } func TestLogFileTee(t *testing.T) { + // t.TempDir() must be called before the cleanup below is registered. + // Cleanups run LIFO, so registering TempDir's RemoveAll first makes it run + // last — after the cleanup that closes the log file. The reverse order + // fails on Windows, which cannot delete a file that is still open. + logPath := filepath.Join(t.TempDir(), "dapr.log") + var console bytes.Buffer consoleWriter = &console t.Cleanup(func() { consoleWriter = os.Stdout - // Re-point all registered loggers back at the real stdout. Doing this - // before restoring consoleWriter would leave them aimed at the dead - // test buffer. + // Re-point all registered loggers back at the real stdout, which also + // closes the log file. Doing this before restoring consoleWriter would + // leave them aimed at the dead test buffer. o := DefaultOptions() require.NoError(t, ApplyOptionsToLoggers(&o)) }) - logPath := filepath.Join(t.TempDir(), "dapr.log") - o := DefaultOptions() o.OutputFile = logPath o.OutputFileTee = true @@ -188,6 +192,9 @@ func TestLogFileTee(t *testing.T) { } func TestLogFileTeeDisabledKeepsFileOnly(t *testing.T) { + // TempDir before Cleanup — see the ordering note in TestLogFileTee. + logPath := filepath.Join(t.TempDir(), "dapr.log") + var console bytes.Buffer consoleWriter = &console @@ -198,8 +205,6 @@ func TestLogFileTeeDisabledKeepsFileOnly(t *testing.T) { require.NoError(t, ApplyOptionsToLoggers(&o)) }) - logPath := filepath.Join(t.TempDir(), "dapr.log") - o := DefaultOptions() o.OutputFile = logPath // OutputFileTee deliberately left false — this is the pre-existing From b8c39a75af5254d67a20c4d537dced2fb59bc5fa Mon Sep 17 00:00:00 2001 From: Nelson Parente Date: Mon, 31 Aug 2026 10:01:57 +0100 Subject: [PATCH 04/11] test: cover rotation behaviour, flag registration and tee+rotation The existing rotation tests asserted only that the lumberjack struct was populated correctly. That would still pass if the rotating writer were never installed as the log output, or if MaxSize were interpreted in the wrong unit, so it verified configuration rather than behaviour. Adds three gaps: - TestFileRotationActuallyRotates drives ~3MB through the configured logger with max-size=1MB and asserts on disk that an archive appeared and the active file was rolled. Only size-based rotation is asserted: lumberjack rotates synchronously on the write that exceeds MaxSize, whereas compression and MaxBackups pruning run on a background goroutine and would make the assertion timing-dependent. - TestOptions/registers_the_exact_set_of_log_flags asserts the full registered flag set rather than spot-checking names. Flag names become D3E chart annotations, so a rename is a breaking change for anyone who has already set them; this makes that fail deliberately rather than silently. - TestTeeWithRotation covers file output that both rotates and tees, which is the combination actually configured in the field. Nothing previously exercised the MultiWriter-wrapping-lumberjack composition. Signed-off-by: Nelson Parente --- logger/options_test.go | 143 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 137 insertions(+), 6 deletions(-) diff --git a/logger/options_test.go b/logger/options_test.go index 79084d6..7c8ed67 100644 --- a/logger/options_test.go +++ b/logger/options_test.go @@ -17,6 +17,7 @@ import ( "bytes" "os" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -63,15 +64,10 @@ func TestOptions(t *testing.T) { } logAsJSONAsserted := false - logFileTeeAsserted := false testBoolVarFn := func(p *bool, name string, value bool, usage string) { if name == "log-as-json" && value == defaultJSONOutput { logAsJSONAsserted = true } - - if name == "log-file-tee" && value == defaultOutputFileTee { - logFileTeeAsserted = true - } } o.AttachCmdFlags(testStringVarFn, testBoolVarFn) @@ -81,7 +77,42 @@ func TestOptions(t *testing.T) { assert.True(t, logFileAsserted) assert.True(t, logTimestampFormatAsserted) assert.True(t, logAsJSONAsserted) - assert.True(t, logFileTeeAsserted) + }) + + // Flag names are load-bearing: they become D3E chart annotations, so a + // rename is a breaking change for users who have already set them. Assert + // the exact registered set rather than spot-checking individual names, so + // that adding, removing or renaming a flag fails here deliberately. + t.Run("registers the exact set of log flags", func(t *testing.T) { + o := DefaultOptions() + + stringFlags := map[string]string{} + boolFlags := map[string]bool{} + + o.AttachCmdFlags( + func(p *string, name string, value string, usage string) { + stringFlags[name] = value + assert.NotEmpty(t, usage, "flag --%s must have usage text", name) + }, + func(p *bool, name string, value bool, usage string) { + boolFlags[name] = value + assert.NotEmpty(t, usage, "flag --%s must have usage text", name) + }, + ) + + assert.Equal(t, map[string]string{ + "log-level": defaultOutputLevel, + "log-file": "", + "log-file-max-size": "", + "log-file-max-backups": "", + "log-file-max-age": "", + }, stringFlags) + + assert.Equal(t, map[string]bool{ + "log-as-json": defaultJSONOutput, + "log-file-tee": defaultOutputFileTee, + "log-file-compress": defaultOutputFileCompress, + }, boolFlags) }) } @@ -339,6 +370,106 @@ func TestApplyOptionsToLoggersRotation(t *testing.T) { assert.Contains(t, string(b), msg) } +// TestFileRotationActuallyRotates is the behavioural counterpart to +// TestNewFileWriter: that test only asserts the lumberjack struct is populated +// correctly, which would still pass if the rotating writer were never actually +// installed as the log output, or if MaxSize were interpreted in the wrong +// unit. This one drives real log volume through the configured logger and +// observes the rotation on disk. +// +// Only size-based rotation is asserted, because lumberjack performs it +// synchronously on the write that would exceed MaxSize. Compression and +// MaxBackups pruning run on a background goroutine ("milling"), so asserting +// on a .gz file appearing would be timing-dependent and flaky in CI. +func TestFileRotationActuallyRotates(t *testing.T) { + dir := t.TempDir() + logPath := filepath.Join(dir, "dapr.log") + + o := DefaultOptions() + o.OutputFile = logPath + o.OutputFileMaxSize = "1" // 1 MB + + l := NewLogger("testLoggerRotationBehaviour") + + require.NoError(t, ApplyOptionsToLoggers(&o)) + + t.Cleanup(func() { + d := DefaultOptions() + require.NoError(t, ApplyOptionsToLoggers(&d)) + }) + + // Write comfortably more than 1 MB. Each line carries a ~2 KB payload, so + // ~1500 lines is roughly 3 MB and forces at least one rotation. + payload := strings.Repeat("x", 2048) + for range 1500 { + l.Info(payload) + } + + entries, err := os.ReadDir(dir) + require.NoError(t, err) + + var active, archives int + + for _, e := range entries { + if e.Name() == "dapr.log" { + active++ + continue + } + // lumberjack names archives dapr-.log[.gz] + if strings.HasPrefix(e.Name(), "dapr-") { + archives++ + } + } + + assert.Equal(t, 1, active, "the active log file should still exist") + assert.Positive(t, archives, + "expected at least one rotated archive after writing ~3MB with max-size=1MB, found none in %v", entries) + + // The active file must have been truncated by the rotation, i.e. it should + // be well under the total volume written. + fi, err := os.Stat(logPath) + require.NoError(t, err) + assert.Less(t, fi.Size(), int64(2*1024*1024), + "active file should have been rolled, not grown past MaxSize unchecked") +} + +// TestTeeWithRotation covers the combination LNRS actually configures: file +// output that both rotates and keeps writing to the console. setLogOutput +// wraps the rotating writer in an io.MultiWriter, and nothing else exercises +// that composition. +func TestTeeWithRotation(t *testing.T) { + dir := t.TempDir() + logPath := filepath.Join(dir, "dapr.log") + + var console bytes.Buffer + + consoleWriter = &console + + t.Cleanup(func() { + consoleWriter = os.Stdout + d := DefaultOptions() + require.NoError(t, ApplyOptionsToLoggers(&d)) + }) + + o := DefaultOptions() + o.OutputFile = logPath + o.OutputFileTee = true + o.OutputFileMaxSize = "1" + o.OutputFileCompress = true + + l := NewLogger("testLoggerTeeRotation") + + require.NoError(t, ApplyOptionsToLoggers(&o)) + + msg := "tee-and-rotate" + l.Info(msg) + + b, err := os.ReadFile(logPath) + require.NoError(t, err) + assert.Contains(t, string(b), msg, "rotating writer should still receive the message") + assert.Contains(t, console.String(), msg, "console should still receive the message when rotation is on") +} + func TestApplyOptionsToLoggersFileOutputReapply(t *testing.T) { dir := t.TempDir() logPath1 := filepath.Join(dir, "dapr1.log") From 6c899438ca74cfb985b723ccd7fbce8b8a14e772 Mon Sep 17 00:00:00 2001 From: Nelson Parente Date: Mon, 31 Aug 2026 11:01:37 +0100 Subject: [PATCH 05/11] feat: warn on inert file options; keep file permissions stable under rotation Two behaviours surfaced by review: 1. Setting --log-file-tee or any rotation flag without --log-file was a silent no-op. Now it logs a warning through a dapr.kit.logger logger. Warn rather than fail: an error here would turn a harmless misconfiguration into a startup failure for every binary that attaches these flags. The logger is fetched at the top of ApplyOptionsToLoggers, before the registry snapshot, so it always follows the configured format, level and output for that apply. 2. lumberjack creates missing log files as 0600 (and preserves the mode of existing ones), where the non-rotating path creates 0644. Enabling rotation would therefore silently change new log file permissions and break log shippers tailing the file as a non-owner user. newFileWriter now pre-creates the file with the same flags and mode as the plain path, so permissions are identical whether or not rotation is enabled; rotated archives and post-rotation files inherit that mode. The permission test compares the plain and rotating paths against each other rather than asserting an absolute mode, so it is immune to umask and Windows permission semantics. Signed-off-by: Nelson Parente --- logger/options.go | 29 +++++++++++++++++ logger/options_test.go | 74 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+) diff --git a/logger/options.go b/logger/options.go index cc8719c..cd0469f 100644 --- a/logger/options.go +++ b/logger/options.go @@ -172,6 +172,12 @@ func DefaultOptions() Options { // ApplyOptionsToLoggers applys options to all registered loggers. func ApplyOptionsToLoggers(options *Options) error { + // optionsLogger reports misconfigurations detected while applying options. + // It is fetched (or created) before the registry snapshot below so that it + // is always part of this apply and therefore follows the configured + // format, level and output like every other logger. + optionsLogger := NewLogger("dapr.kit.logger") + internalLoggers := getLoggers() // Apply formatting options first @@ -204,6 +210,17 @@ func ApplyOptionsToLoggers(options *Options) error { return err } + if options.OutputFile == "" && (options.OutputFileTee || + options.OutputFileCompress || + options.OutputFileMaxSize != "" || + options.OutputFileMaxBackups != "" || + options.OutputFileMaxAge != "") { + // Warn rather than fail: these options are inert without OutputFile, + // and an error here would turn a harmless misconfiguration into a + // startup failure for every binary that attaches these flags. + optionsLogger.Warn("--log-file-tee, --log-file-max-size, --log-file-max-backups, --log-file-max-age and --log-file-compress have no effect because --log-file is not set") + } + return nil } @@ -282,6 +299,18 @@ func newFileWriter(options *Options) (io.Writer, io.Closer, error) { return f, f, nil } + // Pre-create the file with the same permissions as the non-rotating path. + // lumberjack creates missing files as 0600 and preserves the mode of + // existing ones, so without this, enabling rotation would silently change + // new log files from 0644 to 0600 — breaking log shippers that tail the + // file from another container as a non-owner user. + f, ferr := os.OpenFile(options.OutputFile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644) + if ferr != nil { + return nil, nil, fmt.Errorf("failed to open log file %q: %w", options.OutputFile, ferr) + } + + f.Close() + lj := &lumberjack.Logger{ Filename: options.OutputFile, MaxSize: maxSize, // megabytes; lumberjack defaults to 100 when 0 diff --git a/logger/options_test.go b/logger/options_test.go index 7c8ed67..36ab5a9 100644 --- a/logger/options_test.go +++ b/logger/options_test.go @@ -370,6 +370,80 @@ func TestApplyOptionsToLoggersRotation(t *testing.T) { assert.Contains(t, string(b), msg) } +func TestInertFileOptionsWarn(t *testing.T) { + // TempDir before Cleanup — see the ordering note in TestLogFileTee. + logPath := filepath.Join(t.TempDir(), "dapr.log") + + var console bytes.Buffer + + consoleWriter = &console + + t.Cleanup(func() { + consoleWriter = os.Stdout + o := DefaultOptions() + require.NoError(t, ApplyOptionsToLoggers(&o)) + }) + + const warning = "have no effect because --log-file is not set" + + // Tee (or any rotation option) without --log-file warns on the console. + o := DefaultOptions() + o.OutputFileTee = true + require.NoError(t, ApplyOptionsToLoggers(&o)) + assert.Contains(t, console.String(), warning) + + // A default configuration does not warn. + console.Reset() + + o = DefaultOptions() + require.NoError(t, ApplyOptionsToLoggers(&o)) + assert.NotContains(t, console.String(), warning) + + // The same options with --log-file set are effective, so no warning. + console.Reset() + + o = DefaultOptions() + o.OutputFile = logPath + o.OutputFileTee = true + require.NoError(t, ApplyOptionsToLoggers(&o)) + assert.NotContains(t, console.String(), warning) +} + +func TestRotationKeepsFilePermissionParity(t *testing.T) { + dir := t.TempDir() + + plainPath := filepath.Join(dir, "plain.log") + o := DefaultOptions() + o.OutputFile = plainPath + + w, c, err := newFileWriter(&o) + require.NoError(t, err) + _, err = w.Write([]byte("x\n")) + require.NoError(t, err) + require.NoError(t, c.Close()) + + rotPath := filepath.Join(dir, "rotating.log") + o = DefaultOptions() + o.OutputFile = rotPath + o.OutputFileMaxSize = "1" + + w, c, err = newFileWriter(&o) + require.NoError(t, err) + _, err = w.Write([]byte("x\n")) + require.NoError(t, err) + require.NoError(t, c.Close()) + + plainInfo, err := os.Stat(plainPath) + require.NoError(t, err) + rotInfo, err := os.Stat(rotPath) + require.NoError(t, err) + + // Compare the two paths rather than asserting an absolute mode, so the + // test is immune to the process umask and to Windows permission quirks. + assert.Equal(t, plainInfo.Mode(), rotInfo.Mode(), + "enabling rotation must not change log file permissions (lumberjack alone would create 0600 where the plain path creates 0644)") +} + // TestFileRotationActuallyRotates is the behavioural counterpart to // TestNewFileWriter: that test only asserts the lumberjack struct is populated // correctly, which would still pass if the rotating writer were never actually From ef27290eda5069489b12e8ca8e992610b5287995 Mon Sep 17 00:00:00 2001 From: Nelson Parente Date: Tue, 1 Sep 2026 18:21:49 +0100 Subject: [PATCH 06/11] refactor: parse rotation values with ParseUint Review feedback: the rotation values are semantically unsigned, so parse them with strconv.ParseUint (bit size 31 keeps the int conversion safe on 32-bit platforms) instead of Atoi plus a sign check. The struct fields stay string-typed because they bind through AttachCmdFlags(stringVar, boolVar); widening that signature would break every existing caller. Signed-off-by: Nelson Parente --- logger/options.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/logger/options.go b/logger/options.go index cd0469f..aab8342 100644 --- a/logger/options.go +++ b/logger/options.go @@ -322,17 +322,19 @@ func newFileWriter(options *Options) (io.Writer, io.Closer, error) { return lj, lj, nil } -// parseRotationValue parses a rotation flag value as a non-negative integer. -// An empty value means 0, which disables the corresponding limit. +// parseRotationValue parses a rotation flag value as an unsigned integer. An +// empty value means 0, which disables the corresponding limit. The flag is +// string-typed only because AttachCmdFlags binds through (stringVar, boolVar); +// the value itself is unsigned end-to-end. func parseRotationValue(name, value string) (int, error) { if value == "" { return 0, nil } - n, err := strconv.Atoi(value) - if err != nil || n < 0 { + n, err := strconv.ParseUint(value, 10, 31) + if err != nil { return 0, fmt.Errorf("invalid value for --%s: %q (must be a non-negative integer)", name, value) } - return n, nil + return int(n), nil } From 23ea94d5410b3148dca96254e456f3df4b7395a7 Mon Sep 17 00:00:00 2001 From: Nelson Parente Date: Wed, 2 Sep 2026 14:40:33 +0100 Subject: [PATCH 07/11] test: include log-timestamp-format in the exact flag set Rebase onto main after #167 merged: the exact-flag-set test failed as designed on the new flag; register it in the expected set. Signed-off-by: Nelson Parente --- logger/options_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/logger/options_test.go b/logger/options_test.go index 36ab5a9..f9f3cde 100644 --- a/logger/options_test.go +++ b/logger/options_test.go @@ -103,6 +103,7 @@ func TestOptions(t *testing.T) { assert.Equal(t, map[string]string{ "log-level": defaultOutputLevel, "log-file": "", + "log-timestamp-format": "", "log-file-max-size": "", "log-file-max-backups": "", "log-file-max-age": "", From 662b75537a803921b6b75bafffdee4be2dbbcb0a Mon Sep 17 00:00:00 2001 From: Nelson Parente Date: Wed, 2 Sep 2026 15:16:03 +0100 Subject: [PATCH 08/11] refactor: typed optional fields behind separate flag receivers; compression enum MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback: the optional fields are now properly typed and unexported — *uint rotation limits (nil = not provided) and a none|gzip compression enum — with the CLI flags attached to separate unexported string receivers that validate() parses at apply time, mirroring the pattern in dapr/dapr cmd/daprd/options. AttachCmdFlags keeps its (stringVar, boolVar) signature, so no caller changes anywhere. --log-file-compress (bool) becomes --log-file-compression=none|gzip, which leaves room for other codecs without a flag break. Invalid values fail validation before any logger is mutated. Behaviour is unchanged: an explicit 0 still disables the corresponding limit, and with no rotation engaged the writer stays the plain append-mode file. Signed-off-by: Nelson Parente --- logger/options.go | 190 ++++++++++++++++++++++++++--------------- logger/options_test.go | 82 +++++++++++------- 2 files changed, 169 insertions(+), 103 deletions(-) diff --git a/logger/options.go b/logger/options.go index aab8342..22da87e 100644 --- a/logger/options.go +++ b/logger/options.go @@ -25,12 +25,18 @@ import ( ) const ( - defaultJSONOutput = false - defaultOutputLevel = "info" - defaultTimestampFormat = time.RFC3339Nano - defaultOutputFileTee = false - defaultOutputFileCompress = false - undefinedAppID = "" + defaultJSONOutput = false + defaultOutputLevel = "info" + defaultTimestampFormat = time.RFC3339Nano + undefinedAppID = "" +) + +// logFileCompression is the compression applied to rotated log files. +type logFileCompression string + +const ( + compressionNone logFileCompression = "none" + compressionGzip logFileCompression = "gzip" ) var ( @@ -63,25 +69,27 @@ type Options struct { // nanoseconds). TimestampFormat string - // OutputFileTee, when true and OutputFile is set, writes logs to both the + // outputFileTee, when true and OutputFile is set, writes logs to both the // file and the console instead of the file only. It has no effect when // OutputFile is unset. - OutputFileTee bool - - // OutputFileMaxSize is the maximum size in megabytes of the log file - // before it gets rotated. Empty or "0" disables size-based rotation. - OutputFileMaxSize string - - // OutputFileMaxBackups is the maximum number of rotated log files to keep. - // Empty or "0" keeps all files, subject to OutputFileMaxAge. - OutputFileMaxBackups string - - // OutputFileMaxAge is the maximum number of days to retain rotated log - // files. Empty or "0" disables age-based deletion. - OutputFileMaxAge string - - // OutputFileCompress, when true, gzip-compresses rotated log files. - OutputFileCompress bool + outputFileTee bool + + // Typed rotation settings, parsed from the flag receivers below by + // validate(). nil means the flag was not provided; an explicit 0 disables + // the corresponding limit, same as unset. + outputFileMaxSize *uint // megabytes before the log file is rotated + outputFileMaxBackups *uint // rotated files to keep + outputFileMaxAge *uint // days to retain rotated files + outputFileCompression logFileCompression + + // Flag receivers. AttachCmdFlags only binds through (stringVar, boolVar), + // so flags whose real type does not line up with those binders are + // attached to string receivers and parsed into the typed fields above in + // validate() — the same pattern as dapr/dapr cmd/daprd/options. + outputFileMaxSizeStr string + outputFileMaxBackupsStr string + outputFileMaxAgeStr string + outputFileCompressionStr string } // SetOutputLevel sets the log output level. @@ -122,20 +130,25 @@ func (o *Options) AttachCmdFlags( "", "Format for log timestamps, expressed as a Go time layout, e.g. '2006/01/02 15:04:05.000' (default RFC3339 with nanoseconds)") stringVar( - &o.OutputFileMaxSize, + &o.outputFileMaxSizeStr, "log-file-max-size", "", "Maximum size in megabytes of the log file before it gets rotated. 0 disables size-based rotation. No effect without --log-file") stringVar( - &o.OutputFileMaxBackups, + &o.outputFileMaxBackupsStr, "log-file-max-backups", "", "Maximum number of rotated log files to keep. 0 keeps all files. No effect without --log-file") stringVar( - &o.OutputFileMaxAge, + &o.outputFileMaxAgeStr, "log-file-max-age", "", "Maximum number of days to retain rotated log files. 0 disables age-based deletion. No effect without --log-file") + stringVar( + &o.outputFileCompressionStr, + "log-file-compression", + "", + `Compression for rotated log files: "none" or "gzip" (default none). No effect without --log-file`) } if boolVar != nil { @@ -145,28 +158,53 @@ func (o *Options) AttachCmdFlags( defaultJSONOutput, "print log as JSON (default false)") boolVar( - &o.OutputFileTee, + &o.outputFileTee, "log-file-tee", - defaultOutputFileTee, + false, "When --log-file is set, also keep writing logs to the console. No effect without --log-file (default false)") - boolVar( - &o.OutputFileCompress, - "log-file-compress", - defaultOutputFileCompress, - "Gzip-compress rotated log files. No effect without --log-file (default false)") } } +// validate parses the string flag receivers into their typed fields. +func (o *Options) validate() error { + var err error + + o.outputFileMaxSize, err = parseOptionalUint("log-file-max-size", o.outputFileMaxSizeStr) + if err != nil { + return err + } + + o.outputFileMaxBackups, err = parseOptionalUint("log-file-max-backups", o.outputFileMaxBackupsStr) + if err != nil { + return err + } + + o.outputFileMaxAge, err = parseOptionalUint("log-file-max-age", o.outputFileMaxAgeStr) + if err != nil { + return err + } + + switch o.outputFileCompressionStr { + case "", string(compressionNone): + o.outputFileCompression = compressionNone + case string(compressionGzip): + o.outputFileCompression = compressionGzip + default: + return fmt.Errorf("invalid value for --log-file-compression: %q (must be %q or %q)", + o.outputFileCompressionStr, compressionNone, compressionGzip) + } + + return nil +} + // DefaultOptions returns default values of Options. func DefaultOptions() Options { return Options{ - JSONFormatEnabled: defaultJSONOutput, - appID: undefinedAppID, - OutputLevel: defaultOutputLevel, - OutputFile: "", - TimestampFormat: "", - OutputFileTee: defaultOutputFileTee, - OutputFileCompress: defaultOutputFileCompress, + JSONFormatEnabled: defaultJSONOutput, + appID: undefinedAppID, + OutputLevel: defaultOutputLevel, + OutputFile: "", + TimestampFormat: "", } } @@ -178,6 +216,13 @@ func ApplyOptionsToLoggers(options *Options) error { // format, level and output like every other logger. optionsLogger := NewLogger("dapr.kit.logger") + // Parse the string flag receivers into their typed fields before touching + // any logger, so invalid values error out with no partial application. + err := options.validate() + if err != nil { + return err + } + internalLoggers := getLoggers() // Apply formatting options first @@ -205,20 +250,20 @@ func ApplyOptionsToLoggers(options *Options) error { v.SetOutputLevel(daprLogLevel) } - err := setLogOutput(options, internalLoggers) + err = setLogOutput(options, internalLoggers) if err != nil { return err } - if options.OutputFile == "" && (options.OutputFileTee || - options.OutputFileCompress || - options.OutputFileMaxSize != "" || - options.OutputFileMaxBackups != "" || - options.OutputFileMaxAge != "") { + if options.OutputFile == "" && (options.outputFileTee || + options.outputFileCompression == compressionGzip || + options.outputFileMaxSize != nil || + options.outputFileMaxBackups != nil || + options.outputFileMaxAge != nil) { // Warn rather than fail: these options are inert without OutputFile, // and an error here would turn a harmless misconfiguration into a // startup failure for every binary that attaches these flags. - optionsLogger.Warn("--log-file-tee, --log-file-max-size, --log-file-max-backups, --log-file-max-age and --log-file-compress have no effect because --log-file is not set") + optionsLogger.Warn("--log-file-tee, --log-file-max-size, --log-file-max-backups, --log-file-max-age and --log-file-compression have no effect because --log-file is not set") } return nil @@ -226,7 +271,7 @@ func ApplyOptionsToLoggers(options *Options) error { // setLogOutput configures log output destination. If options.OutputFile is // non-empty, logs are written to the file at that path, and additionally to the -// console when options.OutputFileTee is set. If empty, output reverts to the +// console when tee is enabled. If empty, output reverts to the // console. The new file is opened before closing the previous one so that // loggers are never left pointing at a closed file descriptor. func setLogOutput(options *Options, loggers map[string]Logger) error { @@ -247,7 +292,7 @@ func setLogOutput(options *Options, loggers map[string]Logger) error { newCloser = closer out = fileOut - if options.OutputFileTee { + if options.outputFileTee { // Console first: io.MultiWriter stops at the first failed writer, // so this ordering keeps console output alive even when file // writes start failing (e.g. disk full). @@ -275,22 +320,25 @@ func setLogOutput(options *Options, loggers map[string]Logger) error { // writer when any rotation option is enabled. The returned io.Closer releases // the underlying file. func newFileWriter(options *Options) (io.Writer, io.Closer, error) { - maxSize, err := parseRotationValue("log-file-max-size", options.OutputFileMaxSize) - if err != nil { - return nil, nil, err + var maxSize, maxBackups, maxAge uint + + if options.outputFileMaxSize != nil { + maxSize = *options.outputFileMaxSize } - maxBackups, err := parseRotationValue("log-file-max-backups", options.OutputFileMaxBackups) - if err != nil { - return nil, nil, err + if options.outputFileMaxBackups != nil { + maxBackups = *options.outputFileMaxBackups } - maxAge, err := parseRotationValue("log-file-max-age", options.OutputFileMaxAge) - if err != nil { - return nil, nil, err + if options.outputFileMaxAge != nil { + maxAge = *options.outputFileMaxAge } - if maxSize == 0 && maxBackups == 0 && maxAge == 0 && !options.OutputFileCompress { + // An explicit 0 disables the corresponding limit, so rotation is only + // engaged when a limit is non-zero or compression is requested — the + // plain append-mode file path stays byte-for-byte the pre-existing + // behaviour. + if maxSize == 0 && maxBackups == 0 && maxAge == 0 && options.outputFileCompression != compressionGzip { f, ferr := os.OpenFile(options.OutputFile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644) if ferr != nil { return nil, nil, fmt.Errorf("failed to open log file %q: %w", options.OutputFile, ferr) @@ -313,28 +361,28 @@ func newFileWriter(options *Options) (io.Writer, io.Closer, error) { lj := &lumberjack.Logger{ Filename: options.OutputFile, - MaxSize: maxSize, // megabytes; lumberjack defaults to 100 when 0 - MaxBackups: maxBackups, // number of rotated files retained - MaxAge: maxAge, // days - Compress: options.OutputFileCompress, + MaxSize: int(maxSize), // megabytes; lumberjack defaults to 100 when 0 + MaxBackups: int(maxBackups), // number of rotated files retained + MaxAge: int(maxAge), // days + Compress: options.outputFileCompression == compressionGzip, } return lj, lj, nil } -// parseRotationValue parses a rotation flag value as an unsigned integer. An -// empty value means 0, which disables the corresponding limit. The flag is -// string-typed only because AttachCmdFlags binds through (stringVar, boolVar); -// the value itself is unsigned end-to-end. -func parseRotationValue(name, value string) (int, error) { +// parseOptionalUint parses an optional unsigned-integer flag value. An empty +// value returns nil, meaning the flag was not provided. +func parseOptionalUint(name, value string) (*uint, error) { if value == "" { - return 0, nil + return nil, nil } n, err := strconv.ParseUint(value, 10, 31) if err != nil { - return 0, fmt.Errorf("invalid value for --%s: %q (must be a non-negative integer)", name, value) + return nil, fmt.Errorf("invalid value for --%s: %q (must be a non-negative integer)", name, value) } - return int(n), nil + u := uint(n) + + return &u, nil } diff --git a/logger/options_test.go b/logger/options_test.go index f9f3cde..fa2ebc5 100644 --- a/logger/options_test.go +++ b/logger/options_test.go @@ -32,7 +32,6 @@ func TestOptions(t *testing.T) { assert.Equal(t, undefinedAppID, o.appID) assert.Equal(t, defaultOutputLevel, o.OutputLevel) assert.Empty(t, o.OutputFile) - assert.Equal(t, defaultOutputFileTee, o.OutputFileTee) }) t.Run("set dapr ID", func(t *testing.T) { @@ -107,12 +106,12 @@ func TestOptions(t *testing.T) { "log-file-max-size": "", "log-file-max-backups": "", "log-file-max-age": "", + "log-file-compression": "", }, stringFlags) assert.Equal(t, map[string]bool{ - "log-as-json": defaultJSONOutput, - "log-file-tee": defaultOutputFileTee, - "log-file-compress": defaultOutputFileCompress, + "log-as-json": defaultJSONOutput, + "log-file-tee": false, }, boolFlags) }) } @@ -208,7 +207,7 @@ func TestLogFileTee(t *testing.T) { o := DefaultOptions() o.OutputFile = logPath - o.OutputFileTee = true + o.outputFileTee = true l := NewLogger("testLoggerTee") @@ -239,7 +238,7 @@ func TestLogFileTeeDisabledKeepsFileOnly(t *testing.T) { o := DefaultOptions() o.OutputFile = logPath - // OutputFileTee deliberately left false — this is the pre-existing + // outputFileTee deliberately left false — this is the pre-existing // behaviour and must not change. l := NewLogger("testLoggerTeeDisabled") @@ -259,10 +258,13 @@ func TestNewFileWriter(t *testing.T) { t.Run("rotation options build a rotating writer", func(t *testing.T) { o := DefaultOptions() o.OutputFile = filepath.Join(t.TempDir(), "dapr.log") - o.OutputFileMaxSize = "1" - o.OutputFileMaxBackups = "3" - o.OutputFileMaxAge = "7" - o.OutputFileCompress = true + o.outputFileMaxSize = new(uint) + *o.outputFileMaxSize = 1 + o.outputFileMaxBackups = new(uint) + *o.outputFileMaxBackups = 3 + o.outputFileMaxAge = new(uint) + *o.outputFileMaxAge = 7 + o.outputFileCompression = compressionGzip w, c, err := newFileWriter(&o) require.NoError(t, err) @@ -294,7 +296,7 @@ func TestNewFileWriter(t *testing.T) { t.Run("compress alone is enough to enable rotation", func(t *testing.T) { o := DefaultOptions() o.OutputFile = filepath.Join(t.TempDir(), "dapr.log") - o.OutputFileCompress = true + o.outputFileCompression = compressionGzip w, c, err := newFileWriter(&o) require.NoError(t, err) @@ -305,43 +307,58 @@ func TestNewFileWriter(t *testing.T) { require.NoError(t, c.Close()) }) - t.Run("invalid rotation value errors", func(t *testing.T) { + t.Run("invalid flag values fail validation", func(t *testing.T) { for _, tc := range []struct { name string apply func(o *Options) }{ - {"max-size not a number", func(o *Options) { o.OutputFileMaxSize = "not-a-number" }}, - {"max-backups negative", func(o *Options) { o.OutputFileMaxBackups = "-1" }}, - {"max-age not a number", func(o *Options) { o.OutputFileMaxAge = "7d" }}, + {"max-size not a number", func(o *Options) { o.outputFileMaxSizeStr = "not-a-number" }}, + {"max-backups negative", func(o *Options) { o.outputFileMaxBackupsStr = "-1" }}, + {"max-age not a number", func(o *Options) { o.outputFileMaxAgeStr = "7d" }}, + {"unknown compression", func(o *Options) { o.outputFileCompressionStr = "zstd" }}, } { t.Run(tc.name, func(t *testing.T) { o := DefaultOptions() o.OutputFile = filepath.Join(t.TempDir(), "dapr.log") tc.apply(&o) - _, _, err := newFileWriter(&o) - require.Error(t, err) + require.Error(t, o.validate()) }) } }) + + t.Run("compression values parse", func(t *testing.T) { + for str, want := range map[string]logFileCompression{ + "": compressionNone, + "none": compressionNone, + "gzip": compressionGzip, + } { + o := DefaultOptions() + o.outputFileCompressionStr = str + + require.NoError(t, o.validate()) + assert.Equal(t, want, o.outputFileCompression) + } + }) } -func TestParseRotationValue(t *testing.T) { - t.Run("empty means disabled", func(t *testing.T) { - got, err := parseRotationValue("log-file-max-size", "") +func TestParseOptionalUint(t *testing.T) { + t.Run("empty means not provided", func(t *testing.T) { + got, err := parseOptionalUint("log-file-max-size", "") require.NoError(t, err) - assert.Equal(t, 0, got) + assert.Nil(t, got) }) t.Run("parses a non-negative integer", func(t *testing.T) { - got, err := parseRotationValue("log-file-max-size", "42") + got, err := parseOptionalUint("log-file-max-size", "42") require.NoError(t, err) - assert.Equal(t, 42, got) + require.NotNil(t, got) + assert.Equal(t, uint(42), *got) }) t.Run("rejects negatives and non-numbers", func(t *testing.T) { for _, v := range []string{"-1", "abc", "1.5", " 1"} { - _, err := parseRotationValue("log-file-max-size", v) + _, err := parseOptionalUint("log-file-max-size", v) require.Error(t, err, "value %q should be rejected", v) } }) @@ -352,7 +369,7 @@ func TestApplyOptionsToLoggersRotation(t *testing.T) { o := DefaultOptions() o.OutputFile = logPath - o.OutputFileMaxSize = "1" + o.outputFileMaxSizeStr = "1" l := NewLogger("testLoggerRotation") @@ -389,7 +406,7 @@ func TestInertFileOptionsWarn(t *testing.T) { // Tee (or any rotation option) without --log-file warns on the console. o := DefaultOptions() - o.OutputFileTee = true + o.outputFileTee = true require.NoError(t, ApplyOptionsToLoggers(&o)) assert.Contains(t, console.String(), warning) @@ -405,7 +422,7 @@ func TestInertFileOptionsWarn(t *testing.T) { o = DefaultOptions() o.OutputFile = logPath - o.OutputFileTee = true + o.outputFileTee = true require.NoError(t, ApplyOptionsToLoggers(&o)) assert.NotContains(t, console.String(), warning) } @@ -426,7 +443,8 @@ func TestRotationKeepsFilePermissionParity(t *testing.T) { rotPath := filepath.Join(dir, "rotating.log") o = DefaultOptions() o.OutputFile = rotPath - o.OutputFileMaxSize = "1" + o.outputFileMaxSize = new(uint) + *o.outputFileMaxSize = 1 w, c, err = newFileWriter(&o) require.NoError(t, err) @@ -462,7 +480,7 @@ func TestFileRotationActuallyRotates(t *testing.T) { o := DefaultOptions() o.OutputFile = logPath - o.OutputFileMaxSize = "1" // 1 MB + o.outputFileMaxSizeStr = "1" // 1 MB l := NewLogger("testLoggerRotationBehaviour") @@ -528,9 +546,9 @@ func TestTeeWithRotation(t *testing.T) { o := DefaultOptions() o.OutputFile = logPath - o.OutputFileTee = true - o.OutputFileMaxSize = "1" - o.OutputFileCompress = true + o.outputFileTee = true + o.outputFileMaxSizeStr = "1" + o.outputFileCompressionStr = "gzip" l := NewLogger("testLoggerTeeRotation") From c4915c86a91f81670303c449d55d58b2ebab8171 Mon Sep 17 00:00:00 2001 From: Nelson Parente Date: Thu, 3 Sep 2026 10:09:08 +0100 Subject: [PATCH 09/11] feat: replace --log-file-tee with a --log-outputs destination list Review feedback: rather than a tee bool, log destinations are a list. --log-outputs takes a comma-separated list of "stdout", "stderr", or file paths; --log-file merges into that list, so both flags compose and existing behaviour is unchanged. The previous tee semantics are expressed as --log-outputs=stdout,/path/to/file. Destinations are deduplicated (two writers on one path would double every line and corrupt rotation) and console destinations order before files, so io.MultiWriter keeps console output alive when file writes start failing. Rotation and compression options apply to every file destination. The inert-option warning now keys on "no file destination configured" rather than --log-file specifically. Signed-off-by: Nelson Parente --- logger/options.go | 186 +++++++++++++++++++++++++++++++---------- logger/options_test.go | 137 +++++++++++++++++++++++++----- 2 files changed, 260 insertions(+), 63 deletions(-) diff --git a/logger/options.go b/logger/options.go index 22da87e..f045e96 100644 --- a/logger/options.go +++ b/logger/options.go @@ -14,10 +14,13 @@ limitations under the License. package logger import ( + "errors" "fmt" "io" "os" + "sort" "strconv" + "strings" "sync" "time" @@ -44,10 +47,17 @@ var ( logOutputMu sync.Mutex logOutputCloser io.Closer - // consoleWriter is the console log destination. It is a variable rather - // than a direct os.Stdout reference so that tests can capture console - // output. + // consoleWriter and stderrWriter are the console log destinations. They + // are variables rather than direct os.Stdout/os.Stderr references so that + // tests can capture console output. consoleWriter io.Writer = os.Stdout + stderrWriter io.Writer = os.Stderr +) + +// Console destination names accepted in --log-outputs. +const ( + destStdout = "stdout" + destStderr = "stderr" ) // Options defines the sets of options for Dapr logging. @@ -69,10 +79,12 @@ type Options struct { // nanoseconds). TimestampFormat string - // outputFileTee, when true and OutputFile is set, writes logs to both the - // file and the console instead of the file only. It has no effect when - // OutputFile is unset. - outputFileTee bool + // outputDestinations is the resolved, deduplicated list of log + // destinations, parsed by validate() from the --log-outputs receiver + // below merged with OutputFile. Console destinations sort before files so + // that io.MultiWriter keeps console output alive when file writes start + // failing (e.g. disk full). Empty means the console default. + outputDestinations []string // Typed rotation settings, parsed from the flag receivers below by // validate(). nil means the flag was not provided; an explicit 0 disables @@ -86,6 +98,7 @@ type Options struct { // so flags whose real type does not line up with those binders are // attached to string receivers and parsed into the typed fields above in // validate() — the same pattern as dapr/dapr cmd/daprd/options. + outputsStr string outputFileMaxSizeStr string outputFileMaxBackupsStr string outputFileMaxAgeStr string @@ -148,7 +161,12 @@ func (o *Options) AttachCmdFlags( &o.outputFileCompressionStr, "log-file-compression", "", - `Compression for rotated log files: "none" or "gzip" (default none). No effect without --log-file`) + `Compression for rotated log files: "none" or "gzip" (default none). No effect without a file destination`) + stringVar( + &o.outputsStr, + "log-outputs", + "", + `Comma-separated list of log destinations: "stdout", "stderr", or a file path (default stdout). Merged with --log-file when both are set`) } if boolVar != nil { @@ -157,11 +175,6 @@ func (o *Options) AttachCmdFlags( "log-as-json", defaultJSONOutput, "print log as JSON (default false)") - boolVar( - &o.outputFileTee, - "log-file-tee", - false, - "When --log-file is set, also keep writing logs to the console. No effect without --log-file (default false)") } } @@ -184,6 +197,40 @@ func (o *Options) validate() error { return err } + seen := make(map[string]struct{}) + o.outputDestinations = nil + + addDest := func(entry string) { + entry = strings.TrimSpace(entry) + if entry == "" { + return + } + + if _, ok := seen[entry]; ok { + return + } + + seen[entry] = struct{}{} + o.outputDestinations = append(o.outputDestinations, entry) + } + + if o.outputsStr != "" { + for entry := range strings.SplitSeq(o.outputsStr, ",") { + addDest(entry) + } + } + + if o.OutputFile != "" { + addDest(o.OutputFile) + } + + // Console destinations first: io.MultiWriter stops at the first failed + // writer, so this ordering keeps console output alive even when file + // writes start failing (e.g. disk full). + sort.SliceStable(o.outputDestinations, func(i, j int) bool { + return isConsoleDestination(o.outputDestinations[i]) && !isConsoleDestination(o.outputDestinations[j]) + }) + switch o.outputFileCompressionStr { case "", string(compressionNone): o.outputFileCompression = compressionNone @@ -255,24 +302,23 @@ func ApplyOptionsToLoggers(options *Options) error { return err } - if options.OutputFile == "" && (options.outputFileTee || - options.outputFileCompression == compressionGzip || + if !options.hasFileDestination() && (options.outputFileCompression == compressionGzip || options.outputFileMaxSize != nil || options.outputFileMaxBackups != nil || options.outputFileMaxAge != nil) { - // Warn rather than fail: these options are inert without OutputFile, - // and an error here would turn a harmless misconfiguration into a - // startup failure for every binary that attaches these flags. - optionsLogger.Warn("--log-file-tee, --log-file-max-size, --log-file-max-backups, --log-file-max-age and --log-file-compression have no effect because --log-file is not set") + // Warn rather than fail: these options are inert without a file + // destination, and an error here would turn a harmless + // misconfiguration into a startup failure for every binary that + // attaches these flags. + optionsLogger.Warn("--log-file-max-size, --log-file-max-backups, --log-file-max-age and --log-file-compression have no effect because no file destination is configured (--log-file or --log-outputs)") } return nil } -// setLogOutput configures log output destination. If options.OutputFile is -// non-empty, logs are written to the file at that path, and additionally to the -// console when tee is enabled. If empty, output reverts to the -// console. The new file is opened before closing the previous one so that +// setLogOutput points every logger at the configured destinations: the +// console by default, or the resolved --log-outputs / --log-file destination +// list. New files are opened before the previous ones are closed so that // loggers are never left pointing at a closed file descriptor. func setLogOutput(options *Options, loggers map[string]Logger) error { logOutputMu.Lock() @@ -283,20 +329,39 @@ func setLogOutput(options *Options, loggers map[string]Logger) error { newCloser io.Closer ) - if options.OutputFile != "" { - fileOut, closer, err := newFileWriter(options) - if err != nil { - return err + if len(options.outputDestinations) > 0 { + writers := make([]io.Writer, 0, len(options.outputDestinations)) + + var closers multiCloser + + for _, dest := range options.outputDestinations { + switch dest { + case destStdout: + writers = append(writers, consoleWriter) + case destStderr: + writers = append(writers, stderrWriter) + default: + fileOut, closer, err := newFileWriter(dest, options) + if err != nil { + // Release any files already opened for this apply. + closers.Close() + + return err + } + + writers = append(writers, fileOut) + closers = append(closers, closer) + } } - newCloser = closer - out = fileOut + if len(writers) == 1 { + out = writers[0] + } else { + out = io.MultiWriter(writers...) + } - if options.outputFileTee { - // Console first: io.MultiWriter stops at the first failed writer, - // so this ordering keeps console output alive even when file - // writes start failing (e.g. disk full). - out = io.MultiWriter(consoleWriter, fileOut) + if len(closers) > 0 { + newCloser = closers } } @@ -315,11 +380,11 @@ func setLogOutput(options *Options, loggers map[string]Logger) error { return nil } -// newFileWriter returns the file-backed writer for the options: a plain -// append-mode file when no rotation option is set, or a rotating (lumberjack) -// writer when any rotation option is enabled. The returned io.Closer releases -// the underlying file. -func newFileWriter(options *Options) (io.Writer, io.Closer, error) { +// newFileWriter returns the file-backed writer for one destination path: a +// plain append-mode file when no rotation option is set, or a rotating +// (lumberjack) writer when any rotation option is enabled. The returned +// io.Closer releases the underlying file. +func newFileWriter(path string, options *Options) (io.Writer, io.Closer, error) { var maxSize, maxBackups, maxAge uint if options.outputFileMaxSize != nil { @@ -339,9 +404,9 @@ func newFileWriter(options *Options) (io.Writer, io.Closer, error) { // plain append-mode file path stays byte-for-byte the pre-existing // behaviour. if maxSize == 0 && maxBackups == 0 && maxAge == 0 && options.outputFileCompression != compressionGzip { - f, ferr := os.OpenFile(options.OutputFile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644) + f, ferr := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644) if ferr != nil { - return nil, nil, fmt.Errorf("failed to open log file %q: %w", options.OutputFile, ferr) + return nil, nil, fmt.Errorf("failed to open log file %q: %w", path, ferr) } return f, f, nil @@ -352,15 +417,15 @@ func newFileWriter(options *Options) (io.Writer, io.Closer, error) { // existing ones, so without this, enabling rotation would silently change // new log files from 0644 to 0600 — breaking log shippers that tail the // file from another container as a non-owner user. - f, ferr := os.OpenFile(options.OutputFile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644) + f, ferr := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644) if ferr != nil { - return nil, nil, fmt.Errorf("failed to open log file %q: %w", options.OutputFile, ferr) + return nil, nil, fmt.Errorf("failed to open log file %q: %w", path, ferr) } f.Close() lj := &lumberjack.Logger{ - Filename: options.OutputFile, + Filename: path, MaxSize: int(maxSize), // megabytes; lumberjack defaults to 100 when 0 MaxBackups: int(maxBackups), // number of rotated files retained MaxAge: int(maxAge), // days @@ -386,3 +451,36 @@ func parseOptionalUint(name, value string) (*uint, error) { return &u, nil } + +// isConsoleDestination reports whether a --log-outputs entry names a console +// stream rather than a file path. +func isConsoleDestination(dest string) bool { + return dest == destStdout || dest == destStderr +} + +// hasFileDestination reports whether any configured destination is a file. +func (o *Options) hasFileDestination() bool { + for _, dest := range o.outputDestinations { + if !isConsoleDestination(dest) { + return true + } + } + + return false +} + +// multiCloser closes a set of io.Closers, joining any errors. +type multiCloser []io.Closer + +func (m multiCloser) Close() error { + var errs []error + + for _, c := range m { + err := c.Close() + if err != nil { + errs = append(errs, err) + } + } + + return errors.Join(errs...) +} diff --git a/logger/options_test.go b/logger/options_test.go index fa2ebc5..4f87105 100644 --- a/logger/options_test.go +++ b/logger/options_test.go @@ -107,11 +107,11 @@ func TestOptions(t *testing.T) { "log-file-max-backups": "", "log-file-max-age": "", "log-file-compression": "", + "log-outputs": "", }, stringFlags) assert.Equal(t, map[string]bool{ - "log-as-json": defaultJSONOutput, - "log-file-tee": false, + "log-as-json": defaultJSONOutput, }, boolFlags) }) } @@ -206,8 +206,7 @@ func TestLogFileTee(t *testing.T) { }) o := DefaultOptions() - o.OutputFile = logPath - o.outputFileTee = true + o.outputsStr = "stdout," + logPath l := NewLogger("testLoggerTee") @@ -237,9 +236,9 @@ func TestLogFileTeeDisabledKeepsFileOnly(t *testing.T) { }) o := DefaultOptions() - o.OutputFile = logPath - // outputFileTee deliberately left false — this is the pre-existing - // behaviour and must not change. + o.outputsStr = logPath + // No console entry in --log-outputs: the file is the complete + // destination list, so the console must stay silent. l := NewLogger("testLoggerTeeDisabled") @@ -266,7 +265,7 @@ func TestNewFileWriter(t *testing.T) { *o.outputFileMaxAge = 7 o.outputFileCompression = compressionGzip - w, c, err := newFileWriter(&o) + w, c, err := newFileWriter(o.OutputFile, &o) require.NoError(t, err) lj, ok := w.(*lumberjack.Logger) @@ -284,7 +283,7 @@ func TestNewFileWriter(t *testing.T) { o := DefaultOptions() o.OutputFile = filepath.Join(t.TempDir(), "dapr.log") - w, c, err := newFileWriter(&o) + w, c, err := newFileWriter(o.OutputFile, &o) require.NoError(t, err) _, ok := w.(*os.File) @@ -298,7 +297,7 @@ func TestNewFileWriter(t *testing.T) { o.OutputFile = filepath.Join(t.TempDir(), "dapr.log") o.outputFileCompression = compressionGzip - w, c, err := newFileWriter(&o) + w, c, err := newFileWriter(o.OutputFile, &o) require.NoError(t, err) _, ok := w.(*lumberjack.Logger) @@ -402,11 +401,11 @@ func TestInertFileOptionsWarn(t *testing.T) { require.NoError(t, ApplyOptionsToLoggers(&o)) }) - const warning = "have no effect because --log-file is not set" + const warning = "have no effect because no file destination is configured" - // Tee (or any rotation option) without --log-file warns on the console. + // A rotation option without any file destination warns on the console. o := DefaultOptions() - o.outputFileTee = true + o.outputFileMaxSizeStr = "1" require.NoError(t, ApplyOptionsToLoggers(&o)) assert.Contains(t, console.String(), warning) @@ -417,12 +416,12 @@ func TestInertFileOptionsWarn(t *testing.T) { require.NoError(t, ApplyOptionsToLoggers(&o)) assert.NotContains(t, console.String(), warning) - // The same options with --log-file set are effective, so no warning. + // The same options with a file destination are effective, so no warning. console.Reset() o = DefaultOptions() o.OutputFile = logPath - o.outputFileTee = true + o.outputFileMaxSizeStr = "1" require.NoError(t, ApplyOptionsToLoggers(&o)) assert.NotContains(t, console.String(), warning) } @@ -434,7 +433,7 @@ func TestRotationKeepsFilePermissionParity(t *testing.T) { o := DefaultOptions() o.OutputFile = plainPath - w, c, err := newFileWriter(&o) + w, c, err := newFileWriter(o.OutputFile, &o) require.NoError(t, err) _, err = w.Write([]byte("x\n")) require.NoError(t, err) @@ -446,7 +445,7 @@ func TestRotationKeepsFilePermissionParity(t *testing.T) { o.outputFileMaxSize = new(uint) *o.outputFileMaxSize = 1 - w, c, err = newFileWriter(&o) + w, c, err = newFileWriter(o.OutputFile, &o) require.NoError(t, err) _, err = w.Write([]byte("x\n")) require.NoError(t, err) @@ -545,8 +544,7 @@ func TestTeeWithRotation(t *testing.T) { }) o := DefaultOptions() - o.OutputFile = logPath - o.outputFileTee = true + o.outputsStr = "stdout," + logPath o.outputFileMaxSizeStr = "1" o.outputFileCompressionStr = "gzip" @@ -563,6 +561,107 @@ func TestTeeWithRotation(t *testing.T) { assert.Contains(t, console.String(), msg, "console should still receive the message when rotation is on") } +// TestLogOutputsUnionWithLogFile covers both flags together: --log-file merges +// into the destination list rather than being overridden or duplicated. +func TestLogOutputsUnionWithLogFile(t *testing.T) { + logPath := filepath.Join(t.TempDir(), "dapr.log") + + var console bytes.Buffer + + consoleWriter = &console + + t.Cleanup(func() { + consoleWriter = os.Stdout + o := DefaultOptions() + require.NoError(t, ApplyOptionsToLoggers(&o)) + }) + + o := DefaultOptions() + o.OutputFile = logPath + o.outputsStr = "stdout" + + l := NewLogger("testLoggerUnion") + + require.NoError(t, ApplyOptionsToLoggers(&o)) + + msg := "union-msg" + l.Info(msg) + + b, err := os.ReadFile(logPath) + require.NoError(t, err) + assert.Contains(t, string(b), msg, "the --log-file destination should receive the message") + assert.Contains(t, console.String(), msg, "the stdout destination should receive the message") +} + +// TestLogOutputsDeduplicates proves a destination listed twice writes once — +// two writers on one file would double every line, and two lumberjack +// instances on one path would corrupt rotation. +func TestLogOutputsDeduplicates(t *testing.T) { + logPath := filepath.Join(t.TempDir(), "dapr.log") + + o := DefaultOptions() + o.OutputFile = logPath + o.outputsStr = logPath + "," + logPath + + l := NewLogger("testLoggerDedupe") + + require.NoError(t, ApplyOptionsToLoggers(&o)) + + t.Cleanup(func() { + d := DefaultOptions() + require.NoError(t, ApplyOptionsToLoggers(&d)) + }) + + msg := "dedupe-msg" + l.Info(msg) + + b, err := os.ReadFile(logPath) + require.NoError(t, err) + assert.Equal(t, 1, strings.Count(string(b), msg), "a deduplicated destination must receive the message exactly once") +} + +func TestLogOutputsStderr(t *testing.T) { + var stderrBuf bytes.Buffer + + stderrWriter = &stderrBuf + + t.Cleanup(func() { + stderrWriter = os.Stderr + o := DefaultOptions() + require.NoError(t, ApplyOptionsToLoggers(&o)) + }) + + o := DefaultOptions() + o.outputsStr = "stderr" + + l := NewLogger("testLoggerStderr") + + require.NoError(t, ApplyOptionsToLoggers(&o)) + + msg := "stderr-msg" + l.Info(msg) + + assert.Contains(t, stderrBuf.String(), msg) +} + +func TestValidateDestinations(t *testing.T) { + t.Run("consoles sort before files, entries trimmed and deduplicated", func(t *testing.T) { + o := DefaultOptions() + o.OutputFile = "/var/log/a.log" + o.outputsStr = " /var/log/a.log , stdout,, stderr " + + require.NoError(t, o.validate()) + assert.Equal(t, []string{"stdout", "stderr", "/var/log/a.log"}, o.outputDestinations) + }) + + t.Run("empty configuration means no destinations", func(t *testing.T) { + o := DefaultOptions() + + require.NoError(t, o.validate()) + assert.Empty(t, o.outputDestinations) + }) +} + func TestApplyOptionsToLoggersFileOutputReapply(t *testing.T) { dir := t.TempDir() logPath1 := filepath.Join(dir, "dapr1.log") From d0f0631ba298518855cfc6d472b9f3b24b1c0d70 Mon Sep 17 00:00:00 2001 From: Nelson Parente Date: Thu, 3 Sep 2026 10:36:54 +0100 Subject: [PATCH 10/11] fix: align rotation usage strings with the destination model; test the open-failure unwind Pre-merge review findings: - The three rotation usage strings still said "No effect without --log-file", but the actual gating (and the warning) treat a file entry in --log-outputs as an equally valid destination. All four file-option usage strings now say "a file destination". - The mid-list open-failure unwind in setLogOutput (close already-opened files, leave every logger on its previous output) had no test. Added one that applies destinations [stdout, ] and asserts the apply errors while loggers keep writing to their previous output. - File paths are normalized with filepath.Clean before deduplication, so the same file spelled differently (./x.log vs x.log) resolves to a single writer instead of two writers corrupting rotation. Signed-off-by: Nelson Parente --- logger/options.go | 15 ++++++++++++--- logger/options_test.go | 43 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/logger/options.go b/logger/options.go index f045e96..5ffabe2 100644 --- a/logger/options.go +++ b/logger/options.go @@ -18,6 +18,7 @@ import ( "fmt" "io" "os" + "path/filepath" "sort" "strconv" "strings" @@ -146,17 +147,17 @@ func (o *Options) AttachCmdFlags( &o.outputFileMaxSizeStr, "log-file-max-size", "", - "Maximum size in megabytes of the log file before it gets rotated. 0 disables size-based rotation. No effect without --log-file") + "Maximum size in megabytes of the log file before it gets rotated. 0 disables size-based rotation. No effect without a file destination") stringVar( &o.outputFileMaxBackupsStr, "log-file-max-backups", "", - "Maximum number of rotated log files to keep. 0 keeps all files. No effect without --log-file") + "Maximum number of rotated log files to keep. 0 keeps all files. No effect without a file destination") stringVar( &o.outputFileMaxAgeStr, "log-file-max-age", "", - "Maximum number of days to retain rotated log files. 0 disables age-based deletion. No effect without --log-file") + "Maximum number of days to retain rotated log files. 0 disables age-based deletion. No effect without a file destination") stringVar( &o.outputFileCompressionStr, "log-file-compression", @@ -206,6 +207,14 @@ func (o *Options) validate() error { return } + if !isConsoleDestination(entry) { + // Normalize file paths so the same file spelled differently + // (./x.log vs x.log) deduplicates to a single writer — two + // writers on one file would double every line and corrupt + // rotation. + entry = filepath.Clean(entry) + } + if _, ok := seen[entry]; ok { return } diff --git a/logger/options_test.go b/logger/options_test.go index 4f87105..8e79d98 100644 --- a/logger/options_test.go +++ b/logger/options_test.go @@ -644,6 +644,40 @@ func TestLogOutputsStderr(t *testing.T) { assert.Contains(t, stderrBuf.String(), msg) } +// TestApplyOptionsDestinationOpenFailure pins the error-unwind path: when a +// destination fails to open mid-list, the apply must fail without redirecting +// any logger away from its previous output. +func TestApplyOptionsDestinationOpenFailure(t *testing.T) { + // A directory cannot be opened O_WRONLY, so it fails as a file destination. + dir := t.TempDir() + + var console bytes.Buffer + + consoleWriter = &console + + t.Cleanup(func() { + consoleWriter = os.Stdout + o := DefaultOptions() + require.NoError(t, ApplyOptionsToLoggers(&o)) + }) + + l := NewLogger("testLoggerOpenFailure") + + // Point loggers at the captured console first, so "unchanged" is + // observable after the failed apply. + o := DefaultOptions() + require.NoError(t, ApplyOptionsToLoggers(&o)) + + bad := DefaultOptions() + bad.outputsStr = "stdout," + dir + require.Error(t, ApplyOptionsToLoggers(&bad)) + + msg := "still-on-previous-output" + l.Info(msg) + assert.Contains(t, console.String(), msg, + "loggers must keep their previous output when a destination fails to open") +} + func TestValidateDestinations(t *testing.T) { t.Run("consoles sort before files, entries trimmed and deduplicated", func(t *testing.T) { o := DefaultOptions() @@ -654,6 +688,15 @@ func TestValidateDestinations(t *testing.T) { assert.Equal(t, []string{"stdout", "stderr", "/var/log/a.log"}, o.outputDestinations) }) + t.Run("path spellings normalize to one destination", func(t *testing.T) { + o := DefaultOptions() + o.OutputFile = "a.log" + o.outputsStr = "./a.log" + + require.NoError(t, o.validate()) + assert.Equal(t, []string{"a.log"}, o.outputDestinations) + }) + t.Run("empty configuration means no destinations", func(t *testing.T) { o := DefaultOptions() From a6f90120da7af4026867066ca6aedd86ab0be7ac Mon Sep 17 00:00:00 2001 From: Nelson Parente Date: Thu, 3 Sep 2026 14:09:46 +0100 Subject: [PATCH 11/11] fix(test): compare destinations against the filepath.Clean form File paths in the destination list pass through filepath.Clean, whose separator differs by OS. The ordering/dedupe test hardcoded the Unix form and failed on Windows (\var\log\a.log vs /var/log/a.log); compare against the cleaned form so the assertion is platform-correct. Signed-off-by: Nelson Parente --- logger/options_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/logger/options_test.go b/logger/options_test.go index 8e79d98..e580f7f 100644 --- a/logger/options_test.go +++ b/logger/options_test.go @@ -685,7 +685,10 @@ func TestValidateDestinations(t *testing.T) { o.outputsStr = " /var/log/a.log , stdout,, stderr " require.NoError(t, o.validate()) - assert.Equal(t, []string{"stdout", "stderr", "/var/log/a.log"}, o.outputDestinations) + // File paths pass through filepath.Clean, whose separator differs by + // OS — compare against the cleaned form so the assertion is + // platform-correct (this is what broke Windows CI). + assert.Equal(t, []string{"stdout", "stderr", filepath.Clean("/var/log/a.log")}, o.outputDestinations) }) t.Run("path spellings normalize to one destination", func(t *testing.T) {