From 92e768f54110bb03f45566fe8ee00234c6d2cda7 Mon Sep 17 00:00:00 2001 From: Aayush7352 <121332319+Aayush7352@users.noreply.github.com> Date: Mon, 6 Jul 2026 01:13:57 +0530 Subject: [PATCH 1/3] fix(server): reopen log file on SIGHUP instead of shutting down - #5721 SIGHUP now reopens the configured log file (standard daemon behavior for log rotation with newsyslog/logrotate) instead of triggering a graceful shutdown. On Windows/Plan 9 SIGHUP still triggers shutdown, preserving console-close semantics. Signed-off-by: Aayush7352 <121332319+Aayush7352@users.noreply.github.com> --- cmd/root.go | 9 +------- cmd/signaller_nounix.go | 6 ++++++ cmd/signaller_unix.go | 42 ++++++++++++++++++++++++++++++-------- conf/configuration.go | 10 ++------- conf/configuration_test.go | 2 +- log/log.go | 36 ++++++++++++++++++++++++++++++++ log/log_test.go | 39 ++++++++++++++++++++++++++++++++++- 7 files changed, 118 insertions(+), 26 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index 08773176a..e0a99fe56 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -2,10 +2,8 @@ package cmd import ( "context" - "os" "os/signal" "strings" - "syscall" "time" "github.com/go-chi/chi/v5/middleware" @@ -103,12 +101,7 @@ func runNavidrome(ctx context.Context) { // mainContext returns a context that is cancelled when the process receives a signal to exit. func mainContext(ctx context.Context) (context.Context, context.CancelFunc) { - return signal.NotifyContext(ctx, - os.Interrupt, - syscall.SIGHUP, - syscall.SIGTERM, - syscall.SIGABRT, - ) + return signal.NotifyContext(ctx, shutdownSignals...) } // startServer starts the Navidrome web server, adding all the necessary routers. diff --git a/cmd/signaller_nounix.go b/cmd/signaller_nounix.go index de488cbd2..67fe241db 100644 --- a/cmd/signaller_nounix.go +++ b/cmd/signaller_nounix.go @@ -4,8 +4,14 @@ package cmd import ( "context" + "os" + "syscall" ) +// SIGHUP is kept as a shutdown signal here, as on Windows it is delivered when the console +// window is closed, and there is no log rotation convention based on it. +var shutdownSignals = []os.Signal{os.Interrupt, syscall.SIGHUP, syscall.SIGTERM, syscall.SIGABRT} + // Windows and Plan9 don't support SIGUSR1, so we don't need to start a signaler func startSignaller(ctx context.Context) func() error { return func() error { diff --git a/cmd/signaller_unix.go b/cmd/signaller_unix.go index f47dbf46a..85308293e 100644 --- a/cmd/signaller_unix.go +++ b/cmd/signaller_unix.go @@ -9,32 +9,58 @@ import ( "syscall" "time" + "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model" ) const triggerScanSignal = syscall.SIGUSR1 +// shutdownSignals does not include SIGHUP: as expected from a daemon, the server handles it +// by reopening its log file (see handleSignal), so log rotation tools can use it. +var shutdownSignals = []os.Signal{os.Interrupt, syscall.SIGTERM, syscall.SIGABRT} + func startSignaller(ctx context.Context) func() error { log.Info(ctx, "Starting signaler") scanner := CreateScanner(ctx) return func() error { var sigChan = make(chan os.Signal, 1) - signal.Notify(sigChan, triggerScanSignal) + signal.Notify(sigChan, triggerScanSignal, syscall.SIGHUP) for { select { case sig := <-sigChan: - log.Info(ctx, "Received signal, triggering a new scan", "signal", sig) - start := time.Now() - _, err := scanner.ScanAll(ctx, false) - if err != nil { - log.Error(ctx, "Error scanning", err) - } - log.Info(ctx, "Triggered scan complete", "elapsed", time.Since(start)) + handleSignal(ctx, sig, scanner) case <-ctx.Done(): return nil } } } } + +func handleSignal(ctx context.Context, sig os.Signal, scanner model.Scanner) { + switch sig { + case syscall.SIGHUP: + reopenLogFile(ctx, sig) + case triggerScanSignal: + log.Info(ctx, "Received signal, triggering a new scan", "signal", sig) + start := time.Now() + _, err := scanner.ScanAll(ctx, false) + if err != nil { + log.Error(ctx, "Error scanning", err) + } + log.Info(ctx, "Triggered scan complete", "elapsed", time.Since(start)) + } +} + +func reopenLogFile(ctx context.Context, sig os.Signal) { + if conf.Server.LogFile == "" { + log.Debug(ctx, "Received signal, but no log file configured. Ignoring", "signal", sig) + return + } + log.Info(ctx, "Received signal, reopening log file", "signal", sig, "logFile", conf.Server.LogFile) + if err := log.SetOutputFile(conf.Server.LogFile); err != nil { + log.Error(ctx, "Error reopening log file", "logFile", conf.Server.LogFile, err) + } +} diff --git a/conf/configuration.go b/conf/configuration.go index 8646bf075..3a6920673 100644 --- a/conf/configuration.go +++ b/conf/configuration.go @@ -367,16 +367,10 @@ func Load(noConfigDump bool) { Server.DbPath = filepath.Join(Server.DataFolder.String(), consts.DefaultDbPath) } - out := os.Stderr if Server.LogFile != "" { - if mkErr := os.MkdirAll(filepath.Dir(Server.LogFile), os.ModePerm); mkErr != nil { - logFatal(fmt.Sprintf("Error creating log file directory: %s", mkErr.Error())) - } - out, err = os.OpenFile(Server.LogFile, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) - if err != nil { + if err := log.SetOutputFile(Server.LogFile); err != nil { logFatal(fmt.Sprintf("Error opening log file %s: %s", Server.LogFile, err.Error())) } - log.SetOutput(out) } else if os.Getenv("ND_SYSTEMD_PRIORITY_LOGGING") != "" && os.Getenv("JOURNAL_STREAM") != "" { // When running under systemd, prepend syslog priority prefixes so // journald assigns the correct severity to each log line. @@ -431,7 +425,7 @@ func Load(noConfigDump bool) { if Server.EnableLogRedacting { prettyConf = log.Redact(prettyConf) } - _, _ = fmt.Fprintln(out, prettyConf) + _, _ = fmt.Fprintln(log.Output(), prettyConf) } if !Server.EnableExternalServices { diff --git a/conf/configuration_test.go b/conf/configuration_test.go index 9c25a0d19..fdbde0fb3 100644 --- a/conf/configuration_test.go +++ b/conf/configuration_test.go @@ -191,7 +191,7 @@ var _ = Describe("Configuration", func() { viper.SetDefault("logfile", filepath.Join(invalidPath, "log.txt")) Expect(func() { conf.Load(true) - }).To(PanicWith(ContainSubstring("Error creating log file directory"))) + }).To(PanicWith(ContainSubstring("creating log file directory"))) }) It("is called when BaseURL is invalid", func() { diff --git a/log/log.go b/log/log.go index eaea75fb9..4cd3e0c79 100644 --- a/log/log.go +++ b/log/log.go @@ -8,6 +8,7 @@ import ( "iter" "net/http" "os" + "path/filepath" "runtime" "sort" "strings" @@ -145,6 +146,41 @@ func SetOutput(w io.Writer) { defaultLogger.SetOutput(w) } +// Output returns the current writer used by the default logger. +func Output() io.Writer { + loggerMu.RLock() + defer loggerMu.RUnlock() + return defaultLogger.Out +} + +var ( + outputFileMu sync.Mutex + outputFile *os.File +) + +// SetOutputFile opens (or creates) the given file in append mode and sets it as the log +// output, closing the previously opened log file, if any. Calling it again with the same +// path reopens the file, allowing external log rotation tools to signal the process +// (e.g. with SIGHUP) after rotating the log file. +func SetOutputFile(path string) error { + if err := os.MkdirAll(filepath.Dir(path), os.ModePerm); err != nil { + return fmt.Errorf("creating log file directory: %w", err) + } + f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) + if err != nil { + return err + } + SetOutput(f) + outputFileMu.Lock() + prev := outputFile + outputFile = f + outputFileMu.Unlock() + if prev != nil { + _ = prev.Close() + } + return nil +} + // EnableJournalFormat wraps the current logger formatter with syslog // priority prefixes for systemd-journald. Only call this when output // goes to stderr and JOURNAL_STREAM is set. diff --git a/log/log_test.go b/log/log_test.go index 7e1f3f3cc..cbce85bb2 100644 --- a/log/log_test.go +++ b/log/log_test.go @@ -4,6 +4,8 @@ import ( "context" "errors" "net/http/httptest" + "os" + "path/filepath" "testing" "time" @@ -92,7 +94,7 @@ var _ = Describe("Logger", func() { SetLogSourceLine(true) Error("A crash happened") // NOTE: This assertion breaks if the line number above changes - Expect(hook.LastEntry().Data[" source"]).To(ContainSubstring("/log/log_test.go:93")) + Expect(hook.LastEntry().Data[" source"]).To(ContainSubstring("/log/log_test.go:95")) Expect(hook.LastEntry().Message).To(Equal("A crash happened")) }) @@ -260,4 +262,39 @@ var _ = Describe("Logger", func() { Expect(Redact(msg)).To(Equal("getLyrics.view?v=1.2.0&c=iSub&u=user_name&p=[REDACTED]&title=Title")) }) }) + + Describe("SetOutputFile", func() { + var path string + + BeforeEach(func() { + path = filepath.Join(GinkgoT().TempDir(), "logs", "navidrome.log") + Expect(SetOutputFile(path)).To(Succeed()) + }) + + It("creates the log file directory and writes log messages to the file", func() { + Error("first message") + + content, err := os.ReadFile(path) + Expect(err).ToNot(HaveOccurred()) + Expect(string(content)).To(ContainSubstring("first message")) + }) + + It("recreates the log file after it has been rotated away", func() { + Error("first message") + rotated := path + ".1" + Expect(os.Rename(path, rotated)).To(Succeed()) + + Expect(SetOutputFile(path)).To(Succeed()) + Error("second message") + + rotatedContent, err := os.ReadFile(rotated) + Expect(err).ToNot(HaveOccurred()) + Expect(string(rotatedContent)).To(ContainSubstring("first message")) + Expect(string(rotatedContent)).ToNot(ContainSubstring("second message")) + + content, err := os.ReadFile(path) + Expect(err).ToNot(HaveOccurred()) + Expect(string(content)).To(ContainSubstring("second message")) + }) + }) }) From 21b363c2bbb40e9cb80dfe02102761e44df9e64a Mon Sep 17 00:00:00 2001 From: Aayush7352 Date: Mon, 6 Jul 2026 01:35:47 +0530 Subject: [PATCH 2/3] fix(server): address review feedback on SIGHUP log reopening - #5721 Hold the output file mutex across the whole open/swap/close sequence in log.SetOutputFile, so a concurrent call cannot close the file the logger is currently writing to. Also deregister the signal channel when the signaller goroutine exits. Signed-off-by: Aayush7352 --- cmd/signaller_unix.go | 1 + log/log.go | 11 +++++------ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/cmd/signaller_unix.go b/cmd/signaller_unix.go index 85308293e..52fb5494f 100644 --- a/cmd/signaller_unix.go +++ b/cmd/signaller_unix.go @@ -27,6 +27,7 @@ func startSignaller(ctx context.Context) func() error { return func() error { var sigChan = make(chan os.Signal, 1) signal.Notify(sigChan, triggerScanSignal, syscall.SIGHUP) + defer signal.Stop(sigChan) for { select { diff --git a/log/log.go b/log/log.go index 4cd3e0c79..7e9d30dda 100644 --- a/log/log.go +++ b/log/log.go @@ -166,18 +166,17 @@ func SetOutputFile(path string) error { if err := os.MkdirAll(filepath.Dir(path), os.ModePerm); err != nil { return fmt.Errorf("creating log file directory: %w", err) } + outputFileMu.Lock() + defer outputFileMu.Unlock() f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) if err != nil { return err } SetOutput(f) - outputFileMu.Lock() - prev := outputFile - outputFile = f - outputFileMu.Unlock() - if prev != nil { - _ = prev.Close() + if outputFile != nil { + _ = outputFile.Close() } + outputFile = f return nil } From 172aeb2a6f38dbf89463599124afb20df8bcfdc5 Mon Sep 17 00:00:00 2001 From: Aayush7352 Date: Fri, 17 Jul 2026 14:17:28 +0530 Subject: [PATCH 3/3] fix(server): close old log file after swap in SetOutputFile Swap outputFile before closing the previous file handle, so no read of outputFile can observe a dangling pointer to a closed file. Also wrap the file-open in the mutex (already done) for atomicity of the entire file rotation sequence. --- log/log.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/log/log.go b/log/log.go index 7e9d30dda..845e3f81a 100644 --- a/log/log.go +++ b/log/log.go @@ -173,10 +173,11 @@ func SetOutputFile(path string) error { return err } SetOutput(f) - if outputFile != nil { - _ = outputFile.Close() - } + prev := outputFile outputFile = f + if prev != nil { + _ = prev.Close() + } return nil }