diff --git a/pkg/service/datastore/atomic_write_test.go b/pkg/service/datastore/atomic_write_test.go new file mode 100644 index 0000000..00f81cd --- /dev/null +++ b/pkg/service/datastore/atomic_write_test.go @@ -0,0 +1,52 @@ +package datastore + +import ( + "os" + "path/filepath" + "testing" +) + +// TestAtomicWriteFile_DurableRoundTrip guards the durable write path added for +// #458: content must round-trip, an overwrite must truncate cleanly, and no +// `.tmp` sidecar may be left behind. (The fsync durability itself isn't +// unit-testable without power-loss fault injection; this is the functional +// regression guard so the fsync rework doesn't break writes.) +func TestAtomicWriteFile_DurableRoundTrip(t *testing.T) { + tempDir, err := os.MkdirTemp("", "st-atomic-test-*") + if err != nil { + t.Fatal(err) + } + + t.Cleanup(func() { _ = os.RemoveAll(tempDir) }) + + ds := NewDataStore(tempDir) + + dir := ds.AccountDeviceDir("1234567", "001122334455") + if err := os.MkdirAll(dir, 0755); err != nil { + t.Fatal(err) + } + + path := filepath.Join(dir, "Sources.xml") + + if err := ds.atomicWriteFile(path, []byte("first")); err != nil { + t.Fatalf("atomicWriteFile (create) failed: %v", err) + } + + if got, _ := os.ReadFile(path); string(got) != "first" { + t.Errorf("content after create = %q, want %q", got, "first") + } + + // Overwrite must truncate the previous (longer) content, not leave a tail. + if err := ds.atomicWriteFile(path, []byte("hi")); err != nil { + t.Fatalf("atomicWriteFile (overwrite) failed: %v", err) + } + + if got, _ := os.ReadFile(path); string(got) != "hi" { + t.Errorf("content after overwrite = %q, want %q", got, "hi") + } + + // No leftover temp sidecar. + if _, err := os.Stat(path + ".tmp"); !os.IsNotExist(err) { + t.Errorf("expected no %s.tmp leftover, stat err = %v", path, err) + } +} diff --git a/pkg/service/datastore/datastore.go b/pkg/service/datastore/datastore.go index d2cbe8e..bae1dac 100644 --- a/pkg/service/datastore/datastore.go +++ b/pkg/service/datastore/datastore.go @@ -1210,15 +1210,83 @@ func (ds *DataStore) SavePresets(account, device string, presets []models.Servic return ds.atomicWriteFile(path, append(header, data...)) } +// atomicWriteFile writes data to filename atomically AND durably: it writes a +// temp file, fsyncs it, renames it into place, then fsyncs the parent +// directory. Without the fsyncs an unclean power-cut on a journaling NAND +// filesystem (UBIFS, the speaker's /mnt/nv) can leave the renamed file present +// but 0 bytes — the rename was journalled but the data blocks were never +// flushed. See #458. func (ds *DataStore) atomicWriteFile(filename string, data []byte) error { perm := os.FileMode(0644) tempFile := filename + ".tmp" - if err := ds.rootWriteFile(tempFile, data, perm); err != nil { + if err := ds.rootWriteFileSync(tempFile, data, perm); err != nil { return err } - return ds.rootRename(tempFile, filename) + if err := ds.rootRename(tempFile, filename); err != nil { + return err + } + + // Fsync the parent directory so the rename itself survives a power-cut. + // Best-effort: not every filesystem permits directory fsync, and the data + + // rename have already succeeded by this point. + ds.rootSyncDir(filepath.Dir(filename)) + + return nil +} + +// rootWriteFileSync writes data to absPath (truncating any existing file) and +// fsyncs the file before returning, so the contents are on stable storage. This +// is the durable equivalent of rootWriteFile. +func (ds *DataStore) rootWriteFileSync(absPath string, data []byte, perm os.FileMode) error { + r, err := ds.getRoot() + if err != nil { + return err + } + + rel, err := ds.rootRel(absPath) + if err != nil { + return err + } + + f, err := r.OpenFile(rel, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, perm) + if err != nil { + return err + } + + if _, werr := f.Write(data); werr != nil { + _ = f.Close() + + return werr + } + + if serr := f.Sync(); serr != nil { + _ = f.Close() + + return serr + } + + return f.Close() +} + +// rootSyncDir fsyncs the directory at absDir so a preceding create/rename is +// durable. Best-effort: directory fsync isn't supported on every filesystem, so +// failures are logged and swallowed rather than failing an already-successful +// write. +func (ds *DataStore) rootSyncDir(absDir string) { + d, err := ds.rootOpen(absDir) + if err != nil { + log.Printf("[Datastore] rootSyncDir: open %s failed (best-effort): %v", sanitizeLog(absDir), err) + + return + } + + if serr := d.Sync(); serr != nil { + log.Printf("[Datastore] rootSyncDir: fsync %s failed (best-effort): %v", sanitizeLog(absDir), serr) + } + + _ = d.Close() } // GetRecents returns the list of recently played items for the specified account and device.