From a1eed5c6a71ceb36a1412393195b2726e7d6bc95 Mon Sep 17 00:00:00 2001 From: Amp Date: Sat, 26 Sep 2026 08:36:40 +0000 Subject: [PATCH 1/4] 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 936425354..8445921af 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path/filepath" + "runtime" "sort" "strings" @@ -1114,6 +1115,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 != "" { @@ -1140,11 +1157,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..ae85dd4b7 100644 --- a/internal/config/writer_test.go +++ b/internal/config/writer_test.go @@ -15,6 +15,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 5c9a0aefe949a785c03c19ac7e13e7fbff918e1f Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sun, 27 Sep 2026 21:44:33 +0200 Subject: [PATCH 2/4] 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 8445921af..37efcd0ef 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -1119,24 +1119,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 ae85dd4b7..21131e10d 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" @@ -51,7 +52,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) @@ -77,7 +78,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) @@ -88,18 +89,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) } } From 40bff0416c6b5e7086820ca3f8f002afc26f8dca Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Wed, 14 Oct 2026 15:53:02 +0800 Subject: [PATCH 3/4] fix(config): report directory-open failures except for write-only directories syncConfigDir treated every os.Open failure as success, so a caller could acknowledge a save whose directory barrier was never reached (Copilot review, PR #1093). Keep the maintainer-requested best effort for a directory the user can write but not read, as in the sessions store, and return every other open failure. TestSyncConfigDir now expects os.ErrNotExist for a missing directory on non-Windows hosts; it fails on the previous code with "missing directory: error = , want file does not exist". --- internal/config/writer.go | 15 +++++++++----- internal/config/writer_test.go | 36 ++++++++++++++++++++++------------ 2 files changed, 33 insertions(+), 18 deletions(-) diff --git a/internal/config/writer.go b/internal/config/writer.go index 37efcd0ef..2f0b059e3 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -1119,17 +1119,22 @@ func writeConfigFile(path string, cfg FileConfig) error { var syncConfigFileFn = (*os.File).Sync var syncConfigDirFn = syncConfigDir -// 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. +// 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 { - return nil + if errors.Is(err, os.ErrPermission) { + return nil + } + return err } return errors.Join(d.Sync(), d.Close()) } diff --git a/internal/config/writer_test.go b/internal/config/writer_test.go index 21131e10d..0c4d7721e 100644 --- a/internal/config/writer_test.go +++ b/internal/config/writer_test.go @@ -139,20 +139,30 @@ func TestSyncConfigDir(t *testing.T) { if err := syncConfigDir(dir); err != nil { t.Fatal(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) + 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) } } From 607a4cc15954cdffce76d5b3c75003ac41f8f6c6 Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Wed, 14 Oct 2026 16:13:01 +0800 Subject: [PATCH 4/4] fix(config): create the config directory durably on the lock path and roll back failed barriers Every exported mutator acquires the config lock before it writes, and the lock file is a sibling of the config, so lockConfigFile's MkdirAll was what created the config directory on a first write. writeConfigData then found nothing missing and never synced the new entries into their parents, so a locking mutator could report success without the directory creation being durable (CodeRabbit review, PR #1093). A failed parent sync also left the newly created directories in place: a retry found nothing missing, skipped the barrier, and reported success. Route both creation sites through one helper, ensureConfigDir. It creates each missing component with os.Mkdir so it knows exactly what it created, syncs each new entry into its parent deepest first, and on any barrier failure removes only the directories this call created, deepest first, with os.Remove so a directory another process has populated survives. A failed rollback is joined into the error, never swallowed. A component that appeared between the lookup and the Mkdir is not rolled back but its parent is still synced. Regression coverage: - TestLockingMutatorSyncsCreatedDirectories drives UpdateNotify on a missing nested directory, asserts the parent syncs happen before the lock file exists, that a failed barrier rolls the directories back, and that the retry re-runs the barrier. On the previous code it fails with "error = , want injected sync failure" because the lock path bypassed the barrier entirely. - TestWriteConfigDataRollsBackOnlyCreatedDirectories asserts a failed barrier removes the created directories but keeps a pre-existing directory and its file, and that a directory populated by another process is kept and the failed rollback reported. On the previous code it fails with "must be removed after a failed parent sync: ". --- internal/config/lock.go | 11 +-- internal/config/writer.go | 83 ++++++++++++++++---- internal/config/writer_test.go | 139 +++++++++++++++++++++++++++++++++ 3 files changed, 213 insertions(+), 20 deletions(-) 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 2f0b059e3..ad41592a9 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -1139,9 +1139,70 @@ func syncConfigDir(dir string) error { 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 the entries MkdirAll creates can be synced into their -// parents. +// 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; ; { @@ -1159,19 +1220,11 @@ func missingConfigDirs(dir string) []string { 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) - } - } + // 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') diff --git a/internal/config/writer_test.go b/internal/config/writer_test.go index 0c4d7721e..6fa9d037f 100644 --- a/internal/config/writer_test.go +++ b/internal/config/writer_test.go @@ -134,6 +134,145 @@ func TestWriteConfigDataSyncsCreatedDirectories(t *testing.T) { } } +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 {