diff --git a/TODO.md b/TODO.md index 775c8fb..c212724 100644 --- a/TODO.md +++ b/TODO.md @@ -19,6 +19,8 @@ trial run of the finished image: https://git.eeqj.de/sneak/dnswatcher/issues/149 # Completed Steps +- 2026-10-13: a save syncs the state file to disk before renaming it into place, + so a crash cannot leave it empty and stop the next start (closes #264). - 2026-10-13: a `PORT` that cannot be bound stops startup with an error naming the address, instead of running without the HTTP server (closes #266). - 2026-10-13: a bad `PORT`, `DNSWATCHER_TLS_EXPIRY_WARNING` or true/false diff --git a/internal/state/state.go b/internal/state/state.go index d8348bb..f183764 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -4,6 +4,7 @@ package state import ( "context" "encoding/json" + "errors" "fmt" "log/slog" "maps" @@ -253,10 +254,20 @@ func (s *State) Save() error { return fmt.Errorf("creating data directory: %w", err) } - // Atomic write: write to temp file, then rename + // Atomic write: write to temp file, then rename. The temp file is + // synced before the rename and the directory after it, or a crash + // can leave the renamed state file empty. tmpPath := path + ".tmp" - err = os.WriteFile(tmpPath, data, filePermissions) + //nolint:gosec // path is from trusted config + tmpFile, err := os.OpenFile( + tmpPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, filePermissions, + ) + if err == nil { + _, err = tmpFile.Write(data) + err = errors.Join(err, tmpFile.Sync(), tmpFile.Close()) + } + if err != nil { return fmt.Errorf("writing temp state file: %w", err) } @@ -266,6 +277,15 @@ func (s *State) Save() error { return fmt.Errorf("renaming state file: %w", err) } + dir, err := os.Open(filepath.Dir(path)) + if err == nil { + err = errors.Join(dir.Sync(), dir.Close()) + } + + if err != nil { + return fmt.Errorf("syncing data directory: %w", err) + } + s.log.Debug("state saved to disk", "path", path) return nil diff --git a/internal/state/state_test.go b/internal/state/state_test.go index ce65140..d2a660f 100644 --- a/internal/state/state_test.go +++ b/internal/state/state_test.go @@ -618,6 +618,53 @@ func TestSaveAtomicWrite(t *testing.T) { } } +// TestSaveOverLeftoverTempFile verifies that a temp file left by a +// crash, longer than the state Save writes, leaves nothing of itself in +// the state file, and that no temp file remains after Save. +func TestSaveOverLeftoverTempFile(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + statePath := filepath.Join(dir, "state.json") + tmpPath := statePath + ".tmp" + + err := os.WriteFile(tmpPath, []byte(strings.Repeat("x", 64*1024)), 0o600) + if err != nil { + t.Fatalf("writing leftover temp file: %v", err) + } + + s := state.NewForTestWithDataDir(dir) + + populateState(t, s) + + err = s.Save() + if err != nil { + t.Fatalf("Save() error: %v", err) + } + + //nolint:gosec // test file with known path + data, err := os.ReadFile(statePath) + if err != nil { + t.Fatalf("reading state file: %v", err) + } + + var snap state.Snapshot + + err = json.Unmarshal(data, &snap) + if err != nil { + t.Fatalf("state file is not complete JSON: %v", err) + } + + if len(snap.Domains) != 2 { + t.Errorf("state file has %d domains, want 2", len(snap.Domains)) + } + + _, err = os.Stat(tmpPath) + if !os.IsNotExist(err) { + t.Error("temp file should not exist after save") + } +} + // TestLoadMissingFile verifies that loading a nonexistent file is not an error. func TestLoadMissingFile(t *testing.T) { t.Parallel()