diff --git a/internal/edit/file.go b/internal/edit/file.go index d8f1c161..ce8917ab 100644 --- a/internal/edit/file.go +++ b/internal/edit/file.go @@ -12,6 +12,8 @@ import ( "path/filepath" "strings" "time" + + "github.com/yetone/magpie/internal/steady" ) // Read returns the file contents, or (nil, nil) when the file does not exist. @@ -79,7 +81,7 @@ func WriteAtomic(path string, data []byte) error { cleanup() return err } - if err := os.Rename(tmpName, path); err != nil { + if err := steady.Rename(tmpName, path); err != nil { cleanup() return err } diff --git a/internal/filememo/filememo.go b/internal/filememo/filememo.go index 6e4dec35..0e92e45f 100644 --- a/internal/filememo/filememo.go +++ b/internal/filememo/filememo.go @@ -3,24 +3,37 @@ package filememo import ( + "bytes" "os" "sync" "time" + + "github.com/yetone/magpie/internal/steady" ) type entry struct { mod time.Time size int64 v any + // the bytes parsed, while the file's time is too recent to tell a + // second write by: Linux stamps a file from a clock a tick (a few + // milliseconds) coarse, so two writes of one size inside a tick (two + // accounts swapped) leave the same time and size + raw []byte } +// settled is how old a file's time must be for any later write to stamp +// a later one, many ticks of the coarsest clock. +const settled = 2 * time.Second + var ( mu sync.Mutex seen = map[string]entry{} ) // Read is parse of the file at path, from what it gave last time when the -// file has the same size and time as then. kind tells apart two parses of +// file has the same size and time as then (and, written just now, the same +// bytes). kind tells apart two parses of // one file. What it returns is shared: the caller must not change it. func Read[T any](kind, path string, parse func([]byte) (T, error)) (T, error) { var zero T @@ -32,19 +45,32 @@ func Read[T any](kind, path string, parse func([]byte) (T, error)) (T, error) { mu.Lock() e, ok := seen[key] mu.Unlock() - if ok && e.mod.Equal(fi.ModTime()) && e.size == fi.Size() { + if ok && e.mod.Equal(fi.ModTime()) && e.size == fi.Size() && e.raw == nil { return e.v.(T), nil } - b, err := os.ReadFile(path) + b, err := steady.ReadFile(path) if err != nil { return zero, err } + if ok && e.mod.Equal(fi.ModTime()) && bytes.Equal(b, e.raw) { + if time.Since(e.mod) > settled { + mu.Lock() + e.raw = nil + seen[key] = e + mu.Unlock() + } + return e.v.(T), nil + } v, err := parse(b) if err != nil { return zero, err } mu.Lock() - seen[key] = entry{mod: fi.ModTime(), size: fi.Size(), v: v} + e = entry{mod: fi.ModTime(), size: fi.Size(), v: v} + if time.Since(e.mod) <= settled { + e.raw = b + } + seen[key] = e mu.Unlock() return v, nil } diff --git a/internal/filememo/filememo_test.go b/internal/filememo/filememo_test.go index d75f4e02..314313c5 100644 --- a/internal/filememo/filememo_test.go +++ b/internal/filememo/filememo_test.go @@ -1,6 +1,7 @@ package filememo import ( + "fmt" "os" "path/filepath" "testing" @@ -30,3 +31,22 @@ func TestRead(t *testing.T) { t.Fatal("a file gone is an error") } } + +// two writes of one size inside one tick of the file system's clock (Linux +// stamps a file's time from a clock a few milliseconds coarse) leave the +// same time and size: the second is still read, not the first's parse +func TestReadSameSizeSameTick(t *testing.T) { + p := filepath.Join(t.TempDir(), "f") + parse := func(b []byte) (string, error) { return string(b), nil } + for i := range 200 { + a, b := fmt.Sprintf("a%03d,b", i), fmt.Sprintf("b%03d,a", i) + os.WriteFile(p, []byte(a), 0o600) + if v, _ := Read("tick", p, parse); v != a { + t.Fatalf("read %q, want %q", v, a) + } + os.WriteFile(p, []byte(b), 0o600) + if v, _ := Read("tick", p, parse); v != b { + t.Fatalf("read %q after writing %q: the first write's parse", v, b) + } + } +} diff --git a/internal/plugin/host.js b/internal/plugin/host.js index 17ad098d..442ba6b1 100644 --- a/internal/plugin/host.js +++ b/internal/plugin/host.js @@ -120,8 +120,21 @@ globalThis.fetch = Object.assign(function fetch(input, init) { // ---- auth.json --------------------------------------------------------------- function readAuth() { + let text + for (let i = 0; ; i++) { + try { + text = fs.readFileSync(authPath, "utf8") + break + } catch (e) { + if (e?.code === "ENOENT") return {} + // Windows refuses a file being renamed over for a moment: read as + // none, the next setAuth would write the others' accounts away + if (i >= 50) throw e + pause(10) + } + } try { - const v = JSON.parse(fs.readFileSync(authPath, "utf8")) + const v = JSON.parse(text) return v && typeof v === "object" ? v : {} } catch { return {} @@ -132,7 +145,19 @@ function writeAuth(all) { fs.mkdirSync(path.dirname(authPath), { recursive: true }) const tmp = authPath + ".tmp-" + process.pid fs.writeFileSync(tmp, JSON.stringify(all, null, 2) + "\n", { mode: 0o600 }) - fs.renameSync(tmp, authPath) + // and a file held open by a reader refuses to be renamed over + for (let i = 0; ; i++) { + try { + return fs.renameSync(tmp, authPath) + } catch (e) { + if (i >= 50 || !["EPERM", "EACCES", "EBUSY"].includes(e?.code)) throw e + pause(10) + } + } +} + +function pause(ms) { + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms) } function setAuth(key, info) { diff --git a/internal/plugin/plugin_test.go b/internal/plugin/plugin_test.go index 65e21993..0998f05d 100644 --- a/internal/plugin/plugin_test.go +++ b/internal/plugin/plugin_test.go @@ -12,6 +12,7 @@ import ( "os/exec" "path/filepath" "reflect" + "runtime" "slices" "strings" "sync" @@ -121,7 +122,7 @@ func TestFakePlugin(t *testing.T) { if saved["fakeco"]["type"] != "oauth" || saved["fakeco"]["refresh"] != "r-blue" || saved["fakeco"]["accountId"] != "blue@fake" { t.Fatalf("saved %v", saved) } - if fi, _ := os.Stat(AuthPath()); fi.Mode().Perm() != 0o600 { + if fi, _ := os.Stat(AuthPath()); runtime.GOOS != "windows" && fi.Mode().Perm() != 0o600 { // Windows keeps no mode bits t.Fatalf("plugin-auth.json is %v", fi.Mode().Perm()) } if !SignedIn("fakeco") { diff --git a/internal/plugin/providers.go b/internal/plugin/providers.go index e0be826c..5a4c506a 100644 --- a/internal/plugin/providers.go +++ b/internal/plugin/providers.go @@ -13,6 +13,7 @@ import ( "time" "github.com/yetone/magpie/internal/settings" + "github.com/yetone/magpie/internal/steady" ) // Method is a way a plugin signs in: "oauth" (a browser, then a code @@ -133,7 +134,7 @@ func Providers(ctx context.Context) ([]Provider, error) { provCache, provGood = ps, true provMu.Unlock() if b, err := json.Marshal(ps); err == nil { - _ = os.WriteFile(providersPath(), b, 0o600) + _ = writeWhole(providersPath(), b) } return ps, nil } @@ -145,7 +146,7 @@ func Providers(ctx context.Context) ([]Provider, error) { // why, rather than going as if it were removed. func keepUnloaded(ps, last []Provider) []Provider { if last == nil { - if b, err := os.ReadFile(providersPath()); err == nil { + if b, err := steady.ReadFile(providersPath()); err == nil { _ = json.Unmarshal(b, &last) } } @@ -197,7 +198,7 @@ func Cached() []Provider { good := provGood provMu.Unlock() if ps == nil { - if b, err := os.ReadFile(providersPath()); err == nil { + if b, err := steady.ReadFile(providersPath()); err == nil { _ = json.Unmarshal(b, &ps) } } @@ -302,7 +303,7 @@ func firstNonEmpty(ss ...string) string { func readAuth() map[string]storedAuth { var m map[string]storedAuth - if b, err := os.ReadFile(AuthPath()); err == nil { + if b, err := steady.ReadFile(AuthPath()); err == nil { _ = json.Unmarshal(b, &m) } return m @@ -433,7 +434,7 @@ func SignOut(ctx context.Context, provider, account string) error { return Call(ctx, "signOut", map[string]any{"provider": provider, "account": account}, nil) } var m map[string]json.RawMessage - b, err := os.ReadFile(AuthPath()) + b, err := steady.ReadFile(AuthPath()) if err != nil { return nil } @@ -446,7 +447,7 @@ func SignOut(ctx context.Context, provider, account string) error { } } b, _ = json.MarshalIndent(m, "", " ") - if err := os.WriteFile(AuthPath(), append(b, '\n'), 0o600); err != nil { + if err := writeWhole(AuthPath(), append(b, '\n')); err != nil { return err } changed() @@ -540,7 +541,7 @@ func Check(ctx context.Context, provider, account string) (Checked, error) { // Auths are provider's sign-ins as plugin-auth.json keeps them, by key. func Auths(provider string) map[string]map[string]any { var m map[string]map[string]any - if b, err := os.ReadFile(AuthPath()); err == nil { + if b, err := steady.ReadFile(AuthPath()); err == nil { _ = json.Unmarshal(b, &m) } out := map[string]map[string]any{} diff --git a/internal/plugin/store.go b/internal/plugin/store.go index 14df2893..a44f893b 100644 --- a/internal/plugin/store.go +++ b/internal/plugin/store.go @@ -33,6 +33,7 @@ import ( "github.com/yetone/magpie/internal/netproxy" "github.com/yetone/magpie/internal/proc" "github.com/yetone/magpie/internal/settings" + "github.com/yetone/magpie/internal/steady" ) // Entry is one plugin the user added. @@ -69,12 +70,33 @@ func listPath() string { return filepath.Join(settings.Dir(), "plugins.json") } // Load reads plugins.json. func Load() List { var l List - if b, err := os.ReadFile(listPath()); err == nil { + if b, err := steady.ReadFile(listPath()); err == nil { _ = json.Unmarshal(b, &l) } return l } +// writeWhole writes b to p by a rename, so a magpie or the host reading +// p meanwhile reads the old file or the new one, never one half-written; +// read back with steady.ReadFile, which waits out the rename on Windows. +func writeWhole(p string, b []byte) error { + f, err := os.CreateTemp(filepath.Dir(p), filepath.Base(p)+".*") + if err != nil { + return err + } + _, err = f.Write(b) + if cerr := f.Close(); err == nil { + err = cerr + } + if err == nil { + err = steady.Rename(f.Name(), p) + } + if err != nil { + os.Remove(f.Name()) + } + return err +} + func save(l List) error { b, err := json.MarshalIndent(l, "", " ") if err != nil { @@ -83,7 +105,7 @@ func save(l List) error { if err := os.MkdirAll(settings.Dir(), 0o700); err != nil { return err } - if err := os.WriteFile(listPath(), append(b, '\n'), 0o600); err != nil { + if err := writeWhole(listPath(), append(b, '\n')); err != nil { return err } listSeen.Lock() diff --git a/internal/provider/account_test.go b/internal/provider/account_test.go index bbb683da..de2d1fa0 100644 --- a/internal/provider/account_test.go +++ b/internal/provider/account_test.go @@ -222,6 +222,8 @@ func claudeHome(t *testing.T) string { isolate(t) home := t.TempDir() t.Setenv("HOME", home) + // Windows finds the home in USERPROFILE + t.Setenv("USERPROFILE", home) t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, ".config")) t.Setenv("XDG_CACHE_HOME", filepath.Join(home, ".cache")) forgetClaudeCredential() diff --git a/internal/provider/devin_test.go b/internal/provider/devin_test.go index 7519802c..64e84652 100644 --- a/internal/provider/devin_test.go +++ b/internal/provider/devin_test.go @@ -174,6 +174,9 @@ func TestDevinCredentials(t *testing.T) { } func TestDevinSignIn(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("the fake devin is a shell script") + } home := claudeHome(t) t.Setenv("XDG_DATA_HOME", filepath.Join(home, "data")) @@ -242,6 +245,9 @@ func TestDevinSignIn(t *testing.T) { // own: it gets a home of magpie's, the CLI's file is left alone, and the // gateway can use either. func TestDevinSeveralAccounts(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("the fake devin is a shell script") + } home := claudeHome(t) data := filepath.Join(home, "data") t.Setenv("XDG_DATA_HOME", data) @@ -362,6 +368,9 @@ const devinUnreachable = `printf '%s\n' 'Logged in (via Devin).' '' 'User / team // either. func fakeDevin(t *testing.T, exe string) { t.Helper() + if runtime.GOOS == "windows" { + t.Skip("the fake devin is a shell script") + } old := DevinExecutable DevinExecutable = func() string { return exe } t.Cleanup(func() { diff --git a/internal/provider/google_test.go b/internal/provider/google_test.go index 7a5f22f4..f80ccea0 100644 --- a/internal/provider/google_test.go +++ b/internal/provider/google_test.go @@ -79,6 +79,7 @@ func googleSandbox(t *testing.T, f *fakeGoogle) { t.Helper() home := t.TempDir() t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) // Windows's home t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, ".config")) t.Setenv("XDG_CACHE_HOME", filepath.Join(home, ".cache")) srv := httptest.NewServer(f) diff --git a/internal/provider/kiro.go b/internal/provider/kiro.go index b3a15e66..8daccefc 100644 --- a/internal/provider/kiro.go +++ b/internal/provider/kiro.go @@ -33,6 +33,7 @@ import ( "github.com/yetone/magpie/internal/catalog" "github.com/yetone/magpie/internal/edit" "github.com/yetone/magpie/internal/proc" + "github.com/yetone/magpie/internal/steady" ) // KiroExecutable finds kiro-cli, which refreshes its own sign-in; a var so @@ -464,7 +465,7 @@ func writeFileAtomic(path string, b []byte) error { if err := os.WriteFile(tmp, b, 0o600); err != nil { return err } - return os.Rename(tmp, path) + return steady.Rename(tmp, path) } // kiroPost posts to a sign-in endpoint. diff --git a/internal/provider/logins.go b/internal/provider/logins.go index 77359cf4..cc5f4c9c 100644 --- a/internal/provider/logins.go +++ b/internal/provider/logins.go @@ -24,6 +24,7 @@ import ( "github.com/yetone/magpie/internal/edit" "github.com/yetone/magpie/internal/filememo" "github.com/yetone/magpie/internal/plugin" + "github.com/yetone/magpie/internal/steady" ) // Login is a remembered subscription account, without its secrets. @@ -148,7 +149,7 @@ func writePrivate(path string, b []byte) error { if err := tmp.Close(); err != nil { return err } - return os.Rename(tmp.Name(), path) + return steady.Rename(tmp.Name(), path) } func upsertLogin(ls []savedLogin, l savedLogin) []savedLogin { diff --git a/internal/provider/migrate_test.go b/internal/provider/migrate_test.go index 14fce643..1510acd7 100644 --- a/internal/provider/migrate_test.go +++ b/internal/provider/migrate_test.go @@ -196,13 +196,15 @@ func TestMoveToPlugin(t *testing.T) { } // a built-in with no usage card of its own (Devin) keeps the plugin's cards := 0 - for _, q := range fetchSubscriptionUsage() { + qs := fetchSubscriptionUsage() + for _, q := range qs { if q.Provider == "fakeco" { cards++ } } if cards == 0 { - t.Fatal("the moved plugin's accounts show no usage") + t.Fatalf("the moved plugin's accounts show no usage: moved %v, plugins %d, logins %d, cards %+v", + Moved("fakeco"), len(plugin.Cached()), len(pluginUsageLogins(mustPlugin(t))), qs) } // the user reorders on the plugin, and it renews a's token; back, the diff --git a/internal/steady/steady.go b/internal/steady/steady.go new file mode 100644 index 00000000..be9c1099 --- /dev/null +++ b/internal/steady/steady.go @@ -0,0 +1,46 @@ +// Package steady reads and replaces files that another process may be +// replacing at that moment. On Windows a file being renamed over refuses +// to open, and one held open refuses to be renamed over, both for a +// moment ("Access is denied"): read as none, a file of accounts is no +// accounts, and the next write leaves them out. Elsewhere a rename is +// atomic and these are os.ReadFile and os.Rename. +package steady + +import ( + "errors" + "io/fs" + "os" + "runtime" + "time" +) + +// tries and pause bound the wait to half a second: what holds the file is +// another read or rename, done in milliseconds. +const ( + tries = 50 + pause = 10 * time.Millisecond +) + +func retry(f func() error) error { + err := f() + for i := 0; err != nil && runtime.GOOS == "windows" && !errors.Is(err, fs.ErrNotExist) && i < tries; i++ { + time.Sleep(pause) + err = f() + } + return err +} + +// ReadFile is os.ReadFile, waiting out a file being replaced. +func ReadFile(p string) ([]byte, error) { + var b []byte + err := retry(func() (err error) { + b, err = os.ReadFile(p) + return err + }) + return b, err +} + +// Rename is os.Rename, waiting out a reader holding newpath open. +func Rename(oldpath, newpath string) error { + return retry(func() error { return os.Rename(oldpath, newpath) }) +} diff --git a/internal/steady/steady_windows_test.go b/internal/steady/steady_windows_test.go new file mode 100644 index 00000000..a26b8c9e --- /dev/null +++ b/internal/steady/steady_windows_test.go @@ -0,0 +1,60 @@ +package steady + +import ( + "os" + "path/filepath" + "syscall" + "testing" + "time" +) + +// a reader holding the file without FILE_SHARE_DELETE, as other programs +// open it, for a moment: os.Rename over it is refused, Rename waits it out +func TestRenameOverAFileHeldOpen(t *testing.T) { + dir := t.TempDir() + p, tmp := filepath.Join(dir, "logins.json"), filepath.Join(dir, "logins.json.tmp") + os.WriteFile(p, []byte("old"), 0o600) + os.WriteFile(tmp, []byte("new"), 0o600) + name, _ := syscall.UTF16PtrFromString(p) + h, err := syscall.CreateFile(name, syscall.GENERIC_READ, syscall.FILE_SHARE_READ, nil, syscall.OPEN_EXISTING, 0, 0) + if err != nil { + t.Fatal(err) + } + if err := os.Rename(tmp, p); err == nil { + t.Fatal("renamed over a file held open: nothing to wait out") + } + go func() { + time.Sleep(100 * time.Millisecond) + syscall.CloseHandle(h) + }() + if err := Rename(tmp, p); err != nil { + t.Fatal(err) + } + if b, err := ReadFile(p); err != nil || string(b) != "new" { + t.Fatalf("read %q, %v", b, err) + } +} + +// and a file held so refuses to be read until let go +func TestReadAFileHeldOpen(t *testing.T) { + p := filepath.Join(t.TempDir(), "plugin-auth.json") + os.WriteFile(p, []byte("accounts"), 0o600) + name, _ := syscall.UTF16PtrFromString(p) + h, err := syscall.CreateFile(name, syscall.GENERIC_WRITE, 0, nil, syscall.OPEN_EXISTING, 0, 0) + if err != nil { + t.Fatal(err) + } + if _, err := os.ReadFile(p); err == nil { + t.Fatal("read a file held unshared: nothing to wait out") + } + go func() { + time.Sleep(100 * time.Millisecond) + syscall.CloseHandle(h) + }() + if b, err := ReadFile(p); err != nil || string(b) != "accounts" { + t.Fatalf("read %q, %v", b, err) + } + if _, err := ReadFile(filepath.Join(filepath.Dir(p), "none")); !os.IsNotExist(err) { + t.Fatalf("a file not there: %v", err) + } +}