diff --git a/internal/config/lock.go b/internal/config/lock.go index 0b1d61256..b867ba246 100644 --- a/internal/config/lock.go +++ b/internal/config/lock.go @@ -3,7 +3,6 @@ package config import ( "errors" "fmt" - "os" "path/filepath" "time" @@ -72,10 +71,12 @@ var lockConfigFileFn = lockConfigFile func lockConfigFile(path string) (func() error, error) { lockPath := path + ".lock" - if dir := filepath.Dir(lockPath); dir != "." && dir != "" { - if err := os.MkdirAll(dir, 0o700); err != nil { - return nil, fmt.Errorf("create config directory %s: %w", dir, err) - } + // Every mutator locks before it writes, so this is where the config + // directory is normally created. ensureConfigDir also syncs each new entry + // into its parent before the lock file exists; writeConfigData would find + // nothing missing by then and could not run that barrier itself. + if err := ensureConfigDir(filepath.Dir(lockPath)); err != nil { + return nil, err } deadline := time.Now().Add(configLockTimeout) for { diff --git a/internal/config/writer.go b/internal/config/writer.go index 936425354..ad41592a9 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path/filepath" + "runtime" "sort" "strings" @@ -1114,12 +1115,116 @@ func writeConfigFile(path string, cfg FileConfig) error { return writeConfigData(path, data) } +// Sync seams allow tests to inject persistence failures at each barrier. +var syncConfigFileFn = (*os.File).Sync +var syncConfigDirFn = syncConfigDir + +// Windows cannot sync directory handles, so rename durability is best effort +// there, matching the sessions store. Elsewhere a directory the user can write +// but not read cannot be opened for syncing either; that is also best effort, +// because the write it follows has already succeeded. Every other failure to +// open, sync, or close the directory is reported: the barrier was requested and +// not reached, so the caller must not acknowledge the save. +func syncConfigDir(dir string) error { + if runtime.GOOS == "windows" { + return nil + } + d, err := os.Open(dir) + if err != nil { + if errors.Is(err, os.ErrPermission) { + return nil + } + return err + } + return errors.Join(d.Sync(), d.Close()) +} + +// ensureConfigDir is the ONE path that creates the config directory, shared by +// lockConfigFile and writeConfigData. Every exported mutator locks before it +// writes, and the lock file is a sibling of the config, so in practice the lock +// path is what first creates the directory; a barrier that lived only in +// writeConfigData would find nothing missing by then and never run. +// +// Each missing component is created with os.Mkdir rather than os.MkdirAll so +// the call knows exactly which entries it created, then each new entry is +// synced into its parent (deepest first) so a crash cannot lose the directory, +// and with it the config, after a successful write. A component that appeared +// between the lookup and the Mkdir belongs to another writer: it is not rolled +// back, but its parent is still synced, because this write depends on the +// entry being durable whoever created it. +// +// If a barrier fails, the directories this call created are removed again. +// Leaving them would let a retry find nothing missing, skip the barrier, and +// report a durability it never established. Only what this call created is +// removed, with os.Remove so a directory another process has since populated +// stays; a failed rollback is joined into the error, never swallowed. +func ensureConfigDir(dir string) error { + if dir == "." || dir == "" { + return nil + } + missing := missingConfigDirs(dir) + if len(missing) == 0 { + return nil + } + var created []string + for i := len(missing) - 1; i >= 0; i-- { + d := missing[i] + err := os.Mkdir(d, 0o700) + if errors.Is(err, os.ErrExist) { + continue + } + if err != nil { + return errors.Join(fmt.Errorf("create config directory %s: %w", d, err), removeConfigDirs(created)) + } + created = append(created, d) + } + for _, d := range missing { + parent := filepath.Dir(d) + if err := syncConfigDirFn(parent); err != nil { + return errors.Join(fmt.Errorf("sync config directory %s: %w", parent, err), removeConfigDirs(created)) + } + } + return nil +} + +// removeConfigDirs rolls back directories ensureConfigDir created in this +// call, deepest first. os.Remove refuses a non-empty directory, so an entry +// another process already populated survives and its error is reported. +func removeConfigDirs(created []string) error { + var errs []error + for i := len(created) - 1; i >= 0; i-- { + if err := os.Remove(created[i]); err != nil { + errs = append(errs, fmt.Errorf("roll back config directory %s: %w", created[i], err)) + } + } + return errors.Join(errs...) +} + +// missingConfigDirs lists dir and each ancestor that does not exist yet, +// deepest first, so ensureConfigDir can create them shallowest first and sync +// each new entry into its parent deepest first. +func missingConfigDirs(dir string) []string { + var missing []string + for d := dir; ; { + if _, err := os.Lstat(d); !errors.Is(err, os.ErrNotExist) { + return missing + } + missing = append(missing, d) + parent := filepath.Dir(d) + if parent == d { + return missing + } + d = parent + } +} + func writeConfigData(path string, data []byte) error { dir := filepath.Dir(path) - if dir != "." && dir != "" { - if err := os.MkdirAll(dir, 0o700); err != nil { - return fmt.Errorf("create config directory %s: %w", dir, err) - } + // Usually a no-op: the lock path has already created and synced the + // directory. It still matters for the rare unlocked caller and for a + // directory removed between the lock and the write. + if err := ensureConfigDir(dir); err != nil { + return err } if len(data) == 0 || data[len(data)-1] != '\n' { data = append(data, '\n') @@ -1140,11 +1245,19 @@ func writeConfigData(path string, data []byte) error { _ = tmp.Close() return fmt.Errorf("write config %s: %w", path, err) } + // Persist the complete contents before making the replacement visible. + if err := syncConfigFileFn(tmp); err != nil { + return fmt.Errorf("sync config %s: %w", path, errors.Join(err, tmp.Close())) + } if err := tmp.Close(); err != nil { return fmt.Errorf("write config %s: %w", path, err) } if err := os.Rename(tmpPath, path); err != nil { return fmt.Errorf("write config %s: %w", path, err) } + // The replacement is already visible if this fails; do not roll it back. + if err := syncConfigDirFn(dir); err != nil { + return fmt.Errorf("sync config directory %s: %w", dir, err) + } return nil } diff --git a/internal/config/writer_test.go b/internal/config/writer_test.go index 046753fe7..6fa9d037f 100644 --- a/internal/config/writer_test.go +++ b/internal/config/writer_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "io/fs" "os" "os/exec" @@ -15,6 +16,295 @@ import ( "testing" ) +func TestWriteConfigDataSync(t *testing.T) { + failure := errors.New("injected sync failure") + for _, stage := range []string{"success", "file", "directory"} { + t.Run(stage, func(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.json") + oldData, newData := "{\"old\":true}\n", "{\"new\":true}\n" + if err := os.WriteFile(path, []byte(oldData), 0o600); err != nil { + t.Fatal(err) + } + checkData := func(path, want string) { + t.Helper() + got, err := os.ReadFile(path) + if err != nil || string(got) != want { + t.Fatalf("read %s = %q, %v; want %q", path, got, err, want) + } + } + fileSync, dirSync := syncConfigFileFn, syncConfigDirFn + t.Cleanup(func() { syncConfigFileFn, syncConfigDirFn = fileSync, dirSync }) + var tmp *os.File + var calls []string + syncConfigFileFn = func(f *os.File) error { + tmp = f + calls = append(calls, "file") + checkData(f.Name(), newData) + checkData(path, oldData) + if stage == "file" { + return failure + } + return fileSync(f) + } + syncConfigDirFn = func(got string) error { + calls = append(calls, "directory") + if got != dir || tmp == nil { + t.Fatalf("directory sync = %q, temp = %v", got, tmp) + } + if _, err := tmp.Seek(0, io.SeekCurrent); !errors.Is(err, os.ErrClosed) { + t.Fatalf("temp must be closed before directory sync: %v", err) + } + checkData(path, newData) + if stage == "directory" { + return failure + } + return dirSync(got) + } + err := writeConfigData(path, []byte(strings.TrimSuffix(newData, "\n"))) + if stage == "success" { + if err != nil { + t.Fatal(err) + } + } else if !errors.Is(err, failure) { + t.Fatalf("error = %v, want injected sync failure", err) + } + wantCalls := []string{"file", "directory"} + wantData := newData + if stage == "file" { + wantCalls, wantData = []string{"file"}, oldData + } + if !reflect.DeepEqual(calls, wantCalls) { + t.Fatalf("sync calls = %v, want %v", calls, wantCalls) + } + checkData(path, wantData) + if _, err := tmp.Seek(0, io.SeekCurrent); !errors.Is(err, os.ErrClosed) { + t.Fatalf("temp must be closed on return: %v", err) + } + entries, err := os.ReadDir(dir) + if err != nil || len(entries) != 1 || entries[0].Name() != "config.json" { + t.Fatalf("temporary file not cleaned up: %v, %v", entries, err) + } + }) + } +} + +func TestWriteConfigDataSyncsCreatedDirectories(t *testing.T) { + root := t.TempDir() + outer := filepath.Join(root, "a") + dir := filepath.Join(outer, "b") + path := filepath.Join(dir, "config.json") + failure := errors.New("injected sync failure") + dirSync := syncConfigDirFn + t.Cleanup(func() { syncConfigDirFn = dirSync }) + + for _, fail := range []bool{false, true} { + var calls []string + syncConfigDirFn = func(got string) error { + calls = append(calls, got) + if fail && got == root { + return failure + } + return dirSync(got) + } + if err := os.RemoveAll(outer); err != nil { + t.Fatal(err) + } + err := writeConfigData(path, []byte(`{}`)) + if fail { + if !errors.Is(err, failure) { + t.Fatalf("error = %v, want injected sync failure", err) + } + if want := []string{outer, root}; !reflect.DeepEqual(calls, want) { + t.Fatalf("sync calls = %v, want %v", calls, want) + } + if _, err := os.Stat(path); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("config must not be written after a failed directory sync: %v", err) + } + continue + } + if err != nil { + t.Fatal(err) + } + // Each new directory's entry is synced into its parent, then the + // config's own directory after the rename. + if want := []string{outer, root, dir}; !reflect.DeepEqual(calls, want) { + t.Fatalf("sync calls = %v, want %v", calls, want) + } + } +} + +func TestLockingMutatorSyncsCreatedDirectories(t *testing.T) { + root := t.TempDir() + outer := filepath.Join(root, "a") + dir := filepath.Join(outer, "b") + path := filepath.Join(dir, "config.json") + failure := errors.New("injected sync failure") + dirSync := syncConfigDirFn + t.Cleanup(func() { syncConfigDirFn = dirSync }) + + exists := func(p string) bool { + t.Helper() + _, err := os.Lstat(p) + if err != nil && !errors.Is(err, os.ErrNotExist) { + t.Fatal(err) + } + return err == nil + } + + // Every exported mutator acquires the config lock before it writes, and + // the lock file is a sibling of the config, so the lock path is what first + // creates the config directory. The barrier for those new entries has to + // run there, before the lock file exists: by the time writeConfigData + // looks, nothing is missing any more. + for _, fail := range []bool{true, false} { + var calls []string + syncConfigDirFn = func(got string) error { + calls = append(calls, got) + if got == outer || got == root { + if exists(path+".lock") || exists(path) { + t.Fatalf("parent sync of %s ran after the lock file or config was created", got) + } + } + if fail && got == root { + return failure + } + return dirSync(got) + } + _, err := UpdateNotify(path, func(NotifyConfig) NotifyConfig { + return NotifyConfig{Mode: "off"} + }) + if fail { + if !errors.Is(err, failure) { + t.Fatalf("error = %v, want injected sync failure", err) + } + if want := []string{outer, root}; !reflect.DeepEqual(calls, want) { + t.Fatalf("sync calls = %v, want %v", calls, want) + } + // The directories this attempt created are gone again, so the + // retry below re-creates them and re-runs the barrier instead of + // finding them present and reporting a durability it never had. + if exists(outer) { + t.Fatalf("%s must be rolled back after a failed parent sync", outer) + } + continue + } + if err != nil { + t.Fatal(err) + } + if want := []string{outer, root, dir}; !reflect.DeepEqual(calls, want) { + t.Fatalf("sync calls = %v, want %v", calls, want) + } + got, err := UserNotify(path) + if err != nil || got.Mode != "off" { + t.Fatalf("UserNotify = %+v, %v; want mode off", got, err) + } + } +} + +func TestWriteConfigDataRollsBackOnlyCreatedDirectories(t *testing.T) { + root := t.TempDir() + existing := filepath.Join(root, "existing") + if err := os.Mkdir(existing, 0o700); err != nil { + t.Fatal(err) + } + keep := filepath.Join(existing, "keep") + if err := os.WriteFile(keep, []byte("keep\n"), 0o600); err != nil { + t.Fatal(err) + } + outer := filepath.Join(existing, "a") + dir := filepath.Join(outer, "b") + path := filepath.Join(dir, "config.json") + failure := errors.New("injected sync failure") + dirSync := syncConfigDirFn + t.Cleanup(func() { syncConfigDirFn = dirSync }) + stat := func(p string) error { + _, err := os.Lstat(p) + return err + } + + // The barrier for "a" fails: both directories this call created go, the + // pre-existing directory and its file stay. + syncConfigDirFn = func(got string) error { + if got == existing { + return failure + } + return dirSync(got) + } + if err := writeConfigData(path, []byte(`{}`)); !errors.Is(err, failure) { + t.Fatalf("error = %v, want injected sync failure", err) + } + for _, p := range []string{dir, outer} { + if err := stat(p); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("%s must be removed after a failed parent sync: %v", p, err) + } + } + for _, p := range []string{existing, keep} { + if err := stat(p); err != nil { + t.Fatalf("pre-existing %s must survive the rollback: %v", p, err) + } + } + + // A directory another process populated in the meantime is not ours to + // remove: it stays, the failed rollback is reported with the barrier + // failure, and the write still does not acknowledge the save. + intruder := filepath.Join(dir, "other.lock") + syncConfigDirFn = func(got string) error { + if got == existing { + if err := os.WriteFile(intruder, nil, 0o600); err != nil { + t.Fatal(err) + } + return failure + } + return dirSync(got) + } + err := writeConfigData(path, []byte(`{}`)) + if !errors.Is(err, failure) { + t.Fatalf("error = %v, want injected sync failure", err) + } + if err := stat(intruder); err != nil { + t.Fatalf("a populated directory must not be removed: %v", err) + } + if !strings.Contains(err.Error(), "roll back config directory") { + t.Fatalf("error must report the failed rollback: %v", err) + } + if err := stat(path); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("config must not be written after a failed directory sync: %v", err) + } +} + +func TestSyncConfigDir(t *testing.T) { + dir := t.TempDir() + if err := syncConfigDir(dir); err != nil { + t.Fatal(err) + } + err := syncConfigDir(filepath.Join(dir, "missing")) + if runtime.GOOS == "windows" { + // Directory handles cannot be synced on Windows, so every call is a + // no-op there, including one for a directory that does not exist. + if err != nil { + t.Fatalf("syncConfigDir must be a no-op on windows: %v", err) + } + return + } + // A directory that is gone means the persistence barrier was never + // reached, so the caller must not acknowledge the save. + if !errors.Is(err, os.ErrNotExist) { + t.Fatalf("missing directory: error = %v, want %v", err, os.ErrNotExist) + } + // A directory the user can write but not read cannot be opened for + // syncing; that is best effort, as in the sessions store, because the + // write it follows has already succeeded. + unreadable := filepath.Join(dir, "unreadable") + if err := os.Mkdir(unreadable, 0o300); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(unreadable, 0o700) }) + if err := syncConfigDir(unreadable); err != nil { + t.Fatalf("write-only directory must be best effort: %v", err) + } +} + func TestSetActiveProviderSwitchesConfiguredProvider(t *testing.T) { path := filepath.Join(t.TempDir(), "zero.json") writeConfigFixture(t, path, FileConfig{