Skip to content

Commit 671037f

Browse files
skarimskarim
committed
Add atomic stack storage and migration primitives
1 parent 94dbc83 commit 671037f

11 files changed

Lines changed: 1886 additions & 48 deletions

File tree

‎cmd/rebase.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -520,7 +520,7 @@ func saveRebaseState(gitDir string, state *rebaseState) error {
520520
if err != nil {
521521
return fmt.Errorf("error serializing rebase state: %w", err)
522522
}
523-
if err := os.WriteFile(filepath.Join(gitDir, rebaseStateFile), data, 0644); err != nil {
523+
if err := stack.WriteAtomic(filepath.Join(gitDir, rebaseStateFile), data); err != nil {
524524
return fmt.Errorf("error writing rebase state: %w", err)
525525
}
526526
return nil

‎internal/modify/state.go‎

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ import (
77
"os"
88
"path/filepath"
99
"time"
10+
11+
"github.com/github/gh-stack/internal/stack"
1012
)
1113

1214
const stateFileName = "gh-stack-modify-state"
@@ -96,18 +98,9 @@ func SaveState(gitDir string, state *StateFile) error {
9698
if err != nil {
9799
return fmt.Errorf("marshaling modify state: %w", err)
98100
}
99-
target := StatePath(gitDir)
100-
tmp := target + ".tmp"
101-
if err := os.WriteFile(tmp, data, 0644); err != nil {
101+
if err := stack.WriteAtomic(StatePath(gitDir), data); err != nil {
102102
return fmt.Errorf("writing modify state: %w", err)
103103
}
104-
// Remove existing target before rename for Windows compatibility
105-
// (os.Rename fails on Windows if the target already exists).
106-
_ = os.Remove(target)
107-
if err := os.Rename(tmp, target); err != nil {
108-
_ = os.Remove(tmp)
109-
return fmt.Errorf("committing modify state: %w", err)
110-
}
111104
return nil
112105
}
113106

‎internal/stack/atomic.go‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
package stack
2+
3+
import (
4+
"errors"
5+
"fmt"
6+
"os"
7+
"path/filepath"
8+
)
9+
10+
// WriteAtomic publishes data at path using a fully written temporary file in
11+
// the same directory. It preserves an existing regular file's permissions and
12+
// uses 0644 for a new file. The parent directory must already exist.
13+
// It does not acquire locks; callers must serialize mutations.
14+
func WriteAtomic(path string, data []byte) error {
15+
return writeFileAtomic(path, data, 0644, true)
16+
}
17+
18+
// writeFileAtomic publishes a fully written sibling of path. When replace is
19+
// false, an existing destination is never overwritten, even on a racing create.
20+
func writeFileAtomic(path string, data []byte, mode os.FileMode, replace bool) (err error) {
21+
if replace {
22+
info, statErr := os.Lstat(path)
23+
switch {
24+
case statErr == nil:
25+
if !info.Mode().IsRegular() {
26+
return fmt.Errorf("cannot replace non-regular file %q", path)
27+
}
28+
mode = info.Mode().Perm()
29+
case !errors.Is(statErr, os.ErrNotExist):
30+
return statErr
31+
}
32+
}
33+
34+
f, err := os.CreateTemp(filepath.Dir(path), "."+filepath.Base(path)+"-*")
35+
if err != nil {
36+
return err
37+
}
38+
temp := f.Name()
39+
closed := false
40+
defer func() {
41+
if !closed {
42+
err = errors.Join(err, f.Close())
43+
}
44+
if removeErr := os.Remove(temp); removeErr != nil && !errors.Is(removeErr, os.ErrNotExist) {
45+
err = errors.Join(err, fmt.Errorf("removing temporary file: %w", removeErr))
46+
}
47+
}()
48+
49+
if err := f.Chmod(mode); err != nil {
50+
return err
51+
}
52+
if _, err := f.Write(data); err != nil {
53+
return err
54+
}
55+
if err := f.Sync(); err != nil {
56+
return err
57+
}
58+
closed = true
59+
if err := f.Close(); err != nil {
60+
return err
61+
}
62+
if err := publishFile(temp, path, replace); err != nil {
63+
return err
64+
}
65+
return syncDirectory(filepath.Dir(path))
66+
}

‎internal/stack/atomic_unix.go‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
//go:build !windows
2+
3+
package stack
4+
5+
import (
6+
"errors"
7+
"os"
8+
)
9+
10+
func readStateFile(path string) ([]byte, error) {
11+
return os.ReadFile(path)
12+
}
13+
14+
func publishFile(temp, path string, replace bool) error {
15+
if replace {
16+
return os.Rename(temp, path)
17+
}
18+
// Linking publishes a complete file without replacing an existing backup.
19+
// The temporary name is removed by writeFileAtomic.
20+
return os.Link(temp, path)
21+
}
22+
23+
func syncDirectory(path string) error {
24+
f, err := os.Open(path)
25+
if err != nil {
26+
return err
27+
}
28+
return errors.Join(f.Sync(), f.Close())
29+
}

‎internal/stack/atomic_windows.go‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
//go:build windows
2+
3+
package stack
4+
5+
import (
6+
"errors"
7+
"io"
8+
"os"
9+
"path/filepath"
10+
"strings"
11+
"unsafe"
12+
13+
"golang.org/x/sys/windows"
14+
)
15+
16+
func readStateFile(path string) ([]byte, error) {
17+
name, err := windowsFilePath(path)
18+
if err != nil {
19+
return nil, err
20+
}
21+
// Let publication replace the name while readers finish with the old file.
22+
handle, err := windows.CreateFile(&name[0], windows.GENERIC_READ,
23+
windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE,
24+
nil, windows.OPEN_EXISTING, windows.FILE_ATTRIBUTE_NORMAL, 0)
25+
if err != nil {
26+
return nil, &os.PathError{Op: "open", Path: path, Err: err}
27+
}
28+
f := os.NewFile(uintptr(handle), path)
29+
data, err := io.ReadAll(f)
30+
return data, errors.Join(err, f.Close())
31+
}
32+
33+
func publishFile(temp, path string, replace bool) error {
34+
from, err := windowsFilePath(temp)
35+
if err != nil {
36+
return err
37+
}
38+
to, err := windowsFilePath(path)
39+
if err != nil {
40+
return err
41+
}
42+
// Never remove the destination first, or allow a cross-volume copy/delete.
43+
if replace {
44+
err = replaceFileWindows(from, to)
45+
} else {
46+
err = windows.MoveFileEx(&from[0], &to[0], windows.MOVEFILE_WRITE_THROUGH)
47+
}
48+
if err != nil {
49+
return &os.LinkError{Op: "publish", Old: temp, New: path, Err: err}
50+
}
51+
return nil
52+
}
53+
54+
func replaceFileWindows(from, to []uint16) error {
55+
handle, err := windows.CreateFile(&from[0], windows.DELETE|windows.GENERIC_WRITE,
56+
windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE,
57+
nil, windows.OPEN_EXISTING, windows.FILE_ATTRIBUTE_NORMAL|windows.FILE_FLAG_WRITE_THROUGH, 0)
58+
if err != nil {
59+
return err
60+
}
61+
62+
// FILE_RENAME_INFO has pointer-sized padding and a trailing UTF-16 name.
63+
type renameInfo struct {
64+
Flags uint32
65+
RootDirectory windows.Handle
66+
FileNameLength uint32
67+
FileName [1]uint16
68+
}
69+
buffer := make([]byte, int(unsafe.Sizeof(renameInfo{}))+len(to)*2)
70+
info := (*renameInfo)(unsafe.Pointer(&buffer[0]))
71+
info.Flags = windows.FILE_RENAME_REPLACE_IF_EXISTS | windows.FILE_RENAME_POSIX_SEMANTICS
72+
info.FileNameLength = uint32(len(to)-1) * 2
73+
copy(unsafe.Slice(&info.FileName[0], len(to)), to)
74+
75+
err = windows.SetFileInformationByHandle(handle, windows.FileRenameInfoEx, &buffer[0], uint32(len(buffer)))
76+
if err == nil {
77+
return errors.Join(windows.FlushFileBuffers(handle), windows.CloseHandle(handle))
78+
}
79+
if closeErr := windows.CloseHandle(handle); closeErr != nil {
80+
return errors.Join(err, closeErr)
81+
}
82+
83+
// Older Windows versions/filesystems may lack POSIX rename. Only fall back
84+
// for unsupported operations; legacy rename still reports open-reader errors.
85+
if errors.Is(err, windows.ERROR_NOT_SUPPORTED) || errors.Is(err, windows.ERROR_INVALID_PARAMETER) ||
86+
errors.Is(err, windows.ERROR_INVALID_FUNCTION) {
87+
return windows.MoveFileEx(&from[0], &to[0], windows.MOVEFILE_REPLACE_EXISTING|windows.MOVEFILE_WRITE_THROUGH)
88+
}
89+
return err
90+
}
91+
92+
func windowsFilePath(path string) ([]uint16, error) {
93+
path, err := filepath.Abs(path)
94+
if err != nil {
95+
return nil, err
96+
}
97+
switch {
98+
case strings.HasPrefix(path, `\\?\`), strings.HasPrefix(path, `\\.\`):
99+
case strings.HasPrefix(path, `\\`):
100+
path = `\\?\UNC\` + path[2:]
101+
default:
102+
path = `\\?\` + path
103+
}
104+
return windows.UTF16FromString(path)
105+
}
106+
107+
func syncDirectory(string) error {
108+
// Windows has no directory fsync; publication flushes the file or uses WRITE_THROUGH.
109+
return nil
110+
}

‎internal/stack/atomic_windows_test.go‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
//go:build windows
2+
3+
package stack
4+
5+
import (
6+
"io"
7+
"os"
8+
"path/filepath"
9+
"testing"
10+
11+
"github.com/stretchr/testify/assert"
12+
"github.com/stretchr/testify/require"
13+
"golang.org/x/sys/windows"
14+
)
15+
16+
func TestAtomicPublication_HeldWindowsReader(t *testing.T) {
17+
for _, tt := range []struct {
18+
name string
19+
replace bool
20+
}{
21+
{"replace", true},
22+
{"exclusive", false},
23+
} {
24+
t.Run(tt.name, func(t *testing.T) {
25+
dir := t.TempDir()
26+
path := filepath.Join(dir, "state with spaces")
27+
original, replacement := []byte("original recovery state"), []byte("new state")
28+
require.NoError(t, WriteAtomic(path, original))
29+
name, err := windowsFilePath(path)
30+
require.NoError(t, err)
31+
handle, err := windows.CreateFile(&name[0], windows.GENERIC_READ,
32+
windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE,
33+
nil, windows.OPEN_EXISTING, windows.FILE_ATTRIBUTE_NORMAL, 0)
34+
require.NoError(t, err)
35+
reader := os.NewFile(uintptr(handle), path)
36+
t.Cleanup(func() { assert.NoError(t, reader.Close()) })
37+
38+
err = writeFileAtomic(path, replacement, 0644, tt.replace)
39+
if tt.replace {
40+
require.NoError(t, err)
41+
} else {
42+
require.ErrorIs(t, err, os.ErrExist)
43+
}
44+
old, err := io.ReadAll(reader)
45+
require.NoError(t, err)
46+
assert.Equal(t, original, old, "the held reader must retain the original file")
47+
current, err := readStateFile(path)
48+
require.NoError(t, err)
49+
if tt.replace {
50+
assert.Equal(t, replacement, current, "new readers must see the published file")
51+
} else {
52+
assert.Equal(t, original, current, "exclusive publication must not replace the target")
53+
}
54+
temps, err := filepath.Glob(filepath.Join(dir, ".state with spaces-*"))
55+
require.NoError(t, err)
56+
assert.Empty(t, temps)
57+
})
58+
}
59+
}

0 commit comments

Comments
 (0)