state: Save syncs the state file before renaming it into place (closes #264)
check / check (push) Waiting to run
check / check (push) Waiting to run
Save wrote state.json.tmp with os.WriteFile and renamed it over state.json without syncing. After a power loss the rename could reach the disk before the data, leaving an empty state.json that Load cannot parse, so every start failed. Save now writes the temp file through its own handle, syncs and closes it, renames it, then syncs the data directory so the rename is on disk too. The new test starts from a leftover temp file longer than the state; a test cannot observe the syncs, which are confirmed by reading. Rule suppressed: gosec's file-path warning on the temp file's open, as Load already does. Model: opus-5-5
This commit is contained in:
@@ -19,6 +19,8 @@ trial run of the finished image: https://git.eeqj.de/sneak/dnswatcher/issues/149
|
|||||||
|
|
||||||
# Completed Steps
|
# 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
|
- 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).
|
the address, instead of running without the HTTP server (closes #266).
|
||||||
- 2026-10-13: a bad `PORT`, `DNSWATCHER_TLS_EXPIRY_WARNING` or true/false
|
- 2026-10-13: a bad `PORT`, `DNSWATCHER_TLS_EXPIRY_WARNING` or true/false
|
||||||
|
|||||||
+22
-2
@@ -4,6 +4,7 @@ package state
|
|||||||
import (
|
import (
|
||||||
"context"
|
"context"
|
||||||
"encoding/json"
|
"encoding/json"
|
||||||
|
"errors"
|
||||||
"fmt"
|
"fmt"
|
||||||
"log/slog"
|
"log/slog"
|
||||||
"maps"
|
"maps"
|
||||||
@@ -253,10 +254,20 @@ func (s *State) Save() error {
|
|||||||
return fmt.Errorf("creating data directory: %w", err)
|
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"
|
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 {
|
if err != nil {
|
||||||
return fmt.Errorf("writing temp state file: %w", err)
|
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)
|
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)
|
s.log.Debug("state saved to disk", "path", path)
|
||||||
|
|
||||||
return nil
|
return nil
|
||||||
|
|||||||
@@ -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.
|
// TestLoadMissingFile verifies that loading a nonexistent file is not an error.
|
||||||
func TestLoadMissingFile(t *testing.T) {
|
func TestLoadMissingFile(t *testing.T) {
|
||||||
t.Parallel()
|
t.Parallel()
|
||||||
|
|||||||
Reference in New Issue
Block a user