From 4c19226342256694c3092682720c45d9f1e51a1f Mon Sep 17 00:00:00 2001 From: Theodor Midtlien Date: Thu, 24 Sep 2026 10:23:50 +0200 Subject: [PATCH] [client] Use POSIX style file read/write of json for windows (#7631) * Use POSIX-like file read/write of json for windows + tests: allow renaming an open file. --- .../profilemanager/active_state_test.go | 76 ++++++++++++ util/file.go | 6 +- util/file_nonwindows.go | 16 +++ util/file_read_test.go | 58 +++++++++ util/file_windows.go | 79 ++++++++++++ util/file_windows_test.go | 116 ++++++++++++++++++ 6 files changed, 348 insertions(+), 3 deletions(-) create mode 100644 client/internal/profilemanager/active_state_test.go create mode 100644 util/file_nonwindows.go create mode 100644 util/file_read_test.go create mode 100644 util/file_windows.go create mode 100644 util/file_windows_test.go diff --git a/client/internal/profilemanager/active_state_test.go b/client/internal/profilemanager/active_state_test.go new file mode 100644 index 000000000..3b7fcd29c --- /dev/null +++ b/client/internal/profilemanager/active_state_test.go @@ -0,0 +1,76 @@ +package profilemanager + +import ( + "fmt" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Regression test: a concurrent Get and Set of the ActiveProfileState will +// fail on Windows since the write is a temp file renamed over an open file. +// Windows will refuse to replace a file another handle holds open by default. +func TestActiveProfileState_ReadsDoNotBreakAConcurrentWrite(t *testing.T) { + withTempConfigDir(t, func(configDir string) { + withPatchedGlobals(t, configDir, func() { + sm := &ServiceManager{} + require.NoError(t, sm.CreateDefaultProfile()) + require.NoError(t, sm.SetActiveProfileStateToDefault()) + + const switched = ID("0123456789abcdef0123456789abcdef") + const rounds = 50 + + var wg sync.WaitGroup + errs := make(chan error, 128) + + for i := 0; i < 8; i++ { + wg.Add(1) + go func() { + defer wg.Done() + for r := 0; r < rounds; r++ { + state, err := sm.GetActiveProfileState() + if err != nil { + errs <- fmt.Errorf("read: %w", err) + return + } + if state.ID != defaultProfileName && state.ID != switched { + errs <- fmt.Errorf("read: active profile is %q, which no writer wrote", state.ID) + return + } + } + }() + } + + for i := 0; i < 2; i++ { + wg.Add(1) + go func() { + defer wg.Done() + for r := 0; r < rounds; r++ { + id := switched + if r%2 == 0 { + id = defaultProfileName + } + if err := sm.SetActiveProfileState(&ActiveProfileState{ID: id, Username: "testuser"}); err != nil { + errs <- fmt.Errorf("switch: %w", err) + return + } + } + }() + } + + wg.Wait() + close(errs) + + for err := range errs { + assert.NoError(t, err, "a switch and a read of the active profile state must not collide") + } + + state, err := sm.GetActiveProfileState() + require.NoError(t, err) + assert.Contains(t, []ID{defaultProfileName, switched}, state.ID, + "the file holds whichever switch landed last, not a mix of the two") + }) + }) +} diff --git a/util/file.go b/util/file.go index 52eb91c0f..4eb2f3ece 100644 --- a/util/file.go +++ b/util/file.go @@ -162,7 +162,7 @@ func writeBytes(ctx context.Context, file string, configDir string, configFileNa return fmt.Errorf("after temp file: %w", ctx.Err()) } - if err = os.Rename(tempFileName, file); err != nil { + if err = renameFile(tempFileName, file); err != nil { return fmt.Errorf("move %s to %s: %w", tempFileName, file, err) } @@ -195,7 +195,7 @@ func openOrCreateFile(file string) (*os.File, error) { // ReadJson reads JSON config file and maps to a provided interface func ReadJson(file string, res interface{}) (interface{}, error) { - f, err := os.Open(file) + f, err := openRead(file) if err != nil { return nil, err } @@ -248,7 +248,7 @@ func ListFiles(dir, pattern string) ([]string, error) { func ReadJsonWithEnvSub(file string, res interface{}) (interface{}, error) { envVars := getEnvMap() - f, err := os.Open(file) + f, err := openRead(file) if err != nil { return nil, err } diff --git a/util/file_nonwindows.go b/util/file_nonwindows.go new file mode 100644 index 000000000..c1db10244 --- /dev/null +++ b/util/file_nonwindows.go @@ -0,0 +1,16 @@ +//go:build !windows + +package util + +import "os" + +// openRead opens path for reading. Only Windows needs more than this: there a +// plain open holds the file against the rename that replaces it. +func openRead(path string) (*os.File, error) { + return os.Open(path) +} + +// renameFile replaces newpath with oldpath. +func renameFile(oldpath, newpath string) error { + return os.Rename(oldpath, newpath) +} diff --git a/util/file_read_test.go b/util/file_read_test.go new file mode 100644 index 000000000..d7276798f --- /dev/null +++ b/util/file_read_test.go @@ -0,0 +1,58 @@ +package util + +import ( + "errors" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestReadJson_ReadsTheFile(t *testing.T) { + path := filepath.Join(t.TempDir(), "state.json") + require.NoError(t, os.WriteFile(path, []byte(`{"SomeField": 7}`), 0o600)) + + var got TestConfig + _, err := ReadJson(path, &got) + + require.NoError(t, err) + assert.Equal(t, 7, got.SomeField, "the decoded value") +} + +// Callers tell a missing file from a broken one so they can seed a default in +// its place. The Windows path opens through a root and rebuilds the error, so +// the mapping has to survive that. +func TestReadJson_MissingFileIsErrNotExist(t *testing.T) { + dir := t.TempDir() + + for _, tc := range []struct { + name string + path string + }{ + {"missing file", filepath.Join(dir, "absent.json")}, + {"missing directory", filepath.Join(dir, "absent", "absent.json")}, + } { + t.Run(tc.name, func(t *testing.T) { + var got TestConfig + _, err := ReadJson(tc.path, &got) + + require.Error(t, err) + assert.ErrorIs(t, err, os.ErrNotExist) + assert.Contains(t, err.Error(), tc.path, "the error names the file the caller asked for") + }) + } +} + +func TestReadJson_MalformedFileIsNotErrNotExist(t *testing.T) { + path := filepath.Join(t.TempDir(), "state.json") + require.NoError(t, os.WriteFile(path, []byte("{not json"), 0o600)) + + var got TestConfig + _, err := ReadJson(path, &got) + + require.Error(t, err) + assert.False(t, errors.Is(err, os.ErrNotExist), + "a file that is there but unreadable must not be seeded over: %v", err) +} diff --git a/util/file_windows.go b/util/file_windows.go new file mode 100644 index 000000000..6abf6e308 --- /dev/null +++ b/util/file_windows.go @@ -0,0 +1,79 @@ +package util + +import ( + "errors" + "io/fs" + "os" + "path/filepath" +) + +// openRead opens path for reading without holding it against a rename. +// +// os.Open does not set FILE_SHARE_DELETE on Windows, so you cannot rename an +// open file like on UNIX. This caused concurrency issues with active state +// config file. +// +// os.Root opens through NtCreateFile with delete sharing, which is the +// behaviour Unix has. +// https://cs.opensource.google/go/go/+/refs/tags/go1.27.1:src/os/root_windows.go;drc=a4f5d9bbdbdf42da7e2d7e976ac85753c4db5d75;l=176 +func openRead(path string) (*os.File, error) { + root, err := os.OpenRoot(filepath.Dir(path)) + if err != nil { + // Names the file the caller asked for, not the directory the root + // failed on, so a missing directory reads like a missing file. + return nil, pathError("open", path, err) + } + defer func() { _ = root.Close() }() + + // The file outlives the root: closing a Root closes the directory handle it + // holds, not the files opened through it. + f, err := root.Open(filepath.Base(path)) + if err != nil { + return nil, pathError("open", path, err) + } + return f, nil +} + +// renameFile replaces newpath with oldpath, including while something holds +// newpath open for reading. +// +// os.Root.Rename asks for POSIX semantics, which unlink the destination +// immediately and leave open handles reading the version they opened. +// https://cs.opensource.google/go/go/+/master:src/internal/syscall/windows/at_windows.go;drc=a4f5d9bbdbdf42da7e2d7e976ac85753c4db5d75;l=384 +func renameFile(oldpath, newpath string) error { + dir := filepath.Dir(newpath) + if filepath.Dir(oldpath) != dir { + return os.Rename(oldpath, newpath) + } + + root, err := os.OpenRoot(dir) + if err != nil { + return os.Rename(oldpath, newpath) + } + defer func() { _ = root.Close() }() + + if err := root.Rename(filepath.Base(oldpath), filepath.Base(newpath)); err != nil { + return linkError("rename", oldpath, newpath, err) + } + return nil +} + +// pathError restores the full path on an error from a root, which names the +// file by the base name it was opened with. +func pathError(op, path string, err error) error { + var perr *fs.PathError + if errors.As(err, &perr) { + err = perr.Err + } + return &fs.PathError{Op: op, Path: path, Err: err} +} + +// linkError does the same as pathError for a rename, which reports both files +// by their base names. +func linkError(op, oldpath, newpath string, err error) error { + var lerr *os.LinkError + if errors.As(err, &lerr) { + err = lerr.Err + } + return &os.LinkError{Op: op, Old: oldpath, New: newpath, Err: err} +} diff --git a/util/file_windows_test.go b/util/file_windows_test.go new file mode 100644 index 000000000..eb7ba344f --- /dev/null +++ b/util/file_windows_test.go @@ -0,0 +1,116 @@ +package util + +import ( + "context" + "io" + "os" + "path/filepath" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// seedReplace lays out a write as writeBytes leaves it: the destination that +// exists and the temp file that is to take its place. +func seedReplace(t *testing.T) (src, dst string) { + t.Helper() + dir := t.TempDir() + src = filepath.Join(dir, ".tmpstate.json") + dst = filepath.Join(dir, "state.json") + require.NoError(t, os.WriteFile(src, []byte(`{"SomeField": 2}`), 0o600)) + require.NoError(t, os.WriteFile(dst, []byte(`{"SomeField": 1}`), 0o600)) + return src, dst +} + +// The reader has to share the file for delete, or the rename cannot take +// delete access on it. Regression test. +func TestRenameFile_ReplacesAFileBeingRead(t *testing.T) { + t.Run("a reader that shares delete", func(t *testing.T) { + src, dst := seedReplace(t) + + f, err := openRead(dst) + require.NoError(t, err) + defer f.Close() + + require.Error(t, os.Rename(src, dst), + "delete sharing alone has to be too little, or this test proves nothing") + require.NoError(t, renameFile(src, dst), "POSIX semantics have to get the replace through") + + // The handle stays on the file it opened, so a read in flight finishes + // on that version instead of seeing the replacement. + held, err := io.ReadAll(f) + require.NoError(t, err) + assert.JSONEq(t, `{"SomeField": 1}`, string(held), "the version the reader opened") + + landed, err := os.ReadFile(dst) + require.NoError(t, err) + assert.JSONEq(t, `{"SomeField": 2}`, string(landed), "the version the writer put there") + }) + + t.Run("a reader that does not", func(t *testing.T) { + src, dst := seedReplace(t) + + f, err := os.Open(dst) + require.NoError(t, err) + defer f.Close() + + require.Error(t, renameFile(src, dst), + "a plain read still holds the file, and the caller is owed that error") + }) + + t.Run("no readers at all", func(t *testing.T) { + src, dst := seedReplace(t) + + require.NoError(t, renameFile(src, dst)) + + landed, err := os.ReadFile(dst) + require.NoError(t, err) + assert.JSONEq(t, `{"SomeField": 2}`, string(landed), "the destination holds what replaced it") + }) +} + +// A config rewritten while it is being read, which is the daemon reading the +// active profile against a profile switch writing it. +func TestReadJsonWriteJson_Concurrently(t *testing.T) { + path := filepath.Join(t.TempDir(), "state.json") + require.NoError(t, WriteJson(context.Background(), path, &TestConfig{SomeField: 1})) + + var wg sync.WaitGroup + errs := make(chan error, 128) + + for i := 0; i < 8; i++ { + wg.Add(1) + go func() { + defer wg.Done() + for r := 0; r < 50; r++ { + var got TestConfig + if _, err := ReadJson(path, &got); err != nil { + errs <- err + return + } + } + }() + } + + for i := 0; i < 2; i++ { + wg.Add(1) + go func(writer int) { + defer wg.Done() + for r := 0; r < 50; r++ { + if err := WriteJson(context.Background(), path, &TestConfig{SomeField: writer}); err != nil { + errs <- err + return + } + } + }(i) + } + + wg.Wait() + close(errs) + + for err := range errs { + assert.NoError(t, err, "a read and a write of the same config must not collide") + } +}