From ab0b17d1e59b713b4817801b4abb18bcdab69b2d Mon Sep 17 00:00:00 2001 From: Amp Date: Sun, 19 Jul 2026 17:03:55 +0000 Subject: [PATCH] fix: make extension installs transactional Co-authored-by: Pierre Bruno --- internal/installtxn/installtxn.go | 147 +++++++++++++++++++++++++ internal/installtxn/installtxn_test.go | 78 +++++++++++++ internal/installtxn/lock_unix.go | 29 +++++ internal/installtxn/lock_windows.go | 31 ++++++ internal/plugins/install.go | 63 ++++++++--- internal/plugins/install_test.go | 33 ++++++ internal/skills/install.go | 68 ++++++++---- internal/skills/install_test.go | 32 ++++++ 8 files changed, 444 insertions(+), 37 deletions(-) create mode 100644 internal/installtxn/installtxn.go create mode 100644 internal/installtxn/installtxn_test.go create mode 100644 internal/installtxn/lock_unix.go create mode 100644 internal/installtxn/lock_windows.go diff --git a/internal/installtxn/installtxn.go b/internal/installtxn/installtxn.go new file mode 100644 index 000000000..da4f2ad45 --- /dev/null +++ b/internal/installtxn/installtxn.go @@ -0,0 +1,147 @@ +// Package installtxn provides the cross-process filesystem transaction used by +// plugin and skill installation. Callers stage content before taking the lock, +// then commit the content swap and lockfile update together while holding it. +package installtxn + +import ( + "errors" + "fmt" + "os" + "path/filepath" +) + +const lockFileName = ".zero-install.lock" + +// Lock takes the per-install-root cross-process lock. It blocks until any other +// installer or remover using dir has completed. +func Lock(dir string) (func(), error) { + if err := os.MkdirAll(dir, 0o755); err != nil { + return nil, fmt.Errorf("create install dir: %w", err) + } + return lockFile(filepath.Join(dir, lockFileName)) +} + +// StageDir creates an install workspace on the target filesystem. Content must +// be built and validated in the returned stage directory before CommitDir is +// called. cleanup is always safe to call. +func StageDir(dir string) (stage string, cleanup func(), err error) { + if err := os.MkdirAll(dir, 0o755); err != nil { + return "", func() {}, fmt.Errorf("create install dir: %w", err) + } + workspace, err := os.MkdirTemp(dir, ".zero-install-txn-") + if err != nil { + return "", func() {}, fmt.Errorf("create install staging dir: %w", err) + } + return filepath.Join(workspace, "staged"), func() { cleanupWorkspace(workspace) }, nil +} + +// CommitDir replaces target with staged and runs publish while retaining the +// previous target. If either the swap or publish fails, the previous target is +// restored (or the new target is removed for a first install). +// +// The caller must hold the install-root lock returned by Lock. +func CommitDir(target string, staged string, publish func() error) error { + workspace := filepath.Dir(staged) + backup := filepath.Join(workspace, "previous") + hadPrevious := false + if _, err := os.Stat(target); err == nil { + if err := os.Rename(target, backup); err != nil { + return fmt.Errorf("retain previous install: %w", err) + } + hadPrevious = true + } else if !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("inspect previous install: %w", err) + } + + if err := os.Rename(staged, target); err != nil { + if hadPrevious { + if restoreErr := os.Rename(backup, target); restoreErr != nil { + return errors.Join(fmt.Errorf("publish staged install: %w", err), fmt.Errorf("restore previous install: %w", restoreErr)) + } + } + return fmt.Errorf("publish staged install: %w", err) + } + if err := publish(); err != nil { + return rollback(target, backup, hadPrevious, err) + } + if hadPrevious { + _ = os.RemoveAll(backup) + } + cleanupWorkspace(workspace) + return nil +} + +// RemoveDir removes target and runs publish while retaining the target until +// publish succeeds. A publish failure restores the directory. +// +// The caller must hold the install-root lock returned by Lock. +func RemoveDir(target string, publish func() error) error { + workspace, err := os.MkdirTemp(filepath.Dir(target), ".zero-install-txn-") + if err != nil { + return fmt.Errorf("create removal staging dir: %w", err) + } + defer cleanupWorkspace(workspace) + backup := filepath.Join(workspace, "previous") + if err := os.Rename(target, backup); err != nil { + return fmt.Errorf("retain removed install: %w", err) + } + if err := publish(); err != nil { + if restoreErr := os.Rename(backup, target); restoreErr != nil { + return errors.Join(err, fmt.Errorf("restore removed install: %w", restoreErr)) + } + return err + } + _ = os.RemoveAll(backup) + return nil +} + +func rollback(target string, backup string, hadPrevious bool, cause error) error { + if err := os.RemoveAll(target); err != nil { + return errors.Join(cause, fmt.Errorf("remove failed install: %w", err)) + } + if hadPrevious { + if err := os.Rename(backup, target); err != nil { + return errors.Join(cause, fmt.Errorf("restore previous install: %w", err)) + } + } + return cause +} + +// cleanupWorkspace never removes a retained previous install. If rollback was +// unable to restore it (for example because Windows still has a target file +// open), preserving the workspace is safer than turning a recoverable error +// into data loss. +func cleanupWorkspace(workspace string) { + if _, err := os.Stat(filepath.Join(workspace, "previous")); err == nil { + return + } + _ = os.RemoveAll(workspace) +} + +// WriteFileAtomically publishes data by renaming a complete sibling temporary +// file over path. The caller is responsible for any surrounding transaction +// lock. +func WriteFileAtomically(path string, data []byte, perm os.FileMode) error { + temp, err := os.CreateTemp(filepath.Dir(path), ".zero-lockfile-") + if err != nil { + return err + } + tempPath := temp.Name() + defer func() { _ = os.Remove(tempPath) }() + if err := temp.Chmod(perm); err != nil { + _ = temp.Close() + return err + } + if _, err := temp.Write(data); err != nil { + _ = temp.Close() + return err + } + if err := temp.Sync(); err != nil { + _ = temp.Close() + return err + } + if err := temp.Close(); err != nil { + return err + } + return replaceFile(tempPath, path) +} diff --git a/internal/installtxn/installtxn_test.go b/internal/installtxn/installtxn_test.go new file mode 100644 index 000000000..fdd27e4c9 --- /dev/null +++ b/internal/installtxn/installtxn_test.go @@ -0,0 +1,78 @@ +package installtxn + +import ( + "errors" + "os" + "path/filepath" + "testing" +) + +func TestCommitDirRestoresPreviousInstallWhenPublishFails(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "demo") + if err := os.MkdirAll(target, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(target, "version"), []byte("old"), 0o644); err != nil { + t.Fatal(err) + } + staged, cleanup, err := StageDir(root) + if err != nil { + t.Fatal(err) + } + defer cleanup() + if err := os.MkdirAll(staged, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(staged, "version"), []byte("new"), 0o644); err != nil { + t.Fatal(err) + } + + publishErr := errors.New("publish failed") + err = CommitDir(target, staged, func() error { return publishErr }) + if !errors.Is(err, publishErr) { + t.Fatalf("CommitDir error = %v, want publish failure", err) + } + data, err := os.ReadFile(filepath.Join(target, "version")) + if err != nil { + t.Fatalf("read restored install: %v", err) + } + if string(data) != "old" { + t.Fatalf("restored content = %q, want old", data) + } +} + +func TestCommitDirRemovesFirstInstallWhenPublishFails(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "demo") + staged, cleanup, err := StageDir(root) + if err != nil { + t.Fatal(err) + } + defer cleanup() + if err := os.MkdirAll(staged, 0o755); err != nil { + t.Fatal(err) + } + + err = CommitDir(target, staged, func() error { return errors.New("publish failed") }) + if err == nil { + t.Fatal("CommitDir unexpectedly succeeded") + } + if _, statErr := os.Stat(target); !errors.Is(statErr, os.ErrNotExist) { + t.Fatalf("failed first install remains at target: %v", statErr) + } +} + +func TestCleanupWorkspacePreservesRetainedPreviousInstall(t *testing.T) { + workspace := t.TempDir() + previous := filepath.Join(workspace, "previous") + if err := os.MkdirAll(previous, 0o755); err != nil { + t.Fatal(err) + } + + cleanupWorkspace(workspace) + + if _, err := os.Stat(previous); err != nil { + t.Fatalf("cleanup removed retained previous install: %v", err) + } +} diff --git a/internal/installtxn/lock_unix.go b/internal/installtxn/lock_unix.go new file mode 100644 index 000000000..e3917261b --- /dev/null +++ b/internal/installtxn/lock_unix.go @@ -0,0 +1,29 @@ +//go:build !windows + +package installtxn + +import ( + "fmt" + "os" + + "golang.org/x/sys/unix" +) + +func lockFile(path string) (func(), error) { + file, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0o600) + if err != nil { + return nil, fmt.Errorf("open install lock: %w", err) + } + if err := unix.Flock(int(file.Fd()), unix.LOCK_EX); err != nil { + _ = file.Close() + return nil, fmt.Errorf("lock install root: %w", err) + } + return func() { + _ = unix.Flock(int(file.Fd()), unix.LOCK_UN) + _ = file.Close() + }, nil +} + +func replaceFile(source string, target string) error { + return os.Rename(source, target) +} diff --git a/internal/installtxn/lock_windows.go b/internal/installtxn/lock_windows.go new file mode 100644 index 000000000..1132d0238 --- /dev/null +++ b/internal/installtxn/lock_windows.go @@ -0,0 +1,31 @@ +//go:build windows + +package installtxn + +import ( + "fmt" + "os" + + "golang.org/x/sys/windows" +) + +func lockFile(path string) (func(), error) { + file, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0o600) + if err != nil { + return nil, fmt.Errorf("open install lock: %w", err) + } + handle := windows.Handle(file.Fd()) + overlapped := new(windows.Overlapped) + if err := windows.LockFileEx(handle, windows.LOCKFILE_EXCLUSIVE_LOCK, 0, 1, 0, overlapped); err != nil { + _ = file.Close() + return nil, fmt.Errorf("lock install root: %w", err) + } + return func() { + _ = windows.UnlockFileEx(handle, 0, 1, 0, overlapped) + _ = file.Close() + }, nil +} + +func replaceFile(source string, target string) error { + return windows.MoveFileEx(windows.StringToUTF16Ptr(source), windows.StringToUTF16Ptr(target), windows.MOVEFILE_REPLACE_EXISTING|windows.MOVEFILE_WRITE_THROUGH) +} diff --git a/internal/plugins/install.go b/internal/plugins/install.go index c201b27ea..1335b67be 100644 --- a/internal/plugins/install.go +++ b/internal/plugins/install.go @@ -19,6 +19,8 @@ import ( "path/filepath" "sort" "strings" + + "github.com/Gitlawb/zero/internal/installtxn" ) // manifestFileName is the plugin manifest filename, matching the loader. @@ -128,6 +130,32 @@ func Install(ctx context.Context, options InstallOptions) (InstallResult, error) return InstallResult{}, fmt.Errorf("hash plugin: %w", err) } + staged, cleanupStage, err := installtxn.StageDir(dir) + if err != nil { + return InstallResult{}, err + } + defer cleanupStage() + // Copy the whole plugin tree (entry scripts, prompts, skills) into a sibling + // staging area. Copy DATA only — never execute it. + if err := copyTree(pluginDir, staged); err != nil { + return InstallResult{}, fmt.Errorf("stage plugin: %w", err) + } + stagedHash, err := hashTree(staged) + if err != nil { + return InstallResult{}, fmt.Errorf("validate staged plugin: %w", err) + } + if stagedHash != hash { + return InstallResult{}, errors.New("validate staged plugin: copied content hash differs from source") + } + + unlock, err := installtxn.Lock(dir) + if err != nil { + return InstallResult{}, err + } + defer unlock() + + // Re-read under the cross-process lock. Another install may have updated the + // lockfile while this plugin was fetched and staged. lock, err := ReadLock(dir) if err != nil { return InstallResult{}, err @@ -138,20 +166,10 @@ func Install(ctx context.Context, options InstallOptions) (InstallResult, error) } target := filepath.Join(dir, id) - if err := os.MkdirAll(dir, 0o755); err != nil { - return InstallResult{}, fmt.Errorf("create plugins dir: %w", err) - } - if err := os.RemoveAll(target); err != nil { - return InstallResult{}, fmt.Errorf("clear previous plugin: %w", err) - } - // Copy the whole plugin tree (entry scripts, prompts, skills) so the installed - // plugin is runnable through activation. Copy DATA only — never execute it. - if err := copyTree(pluginDir, target); err != nil { - return InstallResult{}, fmt.Errorf("copy plugin: %w", err) - } - lock[id] = LockEntry{Source: source, Hash: hash} - if err := writeLock(dir, lock); err != nil { + if err := installtxn.CommitDir(target, staged, func() error { + return writeLock(dir, lock) + }); err != nil { return InstallResult{}, err } @@ -182,6 +200,12 @@ func Remove(dir string, id string) error { return fmt.Errorf("invalid plugin id %q", id) } + unlock, err := installtxn.Lock(dir) + if err != nil { + return err + } + defer unlock() + lock, err := ReadLock(dir) if err != nil { return err @@ -194,11 +218,16 @@ func Remove(dir string, id string) error { return fmt.Errorf("plugin %q is not installed", id) } if present { - if err := os.RemoveAll(target); err != nil { + if err := installtxn.RemoveDir(target, func() error { + if !locked { + return nil + } + delete(lock, id) + return writeLock(dir, lock) + }); err != nil { return fmt.Errorf("remove plugin dir: %w", err) } - } - if locked { + } else if locked { delete(lock, id) if err := writeLock(dir, lock); err != nil { return err @@ -239,7 +268,7 @@ func writeLock(dir string, entries map[string]LockEntry) error { if err != nil { return fmt.Errorf("encode %s: %w", LockFileName, err) } - if err := os.WriteFile(filepath.Join(dir, LockFileName), append(data, '\n'), 0o644); err != nil { + if err := installtxn.WriteFileAtomically(filepath.Join(dir, LockFileName), append(data, '\n'), 0o644); err != nil { return fmt.Errorf("write %s: %w", LockFileName, err) } return nil diff --git a/internal/plugins/install_test.go b/internal/plugins/install_test.go index c0f717679..ce1b4b597 100644 --- a/internal/plugins/install_test.go +++ b/internal/plugins/install_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "os" "os/exec" "path/filepath" @@ -187,6 +188,38 @@ func TestInstallReinstallShowsHashChange(t *testing.T) { } } +func TestConcurrentInstallsPreserveEveryLockEntry(t *testing.T) { + destDir := t.TempDir() + const count = 12 + sources := make([]string, count) + for index := range count { + manifest := validManifest() + manifest["id"] = fmt.Sprintf("zero.concurrent.%02d", index) + sources[index] = writeSourcePlugin(t, filepath.Join(t.TempDir(), "src"), manifest) + } + + errs := make(chan error, count) + for _, source := range sources { + go func() { + _, err := Install(context.Background(), InstallOptions{Source: source, Dir: destDir}) + errs <- err + }() + } + for range count { + if err := <-errs; err != nil { + t.Fatalf("concurrent Install: %v", err) + } + } + + entries, err := ReadLock(destDir) + if err != nil { + t.Fatalf("ReadLock: %v", err) + } + if len(entries) != count { + t.Fatalf("lockfile has %d entries, want %d: %#v", len(entries), count, entries) + } +} + // TestInstallReinstallDetectsNestedFileChange guards that the recorded hash // covers the whole installed tree, not just plugin.json. A change to a tool // script (with the manifest unchanged) must still be reported as an update so diff --git a/internal/skills/install.go b/internal/skills/install.go index 1ac8c17f7..610fe8831 100644 --- a/internal/skills/install.go +++ b/internal/skills/install.go @@ -17,6 +17,8 @@ import ( "path/filepath" "sort" "strings" + + "github.com/Gitlawb/zero/internal/installtxn" ) // LockFileName is the name of the per-directory lockfile that maps an installed @@ -122,6 +124,34 @@ func Install(ctx context.Context, options InstallOptions) (InstallResult, error) hash := hashContent(data) + staged, cleanupStage, err := installtxn.StageDir(dir) + if err != nil { + return InstallResult{}, err + } + defer cleanupStage() + if err := os.MkdirAll(staged, 0o755); err != nil { + return InstallResult{}, fmt.Errorf("create staged skill dir: %w", err) + } + stagedManifest := filepath.Join(staged, skillFileName) + if err := os.WriteFile(stagedManifest, data, 0o644); err != nil { + return InstallResult{}, fmt.Errorf("stage SKILL.md: %w", err) + } + stagedData, err := os.ReadFile(stagedManifest) + if err != nil || hashContent(stagedData) != hash { + if err != nil { + return InstallResult{}, fmt.Errorf("validate staged SKILL.md: %w", err) + } + return InstallResult{}, errors.New("validate staged SKILL.md: copied content hash differs from source") + } + + unlock, err := installtxn.Lock(dir) + if err != nil { + return InstallResult{}, err + } + defer unlock() + + // Re-read under the cross-process lock. Another install may have updated the + // lockfile while this skill was fetched and staged. lock, err := ReadLock(dir) if err != nil { return InstallResult{}, err @@ -133,23 +163,10 @@ func Install(ctx context.Context, options InstallOptions) (InstallResult, error) } target := filepath.Join(dir, name) - if err := os.MkdirAll(dir, 0o755); err != nil { - return InstallResult{}, fmt.Errorf("create skills dir: %w", err) - } - // Replace any existing install atomically-enough: write the new SKILL.md after - // clearing a prior directory so a re-install never mixes old and new files. - if err := os.RemoveAll(target); err != nil { - return InstallResult{}, fmt.Errorf("clear previous skill: %w", err) - } - if err := os.MkdirAll(target, 0o755); err != nil { - return InstallResult{}, fmt.Errorf("create skill dir: %w", err) - } - if err := os.WriteFile(filepath.Join(target, skillFileName), data, 0o644); err != nil { - return InstallResult{}, fmt.Errorf("write SKILL.md: %w", err) - } - lock[name] = LockEntry{Source: source, Hash: hash} - if err := writeLock(dir, lock); err != nil { + if err := installtxn.CommitDir(target, staged, func() error { + return writeLock(dir, lock) + }); err != nil { return InstallResult{}, err } @@ -178,6 +195,12 @@ func Remove(dir string, name string) error { return fmt.Errorf("invalid skill name %q", name) } + unlock, err := installtxn.Lock(dir) + if err != nil { + return err + } + defer unlock() + lock, err := ReadLock(dir) if err != nil { return err @@ -191,11 +214,16 @@ func Remove(dir string, name string) error { } if present { - if err := os.RemoveAll(target); err != nil { + if err := installtxn.RemoveDir(target, func() error { + if !locked { + return nil + } + delete(lock, name) + return writeLock(dir, lock) + }); err != nil { return fmt.Errorf("remove skill dir: %w", err) } - } - if locked { + } else if locked { delete(lock, name) if err := writeLock(dir, lock); err != nil { return err @@ -301,7 +329,7 @@ func writeLock(dir string, entries map[string]LockEntry) error { if err != nil { return fmt.Errorf("encode %s: %w", LockFileName, err) } - if err := os.WriteFile(filepath.Join(dir, LockFileName), append(data, '\n'), 0o644); err != nil { + if err := installtxn.WriteFileAtomically(filepath.Join(dir, LockFileName), append(data, '\n'), 0o644); err != nil { return fmt.Errorf("write %s: %w", LockFileName, err) } return nil diff --git a/internal/skills/install_test.go b/internal/skills/install_test.go index cd0582994..df519fbc7 100644 --- a/internal/skills/install_test.go +++ b/internal/skills/install_test.go @@ -3,6 +3,7 @@ package skills import ( "context" "errors" + "fmt" "os" "os/exec" "path/filepath" @@ -192,6 +193,37 @@ func TestInstallReinstallShowsHashChange(t *testing.T) { } } +func TestConcurrentInstallsPreserveEveryLockEntry(t *testing.T) { + destDir := t.TempDir() + const count = 12 + sources := make([]string, count) + for index := range count { + content := fmt.Sprintf("---\nname: concurrent-%02d\ndescription: test\n---\nbody\n", index) + sources[index] = writeSourceSkill(t, filepath.Join(t.TempDir(), "src"), content) + } + + errs := make(chan error, count) + for _, source := range sources { + go func() { + _, err := Install(context.Background(), InstallOptions{Source: source, Dir: destDir}) + errs <- err + }() + } + for range count { + if err := <-errs; err != nil { + t.Fatalf("concurrent Install: %v", err) + } + } + + entries, err := ReadLock(destDir) + if err != nil { + t.Fatalf("ReadLock: %v", err) + } + if len(entries) != count { + t.Fatalf("lockfile has %d entries, want %d: %#v", len(entries), count, entries) + } +} + // TestInstallSameLocalSourceDifferentSpellingIsNotAClash verifies that a local // source installed via one spelling (e.g. a relative path) and re-installed via // an equivalent spelling (the absolute path) is treated as the same source, not