diff --git a/modules/diary/daily.go b/modules/diary/daily.go index 20f73019..550177e6 100644 --- a/modules/diary/daily.go +++ b/modules/diary/daily.go @@ -40,6 +40,10 @@ type dailyWriter struct { retryAt time.Time // warnedDate is the last date a failure was reported for. warnedDate string + // retired is set when a new setup replaced the writer's channels: a + // late record is still appended, but the file is closed again after + // every write, so the writer keeps no descriptor. + retired bool } func newDailyWriter(path string, days int, now func() time.Time, warn func(string, error)) *dailyWriter { @@ -88,6 +92,10 @@ func (d *dailyWriter) Write(p []byte) (int, error) { d.failedDate = "" } n, err := d.file.Write(p) + if d.retired { + _ = d.file.Close() + d.file, d.date = nil, "" + } d.mu.Unlock() return n, err } @@ -155,6 +163,16 @@ func (d *dailyWriter) prune(today string) { } } +// retire closes the open file for good: later writes, from goroutines that +// still hold the replaced channels, open the day's file, append and close +// it again. +func (d *dailyWriter) retire() error { + d.mu.Lock() + d.retired = true + d.mu.Unlock() + return d.Close() +} + // Close closes the open file. A later write opens it again. func (d *dailyWriter) Close() error { d.mu.Lock() diff --git a/modules/diary/diary.go b/modules/diary/diary.go index 938e1996..4fdbbbb3 100644 --- a/modules/diary/diary.go +++ b/modules/diary/diary.go @@ -156,6 +156,12 @@ func Setup(cfg *compass.Config, opts Options) error { func install(conf Config, ch *channels, opts Options) { mu.Lock() defer mu.Unlock() + installLocked(conf, ch, opts) +} + +// installLocked makes ch the installed setup and retires the channels it +// replaces; the caller holds mu. +func installLocked(conf Config, ch *channels, opts Options) { if saved == nil { saved = &savedDefault{logger: slog.Default(), writer: log.Writer(), flags: log.Flags()} } @@ -174,7 +180,7 @@ func install(conf Config, ch *channels, opts Options) { old := current.Swap(st) slog.SetDefault(slog.New(newComponentHandler(""))) if old != nil { - closeChannels(old.channels) + retireChannels(old.channels) } } @@ -186,6 +192,11 @@ func install(conf Config, ch *channels, opts Options) { func SetVerbosity(v int, quiet bool) { mu.Lock() defer mu.Unlock() + setVerbosityLocked(v, quiet) +} + +// setVerbosityLocked is SetVerbosity for a caller that holds mu. +func setVerbosityLocked(v int, quiet bool) { verbosity.Store(int64(v)) st := current.Load() if st == nil { @@ -212,18 +223,21 @@ func FromGlobals(g bonfire.Globals) Options { // the verbosity and quiet switch (SetVerbosity) and, when it differs from // the one the channels were built with, the console color mode, which // rebuilds the channels from the same configuration. The generated main -// registers it with bonfire.OnGlobals. +// registers it with bonfire.OnGlobals. It holds the setup lock for its +// whole run, so concurrent calls rebuild the channels once. func ApplyGlobals(g bonfire.Globals) { + mu.Lock() + defer mu.Unlock() st := current.Load() if st != nil && st.opts.Color != g.Color { opts := st.opts opts.Color, opts.Verbosity, opts.Quiet = g.Color, g.Verbosity, g.Quiet if ch, err := buildChannels(st.conf, opts); err == nil { - install(st.conf, ch, opts) + installLocked(st.conf, ch, opts) return } } - SetVerbosity(g.Verbosity, g.Quiet) + setVerbosityLocked(g.Verbosity, g.Quiet) } type hideOnTTYKey struct{} @@ -264,6 +278,22 @@ func Close() error { return closeChannels(st.channels) } +// retireChannels closes the files of channels a new setup replaced, for +// good: a goroutine that still holds the old state appends its record and +// the file is closed again, so no descriptor stays open. +func retireChannels(ch *channels) { + if ch == nil { + return + } + for _, c := range ch.closers { + if r, ok := c.(interface{ retire() error }); ok { + _ = r.retire() + continue + } + _ = c.Close() + } +} + func closeChannels(ch *channels) error { if ch == nil { return nil @@ -275,9 +305,9 @@ func closeChannels(ch *channels) error { return errors.Join(errs...) } -// Reset drops the installed setup, closes its files and restores the process -// default logger (and the standard log package's output) captured at the -// first Bootstrap or Setup. Tests call it in t.Cleanup. +// Reset drops the installed setup, closes its files for good and restores +// the process default logger (and the standard log package's output) +// captured at the first Bootstrap or Setup. Tests call it in t.Cleanup. func Reset() { mu.Lock() defer mu.Unlock() @@ -290,6 +320,6 @@ func Reset() { saved = nil } if old != nil { - closeChannels(old.channels) + retireChannels(old.channels) } } diff --git a/modules/diary/retire_internal_test.go b/modules/diary/retire_internal_test.go new file mode 100644 index 00000000..bdafa2c8 --- /dev/null +++ b/modules/diary/retire_internal_test.go @@ -0,0 +1,123 @@ +package diary + +import ( + "io" + "os" + "path/filepath" + "sync" + "testing" + "time" + + "git.golem15.com/golem15/summercms/modules/bonfire" +) + +// installDefault builds the default configuration (console and daily) on +// root and installs it, as Setup does. +func installDefault(t *testing.T, root string, color bonfire.ColorMode) *channels { + t.Helper() + opts := Options{ + Root: root, + Stderr: io.Discard, + Environ: []string{}, + Color: color, + Now: func() time.Time { return time.Date(2026, 10, 21, 12, 0, 0, 0, time.Local) }, + }.normalize() + conf := DefaultConfig() + ch, err := buildChannels(conf, opts) + if err != nil { + t.Fatal(err) + } + install(conf, ch, opts) + return ch +} + +// dailyOf returns the daily writer of built channels. +func dailyOf(t *testing.T, ch *channels) *dailyWriter { + t.Helper() + for _, c := range ch.closers { + if d, ok := c.(*dailyWriter); ok { + return d + } + } + t.Fatal("no daily writer in the channels") + return nil +} + +func (d *dailyWriter) hasFile() bool { + d.mu.Lock() + defer d.mu.Unlock() + return d.file != nil +} + +// TestRetiredDailyWriterKeepsNoFile (WR-06): a daily writer replaced by a +// new setup is retired: a late record through it (from a goroutine that +// still holds the old state) is appended to the day's file and the file is +// closed again. diary.Close keeps letting the installed writers reopen. +func TestRetiredDailyWriterKeepsNoFile(t *testing.T) { + t.Cleanup(Reset) + root := t.TempDir() + file := filepath.Join(root, "storage", "logs", "summer-2026-10-21.log") + + old := dailyOf(t, installDefault(t, root, bonfire.ColorNever)) + if _, err := old.Write([]byte("first\n")); err != nil { + t.Fatal(err) + } + installed := dailyOf(t, installDefault(t, root, bonfire.ColorNever)) + if old.hasFile() { + t.Fatal("the replaced writer still holds its file") + } + if _, err := old.Write([]byte("late\n")); err != nil { + t.Fatal(err) + } + if old.hasFile() { + t.Error("a late record through the replaced writer left its file open") + } + + if _, err := installed.Write([]byte("current\n")); err != nil { + t.Fatal(err) + } + if err := Close(); err != nil { + t.Fatal(err) + } + if _, err := installed.Write([]byte("reopened\n")); err != nil { + t.Fatal(err) + } + if !installed.hasFile() { + t.Error("the installed writer did not reopen its file after diary.Close") + } + raw, err := os.ReadFile(file) + if err != nil { + t.Fatal(err) + } + if got := string(raw); got != "first\nlate\ncurrent\nreopened\n" { + t.Errorf("daily file = %q", got) + } +} + +// TestApplyGlobalsRebuildsOnce (WR-06): concurrent ApplyGlobals calls with +// a new color mode rebuild the channels once, and every call's verbosity +// is applied. +func TestApplyGlobalsRebuildsOnce(t *testing.T) { + t.Cleanup(Reset) + installDefault(t, t.TempDir(), bonfire.ColorAuto) + before := gens.Load() + var start, done sync.WaitGroup + start.Add(1) + for range 50 { + done.Add(1) + go func() { + defer done.Done() + start.Wait() + ApplyGlobals(bonfire.Globals{Color: bonfire.ColorNever, Verbosity: 2}) + }() + } + start.Done() + done.Wait() + if n := gens.Load() - before; n != 1 { + t.Errorf("the channels were rebuilt %d times, want 1", n) + } + st := current.Load() + if st.opts.Color != bonfire.ColorNever || st.verbosity != 2 || Verbosity() != 2 { + t.Errorf("state after ApplyGlobals: color %v, verbosity %d (Verbosity %d)", st.opts.Color, st.verbosity, Verbosity()) + } +}