[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.

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")
+		})
+	})
+}

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
 	}

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)
+}

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)
+}

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}
+}

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")
+	}
+}