fix(plugins,skills): make reinstall transactions atomic

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

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

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

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

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