From 63daf2e36127d6d7ffd8697c57d1b303b15f5835 Mon Sep 17 00:00:00 2001 From: SultanGun Date: Mon, 26 Oct 2026 18:34:52 +0700 Subject: [PATCH] fix: retry transient Windows file sharing violations --- internal/tools/file_patch.go | 2 +- internal/tools/file_write.go | 2 +- internal/tools/file_writer_other.go | 11 + internal/tools/file_writer_windows.go | 79 +++++++ internal/tools/file_writer_windows_test.go | 247 +++++++++++++++++++++ 5 files changed, 339 insertions(+), 2 deletions(-) create mode 100644 internal/tools/file_writer_other.go create mode 100644 internal/tools/file_writer_windows.go create mode 100644 internal/tools/file_writer_windows_test.go diff --git a/internal/tools/file_patch.go b/internal/tools/file_patch.go index 2d25fbb..3d35109 100644 --- a/internal/tools/file_patch.go +++ b/internal/tools/file_patch.go @@ -78,7 +78,7 @@ func (f *FilePatchTool) Execute(ctx context.Context, args json.RawMessage) (Resu diff := generateUnifiedDiff(path, oldContent, newContent) - if err := os.WriteFile(path, []byte(newContent), 0o644); err != nil { + if err := writeToolFile(path, []byte(newContent), 0o644); err != nil { return Result{Error: fmt.Sprintf("cannot write file: %v", err)}, nil } diff --git a/internal/tools/file_write.go b/internal/tools/file_write.go index f6cb162..a3e7b79 100644 --- a/internal/tools/file_write.go +++ b/internal/tools/file_write.go @@ -89,7 +89,7 @@ func (f *FileWriteTool) Execute(ctx context.Context, args json.RawMessage) (Resu return Result{Error: fmt.Sprintf("cannot create directory %s: %v", dir, err)}, nil } - if err := os.WriteFile(path, []byte(a.Content), 0o644); err != nil { + if err := writeToolFile(path, []byte(a.Content), 0o644); err != nil { return Result{Error: fmt.Sprintf("cannot write file: %v", err)}, nil } diff --git a/internal/tools/file_writer_other.go b/internal/tools/file_writer_other.go new file mode 100644 index 0000000..aef9b7c --- /dev/null +++ b/internal/tools/file_writer_other.go @@ -0,0 +1,11 @@ +//go:build !windows + +package tools + +import "os" + +// writeToolFile writes data to path with perm. +// On non-Windows platforms, it delegates directly to os.WriteFile without retries. +func writeToolFile(path string, data []byte, perm os.FileMode) error { + return os.WriteFile(path, data, perm) +} diff --git a/internal/tools/file_writer_windows.go b/internal/tools/file_writer_windows.go new file mode 100644 index 0000000..cbe7869 --- /dev/null +++ b/internal/tools/file_writer_windows.go @@ -0,0 +1,79 @@ +//go:build windows + +package tools + +import ( + "errors" + "os" + "syscall" + "time" + + "golang.org/x/sys/windows" +) + +const ( + maxWriteAttempts = 5 + initialBackoff = 2 * time.Millisecond + maxBackoff = 20 * time.Millisecond +) + +// writeOpFunc is the signature for the underlying write operation, exposed for testing. +type writeOpFunc func(path string, data []byte, perm os.FileMode) error + +// sleepFunc is the signature for the backoff sleep, exposed for testing. +type sleepFunc func(d time.Duration) + +var ( + // defaultWriteOp is the actual filesystem write operation. + defaultWriteOp writeOpFunc = os.WriteFile + + // defaultSleep is time.Sleep. + defaultSleep sleepFunc = time.Sleep +) + +// writeToolFile writes data to path with perm. On Windows, if a sharing violation +// (ERROR_SHARING_VIOLATION) occurs due to transient filesystem locks, it retries +// with a bounded exponential backoff up to 5 attempts (total wait ~30-37ms). +// Any other error returns immediately without retry. +func writeToolFile(path string, data []byte, perm os.FileMode) error { + return writeToolFileWithRetry(path, data, perm, defaultWriteOp, defaultSleep) +} + +func writeToolFileWithRetry(path string, data []byte, perm os.FileMode, writeOp writeOpFunc, sleep sleepFunc) error { + var err error + backoff := initialBackoff + + for attempt := range maxWriteAttempts { + err = writeOp(path, data, perm) + if err == nil { + return nil + } + + if !isSharingViolation(err) { + return err + } + + if attempt < maxWriteAttempts-1 { + if sleep != nil { + sleep(backoff) + } + backoff *= 2 + if backoff > maxBackoff { + backoff = maxBackoff + } + } + } + + return err +} + +func isSharingViolation(err error) bool { + if errors.Is(err, windows.ERROR_SHARING_VIOLATION) { + return true + } + var errno syscall.Errno + if errors.As(err, &errno) && errno == windows.ERROR_SHARING_VIOLATION { + return true + } + return false +} diff --git a/internal/tools/file_writer_windows_test.go b/internal/tools/file_writer_windows_test.go new file mode 100644 index 0000000..ffa864d --- /dev/null +++ b/internal/tools/file_writer_windows_test.go @@ -0,0 +1,247 @@ +//go:build windows + +package tools + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "os" + "path/filepath" + "testing" + "time" + + "golang.org/x/sys/windows" +) + +func TestWriteToolFile_RetryOnSharingViolation_Success(t *testing.T) { + var attempts int + var sleeps []time.Duration + + mockWrite := func(path string, data []byte, perm os.FileMode) error { + attempts++ + if attempts < 3 { + return windows.ERROR_SHARING_VIOLATION + } + return nil + } + + mockSleep := func(d time.Duration) { + sleeps = append(sleeps, d) + } + + err := writeToolFileWithRetry("test.txt", []byte("data"), 0o644, mockWrite, mockSleep) + if err != nil { + t.Fatalf("expected success, got error: %v", err) + } + + if attempts != 3 { + t.Errorf("expected 3 attempts, got %d", attempts) + } + + expectedSleeps := []time.Duration{2 * time.Millisecond, 4 * time.Millisecond} + if len(sleeps) != len(expectedSleeps) { + t.Fatalf("expected %d sleeps, got %d: %v", len(expectedSleeps), len(sleeps), sleeps) + } + for i, d := range expectedSleeps { + if sleeps[i] != d { + t.Errorf("sleep[%d] = %v, expected %v", i, sleeps[i], d) + } + } +} + +func TestWriteToolFile_RetryOnSharingViolation_Exhaustion(t *testing.T) { + var attempts int + var sleeps []time.Duration + + mockWrite := func(path string, data []byte, perm os.FileMode) error { + attempts++ + return windows.ERROR_SHARING_VIOLATION + } + + mockSleep := func(d time.Duration) { + sleeps = append(sleeps, d) + } + + err := writeToolFileWithRetry("test.txt", []byte("data"), 0o644, mockWrite, mockSleep) + if err == nil { + t.Fatal("expected error on exhaustion, got nil") + } + + if !errors.Is(err, windows.ERROR_SHARING_VIOLATION) { + t.Errorf("expected ERROR_SHARING_VIOLATION, got: %v", err) + } + + if attempts != 5 { + t.Errorf("expected 5 attempts, got %d", attempts) + } + + // 5 attempts means 4 sleeps between them (2ms, 4ms, 8ms, 16ms) + expectedSleeps := []time.Duration{ + 2 * time.Millisecond, + 4 * time.Millisecond, + 8 * time.Millisecond, + 16 * time.Millisecond, + } + if len(sleeps) != len(expectedSleeps) { + t.Fatalf("expected %d sleeps, got %d: %v", len(expectedSleeps), len(sleeps), sleeps) + } + for i, d := range expectedSleeps { + if sleeps[i] != d { + t.Errorf("sleep[%d] = %v, expected %v", i, sleeps[i], d) + } + } +} + +func TestWriteToolFile_ImmediateFailureOnUnrelatedError(t *testing.T) { + testCases := []struct { + name string + err error + }{ + {"permission denied", os.ErrPermission}, + {"not found", os.ErrNotExist}, + {"access denied", windows.ERROR_ACCESS_DENIED}, + {"disk full", windows.ERROR_DISK_FULL}, + {"custom error", errors.New("something went wrong")}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var attempts int + var sleeps []time.Duration + + mockWrite := func(path string, data []byte, perm os.FileMode) error { + attempts++ + return tc.err + } + + mockSleep := func(d time.Duration) { + sleeps = append(sleeps, d) + } + + err := writeToolFileWithRetry("test.txt", []byte("data"), 0o644, mockWrite, mockSleep) + if err == nil { + t.Fatalf("expected error, got nil") + } + if !errors.Is(err, tc.err) { + t.Errorf("expected %v, got %v", tc.err, err) + } + if attempts != 1 { + t.Errorf("expected exactly 1 attempt without retries, got %d", attempts) + } + if len(sleeps) != 0 { + t.Errorf("expected 0 sleeps for unrelated error, got %d", len(sleeps)) + } + }) + } +} + +func TestWriteToolFile_WrappedSharingViolation(t *testing.T) { + wrapped := &os.PathError{ + Op: "open", + Path: `C:\temp\file.txt`, + Err: windows.ERROR_SHARING_VIOLATION, + } + + if !isSharingViolation(wrapped) { + t.Error("expected wrapped PathError to be recognized as sharing violation") + } + + doubleWrapped := fmt.Errorf("write failed: %w", wrapped) + if !isSharingViolation(doubleWrapped) { + t.Error("expected double wrapped error to be recognized as sharing violation") + } + + unrelated := &os.PathError{ + Op: "open", + Path: `C:\temp\file.txt`, + Err: windows.ERROR_ACCESS_DENIED, + } + if isSharingViolation(unrelated) { + t.Error("ACCESS_DENIED should not be recognized as sharing violation") + } +} + +func TestWriteToolFile_RepeatedPatchAndOverwriteWindows(t *testing.T) { + const iterations = 30 + dir := t.TempDir() + + for i := range iterations { + testFile := filepath.Join(dir, fmt.Sprintf("repeat_%d.txt", i)) + + // 1. Write file via FileWriteTool + writeTool := &FileWriteTool{} + wArgs, _ := json.Marshal(fileWriteArgs{Path: testFile, Content: "initial content"}) + wRes, err := writeTool.Execute(context.Background(), wArgs) + if err != nil { + t.Fatalf("iteration %d write error: %v", i, err) + } + if wRes.Error != "" { + t.Fatalf("iteration %d write result error: %s", i, wRes.Error) + } + + // 2. Overwrite file via FileWriteTool (exercises read -> write) + wArgs2, _ := json.Marshal(fileWriteArgs{Path: testFile, Content: "overwritten content"}) + wRes2, err := writeTool.Execute(context.Background(), wArgs2) + if err != nil { + t.Fatalf("iteration %d overwrite error: %v", i, err) + } + if wRes2.Error != "" { + t.Fatalf("iteration %d overwrite result error: %s", i, wRes2.Error) + } + + // 3. Patch file via FilePatchTool (exercises read -> write) + patchTool := &FilePatchTool{} + pArgs, _ := json.Marshal(filePatchArgs{Path: testFile, Find: "overwritten", Replace: "patched"}) + pRes, err := patchTool.Execute(context.Background(), pArgs) + if err != nil { + t.Fatalf("iteration %d patch error: %v", i, err) + } + if pRes.Error != "" { + t.Fatalf("iteration %d patch result error: %s", i, pRes.Error) + } + + data, err := os.ReadFile(testFile) + if err != nil { + t.Fatalf("iteration %d read verify error: %v", i, err) + } + if string(data) != "patched content" { + t.Errorf("iteration %d expected 'patched content', got %q", i, string(data)) + } + } +} + +func TestTools_OnModified_FailedWriteDoesNotTrigger(t *testing.T) { + var modified []string + onMod := func(p string) { + modified = append(modified, p) + } + + invalidPath := "C:\\invalid*dir?\\file.txt" + writeTool := &FileWriteTool{OnModified: onMod} + wArgs, _ := json.Marshal(fileWriteArgs{Path: invalidPath, Content: "content"}) + wRes, err := writeTool.Execute(context.Background(), wArgs) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + if wRes.Error == "" { + t.Error("expected write error for invalid path") + } + if len(modified) != 0 { + t.Errorf("expected OnModified not to be called on failed write, got %v", modified) + } + + patchTool := &FilePatchTool{OnModified: onMod} + pArgs, _ := json.Marshal(filePatchArgs{Path: filepath.Join(t.TempDir(), "nonexistent.txt"), Find: "a", Replace: "b"}) + pRes, err := patchTool.Execute(context.Background(), pArgs) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + if pRes.Error == "" { + t.Error("expected patch error for nonexistent file") + } + if len(modified) != 0 { + t.Errorf("expected OnModified not to be called on failed patch, got %v", modified) + } +}