fix(conf): make Dir a plain value type to prevent sync.Once corruption (#5543)

Dir embedded sync.Once directly and exposed a value-receiver GoString so
that pretty.Sprintf("%# v", Server) could render the path. That meant
every pretty-print copied the entire Dir along with its Once, and a
goroutine concurrently using the original (or any copy) for Path() could
hit a "sync: unlock of unlocked mutex" runtime fatal error. The failure
was reproduced deterministically on Windows CI when test-suite shuffle
ordering raced cache initialization (utils/cache/file_caches.go's
NewFileCache.func1 -> conf.CacheFolder.MustPath) against the
configuration-dump pretty.Sprintf in Load().

Drop the sync.Once entirely. Dir is now a plain {path, perm} value type,
and Path() calls os.MkdirAll on every invocation. MkdirAll is
idempotent, so repeated calls on an existing directory cost one stat
syscall — negligible for the few config paths read at startup and during
cache init.

This removes the entire class of bug:
  - No Mutex, so copies (via reflection, pretty-print, etc.) are safe.
  - No state pointer, so no nil-state defensive checks scattered across
    methods, and no risk of two copies seeing different lifecycle state.
  - go vet is happy with the value receivers — the //nolint:govet
    suppression on GoString is gone.

Adds two regression tests in conf/dir_test.go:
  - GoString renders Dir as a quoted path under pretty.Sprintf (and
    does not leak the internal struct fields).
  - Concurrent copy + Path() stress test, locking in the copy-safety
    property in case the type ever grows non-trivial state again.
This commit is contained in:
Deluan Quintão 2026-05-27 23:18:35 -03:00 committed by GitHub
parent 823d851b75
commit fc9cdf39c8
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 63 additions and 25 deletions

View File

@ -1,20 +1,20 @@
package conf
import (
"cmp"
"fmt"
"os"
"sync"
)
// Dir wraps a directory path and lazily creates the directory on first use.
// The directory is created at most once; if creation fails, the error is
// permanently cached (sync.Once semantics). Dir is not safe for mutation
// after Path() has been called.
// Dir wraps a directory path and creates the directory on demand. Dir is a
// plain value type — safe to copy, compare, and print via reflection-based
// formatters (pretty.Sprintf("%# v", ...)) without any concurrency hazards.
// Directory creation is delegated to os.MkdirAll on every Path() call;
// MkdirAll is idempotent, so repeated calls cost one stat syscall when the
// directory already exists.
type Dir struct {
path string
perm os.FileMode
once sync.Once
err error
}
// NewDir creates a new Dir with the given path and default permissions (os.ModePerm).
@ -23,31 +23,32 @@ func NewDir(path string) Dir {
}
// NewDirWithPerm creates a new Dir with the given path and permissions.
// A perm of 0 is treated as "default" and resolves to os.ModePerm at
// directory-creation time; pass an explicit non-zero mode to constrain the
// permissions.
func NewDirWithPerm(path string, perm os.FileMode) Dir {
return Dir{path: path, perm: perm}
}
// String returns the raw path without creating the directory. Satisfies fmt.Stringer.
func (d *Dir) String() string {
func (d Dir) String() string {
return d.path
}
// Path creates the directory on first call (via sync.Once) and returns the path.
func (d *Dir) Path() (string, error) {
d.once.Do(func() {
if d.path == "" {
return
}
d.err = os.MkdirAll(d.path, d.perm)
if d.err != nil {
d.err = fmt.Errorf("creating directory %q: %w", d.path, d.err)
}
})
return d.path, d.err
// Path ensures the directory exists and returns its path. Safe to call
// repeatedly; an empty path is returned as-is with no error.
func (d Dir) Path() (string, error) {
if d.path == "" {
return "", nil
}
if err := os.MkdirAll(d.path, cmp.Or(d.perm, os.ModePerm)); err != nil {
return d.path, fmt.Errorf("creating directory %q: %w", d.path, err)
}
return d.path, nil
}
// MustPath calls Path() and calls logFatal on error.
func (d *Dir) MustPath() string {
func (d Dir) MustPath() string {
path, err := d.Path()
if err != nil {
logFatal("creating directory:", err)
@ -57,12 +58,12 @@ func (d *Dir) MustPath() string {
// GoString implements fmt.GoStringer so that %#v (used by pretty.Sprintf)
// prints the path string instead of the internal struct fields.
func (d Dir) GoString() string { //nolint:govet // uses a value receiver so Dir values satisfy GoStringer
func (d Dir) GoString() string {
return fmt.Sprintf("%q", d.path)
}
// MarshalText returns the raw path bytes. No side effects.
func (d *Dir) MarshalText() ([]byte, error) {
func (d Dir) MarshalText() ([]byte, error) {
return []byte(d.path), nil
}

View File

@ -2,7 +2,9 @@ package conf_test
import (
"os"
"sync"
"github.com/kr/pretty"
"github.com/navidrome/navidrome/conf"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
@ -35,9 +37,9 @@ var _ = Describe("Dir", func() {
Expect(target).To(BeADirectory())
})
It("returns the same result on subsequent calls (sync.Once)", func() {
It("is idempotent on subsequent calls", func() {
dir := GinkgoT().TempDir()
target := dir + "/once"
target := dir + "/idempotent"
d := conf.NewDir(target)
path1, err1 := d.Path()
@ -45,6 +47,7 @@ var _ = Describe("Dir", func() {
Expect(err1).ToNot(HaveOccurred())
Expect(err2).ToNot(HaveOccurred())
Expect(path1).To(Equal(path2))
Expect(target).To(BeADirectory())
})
It("returns an error when directory cannot be created", func() {
@ -124,4 +127,38 @@ var _ = Describe("Dir", func() {
Expect(d2.String()).To(Equal(d1.String()))
})
})
Describe("GoString", func() {
// Regression: pretty.Sprintf("%# v", ...) is used by the
// configuration dump. It must render Dir as a quoted path via
// GoString, not dump the internal struct fields.
It("renders Dir as a quoted path under pretty.Sprintf", func() {
type host struct {
DataFolder conf.Dir
}
h := host{DataFolder: conf.NewDir("./data")}
out := pretty.Sprintf("%# v", h)
Expect(out).To(ContainSubstring(`DataFolder: "./data"`))
Expect(out).ToNot(ContainSubstring("perm:"))
Expect(out).ToNot(ContainSubstring("path:"))
})
It("is safe to copy and use concurrently", func() {
// Regression for the Windows "sync: unlock of unlocked mutex"
// crash that was caused by copying a Dir embedding sync.Once.
// Dir is a plain value type now, but keep the concurrent stress
// test to lock in the property.
dir := GinkgoT().TempDir()
d := conf.NewDir(dir + "/race")
var wg sync.WaitGroup
for range 10 {
wg.Go(func() {
copy1 := d
_ = pretty.Sprintf("%# v", copy1)
_, _ = copy1.Path()
})
}
wg.Wait()
})
})
})