Skip to content

Commit 873c03e

Browse files
committed
fix: windows migration panic with platform-specific atomic operations
implements platform-specific atomic file operations to handle Windows file locking during migrations. key changes: - read config into memory before atomic operations to avoid file locks - use simple write for backup (not atomic) since it's a new file - add sync and retry logic with exponential backoff on Windows - use MoveFileEx with MOVEFILE_REPLACE_EXISTING for atomic renames - fix TestRepoDir to set USERPROFILE on Windows (not just HOME) fixes the "panic: error can't be dealt with transactionally: Access is denied" error during repo migration from v17 to v18 on Windows.
1 parent 2993b92 commit 873c03e

8 files changed

Lines changed: 326 additions & 36 deletions

File tree

docs/changelogs/v0.38.md

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
This release was brought to you by the [Shipyard](https://ipshipyard.com/) team.
66

77
- [v0.38.0](#v0380)
8+
- [v0.38.1](#v0381)
89

910
## v0.38.0
1011

@@ -290,3 +291,26 @@ The new [`Internal.MFSNoFlushLimit`](https://github.com/ipfs/kubo/blob/master/do
290291
| Jakub Sztandera | 1 | +67/-15 | 3 |
291292
| Masih H. Derkani | 1 | +1/-2 | 2 |
292293
| Dominic Della Valle | 1 | +2/-1 | 1 |
294+
295+
## v0.38.1
296+
297+
This patch release fixes a critical issue on Windows where repository migrations from version 17 to 18 would fail with "panic: error can't be dealt with transactionally: Access is denied."
298+
299+
The issue was caused by Windows file locking semantics preventing atomic file operations during the migration. This release implements platform-specific file handling with retry logic to work around Windows file system limitations.
300+
301+
### Changelog
302+
303+
<details>
304+
<summary>Full Changelog</summary>
305+
306+
- github.com/ipfs/kubo:
307+
- fix: windows migration panic with platform-specific atomic file operations
308+
- fix: add sync and retry logic for Windows file operations to handle transient locks
309+
310+
</details>
311+
312+
### 👨‍👩‍👧‍👦 Contributors
313+
314+
| Contributor | Commits | Lines ± | Files Changed |
315+
|-------------|---------|---------|---------------|
316+
| TBD | | | |

repo/fsrepo/migrations/atomicfile/atomicfile.go renamed to repo/fsrepo/migrations/atomicfile/atomicfile_common.go

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -32,21 +32,6 @@ func New(path string, mode os.FileMode) (*File, error) {
3232
}, nil
3333
}
3434

35-
// Close atomically replaces the target file with the temporary file
36-
func (f *File) Close() error {
37-
if err := f.File.Close(); err != nil {
38-
os.Remove(f.File.Name())
39-
return err
40-
}
41-
42-
if err := os.Rename(f.File.Name(), f.path); err != nil {
43-
os.Remove(f.File.Name())
44-
return err
45-
}
46-
47-
return nil
48-
}
49-
5035
// Abort removes the temporary file without replacing the target
5136
func (f *File) Abort() error {
5237
f.File.Close()
Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
package atomicfile
2+
3+
import (
4+
"os"
5+
"path/filepath"
6+
"testing"
7+
)
8+
9+
func TestAtomicFile(t *testing.T) {
10+
tempDir := t.TempDir()
11+
targetPath := filepath.Join(tempDir, "target.txt")
12+
13+
// Test creating a new file atomically
14+
af, err := New(targetPath, 0600)
15+
if err != nil {
16+
t.Fatalf("failed to create atomic file: %v", err)
17+
}
18+
19+
testContent := "test content"
20+
if _, err := af.Write([]byte(testContent)); err != nil {
21+
t.Fatalf("failed to write to atomic file: %v", err)
22+
}
23+
24+
// Close should rename the temp file to target
25+
if err := af.Close(); err != nil {
26+
t.Fatalf("failed to close atomic file: %v", err)
27+
}
28+
29+
// Verify target file exists with correct content
30+
content, err := os.ReadFile(targetPath)
31+
if err != nil {
32+
t.Fatalf("failed to read target file: %v", err)
33+
}
34+
if string(content) != testContent {
35+
t.Errorf("content mismatch: got %q, want %q", string(content), testContent)
36+
}
37+
38+
// Test replacing an existing file
39+
af2, err := New(targetPath, 0600)
40+
if err != nil {
41+
t.Fatalf("failed to create second atomic file: %v", err)
42+
}
43+
44+
newContent := "new content"
45+
if _, err := af2.Write([]byte(newContent)); err != nil {
46+
t.Fatalf("failed to write to second atomic file: %v", err)
47+
}
48+
49+
// This should replace the existing file
50+
if err := af2.Close(); err != nil {
51+
t.Fatalf("failed to close second atomic file: %v", err)
52+
}
53+
54+
// Verify file was replaced
55+
content, err = os.ReadFile(targetPath)
56+
if err != nil {
57+
t.Fatalf("failed to read replaced file: %v", err)
58+
}
59+
if string(content) != newContent {
60+
t.Errorf("content not replaced: got %q, want %q", string(content), newContent)
61+
}
62+
}
63+
64+
func TestAtomicFileAbort(t *testing.T) {
65+
tempDir := t.TempDir()
66+
targetPath := filepath.Join(tempDir, "target.txt")
67+
68+
// Create an atomic file
69+
af, err := New(targetPath, 0600)
70+
if err != nil {
71+
t.Fatalf("failed to create atomic file: %v", err)
72+
}
73+
74+
// Write some content
75+
if _, err := af.Write([]byte("should be aborted")); err != nil {
76+
t.Fatalf("failed to write to atomic file: %v", err)
77+
}
78+
79+
// Abort should remove temp file without creating target
80+
if err := af.Abort(); err != nil {
81+
t.Fatalf("failed to abort atomic file: %v", err)
82+
}
83+
84+
// Verify target file doesn't exist
85+
if _, err := os.Stat(targetPath); !os.IsNotExist(err) {
86+
t.Errorf("target file should not exist after abort")
87+
}
88+
}
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
//go:build !windows
2+
// +build !windows
3+
4+
package atomicfile
5+
6+
import (
7+
"os"
8+
)
9+
10+
// Close atomically replaces the target file with the temporary file on Unix-like systems
11+
func (f *File) Close() error {
12+
if err := f.File.Close(); err != nil {
13+
os.Remove(f.File.Name())
14+
return err
15+
}
16+
17+
// On Unix-like systems, os.Rename is atomic and can replace existing files
18+
if err := os.Rename(f.File.Name(), f.path); err != nil {
19+
os.Remove(f.File.Name())
20+
return err
21+
}
22+
23+
return nil
24+
}
Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
//go:build windows
2+
// +build windows
3+
4+
package atomicfile
5+
6+
import (
7+
"os"
8+
"time"
9+
10+
"golang.org/x/sys/windows"
11+
)
12+
13+
// Close atomically replaces the target file with the temporary file on Windows
14+
func (f *File) Close() error {
15+
// Sync to ensure all data is written to disk before closing
16+
// This is important on Windows to avoid file locking issues
17+
if err := f.File.Sync(); err != nil {
18+
// Sync error is non-fatal, continue with close
19+
_ = err
20+
}
21+
22+
// Close the file handle first
23+
if err := f.File.Close(); err != nil {
24+
os.Remove(f.File.Name())
25+
return err
26+
}
27+
28+
// On Windows, always use MoveFileEx for atomic operations
29+
// os.Rename fails when files are locked or in use, which is common on Windows
30+
31+
// First attempt with standard rename to see if it works
32+
// This is faster when there are no locks
33+
err := os.Rename(f.File.Name(), f.path)
34+
if err == nil {
35+
return nil
36+
}
37+
38+
// If rename failed, wait a bit for file handles to be released
39+
// Windows sometimes holds file handles briefly after close
40+
time.Sleep(time.Millisecond * 10)
41+
42+
// Retry logic to handle transient locks from antivirus/indexing
43+
const maxRetries = 5
44+
var lastErr error
45+
for i := 0; i < maxRetries; i++ {
46+
if i > 0 {
47+
// Exponential backoff with longer delays
48+
time.Sleep(time.Millisecond * 100 * time.Duration(i))
49+
}
50+
51+
// Convert paths to UTF16 for Windows API
52+
from, err := windows.UTF16PtrFromString(f.File.Name())
53+
if err != nil {
54+
lastErr = err
55+
continue
56+
}
57+
58+
to, err := windows.UTF16PtrFromString(f.path)
59+
if err != nil {
60+
lastErr = err
61+
continue
62+
}
63+
64+
// Use MoveFileEx with MOVEFILE_REPLACE_EXISTING to atomically replace the file
65+
// MOVEFILE_WRITE_THROUGH ensures the file is flushed to disk
66+
err = windows.MoveFileEx(from, to,
67+
windows.MOVEFILE_REPLACE_EXISTING|windows.MOVEFILE_WRITE_THROUGH)
68+
69+
if err == nil {
70+
return nil
71+
}
72+
73+
lastErr = err
74+
// Check if it's a retryable error
75+
if err != windows.ERROR_SHARING_VIOLATION && err != windows.ERROR_LOCK_VIOLATION && err != windows.ERROR_ACCESS_DENIED {
76+
break
77+
}
78+
}
79+
80+
// Clean up temp file on failure
81+
os.Remove(f.File.Name())
82+
return lastErr
83+
}

repo/fsrepo/migrations/common/utils.go

Lines changed: 17 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package common
22

33
import (
4+
"bytes"
45
"encoding/json"
56
"fmt"
67
"io"
@@ -40,47 +41,42 @@ func Must(err error) {
4041

4142
// WithBackup performs a config file operation with automatic backup and rollback on error
4243
func WithBackup(configPath string, backupSuffix string, fn func(in io.ReadSeeker, out io.Writer) error) error {
43-
in, err := os.Open(configPath)
44+
// Read the entire file into memory first
45+
// This allows us to close the file before doing atomic operations,
46+
// which is necessary on Windows where open files can't be renamed
47+
data, err := os.ReadFile(configPath)
4448
if err != nil {
4549
return err
4650
}
47-
defer in.Close()
4851

49-
// Create backup
50-
backup, err := atomicfile.New(configPath+backupSuffix, 0600)
51-
if err != nil {
52-
return err
53-
}
54-
55-
// Copy to backup
56-
if _, err := backup.ReadFrom(in); err != nil {
57-
Must(backup.Abort())
58-
return err
59-
}
52+
// Create an in-memory reader for the data
53+
in := bytes.NewReader(data)
6054

61-
// Reset input for reading
62-
if _, err := in.Seek(0, io.SeekStart); err != nil {
63-
Must(backup.Abort())
55+
// Create backup using direct write (not atomic, since backup doesn't need to be atomic)
56+
backupPath := configPath + backupSuffix
57+
if err := os.WriteFile(backupPath, data, 0600); err != nil {
6458
return err
6559
}
6660

67-
// Create output file
61+
// Create output file atomically
6862
out, err := atomicfile.New(configPath, 0600)
6963
if err != nil {
70-
Must(backup.Abort())
64+
// Clean up backup on error
65+
os.Remove(backupPath)
7166
return err
7267
}
7368

7469
// Run the conversion function
7570
if err := fn(in, out); err != nil {
7671
Must(out.Abort())
77-
Must(backup.Abort())
72+
// Clean up backup on error
73+
os.Remove(backupPath)
7874
return err
7975
}
8076

81-
// Close everything on success
77+
// Close the output file atomically
8278
Must(out.Close())
83-
Must(backup.Close())
79+
// Backup remains for potential revert
8480

8581
return nil
8682
}

0 commit comments

Comments
 (0)