From c0ba649ff1d4c57a6766842f12e58cd12d698a59 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sun, 19 Jul 2026 15:38:43 +0200 Subject: [PATCH 1/8] fix(specialist): make overwrites atomic --- internal/specialist/storage.go | 57 ++++++++++++++++++++++------- internal/specialist/storage_test.go | 47 ++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 14 deletions(-) diff --git a/internal/specialist/storage.go b/internal/specialist/storage.go index 952f4f6f9..65202dcf0 100644 --- a/internal/specialist/storage.go +++ b/internal/specialist/storage.go @@ -63,25 +63,17 @@ func (storage *Storage) Create(input CreateInput) (Manifest, error) { return Manifest{}, fmt.Errorf("specialist %q requires a system prompt", manifest.Metadata.Name) } content := FormatMarkdown(manifest) - if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + dir := filepath.Dir(path) + if err := os.MkdirAll(dir, 0o700); err != nil { return Manifest{}, fmt.Errorf("create specialist directory: %w", err) } if input.Overwrite { - info, err := os.Lstat(path) - if err != nil && !os.IsNotExist(err) { - return Manifest{}, fmt.Errorf("inspect specialist file: %w", err) + if err := writeSpecialistAtomic(path, content); err != nil { + return Manifest{}, err } - if err == nil && info.Mode()&os.ModeSymlink != 0 { - return Manifest{}, fmt.Errorf("refusing to overwrite symlink specialist file: %s", path) - } - } - flags := os.O_WRONLY | os.O_CREATE - if input.Overwrite { - flags |= os.O_TRUNC - } else { - flags |= os.O_EXCL + return manifest, nil } - file, err := os.OpenFile(path, flags, 0o600) + file, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) if err != nil { if os.IsExist(err) { return Manifest{}, fmt.Errorf("specialist already exists: %s", manifest.Metadata.Name) @@ -98,6 +90,43 @@ func (storage *Storage) Create(input CreateInput) (Manifest, error) { return manifest, nil } +func writeSpecialistAtomic(path string, content string) (err error) { + temp, err := os.CreateTemp(filepath.Dir(path), ".specialist-*.tmp") + if err != nil { + return fmt.Errorf("create temporary specialist file: %w", err) + } + tempPath := temp.Name() + defer func() { + _ = temp.Close() + _ = os.Remove(tempPath) + }() + + if err := temp.Chmod(0o600); err != nil { + return fmt.Errorf("set temporary specialist file permissions: %w", err) + } + if _, err := temp.WriteString(content); err != nil { + return fmt.Errorf("write temporary specialist file: %w", err) + } + if err := temp.Sync(); err != nil { + return fmt.Errorf("sync temporary specialist file: %w", err) + } + if err := temp.Close(); err != nil { + return fmt.Errorf("close temporary specialist file: %w", err) + } + + info, err := os.Lstat(path) + if err != nil && !os.IsNotExist(err) { + return fmt.Errorf("inspect specialist file: %w", err) + } + if err == nil && info.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("refusing to overwrite symlink specialist file: %s", path) + } + if err := os.Rename(tempPath, path); err != nil { + return fmt.Errorf("replace specialist file: %w", err) + } + return nil +} + func (storage *Storage) Delete(input DeleteInput) (string, error) { location := normalizeWritableLocation(input.Location) path, err := storage.path(input.Name, location) diff --git a/internal/specialist/storage_test.go b/internal/specialist/storage_test.go index fe4c41a9c..df6d86cce 100644 --- a/internal/specialist/storage_test.go +++ b/internal/specialist/storage_test.go @@ -3,6 +3,7 @@ package specialist import ( "os" "path/filepath" + "runtime" "strings" "testing" ) @@ -88,4 +89,50 @@ func TestStorageCreateForceRejectsSymlink(t *testing.T) { if string(data) != "outside" { t.Fatalf("symlink target was modified: %q", string(data)) } + assertNoTemporarySpecialistFiles(t, userDir) +} + +func TestStorageCreateForceAtomicallyReplacesFile(t *testing.T) { + userDir := t.TempDir() + path := filepath.Join(userDir, "safe.md") + if err := os.WriteFile(path, []byte("old content"), 0o644); err != nil { + t.Fatal(err) + } + storage := NewStorage(Paths{UserDir: userDir}) + + manifest, err := storage.Create(CreateInput{ + Name: "safe", + Description: "Safe", + SystemPrompt: "new content", + Overwrite: true, + }) + if err != nil { + t.Fatalf("Create returned error: %v", err) + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if got, want := string(data), FormatMarkdown(manifest); got != want { + t.Fatalf("file content = %q, want %q", got, want) + } + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if got := info.Mode().Perm(); runtime.GOOS != "windows" && got != 0o600 { + t.Fatalf("file permissions = %o, want 600", got) + } + assertNoTemporarySpecialistFiles(t, userDir) +} + +func assertNoTemporarySpecialistFiles(t *testing.T, dir string) { + t.Helper() + matches, err := filepath.Glob(filepath.Join(dir, ".specialist-*.tmp")) + if err != nil { + t.Fatal(err) + } + if len(matches) != 0 { + t.Fatalf("temporary specialist files remain: %v", matches) + } } From c346ba359b079f7f6bf4fc431a3a38b304d31e61 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sun, 19 Jul 2026 16:10:50 +0200 Subject: [PATCH 2/8] fix(specialist): harden atomic replacement --- internal/specialist/storage.go | 29 ++++++++++++++-- internal/specialist/storage_test.go | 51 +++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/internal/specialist/storage.go b/internal/specialist/storage.go index 65202dcf0..cfa52d13b 100644 --- a/internal/specialist/storage.go +++ b/internal/specialist/storage.go @@ -4,8 +4,11 @@ import ( "fmt" "os" "path/filepath" + "runtime" "strconv" "strings" + + "github.com/Gitlawb/zero/internal/fsutil" ) type Storage struct { @@ -90,7 +93,11 @@ func (storage *Storage) Create(input CreateInput) (Manifest, error) { return manifest, nil } -func writeSpecialistAtomic(path string, content string) (err error) { +func writeSpecialistAtomic(path string, content string) error { + return writeSpecialistAtomicWith(path, content, nil, syncSpecialistDir) +} + +func writeSpecialistAtomicWith(path string, content string, rename func(string, string) error, syncDir func(string) error) (err error) { temp, err := os.CreateTemp(filepath.Dir(path), ".specialist-*.tmp") if err != nil { return fmt.Errorf("create temporary specialist file: %w", err) @@ -121,12 +128,30 @@ func writeSpecialistAtomic(path string, content string) (err error) { if err == nil && info.Mode()&os.ModeSymlink != 0 { return fmt.Errorf("refusing to overwrite symlink specialist file: %s", path) } - if err := os.Rename(tempPath, path); err != nil { + if err := fsutil.RenameWithRetry(tempPath, path, rename); err != nil { return fmt.Errorf("replace specialist file: %w", err) } + if err := syncDir(filepath.Dir(path)); err != nil { + return fmt.Errorf("sync specialist directory: %w", err) + } return nil } +func syncSpecialistDir(path string) error { + if runtime.GOOS == "windows" { + return nil + } + dir, err := os.Open(path) + if err != nil { + return err + } + if err := dir.Sync(); err != nil { + _ = dir.Close() + return err + } + return dir.Close() +} + func (storage *Storage) Delete(input DeleteInput) (string, error) { location := normalizeWritableLocation(input.Location) path, err := storage.path(input.Name, location) diff --git a/internal/specialist/storage_test.go b/internal/specialist/storage_test.go index df6d86cce..fb9c5f08f 100644 --- a/internal/specialist/storage_test.go +++ b/internal/specialist/storage_test.go @@ -1,10 +1,12 @@ package specialist import ( + "errors" "os" "path/filepath" "runtime" "strings" + "syscall" "testing" ) @@ -126,6 +128,55 @@ func TestStorageCreateForceAtomicallyReplacesFile(t *testing.T) { assertNoTemporarySpecialistFiles(t, userDir) } +func TestWriteSpecialistAtomicRetriesTransientWindowsRename(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("rename retries are Windows-specific") + } + dir := t.TempDir() + path := filepath.Join(dir, "safe.md") + if err := os.WriteFile(path, []byte("old content"), 0o600); err != nil { + t.Fatal(err) + } + attempts := 0 + err := writeSpecialistAtomicWith(path, "new content", func(src, dst string) error { + attempts++ + if attempts == 1 { + return syscall.Errno(32) // ERROR_SHARING_VIOLATION + } + return os.Rename(src, dst) + }, func(string) error { return nil }) + if err != nil { + t.Fatalf("writeSpecialistAtomicWith returned error: %v", err) + } + if attempts != 2 { + t.Fatalf("rename attempts = %d, want 2", attempts) + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if got := string(data); got != "new content" { + t.Fatalf("file content = %q, want %q", got, "new content") + } + assertNoTemporarySpecialistFiles(t, dir) +} + +func TestWriteSpecialistAtomicPropagatesDirectorySyncError(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "safe.md") + syncErr := errors.New("sync failed") + err := writeSpecialistAtomicWith(path, "new content", nil, func(got string) error { + if got != dir { + t.Fatalf("sync directory = %q, want %q", got, dir) + } + return syncErr + }) + if !errors.Is(err, syncErr) { + t.Fatalf("writeSpecialistAtomicWith error = %v, want %v", err, syncErr) + } + assertNoTemporarySpecialistFiles(t, dir) +} + func assertNoTemporarySpecialistFiles(t *testing.T, dir string) { t.Helper() matches, err := filepath.Glob(filepath.Join(dir, ".specialist-*.tmp")) From c26fe90a1a42f539a08552e5796163bda20bf630 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sat, 25 Jul 2026 15:25:05 +0200 Subject: [PATCH 3/8] fix(specialist): replace through a Windows-atomic, DACL-preserving primitive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses both P1 findings on #757. os.Rename — which fsutil.RenameWithRetry wraps — is documented non-atomic outside Unix, and the retry loop only handles sharing violations. Worse, the replacement is a freshly created temporary file, so renaming it over the destination also publishes the directory's inherited DACL in place of the destination's own: overwriting a specialist that had been explicitly restricted inside a group-readable directory would expose its system prompt to that group. temp.Chmod(0o600) cannot prevent that on Windows, where Go maps only the owner-write bit. New fsutil.ReplaceWithRetry publishes through a platform primitive: - Windows uses ReplaceFileW with REPLACEFILE_WRITE_THROUGH, one operation that carries the replaced file's security descriptor, attributes, and streams over to the replacement. REPLACEFILE_IGNORE_ACL_ERRORS is deliberately not passed — silently losing the descriptor is the failure being fixed — so a merge failure surfaces and leaves the destination untouched. A missing destination falls back to a rename (nothing to replace or preserve), as do volumes that cannot perform a replace at all (FAT/exFAT, some network redirectors), which carry no ACL either. The transient-lock retry is unchanged. - Unix keeps rename(2), which is already atomic within one filesystem. Tests: Windows coverage asserts the destination's creation time survives (a rename would not, proving the call goes through ReplaceFileW) and that a protected owner-only DACL survives both at the fsutil layer and through the real `Create --force` path; a plain rename was verified to replace that DACL with the directory's inherited ACEs, so the assertion has teeth. POSIX covers both destination shapes. --- internal/fsutil/rename.go | 17 ++ internal/fsutil/replace_other.go | 12 ++ internal/fsutil/replace_other_test.go | 50 +++++ internal/fsutil/replace_windows.go | 83 ++++++++ internal/fsutil/replace_windows_test.go | 180 ++++++++++++++++++ internal/specialist/storage.go | 9 +- .../specialist/storage_dacl_windows_test.go | 85 +++++++++ 7 files changed, 435 insertions(+), 1 deletion(-) create mode 100644 internal/fsutil/replace_other.go create mode 100644 internal/fsutil/replace_other_test.go create mode 100644 internal/fsutil/replace_windows.go create mode 100644 internal/fsutil/replace_windows_test.go create mode 100644 internal/specialist/storage_dacl_windows_test.go diff --git a/internal/fsutil/rename.go b/internal/fsutil/rename.go index ace33502a..889a60604 100644 --- a/internal/fsutil/rename.go +++ b/internal/fsutil/rename.go @@ -9,6 +9,23 @@ import ( "time" ) +// ReplaceWithRetry publishes src over dst using the platform's replacement +// primitive, retrying on the same transient Windows lock errors RenameWithRetry +// handles. Prefer it over RenameWithRetry when dst may already exist and is +// expected to keep its identity: on Windows it replaces through ReplaceFileW, +// which is a single operation (os.Rename is documented non-atomic there) and +// carries the destination's security descriptor over to the replacement instead +// of publishing the temporary file's inherited ACL. On Unix it is os.Rename. +// +// replace overrides the platform primitive so tests can exercise the retry path; +// pass nil for the default. +func ReplaceWithRetry(src, dst string, replace func(src, dst string) error) error { + if replace == nil { + replace = replaceExisting + } + return RenameWithRetry(src, dst, replace) +} + // RenameWithRetry renames src to dst, retrying briefly on Windows when the // destination is transiently locked (antivirus scanners, search indexers, or // a concurrent reader holding the file open). rename overrides os.Rename so diff --git a/internal/fsutil/replace_other.go b/internal/fsutil/replace_other.go new file mode 100644 index 000000000..35a985518 --- /dev/null +++ b/internal/fsutil/replace_other.go @@ -0,0 +1,12 @@ +//go:build !windows + +package fsutil + +import "os" + +// replaceExisting publishes src over dst. rename(2) already replaces the +// destination atomically within one filesystem on Unix, and it neither creates +// nor consults an ACL, so there is nothing extra to preserve here. +func replaceExisting(src, dst string) error { + return os.Rename(src, dst) +} diff --git a/internal/fsutil/replace_other_test.go b/internal/fsutil/replace_other_test.go new file mode 100644 index 000000000..9c53c9225 --- /dev/null +++ b/internal/fsutil/replace_other_test.go @@ -0,0 +1,50 @@ +//go:build !windows + +package fsutil + +import ( + "os" + "path/filepath" + "testing" +) + +// On Unix the replacement primitive is rename(2), which already publishes +// atomically within one filesystem and neither creates nor consults an ACL. These +// cover both shapes so the shared helper is exercised on every platform. +func TestReplaceWithRetryPublishesOverExistingAndMissingDestinations(t *testing.T) { + for _, tc := range []struct { + name string + existing bool + }{ + {name: "existing destination", existing: true}, + {name: "missing destination"}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + src := filepath.Join(dir, ".manifest.tmp") + dst := filepath.Join(dir, "manifest.md") + if err := os.WriteFile(src, []byte("new"), 0o600); err != nil { + t.Fatalf("WriteFile src: %v", err) + } + if tc.existing { + if err := os.WriteFile(dst, []byte("old"), 0o600); err != nil { + t.Fatalf("WriteFile dst: %v", err) + } + } + + if err := ReplaceWithRetry(src, dst, nil); err != nil { + t.Fatalf("ReplaceWithRetry: %v", err) + } + data, err := os.ReadFile(dst) + if err != nil { + t.Fatalf("ReadFile dst: %v", err) + } + if string(data) != "new" { + t.Fatalf("destination content = %q, want the replacement bytes", data) + } + if _, err := os.Lstat(src); !os.IsNotExist(err) { + t.Fatalf("the replacement file should be consumed by the replace: %v", err) + } + }) + } +} diff --git a/internal/fsutil/replace_windows.go b/internal/fsutil/replace_windows.go new file mode 100644 index 000000000..c4aa27a3c --- /dev/null +++ b/internal/fsutil/replace_windows.go @@ -0,0 +1,83 @@ +//go:build windows + +package fsutil + +import ( + "errors" + "fmt" + "os" + "syscall" + "unsafe" +) + +const ( + replaceFileWriteThrough = 0x00000001 + replaceFileIgnoreMergeErrors = 0x00000002 + + // Returned when the volume cannot perform a replace (FAT/exFAT, some network + // redirectors). Such volumes carry no ACL to preserve, so a plain rename is + // an equivalent publish there. + errorInvalidFunction = syscall.Errno(1) + errorNotSupported = syscall.Errno(50) +) + +var ( + replaceKernel32 = syscall.NewLazyDLL("kernel32.dll") + replaceProcReplaceFil = replaceKernel32.NewProc("ReplaceFileW") +) + +// replaceExisting publishes src over dst with ReplaceFileW rather than +// MoveFileEx (what os.Rename uses). Two reasons, both of which os.Rename fails +// to provide on Windows: +// +// - Atomicity. Go documents os.Rename as NOT atomic outside Unix, so an +// interrupted or concurrently observed overwrite can expose a missing or +// intermediate file. ReplaceFileW performs the swap as one operation, and +// REPLACEFILE_WRITE_THROUGH flushes it before returning. +// - Security descriptor. The replacement is a freshly created temporary file, +// so it carries the directory's inherited DACL. Renaming it over the +// destination therefore REPLACES the destination's ACL — silently widening +// access to a file that had been restricted explicitly (os.File.Chmod cannot +// express that on Windows; Go only maps the owner-write bit). ReplaceFileW +// carries the replaced file's security descriptor, attributes, and streams +// over to the replacement instead. +// +// REPLACEFILE_IGNORE_ACL_ERRORS is deliberately NOT passed: silently losing the +// descriptor is the very failure this exists to prevent, so an ACL merge failure +// surfaces as an error, leaving the destination untouched and the caller free to +// clean up its temporary file. +func replaceExisting(src, dst string) error { + if _, err := os.Lstat(dst); err != nil { + if os.IsNotExist(err) { + // Nothing to replace and no descriptor to preserve. + return os.Rename(src, dst) + } + return err + } + replaced, err := syscall.UTF16PtrFromString(dst) + if err != nil { + return err + } + replacement, err := syscall.UTF16PtrFromString(src) + if err != nil { + return err + } + result, _, callErr := replaceProcReplaceFil.Call( + uintptr(unsafe.Pointer(replaced)), + uintptr(unsafe.Pointer(replacement)), + 0, // no backup copy + uintptr(replaceFileWriteThrough|replaceFileIgnoreMergeErrors), + 0, + 0, + ) + if result != 0 { + return nil + } + if callErr == nil || errors.Is(callErr, syscall.Errno(0)) { + return fmt.Errorf("replace %s: ReplaceFileW failed", dst) + } + if errors.Is(callErr, errorInvalidFunction) || errors.Is(callErr, errorNotSupported) { + return os.Rename(src, dst) + } + return callErr +} diff --git a/internal/fsutil/replace_windows_test.go b/internal/fsutil/replace_windows_test.go new file mode 100644 index 000000000..0260fbfcb --- /dev/null +++ b/internal/fsutil/replace_windows_test.go @@ -0,0 +1,180 @@ +//go:build windows + +package fsutil + +import ( + "os" + "path/filepath" + "strings" + "syscall" + "testing" + + "golang.org/x/sys/windows" +) + +// TestReplaceWithRetryKeepsTheDestinationIdentity proves the publish goes through +// ReplaceFileW and not os.Rename: ReplaceFileW gives the replacement the replaced +// file's identity, so the destination's creation time survives. A rename would +// carry the temporary file's creation time over instead, and os.Rename is also +// documented non-atomic on Windows. +func TestReplaceWithRetryKeepsTheDestinationIdentity(t *testing.T) { + dir := t.TempDir() + dst := filepath.Join(dir, "manifest.md") + if err := os.WriteFile(dst, []byte("old"), 0o600); err != nil { + t.Fatalf("WriteFile dst: %v", err) + } + created := creationTime(t, dst) + + src := filepath.Join(dir, ".manifest.tmp") + if err := os.WriteFile(src, []byte("new"), 0o600); err != nil { + t.Fatalf("WriteFile src: %v", err) + } + // Push the replacement's own creation time clearly past the destination's so a + // rename would be visible in the comparison below. + future := windows.NsecToFiletime(created.Nanoseconds() + int64(10*1e9)) + setCreationTime(t, src, future) + + if err := ReplaceWithRetry(src, dst, nil); err != nil { + t.Fatalf("ReplaceWithRetry: %v", err) + } + data, err := os.ReadFile(dst) + if err != nil { + t.Fatalf("ReadFile dst: %v", err) + } + if string(data) != "new" { + t.Fatalf("destination content = %q, want the replacement bytes", data) + } + if _, err := os.Lstat(src); !os.IsNotExist(err) { + t.Fatalf("the replacement file should be consumed by the replace: %v", err) + } + if got := creationTime(t, dst); got != created { + t.Fatalf("creation time = %v, want the replaced file's %v (a rename would not preserve it)", got, created) + } +} + +// TestReplaceWithRetryPreservesDestinationDACL is the regression test for the +// second half of the finding: the replacement is a freshly created temporary file +// carrying the directory's inherited DACL, so publishing it with a rename would +// REPLACE the restrictive descriptor an explicitly locked-down file had. +func TestReplaceWithRetryPreservesDestinationDACL(t *testing.T) { + dir := t.TempDir() + dst := filepath.Join(dir, "manifest.md") + if err := os.WriteFile(dst, []byte("old"), 0o600); err != nil { + t.Fatalf("WriteFile dst: %v", err) + } + // A protected DACL granting only the owner: distinct from whatever the temp + // file inherits from the directory. + restricted, err := windows.SecurityDescriptorFromString("D:P(A;;FA;;;OW)") + if err != nil { + t.Skipf("cannot build a test security descriptor: %v", err) + } + dacl, _, err := restricted.DACL() + if err != nil { + t.Skipf("cannot read the test DACL: %v", err) + } + if err := windows.SetNamedSecurityInfo( + dst, + windows.SE_FILE_OBJECT, + windows.DACL_SECURITY_INFORMATION|windows.PROTECTED_DACL_SECURITY_INFORMATION, + nil, nil, dacl, nil, + ); err != nil { + t.Skipf("cannot apply a restrictive DACL on this filesystem: %v", err) + } + want := describeDACL(t, dst) + + src := filepath.Join(dir, ".manifest.tmp") + if err := os.WriteFile(src, []byte("new"), 0o600); err != nil { + t.Fatalf("WriteFile src: %v", err) + } + if inherited := describeDACL(t, src); inherited == want { + t.Skip("the temporary file already carries the same DACL; this filesystem cannot show the difference") + } + + if err := ReplaceWithRetry(src, dst, nil); err != nil { + t.Fatalf("ReplaceWithRetry: %v", err) + } + if got := describeDACL(t, dst); got != want { + t.Fatalf("DACL after replace = %q, want the destination's own %q", got, want) + } +} + +// TestReplaceWithRetryPublishesWhenDestinationIsMissing covers the no-destination +// case: ReplaceFileW requires an existing file to replace, so there is a rename +// fallback (and nothing to preserve). +func TestReplaceWithRetryPublishesWhenDestinationIsMissing(t *testing.T) { + dir := t.TempDir() + src := filepath.Join(dir, ".manifest.tmp") + dst := filepath.Join(dir, "manifest.md") + if err := os.WriteFile(src, []byte("new"), 0o600); err != nil { + t.Fatalf("WriteFile src: %v", err) + } + if err := ReplaceWithRetry(src, dst, nil); err != nil { + t.Fatalf("ReplaceWithRetry: %v", err) + } + if data, err := os.ReadFile(dst); err != nil || string(data) != "new" { + t.Fatalf("destination = %q err=%v, want the replacement bytes", data, err) + } +} + +// TestReplaceWithRetryRetriesTransientLockViolation keeps the retry behavior that +// RenameWithRetry provides for antivirus/indexer holds. +func TestReplaceWithRetryRetriesTransientLockViolation(t *testing.T) { + attempts := 0 + err := ReplaceWithRetry("src", "dst", func(src, dst string) error { + attempts++ + if attempts < 3 { + return &os.PathError{Op: "replace", Path: dst, Err: syscall.Errno(32)} // ERROR_SHARING_VIOLATION + } + return nil + }) + if err != nil { + t.Fatalf("ReplaceWithRetry: %v", err) + } + if attempts != 3 { + t.Fatalf("attempts = %d, want the transient violations retried", attempts) + } +} + +func creationTime(t *testing.T, path string) syscall.Filetime { + t.Helper() + info, err := os.Stat(path) + if err != nil { + t.Fatalf("Stat %s: %v", path, err) + } + data, ok := info.Sys().(*syscall.Win32FileAttributeData) + if !ok { + t.Skipf("no Windows file attributes for %s", path) + } + return data.CreationTime +} + +func setCreationTime(t *testing.T, path string, created windows.Filetime) { + t.Helper() + pathPtr, err := windows.UTF16PtrFromString(path) + if err != nil { + t.Fatalf("UTF16PtrFromString: %v", err) + } + handle, err := windows.CreateFile(pathPtr, windows.FILE_WRITE_ATTRIBUTES, 0, nil, windows.OPEN_EXISTING, windows.FILE_ATTRIBUTE_NORMAL, 0) + if err != nil { + t.Fatalf("CreateFile %s: %v", path, err) + } + defer func() { + _ = windows.CloseHandle(handle) + }() + if err := windows.SetFileTime(handle, &created, nil, nil); err != nil { + t.Fatalf("SetFileTime %s: %v", path, err) + } +} + +func describeDACL(t *testing.T, path string) string { + t.Helper() + sd, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + t.Skipf("cannot read the security descriptor of %s: %v", path, err) + } + text := sd.String() + if index := strings.Index(text, "D:"); index >= 0 { + return text[index:] + } + return text +} diff --git a/internal/specialist/storage.go b/internal/specialist/storage.go index cfa52d13b..719eb8c26 100644 --- a/internal/specialist/storage.go +++ b/internal/specialist/storage.go @@ -128,7 +128,14 @@ func writeSpecialistAtomicWith(path string, content string, rename func(string, if err == nil && info.Mode()&os.ModeSymlink != 0 { return fmt.Errorf("refusing to overwrite symlink specialist file: %s", path) } - if err := fsutil.RenameWithRetry(tempPath, path, rename); err != nil { + // ReplaceWithRetry, not RenameWithRetry: os.Rename is documented non-atomic + // outside Unix, and renaming the freshly created temporary file over the + // destination would also publish the directory's inherited DACL in place of + // whatever the destination had — silently widening access to a specialist that + // had been restricted explicitly. On Windows this replaces through + // ReplaceFileW, which is one operation and preserves the destination's + // security descriptor; on Unix it is the same atomic rename as before. + if err := fsutil.ReplaceWithRetry(tempPath, path, rename); err != nil { return fmt.Errorf("replace specialist file: %w", err) } if err := syncDir(filepath.Dir(path)); err != nil { diff --git a/internal/specialist/storage_dacl_windows_test.go b/internal/specialist/storage_dacl_windows_test.go new file mode 100644 index 000000000..7ef53f999 --- /dev/null +++ b/internal/specialist/storage_dacl_windows_test.go @@ -0,0 +1,85 @@ +//go:build windows + +package specialist + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "golang.org/x/sys/windows" +) + +// TestStorageCreateForceKeepsWindowsDACL is the regression test for the Windows +// half of the atomic-overwrite change: the replacement is a freshly created +// temporary file, so publishing it with a plain rename would hand the destination +// the directory's inherited DACL and drop the restrictive one an explicitly +// locked-down specialist had — exposing its system prompt to anyone the directory +// grants access to. temp.Chmod(0o600) cannot express that on Windows (Go only +// maps the owner-write bit there), so the replacement primitive has to carry the +// descriptor over. +func TestStorageCreateForceKeepsWindowsDACL(t *testing.T) { + userDir := t.TempDir() + path := filepath.Join(userDir, "safe.md") + if err := os.WriteFile(path, []byte("old content"), 0o600); err != nil { + t.Fatal(err) + } + // A protected, owner-only DACL: what an operator restricting one specialist + // inside a group-readable directory would end up with. + restricted, err := windows.SecurityDescriptorFromString("D:P(A;;FA;;;OW)") + if err != nil { + t.Skipf("cannot build a test security descriptor: %v", err) + } + dacl, _, err := restricted.DACL() + if err != nil { + t.Skipf("cannot read the test DACL: %v", err) + } + if err := windows.SetNamedSecurityInfo( + path, + windows.SE_FILE_OBJECT, + windows.DACL_SECURITY_INFORMATION|windows.PROTECTED_DACL_SECURITY_INFORMATION, + nil, nil, dacl, nil, + ); err != nil { + t.Skipf("cannot apply a restrictive DACL on this filesystem: %v", err) + } + want := specialistDACL(t, path) + if !strings.Contains(want, "(A;;FA;;;OW)") { + t.Skipf("the restrictive DACL did not take effect on this filesystem: %q", want) + } + + storage := NewStorage(Paths{UserDir: userDir}) + manifest, err := storage.Create(CreateInput{ + Name: "safe", + Description: "Safe", + SystemPrompt: "new content", + Overwrite: true, + }) + if err != nil { + t.Fatalf("Create returned error: %v", err) + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if got, want := string(data), FormatMarkdown(manifest); got != want { + t.Fatalf("file content = %q, want %q", got, want) + } + if got := specialistDACL(t, path); got != want { + t.Fatalf("DACL after overwrite = %q, want the destination's own %q", got, want) + } + assertNoTemporarySpecialistFiles(t, userDir) +} + +func specialistDACL(t *testing.T, path string) string { + t.Helper() + sd, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + t.Skipf("cannot read the security descriptor of %s: %v", path, err) + } + text := sd.String() + if index := strings.Index(text, "D:"); index >= 0 { + return text[index:] + } + return text +} From 32140de25a0d4a1ebc4fe49357110bef226f702e Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sun, 26 Jul 2026 15:16:40 +0200 Subject: [PATCH 4/8] fix(fsutil): stop ignoring ACL merge errors, recover partial ReplaceFileW Addresses jatmn's two #757 review findings: - The call passed REPLACEFILE_IGNORE_MERGE_ERRORS (0x2) while the comment claimed ACL failures were fail-closed. Microsoft documents 0x2 and REPLACEFILE_IGNORE_ACL_ERRORS (0x4) identically: with either set, a call lacking WRITE_DAC "succeeds but the ACLs are not preserved". Passing 0x2 therefore let a --force overwrite silently publish the temp file's inherited directory DACL over a restricted specialist and expose its system prompt. Pass no flags at all. - With no backup file, ERROR_UNABLE_TO_MOVE_REPLACEMENT (1176) and ERROR_UNABLE_TO_MOVE_REPLACEMENT_2 (1177) leave the destination deleted (1176) or renamed to an unreachable name (1177) while the replacement survives only under its temporary name. Returning those unrepaired let the caller's deferred temp-file cleanup remove the sole surviving copy, so one failed 'specialist create --force' destroyed both the old manifest and the new one. recoverPartialReplace now moves the replacement into place and still reports the error, since the destination's descriptor died with the destination. Also drops the doc claim that REPLACEFILE_WRITE_THROUGH (0x1) flushes before returning: Microsoft documents that value as "not supported". The two fsutil regression tests are verified to fail against the pre-fix code. The specialist-side test guards the caller half of the recovery contract and does not fail pre-fix; its comment says so. Co-Authored-By: Claude Opus 5 (1M context) --- internal/fsutil/replace_windows.go | 92 ++++++++++++++++--- internal/fsutil/replace_windows_test.go | 113 ++++++++++++++++++++++++ internal/specialist/storage_test.go | 49 ++++++++++ 3 files changed, 243 insertions(+), 11 deletions(-) diff --git a/internal/fsutil/replace_windows.go b/internal/fsutil/replace_windows.go index c4aa27a3c..35a8a9151 100644 --- a/internal/fsutil/replace_windows.go +++ b/internal/fsutil/replace_windows.go @@ -10,15 +10,36 @@ import ( "unsafe" ) -const ( - replaceFileWriteThrough = 0x00000001 - replaceFileIgnoreMergeErrors = 0x00000002 +// replaceFileFlags is deliberately ZERO. Every REPLACEFILE_* flag ReplaceFileW +// accepts either defeats the reason this function exists or does nothing: +// +// - REPLACEFILE_IGNORE_MERGE_ERRORS (0x2) and REPLACEFILE_IGNORE_ACL_ERRORS +// (0x4). Microsoft documents BOTH with the same consequence: "if you specify +// this flag and do not have WRITE_DAC access, the function succeeds but the +// ACLs are not preserved." A silent success that publishes the temporary +// file's inherited directory DACL over an explicitly restricted specialist — +// exposing its system prompt — is precisely the failure this function exists +// to prevent, so a merge failure MUST surface as an error and leave the +// destination untouched. Passing 0x2 while omitting only 0x4 buys nothing: +// ACL merging is part of the metadata merge 0x2 covers. +// - REPLACEFILE_WRITE_THROUGH (0x1) is documented as "This value is not +// supported", so it cannot be relied on to flush anything. The single- +// operation swap is ReplaceFileW's own documented behavior, not this flag's. +const replaceFileFlags = 0 +const ( // Returned when the volume cannot perform a replace (FAT/exFAT, some network // redirectors). Such volumes carry no ACL to preserve, so a plain rename is // an equivalent publish there. errorInvalidFunction = syscall.Errno(1) errorNotSupported = syscall.Errno(50) + + // Partial-failure codes: ReplaceFileW got far enough to move or delete + // something, so the on-disk state needs repair rather than a bare error. See + // recoverPartialReplace. + errorUnableToRemoveReplaced = syscall.Errno(1175) + errorUnableToMoveReplacement = syscall.Errno(1176) + errorUnableToMoveReplacement2 = syscall.Errno(1177) ) var ( @@ -32,8 +53,7 @@ var ( // // - Atomicity. Go documents os.Rename as NOT atomic outside Unix, so an // interrupted or concurrently observed overwrite can expose a missing or -// intermediate file. ReplaceFileW performs the swap as one operation, and -// REPLACEFILE_WRITE_THROUGH flushes it before returning. +// intermediate file. ReplaceFileW performs the swap as one operation. // - Security descriptor. The replacement is a freshly created temporary file, // so it carries the directory's inherited DACL. Renaming it over the // destination therefore REPLACES the destination's ACL — silently widening @@ -42,10 +62,11 @@ var ( // carries the replaced file's security descriptor, attributes, and streams // over to the replacement instead. // -// REPLACEFILE_IGNORE_ACL_ERRORS is deliberately NOT passed: silently losing the -// descriptor is the very failure this exists to prevent, so an ACL merge failure -// surfaces as an error, leaving the destination untouched and the caller free to -// clean up its temporary file. +// No REPLACEFILE_* flag is passed at all — see replaceFileFlags for why each one +// would either silently lose the descriptor this function exists to preserve or +// do nothing. A merge failure therefore surfaces as an error, leaving the +// destination untouched and the caller free to clean up its temporary file, +// except in the partial-failure states recoverPartialReplace repairs. func replaceExisting(src, dst string) error { if _, err := os.Lstat(dst); err != nil { if os.IsNotExist(err) { @@ -66,7 +87,7 @@ func replaceExisting(src, dst string) error { uintptr(unsafe.Pointer(replaced)), uintptr(unsafe.Pointer(replacement)), 0, // no backup copy - uintptr(replaceFileWriteThrough|replaceFileIgnoreMergeErrors), + uintptr(replaceFileFlags), 0, 0, ) @@ -79,5 +100,54 @@ func replaceExisting(src, dst string) error { if errors.Is(callErr, errorInvalidFunction) || errors.Is(callErr, errorNotSupported) { return os.Rename(src, dst) } - return callErr + return recoverPartialReplace(callErr, src, dst) +} + +// recoverPartialReplace repairs the on-disk state after a ReplaceFileW failure +// that had already moved or deleted something, then returns the error to report. +// +// We pass no backup file, and Microsoft documents that this makes two failure +// codes leave the DESTINATION GONE while the replacement survives only under its +// original (temporary) name: +// +// - ERROR_UNABLE_TO_MOVE_REPLACEMENT (1176): "If lpBackupFileName was +// specified, the replaced and replacement files retain their original file +// names. Otherwise, the replaced file no longer exists and the replacement +// file exists under its original name." +// - ERROR_UNABLE_TO_MOVE_REPLACEMENT_2 (1177): the replacement survives under +// its original name having already inherited the replaced file's streams and +// attributes, while the replaced file "still exists with a different name" — +// a name only the backup parameter would have reported, so with no backup it +// is unreachable and the destination path is equally empty. +// +// Callers write to a temporary file and remove it unconditionally on failure +// (internal/specialist's writeSpecialistAtomicWith does, via defer), so +// returning either error unrepaired destroys BOTH copies: the destination +// Windows already deleted, and the only surviving copy of the new content. A +// failed `specialist create --force` would leave no manifest at all. +// +// Moving the replacement into place is therefore the recovery. The destination +// path is free — Windows deleted or renamed away whatever was there — so the new +// content lands where it belongs and nothing is left for the caller's cleanup to +// delete. The error is still returned rather than swallowed: the destination's +// original security descriptor died with the destination, so the published file +// carries the temporary file's inherited DACL, and reporting success would be +// exactly the silent ACL widening replaceFileFlags refuses to allow. +func recoverPartialReplace(callErr error, src, dst string) error { + if !errors.Is(callErr, errorUnableToMoveReplacement) && !errors.Is(callErr, errorUnableToMoveReplacement2) { + // Everything else — including ERROR_UNABLE_TO_REMOVE_REPLACED (1175) and + // every unlisted error, for which Microsoft documents that "the replaced and + // replacement files retain their original file names" — leaves the + // destination holding its original content. The caller's temporary-file + // cleanup is then correct and there is nothing to repair. + return callErr + } + if _, err := os.Lstat(src); err != nil { + // No replacement left to recover with; report the original failure. + return callErr + } + if err := os.Rename(src, dst); err != nil { + return fmt.Errorf("replace %s: %w (the replacement survives at %s; recovering it failed: %v)", dst, callErr, src, err) + } + return fmt.Errorf("replace %s: %w (the replacement was moved into place, but the destination's security descriptor was lost with the destination — re-check its permissions)", dst, callErr) } diff --git a/internal/fsutil/replace_windows_test.go b/internal/fsutil/replace_windows_test.go index 0260fbfcb..13846aa2c 100644 --- a/internal/fsutil/replace_windows_test.go +++ b/internal/fsutil/replace_windows_test.go @@ -3,6 +3,7 @@ package fsutil import ( + "errors" "os" "path/filepath" "strings" @@ -135,6 +136,118 @@ func TestReplaceWithRetryRetriesTransientLockViolation(t *testing.T) { } } +// TestReplaceFileFlagsDoNotIgnoreMergeErrors is the regression test for jatmn's +// #757 P1 finding on flags: the call used to pass REPLACEFILE_IGNORE_MERGE_ERRORS +// (0x2) while the comment claimed ACL failures were fail-closed. Microsoft +// documents 0x2 and REPLACEFILE_IGNORE_ACL_ERRORS (0x4) identically — with either +// one set, a call lacking WRITE_DAC "succeeds but the ACLs are not preserved" — +// so passing 0x2 let a --force overwrite silently publish the temporary file's +// inherited directory DACL over a restricted specialist and expose its system +// prompt. +// +// This asserts the flag word directly rather than a live denied merge because a +// denied merge is not constructible for a file this process owns: Windows grants +// an object's owner READ_CONTROL and WRITE_DAC implicitly, so no DACL a test can +// apply to its own temp file can withhold WRITE_DAC from it. Pinning the flags is +// what actually prevents the regression — re-adding either bit fails here. +func TestReplaceFileFlagsDoNotIgnoreMergeErrors(t *testing.T) { + const ( + ignoreMergeErrors = 0x00000002 + ignoreACLErrors = 0x00000004 + ) + if replaceFileFlags&ignoreMergeErrors != 0 { + t.Error("REPLACEFILE_IGNORE_MERGE_ERRORS must not be set: it makes ReplaceFileW succeed WITHOUT preserving ACLs when it cannot obtain WRITE_DAC") + } + if replaceFileFlags&ignoreACLErrors != 0 { + t.Error("REPLACEFILE_IGNORE_ACL_ERRORS must not be set: it makes ReplaceFileW succeed WITHOUT preserving ACLs when it cannot obtain WRITE_DAC") + } +} + +// TestRecoverPartialReplaceRescuesTheReplacement is the regression test for +// jatmn's #757 second P1 finding. With no backup file, ReplaceFileW's 1176/1177 +// failures leave the DESTINATION DELETED and the replacement surviving only under +// its temporary name. Returning those errors unrepaired let the caller's deferred +// temp-file cleanup remove that sole surviving copy, so one failed +// `specialist create --force` destroyed the old manifest AND the new one. +func TestRecoverPartialReplaceRescuesTheReplacement(t *testing.T) { + for _, tc := range []struct { + name string + code syscall.Errno + }{ + {name: "ERROR_UNABLE_TO_MOVE_REPLACEMENT", code: errorUnableToMoveReplacement}, + {name: "ERROR_UNABLE_TO_MOVE_REPLACEMENT_2", code: errorUnableToMoveReplacement2}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + src := filepath.Join(dir, ".manifest.tmp") + dst := filepath.Join(dir, "manifest.md") + if err := os.WriteFile(src, []byte("new"), 0o600); err != nil { + t.Fatalf("WriteFile src: %v", err) + } + // dst is deliberately absent: the documented post-failure state is that + // Windows has already removed (1176) or renamed away (1177) the + // destination while the replacement stayed put. + + err := recoverPartialReplace(tc.code, src, dst) + if err == nil { + t.Fatal("recoverPartialReplace must still report the failure; the destination's security descriptor was lost with the destination") + } + if !errors.Is(err, tc.code) { + t.Fatalf("error = %v, want it to wrap %v", err, tc.code) + } + data, readErr := os.ReadFile(dst) + if readErr != nil { + t.Fatalf("the replacement was not recovered to the destination: %v", readErr) + } + if string(data) != "new" { + t.Fatalf("destination content = %q, want the replacement bytes", data) + } + // The replacement must no longer sit at its temporary name, so the + // caller's cleanup cannot delete the only copy of the new content. + if _, err := os.Lstat(src); !os.IsNotExist(err) { + t.Fatalf("the replacement should have been moved off its temporary name: %v", err) + } + }) + } +} + +// TestRecoverPartialReplaceLeavesIntactStatesAlone covers the other half of the +// contract: for failures Microsoft documents as leaving "the replaced and +// replacement files retain their original file names", the destination still +// holds its original content, so the error must pass through untouched and the +// temporary file must stay put for the caller's cleanup to remove. +func TestRecoverPartialReplaceLeavesIntactStatesAlone(t *testing.T) { + for _, tc := range []struct { + name string + code syscall.Errno + }{ + {name: "ERROR_UNABLE_TO_REMOVE_REPLACED", code: errorUnableToRemoveReplaced}, + {name: "ERROR_ACCESS_DENIED", code: syscall.Errno(5)}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + src := filepath.Join(dir, ".manifest.tmp") + dst := filepath.Join(dir, "manifest.md") + if err := os.WriteFile(src, []byte("new"), 0o600); err != nil { + t.Fatalf("WriteFile src: %v", err) + } + if err := os.WriteFile(dst, []byte("old"), 0o600); err != nil { + t.Fatalf("WriteFile dst: %v", err) + } + + if err := recoverPartialReplace(tc.code, src, dst); !errors.Is(err, tc.code) { + t.Fatalf("error = %v, want the original %v unchanged", err, tc.code) + } + if data, err := os.ReadFile(dst); err != nil || string(data) != "old" { + t.Fatalf("destination = %q err=%v, want its original content untouched", data, err) + } + if _, err := os.Lstat(src); err != nil { + t.Fatalf("the temporary file must stay put for the caller to clean up: %v", err) + } + }) + } +} + func creationTime(t *testing.T, path string) syscall.Filetime { t.Helper() info, err := os.Stat(path) diff --git a/internal/specialist/storage_test.go b/internal/specialist/storage_test.go index fb9c5f08f..46594c6b6 100644 --- a/internal/specialist/storage_test.go +++ b/internal/specialist/storage_test.go @@ -177,6 +177,55 @@ func TestWriteSpecialistAtomicPropagatesDirectorySyncError(t *testing.T) { assertNoTemporarySpecialistFiles(t, dir) } +// TestWriteSpecialistAtomicKeepsAReplacementRecoveredByAFailedReplace pins the +// caller-side contract that makes fsutil's partial-ReplaceFileW recovery work +// (jatmn's #757 finding). With no backup file, ReplaceFileW's 1176/1177 failures +// delete the destination and leave the replacement under its temporary name; +// fsutil.recoverPartialReplace moves it into place and STILL reports the error. +// That recovery only holds if this function's failure path leaves the recovered +// destination alone — its deferred cleanup removes the temporary path, so a +// future change removing `path` on error (or recreating/truncating it) would +// silently re-destroy the sole surviving copy of the new content. +// +// Unlike the fsutil-side tests, this one does NOT fail against the pre-fix code: +// it asserts a property of this caller, which the fix did not change. It is a +// guard on the half of the contract that lives here. The replace primitive is +// injected to stage the post-recovery shape (content published at path, error +// returned) on every platform, since ReplaceFileW's partial failures cannot be +// provoked for real. +func TestWriteSpecialistAtomicKeepsAReplacementRecoveredByAFailedReplace(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "safe.md") + if err := os.WriteFile(path, []byte("old content"), 0o600); err != nil { + t.Fatal(err) + } + replaceErr := errors.New("ReplaceFileW could not rename the replacement") + err := writeSpecialistAtomicWith(path, "new content", func(src, dst string) error { + // What recoverPartialReplace leaves behind: the destination Windows + // already removed, the replacement moved into its place, and the failure + // still reported because the destination's descriptor is gone. + if err := os.Remove(dst); err != nil { + t.Fatalf("simulate the destination Windows deleted: %v", err) + } + if err := os.Rename(src, dst); err != nil { + t.Fatalf("simulate the recovery move: %v", err) + } + return replaceErr + }, func(string) error { return nil }) + + if !errors.Is(err, replaceErr) { + t.Fatalf("writeSpecialistAtomicWith error = %v, want it to report %v", err, replaceErr) + } + data, readErr := os.ReadFile(path) + if readErr != nil { + t.Fatalf("the recovered replacement was deleted by the failure cleanup: %v", readErr) + } + if got := string(data); got != "new content" { + t.Fatalf("file content = %q, want the recovered replacement bytes", got) + } + assertNoTemporarySpecialistFiles(t, dir) +} + func assertNoTemporarySpecialistFiles(t *testing.T, dir string) { t.Helper() matches, err := filepath.Glob(filepath.Join(dir, ".specialist-*.tmp")) From c2a07a1dbec9b339df60b2819ae4e5436c9d8e44 Mon Sep 17 00:00:00 2001 From: Amp Date: Sun, 26 Jul 2026 21:05:49 +0000 Subject: [PATCH 5/8] fix(fsutil): preserve Windows replacement backups Use a managed ReplaceFileW backup so partial failures can restore the original file and DACL. Fail closed when atomic replacement is unsupported, retry rollback and backup cleanup, and retain a known backup if rollback cannot complete.\n\nCover unsupported volumes, 1176/1177 semantics, cleanup, rollback retries, and symlink rejection. Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-be8e-75eb-b274-b7e22af373a0 Co-authored-by: Pierre Bruno --- internal/fsutil/replace_windows.go | 182 +++++++++++----- internal/fsutil/replace_windows_test.go | 274 ++++++++++++++++++++---- internal/specialist/storage_test.go | 39 +--- 3 files changed, 370 insertions(+), 125 deletions(-) diff --git a/internal/fsutil/replace_windows.go b/internal/fsutil/replace_windows.go index 35a8a9151..8165248e2 100644 --- a/internal/fsutil/replace_windows.go +++ b/internal/fsutil/replace_windows.go @@ -6,7 +6,9 @@ import ( "errors" "fmt" "os" + "path/filepath" "syscall" + "time" "unsafe" ) @@ -27,10 +29,11 @@ import ( // operation swap is ReplaceFileW's own documented behavior, not this flag's. const replaceFileFlags = 0 +const replaceBackupPattern = ".zero-replace-*.backup" + const ( - // Returned when the volume cannot perform a replace (FAT/exFAT, some network - // redirectors). Such volumes carry no ACL to preserve, so a plain rename is - // an equivalent publish there. + // Returned when the volume or redirector cannot provide ReplaceFileW's atomic, + // descriptor-preserving semantics. Existing destinations must fail closed. errorInvalidFunction = syscall.Errno(1) errorNotSupported = syscall.Errno(50) @@ -68,25 +71,53 @@ var ( // destination untouched and the caller free to clean up its temporary file, // except in the partial-failure states recoverPartialReplace repairs. func replaceExisting(src, dst string) error { - if _, err := os.Lstat(dst); err != nil { + return replaceExistingWith(src, dst, callReplaceFile, nil) +} + +func replaceExistingWith(src, dst string, replace func(string, string, string) error, restore func(string, string) error) error { + info, err := os.Lstat(dst) + if err != nil { if os.IsNotExist(err) { // Nothing to replace and no descriptor to preserve. return os.Rename(src, dst) } return err } - replaced, err := syscall.UTF16PtrFromString(dst) + if info.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("refusing to replace symlink destination: %s", dst) + } + + backup, err := prepareReplaceBackup(dst) + if err != nil { + return fmt.Errorf("prepare replacement backup: %w", err) + } + callErr := replace(dst, src, backup) + if callErr == nil { + return cleanupReplaceBackup(nil, backup) + } + if errors.Is(callErr, errorInvalidFunction) || errors.Is(callErr, errorNotSupported) { + callErr = fmt.Errorf("atomic replacement is not supported for %s: %w", dst, callErr) + } + return recoverPartialReplace(callErr, src, dst, backup, restore) +} + +func callReplaceFile(replacedPath, replacementPath, backupPath string) error { + replaced, err := syscall.UTF16PtrFromString(replacedPath) + if err != nil { + return err + } + replacement, err := syscall.UTF16PtrFromString(replacementPath) if err != nil { return err } - replacement, err := syscall.UTF16PtrFromString(src) + backup, err := syscall.UTF16PtrFromString(backupPath) if err != nil { return err } result, _, callErr := replaceProcReplaceFil.Call( uintptr(unsafe.Pointer(replaced)), uintptr(unsafe.Pointer(replacement)), - 0, // no backup copy + uintptr(unsafe.Pointer(backup)), uintptr(replaceFileFlags), 0, 0, @@ -95,59 +126,96 @@ func replaceExisting(src, dst string) error { return nil } if callErr == nil || errors.Is(callErr, syscall.Errno(0)) { - return fmt.Errorf("replace %s: ReplaceFileW failed", dst) + return fmt.Errorf("replace %s: ReplaceFileW failed", replacedPath) } - if errors.Is(callErr, errorInvalidFunction) || errors.Is(callErr, errorNotSupported) { - return os.Rename(src, dst) + return callErr +} + +func prepareReplaceBackup(dst string) (string, error) { + file, err := os.CreateTemp(filepath.Dir(dst), replaceBackupPattern) + if err != nil { + return "", err + } + path := file.Name() + if err := file.Close(); err != nil { + cleanupErr := removeReplaceBackup(path) + return "", fmt.Errorf("close backup placeholder %s: %w (cleanup error: %v)", path, err, cleanupErr) } - return recoverPartialReplace(callErr, src, dst) + if err := removeReplaceBackup(path); err != nil { + return "", fmt.Errorf("release backup path %s: %w", path, err) + } + return path, nil } -// recoverPartialReplace repairs the on-disk state after a ReplaceFileW failure -// that had already moved or deleted something, then returns the error to report. -// -// We pass no backup file, and Microsoft documents that this makes two failure -// codes leave the DESTINATION GONE while the replacement survives only under its -// original (temporary) name: -// -// - ERROR_UNABLE_TO_MOVE_REPLACEMENT (1176): "If lpBackupFileName was -// specified, the replaced and replacement files retain their original file -// names. Otherwise, the replaced file no longer exists and the replacement -// file exists under its original name." -// - ERROR_UNABLE_TO_MOVE_REPLACEMENT_2 (1177): the replacement survives under -// its original name having already inherited the replaced file's streams and -// attributes, while the replaced file "still exists with a different name" — -// a name only the backup parameter would have reported, so with no backup it -// is unreachable and the destination path is equally empty. -// -// Callers write to a temporary file and remove it unconditionally on failure -// (internal/specialist's writeSpecialistAtomicWith does, via defer), so -// returning either error unrepaired destroys BOTH copies: the destination -// Windows already deleted, and the only surviving copy of the new content. A -// failed `specialist create --force` would leave no manifest at all. +func removeReplaceBackup(path string) error { + var err error + for i := 0; i < 10; i++ { + err = os.Remove(path) + if err == nil || os.IsNotExist(err) { + return nil + } + if os.IsPermission(err) { + // ReplaceFileW can leave the original's read-only attribute on the + // backup. Chmod clears that bit on Windows without changing its DACL. + _ = os.Chmod(path, 0o600) + } + if os.IsPermission(err) || isWindowsSharingOrLockViolation(err) { + time.Sleep(10 * time.Millisecond) + continue + } + break + } + return err +} + +func cleanupReplaceBackup(callErr error, backup string) error { + if err := removeReplaceBackup(backup); err != nil { + if callErr == nil { + // Do not wrap a transient cleanup error: the replacement already + // succeeded, so ReplaceWithRetry must not run the operation again. + return fmt.Errorf("replacement succeeded, but backup %s could not be removed: %v", backup, err) + } + return fmt.Errorf("%w (backup %s could not be removed: %v)", callErr, backup, err) + } + return callErr +} + +// recoverPartialReplace restores the original after ReplaceFileW reports a +// partial failure, then returns the original error. Supplying a managed backup +// changes the dangerous failure states documented by Microsoft: // -// Moving the replacement into place is therefore the recovery. The destination -// path is free — Windows deleted or renamed away whatever was there — so the new -// content lands where it belongs and nothing is left for the caller's cleanup to -// delete. The error is still returned rather than swallowed: the destination's -// original security descriptor died with the destination, so the published file -// carries the temporary file's inherited DACL, and reporting success would be -// exactly the silent ACL widening replaceFileFlags refuses to allow. -func recoverPartialReplace(callErr error, src, dst string) error { - if !errors.Is(callErr, errorUnableToMoveReplacement) && !errors.Is(callErr, errorUnableToMoveReplacement2) { - // Everything else — including ERROR_UNABLE_TO_REMOVE_REPLACED (1175) and - // every unlisted error, for which Microsoft documents that "the replaced and - // replacement files retain their original file names" — leaves the - // destination holding its original content. The caller's temporary-file - // cleanup is then correct and there is nothing to repair. - return callErr - } - if _, err := os.Lstat(src); err != nil { - // No replacement left to recover with; report the original failure. - return callErr - } - if err := os.Rename(src, dst); err != nil { - return fmt.Errorf("replace %s: %w (the replacement survives at %s; recovering it failed: %v)", dst, callErr, src, err) - } - return fmt.Errorf("replace %s: %w (the replacement was moved into place, but the destination's security descriptor was lost with the destination — re-check its permissions)", dst, callErr) +// - ERROR_UNABLE_TO_MOVE_REPLACEMENT (1176) leaves both files under their +// original names, so the failed write can discard src normally. +// - ERROR_UNABLE_TO_MOVE_REPLACEMENT_2 (1177) leaves src in place and moves the +// original destination to backup. Moving backup back to dst rolls the failed +// overwrite back without losing the original content or its DACL. The src +// file, including any streams it inherited during the failed operation, stays +// under the caller's temporary name for the caller's normal cleanup. +func recoverPartialReplace(callErr error, src, dst, backup string, restore func(string, string) error) error { + if !errors.Is(callErr, errorUnableToMoveReplacement2) { + // For 1175, 1176, unsupported volumes, and ordinary failures, Windows + // documents that dst remains at its original name. Remove any redundant + // backup only after confirming that the destination still exists. + if _, err := os.Lstat(dst); err == nil { + return cleanupReplaceBackup(callErr, backup) + } else if !os.IsNotExist(err) { + return fmt.Errorf("replace %s: %w (inspect destination during recovery: %v)", dst, callErr, err) + } + } + + if _, err := os.Lstat(backup); err != nil { + return fmt.Errorf("replace %s: %w (original backup expected at %s is unavailable: %v; replacement remains at %s)", dst, callErr, backup, err, src) + } + if _, err := os.Lstat(dst); err == nil { + return fmt.Errorf("replace %s: %w (destination unexpectedly exists; original remains at backup %s and replacement at %s)", dst, callErr, backup, src) + } else if !os.IsNotExist(err) { + return fmt.Errorf("replace %s: %w (inspect destination before restoring backup %s: %v)", dst, callErr, backup, err) + } + if err := RenameWithRetry(backup, dst, restore); err != nil { + // Only callErr is wrapped. Exposing a sharing violation from the exhausted + // rollback would make the outer ReplaceWithRetry call retry after the + // filesystem was already mutated. + return fmt.Errorf("replace %s: %w (original survives at backup %s; restoring it failed: %v; replacement remains at %s)", dst, callErr, backup, err, src) + } + return fmt.Errorf("replace %s: %w (original was restored; replacement remains at %s for cleanup)", dst, callErr, src) } diff --git a/internal/fsutil/replace_windows_test.go b/internal/fsutil/replace_windows_test.go index 13846aa2c..e2d607697 100644 --- a/internal/fsutil/replace_windows_test.go +++ b/internal/fsutil/replace_windows_test.go @@ -48,6 +48,7 @@ func TestReplaceWithRetryKeepsTheDestinationIdentity(t *testing.T) { if _, err := os.Lstat(src); !os.IsNotExist(err) { t.Fatalf("the replacement file should be consumed by the replace: %v", err) } + assertNoReplaceBackups(t, dir) if got := creationTime(t, dst); got != created { t.Fatalf("creation time = %v, want the replaced file's %v (a rename would not preserve it)", got, created) } @@ -97,6 +98,7 @@ func TestReplaceWithRetryPreservesDestinationDACL(t *testing.T) { if got := describeDACL(t, dst); got != want { t.Fatalf("DACL after replace = %q, want the destination's own %q", got, want) } + assertNoReplaceBackups(t, dir) } // TestReplaceWithRetryPublishesWhenDestinationIsMissing covers the no-destination @@ -115,6 +117,7 @@ func TestReplaceWithRetryPublishesWhenDestinationIsMissing(t *testing.T) { if data, err := os.ReadFile(dst); err != nil || string(data) != "new" { t.Fatalf("destination = %q err=%v, want the replacement bytes", data, err) } + assertNoReplaceBackups(t, dir) } // TestReplaceWithRetryRetriesTransientLockViolation keeps the retry behavior that @@ -163,19 +166,13 @@ func TestReplaceFileFlagsDoNotIgnoreMergeErrors(t *testing.T) { } } -// TestRecoverPartialReplaceRescuesTheReplacement is the regression test for -// jatmn's #757 second P1 finding. With no backup file, ReplaceFileW's 1176/1177 -// failures leave the DESTINATION DELETED and the replacement surviving only under -// its temporary name. Returning those errors unrepaired let the caller's deferred -// temp-file cleanup remove that sole surviving copy, so one failed -// `specialist create --force` destroyed the old manifest AND the new one. -func TestRecoverPartialReplaceRescuesTheReplacement(t *testing.T) { +func TestReplaceExistingRejectsUnsupportedAtomicReplacement(t *testing.T) { for _, tc := range []struct { name string code syscall.Errno }{ - {name: "ERROR_UNABLE_TO_MOVE_REPLACEMENT", code: errorUnableToMoveReplacement}, - {name: "ERROR_UNABLE_TO_MOVE_REPLACEMENT_2", code: errorUnableToMoveReplacement2}, + {name: "ERROR_INVALID_FUNCTION", code: errorInvalidFunction}, + {name: "ERROR_NOT_SUPPORTED", code: errorNotSupported}, } { t.Run(tc.name, func(t *testing.T) { dir := t.TempDir() @@ -184,38 +181,217 @@ func TestRecoverPartialReplaceRescuesTheReplacement(t *testing.T) { if err := os.WriteFile(src, []byte("new"), 0o600); err != nil { t.Fatalf("WriteFile src: %v", err) } - // dst is deliberately absent: the documented post-failure state is that - // Windows has already removed (1176) or renamed away (1177) the - // destination while the replacement stayed put. - - err := recoverPartialReplace(tc.code, src, dst) - if err == nil { - t.Fatal("recoverPartialReplace must still report the failure; the destination's security descriptor was lost with the destination") + if err := os.WriteFile(dst, []byte("old"), 0o600); err != nil { + t.Fatalf("WriteFile dst: %v", err) } + + err := replaceExistingWith(src, dst, func(replaced, replacement, backup string) error { + if replaced != dst || replacement != src { + t.Fatalf("ReplaceFileW paths = (%q, %q), want (%q, %q)", replaced, replacement, dst, src) + } + if filepath.Dir(backup) != dir { + t.Fatalf("backup directory = %q, want sibling directory %q", filepath.Dir(backup), dir) + } + return tc.code + }, nil) if !errors.Is(err, tc.code) { - t.Fatalf("error = %v, want it to wrap %v", err, tc.code) + t.Fatalf("error = %v, want unsupported error %v", err, tc.code) } - data, readErr := os.ReadFile(dst) - if readErr != nil { - t.Fatalf("the replacement was not recovered to the destination: %v", readErr) + if !strings.Contains(err.Error(), "atomic replacement is not supported") { + t.Fatalf("error = %v, want an atomic-replacement explanation", err) } - if string(data) != "new" { - t.Fatalf("destination content = %q, want the replacement bytes", data) - } - // The replacement must no longer sit at its temporary name, so the - // caller's cleanup cannot delete the only copy of the new content. - if _, err := os.Lstat(src); !os.IsNotExist(err) { - t.Fatalf("the replacement should have been moved off its temporary name: %v", err) + assertFileContent(t, dst, "old") + assertFileContent(t, src, "new") + assertNoReplaceBackups(t, dir) + }) + } +} + +func TestReplaceExistingCleansManagedBackup(t *testing.T) { + dir := t.TempDir() + src := filepath.Join(dir, ".manifest.tmp") + dst := filepath.Join(dir, "manifest.md") + if err := os.WriteFile(src, []byte("new"), 0o600); err != nil { + t.Fatalf("WriteFile src: %v", err) + } + if err := os.WriteFile(dst, []byte("old"), 0o600); err != nil { + t.Fatalf("WriteFile dst: %v", err) + } + + var backupPath string + err := replaceExistingWith(src, dst, func(replaced, replacement, backup string) error { + backupPath = backup + if matched, matchErr := filepath.Match(replaceBackupPattern, filepath.Base(backup)); matchErr != nil || !matched { + t.Fatalf("backup path = %q, want pattern %q (match error: %v)", backup, replaceBackupPattern, matchErr) + } + if _, statErr := os.Lstat(backup); !os.IsNotExist(statErr) { + t.Fatalf("backup path must be vacant before ReplaceFileW: %v", statErr) + } + if renameErr := os.Rename(replaced, backup); renameErr != nil { + t.Fatalf("stage backup: %v", renameErr) + } + // Windows refuses to remove a read-only file. The cleanup path must clear + // that attribute without changing the backup's DACL, then remove it. + if chmodErr := os.Chmod(backup, 0o400); chmodErr != nil { + t.Fatalf("make backup read-only: %v", chmodErr) + } + return os.Rename(replacement, replaced) + }, nil) + if err != nil { + t.Fatalf("replaceExistingWith: %v", err) + } + assertFileContent(t, dst, "new") + if _, err := os.Lstat(src); !os.IsNotExist(err) { + t.Fatalf("replacement source should be consumed: %v", err) + } + if _, err := os.Lstat(backupPath); !os.IsNotExist(err) { + t.Fatalf("managed backup should be removed: %v", err) + } + assertNoReplaceBackups(t, dir) +} + +func TestReplaceExistingKeeps1176FilesAtOriginalNames(t *testing.T) { + dir := t.TempDir() + src := filepath.Join(dir, ".manifest.tmp") + dst := filepath.Join(dir, "manifest.md") + if err := os.WriteFile(src, []byte("new"), 0o600); err != nil { + t.Fatalf("WriteFile src: %v", err) + } + if err := os.WriteFile(dst, []byte("old"), 0o600); err != nil { + t.Fatalf("WriteFile dst: %v", err) + } + + err := replaceExistingWith(src, dst, func(_, _, _ string) error { + // With a backup name, Microsoft documents that 1176 leaves the replaced + // and replacement files under their original names. + return errorUnableToMoveReplacement + }, nil) + if !errors.Is(err, errorUnableToMoveReplacement) { + t.Fatalf("error = %v, want %v", err, errorUnableToMoveReplacement) + } + assertFileContent(t, dst, "old") + assertFileContent(t, src, "new") + assertNoReplaceBackups(t, dir) +} + +func TestReplaceExistingRollsBack1177AndRetriesTransientRestore(t *testing.T) { + dir := t.TempDir() + src := filepath.Join(dir, ".manifest.tmp") + dst := filepath.Join(dir, "manifest.md") + if err := os.WriteFile(src, []byte("new"), 0o600); err != nil { + t.Fatalf("WriteFile src: %v", err) + } + if err := os.WriteFile(dst, []byte("old"), 0o600); err != nil { + t.Fatalf("WriteFile dst: %v", err) + } + + var backupPath string + restoreAttempts := 0 + err := replaceExistingWith(src, dst, func(replaced, _, backup string) error { + backupPath = backup + if renameErr := os.Rename(replaced, backup); renameErr != nil { + t.Fatalf("stage documented 1177 backup: %v", renameErr) + } + return errorUnableToMoveReplacement2 + }, func(backup, destination string) error { + restoreAttempts++ + if restoreAttempts < 3 { + return &os.PathError{Op: "rename", Path: backup, Err: syscall.Errno(32)} + } + return os.Rename(backup, destination) + }) + if !errors.Is(err, errorUnableToMoveReplacement2) { + t.Fatalf("error = %v, want %v", err, errorUnableToMoveReplacement2) + } + if restoreAttempts != 3 { + t.Fatalf("restore attempts = %d, want transient lock failures retried", restoreAttempts) + } + assertFileContent(t, dst, "old") + assertFileContent(t, src, "new") + if _, err := os.Lstat(backupPath); !os.IsNotExist(err) { + t.Fatalf("backup should be consumed by rollback: %v", err) + } + assertNoReplaceBackups(t, dir) +} + +func TestReplaceExistingPreserves1177BackupWhenRollbackFails(t *testing.T) { + dir := t.TempDir() + src := filepath.Join(dir, ".manifest.tmp") + dst := filepath.Join(dir, "manifest.md") + if err := os.WriteFile(src, []byte("new"), 0o600); err != nil { + t.Fatalf("WriteFile src: %v", err) + } + if err := os.WriteFile(dst, []byte("old"), 0o600); err != nil { + t.Fatalf("WriteFile dst: %v", err) + } + + var backupPath string + replaceAttempts := 0 + err := ReplaceWithRetry(src, dst, func(src, dst string) error { + replaceAttempts++ + return replaceExistingWith(src, dst, func(replaced, _, backup string) error { + backupPath = backup + if renameErr := os.Rename(replaced, backup); renameErr != nil { + t.Fatalf("stage documented 1177 backup: %v", renameErr) } + return errorUnableToMoveReplacement2 + }, func(_, _ string) error { + return &os.PathError{Op: "rename", Path: backupPath, Err: syscall.Errno(32)} }) + }) + if !errors.Is(err, errorUnableToMoveReplacement2) { + t.Fatalf("error = %v, want %v", err, errorUnableToMoveReplacement2) + } + if replaceAttempts != 1 { + t.Fatalf("replace attempts = %d, want no retry after a partial failure", replaceAttempts) + } + if os.IsPermission(err) || isWindowsSharingOrLockViolation(err) { + t.Fatalf("partial-failure error exposes the rollback lock error to the outer retry loop: %v", err) + } + if !strings.Contains(err.Error(), backupPath) { + t.Fatalf("error = %v, want retained backup path %q", err, backupPath) + } + if _, err := os.Lstat(dst); !os.IsNotExist(err) { + t.Fatalf("destination should remain absent after failed rollback: %v", err) + } + assertFileContent(t, backupPath, "old") + assertFileContent(t, src, "new") +} + +func TestReplaceExistingRejectsSymlinkDestination(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "target.md") + dst := filepath.Join(dir, "manifest.md") + src := filepath.Join(dir, ".manifest.tmp") + if err := os.WriteFile(target, []byte("old"), 0o600); err != nil { + t.Fatalf("WriteFile target: %v", err) + } + if err := os.WriteFile(src, []byte("new"), 0o600); err != nil { + t.Fatalf("WriteFile src: %v", err) + } + if err := os.Symlink(target, dst); err != nil { + t.Skipf("symlink unavailable: %v", err) + } + + called := false + err := replaceExistingWith(src, dst, func(_, _, _ string) error { + called = true + return nil + }, nil) + if err == nil || !strings.Contains(err.Error(), "refusing to replace symlink") { + t.Fatalf("symlink replacement error = %v", err) + } + if called { + t.Fatal("ReplaceFileW callback was called for a symlink destination") + } + assertFileContent(t, target, "old") + assertFileContent(t, src, "new") + if info, err := os.Lstat(dst); err != nil || info.Mode()&os.ModeSymlink == 0 { + t.Fatalf("destination symlink was changed: info=%v err=%v", info, err) } + assertNoReplaceBackups(t, dir) } -// TestRecoverPartialReplaceLeavesIntactStatesAlone covers the other half of the -// contract: for failures Microsoft documents as leaving "the replaced and -// replacement files retain their original file names", the destination still -// holds its original content, so the error must pass through untouched and the -// temporary file must stay put for the caller's cleanup to remove. func TestRecoverPartialReplaceLeavesIntactStatesAlone(t *testing.T) { for _, tc := range []struct { name string @@ -228,6 +404,7 @@ func TestRecoverPartialReplaceLeavesIntactStatesAlone(t *testing.T) { dir := t.TempDir() src := filepath.Join(dir, ".manifest.tmp") dst := filepath.Join(dir, "manifest.md") + backup := filepath.Join(dir, ".zero-replace-test.backup") if err := os.WriteFile(src, []byte("new"), 0o600); err != nil { t.Fatalf("WriteFile src: %v", err) } @@ -235,19 +412,40 @@ func TestRecoverPartialReplaceLeavesIntactStatesAlone(t *testing.T) { t.Fatalf("WriteFile dst: %v", err) } - if err := recoverPartialReplace(tc.code, src, dst); !errors.Is(err, tc.code) { + if err := recoverPartialReplace(tc.code, src, dst, backup, nil); !errors.Is(err, tc.code) { t.Fatalf("error = %v, want the original %v unchanged", err, tc.code) } - if data, err := os.ReadFile(dst); err != nil || string(data) != "old" { - t.Fatalf("destination = %q err=%v, want its original content untouched", data, err) - } - if _, err := os.Lstat(src); err != nil { - t.Fatalf("the temporary file must stay put for the caller to clean up: %v", err) + assertFileContent(t, dst, "old") + assertFileContent(t, src, "new") + if _, err := os.Lstat(backup); !os.IsNotExist(err) { + t.Fatalf("unexpected backup residue: %v", err) } }) } } +func assertFileContent(t *testing.T, path, want string) { + t.Helper() + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile %s: %v", path, err) + } + if string(data) != want { + t.Fatalf("content of %s = %q, want %q", path, data, want) + } +} + +func assertNoReplaceBackups(t *testing.T, dir string) { + t.Helper() + matches, err := filepath.Glob(filepath.Join(dir, replaceBackupPattern)) + if err != nil { + t.Fatalf("Glob replacement backups: %v", err) + } + if len(matches) != 0 { + t.Fatalf("replacement backups remain: %v", matches) + } +} + func creationTime(t *testing.T, path string) syscall.Filetime { t.Helper() info, err := os.Stat(path) diff --git a/internal/specialist/storage_test.go b/internal/specialist/storage_test.go index 46594c6b6..22438bb45 100644 --- a/internal/specialist/storage_test.go +++ b/internal/specialist/storage_test.go @@ -177,39 +177,18 @@ func TestWriteSpecialistAtomicPropagatesDirectorySyncError(t *testing.T) { assertNoTemporarySpecialistFiles(t, dir) } -// TestWriteSpecialistAtomicKeepsAReplacementRecoveredByAFailedReplace pins the -// caller-side contract that makes fsutil's partial-ReplaceFileW recovery work -// (jatmn's #757 finding). With no backup file, ReplaceFileW's 1176/1177 failures -// delete the destination and leave the replacement under its temporary name; -// fsutil.recoverPartialReplace moves it into place and STILL reports the error. -// That recovery only holds if this function's failure path leaves the recovered -// destination alone — its deferred cleanup removes the temporary path, so a -// future change removing `path` on error (or recreating/truncating it) would -// silently re-destroy the sole surviving copy of the new content. -// -// Unlike the fsutil-side tests, this one does NOT fail against the pre-fix code: -// it asserts a property of this caller, which the fix did not change. It is a -// guard on the half of the contract that lives here. The replace primitive is -// injected to stage the post-recovery shape (content published at path, error -// returned) on every platform, since ReplaceFileW's partial failures cannot be -// provoked for real. -func TestWriteSpecialistAtomicKeepsAReplacementRecoveredByAFailedReplace(t *testing.T) { +// TestWriteSpecialistAtomicKeepsTheOriginalAfterFailedReplace pins the caller +// side of Windows partial-failure rollback: fsutil restores the original at dst, +// reports the ReplaceFileW error, and leaves src for this function's deferred +// temporary-file cleanup. +func TestWriteSpecialistAtomicKeepsTheOriginalAfterFailedReplace(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "safe.md") if err := os.WriteFile(path, []byte("old content"), 0o600); err != nil { t.Fatal(err) } replaceErr := errors.New("ReplaceFileW could not rename the replacement") - err := writeSpecialistAtomicWith(path, "new content", func(src, dst string) error { - // What recoverPartialReplace leaves behind: the destination Windows - // already removed, the replacement moved into its place, and the failure - // still reported because the destination's descriptor is gone. - if err := os.Remove(dst); err != nil { - t.Fatalf("simulate the destination Windows deleted: %v", err) - } - if err := os.Rename(src, dst); err != nil { - t.Fatalf("simulate the recovery move: %v", err) - } + err := writeSpecialistAtomicWith(path, "new content", func(_, _ string) error { return replaceErr }, func(string) error { return nil }) @@ -218,10 +197,10 @@ func TestWriteSpecialistAtomicKeepsAReplacementRecoveredByAFailedReplace(t *test } data, readErr := os.ReadFile(path) if readErr != nil { - t.Fatalf("the recovered replacement was deleted by the failure cleanup: %v", readErr) + t.Fatalf("the original destination was lost after failed replacement: %v", readErr) } - if got := string(data); got != "new content" { - t.Fatalf("file content = %q, want the recovered replacement bytes", got) + if got := string(data); got != "old content" { + t.Fatalf("file content = %q, want the original bytes", got) } assertNoTemporarySpecialistFiles(t, dir) } From 38105c03918e8dfe63f9f5aa96e0b43da56c8859 Mon Sep 17 00:00:00 2001 From: Amp Date: Tue, 28 Jul 2026 10:40:37 +0000 Subject: [PATCH 6/8] fix(specialist): report committed cleanup warnings Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno --- internal/cli/specialist.go | 11 ++++- internal/cli/specialist_test.go | 37 +++++++++++++++++ internal/fsutil/rename.go | 24 +++++++++++ internal/fsutil/replace_windows.go | 14 ++++--- internal/fsutil/replace_windows_test.go | 43 +++++++++++++++++++ internal/specialist/generate_tool.go | 14 ++++++- internal/specialist/generate_tool_test.go | 31 ++++++++++++++ internal/specialist/storage.go | 24 ++++++++--- internal/specialist/storage_test.go | 50 +++++++++++++++++++++-- 9 files changed, 231 insertions(+), 17 deletions(-) diff --git a/internal/cli/specialist.go b/internal/cli/specialist.go index f77887714..b5014d6ef 100644 --- a/internal/cli/specialist.go +++ b/internal/cli/specialist.go @@ -286,7 +286,11 @@ func runSpecialistCreate(paths specialist.Paths, name string, options specialist if err != nil { return writeExecUsageError(stderr, err.Error()) } - if options.json { + return writeSpecialistCreateResult(manifest, options.json, stdout, stderr) +} + +func writeSpecialistCreateResult(manifest specialist.Manifest, jsonOutput bool, stdout io.Writer, stderr io.Writer) int { + if jsonOutput { if err := writePrettyJSON(stdout, manifest); err != nil { return exitCrash } @@ -295,6 +299,11 @@ func runSpecialistCreate(paths specialist.Paths, name string, options specialist if _, err := fmt.Fprintf(stdout, "Created specialist %s at %s\n", manifest.Metadata.Name, manifest.FilePath); err != nil { return exitCrash } + for _, warning := range manifest.Warnings { + if _, err := fmt.Fprintf(stderr, "warning: %s\n", warning); err != nil { + return exitCrash + } + } return exitSuccess } diff --git a/internal/cli/specialist_test.go b/internal/cli/specialist_test.go index cd20c22f8..ab01e22e8 100644 --- a/internal/cli/specialist_test.go +++ b/internal/cli/specialist_test.go @@ -10,6 +10,7 @@ import ( "testing" "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/specialist" ) func TestRunSpecialistListShowAndPath(t *testing.T) { @@ -187,6 +188,42 @@ func TestRunSpecialistCreateDeleteAndEdit(t *testing.T) { } } +func TestWriteSpecialistCreateResultSurfacesWarningsOnlyForHumanOutput(t *testing.T) { + manifest := specialist.Manifest{ + Metadata: specialist.Metadata{Name: "triage"}, + FilePath: "/specialists/triage.md", + Warnings: []string{"replacement backup /specialists/.triage.bak retained: access denied"}, + } + + var stdout bytes.Buffer + var stderr bytes.Buffer + if code := writeSpecialistCreateResult(manifest, false, &stdout, &stderr); code != exitSuccess { + t.Fatalf("human output exit code = %d", code) + } + if got, want := stdout.String(), "Created specialist triage at /specialists/triage.md\n"; got != want { + t.Fatalf("stdout = %q, want %q", got, want) + } + if got, want := stderr.String(), "warning: replacement backup /specialists/.triage.bak retained: access denied\n"; got != want { + t.Fatalf("stderr = %q, want %q", got, want) + } + + stdout.Reset() + stderr.Reset() + if code := writeSpecialistCreateResult(manifest, true, &stdout, &stderr); code != exitSuccess { + t.Fatalf("JSON output exit code = %d", code) + } + var payload specialist.Manifest + if err := json.Unmarshal(stdout.Bytes(), &payload); err != nil { + t.Fatalf("decode JSON: %v\n%s", err, stdout.String()) + } + if len(payload.Warnings) != 1 || payload.Warnings[0] != manifest.Warnings[0] { + t.Fatalf("JSON warnings = %#v", payload.Warnings) + } + if stderr.Len() != 0 { + t.Fatalf("JSON warning duplicated to stderr: %q", stderr.String()) + } +} + func TestRunSpecialistEditRejectsSymlink(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("symlink creation needs extra privileges on Windows") diff --git a/internal/fsutil/rename.go b/internal/fsutil/rename.go index 889a60604..7dd22e3fc 100644 --- a/internal/fsutil/rename.go +++ b/internal/fsutil/rename.go @@ -3,12 +3,30 @@ package fsutil import ( "errors" + "fmt" "os" "runtime" "syscall" "time" ) +// CommittedReplacementCleanupError reports that a replacement was committed, +// but the old destination retained at BackupPath could not be removed. Callers +// must treat the replacement itself as successful and surface the cleanup +// problem separately. +type CommittedReplacementCleanupError struct { + BackupPath string + Cause error +} + +func (err *CommittedReplacementCleanupError) Error() string { + return fmt.Sprintf("replacement committed, but backup %s could not be removed: %v", err.BackupPath, err.Cause) +} + +func (err *CommittedReplacementCleanupError) Unwrap() error { + return err.Cause +} + // ReplaceWithRetry publishes src over dst using the platform's replacement // primitive, retrying on the same transient Windows lock errors RenameWithRetry // handles. Prefer it over RenameWithRetry when dst may already exist and is @@ -40,6 +58,12 @@ func RenameWithRetry(src, dst string, rename func(src, dst string) error) error if err == nil { return nil } + var committed *CommittedReplacementCleanupError + if errors.As(err, &committed) { + // The source has already been consumed. In particular, never retry if + // the cleanup cause itself is a transient Windows sharing violation. + break + } if runtime.GOOS == "windows" { if os.IsPermission(err) || isWindowsSharingOrLockViolation(err) { time.Sleep(10 * time.Millisecond) diff --git a/internal/fsutil/replace_windows.go b/internal/fsutil/replace_windows.go index 8165248e2..f64f2fa9f 100644 --- a/internal/fsutil/replace_windows.go +++ b/internal/fsutil/replace_windows.go @@ -75,6 +75,10 @@ func replaceExisting(src, dst string) error { } func replaceExistingWith(src, dst string, replace func(string, string, string) error, restore func(string, string) error) error { + return replaceExistingWithCleanup(src, dst, replace, restore, removeReplaceBackup) +} + +func replaceExistingWithCleanup(src, dst string, replace func(string, string, string) error, restore func(string, string) error, removeBackup func(string) error) error { info, err := os.Lstat(dst) if err != nil { if os.IsNotExist(err) { @@ -93,7 +97,10 @@ func replaceExistingWith(src, dst string, replace func(string, string, string) e } callErr := replace(dst, src, backup) if callErr == nil { - return cleanupReplaceBackup(nil, backup) + if err := removeBackup(backup); err != nil { + return &CommittedReplacementCleanupError{BackupPath: backup, Cause: err} + } + return nil } if errors.Is(callErr, errorInvalidFunction) || errors.Is(callErr, errorNotSupported) { callErr = fmt.Errorf("atomic replacement is not supported for %s: %w", dst, callErr) @@ -170,11 +177,6 @@ func removeReplaceBackup(path string) error { func cleanupReplaceBackup(callErr error, backup string) error { if err := removeReplaceBackup(backup); err != nil { - if callErr == nil { - // Do not wrap a transient cleanup error: the replacement already - // succeeded, so ReplaceWithRetry must not run the operation again. - return fmt.Errorf("replacement succeeded, but backup %s could not be removed: %v", backup, err) - } return fmt.Errorf("%w (backup %s could not be removed: %v)", callErr, backup, err) } return callErr diff --git a/internal/fsutil/replace_windows_test.go b/internal/fsutil/replace_windows_test.go index e2d607697..17f61ab5e 100644 --- a/internal/fsutil/replace_windows_test.go +++ b/internal/fsutil/replace_windows_test.go @@ -250,6 +250,49 @@ func TestReplaceExistingCleansManagedBackup(t *testing.T) { assertNoReplaceBackups(t, dir) } +func TestReplaceWithRetryDoesNotRetryCommittedReplacementWhenBackupCleanupFails(t *testing.T) { + dir := t.TempDir() + src := filepath.Join(dir, ".manifest.tmp") + dst := filepath.Join(dir, "manifest.md") + if err := os.WriteFile(src, []byte("new"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(dst, []byte("old"), 0o600); err != nil { + t.Fatal(err) + } + + attempts := 0 + cleanupErr := &os.PathError{Op: "remove", Path: "managed backup", Err: syscall.Errno(32)} + var backupPath string + err := ReplaceWithRetry(src, dst, func(src, dst string) error { + attempts++ + return replaceExistingWithCleanup(src, dst, func(replaced, replacement, backup string) error { + backupPath = backup + if err := os.Rename(replaced, backup); err != nil { + return err + } + return os.Rename(replacement, replaced) + }, nil, func(got string) error { + if got != backupPath { + t.Fatalf("cleanup backup path = %q, want %q", got, backupPath) + } + return cleanupErr + }) + }) + var outcome *CommittedReplacementCleanupError + if !errors.As(err, &outcome) { + t.Fatalf("error = %v, want committed cleanup outcome", err) + } + if outcome.BackupPath != backupPath || !errors.Is(outcome, syscall.Errno(32)) { + t.Fatalf("outcome = %#v, want backup %q and sharing violation", outcome, backupPath) + } + if attempts != 1 { + t.Fatalf("replacement attempts = %d, want 1", attempts) + } + assertFileContent(t, dst, "new") + assertFileContent(t, backupPath, "old") +} + func TestReplaceExistingKeeps1176FilesAtOriginalNames(t *testing.T) { dir := t.TempDir() src := filepath.Join(dir, ".manifest.tmp") diff --git a/internal/specialist/generate_tool.go b/internal/specialist/generate_tool.go index a9b523b7f..b96c4596c 100644 --- a/internal/specialist/generate_tool.go +++ b/internal/specialist/generate_tool.go @@ -104,9 +104,21 @@ func (tool *GenerateTool) Run(ctx context.Context, args map[string]any) tools.Re if err != nil { return taskError(err) } + return generateToolResult(manifest) +} + +func generateToolResult(manifest Manifest) tools.Result { + lines := []string{ + fmt.Sprintf("specialist: %s", manifest.Metadata.Name), + fmt.Sprintf("location: %s", manifest.Location), + fmt.Sprintf("path: %s", manifest.FilePath), + } + for _, warning := range manifest.Warnings { + lines = append(lines, "warning: "+warning) + } return tools.Result{ Status: tools.StatusOK, - Output: fmt.Sprintf("specialist: %s\nlocation: %s\npath: %s", manifest.Metadata.Name, manifest.Location, manifest.FilePath), + Output: strings.Join(lines, "\n"), Meta: map[string]string{ "name": manifest.Metadata.Name, "location": string(manifest.Location), diff --git a/internal/specialist/generate_tool_test.go b/internal/specialist/generate_tool_test.go index 79b064b38..fdeeabe9f 100644 --- a/internal/specialist/generate_tool_test.go +++ b/internal/specialist/generate_tool_test.go @@ -40,6 +40,37 @@ func TestGenerateToolCreatesSpecialist(t *testing.T) { } } +func TestGenerateToolResultAppendsWarnings(t *testing.T) { + manifest := Manifest{ + Metadata: Metadata{Name: "api-review"}, + Location: LocationProject, + FilePath: "/project/.zero/specialists/api-review.md", + Warnings: []string{"specialist was updated, but replacement backup /project/.zero/specialists/.api-review.bak could not be removed: access denied"}, + } + + result := generateToolResult(manifest) + + want := "specialist: api-review\nlocation: project\npath: /project/.zero/specialists/api-review.md\nwarning: " + manifest.Warnings[0] + if result.Status != tools.StatusOK || result.Output != want { + t.Fatalf("result = %#v, want output %q", result, want) + } +} + +func TestGenerateToolResultNormalOutputUnchanged(t *testing.T) { + manifest := Manifest{ + Metadata: Metadata{Name: "api-review"}, + Location: LocationProject, + FilePath: "/project/.zero/specialists/api-review.md", + } + + result := generateToolResult(manifest) + + want := "specialist: api-review\nlocation: project\npath: /project/.zero/specialists/api-review.md" + if result.Output != want { + t.Fatalf("output = %q, want %q", result.Output, want) + } +} + func TestGenerateToolDerivesNameAndDefaultPrompt(t *testing.T) { projectDir := filepath.Join(t.TempDir(), "project") tool := NewGenerateTool(NewStorage(Paths{ProjectDir: projectDir})) diff --git a/internal/specialist/storage.go b/internal/specialist/storage.go index 719eb8c26..3a7b4ef4e 100644 --- a/internal/specialist/storage.go +++ b/internal/specialist/storage.go @@ -1,6 +1,7 @@ package specialist import ( + "errors" "fmt" "os" "path/filepath" @@ -12,7 +13,8 @@ import ( ) type Storage struct { - paths Paths + paths Paths + writeAtomic func(string, string) error } type CreateInput struct { @@ -71,7 +73,16 @@ func (storage *Storage) Create(input CreateInput) (Manifest, error) { return Manifest{}, fmt.Errorf("create specialist directory: %w", err) } if input.Overwrite { - if err := writeSpecialistAtomic(path, content); err != nil { + writeAtomic := storage.writeAtomic + if writeAtomic == nil { + writeAtomic = writeSpecialistAtomic + } + if err := writeAtomic(path, content); err != nil { + var cleanupErr *fsutil.CommittedReplacementCleanupError + if errors.As(err, &cleanupErr) { + manifest.Warnings = append(manifest.Warnings, fmt.Sprintf("specialist was updated, but replacement backup %s could not be removed: %v", cleanupErr.BackupPath, cleanupErr.Cause)) + return manifest, nil + } return Manifest{}, err } return manifest, nil @@ -138,9 +149,10 @@ func writeSpecialistAtomicWith(path string, content string, rename func(string, if err := fsutil.ReplaceWithRetry(tempPath, path, rename); err != nil { return fmt.Errorf("replace specialist file: %w", err) } - if err := syncDir(filepath.Dir(path)); err != nil { - return fmt.Errorf("sync specialist directory: %w", err) - } + // The replacement is committed at this point. As in the sessions store, + // opening/fsyncing the parent directory only improves crash durability and + // must not turn a successful overwrite into an API failure. + _ = syncDir(filepath.Dir(path)) return nil } @@ -150,7 +162,7 @@ func syncSpecialistDir(path string) error { } dir, err := os.Open(path) if err != nil { - return err + return nil } if err := dir.Sync(); err != nil { _ = dir.Close() diff --git a/internal/specialist/storage_test.go b/internal/specialist/storage_test.go index 22438bb45..6283d0bb5 100644 --- a/internal/specialist/storage_test.go +++ b/internal/specialist/storage_test.go @@ -8,6 +8,8 @@ import ( "strings" "syscall" "testing" + + "github.com/Gitlawb/zero/internal/fsutil" ) func TestStorageCreateWritesValidManifestAndDeleteRemovesIt(t *testing.T) { @@ -128,6 +130,30 @@ func TestStorageCreateForceAtomicallyReplacesFile(t *testing.T) { assertNoTemporarySpecialistFiles(t, userDir) } +func TestStorageCreateReturnsManifestWarningAfterCommittedCleanupFailure(t *testing.T) { + userDir := t.TempDir() + backupPath := filepath.Join(userDir, ".zero-replace-old.backup") + cleanupErr := &os.PathError{Op: "remove", Path: backupPath, Err: syscall.Errno(32)} + storage := NewStorage(Paths{UserDir: userDir}) + storage.writeAtomic = func(path, content string) error { + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + return err + } + return &fsutil.CommittedReplacementCleanupError{BackupPath: backupPath, Cause: cleanupErr} + } + + manifest, err := storage.Create(CreateInput{Name: "safe", Description: "Safe", Overwrite: true}) + if err != nil { + t.Fatalf("Create returned error after committed replacement: %v", err) + } + if len(manifest.Warnings) != 1 || !strings.Contains(manifest.Warnings[0], backupPath) || !strings.Contains(manifest.Warnings[0], cleanupErr.Error()) { + t.Fatalf("warnings = %#v, want backup path and cleanup problem", manifest.Warnings) + } + if data, readErr := os.ReadFile(manifest.FilePath); readErr != nil || string(data) != FormatMarkdown(manifest) { + t.Fatalf("committed specialist content = %q, error = %v", data, readErr) + } +} + func TestWriteSpecialistAtomicRetriesTransientWindowsRename(t *testing.T) { if runtime.GOOS != "windows" { t.Skip("rename retries are Windows-specific") @@ -161,22 +187,40 @@ func TestWriteSpecialistAtomicRetriesTransientWindowsRename(t *testing.T) { assertNoTemporarySpecialistFiles(t, dir) } -func TestWriteSpecialistAtomicPropagatesDirectorySyncError(t *testing.T) { +func TestWriteSpecialistAtomicIgnoresDirectorySyncErrorAfterCommit(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "safe.md") syncErr := errors.New("sync failed") + called := false err := writeSpecialistAtomicWith(path, "new content", nil, func(got string) error { + called = true if got != dir { t.Fatalf("sync directory = %q, want %q", got, dir) } return syncErr }) - if !errors.Is(err, syncErr) { - t.Fatalf("writeSpecialistAtomicWith error = %v, want %v", err, syncErr) + if err != nil { + t.Fatalf("committed replacement returned sync error: %v", err) + } + if !called { + t.Fatal("directory sync was not attempted") + } + if data, readErr := os.ReadFile(path); readErr != nil || string(data) != "new content" { + t.Fatalf("committed content = %q, error = %v", data, readErr) } assertNoTemporarySpecialistFiles(t, dir) } +func TestSyncSpecialistDirIgnoresOpenFailure(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("directory sync is not attempted on Windows") + } + missing := filepath.Join(t.TempDir(), "missing") + if err := syncSpecialistDir(missing); err != nil { + t.Fatalf("directory open failure should be best-effort, got: %v", err) + } +} + // TestWriteSpecialistAtomicKeepsTheOriginalAfterFailedReplace pins the caller // side of Windows partial-failure rollback: fsutil restores the original at dst, // reports the ReplaceFileW error, and leaves src for this function's deferred From 05358e8a27ef95c91a63517451d68c11e1abda28 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Tue, 28 Jul 2026 22:09:10 +0200 Subject: [PATCH 7/8] docs(specialist): describe what a failed overwrite actually leaves behind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Windows partial-replace recovery errors told an operator the replacement "remains at" the caller's temporary path, but the only caller removes that file in a deferred cleanup before the error ever surfaces — it named a file that was already gone. Recovery from a failed overwrite is always about the original, so the errors now describe only what the replace itself left on disk: the state of the destination and of the managed backup. The replacement path is no longer a parameter, since nothing reports it. The one terminal state that needs a human — 1177 whose rollback exhausted its retries — now says so outright. The original then exists only under an opaque .zero-replace-*.backup name, and the loader reads *.md, so the specialist stays invisible until somebody moves it back. The error names both paths and the action; docs/SPECIALISTS.md walks through it; and loadDirectory records why the non-.md siblings an interrupted overwrite can leave are skipped rather than guessed at. Co-Authored-By: Claude Opus 5 (1M context) --- docs/SPECIALISTS.md | 34 +++++++++++++++++++++++ internal/fsutil/replace_windows.go | 37 ++++++++++++++++++------- internal/fsutil/replace_windows_test.go | 28 ++++++++++++++++++- internal/specialist/manifest.go | 8 ++++++ internal/specialist/storage.go | 6 ++++ 5 files changed, 102 insertions(+), 11 deletions(-) diff --git a/docs/SPECIALISTS.md b/docs/SPECIALISTS.md index 7435a56e0..f11ca72f3 100644 --- a/docs/SPECIALISTS.md +++ b/docs/SPECIALISTS.md @@ -138,3 +138,37 @@ output or stop a still-running task by id. If Zero is restarted while a background task is still marked `running`, the new manager marks that task `error` and clears its PID. This avoids sending `TaskStop` to a stale PID that may now belong to an unrelated process. + +## Recovering an Interrupted Overwrite + +Overwriting a specialist (`--force`) publishes the new file atomically, so an +interrupted write leaves either the old manifest or the new one — never a +half-written file. Windows has one rare exception worth knowing about. + +There, the swap goes through `ReplaceFileW`, which preserves the destination's +security descriptor instead of silently replacing it with the directory's +inherited one. If it fails with `ERROR_UNABLE_TO_MOVE_REPLACEMENT_2` (1177), +Zero has already moved your original aside and tries to move it back. That +rollback almost always succeeds, and the failed write changes nothing. + +If the rollback itself fails — typically because another process is holding a +lock on the file — the original is not lost, but it is left under a name Zero +does not read: + +```text +/.zero-replace-.backup +``` + +Only `*.md` files are loaded as specialists, so until that file is renamed the +specialist will not appear in `zero specialist list` or resolve by name. The +error Zero prints names both paths; recover by closing whatever holds the lock +and renaming the backup back: + +```powershell +Move-Item .zero-replace-.backup .md +``` + +A `.zero-replace-*.backup` can also linger after a *successful* overwrite if the +backup could not be deleted afterward. That case is reported as a warning rather +than an error — the new manifest is already in place, and the leftover file is +safe to delete. diff --git a/internal/fsutil/replace_windows.go b/internal/fsutil/replace_windows.go index f64f2fa9f..2efce011e 100644 --- a/internal/fsutil/replace_windows.go +++ b/internal/fsutil/replace_windows.go @@ -105,7 +105,7 @@ func replaceExistingWithCleanup(src, dst string, replace func(string, string, st if errors.Is(callErr, errorInvalidFunction) || errors.Is(callErr, errorNotSupported) { callErr = fmt.Errorf("atomic replacement is not supported for %s: %w", dst, callErr) } - return recoverPartialReplace(callErr, src, dst, backup, restore) + return recoverPartialReplace(callErr, dst, backup, restore) } func callReplaceFile(replacedPath, replacementPath, backupPath string) error { @@ -190,10 +190,18 @@ func cleanupReplaceBackup(callErr error, backup string) error { // original names, so the failed write can discard src normally. // - ERROR_UNABLE_TO_MOVE_REPLACEMENT_2 (1177) leaves src in place and moves the // original destination to backup. Moving backup back to dst rolls the failed -// overwrite back without losing the original content or its DACL. The src -// file, including any streams it inherited during the failed operation, stays -// under the caller's temporary name for the caller's normal cleanup. -func recoverPartialReplace(callErr error, src, dst, backup string, restore func(string, string) error) error { +// overwrite back without losing the original content or its DACL. +// +// The replacement is left alone either way — nothing here deletes what it was +// handed — but the errors below deliberately say nothing about where it ended up, +// which is also why its path is not a parameter. Whether it still exists when the +// error surfaces is the caller's business: the specialist writer, the only caller +// today, removes its temporary file in a deferred cleanup, so an error promising +// a replacement "remains at" that path would send an operator looking for a file +// that was already gone. What these errors do describe is what this function +// itself left on disk — the state of dst, and of the managed backup holding the +// original. +func recoverPartialReplace(callErr error, dst, backup string, restore func(string, string) error) error { if !errors.Is(callErr, errorUnableToMoveReplacement2) { // For 1175, 1176, unsupported volumes, and ordinary failures, Windows // documents that dst remains at its original name. Remove any redundant @@ -206,10 +214,10 @@ func recoverPartialReplace(callErr error, src, dst, backup string, restore func( } if _, err := os.Lstat(backup); err != nil { - return fmt.Errorf("replace %s: %w (original backup expected at %s is unavailable: %v; replacement remains at %s)", dst, callErr, backup, err, src) + return fmt.Errorf("replace %s: %w (original backup expected at %s is unavailable: %v)", dst, callErr, backup, err) } if _, err := os.Lstat(dst); err == nil { - return fmt.Errorf("replace %s: %w (destination unexpectedly exists; original remains at backup %s and replacement at %s)", dst, callErr, backup, src) + return fmt.Errorf("replace %s: %w (destination unexpectedly exists; the original also remains at backup %s)", dst, callErr, backup) } else if !os.IsNotExist(err) { return fmt.Errorf("replace %s: %w (inspect destination before restoring backup %s: %v)", dst, callErr, backup, err) } @@ -217,7 +225,16 @@ func recoverPartialReplace(callErr error, src, dst, backup string, restore func( // Only callErr is wrapped. Exposing a sharing violation from the exhausted // rollback would make the outer ReplaceWithRetry call retry after the // filesystem was already mutated. - return fmt.Errorf("replace %s: %w (original survives at backup %s; restoring it failed: %v; replacement remains at %s)", dst, callErr, backup, err, src) - } - return fmt.Errorf("replace %s: %w (original was restored; replacement remains at %s for cleanup)", dst, callErr, src) + // + // This is the one terminal state that needs a human: nothing is at dst, and + // the original's only copy is under a name no tool looks for (the specialist + // loader reads *.md and skips everything else), so it is invisible until + // somebody moves it back. Say that outright rather than leaving an operator + // to infer it from a bare error code. + return fmt.Errorf( + "replace %s: %w (rolling the original back failed: %v; %s no longer exists and the original survives only as %s, which must be moved back by hand to restore it)", + dst, callErr, err, dst, backup, + ) + } + return fmt.Errorf("replace %s: %w (the original was restored)", dst, callErr) } diff --git a/internal/fsutil/replace_windows_test.go b/internal/fsutil/replace_windows_test.go index 17f61ab5e..2447ede81 100644 --- a/internal/fsutil/replace_windows_test.go +++ b/internal/fsutil/replace_windows_test.go @@ -349,6 +349,7 @@ func TestReplaceExistingRollsBack1177AndRetriesTransientRestore(t *testing.T) { if restoreAttempts != 3 { t.Fatalf("restore attempts = %d, want transient lock failures retried", restoreAttempts) } + assertErrorDoesNotAdvertiseReplacement(t, err, src) assertFileContent(t, dst, "old") assertFileContent(t, src, "new") if _, err := os.Lstat(backupPath); !os.IsNotExist(err) { @@ -394,6 +395,16 @@ func TestReplaceExistingPreserves1177BackupWhenRollbackFails(t *testing.T) { if !strings.Contains(err.Error(), backupPath) { t.Fatalf("error = %v, want retained backup path %q", err, backupPath) } + // The terminal state an operator has to fix by hand: the original exists only + // under the backup name and nothing is at dst, so the error has to name both + // and say what to do, not just report the Windows code. + if !strings.Contains(err.Error(), dst) { + t.Fatalf("error = %v, want the destination %q an operator must restore to", err, dst) + } + if !strings.Contains(err.Error(), "moved back by hand") { + t.Fatalf("error = %v, want an explicit instruction to move the backup back", err) + } + assertErrorDoesNotAdvertiseReplacement(t, err, src) if _, err := os.Lstat(dst); !os.IsNotExist(err) { t.Fatalf("destination should remain absent after failed rollback: %v", err) } @@ -455,7 +466,7 @@ func TestRecoverPartialReplaceLeavesIntactStatesAlone(t *testing.T) { t.Fatalf("WriteFile dst: %v", err) } - if err := recoverPartialReplace(tc.code, src, dst, backup, nil); !errors.Is(err, tc.code) { + if err := recoverPartialReplace(tc.code, dst, backup, nil); !errors.Is(err, tc.code) { t.Fatalf("error = %v, want the original %v unchanged", err, tc.code) } assertFileContent(t, dst, "old") @@ -467,6 +478,21 @@ func TestRecoverPartialReplaceLeavesIntactStatesAlone(t *testing.T) { } } +// assertErrorDoesNotAdvertiseReplacement guards the messaging fixed for jatmn's +// #757 P3 finding: recovery errors used to say the replacement "remains at" the +// caller's temporary path, but the only caller removes that file in a deferred +// cleanup before the error ever surfaces. Recovery is always about the original, +// so an operator must never be sent looking for the replacement. +func assertErrorDoesNotAdvertiseReplacement(t *testing.T, err error, replacement string) { + t.Helper() + if err == nil { + t.Fatal("expected a partial-replacement error") + } + if strings.Contains(err.Error(), replacement) { + t.Fatalf("error = %v, must not point an operator at the replacement %q, which its caller deletes", err, replacement) + } +} + func assertFileContent(t *testing.T, path, want string) { t.Helper() data, err := os.ReadFile(path) diff --git a/internal/specialist/manifest.go b/internal/specialist/manifest.go index 445c48e07..6f44292fa 100644 --- a/internal/specialist/manifest.go +++ b/internal/specialist/manifest.go @@ -476,6 +476,14 @@ func loadDirectory(dir string, location Location) ([]Manifest, []string, error) manifests := []Manifest{} warnings := []string{} for _, entry := range entries { + // Only *.md is a specialist. Everything else in the directory is + // deliberately invisible here, including the two kinds of sibling an + // interrupted overwrite can leave: a .specialist-*.tmp replacement that was + // never published, and a .zero-replace-*.backup holding an original whose + // rollback failed on Windows. Neither is a manifest, and guessing that one + // of them is would be worse than the gap. The recovery for a backup that + // really does hold the last good copy is a manual rename, spelled out both + // in the fsutil error that reports it and in docs/SPECIALISTS.md. if entry.IsDir() || strings.ToLower(filepath.Ext(entry.Name())) != ".md" { continue } diff --git a/internal/specialist/storage.go b/internal/specialist/storage.go index 3a7b4ef4e..b76cd8257 100644 --- a/internal/specialist/storage.go +++ b/internal/specialist/storage.go @@ -114,6 +114,12 @@ func writeSpecialistAtomicWith(path string, content string, rename func(string, return fmt.Errorf("create temporary specialist file: %w", err) } tempPath := temp.Name() + // The temporary file never survives this function, on any path: a successful + // replace has already consumed it, and every failure — including the Windows + // partial-replace recoveries in fsutil — discards it here. Recovery from a + // failed overwrite is therefore always about the ORIGINAL (which fsutil either + // rolls back or names in its error), never about this file, and no error text + // should tell an operator to go looking for it. defer func() { _ = temp.Close() _ = os.Remove(tempPath) From 532472f3c2e591aa4b03b2892ee49f478a16c798 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Wed, 29 Jul 2026 22:38:40 +0200 Subject: [PATCH 8/8] fix(specialist): serialize managed loads during replacement Amp-Thread-ID: https://ampcode.com/threads/T-019faf74-99d4-75cf-ac7a-661c308240ef Co-authored-by: Amp --- docs/SPECIALISTS.md | 32 +++--- internal/fsutil/rename.go | 10 +- internal/fsutil/replace_windows.go | 31 +++--- internal/fsutil/replace_windows_test.go | 17 ++- internal/specialist/manifest.go | 11 +- internal/specialist/storage.go | 58 ++++++---- .../specialist/storage_dacl_windows_test.go | 2 +- internal/specialist/storage_test.go | 101 +++++++++++++++--- 8 files changed, 181 insertions(+), 81 deletions(-) diff --git a/docs/SPECIALISTS.md b/docs/SPECIALISTS.md index f11ca72f3..5ef27b714 100644 --- a/docs/SPECIALISTS.md +++ b/docs/SPECIALISTS.md @@ -141,15 +141,23 @@ manager marks that task `error` and clears its PID. This avoids sending ## Recovering an Interrupted Overwrite -Overwriting a specialist (`--force`) publishes the new file atomically, so an -interrupted write leaves either the old manifest or the new one — never a -half-written file. Windows has one rare exception worth knowing about. - -There, the swap goes through `ReplaceFileW`, which preserves the destination's -security descriptor instead of silently replacing it with the directory's -inherited one. If it fails with `ERROR_UNABLE_TO_MOVE_REPLACEMENT_2` (1177), -Zero has already moved your original aside and tries to move it back. That -rollback almost always succeeds, and the failed write changes nothing. +Zero writes and flushes a complete temporary file before publishing an overwrite, +so a write failure before publication leaves the existing manifest unchanged +instead of truncating it. On Unix, publication uses a same-directory rename and +preserves the existing file's permission bits. + +On Windows, Zero uses `ReplaceFileW` to preserve the destination DACL instead of +silently replacing it with the temporary file's inherited DACL. `ReplaceFileW` +is not observer-atomic: another process can briefly observe the destination path +as absent during replacement. Zero serializes specialist loads and managed +mutations within one process, but cannot synchronize external processes or +editors. + +Windows errors 1176 (`ERROR_UNABLE_TO_MOVE_REPLACEMENT`) and 1177 +(`ERROR_UNABLE_TO_MOVE_REPLACEMENT_2`) are partial replacement failures. With +Zero's managed backup, 1176 leaves the original names intact and needs no manual +recovery. For 1177, Zero has moved the original aside and tries to move it back. +That rollback almost always succeeds, and the failed write changes nothing. If the rollback itself fails — typically because another process is holding a lock on the file — the original is not lost, but it is left under a name Zero @@ -160,9 +168,9 @@ does not read: ``` Only `*.md` files are loaded as specialists, so until that file is renamed the -specialist will not appear in `zero specialist list` or resolve by name. The -error Zero prints names both paths; recover by closing whatever holds the lock -and renaming the backup back: +specialist will not appear in `zero specialist list` or resolve by name. Zero's +error message includes both the backup path and destination path. Recover by +closing whatever holds the lock and renaming the backup back: ```powershell Move-Item .zero-replace-.backup .md diff --git a/internal/fsutil/rename.go b/internal/fsutil/rename.go index 7dd22e3fc..4f74e8544 100644 --- a/internal/fsutil/rename.go +++ b/internal/fsutil/rename.go @@ -29,11 +29,11 @@ func (err *CommittedReplacementCleanupError) Unwrap() error { // ReplaceWithRetry publishes src over dst using the platform's replacement // primitive, retrying on the same transient Windows lock errors RenameWithRetry -// handles. Prefer it over RenameWithRetry when dst may already exist and is -// expected to keep its identity: on Windows it replaces through ReplaceFileW, -// which is a single operation (os.Rename is documented non-atomic there) and -// carries the destination's security descriptor over to the replacement instead -// of publishing the temporary file's inherited ACL. On Unix it is os.Rename. +// handles. On Windows it uses ReplaceFileW so an existing destination keeps its +// DACL and selected metadata instead of receiving the temporary file's inherited +// DACL. ReplaceFileW is not observer-atomic and may briefly leave dst absent; +// callers that cannot tolerate that must synchronize their own readers. On Unix +// replacement uses os.Rename. // // replace overrides the platform primitive so tests can exercise the retry path; // pass nil for the default. diff --git a/internal/fsutil/replace_windows.go b/internal/fsutil/replace_windows.go index 2efce011e..7cf6be44c 100644 --- a/internal/fsutil/replace_windows.go +++ b/internal/fsutil/replace_windows.go @@ -25,15 +25,14 @@ import ( // destination untouched. Passing 0x2 while omitting only 0x4 buys nothing: // ACL merging is part of the metadata merge 0x2 covers. // - REPLACEFILE_WRITE_THROUGH (0x1) is documented as "This value is not -// supported", so it cannot be relied on to flush anything. The single- -// operation swap is ReplaceFileW's own documented behavior, not this flag's. +// supported", so it cannot be relied on to flush anything. const replaceFileFlags = 0 const replaceBackupPattern = ".zero-replace-*.backup" const ( - // Returned when the volume or redirector cannot provide ReplaceFileW's atomic, - // descriptor-preserving semantics. Existing destinations must fail closed. + // Returned when the volume or redirector cannot provide ReplaceFileW's + // DACL-preserving semantics. Existing destinations must fail closed. errorInvalidFunction = syscall.Errno(1) errorNotSupported = syscall.Errno(50) @@ -51,19 +50,17 @@ var ( ) // replaceExisting publishes src over dst with ReplaceFileW rather than -// MoveFileEx (what os.Rename uses). Two reasons, both of which os.Rename fails -// to provide on Windows: +// MoveFileEx (what os.Rename uses) to preserve destination metadata. The +// replacement is a freshly created temporary file, so it carries the directory's +// inherited DACL. Renaming it over the destination would therefore replace the +// destination's ACL - silently widening access to a file that had been restricted +// explicitly (os.File.Chmod cannot express that on Windows; Go only maps the +// owner-write bit). ReplaceFileW carries the replaced file's DACL and selected +// metadata over to the replacement instead. // -// - Atomicity. Go documents os.Rename as NOT atomic outside Unix, so an -// interrupted or concurrently observed overwrite can expose a missing or -// intermediate file. ReplaceFileW performs the swap as one operation. -// - Security descriptor. The replacement is a freshly created temporary file, -// so it carries the directory's inherited DACL. Renaming it over the -// destination therefore REPLACES the destination's ACL — silently widening -// access to a file that had been restricted explicitly (os.File.Chmod cannot -// express that on Windows; Go only maps the owner-write bit). ReplaceFileW -// carries the replaced file's security descriptor, attributes, and streams -// over to the replacement instead. +// ReplaceFileW combines multiple filesystem steps and is not observer-atomic: an +// external reader can briefly see dst absent even when replacement succeeds. +// Callers that cannot tolerate that window must synchronize their own readers. // // No REPLACEFILE_* flag is passed at all — see replaceFileFlags for why each one // would either silently lose the descriptor this function exists to preserve or @@ -103,7 +100,7 @@ func replaceExistingWithCleanup(src, dst string, replace func(string, string, st return nil } if errors.Is(callErr, errorInvalidFunction) || errors.Is(callErr, errorNotSupported) { - callErr = fmt.Errorf("atomic replacement is not supported for %s: %w", dst, callErr) + callErr = fmt.Errorf("DACL-preserving replacement is not supported for %s: %w", dst, callErr) } return recoverPartialReplace(callErr, dst, backup, restore) } diff --git a/internal/fsutil/replace_windows_test.go b/internal/fsutil/replace_windows_test.go index 2447ede81..3455540d0 100644 --- a/internal/fsutil/replace_windows_test.go +++ b/internal/fsutil/replace_windows_test.go @@ -13,12 +13,11 @@ import ( "golang.org/x/sys/windows" ) -// TestReplaceWithRetryKeepsTheDestinationIdentity proves the publish goes through -// ReplaceFileW and not os.Rename: ReplaceFileW gives the replacement the replaced -// file's identity, so the destination's creation time survives. A rename would -// carry the temporary file's creation time over instead, and os.Rename is also -// documented non-atomic on Windows. -func TestReplaceWithRetryKeepsTheDestinationIdentity(t *testing.T) { +// TestReplaceWithRetryPreservesDestinationCreationTime proves the publish goes +// through ReplaceFileW and not os.Rename: ReplaceFileW carries selected metadata, +// including creation time, from the destination to the replacement. A rename +// would carry the temporary file's creation time over instead. +func TestReplaceWithRetryPreservesDestinationCreationTime(t *testing.T) { dir := t.TempDir() dst := filepath.Join(dir, "manifest.md") if err := os.WriteFile(dst, []byte("old"), 0o600); err != nil { @@ -166,7 +165,7 @@ func TestReplaceFileFlagsDoNotIgnoreMergeErrors(t *testing.T) { } } -func TestReplaceExistingRejectsUnsupportedAtomicReplacement(t *testing.T) { +func TestReplaceExistingRejectsUnsupportedDACLReplacement(t *testing.T) { for _, tc := range []struct { name string code syscall.Errno @@ -197,8 +196,8 @@ func TestReplaceExistingRejectsUnsupportedAtomicReplacement(t *testing.T) { if !errors.Is(err, tc.code) { t.Fatalf("error = %v, want unsupported error %v", err, tc.code) } - if !strings.Contains(err.Error(), "atomic replacement is not supported") { - t.Fatalf("error = %v, want an atomic-replacement explanation", err) + if !strings.Contains(err.Error(), "DACL-preserving replacement is not supported") { + t.Fatalf("error = %v, want a DACL-preserving-replacement explanation", err) } assertFileContent(t, dst, "old") assertFileContent(t, src, "new") diff --git a/internal/specialist/manifest.go b/internal/specialist/manifest.go index 6f44292fa..5335596c2 100644 --- a/internal/specialist/manifest.go +++ b/internal/specialist/manifest.go @@ -9,6 +9,7 @@ import ( "sort" "strconv" "strings" + "sync" "time" "github.com/Gitlawb/zero/internal/config" @@ -72,6 +73,12 @@ type LoadResult struct { var namePattern = regexp.MustCompile(`^[a-z][a-z0-9-]{0,63}$`) +// specialistFilesMu prevents Zero-managed loads from observing a specialist +// mutation in progress. This is especially important on Windows, where +// ReplaceFileW can briefly leave the destination name absent. It cannot +// synchronize external editors or other Zero processes. +var specialistFilesMu sync.RWMutex + var knownMetadataKeys = map[string]bool{ "name": true, "description": true, @@ -143,6 +150,8 @@ func Load(options LoadOptions) (LoadResult, error) { } paths.UserDir = resolved.UserDir } + specialistFilesMu.RLock() + defer specialistFilesMu.RUnlock() manifests := Builtins() warnings := []string{} @@ -477,7 +486,7 @@ func loadDirectory(dir string, location Location) ([]Manifest, []string, error) warnings := []string{} for _, entry := range entries { // Only *.md is a specialist. Everything else in the directory is - // deliberately invisible here, including the two kinds of sibling an + // deliberately invisible here, including the two kinds of sibling files an // interrupted overwrite can leave: a .specialist-*.tmp replacement that was // never published, and a .zero-replace-*.backup holding an original whose // rollback failed on Windows. Neither is a manifest, and guessing that one diff --git a/internal/specialist/storage.go b/internal/specialist/storage.go index b76cd8257..3b42f00f4 100644 --- a/internal/specialist/storage.go +++ b/internal/specialist/storage.go @@ -13,8 +13,8 @@ import ( ) type Storage struct { - paths Paths - writeAtomic func(string, string) error + paths Paths + writeReplacement func(string, string) error } type CreateInput struct { @@ -69,15 +69,17 @@ func (storage *Storage) Create(input CreateInput) (Manifest, error) { } content := FormatMarkdown(manifest) dir := filepath.Dir(path) + specialistFilesMu.Lock() + defer specialistFilesMu.Unlock() if err := os.MkdirAll(dir, 0o700); err != nil { return Manifest{}, fmt.Errorf("create specialist directory: %w", err) } if input.Overwrite { - writeAtomic := storage.writeAtomic - if writeAtomic == nil { - writeAtomic = writeSpecialistAtomic + writeReplacement := storage.writeReplacement + if writeReplacement == nil { + writeReplacement = writeSpecialistReplacement } - if err := writeAtomic(path, content); err != nil { + if err := writeReplacement(path, content); err != nil { var cleanupErr *fsutil.CommittedReplacementCleanupError if errors.As(err, &cleanupErr) { manifest.Warnings = append(manifest.Warnings, fmt.Sprintf("specialist was updated, but replacement backup %s could not be removed: %v", cleanupErr.BackupPath, cleanupErr.Cause)) @@ -104,11 +106,11 @@ func (storage *Storage) Create(input CreateInput) (Manifest, error) { return manifest, nil } -func writeSpecialistAtomic(path string, content string) error { - return writeSpecialistAtomicWith(path, content, nil, syncSpecialistDir) +func writeSpecialistReplacement(path string, content string) error { + return writeSpecialistReplacementWith(path, content, nil, syncSpecialistDir) } -func writeSpecialistAtomicWith(path string, content string, rename func(string, string) error, syncDir func(string) error) (err error) { +func writeSpecialistReplacementWith(path string, content string, rename func(string, string) error, syncDir func(string) error) (err error) { temp, err := os.CreateTemp(filepath.Dir(path), ".specialist-*.tmp") if err != nil { return fmt.Errorf("create temporary specialist file: %w", err) @@ -128,6 +130,22 @@ func writeSpecialistAtomicWith(path string, content string, rename func(string, if err := temp.Chmod(0o600); err != nil { return fmt.Errorf("set temporary specialist file permissions: %w", err) } + info, err := os.Lstat(path) + if err != nil && !os.IsNotExist(err) { + return fmt.Errorf("inspect specialist file: %w", err) + } + if err == nil { + if info.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("refusing to overwrite symlink specialist file: %s", path) + } + if runtime.GOOS != "windows" { + // The old in-place overwrite retained manually configured Unix modes. + // Preserve that behavior when publishing a replacement inode. + if err := temp.Chmod(info.Mode().Perm()); err != nil { + return fmt.Errorf("preserve specialist file permissions: %w", err) + } + } + } if _, err := temp.WriteString(content); err != nil { return fmt.Errorf("write temporary specialist file: %w", err) } @@ -138,20 +156,12 @@ func writeSpecialistAtomicWith(path string, content string, rename func(string, return fmt.Errorf("close temporary specialist file: %w", err) } - info, err := os.Lstat(path) - if err != nil && !os.IsNotExist(err) { - return fmt.Errorf("inspect specialist file: %w", err) - } - if err == nil && info.Mode()&os.ModeSymlink != 0 { - return fmt.Errorf("refusing to overwrite symlink specialist file: %s", path) - } - // ReplaceWithRetry, not RenameWithRetry: os.Rename is documented non-atomic - // outside Unix, and renaming the freshly created temporary file over the - // destination would also publish the directory's inherited DACL in place of - // whatever the destination had — silently widening access to a specialist that - // had been restricted explicitly. On Windows this replaces through - // ReplaceFileW, which is one operation and preserves the destination's - // security descriptor; on Unix it is the same atomic rename as before. + // On Unix, the same-directory rename publishes the complete file atomically. + // On Windows, ReplaceFileW is used to preserve the destination DACL rather + // than publishing the temporary file's inherited DACL. ReplaceFileW can + // briefly leave the destination name absent; specialistFilesMu prevents + // Zero-managed loads in this process from observing that window, but external + // processes and editors are not synchronized. if err := fsutil.ReplaceWithRetry(tempPath, path, rename); err != nil { return fmt.Errorf("replace specialist file: %w", err) } @@ -183,6 +193,8 @@ func (storage *Storage) Delete(input DeleteInput) (string, error) { if err != nil { return "", err } + specialistFilesMu.Lock() + defer specialistFilesMu.Unlock() if err := os.Remove(path); err != nil { if os.IsNotExist(err) { return "", fmt.Errorf("specialist not found: %s", strings.TrimSpace(input.Name)) diff --git a/internal/specialist/storage_dacl_windows_test.go b/internal/specialist/storage_dacl_windows_test.go index 7ef53f999..777d1cb85 100644 --- a/internal/specialist/storage_dacl_windows_test.go +++ b/internal/specialist/storage_dacl_windows_test.go @@ -12,7 +12,7 @@ import ( ) // TestStorageCreateForceKeepsWindowsDACL is the regression test for the Windows -// half of the atomic-overwrite change: the replacement is a freshly created +// DACL-preservation change: the replacement is a freshly created // temporary file, so publishing it with a plain rename would hand the destination // the directory's inherited DACL and drop the restrictive one an explicitly // locked-down specialist had — exposing its system prompt to anyone the directory diff --git a/internal/specialist/storage_test.go b/internal/specialist/storage_test.go index 6283d0bb5..691e6be10 100644 --- a/internal/specialist/storage_test.go +++ b/internal/specialist/storage_test.go @@ -8,6 +8,7 @@ import ( "strings" "syscall" "testing" + "time" "github.com/Gitlawb/zero/internal/fsutil" ) @@ -96,7 +97,7 @@ func TestStorageCreateForceRejectsSymlink(t *testing.T) { assertNoTemporarySpecialistFiles(t, userDir) } -func TestStorageCreateForceAtomicallyReplacesFile(t *testing.T) { +func TestStorageCreateForceReplacesFileAndPreservesMode(t *testing.T) { userDir := t.TempDir() path := filepath.Join(userDir, "safe.md") if err := os.WriteFile(path, []byte("old content"), 0o644); err != nil { @@ -124,18 +125,92 @@ func TestStorageCreateForceAtomicallyReplacesFile(t *testing.T) { if err != nil { t.Fatal(err) } - if got := info.Mode().Perm(); runtime.GOOS != "windows" && got != 0o600 { - t.Fatalf("file permissions = %o, want 600", got) + if got := info.Mode().Perm(); runtime.GOOS != "windows" && got != 0o644 { + t.Fatalf("file permissions = %o, want the existing mode 644", got) } assertNoTemporarySpecialistFiles(t, userDir) } +func TestStorageCreateForceSerializesConcurrentLoad(t *testing.T) { + userDir := t.TempDir() + storage := NewStorage(Paths{UserDir: userDir}) + if _, err := storage.Create(CreateInput{Name: "safe", Description: "old"}); err != nil { + t.Fatal(err) + } + + gapOpen := make(chan struct{}) + finishReplacement := make(chan struct{}) + storage.writeReplacement = func(path, content string) error { + backup := path + ".test-backup" + if err := os.Rename(path, backup); err != nil { + return err + } + close(gapOpen) + <-finishReplacement + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + return err + } + return os.Remove(backup) + } + + writeDone := make(chan error, 1) + go func() { + _, err := storage.Create(CreateInput{Name: "safe", Description: "new", Overwrite: true}) + writeDone <- err + }() + <-gapOpen + defer func() { + select { + case <-finishReplacement: + default: + close(finishReplacement) + } + }() + + loadDone := make(chan LoadResult, 1) + loadErr := make(chan error, 1) + loadStarted := make(chan struct{}) + go func() { + close(loadStarted) + result, err := Load(LoadOptions{Paths: Paths{UserDir: userDir}}) + if err != nil { + loadErr <- err + return + } + loadDone <- result + }() + <-loadStarted + select { + case err := <-loadErr: + t.Fatalf("Load returned during replacement: %v", err) + case result := <-loadDone: + t.Fatalf("Load returned during replacement gap: %#v", result.Specialists) + case <-time.After(25 * time.Millisecond): + } + + close(finishReplacement) + if err := <-writeDone; err != nil { + t.Fatalf("Create returned error: %v", err) + } + select { + case err := <-loadErr: + t.Fatal(err) + case result := <-loadDone: + manifest, ok := Find(result, "safe") + if !ok || manifest.Metadata.Description != "new" { + t.Fatalf("loaded specialist = %#v, want replacement", manifest) + } + case <-time.After(time.Second): + t.Fatal("Load did not resume after replacement") + } +} + func TestStorageCreateReturnsManifestWarningAfterCommittedCleanupFailure(t *testing.T) { userDir := t.TempDir() backupPath := filepath.Join(userDir, ".zero-replace-old.backup") cleanupErr := &os.PathError{Op: "remove", Path: backupPath, Err: syscall.Errno(32)} storage := NewStorage(Paths{UserDir: userDir}) - storage.writeAtomic = func(path, content string) error { + storage.writeReplacement = func(path, content string) error { if err := os.WriteFile(path, []byte(content), 0o600); err != nil { return err } @@ -154,7 +229,7 @@ func TestStorageCreateReturnsManifestWarningAfterCommittedCleanupFailure(t *test } } -func TestWriteSpecialistAtomicRetriesTransientWindowsRename(t *testing.T) { +func TestWriteSpecialistReplacementRetriesTransientWindowsRename(t *testing.T) { if runtime.GOOS != "windows" { t.Skip("rename retries are Windows-specific") } @@ -164,7 +239,7 @@ func TestWriteSpecialistAtomicRetriesTransientWindowsRename(t *testing.T) { t.Fatal(err) } attempts := 0 - err := writeSpecialistAtomicWith(path, "new content", func(src, dst string) error { + err := writeSpecialistReplacementWith(path, "new content", func(src, dst string) error { attempts++ if attempts == 1 { return syscall.Errno(32) // ERROR_SHARING_VIOLATION @@ -172,7 +247,7 @@ func TestWriteSpecialistAtomicRetriesTransientWindowsRename(t *testing.T) { return os.Rename(src, dst) }, func(string) error { return nil }) if err != nil { - t.Fatalf("writeSpecialistAtomicWith returned error: %v", err) + t.Fatalf("writeSpecialistReplacementWith returned error: %v", err) } if attempts != 2 { t.Fatalf("rename attempts = %d, want 2", attempts) @@ -187,12 +262,12 @@ func TestWriteSpecialistAtomicRetriesTransientWindowsRename(t *testing.T) { assertNoTemporarySpecialistFiles(t, dir) } -func TestWriteSpecialistAtomicIgnoresDirectorySyncErrorAfterCommit(t *testing.T) { +func TestWriteSpecialistReplacementIgnoresDirectorySyncErrorAfterCommit(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "safe.md") syncErr := errors.New("sync failed") called := false - err := writeSpecialistAtomicWith(path, "new content", nil, func(got string) error { + err := writeSpecialistReplacementWith(path, "new content", nil, func(got string) error { called = true if got != dir { t.Fatalf("sync directory = %q, want %q", got, dir) @@ -221,23 +296,23 @@ func TestSyncSpecialistDirIgnoresOpenFailure(t *testing.T) { } } -// TestWriteSpecialistAtomicKeepsTheOriginalAfterFailedReplace pins the caller +// TestWriteSpecialistReplacementKeepsTheOriginalAfterFailedReplace pins the caller // side of Windows partial-failure rollback: fsutil restores the original at dst, // reports the ReplaceFileW error, and leaves src for this function's deferred // temporary-file cleanup. -func TestWriteSpecialistAtomicKeepsTheOriginalAfterFailedReplace(t *testing.T) { +func TestWriteSpecialistReplacementKeepsTheOriginalAfterFailedReplace(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "safe.md") if err := os.WriteFile(path, []byte("old content"), 0o600); err != nil { t.Fatal(err) } replaceErr := errors.New("ReplaceFileW could not rename the replacement") - err := writeSpecialistAtomicWith(path, "new content", func(_, _ string) error { + err := writeSpecialistReplacementWith(path, "new content", func(_, _ string) error { return replaceErr }, func(string) error { return nil }) if !errors.Is(err, replaceErr) { - t.Fatalf("writeSpecialistAtomicWith error = %v, want it to report %v", err, replaceErr) + t.Fatalf("writeSpecialistReplacementWith error = %v, want it to report %v", err, replaceErr) } data, readErr := os.ReadFile(path) if readErr != nil {