From c394d3f261b5f8bee29088a21d512960e1d88518 Mon Sep 17 00:00:00 2001 From: Steve Calvert Date: Sat, 11 Apr 2026 13:35:47 -0700 Subject: [PATCH] feat: add atomic file write utility and use it consistently Extract the tmp+rename atomic write pattern from auth/storage.go into a shared fileutil.WriteFileAtomic function. Update session.Save(), config.saveToFile(), and auth writeJSON() to use it, preventing file corruption from crashes mid-write. --- internal/auth/storage.go | 11 ++---- internal/config/config.go | 3 +- internal/fileutil/atomic.go | 33 ++++++++++++++++ internal/fileutil/atomic_test.go | 66 ++++++++++++++++++++++++++++++++ internal/tui/session.go | 3 +- 5 files changed, 106 insertions(+), 10 deletions(-) create mode 100644 internal/fileutil/atomic.go create mode 100644 internal/fileutil/atomic_test.go diff --git a/internal/auth/storage.go b/internal/auth/storage.go index f13d15c..cb0b0ea 100644 --- a/internal/auth/storage.go +++ b/internal/auth/storage.go @@ -7,6 +7,8 @@ import ( "os" "path/filepath" "time" + + "github.com/gleanwork/glean-cli/internal/fileutil" ) // StoredTokens holds persisted OAuth tokens for a Glean host. @@ -53,18 +55,11 @@ func ensureDir(host string) error { } func writeJSON(path string, v any) error { - tmp := path + ".tmp" data, err := json.MarshalIndent(v, "", " ") if err != nil { return err } - if err := os.WriteFile(tmp, data, 0600); err != nil { - return err - } - if err := os.Rename(tmp, path); err != nil { - return err - } - return os.Chmod(path, 0600) + return fileutil.WriteFileAtomic(path, data, 0600) } func readJSON(path string, v any) error { diff --git a/internal/config/config.go b/internal/config/config.go index b853172..9e2a262 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -10,6 +10,7 @@ import ( "strings" "github.com/gleanwork/glean-cli/internal/debug" + "github.com/gleanwork/glean-cli/internal/fileutil" "github.com/zalando/go-keyring" ) @@ -320,7 +321,7 @@ func saveToFile(cfg *Config) error { return fmt.Errorf("error marshaling config: %w", err) } - if err := os.WriteFile(ConfigPath, data, 0600); err != nil { + if err := fileutil.WriteFileAtomic(ConfigPath, data, 0600); err != nil { return fmt.Errorf("error writing config file: %w", err) } diff --git a/internal/fileutil/atomic.go b/internal/fileutil/atomic.go new file mode 100644 index 0000000..44f5dcb --- /dev/null +++ b/internal/fileutil/atomic.go @@ -0,0 +1,33 @@ +package fileutil + +import ( + "os" + "path/filepath" +) + +// WriteFileAtomic writes data to path via a temporary file + rename. +// This prevents corruption from crashes mid-write. +func WriteFileAtomic(path string, data []byte, perm os.FileMode) error { + dir := filepath.Dir(path) + tmp, err := os.CreateTemp(dir, filepath.Base(path)+".tmp") + if err != nil { + return err + } + tmpPath := tmp.Name() + + if _, err := tmp.Write(data); err != nil { + tmp.Close() + os.Remove(tmpPath) + return err + } + if err := tmp.Chmod(perm); err != nil { + tmp.Close() + os.Remove(tmpPath) + return err + } + if err := tmp.Close(); err != nil { + os.Remove(tmpPath) + return err + } + return os.Rename(tmpPath, path) +} diff --git a/internal/fileutil/atomic_test.go b/internal/fileutil/atomic_test.go new file mode 100644 index 0000000..6a88de1 --- /dev/null +++ b/internal/fileutil/atomic_test.go @@ -0,0 +1,66 @@ +package fileutil + +import ( + "os" + "path/filepath" + "runtime" + "testing" +) + +func TestWriteFileAtomic(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "test.json") + content := []byte(`{"key": "value"}`) + + if err := WriteFileAtomic(path, content, 0600); err != nil { + t.Fatalf("WriteFileAtomic failed: %v", err) + } + + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("reading written file: %v", err) + } + if string(got) != string(content) { + t.Errorf("content = %q, want %q", got, content) + } + + info, err := os.Stat(path) + if err != nil { + t.Fatalf("stat: %v", err) + } + if runtime.GOOS != "windows" { + if perm := info.Mode().Perm(); perm != 0600 { + t.Errorf("permissions = %o, want %o", perm, 0600) + } + } +} + +func TestWriteFileAtomic_NoLeftoverTmpFiles(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "data.json") + + if err := WriteFileAtomic(path, []byte("hello"), 0644); err != nil { + t.Fatalf("WriteFileAtomic failed: %v", err) + } + + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("reading dir: %v", err) + } + if len(entries) != 1 { + names := make([]string, len(entries)) + for i, e := range entries { + names[i] = e.Name() + } + t.Errorf("expected 1 file, got %d: %v", len(entries), names) + } +} + +func TestWriteFileAtomic_NonExistentDirectory(t *testing.T) { + path := filepath.Join(t.TempDir(), "no", "such", "dir", "file.json") + + err := WriteFileAtomic(path, []byte("data"), 0600) + if err == nil { + t.Fatal("expected error for non-existent directory, got nil") + } +} diff --git a/internal/tui/session.go b/internal/tui/session.go index e713763..9e9eb20 100644 --- a/internal/tui/session.go +++ b/internal/tui/session.go @@ -8,6 +8,7 @@ import ( "path/filepath" "github.com/gleanwork/glean-cli/internal/debug" + "github.com/gleanwork/glean-cli/internal/fileutil" ) var sessionLog = debug.New("session:persist") @@ -76,7 +77,7 @@ func (s *Session) Save() error { if err != nil { return err } - return os.WriteFile(filepath.Join(dir, "latest.json"), data, 0600) + return fileutil.WriteFileAtomic(filepath.Join(dir, "latest.json"), data, 0600) } // AddTurn appends a turn to the session and saves immediately.