From 0756af52c9ef1d8f43d2fa7952dbecd9bce325f5 Mon Sep 17 00:00:00 2001 From: Amp Date: Sat, 26 Sep 2026 08:36:40 +0000 Subject: [PATCH 1/2] fix(config): sync atomic config replacements Persist temporary config contents before rename, then sync the parent directory on platforms that support it. Return persistence failures and cover ordering, cleanup, and pre/post-replacement failure states. Fixes Gitlawb/zero#1087 Amp-Thread-ID: https://ampcode.com/threads/T-01a0dcd7-47fc-76eb-82d3-3f6c25384912 Co-authored-by: Pierre Bruno --- internal/config/writer.go | 25 ++++++++++ internal/config/writer_test.go | 88 ++++++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+) diff --git a/internal/config/writer.go b/internal/config/writer.go index 4bc95bf6d..55dd56ef3 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path/filepath" + "runtime" "sort" "strings" @@ -973,6 +974,22 @@ 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; rename durability is best effort there. +func syncConfigDir(dir string) error { + if runtime.GOOS == "windows" { + return nil + } + d, err := os.Open(dir) + if err != nil { + return err + } + return errors.Join(d.Sync(), d.Close()) +} + func writeConfigData(path string, data []byte) error { dir := filepath.Dir(path) if dir != "." && dir != "" { @@ -999,11 +1016,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 0aec31a02..3d9f6fd1e 100644 --- a/internal/config/writer_test.go +++ b/internal/config/writer_test.go @@ -13,6 +13,94 @@ 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.Stat(); !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.Stat(); !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 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" { + if err != nil { + t.Fatalf("Windows must skip directory sync: %v", err) + } + } else if !errors.Is(err, os.ErrNotExist) { + t.Fatalf("directory open error = %v, want not exist", err) + } +} + func TestSetActiveProviderSwitchesConfiguredProvider(t *testing.T) { path := filepath.Join(t.TempDir(), "zero.json") writeConfigFixture(t, path, FileConfig{ From 8c05ee05bedce75dd437c79824a155cce9635941 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sun, 27 Sep 2026 21:44:33 +0200 Subject: [PATCH 2/2] fix(config): sync new config directories and relax unopenable dir sync Sync each directory entry created by MkdirAll into its parent so a crash cannot lose the config directory after a successful first write. Treat a config directory that cannot be opened as best-effort durable, matching the sessions store's syncDir; only a failed Sync or Close is reported, since the rename has already replaced the file. Check the temp file is closed with Seek instead of Stat: on Windows, File.Stat on a closed file returns "The handle is invalid" rather than os.ErrClosed. Co-Authored-By: Claude Opus 5.5 (1M context) --- internal/config/writer.go | 34 ++++++++++++++++- internal/config/writer_test.go | 69 ++++++++++++++++++++++++++++++---- 2 files changed, 93 insertions(+), 10 deletions(-) diff --git a/internal/config/writer.go b/internal/config/writer.go index 55dd56ef3..03bea00f6 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -978,24 +978,54 @@ func writeConfigFile(path string, cfg FileConfig) error { var syncConfigFileFn = (*os.File).Sync var syncConfigDirFn = syncConfigDir -// Windows cannot sync directory handles; rename durability is best effort there. +// Windows cannot sync directory handles, and a directory that cannot be opened +// (e.g. writable but not readable) cannot be synced either; rename durability +// is best effort in both cases, matching the sessions store. Only a failed +// Sync or Close is reported. func syncConfigDir(dir string) error { if runtime.GOOS == "windows" { return nil } d, err := os.Open(dir) if err != nil { - return err + return nil } return errors.Join(d.Sync(), d.Close()) } +// missingConfigDirs lists dir and each ancestor that does not exist yet, +// deepest first, so the entries MkdirAll creates can be synced into their +// parents. +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 != "" { + created := missingConfigDirs(dir) if err := os.MkdirAll(dir, 0o700); err != nil { return fmt.Errorf("create config directory %s: %w", dir, err) } + // Persist each new directory entry so a crash cannot lose the + // directory, and with it the config, after a successful write. + for _, d := range created { + parent := filepath.Dir(d) + if err := syncConfigDirFn(parent); err != nil { + return fmt.Errorf("sync config directory %s: %w", parent, err) + } + } } if len(data) == 0 || data[len(data)-1] != '\n' { data = append(data, '\n') diff --git a/internal/config/writer_test.go b/internal/config/writer_test.go index 3d9f6fd1e..d132bfe6e 100644 --- a/internal/config/writer_test.go +++ b/internal/config/writer_test.go @@ -3,6 +3,7 @@ package config import ( "encoding/json" "errors" + "io" "io/fs" "os" "os/exec" @@ -49,7 +50,7 @@ func TestWriteConfigDataSync(t *testing.T) { if got != dir || tmp == nil { t.Fatalf("directory sync = %q, temp = %v", got, tmp) } - if _, err := tmp.Stat(); !errors.Is(err, os.ErrClosed) { + 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) @@ -75,7 +76,7 @@ func TestWriteConfigDataSync(t *testing.T) { t.Fatalf("sync calls = %v, want %v", calls, wantCalls) } checkData(path, wantData) - if _, err := tmp.Stat(); !errors.Is(err, os.ErrClosed) { + 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) @@ -86,18 +87,70 @@ func TestWriteConfigDataSync(t *testing.T) { } } +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 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" { - if err != nil { - t.Fatalf("Windows must skip directory sync: %v", err) + // A directory that cannot be opened is best effort, as in the sessions + // store: the rename has already happened, so the save is not a failure. + if err := syncConfigDir(filepath.Join(dir, "missing")); err != nil { + t.Fatalf("unopenable directory must be best effort: %v", err) + } + if runtime.GOOS != "windows" { + 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) } - } else if !errors.Is(err, os.ErrNotExist) { - t.Fatalf("directory open error = %v, want not exist", err) } }