fix(installtxn): put back an install a killed commit left behind

internal/installtxn/installtxn.go

@@ -8,10 +8,20 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
 )
 
 const lockFileName = ".zero-install.lock"
 
+// workspacePrefix names the per-transaction workspaces created inside an install
+// root. Dot-prefixed so it is never mistaken for an installed plugin or skill.
+const workspacePrefix = ".zero-install-txn-"
+
+// targetFileName records, inside a workspace, which install the backup beside it
+// belongs to. Without it a workspace left by a killed process holds a tree
+// nothing can attribute, and so nothing can put back.
+const targetFileName = "target"
+
 // 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) {
@@ -28,7 +38,7 @@ 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-")
+	workspace, err := os.MkdirTemp(dir, workspacePrefix)
 	if err != nil {
 		return "", func() {}, fmt.Errorf("create install staging dir: %w", err)
 	}
@@ -45,6 +55,12 @@ func CommitDir(target string, staged string, publish func() error) error {
 	backup := filepath.Join(workspace, "previous")
 	hadPrevious := false
 	if _, err := os.Stat(target); err == nil {
+		// Record the target before moving its tree. The two renames below cannot
+		// be made atomic, so a process killed between them leaves the only copy
+		// in the backup, and without this nothing could tell which install it is.
+		if err := os.WriteFile(filepath.Join(workspace, targetFileName), []byte(filepath.Base(target)), 0o600); err != nil {
+			return fmt.Errorf("record install target: %w", err)
+		}
 		if err := os.Rename(target, backup); err != nil {
 			return fmt.Errorf("retain previous install: %w", err)
 		}
@@ -76,7 +92,7 @@ func CommitDir(target string, staged string, publish func() error) error {
 //
 // 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-")
+	workspace, err := os.MkdirTemp(filepath.Dir(target), workspacePrefix)
 	if err != nil {
 		return fmt.Errorf("create removal staging dir: %w", err)
 	}
@@ -95,6 +111,65 @@ func RemoveDir(target string, publish func() error) error {
 	return nil
 }
 
+// Recover puts back an install that CommitDir set aside but never replaced,
+// which is what a process killed between its two renames leaves: the target
+// absent and its only copy retained in a workspace nothing else reads. Anything
+// already at the target wins, and a workspace whose recorded target it has no
+// business naming is left alone rather than acted on. Best effort, since the
+// caller can still reinstall from source.
+//
+// The caller must hold the install-root lock returned by Lock, and EVERY caller
+// that takes that lock must call this first. Recovering only on the install
+// path is worse than not recovering at all: a removal would then report success
+// while the backup it never saw stayed on disk, and the next install would
+// publish it again, reinstating something the user deleted. Recovery is
+// deliberately an explicit call rather than a side effect of Lock, matching how
+// the other staged-swap transactions in this repo invoke their repair pass.
+func Recover(dir string) {
+	entries, err := os.ReadDir(dir)
+	if err != nil {
+		return
+	}
+	for _, entry := range entries {
+		if !entry.IsDir() || !strings.HasPrefix(entry.Name(), workspacePrefix) {
+			continue
+		}
+		workspace := filepath.Join(dir, entry.Name())
+		backup := filepath.Join(workspace, "previous")
+		if _, err := os.Stat(backup); err != nil {
+			continue
+		}
+		name, err := os.ReadFile(filepath.Join(workspace, targetFileName))
+		if err != nil {
+			continue
+		}
+		target, ok := recoverableTarget(dir, string(name))
+		if !ok {
+			continue
+		}
+		// An install already in place is the newer one by construction: the
+		// backup only ever holds the tree that was live before it.
+		if _, err := os.Lstat(target); err == nil {
+			continue
+		}
+		if err := os.Rename(backup, target); err != nil {
+			continue
+		}
+		cleanupWorkspace(workspace)
+	}
+}
+
+// recoverableTarget resolves a recorded target name to a path directly inside
+// dir. A name that is not a single path element could name anything on the
+// filesystem, so it is refused rather than restored over.
+func recoverableTarget(dir string, name string) (string, bool) {
+	name = strings.TrimSpace(name)
+	if name == "" || name == "." || name == ".." || name != filepath.Base(name) {
+		return "", false
+	}
+	return filepath.Join(dir, name), true
+}
+
 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))

internal/installtxn/installtxn_test.go

@@ -76,3 +76,201 @@ func TestCleanupWorkspacePreservesRetainedPreviousInstall(t *testing.T) {
 		t.Fatalf("cleanup removed retained previous install: %v", err)
 	}
 }
+
+// A retained backup is only recoverable if something can tell which install it
+// came from, so CommitDir records the target before it moves anything. publish
+// runs while the workspace is still in place, which is where that is visible.
+func TestCommitDirRecordsItsTargetForRecovery(t *testing.T) {
+	root := t.TempDir()
+	target := filepath.Join(root, "demo")
+	if err := os.MkdirAll(target, 0o755); 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)
+	}
+	workspace := filepath.Dir(staged)
+
+	var marker string
+	var markerErr error
+	if err := CommitDir(target, staged, func() error {
+		data, err := os.ReadFile(filepath.Join(workspace, targetFileName))
+		marker, markerErr = string(data), err
+		return nil
+	}); err != nil {
+		t.Fatalf("CommitDir: %v", err)
+	}
+
+	if markerErr != nil {
+		t.Fatalf("CommitDir left no way to attribute its backup: %v", markerErr)
+	}
+	if marker != "demo" {
+		t.Fatalf("recorded target = %q, want %q", marker, "demo")
+	}
+}
+
+// plantInterruptedCommit builds what a process killed between CommitDir's two
+// renames leaves in dir: a workspace naming its target, the live tree moved into
+// the backup beside it, and nothing at the target.
+func plantInterruptedCommit(t *testing.T, dir, name, recorded, content string) string {
+	t.Helper()
+	target := filepath.Join(dir, name)
+	if err := os.MkdirAll(target, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(target, "version"), []byte(content), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	staged, _, err := StageDir(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	workspace := filepath.Dir(staged)
+	if err := os.WriteFile(filepath.Join(workspace, targetFileName), []byte(recorded), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Rename(target, filepath.Join(workspace, "previous")); err != nil {
+		t.Fatal(err)
+	}
+	return workspace
+}
+
+func TestRecoverPutsBackAnInterruptedCommit(t *testing.T) {
+	dir := t.TempDir()
+	workspace := plantInterruptedCommit(t, dir, "demo", "demo", "old")
+
+	Recover(dir)
+
+	data, err := os.ReadFile(filepath.Join(dir, "demo", "version"))
+	if err != nil || string(data) != "old" {
+		t.Fatalf("the interrupted install was not put back: got %q err %v", data, err)
+	}
+	if _, err := os.Stat(workspace); !os.IsNotExist(err) {
+		t.Errorf("the recovered workspace should be cleared, got %v", err)
+	}
+}
+
+// A backup is a leftover, never a replacement for whatever is at the target now,
+// empty or not. Go's os.Rename refuses an existing directory either way on the
+// platforms tested, but POSIX allows replacing an empty one, so this pins the
+// behavior rather than one syscall's take on it.
+func TestRecoverLeavesALiveInstallAlone(t *testing.T) {
+	for _, tc := range []struct{ name, live string }{
+		{"empty install", ""},
+		{"populated install", "live"},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			dir := t.TempDir()
+			workspace := plantInterruptedCommit(t, dir, "demo", "demo", "old")
+			live := filepath.Join(dir, "demo")
+			if err := os.MkdirAll(live, 0o755); err != nil {
+				t.Fatal(err)
+			}
+			if tc.live != "" {
+				if err := os.WriteFile(filepath.Join(live, "version"), []byte(tc.live), 0o644); err != nil {
+					t.Fatal(err)
+				}
+			}
+
+			Recover(dir)
+
+			data, err := os.ReadFile(filepath.Join(live, "version"))
+			if tc.live == "" {
+				if err == nil {
+					t.Fatalf("an existing install was replaced by a backup: version = %q", data)
+				}
+			} else if err != nil || string(data) != tc.live {
+				t.Fatalf("the live install must win: got %q err %v", data, err)
+			}
+			if _, err := os.Stat(filepath.Join(workspace, "previous", "version")); err != nil {
+				t.Errorf("a backup it did not restore must be left intact: %v", err)
+			}
+		})
+	}
+}
+
+// The recorded target names a directory inside the install root and nothing
+// else. A name that could resolve anywhere is refused, not restored over.
+func TestRecoverRefusesATargetOutsideTheInstallRoot(t *testing.T) {
+	for _, recorded := range []string{"..", ".", "", "  ", "../escape", "a/b", string(filepath.Separator) + "etc"} {
+		t.Run(recorded, func(t *testing.T) {
+			root := t.TempDir()
+			dir := filepath.Join(root, "installs")
+			if err := os.MkdirAll(dir, 0o755); err != nil {
+				t.Fatal(err)
+			}
+			outside := filepath.Join(root, "escape")
+			workspace := plantInterruptedCommit(t, dir, "demo", recorded, "old")
+
+			Recover(dir)
+
+			if _, err := os.Stat(outside); !os.IsNotExist(err) {
+				t.Errorf("recovery wrote outside the install root: %v", err)
+			}
+			if _, err := os.Stat(filepath.Join(workspace, "previous", "version")); err != nil {
+				t.Errorf("an unattributable backup must be left intact: %v", err)
+			}
+		})
+	}
+}
+
+// A workspace mid-transaction has no backup yet, and one whose marker never got
+// written cannot be attributed. Neither is something to act on, and neither is
+// something to delete.
+func TestRecoverSkipsWorkspacesItCannotActOn(t *testing.T) {
+	dir := t.TempDir()
+	noBackup, _, err := StageDir(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := os.MkdirAll(noBackup, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	noMarker := plantInterruptedCommit(t, dir, "demo", "demo", "old")
+	if err := os.Remove(filepath.Join(noMarker, targetFileName)); err != nil {
+		t.Fatal(err)
+	}
+
+	Recover(dir)
+
+	if _, err := os.Stat(noBackup); err != nil {
+		t.Errorf("a workspace with no backup must be left alone: %v", err)
+	}
+	if _, err := os.Stat(filepath.Join(noMarker, "previous", "version")); err != nil {
+		t.Errorf("a backup with no marker must be left intact: %v", err)
+	}
+	if _, err := os.Stat(filepath.Join(dir, "demo")); !os.IsNotExist(err) {
+		t.Errorf("nothing should have been restored, got %v", err)
+	}
+}
+
+// Recovery identifies a workspace by the name its own StageDir gives one. An
+// installed tree that happens to contain the same two entries is not a
+// workspace, and consuming it would destroy installed content.
+func TestRecoverIgnoresAnInstallThatLooksLikeAWorkspace(t *testing.T) {
+	dir := t.TempDir()
+	lookalike := filepath.Join(dir, "demo")
+	if err := os.MkdirAll(filepath.Join(lookalike, "previous"), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(lookalike, "previous", "version"), []byte("mine"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(lookalike, targetFileName), []byte("elsewhere"), 0o600); err != nil {
+		t.Fatal(err)
+	}
+
+	Recover(dir)
+
+	if _, err := os.Stat(filepath.Join(lookalike, "previous", "version")); err != nil {
+		t.Fatalf("recovery consumed installed content: %v", err)
+	}
+	if _, err := os.Stat(filepath.Join(dir, "elsewhere")); !os.IsNotExist(err) {
+		t.Errorf("recovery published from a directory that is not its workspace: %v", err)
+	}
+}

internal/plugins/install.go

@@ -153,6 +153,8 @@ func Install(ctx context.Context, options InstallOptions) (InstallResult, error)
 		return InstallResult{}, err
 	}
 	defer unlock()
+	// Put back anything an earlier run was killed mid-commit; see installtxn.Recover.
+	installtxn.Recover(dir)
 
 	// Re-read under the cross-process lock. Another install may have updated the
 	// lockfile while this plugin was fetched and staged.
@@ -205,6 +207,8 @@ func Remove(dir string, id string) error {
 		return err
 	}
 	defer unlock()
+	// Put back anything an earlier run was killed mid-commit; see installtxn.Recover.
+	installtxn.Recover(dir)
 
 	lock, err := ReadLock(dir)
 	if err != nil {