diff --git a/internal/installtxn/installtxn.go b/internal/installtxn/installtxn.go index da4f2ad45..f6a351a50 100644 --- a/internal/installtxn/installtxn.go +++ b/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,15 +111,108 @@ func RemoveDir(target string, publish func() error) error { 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)) +// 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. The +// target is what records how far the commit got: absent means the swap never +// finished and the backup is put back, present means the publish rename +// committed and the superseded backup beside it is retired. The live target is +// never replaced or removed either way, and a workspace whose recorded target +// Recover 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 } - if hadPrevious { + 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. Leaving that + // backup for a later pass is what made a removal reversible by accident, + // since removing the live target then let the next recovery read the + // absent target as an interrupted swap and publish the stale tree again. + // Only the workspace goes; the live install is never touched. This is the + // one place os.RemoveAll is right over cleanupWorkspace, which refuses a + // workspace holding a previous precisely because it cannot tell a + // superseded backup from one still owed a restore. + if _, err := os.Lstat(target); err == nil { + _ = os.RemoveAll(workspace) + continue + } if err := os.Rename(backup, target); err != nil { - return errors.Join(cause, fmt.Errorf("restore previous install: %w", err)) + 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 !hadPrevious { + // A first install has no backup to protect and recorded no target, so + // recovery never looks here and deleting in place costs nothing. + if err := os.RemoveAll(target); err != nil { + return errors.Join(cause, fmt.Errorf("remove failed install: %w", err)) } + return cause + } + // Move the failed install aside before restoring rather than deleting it in + // place. A process killed partway through an in-place delete would leave a + // half removed tree at the target while the backup was still the only + // complete copy, and recovery reads a target that is there as a committed + // publish and retires the backup beside it. With the renames in this order + // every instant of the rollback has either a whole tree at the target or + // nothing there and the backup intact, which is exactly what recovery's two + // branches are able to tell apart. + failed := filepath.Join(filepath.Dir(backup), "failed") + if err := os.Rename(target, failed); err != nil { + // The move aside can fail too, and then the failed install stays live at + // the target while the backup is still the only copy of what it replaced. + // Recovery reads a tree at the target as a committed publish, so it would + // retire that backup. Dropping the marker leaves the workspace one nothing + // can attribute, which recovery already leaves alone, and the copy is still + // there to rescue by hand. + _ = os.Remove(filepath.Join(filepath.Dir(backup), targetFileName)) + return errors.Join(cause, fmt.Errorf("remove failed install: %w", err)) + } + if err := os.Rename(backup, target); err != nil { + return errors.Join(cause, fmt.Errorf("restore previous install: %w", err)) } + _ = os.RemoveAll(failed) return cause } diff --git a/internal/installtxn/installtxn_test.go b/internal/installtxn/installtxn_test.go index fdd27e4c9..3185abfb4 100644 --- a/internal/installtxn/installtxn_test.go +++ b/internal/installtxn/installtxn_test.go @@ -4,6 +4,7 @@ import ( "errors" "os" "path/filepath" + "runtime" "testing" ) @@ -76,3 +77,396 @@ 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. +// A tree at the target also says the publish rename committed, so the backup +// beside it holds what that install replaced and is retired rather than kept: +// keeping it let a later removal of this live tree hand the stale copy to the +// next recovery. +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(workspace); !os.IsNotExist(err) { + t.Errorf("the superseded workspace should be retired, got %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) + } +} + +// plantPublishedCommit builds what a process killed after CommitDir's second +// rename leaves in dir: the replacement live at the target, and the tree it +// replaced still sitting in the workspace beside it. +func plantPublishedCommit(t *testing.T, dir, name, recorded, published, superseded string) string { + t.Helper() + workspace := plantInterruptedCommit(t, dir, name, recorded, superseded) + 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(published), 0o644); err != nil { + t.Fatal(err) + } + return workspace +} + +// A backup left beside a target that the publish rename already replaced is +// superseded, not pending. Keeping it made a removal reversible by accident: +// the removal deleted the live target, and the next recovery then read the +// absent target as an interrupted swap and published the stale tree again. +func TestRecoverRetiresABackupASuccessfulPublishSuperseded(t *testing.T) { + dir := t.TempDir() + workspace := plantPublishedCommit(t, dir, "demo", "demo", "new", "old") + target := filepath.Join(dir, "demo") + + Recover(dir) + + data, err := os.ReadFile(filepath.Join(target, "version")) + if err != nil || string(data) != "new" { + t.Fatalf("the published install must be left alone: got %q err %v", data, err) + } + if _, err := os.Stat(workspace); !os.IsNotExist(err) { + t.Fatalf("the superseded workspace should be retired, got %v", err) + } + + if err := RemoveDir(target, func() error { return nil }); err != nil { + t.Fatalf("RemoveDir: %v", err) + } + Recover(dir) + + if _, err := os.Stat(target); !os.IsNotExist(err) { + t.Fatalf("a removed install was resurrected by recovery: %v", err) + } +} + +// A rollback must never leave a partial tree at the target, because recovery +// reads a present target as proof the publish committed and retires the backup +// beside it. Deleting the failed install in place opened exactly that window: +// a kill partway through the delete left a husk at the target while the backup +// was still the only complete copy. Moving the failed tree aside first closes +// it. The permission trick makes the in-place delete fail partway on demand. +func TestCommitDirRollbackNeverLeavesAPartialTargetTree(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("directory permissions do not block removal on windows") + } + if os.Geteuid() == 0 { + t.Skip("root ignores the directory permissions this test relies on") + } + 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() + locked := filepath.Join(staged, "locked") + if err := os.MkdirAll(locked, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(locked, "held"), []byte("new"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Chmod(locked, 0o555); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + _ = filepath.WalkDir(root, func(path string, entry os.DirEntry, err error) error { + if err == nil && entry.IsDir() { + _ = os.Chmod(path, 0o755) + } + return nil + }) + }) + + publishErr := errors.New("publish failed") + if err := CommitDir(target, staged, func() error { return publishErr }); !errors.Is(err, publishErr) { + t.Fatalf("CommitDir error = %v, want publish failure", err) + } + + data, err := os.ReadFile(filepath.Join(target, "version")) + if err != nil || string(data) != "old" { + t.Fatalf("the previous install must be back at the target: got %q err %v", data, err) + } + if _, err := os.Stat(filepath.Join(target, "locked")); !os.IsNotExist(err) { + t.Fatalf("part of the failed install was left at the target: %v", err) + } +} + +// The window between rollback's two renames leaves the target absent, the +// backup intact, and the failed install set aside beside it. That is the same +// pre-publish shape recovery already restores, and the set-aside tree must not +// change its reading of it. +func TestRecoverPutsBackAnInterruptedRollback(t *testing.T) { + dir := t.TempDir() + workspace := plantInterruptedCommit(t, dir, "demo", "demo", "old") + failed := filepath.Join(workspace, "failed") + if err := os.MkdirAll(failed, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(failed, "version"), []byte("new"), 0o644); err != nil { + t.Fatal(err) + } + + Recover(dir) + + data, err := os.ReadFile(filepath.Join(dir, "demo", "version")) + if err != nil || string(data) != "old" { + t.Fatalf("the interrupted rollback was not put back: got %q err %v", data, err) + } +} + +// The move aside can itself fail, and then the failed install is still live at +// the target with the backup still the only copy of what the user had. Recovery +// reads a target that is there as a committed publish, so a workspace left +// attributable here would have its backup retired: the one state where the new +// retire branch would destroy a tree that was never superseded. Dropping the +// marker hands it to the guard that already leaves unattributable workspaces +// alone. The parent permissions make the move aside fail on demand. +func TestRollbackKeepsABackupItCouldNotRestore(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("directory permissions do not block renames on windows") + } + if os.Geteuid() == 0 { + t.Skip("root ignores the directory permissions this test relies on") + } + 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) + } + workspace := filepath.Dir(staged) + t.Cleanup(func() { _ = os.Chmod(root, 0o755) }) + + publishErr := errors.New("publish failed") + // The install root goes read only after the publish rename, so rollback + // cannot move the failed install off the target. + err = CommitDir(target, staged, func() error { + if err := os.Chmod(root, 0o555); err != nil { + t.Fatal(err) + } + return publishErr + }) + if !errors.Is(err, publishErr) { + t.Fatalf("CommitDir error = %v, want publish failure", err) + } + if err := os.Chmod(root, 0o755); err != nil { + t.Fatal(err) + } + + Recover(root) + + data, err := os.ReadFile(filepath.Join(workspace, "previous", "version")) + if err != nil || string(data) != "old" { + t.Fatalf("a backup rollback could not restore must be kept: got %q err %v", data, err) + } + data, err = os.ReadFile(filepath.Join(target, "version")) + if err != nil || string(data) != "new" { + t.Fatalf("recovery must leave the tree at the target alone: got %q err %v", data, err) + } +} diff --git a/internal/plugins/install.go b/internal/plugins/install.go index 1335b67be..d27f77b40 100644 --- a/internal/plugins/install.go +++ b/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 { diff --git a/internal/plugins/install_test.go b/internal/plugins/install_test.go index ce1b4b597..b2e02776f 100644 --- a/internal/plugins/install_test.go +++ b/internal/plugins/install_test.go @@ -9,6 +9,8 @@ import ( "os/exec" "path/filepath" "testing" + + "github.com/Gitlawb/zero/internal/installtxn" ) // initGitPluginRepo creates a real local git repo holding a plugin and returns a @@ -403,3 +405,172 @@ func TestInstallCopiesEntireTree(t *testing.T) { t.Fatalf("entry script not copied into install dir: %v", err) } } + +// A process killed between installtxn.CommitDir's two renames leaves the +// plugin's only copy in a workspace backup with nothing at the target: the +// plugin disappears and no later run looks in the workspace. The next install +// over the same directory already takes the same cross-process lock, so it is +// where the interrupted one gets put back. +func TestInstallRecoversAPluginLeftByAnInterruptedCommit(t *testing.T) { + dir := t.TempDir() + src := writeSourcePlugin(t, filepath.Join(t.TempDir(), "src"), validManifest()) + if _, err := Install(context.Background(), InstallOptions{Source: src, Dir: dir}); err != nil { + t.Fatalf("seeding the install: %v", err) + } + + // The on-disk state a kill in that window leaves, built the way CommitDir + // builds it: a staged workspace naming its target, with the live tree moved + // into the backup beside it and the target gone. + staged, _, err := installtxn.StageDir(dir) + if err != nil { + t.Fatal(err) + } + workspace := filepath.Dir(staged) + if err := os.WriteFile(filepath.Join(workspace, "target"), []byte("zero.demo"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Rename(filepath.Join(dir, "zero.demo"), filepath.Join(workspace, "previous")); err != nil { + t.Fatal(err) + } + + other := validManifest() + other["id"] = "zero.other" + src2 := writeSourcePlugin(t, filepath.Join(t.TempDir(), "src2"), other) + if _, err := Install(context.Background(), InstallOptions{Source: src2, Dir: dir}); err != nil { + t.Fatalf("second install: %v", err) + } + + if _, err := os.Stat(filepath.Join(dir, "zero.demo", manifestFileName)); err != nil { + t.Fatalf("the interrupted install was not put back: %v", err) + } + loaded, err := Load(LoadOptions{Roots: []Root{{Source: SourceUser, Path: dir}}}) + if err != nil { + t.Fatal(err) + } + ids := []string{} + for _, p := range loaded.Plugins { + ids = append(ids, p.ID) + } + if len(ids) != 2 { + t.Errorf("Load sees %v, want both plugins", ids) + } + if _, err := os.Stat(workspace); !os.IsNotExist(err) { + t.Errorf("the recovered workspace should be cleared, got %v", err) + } +} + +// A removal has to stick. An install killed mid-commit leaves the tree in a +// workspace backup with nothing at the target, and Remove then takes the +// not-present branch: it drops the lockfile entry, reports success, and leaves +// the backup behind for the next install's recovery to publish again. Since +// Load enumerates directories rather than the lockfile, that republished tree +// is a live plugin the user already deleted. +func TestRemoveLeavesNothingARecoveryCanResurrect(t *testing.T) { + dir := t.TempDir() + src := writeSourcePlugin(t, filepath.Join(t.TempDir(), "src"), validManifest()) + if _, err := Install(context.Background(), InstallOptions{Source: src, Dir: dir}); err != nil { + t.Fatalf("seeding the install: %v", err) + } + staged, _, err := installtxn.StageDir(dir) + if err != nil { + t.Fatal(err) + } + workspace := filepath.Dir(staged) + if err := os.WriteFile(filepath.Join(workspace, "target"), []byte("zero.demo"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Rename(filepath.Join(dir, "zero.demo"), filepath.Join(workspace, "previous")); err != nil { + t.Fatal(err) + } + + if err := Remove(dir, "zero.demo"); err != nil { + t.Fatalf("Remove: %v", err) + } + + other := validManifest() + other["id"] = "zero.other" + src2 := writeSourcePlugin(t, filepath.Join(t.TempDir(), "src2"), other) + if _, err := Install(context.Background(), InstallOptions{Source: src2, Dir: dir}); err != nil { + t.Fatalf("later install: %v", err) + } + + if _, err := os.Stat(filepath.Join(dir, "zero.demo")); !os.IsNotExist(err) { + t.Errorf("a removed plugin was put back on disk: %v", err) + } + loaded, err := Load(LoadOptions{Roots: []Root{{Source: SourceUser, Path: dir}}}) + if err != nil { + t.Fatal(err) + } + for _, p := range loaded.Plugins { + if p.ID == "zero.demo" { + t.Errorf("a removed plugin is loadable again") + } + } +} + +// A commit killed after its publish rename but before it cleared the workspace +// leaves the tree the install replaced in a backup beside the live plugin. +// Removing that plugin deletes the live tree and its lockfile entry, so a +// backup that outlived it would be published again by the next install's +// recovery, and Load enumerates directories rather than the lockfile: the +// removed plugin would be live again with no entry naming it. +func TestRemoveLeavesNoSupersededBackupARecoveryCanResurrect(t *testing.T) { + dir := t.TempDir() + src := writeSourcePlugin(t, filepath.Join(t.TempDir(), "src"), validManifest()) + if _, err := Install(context.Background(), InstallOptions{Source: src, Dir: dir}); err != nil { + t.Fatalf("seeding the install: %v", err) + } + + // The state a kill in that window leaves: the plugin live at its target, + // with the tree it replaced still retained in the workspace beside it. + staged, _, err := installtxn.StageDir(dir) + if err != nil { + t.Fatal(err) + } + workspace := filepath.Dir(staged) + if err := os.WriteFile(filepath.Join(workspace, "target"), []byte("zero.demo"), 0o600); err != nil { + t.Fatal(err) + } + previous := filepath.Join(workspace, "previous") + if err := os.MkdirAll(previous, 0o755); err != nil { + t.Fatal(err) + } + manifest, err := os.ReadFile(filepath.Join(dir, "zero.demo", manifestFileName)) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(previous, manifestFileName), manifest, 0o644); err != nil { + t.Fatal(err) + } + + if err := Remove(dir, "zero.demo"); err != nil { + t.Fatalf("Remove: %v", err) + } + + other := validManifest() + other["id"] = "zero.other" + src2 := writeSourcePlugin(t, filepath.Join(t.TempDir(), "src2"), other) + if _, err := Install(context.Background(), InstallOptions{Source: src2, Dir: dir}); err != nil { + t.Fatalf("later install: %v", err) + } + + if _, err := os.Stat(filepath.Join(dir, "zero.demo")); !os.IsNotExist(err) { + t.Errorf("a removed plugin was put back on disk: %v", err) + } + loaded, err := Load(LoadOptions{Roots: []Root{{Source: SourceUser, Path: dir}}}) + if err != nil { + t.Fatal(err) + } + for _, p := range loaded.Plugins { + if p.ID == "zero.demo" { + t.Errorf("a removed plugin is loadable again") + } + } + lock, err := ReadLock(dir) + if err != nil { + t.Fatal(err) + } + if _, ok := lock["zero.demo"]; ok { + t.Errorf("the lockfile still names a removed plugin") + } +} diff --git a/internal/skills/install.go b/internal/skills/install.go index 3438857c1..9d92837ca 100644 --- a/internal/skills/install.go +++ b/internal/skills/install.go @@ -150,6 +150,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 skill was fetched and staged. @@ -201,6 +203,8 @@ func Remove(dir string, name 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 { diff --git a/internal/skills/install_test.go b/internal/skills/install_test.go index 10f99cb8f..2b2b85002 100644 --- a/internal/skills/install_test.go +++ b/internal/skills/install_test.go @@ -9,6 +9,8 @@ import ( "path/filepath" "strings" "testing" + + "github.com/Gitlawb/zero/internal/installtxn" ) // initGitSkillRepo creates a real local git repo holding a skill and returns a @@ -432,3 +434,100 @@ func TestSkillHashDriftUnreadableLockedPath(t *testing.T) { t.Fatal("missing lock hash must not count as drift") } } + +// skills.Install carries the same recovery call as plugins.Install, so it needs +// the same proof. An install killed mid-commit leaves the skill's only copy in +// a workspace backup; the next install over the same directory has to put it +// back rather than leave it stranded where nothing reads it. +func TestInstallRecoversASkillLeftByAnInterruptedCommit(t *testing.T) { + dir := t.TempDir() + src := writeSourceSkill(t, filepath.Join(t.TempDir(), "src"), + "---\nname: alpha\ndescription: first.\n---\nalpha body\n") + if _, err := Install(context.Background(), InstallOptions{Source: src, Dir: dir}); err != nil { + t.Fatalf("seeding the install: %v", err) + } + + // The state a kill between CommitDir's two renames leaves behind. + staged, _, err := installtxn.StageDir(dir) + if err != nil { + t.Fatal(err) + } + workspace := filepath.Dir(staged) + if err := os.WriteFile(filepath.Join(workspace, "target"), []byte("alpha"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Rename(filepath.Join(dir, "alpha"), filepath.Join(workspace, "previous")); err != nil { + t.Fatal(err) + } + + src2 := writeSourceSkill(t, filepath.Join(t.TempDir(), "src2"), + "---\nname: beta\ndescription: second.\n---\nbeta body\n") + if _, err := Install(context.Background(), InstallOptions{Source: src2, Dir: dir}); err != nil { + t.Fatalf("later install: %v", err) + } + + got, ok := Get(dir, "alpha") + if !ok || !strings.Contains(got.Content, "alpha body") { + t.Fatalf("the interrupted skill install was not put back: ok=%v skill=%+v", ok, got) + } + if _, err := os.Stat(workspace); !os.IsNotExist(err) { + t.Errorf("the recovered workspace should be cleared, got %v", err) + } +} + +// skills.Remove carries the same recovery call as plugins.Remove, so it needs +// the same proof. A commit killed after its publish rename leaves the tree the +// install replaced in a backup beside the live skill; removing the skill must +// not leave that backup for the next install's recovery to publish, since Get +// reads the directory rather than the lockfile and would find the removed skill +// loadable again. +func TestRemoveLeavesNoSupersededBackupARecoveryCanResurrect(t *testing.T) { + dir := t.TempDir() + src := writeSourceSkill(t, filepath.Join(t.TempDir(), "src"), + "---\nname: alpha\ndescription: first.\n---\nalpha body\n") + if _, err := Install(context.Background(), InstallOptions{Source: src, Dir: dir}); err != nil { + t.Fatalf("seeding the install: %v", err) + } + + // The state a kill after CommitDir's second rename leaves behind. + staged, _, err := installtxn.StageDir(dir) + if err != nil { + t.Fatal(err) + } + workspace := filepath.Dir(staged) + if err := os.WriteFile(filepath.Join(workspace, "target"), []byte("alpha"), 0o600); err != nil { + t.Fatal(err) + } + previous := filepath.Join(workspace, "previous") + if err := os.MkdirAll(previous, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(previous, skillFileName), + []byte("---\nname: alpha\ndescription: superseded.\n---\nold alpha body\n"), 0o644); err != nil { + t.Fatal(err) + } + + if err := Remove(dir, "alpha"); err != nil { + t.Fatalf("Remove: %v", err) + } + + src2 := writeSourceSkill(t, filepath.Join(t.TempDir(), "src2"), + "---\nname: beta\ndescription: second.\n---\nbeta body\n") + if _, err := Install(context.Background(), InstallOptions{Source: src2, Dir: dir}); err != nil { + t.Fatalf("later install: %v", err) + } + + if _, err := os.Stat(filepath.Join(dir, "alpha")); !os.IsNotExist(err) { + t.Errorf("a removed skill was put back on disk: %v", err) + } + if _, ok := Get(dir, "alpha"); ok { + t.Errorf("a removed skill is loadable again") + } + lock, err := ReadLock(dir) + if err != nil { + t.Fatal(err) + } + if _, ok := lock["alpha"]; ok { + t.Errorf("the lockfile still names a removed skill") + } +} diff --git a/internal/terminalpet/client.go b/internal/terminalpet/client.go index a01a8aec5..53846e4af 100644 --- a/internal/terminalpet/client.go +++ b/internal/terminalpet/client.go @@ -290,6 +290,8 @@ func (c *Client) Install(ctx context.Context, entry Entry) (*Animation, error) { return nil, err } defer unlock() + // Put back anything an earlier run was killed mid-commit; see installtxn.Recover. + installtxn.Recover(root) target := filepath.Join(root, entry.Slug) if err := installtxn.CommitDir(target, stage, func() error { return nil }); err != nil { return nil, fmt.Errorf("install pet: %w", err)