fix(daemon): publish status atomically via sibling tempfile and parent sync #952
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,182 @@ | ||
| package daemon | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "os" | ||
| "path/filepath" | ||
| "runtime" | ||
| "sync" | ||
| "sync/atomic" | ||
| "testing" | ||
| "time" | ||
| ) | ||
|
|
||
| func TestStatusFileAtomicPublicationConcurrentReaders(t *testing.T) { | ||
| launcher, _ := seqLauncher(&fakeWorker{pid: 1}) | ||
| srv, paths := newTestServer(t, launcher) | ||
| srv.startedAt = time.Now().UTC().Truncate(time.Millisecond) | ||
|
|
||
| // Publish initial status | ||
| if err := srv.writeStatusFile(); err != nil { | ||
| t.Fatalf("initial writeStatusFile: %v", err) | ||
| } | ||
|
|
||
| var stop atomic.Bool | ||
| var readerErrors atomic.Int64 | ||
| var readCount atomic.Int64 | ||
| var wg sync.WaitGroup | ||
|
|
||
| numReaders := 8 | ||
| for i := 0; i < numReaders; i++ { | ||
| wg.Add(1) | ||
| go func() { | ||
| defer wg.Done() | ||
| for !stop.Load() { | ||
| data, err := os.ReadFile(paths.Status) | ||
| if err != nil { | ||
| // Transient Windows absent window or retryable read | ||
| continue | ||
| } | ||
| if len(data) == 0 { | ||
| readerErrors.Add(1) | ||
| t.Errorf("observed empty status file during concurrent read") | ||
| return | ||
| } | ||
| var sf StatusFile | ||
| if err := json.Unmarshal(data, &sf); err != nil { | ||
| readerErrors.Add(1) | ||
| t.Errorf("observed partial or corrupted status JSON: %v (raw: %q)", err, string(data)) | ||
| return | ||
| } | ||
| if sf.PID != os.Getpid() { | ||
| readerErrors.Add(1) | ||
| t.Errorf("invalid PID in status file: got %d, want %d", sf.PID, os.Getpid()) | ||
| return | ||
| } | ||
| if sf.Socket != paths.Socket { | ||
| readerErrors.Add(1) | ||
| t.Errorf("invalid socket in status file: got %q, want %q", sf.Socket, paths.Socket) | ||
| return | ||
| } | ||
| if sf.Version < 1 { | ||
| readerErrors.Add(1) | ||
| t.Errorf("invalid version in status file: %d", sf.Version) | ||
| return | ||
| } | ||
| readCount.Add(1) | ||
| } | ||
| }() | ||
| } | ||
|
|
||
| // Repeatedly update status file to stress concurrent reader/writer synchronization | ||
| numUpdates := 150 | ||
| for i := 1; i <= numUpdates; i++ { | ||
| srv.opts.Version = i | ||
| srv.startedAt = time.Now().UTC().Add(time.Duration(i) * time.Second).Truncate(time.Millisecond) | ||
| if err := srv.writeStatusFile(); err != nil { | ||
| t.Fatalf("writeStatusFile iteration %d: %v", i, err) | ||
| } | ||
| } | ||
|
|
||
| stop.Store(true) | ||
| wg.Wait() | ||
|
|
||
| if readerErrors.Load() > 0 { | ||
| t.Fatalf("%d reader errors detected during atomic status publication", readerErrors.Load()) | ||
| } | ||
| if readCount.Load() == 0 { | ||
| t.Fatal("no successful concurrent reads completed") | ||
| } | ||
| } | ||
|
|
||
| func TestStatusFileFaultInjectionPreservesExisting(t *testing.T) { | ||
| if runtime.GOOS == "windows" { | ||
| t.Skip("directory permissions-based fault injection skipped on Windows") | ||
| } | ||
|
|
||
| launcher, _ := seqLauncher(&fakeWorker{pid: 1}) | ||
| srv, paths := newTestServer(t, launcher) | ||
|
|
||
| initialStartedAt := time.Date(2026, 8, 24, 12, 0, 0, 0, time.UTC) | ||
| srv.opts.Version = 10 | ||
| srv.startedAt = initialStartedAt | ||
|
|
||
| // 1. Initial successful status publication | ||
| if err := srv.writeStatusFile(); err != nil { | ||
| t.Fatalf("initial writeStatusFile: %v", err) | ||
| } | ||
|
|
||
| initialData, err := os.ReadFile(paths.Status) | ||
| if err != nil { | ||
| t.Fatalf("ReadFile initial status: %v", err) | ||
| } | ||
| var initialStatus StatusFile | ||
| if err := json.Unmarshal(initialData, &initialStatus); err != nil { | ||
| t.Fatalf("Unmarshal initial status: %v", err) | ||
| } | ||
| if initialStatus.Version != 10 { | ||
| t.Fatalf("initial version = %d, want 10", initialStatus.Version) | ||
| } | ||
|
|
||
| // 2. Inject fault: make parent directory read-only so sibling temp file creation fails | ||
| dir := filepath.Dir(paths.Status) | ||
| if err := os.Chmod(dir, 0o500); err != nil { | ||
| t.Fatalf("Chmod dir to 0500: %v", err) | ||
| } | ||
| defer func() { _ = os.Chmod(dir, 0o700) }() | ||
|
|
||
| // 3. Attempt update with new version, which must fail | ||
| srv.opts.Version = 99 | ||
| srv.startedAt = time.Now().UTC() | ||
| err = srv.writeStatusFile() | ||
| if err == nil { | ||
| t.Fatal("expected writeStatusFile to fail on read-only directory") | ||
| } | ||
|
|
||
| // 4. Restore directory permissions | ||
| if err := os.Chmod(dir, 0o700); err != nil { | ||
| t.Fatalf("restore Chmod dir: %v", err) | ||
| } | ||
|
|
||
| // 5. Verify the old status document survived unharmed and was not truncated | ||
| survivingData, err := os.ReadFile(paths.Status) | ||
| if err != nil { | ||
| t.Fatalf("ReadFile surviving status: %v", err) | ||
| } | ||
| if len(survivingData) == 0 { | ||
| t.Fatal("status file was truncated in place during failed write") | ||
| } | ||
|
|
||
| var survivingStatus StatusFile | ||
| if err := json.Unmarshal(survivingData, &survivingStatus); err != nil { | ||
| t.Fatalf("surviving status file is corrupted: %v (raw: %q)", err, string(survivingData)) | ||
| } | ||
| if survivingStatus.Version != 10 { | ||
| t.Fatalf("surviving version = %d, want 10 (old document should be preserved)", survivingStatus.Version) | ||
| } | ||
| if !survivingStatus.StartedAt.Equal(initialStartedAt) { | ||
| t.Fatalf("surviving startedAt = %v, want %v", survivingStatus.StartedAt, initialStartedAt) | ||
| } | ||
| } | ||
|
|
||
| func TestStatusFilePermissions(t *testing.T) { | ||
| if runtime.GOOS == "windows" { | ||
| t.Skip("Unix file permission checks skipped on Windows") | ||
| } | ||
|
|
||
| launcher, _ := seqLauncher(&fakeWorker{pid: 1}) | ||
| srv, paths := newTestServer(t, launcher) | ||
| srv.startedAt = time.Now().UTC() | ||
|
|
||
| if err := srv.writeStatusFile(); err != nil { | ||
| t.Fatalf("writeStatusFile: %v", err) | ||
| } | ||
|
|
||
| info, err := os.Stat(paths.Status) | ||
| if err != nil { | ||
| t.Fatalf("Stat status file: %v", err) | ||
| } | ||
| if perm := info.Mode().Perm(); perm != 0o600 { | ||
| t.Fatalf("status file permissions = %04o, want 0600", perm) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,191 @@ | ||
| package fsutil | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "encoding/json" | ||
| "fmt" | ||
| "os" | ||
| "path/filepath" | ||
| "runtime" | ||
| "sync" | ||
| "sync/atomic" | ||
| "testing" | ||
| ) | ||
|
|
||
| func TestWriteFileAtomicBasic(t *testing.T) { | ||
| dir := t.TempDir() | ||
| path := filepath.Join(dir, "test.txt") | ||
| content := []byte("hello atomic world") | ||
|
|
||
| if err := WriteFileAtomic(path, content, 0o600); err != nil { | ||
| t.Fatalf("WriteFileAtomic: %v", err) | ||
| } | ||
|
|
||
| data, err := os.ReadFile(path) | ||
| if err != nil { | ||
| t.Fatalf("ReadFile: %v", err) | ||
| } | ||
| if !bytes.Equal(data, content) { | ||
| t.Fatalf("content = %q, want %q", data, content) | ||
| } | ||
|
|
||
| info, err := os.Stat(path) | ||
| if err != nil { | ||
| t.Fatalf("Stat: %v", err) | ||
| } | ||
| if runtime.GOOS != "windows" { | ||
| if got := info.Mode().Perm(); got != 0o600 { | ||
| t.Fatalf("permissions = %04o, want 0600", got) | ||
| } | ||
| } | ||
|
|
||
| // Overwrite existing file | ||
| newContent := []byte("updated content") | ||
| if err := WriteFileAtomic(path, newContent, 0o600); err != nil { | ||
| t.Fatalf("WriteFileAtomic overwrite: %v", err) | ||
| } | ||
|
|
||
| data, err = os.ReadFile(path) | ||
| if err != nil { | ||
| t.Fatalf("ReadFile after overwrite: %v", err) | ||
| } | ||
| if !bytes.Equal(data, newContent) { | ||
| t.Fatalf("content after overwrite = %q, want %q", data, newContent) | ||
| } | ||
|
|
||
| // Verify no temporary files remain | ||
| entries, err := os.ReadDir(dir) | ||
| if err != nil { | ||
| t.Fatalf("ReadDir: %v", err) | ||
| } | ||
| if len(entries) != 1 || entries[0].Name() != "test.txt" { | ||
| t.Fatalf("unexpected directory entries: %+v", entries) | ||
| } | ||
| } | ||
|
|
||
| func TestWriteFileAtomicConcurrentReaders(t *testing.T) { | ||
| dir := t.TempDir() | ||
| path := filepath.Join(dir, "data.json") | ||
|
|
||
| type record struct { | ||
| Seq int `json:"seq"` | ||
| Padding string `json:"padding"` | ||
| } | ||
|
|
||
| initial := record{Seq: 0, Padding: "initial"} | ||
| initData, _ := json.Marshal(initial) | ||
| if err := WriteFileAtomic(path, initData, 0o600); err != nil { | ||
| t.Fatalf("initial write: %v", err) | ||
| } | ||
|
|
||
| var stop atomic.Bool | ||
| var readerErrors atomic.Int64 | ||
| var readCount atomic.Int64 | ||
| var wg sync.WaitGroup | ||
|
|
||
| // Launch concurrent reader goroutines | ||
| numReaders := 8 | ||
| for i := 0; i < numReaders; i++ { | ||
| wg.Add(1) | ||
| go func() { | ||
| defer wg.Done() | ||
| for !stop.Load() { | ||
| data, err := os.ReadFile(path) | ||
| if err != nil { | ||
| // On Windows, ReplaceFileW may briefly leave dst absent | ||
| continue | ||
| } | ||
| if len(data) == 0 { | ||
| readerErrors.Add(1) | ||
| t.Errorf("observed empty file during concurrent read") | ||
| return | ||
| } | ||
| var rec record | ||
| if err := json.Unmarshal(data, &rec); err != nil { | ||
| readerErrors.Add(1) | ||
| t.Errorf("observed corrupted/partial JSON: %v (raw: %q)", err, string(data)) | ||
| return | ||
| } | ||
| if rec.Seq < 0 { | ||
| readerErrors.Add(1) | ||
| t.Errorf("invalid sequence number: %d", rec.Seq) | ||
| return | ||
| } | ||
| readCount.Add(1) | ||
| } | ||
| }() | ||
| } | ||
|
|
||
| // Writer publishes new versions sequentially | ||
| numWrites := 150 | ||
| for i := 1; i <= numWrites; i++ { | ||
| rec := record{ | ||
| Seq: i, | ||
| Padding: fmt.Sprintf("payload iteration %d with extended text to ensure multi-byte write", i), | ||
| } | ||
| data, err := json.Marshal(rec) | ||
| if err != nil { | ||
| t.Fatalf("Marshal: %v", err) | ||
| } | ||
| if err := WriteFileAtomic(path, data, 0o600); err != nil { | ||
| t.Fatalf("WriteFileAtomic iter %d: %v", i, err) | ||
| } | ||
| } | ||
|
|
||
| stop.Store(true) | ||
| wg.Wait() | ||
|
|
||
| if readerErrors.Load() > 0 { | ||
| t.Fatalf("%d reader errors observed during concurrent writes", readerErrors.Load()) | ||
| } | ||
| if readCount.Load() == 0 { | ||
| t.Fatal("no successful reads completed") | ||
| } | ||
| } | ||
|
|
||
| func TestWriteFileAtomicFaultPreservesExistingFile(t *testing.T) { | ||
| if runtime.GOOS == "windows" { | ||
| t.Skip("read-only directory permissions differ on Windows") | ||
| } | ||
|
|
||
| dir := t.TempDir() | ||
| path := filepath.Join(dir, "target.txt") | ||
| initialContent := []byte("original protected content") | ||
|
|
||
| if err := WriteFileAtomic(path, initialContent, 0o600); err != nil { | ||
| t.Fatalf("initial write: %v", err) | ||
| } | ||
|
|
||
| // Make parent directory read-only to force failure during temp file creation | ||
| if err := os.Chmod(dir, 0o500); err != nil { | ||
| t.Fatalf("Chmod dir: %v", err) | ||
| } | ||
| defer func() { _ = os.Chmod(dir, 0o700) }() | ||
|
|
||
| // Attempt overwrite which must fail | ||
| err := WriteFileAtomic(path, []byte("new doomed content"), 0o600) | ||
| if err == nil { | ||
| t.Fatal("expected WriteFileAtomic to fail on read-only directory") | ||
| } | ||
|
Comment on lines
+159
to
+169
There was a problem hiding this comment. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- relevant files ---'
git ls-files internal/fsutil/atomic_test.go internal/fsutil internal/daemon/status_test.go internal/daemon
printf '%s\n' '--- atomic test outline ---'
ast-grep outline internal/fsutil/atomic_test.go
printf '%s\n' '--- status test outline ---'
ast-grep outline internal/daemon/status_test.go
printf '%s\n' '--- atomic test excerpt ---'
sed -n '1,230p' internal/fsutil/atomic_test.go
printf '%s\n' '--- status test excerpt ---'
sed -n '1,180p' internal/daemon/status_test.go
printf '%s\n' '--- relevant implementation and fault hooks ---'
rg -n -S 'func WriteFileAtomic|WriteFileAtomic|writeStatusFile|chmod|fault|inject|rename|CreateTemp|TempFile|os\.Create|os\.OpenFile' internal/fsutil internal/daemonRepository: Gitlawb/zero Length of output: 24695 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- atomic implementation ---'
cat -n internal/fsutil/rename.go
printf '%s\n' '--- replacement helpers ---'
cat -n internal/fsutil/replace_other.go
cat -n internal/fsutil/replace_windows.go | sed -n '1,210p'
printf '%s\n' '--- daemon status implementation ---'
sed -n '185,225p' internal/daemon/server.go | cat -n
printf '%s\n' '--- fsutil tests ---'
cat -n internal/fsutil/rename_test.go
cat -n internal/fsutil/replace_other_test.go
printf '%s\n' '--- injectable seams in relevant packages ---'
rg -n -S 'var .*func|func\(.*\)|inject|override|restore|RenameWithRetry|os\.Rename|os\.CreateTemp' internal/fsutil internal/daemon --glob '*_test.go' --glob '*.go'Repository: Gitlawb/zero Length of output: 36519 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- execution identity ---'
id
printf '%s\n' '--- standalone Unix permission probe ---'
python3 - <<'PY'
import os
import stat
import tempfile
mode = 0o500
st = os.stat(".", follow_symlinks=False)
print(f"uid={os.getuid()} euid={os.geteuid()} gid={os.getgid()} mode={stat.S_IMODE(st.st_mode):04o}")
print(f"access_write={os.access('.', os.W_OK)} access_search={os.access('.', os.X_OK)}")
print("A 0500 directory denies write permission to an unprivileged process; a privileged uid 0 process can bypass the mode-bit write check.")
PY
printf '%s\n' '--- server options and test setup ---'
sed -n '1,190p' internal/daemon/server.go | cat -n
rg -n -S 'type .*Options struct|newTestServer|Paths:|ServerOptions|WriteFileAtomic' internal/daemon --glob '*.go'
printf '%s\n' '--- repository build/test guidance ---'
rg -n -S 'make|go run ./cmd/zero-release|go test|Windows|macOS|Linux' README.md CONTRIBUTING.md Makefile .github 2>/dev/null | head -200Repository: Gitlawb/zero Length of output: 13016 Use deterministic filesystem failure injection. A privileged Unix process can create the temporary file in a 📍 Affects 2 files
🤖 Prompt for AI AgentsSource: Coding guidelines |
||
|
|
||
| // Restore permissions to inspect destination | ||
| if err := os.Chmod(dir, 0o700); err != nil { | ||
| t.Fatalf("restore Chmod dir: %v", err) | ||
| } | ||
|
|
||
| // Verify original file is intact and unchanged | ||
| data, err := os.ReadFile(path) | ||
| if err != nil { | ||
| t.Fatalf("ReadFile: %v", err) | ||
| } | ||
| if !bytes.Equal(data, initialContent) { | ||
| t.Fatalf("original content altered: got %q, want %q", data, initialContent) | ||
| } | ||
| } | ||
|
|
||
| func TestSyncDir(t *testing.T) { | ||
| dir := t.TempDir() | ||
| if err := SyncDir(dir); err != nil { | ||
| t.Fatalf("SyncDir on valid directory failed: %v", err) | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not ignore status-file read failures.
Both concurrent-reader tests continue after every
os.ReadFileerror. A replacement implementation that intermittently removes the destination can pass when some reads still succeed. This does not verify the required old-or-new document availability.internal/fsutil/atomic_test.go#L93-L97: count read errors as test failures.internal/daemon/status_test.go#L35-L39: count read errors as test failures.If Windows cannot provide this availability guarantee, change the implementation or document a different contract. Do not hide the failure in the regression test.
As per coding guidelines, every behavior or security-boundary change needs a regression test, including the failure path.
📍 Affects 2 files
internal/fsutil/atomic_test.go#L93-L97(this comment)internal/daemon/status_test.go#L35-L39🤖 Prompt for AI Agents
Source: Coding guidelines