From 9cec6eb14f05f5ba6caac3a37b8ec3b777e5e20c Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sun, 30 Aug 2026 01:05:21 -0500 Subject: [PATCH 1/6] fix(installtxn): put back an install a killed commit left behind CommitDir publishes by two renames: the live target moves into a workspace backup, then the staged copy is renamed into place. Neither is journaled, so a process killed between them left the target absent with the install's only copy retained in a workspace nothing ever read. plugins.Load and skills.Load enumerate directories, so the extension simply disappeared, while the lockfile went on listing it. Recovering needed one fact the transaction never wrote down: which install a backup belonged to. CommitDir now records that before it moves anything, and Recover puts the backup back when the target is absent. Recovery is a second data-loss surface, so it is bounded on every side. It refuses a name that is not a single element inside the install root, never replaces a live target, identifies a workspace by the name StageDir gives one rather than by contents alone, and leaves intact anything it cannot attribute. Every path that takes the install lock recovers, not just the two that install. Recovering on the install path alone is worse than not recovering: a removal takes the not-present branch, drops the lockfile entry and reports success while the backup it never looked at stays on disk, and the next install republishes it, reinstating an extension the user deleted. Removal and the terminalpet install hold the same lock and now do the same thing first. Recovery stays an explicit call rather than a side effect of Lock, matching how the other staged-swap transactions here invoke their repair pass and keeping a filesystem mutation visible at the sites that cause it. --- internal/installtxn/installtxn.go | 79 +++++++++- internal/installtxn/installtxn_test.go | 198 +++++++++++++++++++++++++ internal/plugins/install.go | 4 + internal/plugins/install_test.go | 104 +++++++++++++ internal/skills/install.go | 4 + internal/skills/install_test.go | 42 ++++++ internal/terminalpet/client.go | 2 + 7 files changed, 431 insertions(+), 2 deletions(-) diff --git a/internal/installtxn/installtxn.go b/internal/installtxn/installtxn.go index da4f2ad45..357c73051 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,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)) diff --git a/internal/installtxn/installtxn_test.go b/internal/installtxn/installtxn_test.go index fdd27e4c9..3d54ad2d1 100644 --- a/internal/installtxn/installtxn_test.go +++ b/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) + } +} 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..1ac54599a 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,105 @@ 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") + } + } +} 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..992c6464a 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,43 @@ 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) + } +} 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) From 3fe120efce06ea982043bfd1a0d70cc8e0520acb Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Wed, 2 Sep 2026 12:00:09 -0500 Subject: [PATCH 2/6] fix(installtxn): retire a backup the publish rename already superseded Recover skipped any workspace whose target was occupied, which left the backup of a commit that was killed after its publish rename but before its cleanup. Removing that install then deleted the live target and its lockfile entry but not the skipped backup, and the next recovery read the absent target as an interrupted swap and published the stale tree again. plugins.Load and skills.Load enumerate directories, so the removed extension was loadable again with nothing in the lockfile naming it. The target already records how far the commit got, so recovery reads it as the phase rather than carrying a phase file. Absent means the swap never finished and the backup is put back as before. Present means the publish rename committed, so the backup beside it is superseded and its workspace is retired. The live install is never replaced or removed either way, and the guards that skip a workspace with no backup, an unreadable or missing marker, or a recorded name that is not a single element inside the install root are unchanged. The retire path removes the workspace directly rather than through cleanupWorkspace, which refuses one holding a previous precisely because it cannot tell a superseded backup from one still owed a restore. Reading the target that way is only safe once nothing can leave a partial tree there. rollback deleted the failed install in place before restoring, so a process killed partway through that delete left a husk at the target while the backup was still the only complete copy, and recovery would have taken the husk for a committed publish and deleted the last good tree. The renames are now ordered so the target is never partial: the failed install moves aside into the workspace, the backup moves back to the target, and only then is the set aside tree removed. A first install has no backup to protect and recorded no target, so it still deletes in place. The move aside can fail too, and then the failed install stays live at the target with the backup still the only copy of what it replaced. Rollback drops the workspace marker on that path, which leaves a workspace nothing can attribute, and recovery already leaves those alone. --- internal/installtxn/installtxn.go | 54 +++++-- internal/installtxn/installtxn_test.go | 200 ++++++++++++++++++++++++- internal/plugins/install_test.go | 67 +++++++++ internal/skills/install_test.go | 57 +++++++ 4 files changed, 366 insertions(+), 12 deletions(-) diff --git a/internal/installtxn/installtxn.go b/internal/installtxn/installtxn.go index 357c73051..f6a351a50 100644 --- a/internal/installtxn/installtxn.go +++ b/internal/installtxn/installtxn.go @@ -113,10 +113,13 @@ func RemoveDir(target string, publish func() error) error { // 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. +// 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 @@ -148,8 +151,16 @@ func Recover(dir string) { continue } // An install already in place is the newer one by construction: the - // backup only ever holds the tree that was live before it. + // 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 { @@ -171,14 +182,37 @@ func recoverableTarget(dir string, name string) (string, bool) { } func rollback(target string, backup string, hadPrevious bool, cause error) error { - if err := os.RemoveAll(target); err != nil { + 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 hadPrevious { - if err := os.Rename(backup, target); err != nil { - return errors.Join(cause, fmt.Errorf("restore previous 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 3d54ad2d1..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" ) @@ -159,6 +160,10 @@ func TestRecoverPutsBackAnInterruptedCommit(t *testing.T) { // 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", ""}, @@ -187,8 +192,8 @@ func TestRecoverLeavesALiveInstallAlone(t *testing.T) { } 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) + if _, err := os.Stat(workspace); !os.IsNotExist(err) { + t.Errorf("the superseded workspace should be retired, got %v", err) } }) } @@ -274,3 +279,194 @@ func TestRecoverIgnoresAnInstallThatLooksLikeAWorkspace(t *testing.T) { 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_test.go b/internal/plugins/install_test.go index 1ac54599a..b2e02776f 100644 --- a/internal/plugins/install_test.go +++ b/internal/plugins/install_test.go @@ -507,3 +507,70 @@ func TestRemoveLeavesNothingARecoveryCanResurrect(t *testing.T) { } } } + +// 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_test.go b/internal/skills/install_test.go index 992c6464a..2b2b85002 100644 --- a/internal/skills/install_test.go +++ b/internal/skills/install_test.go @@ -474,3 +474,60 @@ func TestInstallRecoversASkillLeftByAnInterruptedCommit(t *testing.T) { 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") + } +} From 66232eba892821c6d5b5cc7715f3005afe9e198e Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Thu, 10 Sep 2026 10:38:33 -0500 Subject: [PATCH 3/6] fix(installtxn): reconcile both halves of an interrupted commit Recovery inferred three facts from incidental filesystem shape: target presence stood in for the transaction phase, a public dot prefix plus two ordinary filenames stood in for ownership, and a void best effort call stood in for both "nothing to recover" and "recovery failed". CommitDir swaps the trees and only then publishes the lockfile, so a kill between the two leaves exactly what a kill after both leaves. A phase bit written after the publish only inverts which side of the write the ambiguity falls on, so Recover now asks the caller which of the two trees its published metadata records. plugins and skills answer from the recorded hash; the terminalpet install publishes no metadata and passes nil, where a live target is the committed one. A backup the metadata still records goes back over the target through the same move aside ordering rollback uses. Recover returns an error, and every caller that takes the install lock aborts on it before reading the lockfile, inspecting the target, installing over it, or reporting a successful removal. Malformed, legacy, and unattributable workspaces are still skipped without error; a workspace we own and could not resolve is reported, and every workspace is processed before returning. Ownership is now proven rather than inferred. The workspace marker carries a magic and version first line, so a user authored skill directory that happens to be named with the workspace prefix and to hold a previous directory is left alone. A recorded name carrying the workspace prefix is refused too, since it names another transaction rather than an install. Retirement clears the backup before the workspace around it: os.RemoveAll unlinks the marker first, so a failure inside the backup used to cost the workspace the attribution that gets it reported at all. --- internal/installtxn/installtxn.go | 247 +++++++++-- internal/installtxn/installtxn_test.go | 543 +++++++++++++++++++++-- internal/plugins/install.go | 77 +++- internal/plugins/install_test.go | 573 ++++++++++++++++++++++++- internal/skills/install.go | 69 ++- internal/skills/install_test.go | 472 +++++++++++++++++++- internal/terminalpet/client.go | 11 +- 7 files changed, 1894 insertions(+), 98 deletions(-) diff --git a/internal/installtxn/installtxn.go b/internal/installtxn/installtxn.go index f6a351a50..a5cbae824 100644 --- a/internal/installtxn/installtxn.go +++ b/internal/installtxn/installtxn.go @@ -17,10 +17,17 @@ const lockFileName = ".zero-install.lock" // 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 +// markerFileName 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" +const markerFileName = ".zero-install-txn" + +// markerMagic is the first line of that marker. Ownership has to be proven +// rather than inferred from filesystem shape: a user authored skill directory +// may legitimately be named with the workspace prefix and hold a previous +// directory, and recovery acting on one would destroy installed content. No +// ordinary content carries this line by accident. +const markerMagic = "zero-install-txn v1" // Lock takes the per-install-root cross-process lock. It blocks until any other // installer or remover using dir has completed. @@ -58,7 +65,7 @@ func CommitDir(target string, staged string, publish func() error) error { // 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 { + if err := os.WriteFile(filepath.Join(workspace, markerFileName), []byte(markerMagic+"\ntarget "+filepath.Base(target)+"\n"), 0o600); err != nil { return fmt.Errorf("record install target: %w", err) } if err := os.Rename(target, backup); err != nil { @@ -111,73 +118,222 @@ 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. 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. +// Phase names which tree in an interrupted workspace the caller's published +// metadata records. +type Phase int + +const ( + // PhaseUnknown means the metadata matches neither tree. Recovery refuses to + // act and reports the workspace as unresolved. + PhaseUnknown Phase = iota + // PhaseCommitted means the live target is what the metadata records: the + // publish committed, and the backup beside it is superseded. + PhaseCommitted + // PhasePrePublish means the metadata still records the retained backup: the + // tree swap landed but the publish never did. + PhasePrePublish +) + +// Reconciler classifies one interrupted workspace for the install named name. +// target is the live install path, backup the retained tree beside it. An error +// means the classification could not be made and recovery must not act. +type Reconciler func(name string, target string, backup string) (Phase, error) + +// Recover resolves the workspaces an earlier run was killed inside, which is +// what a process killed mid-commit leaves: a tree retained in a workspace +// nothing else reads. How far that commit got cannot be read off the +// filesystem. CommitDir swaps the trees and only then publishes, so a kill +// between the two leaves exactly what a kill after both leaves, and a phase bit +// written after the publish only inverts which side of the write the ambiguity +// falls on. Only the caller knows what its published metadata records, so +// reconcile is asked which of the two trees that is, and its answer decides: +// the live target stands and the backup beside it is retired, or the backup is +// the truthful tree and goes back over the target. A nil reconciler means the +// caller publishes no metadata beside the tree, so a live target is the +// committed one. With nothing at the target there is no choice to make and the +// backup, the only copy in existence, is put back. +// +// The returned error means an attributable transaction was left unresolved, +// never that there was nothing to do. Malformed, legacy, and unattributable +// workspaces are skipped without error, since they may be somebody's content or +// somebody else's transaction, but a workspace we own and could not resolve is +// reported. Every workspace is processed before returning, so one unresolved +// transaction does not strand the rest. // // 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) { +// that takes that lock must call this first and abort on its error before it +// reads the lockfile, inspects the target, installs over it, or reports a +// successful removal. 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, reconcile Reconciler) error { entries, err := os.ReadDir(dir) if err != nil { - return + // A recovery set that was never enumerated is not an empty one, and the + // caller must not go on to install over or report the removal of a tree + // that may still be owed a restore. + return fmt.Errorf("enumerate install workspaces: %w", err) } + var unresolved []error 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 { + name, ok := markerTarget(workspace) + if !ok { continue } - target, ok := recoverableTarget(dir, string(name)) + target, ok := recoverableTarget(dir, 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) + backup := filepath.Join(workspace, "previous") + if _, err := os.Stat(backup); err != nil { + if errors.Is(err, os.ErrNotExist) { + // Mid-transaction: the trees never moved, so there is nothing to + // put back and the workspace belongs to whoever made it. + continue + } + unresolved = append(unresolved, fmt.Errorf("inspect retained install %s: %w", name, err)) continue } + if err := recoverWorkspace(workspace, target, backup, name, reconcile); err != nil { + unresolved = append(unresolved, err) + } + } + return errors.Join(unresolved...) +} + +// recoverWorkspace resolves one attributable workspace whose backup is present. +func recoverWorkspace(workspace string, target string, backup string, name string, reconcile Reconciler) error { + if _, err := os.Lstat(target); err != nil { + // Nothing at the target, so the swap never finished and the backup is the + // only copy there is. There is no second tree to weigh it against, and + // asking could only produce an answer that throws it away. if err := os.Rename(backup, target); err != nil { - continue + return fmt.Errorf("restore interrupted install %s: %w", name, err) } cleanupWorkspace(workspace) + return nil + } + phase := PhaseCommitted + if reconcile != nil { + classified, err := reconcile(name, target, backup) + if err != nil { + // A caller that could not classify is not permission to fall back on + // filesystem shape, which is the inference this protocol exists to + // remove. + return fmt.Errorf("classify interrupted install %s: %w", name, err) + } + phase = classified + } + switch phase { + case PhaseCommitted: + // The backup holds what the committed publish replaced. 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. Only the workspace goes, never the live + // install. 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. + // + // The backup goes before the workspace around it, because os.RemoveAll + // walks the workspace in name order and so unlinks the marker first. A + // removal that then failed inside the backup left a workspace holding a + // tree nothing could attribute, which the next pass reads as somebody + // else's content and stays silent about. Clearing the backup first means + // whatever survives a failure is still ours and is still reported. + if err := os.RemoveAll(backup); err != nil { + return fmt.Errorf("retire superseded install %s: %w", name, err) + } + if err := os.RemoveAll(workspace); err != nil { + return fmt.Errorf("retire superseded install %s: %w", name, err) + } + return nil + case PhasePrePublish: + return restoreOverTarget(workspace, target, backup, name) + default: + // Neither tree is the one the metadata records, so recovery cannot tell + // which one the user is owed and either guess risks destroying the other. + return fmt.Errorf("interrupted install %s matches neither the live target nor the retained backup", name) } } +// restoreOverTarget puts the backup back over a target that is still there, +// which is what the metadata recording the backup means: the tree swap landed +// but the publish never did, and recovery cannot publish forward because it does +// not know the source the interrupted install was writing. +func restoreOverTarget(workspace string, target string, backup string, name string) error { + // Same ordering rollback uses. Removing the superseded tree in place would + // leave a husk at the target while the backup was still the only complete + // copy, and a kill in that window is unrecoverable. With the renames in this + // order every instant has either a whole tree at the target or nothing there + // and the backup intact, which is exactly what this pass can tell apart. + failed := filepath.Join(workspace, "failed") + if err := os.Rename(target, failed); err != nil { + return fmt.Errorf("set aside superseded install %s: %w", name, err) + } + if err := os.Rename(backup, target); err != nil { + return fmt.Errorf("restore interrupted install %s: %w", name, err) + } + if err := os.RemoveAll(failed); err != nil { + return fmt.Errorf("remove superseded install %s: %w", name, err) + } + cleanupWorkspace(workspace) + return nil +} + +// markerTarget reads the install name a workspace records, and reports whether +// the workspace is one of ours at all. The prefix alone does not prove that: it +// is a public dot prefixed name and the skill loader enumerates dot prefixed +// directories, so a user authored skill can carry it and hold the same entries a +// workspace does. Only the magic and version first line is evidence no ordinary +// content produces by accident, so anything else is somebody's content and is +// left alone. +func markerTarget(workspace string) (string, bool) { + path := filepath.Join(workspace, markerFileName) + info, err := os.Lstat(path) + if err != nil || !info.Mode().IsRegular() { + return "", false + } + data, err := os.ReadFile(path) + if err != nil { + return "", false + } + magic, rest, ok := strings.Cut(string(data), "\n") + if !ok || magic != markerMagic { + return "", false + } + line, _, ok := strings.Cut(rest, "\n") + if !ok { + return "", false + } + name, ok := strings.CutPrefix(line, "target ") + if !ok { + return "", false + } + return name, true +} + // 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. +// filesystem, so it is refused rather than restored over. A name carrying the +// workspace prefix is refused for the same reason: it names another +// transaction, not an install, and restoring over one that is still in flight +// would destroy it. func recoverableTarget(dir string, name string) (string, bool) { name = strings.TrimSpace(name) if name == "" || name == "." || name == ".." || name != filepath.Base(name) { return "", false } + if strings.HasPrefix(name, workspacePrefix) { + return "", false + } return filepath.Join(dir, name), true } @@ -202,11 +358,12 @@ func rollback(target string, backup string, hadPrevious bool, cause error) error 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)) + // A caller with no reconciler reads a tree at the target as a committed + // publish, so it would retire that backup, and one that has a reconciler + // cannot classify a tree its own publish never recorded. Dropping the + // marker leaves a workspace 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), markerFileName)) return errors.Join(cause, fmt.Errorf("remove failed install: %w", err)) } if err := os.Rename(backup, target); err != nil { diff --git a/internal/installtxn/installtxn_test.go b/internal/installtxn/installtxn_test.go index 3185abfb4..1a3b30ebf 100644 --- a/internal/installtxn/installtxn_test.go +++ b/internal/installtxn/installtxn_test.go @@ -5,6 +5,7 @@ import ( "os" "path/filepath" "runtime" + "strings" "testing" ) @@ -100,7 +101,7 @@ func TestCommitDirRecordsItsTargetForRecovery(t *testing.T) { var marker string var markerErr error if err := CommitDir(target, staged, func() error { - data, err := os.ReadFile(filepath.Join(workspace, targetFileName)) + data, err := os.ReadFile(filepath.Join(workspace, markerFileName)) marker, markerErr = string(data), err return nil }); err != nil { @@ -110,8 +111,18 @@ func TestCommitDirRecordsItsTargetForRecovery(t *testing.T) { 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") + if want := "zero-install-txn v1\ntarget demo\n"; marker != want { + t.Fatalf("recorded marker = %q, want %q", marker, want) + } +} + +// writeMarker plants a workspace ownership marker verbatim, so the tests pin the +// exact bytes recovery has to see rather than whatever the writer happens to +// produce. +func writeMarker(t *testing.T, workspace string, content string) { + t.Helper() + if err := os.WriteFile(filepath.Join(workspace, markerFileName), []byte(content), 0o600); err != nil { + t.Fatal(err) } } @@ -132,9 +143,7 @@ func plantInterruptedCommit(t *testing.T, dir, name, recorded, content string) s t.Fatal(err) } workspace := filepath.Dir(staged) - if err := os.WriteFile(filepath.Join(workspace, targetFileName), []byte(recorded), 0o600); err != nil { - t.Fatal(err) - } + writeMarker(t, workspace, "zero-install-txn v1\ntarget "+recorded+"\n") if err := os.Rename(target, filepath.Join(workspace, "previous")); err != nil { t.Fatal(err) } @@ -145,7 +154,9 @@ func TestRecoverPutsBackAnInterruptedCommit(t *testing.T) { dir := t.TempDir() workspace := plantInterruptedCommit(t, dir, "demo", "demo", "old") - Recover(dir) + if err := Recover(dir, nil); err != nil { + t.Fatalf("Recover: %v", err) + } data, err := os.ReadFile(filepath.Join(dir, "demo", "version")) if err != nil || string(data) != "old" { @@ -156,15 +167,17 @@ func TestRecoverPutsBackAnInterruptedCommit(t *testing.T) { } } -// 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) { +// A caller with no metadata beside its tree passes no reconciler, and then a +// tree at the target is the committed one: the backup beside it holds what that +// install replaced and is retired rather than kept, empty target or not. +// Keeping it let a later removal of the live tree hand the stale copy to the +// next recovery. Go's os.Rename refuses an existing directory on the platforms +// tested, but POSIX allows replacing an empty one, so the empty case pins the +// decision rather than one syscall's take on it. What recovery never does is +// destroy a tree without a complete copy in hand, or touch a target the +// metadata records: replacing a live target happens only on the reconciled +// PhasePrePublish path, which restores through a move aside. +func TestRecoverTreatsALiveInstallAsCommittedWithoutAReconciler(t *testing.T) { for _, tc := range []struct{ name, live string }{ {"empty install", ""}, {"populated install", "live"}, @@ -182,7 +195,9 @@ func TestRecoverLeavesALiveInstallAlone(t *testing.T) { } } - Recover(dir) + if err := Recover(dir, nil); err != nil { + t.Fatalf("Recover: %v", err) + } data, err := os.ReadFile(filepath.Join(live, "version")) if tc.live == "" { @@ -212,7 +227,9 @@ func TestRecoverRefusesATargetOutsideTheInstallRoot(t *testing.T) { outside := filepath.Join(root, "escape") workspace := plantInterruptedCommit(t, dir, "demo", recorded, "old") - Recover(dir) + if err := Recover(dir, nil); err != nil { + t.Fatalf("Recover: %v", err) + } if _, err := os.Stat(outside); !os.IsNotExist(err) { t.Errorf("recovery wrote outside the install root: %v", err) @@ -237,11 +254,13 @@ func TestRecoverSkipsWorkspacesItCannotActOn(t *testing.T) { t.Fatal(err) } noMarker := plantInterruptedCommit(t, dir, "demo", "demo", "old") - if err := os.Remove(filepath.Join(noMarker, targetFileName)); err != nil { + if err := os.Remove(filepath.Join(noMarker, markerFileName)); err != nil { t.Fatal(err) } - Recover(dir) + if err := Recover(dir, nil); err != nil { + t.Fatalf("Recover: %v", err) + } if _, err := os.Stat(noBackup); err != nil { t.Errorf("a workspace with no backup must be left alone: %v", err) @@ -254,23 +273,27 @@ func TestRecoverSkipsWorkspacesItCannotActOn(t *testing.T) { } } -// 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. +// The workspace prefix is a public dot prefixed name, and the skill loader +// enumerates dot prefixed directories, so a user authored skill can legitimately +// be named with it and hold the same two entries a workspace does. Ownership is +// proven by the marker's magic line, which ordinary content never carries, so +// this directory is left untouched and unreported. func TestRecoverIgnoresAnInstallThatLooksLikeAWorkspace(t *testing.T) { dir := t.TempDir() - lookalike := filepath.Join(dir, "demo") + lookalike := filepath.Join(dir, workspacePrefix+"notes") 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 { + if err := os.WriteFile(filepath.Join(lookalike, "target"), []byte("elsewhere"), 0o600); err != nil { t.Fatal(err) } - Recover(dir) + if err := Recover(dir, nil); err != nil { + t.Fatalf("Recover: %v", err) + } if _, err := os.Stat(filepath.Join(lookalike, "previous", "version")); err != nil { t.Fatalf("recovery consumed installed content: %v", err) @@ -305,7 +328,9 @@ func TestRecoverRetiresABackupASuccessfulPublishSuperseded(t *testing.T) { workspace := plantPublishedCommit(t, dir, "demo", "demo", "new", "old") target := filepath.Join(dir, "demo") - Recover(dir) + if err := Recover(dir, nil); err != nil { + t.Fatalf("Recover: %v", err) + } data, err := os.ReadFile(filepath.Join(target, "version")) if err != nil || string(data) != "new" { @@ -318,7 +343,9 @@ func TestRecoverRetiresABackupASuccessfulPublishSuperseded(t *testing.T) { if err := RemoveDir(target, func() error { return nil }); err != nil { t.Fatalf("RemoveDir: %v", err) } - Recover(dir) + if err := Recover(dir, nil); err != nil { + t.Fatalf("Recover: %v", err) + } if _, err := os.Stat(target); !os.IsNotExist(err) { t.Fatalf("a removed install was resurrected by recovery: %v", err) @@ -399,7 +426,9 @@ func TestRecoverPutsBackAnInterruptedRollback(t *testing.T) { t.Fatal(err) } - Recover(dir) + if err := Recover(dir, nil); err != nil { + t.Fatalf("Recover: %v", err) + } data, err := os.ReadFile(filepath.Join(dir, "demo", "version")) if err != nil || string(data) != "old" { @@ -459,7 +488,9 @@ func TestRollbackKeepsABackupItCouldNotRestore(t *testing.T) { t.Fatal(err) } - Recover(root) + if err := Recover(root, nil); err != nil { + t.Fatalf("Recover: %v", err) + } data, err := os.ReadFile(filepath.Join(workspace, "previous", "version")) if err != nil || string(data) != "old" { @@ -470,3 +501,455 @@ func TestRollbackKeepsABackupItCouldNotRestore(t *testing.T) { t.Fatalf("recovery must leave the tree at the target alone: got %q err %v", data, err) } } + +// Everything short of the exact magic and version line, and a marker that is not +// a regular file, leaves the workspace alone and unreported: a workspace we +// cannot prove is ours may be somebody's content. +func TestRecoverSkipsAWorkspaceWithoutAValidMarker(t *testing.T) { + for _, tc := range []struct{ name, marker string }{ + {"wrong magic", "install-txn v1\ntarget demo\n"}, + {"wrong version", "zero-install-txn v2\ntarget demo\n"}, + {"magic not first", "target demo\nzero-install-txn v1\n"}, + {"no target line", "zero-install-txn v1\n"}, + {"legacy plain name", "demo"}, + {"multi element name", "zero-install-txn v1\ntarget nested/demo\n"}, + {"empty", ""}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + workspace := plantInterruptedCommit(t, dir, "demo", "demo", "old") + writeMarker(t, workspace, tc.marker) + + if err := Recover(dir, nil); err != nil { + t.Fatalf("Recover: %v", err) + } + + if _, err := os.Stat(filepath.Join(workspace, "previous", "version")); err != nil { + t.Errorf("an unattributable backup 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) + } + }) + } +} + +// A directory in the marker's place is not a marker. os.ReadFile fails on one +// anyway, but the check is on the file mode so that stays true of any future +// reader. +func TestRecoverSkipsAWorkspaceWhoseMarkerIsNotARegularFile(t *testing.T) { + dir := t.TempDir() + workspace := plantInterruptedCommit(t, dir, "demo", "demo", "old") + marker := filepath.Join(workspace, markerFileName) + if err := os.Remove(marker); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(marker, 0o755); err != nil { + t.Fatal(err) + } + + if err := Recover(dir, nil); err != nil { + t.Fatalf("Recover: %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 recorded name is a target, never another transaction. A marker naming a +// second in-flight workspace would have recovery rename a backup over it or +// publish into it, destroying a transaction that is still running. +func TestRecoverRefusesATargetThatNamesAnotherWorkspace(t *testing.T) { + dir := t.TempDir() + inflight, _, err := StageDir(dir) + if err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(inflight, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(inflight, "version"), []byte("staging"), 0o644); err != nil { + t.Fatal(err) + } + other := filepath.Dir(inflight) + workspace := plantInterruptedCommit(t, dir, "demo", filepath.Base(other), "old") + + if err := Recover(dir, nil); err != nil { + t.Fatalf("Recover: %v", err) + } + + data, err := os.ReadFile(filepath.Join(inflight, "version")) + if err != nil || string(data) != "staging" { + t.Fatalf("recovery wrote over an in-flight transaction: got %q err %v", data, err) + } + if _, err := os.Stat(filepath.Join(workspace, "previous", "version")); err != nil { + t.Errorf("an unattributable backup must be left intact: %v", err) + } +} + +// recordingReconciler answers with a fixed phase and remembers what it was +// asked, so the tests can pin both the decision and the question. +type recordingReconciler struct { + phase Phase + err error + calls int + name string + target string + backup string +} + +func (r *recordingReconciler) reconcile(name string, target string, backup string) (Phase, error) { + r.calls++ + r.name, r.target, r.backup = name, target, backup + return r.phase, r.err +} + +// Which tree the publish recorded cannot be read off the filesystem: a kill +// between the tree swap and the lockfile write leaves exactly what a kill after +// both leaves. Only the caller knows what its metadata says, so recovery asks, +// and a metadata record still naming the backup means the backup is the +// truthful tree and goes back over the live one. +func TestRecoverRestoresTheBackupTheMetadataStillRecords(t *testing.T) { + dir := t.TempDir() + workspace := plantPublishedCommit(t, dir, "demo", "demo", "new", "old") + target := filepath.Join(dir, "demo") + reconciler := &recordingReconciler{phase: PhasePrePublish} + + if err := Recover(dir, reconciler.reconcile); err != nil { + t.Fatalf("Recover: %v", err) + } + + data, err := os.ReadFile(filepath.Join(target, "version")) + if err != nil || string(data) != "old" { + t.Fatalf("the recorded tree must be back at the target: got %q err %v", data, err) + } + if reconciler.calls != 1 { + t.Fatalf("reconciler calls = %d, want 1", reconciler.calls) + } + if reconciler.name != "demo" || reconciler.target != target || reconciler.backup != filepath.Join(workspace, "previous") { + t.Fatalf("reconciler asked about (%q, %q, %q)", reconciler.name, reconciler.target, reconciler.backup) + } + if _, err := os.Stat(workspace); !os.IsNotExist(err) { + t.Errorf("the resolved workspace should be cleared, got %v", err) + } +} + +// The mirror case: the metadata records the live tree, so the publish committed +// and the backup beside it is superseded rather than owed a restore. +func TestRecoverRetiresTheBackupWhenTheMetadataRecordsTheTarget(t *testing.T) { + dir := t.TempDir() + workspace := plantPublishedCommit(t, dir, "demo", "demo", "new", "old") + reconciler := &recordingReconciler{phase: PhaseCommitted} + + if err := Recover(dir, reconciler.reconcile); err != nil { + t.Fatalf("Recover: %v", err) + } + + data, err := os.ReadFile(filepath.Join(dir, "demo", "version")) + if err != nil || string(data) != "new" { + t.Fatalf("the committed install must be left alone: 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) + } +} + +// Metadata matching neither tree means recovery has no idea which one the user +// is owed. Guessing either way risks destroying the other, so it moves nothing +// and reports the workspace as unresolved. +func TestRecoverReportsAWorkspaceItCannotClassify(t *testing.T) { + dir := t.TempDir() + workspace := plantPublishedCommit(t, dir, "demo", "demo", "new", "old") + reconciler := &recordingReconciler{phase: PhaseUnknown} + + err := Recover(dir, reconciler.reconcile) + + if err == nil { + t.Fatal("an unclassifiable workspace must be reported, not skipped silently") + } + data, readErr := os.ReadFile(filepath.Join(dir, "demo", "version")) + if readErr != nil || string(data) != "new" { + t.Fatalf("nothing should have moved: got %q err %v", data, readErr) + } + data, readErr = os.ReadFile(filepath.Join(workspace, "previous", "version")) + if readErr != nil || string(data) != "old" { + t.Fatalf("the backup must be left intact: got %q err %v", data, readErr) + } +} + +// A reconciler that cannot answer (its lockfile is unreadable, a hash fails) is +// not permission to fall back on filesystem shape. +func TestRecoverReportsAReconcilerFailure(t *testing.T) { + dir := t.TempDir() + workspace := plantPublishedCommit(t, dir, "demo", "demo", "new", "old") + classifyErr := errors.New("lockfile unreadable") + reconciler := &recordingReconciler{err: classifyErr} + + err := Recover(dir, reconciler.reconcile) + + if !errors.Is(err, classifyErr) { + t.Fatalf("Recover error = %v, want the reconciler failure", err) + } + if _, statErr := os.Stat(filepath.Join(workspace, "previous", "version")); statErr != nil { + t.Errorf("the backup must be left intact: %v", statErr) + } +} + +// With nothing at the target there is no second tree to choose between, and the +// backup is the only copy in existence. Asking the caller could only produce an +// answer that throws it away. +func TestRecoverDoesNotConsultTheReconcilerWithNothingAtTheTarget(t *testing.T) { + dir := t.TempDir() + plantInterruptedCommit(t, dir, "demo", "demo", "old") + reconciler := &recordingReconciler{phase: PhaseCommitted} + + if err := Recover(dir, reconciler.reconcile); err != nil { + t.Fatalf("Recover: %v", err) + } + + if reconciler.calls != 0 { + t.Errorf("reconciler calls = %d, want 0", reconciler.calls) + } + data, err := os.ReadFile(filepath.Join(dir, "demo", "version")) + if err != nil || string(data) != "old" { + t.Fatalf("the only copy of the install was not put back: got %q err %v", data, err) + } +} + +// A recovery set that was never enumerated is not an empty recovery set. Callers +// abort on this rather than installing over, or reporting the removal of, a tree +// that may still be owed a restore. +func TestRecoverReportsAnInstallRootItCannotRead(t *testing.T) { + if err := Recover(filepath.Join(t.TempDir(), "missing"), nil); err == nil { + t.Fatal("an unreadable install root must be reported, not read as nothing to do") + } +} + +// The restore is the whole point of the transaction, so a rename it cannot +// complete is the loudest failure recovery has. +func TestRecoverReportsAFailedRestore(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") + } + dir := t.TempDir() + workspace := plantPublishedCommit(t, dir, "demo", "demo", "new", "old") + t.Cleanup(func() { _ = os.Chmod(dir, 0o755) }) + if err := os.Chmod(dir, 0o555); err != nil { + t.Fatal(err) + } + reconciler := &recordingReconciler{phase: PhasePrePublish} + + err := Recover(dir, reconciler.reconcile) + + if err == nil { + t.Fatal("a restore that could not be made must be reported") + } + if err := os.Chmod(dir, 0o755); err != nil { + t.Fatal(err) + } + data, readErr := os.ReadFile(filepath.Join(workspace, "previous", "version")) + if readErr != nil || string(data) != "old" { + t.Fatalf("a backup that could not be restored must be kept: got %q err %v", data, readErr) + } +} + +// Retiring a superseded workspace is bookkeeping, but a failure still leaves a +// backup on disk that the next pass will read again, so it is reported too. +func TestRecoverReportsAFailedRetirement(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") + } + dir := t.TempDir() + workspace := plantPublishedCommit(t, dir, "demo", "demo", "new", "old") + t.Cleanup(func() { _ = os.Chmod(workspace, 0o755) }) + if err := os.Chmod(workspace, 0o555); err != nil { + t.Fatal(err) + } + reconciler := &recordingReconciler{phase: PhaseCommitted} + + err := Recover(dir, reconciler.reconcile) + + if err == nil { + t.Fatal("a retirement that could not be made must be reported") + } + data, readErr := os.ReadFile(filepath.Join(dir, "demo", "version")) + if readErr != nil || string(data) != "new" { + t.Fatalf("the committed install must be left alone: got %q err %v", data, readErr) + } +} + +// A backup we cannot even stat is not a missing backup. Reading it as one would +// have the workspace skipped as mid-transaction while a tree sits in it. +func TestRecoverReportsABackupItCannotStat(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation needs privileges on windows") + } + dir := t.TempDir() + staged, _, err := StageDir(dir) + if err != nil { + t.Fatal(err) + } + workspace := filepath.Dir(staged) + writeMarker(t, workspace, "zero-install-txn v1\ntarget demo\n") + if err := os.Symlink("previous", filepath.Join(workspace, "previous")); err != nil { + t.Fatal(err) + } + + if err := Recover(dir, nil); err == nil { + t.Fatal("a backup that could not be inspected must be reported") + } +} + +// One unresolved workspace must not strand the others: the recovery set is +// processed to the end and every failure is reported together. +func TestRecoverProcessesEveryWorkspaceAndReportsThemTogether(t *testing.T) { + dir := t.TempDir() + unresolved := plantPublishedCommit(t, dir, "unknown", "unknown", "new", "old") + plantInterruptedCommit(t, dir, "demo", "demo", "old") + reconciler := &recordingReconciler{phase: PhaseUnknown} + + err := Recover(dir, reconciler.reconcile) + + if err == nil { + t.Fatal("the unresolved workspace must be reported") + } + data, readErr := os.ReadFile(filepath.Join(dir, "demo", "version")) + if readErr != nil || string(data) != "old" { + t.Fatalf("the resolvable workspace must still be recovered: got %q err %v", data, readErr) + } + if _, statErr := os.Stat(filepath.Join(unresolved, "previous")); statErr != nil { + t.Errorf("the unresolved backup must be left intact: %v", statErr) + } +} + +// A workspace killed before the first rename holds a staged tree and no backup. +// There is nothing to put back, the live install never moved, and the workspace +// is somebody else's to clean up. +func TestRecoverSkipsAWorkspaceInterruptedBeforeTheFirstRename(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "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, _, err := StageDir(dir) + if err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(staged, 0o755); err != nil { + t.Fatal(err) + } + workspace := filepath.Dir(staged) + writeMarker(t, workspace, "zero-install-txn v1\ntarget demo\n") + reconciler := &recordingReconciler{phase: PhaseUnknown} + + if err := Recover(dir, reconciler.reconcile); err != nil { + t.Fatalf("Recover: %v", err) + } + + if reconciler.calls != 0 { + t.Errorf("reconciler calls = %d, want 0", reconciler.calls) + } + data, err := os.ReadFile(filepath.Join(target, "version")) + if err != nil || string(data) != "old" { + t.Fatalf("the live install must be left alone: got %q err %v", data, err) + } + if _, err := os.Stat(staged); err != nil { + t.Errorf("a workspace with no backup must be left alone: %v", err) + } +} + +// Recovery runs on every lock acquisition, so it has to be a no-op on a state it +// already resolved. Nothing it leaves behind may read as another interrupted +// transaction. +func TestRecoverIsANoOpOnAResolvedState(t *testing.T) { + for _, tc := range []struct { + name string + plant func(t *testing.T, dir string) + phase Phase + want string + }{ + {"restored backup", func(t *testing.T, dir string) { plantInterruptedCommit(t, dir, "demo", "demo", "old") }, PhaseCommitted, "old"}, + {"restored over the target", func(t *testing.T, dir string) { + plantPublishedCommit(t, dir, "demo", "demo", "new", "old") + }, PhasePrePublish, "old"}, + {"retired backup", func(t *testing.T, dir string) { + plantPublishedCommit(t, dir, "demo", "demo", "new", "old") + }, PhaseCommitted, "new"}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + tc.plant(t, dir) + reconciler := &recordingReconciler{phase: tc.phase} + if err := Recover(dir, reconciler.reconcile); err != nil { + t.Fatalf("first Recover: %v", err) + } + before := reconciler.calls + + if err := Recover(dir, reconciler.reconcile); err != nil { + t.Fatalf("second Recover: %v", err) + } + + if reconciler.calls != before { + t.Errorf("the second pass found another transaction to classify: calls %d then %d", before, reconciler.calls) + } + data, err := os.ReadFile(filepath.Join(dir, "demo", "version")) + if err != nil || string(data) != tc.want { + t.Fatalf("the second pass changed the install: got %q err %v", data, err) + } + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + for _, entry := range entries { + if strings.HasPrefix(entry.Name(), workspacePrefix) { + t.Errorf("a resolved workspace was left behind: %s", entry.Name()) + } + } + }) + } +} + +// A retirement that fails partway must not cost the workspace its attribution. +// os.RemoveAll unlinks the marker before it reaches the backup, so a retirement +// that dies inside the backup used to leave a workspace holding a tree that +// nothing could attribute: the next pass read it as somebody else's content, +// stayed silent, and every later caller went on as though the transaction had +// resolved. The backup goes first, so anything left behind is still ours and is +// still reported. +func TestRecoverKeepsAttributionWhenRetirementFailsPartway(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") + } + dir := t.TempDir() + workspace := plantPublishedCommit(t, dir, "demo", "demo", "new", "old") + backup := filepath.Join(workspace, "previous") + t.Cleanup(func() { _ = os.Chmod(backup, 0o755) }) + if err := os.Chmod(backup, 0o555); err != nil { + t.Fatal(err) + } + reconciler := &recordingReconciler{phase: PhaseCommitted} + + if err := Recover(dir, reconciler.reconcile); err == nil { + t.Fatal("a retirement that could not be made must be reported") + } + + if _, err := os.Stat(filepath.Join(workspace, markerFileName)); err != nil { + t.Fatalf("a failed retirement dropped the marker that attributes the workspace: %v", err) + } + if err := Recover(dir, reconciler.reconcile); err == nil { + t.Fatal("the second pass went silent on a workspace still holding a backup") + } +} diff --git a/internal/plugins/install.go b/internal/plugins/install.go index d27f77b40..ebe9b92e8 100644 --- a/internal/plugins/install.go +++ b/internal/plugins/install.go @@ -153,8 +153,12 @@ 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) + // Put back anything an earlier run was killed mid-commit, and abort if a + // transaction could not be resolved: installing over a tree that is still + // owed a restore destroys the only copy of it. See installtxn.Recover. + if err := installtxn.Recover(dir, reconcileInterrupted(dir)); err != nil { + return InstallResult{}, err + } // Re-read under the cross-process lock. Another install may have updated the // lockfile while this plugin was fetched and staged. @@ -207,8 +211,13 @@ 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) + // Put back anything an earlier run was killed mid-commit, and abort if a + // transaction could not be resolved: reporting a removal while a backup this + // run never saw stays on disk lets the next install publish it again. See + // installtxn.Recover. + if err := installtxn.Recover(dir, reconcileInterrupted(dir)); err != nil { + return err + } lock, err := ReadLock(dir) if err != nil { @@ -240,6 +249,66 @@ func Remove(dir string, id string) error { return nil } +// reconcileInterrupted answers the one question installtxn.Recover cannot read +// off the filesystem: of the two trees an interrupted commit left, which one is +// the one the lockfile records. CommitDir swaps the trees and only then +// publishes, so a kill between the two leaves exactly what a kill after both +// leaves, and only the published metadata tells them apart. The recorded hash is +// the same hashTree the install computed over the tree it copied, so matching it +// against each tree is an exact answer rather than a guess at filesystem shape. +func reconcileInterrupted(dir string) installtxn.Reconciler { + return func(id string, target string, backup string) (installtxn.Phase, error) { + lock, err := ReadLock(dir) + if err != nil { + // An unreadable lockfile is not an empty one. Reporting a phase from it + // would be reading a fact out of a file we could not read. + return installtxn.PhaseUnknown, err + } + entry, ok := lock[id] + if !ok { + // Every publish writes the entry, so a missing one proves the publish + // never ran and the backup is still the tree the lockfile describes. + return installtxn.PhasePrePublish, nil + } + if entry.Hash == "" { + // A hand edited entry records no tree at all, so there is nothing to + // match either side against and no phase to report. + return installtxn.PhaseUnknown, nil + } + matched, err := treeMatchesHash(target, entry.Hash) + if err != nil { + return installtxn.PhaseUnknown, err + } + if matched { + return installtxn.PhaseCommitted, nil + } + matched, err = treeMatchesHash(backup, entry.Hash) + if err != nil { + return installtxn.PhaseUnknown, err + } + if matched { + return installtxn.PhasePrePublish, nil + } + return installtxn.PhaseUnknown, nil + } +} + +// treeMatchesHash reports whether the tree at path hashes to want. A path that +// is not there is a non-match rather than an error: recovery asks about two +// trees it has just seen on disk, so one going missing under it only means that +// tree is not the one the lockfile records. Every other hashing failure is +// returned, because a tree we could not read may well be the recorded one. +func treeMatchesHash(path string, want string) (bool, error) { + hash, err := hashTree(path) + if errors.Is(err, os.ErrNotExist) { + return false, nil + } + if err != nil { + return false, err + } + return hash == want, nil +} + // ReadLock loads the plugins lockfile from dir. A missing lockfile yields an // empty map with no error. func ReadLock(dir string) (map[string]LockEntry, error) { diff --git a/internal/plugins/install_test.go b/internal/plugins/install_test.go index b2e02776f..55da42778 100644 --- a/internal/plugins/install_test.go +++ b/internal/plugins/install_test.go @@ -426,9 +426,7 @@ func TestInstallRecoversAPluginLeftByAnInterruptedCommit(t *testing.T) { t.Fatal(err) } workspace := filepath.Dir(staged) - if err := os.WriteFile(filepath.Join(workspace, "target"), []byte("zero.demo"), 0o644); err != nil { - t.Fatal(err) - } + writeWorkspaceMarker(t, workspace, "zero.demo") if err := os.Rename(filepath.Join(dir, "zero.demo"), filepath.Join(workspace, "previous")); err != nil { t.Fatal(err) } @@ -476,9 +474,7 @@ func TestRemoveLeavesNothingARecoveryCanResurrect(t *testing.T) { t.Fatal(err) } workspace := filepath.Dir(staged) - if err := os.WriteFile(filepath.Join(workspace, "target"), []byte("zero.demo"), 0o600); err != nil { - t.Fatal(err) - } + writeWorkspaceMarker(t, workspace, "zero.demo") if err := os.Rename(filepath.Join(dir, "zero.demo"), filepath.Join(workspace, "previous")); err != nil { t.Fatal(err) } @@ -528,9 +524,7 @@ func TestRemoveLeavesNoSupersededBackupARecoveryCanResurrect(t *testing.T) { t.Fatal(err) } workspace := filepath.Dir(staged) - if err := os.WriteFile(filepath.Join(workspace, "target"), []byte("zero.demo"), 0o600); err != nil { - t.Fatal(err) - } + writeWorkspaceMarker(t, workspace, "zero.demo") previous := filepath.Join(workspace, "previous") if err := os.MkdirAll(previous, 0o755); err != nil { t.Fatal(err) @@ -574,3 +568,564 @@ func TestRemoveLeavesNoSupersededBackupARecoveryCanResurrect(t *testing.T) { t.Errorf("the lockfile still names a removed plugin") } } + +// writeWorkspaceMarker plants the ownership marker installtxn.CommitDir writes +// before it moves a live tree aside. Recovery acts only on a workspace carrying +// that marker's magic first line, so a test standing in for a killed process has +// to write the real thing: the workspace prefix and a previous directory beside +// it are shapes ordinary user content can have too, and prove nothing. +func writeWorkspaceMarker(t *testing.T, workspace string, id string) { + t.Helper() + marker := []byte("zero-install-txn v1\ntarget " + id + "\n") + if err := os.WriteFile(filepath.Join(workspace, ".zero-install-txn"), marker, 0o600); err != nil { + t.Fatal(err) + } +} + +// generationSource writes a plugin source whose content names which generation +// it is, so a test can tell the recovered tree from the one it replaced by +// reading the tree on disk instead of trusting the lockfile that is itself +// under test. +func generationSource(t *testing.T, generation string) string { + t.Helper() + versions := map[string]string{"old": "0.1.0", "new": "0.2.0"} + manifest := validManifest() + manifest["version"] = versions[generation] + dir := writeSourcePlugin(t, filepath.Join(t.TempDir(), "src-"+generation), manifest) + if err := os.WriteFile(filepath.Join(dir, "generation.txt"), []byte(generation), 0o644); err != nil { + t.Fatal(err) + } + return dir +} + +// interruptedUpdate is a plugins dir with the old generation installed, plus +// everything the update over it would have used. An update is the only shape +// that leaves a backup owed a restore, so every interrupted state below is one. +type interruptedUpdate struct { + dir string + oldSource string + oldHash string + newSource string + newHash string +} + +func seedInterruptedUpdate(t *testing.T) interruptedUpdate { + t.Helper() + dir := t.TempDir() + seeded, err := Install(context.Background(), InstallOptions{Source: generationSource(t, "old"), Dir: dir}) + if err != nil { + t.Fatalf("seeding the install: %v", err) + } + newSource := generationSource(t, "new") + newHash, err := hashTree(newSource) + if err != nil { + t.Fatal(err) + } + return interruptedUpdate{ + dir: dir, + oldSource: seeded.Source, + oldHash: seeded.Hash, + newSource: canonicalSource(newSource), + newHash: newHash, + } +} + +// plantWorkspace builds what CommitDir has on disk before it touches the live +// tree: a staged copy of the new generation and the ownership marker naming the +// install it is about to replace. +func (u interruptedUpdate) plantWorkspace(t *testing.T) string { + t.Helper() + staged, _, err := installtxn.StageDir(u.dir) + if err != nil { + t.Fatal(err) + } + if err := copyTree(u.newSource, staged); err != nil { + t.Fatal(err) + } + workspace := filepath.Dir(staged) + writeWorkspaceMarker(t, workspace, "zero.demo") + return workspace +} + +func (u interruptedUpdate) target() string { + return filepath.Join(u.dir, "zero.demo") +} + +func (u interruptedUpdate) mustRename(t *testing.T, from string, to string) { + t.Helper() + if err := os.Rename(from, to); err != nil { + t.Fatal(err) + } +} + +// publishNewEntry writes the lockfile the interrupted update was about to +// publish, which is the only thing separating a commit killed before the +// publish from one killed after it. +func (u interruptedUpdate) publishNewEntry(t *testing.T) { + t.Helper() + lock, err := ReadLock(u.dir) + if err != nil { + t.Fatal(err) + } + lock["zero.demo"] = LockEntry{Source: u.newSource, Hash: u.newHash} + if err := writeLock(u.dir, lock); err != nil { + t.Fatal(err) + } +} + +// recoveryWant is the state a resolved workspace has to leave behind. +type recoveryWant struct { + generation string + version string + source string + hash string + workspace bool +} + +// interruptedState is one point CommitDir can be killed at, with the state the +// next run must arrive at. +type interruptedState struct { + name string + plant func(t *testing.T, u interruptedUpdate) string + want func(u interruptedUpdate) recoveryWant +} + +func interruptedStates() []interruptedState { + oldTree := func(u interruptedUpdate, workspace bool) recoveryWant { + return recoveryWant{generation: "old", version: "0.1.0", source: u.oldSource, hash: u.oldHash, workspace: workspace} + } + return []interruptedState{ + { + // Killed after the marker, before anything moved. Nothing is owed a + // restore, and the workspace belongs to whoever made it. + name: "S1_after_marker", + plant: func(t *testing.T, u interruptedUpdate) string { + return u.plantWorkspace(t) + }, + want: func(u interruptedUpdate) recoveryWant { return oldTree(u, true) }, + }, + { + // Killed between the two renames: the backup is the only copy of the + // plugin in existence and nothing is at the target. + name: "S2_after_retaining_previous", + plant: func(t *testing.T, u interruptedUpdate) string { + workspace := u.plantWorkspace(t) + u.mustRename(t, u.target(), filepath.Join(workspace, "previous")) + return workspace + }, + want: func(u interruptedUpdate) recoveryWant { return oldTree(u, false) }, + }, + { + // Killed after the swap and before the publish. Both trees are on disk + // and only the lockfile says which one the user has: it still records + // the backup, so the backup goes back and the new tree is dropped. + name: "S3_after_swap_before_publish", + plant: func(t *testing.T, u interruptedUpdate) string { + workspace := u.plantWorkspace(t) + u.mustRename(t, u.target(), filepath.Join(workspace, "previous")) + u.mustRename(t, filepath.Join(workspace, "staged"), u.target()) + return workspace + }, + want: func(u interruptedUpdate) recoveryWant { return oldTree(u, false) }, + }, + { + // The same shape on disk as S3, told apart only by the published entry: + // the update committed, so the live tree stands and the backup beside + // it is superseded. + name: "S4_after_publish_before_cleanup", + plant: func(t *testing.T, u interruptedUpdate) string { + workspace := u.plantWorkspace(t) + u.mustRename(t, u.target(), filepath.Join(workspace, "previous")) + u.mustRename(t, filepath.Join(workspace, "staged"), u.target()) + u.publishNewEntry(t) + return workspace + }, + want: func(u interruptedUpdate) recoveryWant { + return recoveryWant{generation: "new", version: "0.2.0", source: u.newSource, hash: u.newHash, workspace: false} + }, + }, + { + // Killed inside a rollback: the failed tree is already set aside and the + // backup is again the only copy, which is the state that ordering buys. + name: "S5_interrupted_rollback", + plant: func(t *testing.T, u interruptedUpdate) string { + workspace := u.plantWorkspace(t) + u.mustRename(t, u.target(), filepath.Join(workspace, "previous")) + u.mustRename(t, filepath.Join(workspace, "staged"), filepath.Join(workspace, "failed")) + return workspace + }, + want: func(u interruptedUpdate) recoveryWant { return oldTree(u, false) }, + }, + } +} + +// installOther installs an unrelated plugin, which is how a recovery pass is +// driven: the next install over the same dir takes the same lock. +func installOther(t *testing.T, dir string, id string) error { + t.Helper() + manifest := validManifest() + manifest["id"] = id + source := writeSourcePlugin(t, filepath.Join(t.TempDir(), id), manifest) + _, err := Install(context.Background(), InstallOptions{Source: source, Dir: dir}) + return err +} + +func assertRecovered(t *testing.T, u interruptedUpdate, workspace string, want recoveryWant) { + t.Helper() + data, err := os.ReadFile(filepath.Join(u.target(), "generation.txt")) + if err != nil { + t.Fatalf("reading the live tree: %v", err) + } + if string(data) != want.generation { + t.Errorf("the live tree is the %q generation, want %q", data, want.generation) + } + lock, err := ReadLock(u.dir) + if err != nil { + t.Fatal(err) + } + entry := lock["zero.demo"] + if entry.Source != want.source { + t.Errorf("lock source = %q, want %q", entry.Source, want.source) + } + if entry.Hash != want.hash { + t.Errorf("lock hash = %q, want %q", entry.Hash, want.hash) + } + // A tree that only stats is not a recovered install: it has to come back + // through the same loader the rest of the program reads plugins with. + loaded, err := Load(LoadOptions{Roots: []Root{{Source: SourceUser, Path: u.dir}}}) + if err != nil { + t.Fatal(err) + } + found := false + for _, plugin := range loaded.Plugins { + if plugin.ID != "zero.demo" { + continue + } + found = true + if plugin.Version != want.version { + t.Errorf("Load sees version %q, want %q", plugin.Version, want.version) + } + } + if !found { + t.Errorf("Load does not see the recovered plugin") + } + _, err = os.Stat(workspace) + if want.workspace && err != nil { + t.Errorf("the workspace should be left alone, got %v", err) + } + if !want.workspace && !os.IsNotExist(err) { + t.Errorf("the resolved workspace should be cleared, got %v", err) + } +} + +func assertRemovedForGood(t *testing.T, dir string) { + t.Helper() + if _, err := os.Stat(filepath.Join(dir, "zero.demo")); !os.IsNotExist(err) { + t.Errorf("a removed plugin is on disk again: %v", err) + } + 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") + } + loaded, err := Load(LoadOptions{Roots: []Root{{Source: SourceUser, Path: dir}}}) + if err != nil { + t.Fatal(err) + } + for _, plugin := range loaded.Plugins { + if plugin.ID == "zero.demo" { + t.Errorf("a removed plugin is loadable again") + } + } +} + +// Every point an update can be killed at has one right resolution, and the next +// install is one of the two places it happens. +func TestInstallResolvesEveryInterruptedUpdateState(t *testing.T) { + for _, state := range interruptedStates() { + t.Run(state.name, func(t *testing.T) { + u := seedInterruptedUpdate(t) + workspace := state.plant(t, u) + + if err := installOther(t, u.dir, "zero.other"); err != nil { + t.Fatalf("install after the interrupted update: %v", err) + } + assertRecovered(t, u, workspace, state.want(u)) + + // A second pass must find nothing to do. Recovery that resolved the same + // workspace twice would undo the state it just settled on. + if err := installOther(t, u.dir, "zero.third"); err != nil { + t.Fatalf("second recovery pass: %v", err) + } + assertRecovered(t, u, workspace, state.want(u)) + }) + } +} + +// The other place recovery runs is a removal, and it has to run there first: +// removing without resolving leaves a backup the next install republishes, +// reinstating a plugin the user deleted. +func TestRemoveResolvesEveryInterruptedUpdateStateAndSticks(t *testing.T) { + for _, state := range interruptedStates() { + t.Run(state.name, func(t *testing.T) { + u := seedInterruptedUpdate(t) + state.plant(t, u) + + if err := Remove(u.dir, "zero.demo"); err != nil { + t.Fatalf("remove after the interrupted update: %v", err) + } + assertRemovedForGood(t, u.dir) + + if err := installOther(t, u.dir, "zero.other"); err != nil { + t.Fatalf("install after the removal: %v", err) + } + assertRemovedForGood(t, u.dir) + }) + } +} + +// assertUntouched pins the two things a caller must not have changed after +// recovery reported a transaction it could not resolve. +func assertUntouched(t *testing.T, u interruptedUpdate, generation string, want LockEntry) { + t.Helper() + data, err := os.ReadFile(filepath.Join(u.target(), "generation.txt")) + if err != nil { + t.Fatalf("reading the live tree: %v", err) + } + if string(data) != generation { + t.Errorf("the live tree is the %q generation, want %q", data, generation) + } + lock, err := ReadLock(u.dir) + if err != nil { + t.Fatal(err) + } + if lock["zero.demo"] != want { + t.Errorf("lock entry = %+v, want %+v", lock["zero.demo"], want) + } + if _, ok := lock["zero.other"]; ok { + t.Errorf("the aborted install still published its own entry") + } + if _, err := os.Stat(filepath.Join(u.dir, "zero.other")); !os.IsNotExist(err) { + t.Errorf("the aborted install still wrote its tree: %v", err) + } +} + +// Neither tree matches what the lockfile records, so there is no answer to give +// and either guess destroys the tree the user is actually owed. The failure has +// to reach the caller, which must not install over or remove anything. +func TestInterruptedUpdateMatchingNeitherTreeAbortsBothCallers(t *testing.T) { + u := seedInterruptedUpdate(t) + workspace := u.plantWorkspace(t) + u.mustRename(t, u.target(), filepath.Join(workspace, "previous")) + u.mustRename(t, filepath.Join(workspace, "staged"), u.target()) + unmatched := LockEntry{Source: u.oldSource, Hash: "sha256:0000000000000000000000000000000000000000000000000000000000000000"} + lock, err := ReadLock(u.dir) + if err != nil { + t.Fatal(err) + } + lock["zero.demo"] = unmatched + if err := writeLock(u.dir, lock); err != nil { + t.Fatal(err) + } + + if err := installOther(t, u.dir, "zero.other"); err == nil { + t.Errorf("Install did not report the unresolved transaction") + } + assertUntouched(t, u, "new", unmatched) + if err := Remove(u.dir, "zero.demo"); err == nil { + t.Errorf("Remove did not report the unresolved transaction") + } + assertUntouched(t, u, "new", unmatched) + if _, err := os.Stat(filepath.Join(workspace, "previous")); err != nil { + t.Errorf("the backup should still be there for a hand rescue, got %v", err) + } +} + +// A lockfile that cannot be read is not an empty one: the reconciler has no +// answer, and a caller that went on would install over or report the removal of +// a tree that may still be owed a restore. +func TestInterruptedUpdateWithAnUnreadableLockAbortsBothCallers(t *testing.T) { + u := seedInterruptedUpdate(t) + workspace := u.plantWorkspace(t) + u.mustRename(t, u.target(), filepath.Join(workspace, "previous")) + u.mustRename(t, filepath.Join(workspace, "staged"), u.target()) + if err := os.WriteFile(filepath.Join(u.dir, LockFileName), []byte("{not json"), 0o644); err != nil { + t.Fatal(err) + } + + if err := installOther(t, u.dir, "zero.other"); err == nil { + t.Errorf("Install did not report the unresolved transaction") + } + if err := Remove(u.dir, "zero.demo"); err == nil { + t.Errorf("Remove did not report the unresolved transaction") + } + data, err := os.ReadFile(filepath.Join(u.target(), "generation.txt")) + if err != nil || string(data) != "new" { + t.Errorf("the live tree changed under an unresolved transaction: %q, %v", data, err) + } + if _, err := os.Stat(filepath.Join(workspace, "previous")); err != nil { + t.Errorf("the backup should still be there for a hand rescue, got %v", err) + } +} + +// A restore that cannot be carried out is the case where silence is worst: the +// plugin is missing from the target and its only copy is in a workspace nothing +// else reads. +func TestAFailedRestoreAbortsBothCallers(t *testing.T) { + u := seedInterruptedUpdate(t) + workspace := u.plantWorkspace(t) + u.mustRename(t, u.target(), filepath.Join(workspace, "previous")) + // A workspace that cannot be written is the cheap stand-in for a rename that + // fails: the backup cannot leave it. + if err := os.Chmod(workspace, 0o555); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(workspace, 0o755) }) + + if err := installOther(t, u.dir, "zero.other"); err == nil { + t.Errorf("Install did not report the failed restore") + } + if err := Remove(u.dir, "zero.demo"); err == nil { + t.Errorf("Remove did not report the failed restore") + } + if _, err := os.Stat(u.target()); !os.IsNotExist(err) { + t.Errorf("nothing should have been written at the target: %v", err) + } + lock, err := ReadLock(u.dir) + if err != nil { + t.Fatal(err) + } + if lock["zero.demo"] != (LockEntry{Source: u.oldSource, Hash: u.oldHash}) { + t.Errorf("lock entry = %+v, want the untouched old entry", lock["zero.demo"]) + } + if _, ok := lock["zero.other"]; ok { + t.Errorf("the aborted install still published its own entry") + } + if _, err := os.Stat(filepath.Join(workspace, "previous")); err != nil { + t.Errorf("the only copy of the plugin should still be there, got %v", err) + } +} + +// A publish always writes the lockfile entry, so an interrupted update with no +// entry naming it proves the publish never ran and the backup is still the tree +// the lockfile describes, even though there is no hash left to match it against. +func TestInterruptedUpdateWithNoLockEntryRestoresTheBackup(t *testing.T) { + u := seedInterruptedUpdate(t) + workspace := u.plantWorkspace(t) + u.mustRename(t, u.target(), filepath.Join(workspace, "previous")) + u.mustRename(t, filepath.Join(workspace, "staged"), u.target()) + lock, err := ReadLock(u.dir) + if err != nil { + t.Fatal(err) + } + delete(lock, "zero.demo") + if err := writeLock(u.dir, lock); err != nil { + t.Fatal(err) + } + + if err := installOther(t, u.dir, "zero.other"); err != nil { + t.Fatalf("install after the interrupted update: %v", err) + } + assertRecovered(t, u, workspace, recoveryWant{generation: "old", version: "0.1.0"}) +} + +// An entry that records no hash describes no tree, so neither side can be +// matched against it and there is no phase to report. +func TestInterruptedUpdateWithAHashlessLockEntryAbortsTheCaller(t *testing.T) { + u := seedInterruptedUpdate(t) + workspace := u.plantWorkspace(t) + u.mustRename(t, u.target(), filepath.Join(workspace, "previous")) + u.mustRename(t, filepath.Join(workspace, "staged"), u.target()) + hashless := LockEntry{Source: u.oldSource} + lock, err := ReadLock(u.dir) + if err != nil { + t.Fatal(err) + } + lock["zero.demo"] = hashless + if err := writeLock(u.dir, lock); err != nil { + t.Fatal(err) + } + + if err := installOther(t, u.dir, "zero.other"); err == nil { + t.Errorf("Install did not report the unresolved transaction") + } + assertUntouched(t, u, "new", hashless) +} + +// interruptedCaller is one of the two entry points that take the install lock. +// Both must abort on a transaction recovery could not resolve. +type interruptedCaller struct { + name string + run func(t *testing.T, dir string) error +} + +func interruptedCallers() []interruptedCaller { + return []interruptedCaller{ + {name: "install", run: func(t *testing.T, dir string) error { return installOther(t, dir, "zero.other") }}, + {name: "remove", run: func(t *testing.T, dir string) error { return Remove(dir, "zero.demo") }}, + } +} + +// Retiring a superseded backup can fail too, and leaving it behind is not +// harmless: a later removal deletes the live tree, and a backup that outlived it +// would be read as an interrupted swap and published again. Each caller gets its +// own planted state here, because a retirement that fails partway can take the +// ownership marker with it and leave the next pass a workspace it cannot +// attribute. +func TestAFailedRetirementAbortsBothCallers(t *testing.T) { + for _, caller := range interruptedCallers() { + t.Run(caller.name, func(t *testing.T) { + u := seedInterruptedUpdate(t) + workspace := u.plantWorkspace(t) + u.mustRename(t, u.target(), filepath.Join(workspace, "previous")) + u.mustRename(t, filepath.Join(workspace, "staged"), u.target()) + u.publishNewEntry(t) + // The backup's own contents cannot be unlinked, so removing the + // workspace around it fails. + if err := os.Chmod(filepath.Join(workspace, "previous"), 0o555); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(filepath.Join(workspace, "previous"), 0o755) }) + + if err := caller.run(t, u.dir); err == nil { + t.Errorf("%s did not report the failed retirement", caller.name) + } + assertUntouched(t, u, "new", LockEntry{Source: u.newSource, Hash: u.newHash}) + }) + } +} + +// The workspace prefix is a public dot prefixed name and a previous directory +// beside a target file is a shape ordinary content can have, so neither is +// evidence. Without the magic marker this is somebody's content and recovery +// must leave it exactly as it found it, and say nothing. +func TestRecoveryLeavesAPrefixCollidingDirectoryAlone(t *testing.T) { + dir := t.TempDir() + collided := filepath.Join(dir, ".zero-install-txn-notes") + previous := filepath.Join(collided, "previous") + if err := os.MkdirAll(previous, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(previous, "SKILL.md"), []byte("notes\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(collided, "target"), []byte("zero.demo"), 0o644); err != nil { + t.Fatal(err) + } + + if err := installOther(t, dir, "zero.other"); err != nil { + t.Fatalf("install alongside a prefix colliding directory: %v", err) + } + data, err := os.ReadFile(filepath.Join(previous, "SKILL.md")) + if err != nil || string(data) != "notes\n" { + t.Errorf("user content inside the colliding directory was disturbed: %q, %v", data, err) + } + if _, err := os.Stat(filepath.Join(collided, "target")); err != nil { + t.Errorf("the colliding directory lost a file: %v", err) + } + if _, err := os.Stat(filepath.Join(dir, "zero.demo")); !os.IsNotExist(err) { + t.Errorf("recovery published content it does not own: %v", err) + } +} diff --git a/internal/skills/install.go b/internal/skills/install.go index 9d92837ca..0169a6dba 100644 --- a/internal/skills/install.go +++ b/internal/skills/install.go @@ -151,7 +151,11 @@ func Install(ctx context.Context, options InstallOptions) (InstallResult, error) } defer unlock() // Put back anything an earlier run was killed mid-commit; see installtxn.Recover. - installtxn.Recover(dir) + // Abort on a transaction it could not resolve: installing over a target that is + // still owed a restore destroys the only copy of the tree it is owed. + if err := installtxn.Recover(dir, lockReconciler(dir)); err != nil { + return InstallResult{}, err + } // Re-read under the cross-process lock. Another install may have updated the // lockfile while this skill was fetched and staged. @@ -186,6 +190,62 @@ func Install(ctx context.Context, options InstallOptions) (InstallResult, error) return result, nil } +// lockReconciler answers, for one interrupted workspace, which of the two trees +// beside each other the lockfile records. A kill between the tree swap and the +// lockfile publish leaves exactly what a kill after both leaves, so filesystem +// shape cannot tell them apart and only the recorded hash can: the tree it +// describes is the one the user was told they had. +func lockReconciler(dir string) installtxn.Reconciler { + return func(name string, target string, backup string) (installtxn.Phase, error) { + lock, err := ReadLock(dir) + if err != nil { + return installtxn.PhaseUnknown, err + } + entry, locked := lock[name] + if !locked { + // The publish writes the entry for every install, so no entry at all + // proves the publish never ran and the backup is what the user had. + return installtxn.PhasePrePublish, nil + } + if entry.Hash == "" { + // Nothing to compare against, so neither tree can be shown to be the + // recorded one and recovery must not pick between them. + return installtxn.PhaseUnknown, nil + } + targetHash, err := manifestHash(target) + if err != nil { + return installtxn.PhaseUnknown, err + } + if targetHash == entry.Hash { + return installtxn.PhaseCommitted, nil + } + backupHash, err := manifestHash(backup) + if err != nil { + return installtxn.PhaseUnknown, err + } + if backupHash == entry.Hash { + return installtxn.PhasePrePublish, nil + } + return installtxn.PhaseUnknown, nil + } +} + +// manifestHash hashes the SKILL.md under path the same way an install records +// it. A tree with no SKILL.md is a non-match rather than a failure: an install +// killed mid-copy can leave one, and it simply is not the tree the lockfile +// describes. Any other read error means the comparison could not be made at all, +// which is not the same answer and is reported. +func manifestHash(path string) (string, error) { + data, err := os.ReadFile(filepath.Join(path, skillFileName)) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return "", nil + } + return "", fmt.Errorf("read %s: %w", skillFileName, err) + } + return hashContent(data), nil +} + // Remove deletes an installed skill directory and its lockfile entry. It errors // if the named skill is not present in either the dir or the lockfile. func Remove(dir string, name string) error { @@ -204,7 +264,12 @@ func Remove(dir string, name string) error { } defer unlock() // Put back anything an earlier run was killed mid-commit; see installtxn.Recover. - installtxn.Recover(dir) + // Abort on a transaction it could not resolve: reporting a successful removal + // leaves a backup nothing else reads, and the next install's recovery would + // publish it again and reinstate what the user deleted. + if err := installtxn.Recover(dir, lockReconciler(dir)); err != nil { + return err + } lock, err := ReadLock(dir) if err != nil { diff --git a/internal/skills/install_test.go b/internal/skills/install_test.go index 2b2b85002..1d470212e 100644 --- a/internal/skills/install_test.go +++ b/internal/skills/install_test.go @@ -7,6 +7,7 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "strings" "testing" @@ -74,6 +75,18 @@ func writeSourceSkill(t *testing.T, dir string, content string) string { return dir } +// writeWorkspaceMarker plants the ownership marker installtxn writes inside a +// workspace before it moves any tree. Recovery acts on a workspace only when +// this marker proves the workspace is one of its own, so a planted interrupted +// state is invisible to it without one. +func writeWorkspaceMarker(t *testing.T, workspace string, name string) { + t.Helper() + marker := []byte("zero-install-txn v1\ntarget " + name + "\n") + if err := os.WriteFile(filepath.Join(workspace, ".zero-install-txn"), marker, 0o600); err != nil { + t.Fatalf("write workspace marker: %v", err) + } +} + func TestInstallCopiesLocalSkillAndRecordsHash(t *testing.T) { destDir := t.TempDir() source := writeSourceSkill(t, filepath.Join(t.TempDir(), "src"), @@ -453,9 +466,7 @@ func TestInstallRecoversASkillLeftByAnInterruptedCommit(t *testing.T) { t.Fatal(err) } workspace := filepath.Dir(staged) - if err := os.WriteFile(filepath.Join(workspace, "target"), []byte("alpha"), 0o600); err != nil { - t.Fatal(err) - } + writeWorkspaceMarker(t, workspace, "alpha") if err := os.Rename(filepath.Join(dir, "alpha"), filepath.Join(workspace, "previous")); err != nil { t.Fatal(err) } @@ -495,9 +506,7 @@ func TestRemoveLeavesNoSupersededBackupARecoveryCanResurrect(t *testing.T) { t.Fatal(err) } workspace := filepath.Dir(staged) - if err := os.WriteFile(filepath.Join(workspace, "target"), []byte("alpha"), 0o600); err != nil { - t.Fatal(err) - } + writeWorkspaceMarker(t, workspace, "alpha") previous := filepath.Join(workspace, "previous") if err := os.MkdirAll(previous, 0o755); err != nil { t.Fatal(err) @@ -531,3 +540,454 @@ func TestRemoveLeavesNoSupersededBackupARecoveryCanResurrect(t *testing.T) { t.Errorf("the lockfile still names a removed skill") } } + +// The recovery states an interrupted update of an installed skill can be killed +// in. Each is planted by hand rather than by killing a real commit, because a +// real kill cannot be aimed at a single step between two renames. +const ( + oldAlphaSkill = "---\nname: alpha\ndescription: old.\n---\nold alpha body\n" + newAlphaSkill = "---\nname: alpha\ndescription: new.\n---\nnew alpha body\n" + gammaSkill = "---\nname: gamma\ndescription: unrelated.\n---\ngamma body\n" + betaSkill = "---\nname: beta\ndescription: later.\n---\nbeta body\n" + deltaSkill = "---\nname: delta\ndescription: later still.\n---\ndelta body\n" +) + +// interruptedUpdate is a skills dir holding an installed alpha and an unrelated +// gamma, plus the workspace an update of alpha to newAlphaSkill left behind when +// it was killed partway through its commit. +type interruptedUpdate struct { + dir string + workspace string + oldSource string + newSource string +} + +// plantInterruptedAlphaUpdate builds the on-disk state a kill at the named step +// of CommitDir leaves. The steps are the commit's own order: write the marker, +// move the target aside, move the staged tree in, publish the lockfile, clean up. +func plantInterruptedAlphaUpdate(t *testing.T, step string) interruptedUpdate { + t.Helper() + dir := t.TempDir() + oldSource := writeSourceSkill(t, filepath.Join(t.TempDir(), "old-alpha"), oldAlphaSkill) + if _, err := Install(context.Background(), InstallOptions{Source: oldSource, Dir: dir}); err != nil { + t.Fatalf("seed alpha: %v", err) + } + gammaSource := writeSourceSkill(t, filepath.Join(t.TempDir(), "gamma"), gammaSkill) + if _, err := Install(context.Background(), InstallOptions{Source: gammaSource, Dir: dir}); err != nil { + t.Fatalf("seed gamma: %v", err) + } + newSource := writeSourceSkill(t, filepath.Join(t.TempDir(), "new-alpha"), newAlphaSkill) + + staged, _, err := installtxn.StageDir(dir) + if err != nil { + t.Fatal(err) + } + workspace := filepath.Dir(staged) + writeWorkspaceMarker(t, workspace, "alpha") + writeSourceSkill(t, staged, newAlphaSkill) + + target := filepath.Join(dir, "alpha") + previous := filepath.Join(workspace, "previous") + rename := func(from string, to string) { + if err := os.Rename(from, to); err != nil { + t.Fatalf("plant %s: %v", step, err) + } + } + switch step { + case "S1": + // The marker is down and no tree has moved yet. + case "S2": + rename(target, previous) + case "S3": + rename(target, previous) + rename(staged, target) + case "S4": + rename(target, previous) + rename(staged, target) + publishLockEntry(t, dir, "alpha", LockEntry{Source: newSource, Hash: hashContent([]byte(newAlphaSkill))}) + case "S5": + // An interrupted rollback: the failed tree was set aside and the backup is + // still the only complete copy. + rename(target, previous) + writeSourceSkill(t, filepath.Join(workspace, "failed"), newAlphaSkill) + if err := os.RemoveAll(staged); err != nil { + t.Fatal(err) + } + default: + t.Fatalf("unknown step %q", step) + } + return interruptedUpdate{dir: dir, workspace: workspace, oldSource: oldSource, newSource: newSource} +} + +func publishLockEntry(t *testing.T, dir string, name string, entry LockEntry) { + t.Helper() + lock, err := ReadLock(dir) + if err != nil { + t.Fatal(err) + } + lock[name] = entry + if err := writeLock(dir, lock); err != nil { + t.Fatal(err) + } +} + +// recoveredAlpha is what the skills dir must hold once recovery has resolved a +// planted state: the SKILL.md at the target, the lock entry beside it, and +// whether the workspace was resolved away or left for somebody else. +type recoveredAlpha struct { + content string + description string + fromNewSource bool + workspaceRetained bool +} + +func assertRecoveredAlpha(t *testing.T, update interruptedUpdate, want recoveredAlpha) { + t.Helper() + data, err := os.ReadFile(filepath.Join(update.dir, "alpha", skillFileName)) + if err != nil { + t.Fatalf("read recovered alpha: %v", err) + } + if string(data) != want.content { + t.Errorf("recovered tree is the wrong one: got %q want %q", data, want.content) + } + lock, err := ReadLock(update.dir) + if err != nil { + t.Fatal(err) + } + entry, locked := lock["alpha"] + if !locked { + t.Fatal("the lockfile no longer records the recovered skill") + } + wantSource := update.oldSource + if want.fromNewSource { + wantSource = update.newSource + } + if entry.Source != wantSource { + t.Errorf("lock source: got %q want %q", entry.Source, wantSource) + } + if wantHash := hashContent([]byte(want.content)); entry.Hash != wantHash { + t.Errorf("lock hash: got %q want %q (the recorded hash must describe the tree on disk)", entry.Hash, wantHash) + } + // The tree has to come back through the loader every other caller reads, not + // merely be present as bytes on disk. + skill, ok := Get(update.dir, "alpha") + if !ok { + t.Fatal("the recovered skill does not load") + } + if skill.Description != want.description { + t.Errorf("loaded skill is the wrong tree: got description %q want %q", skill.Description, want.description) + } + _, statErr := os.Stat(update.workspace) + if want.workspaceRetained && statErr != nil { + t.Errorf("a workspace recovery cannot attribute must be left alone: %v", statErr) + } + if !want.workspaceRetained && !os.IsNotExist(statErr) { + t.Errorf("a resolved workspace must be cleared, got %v", statErr) + } +} + +// installDriver and removeDriver are the two entry points that take the install +// lock, so every recovery state has to come out the same through both. +func installDriver(t *testing.T, dir string) error { + return installSkill(t, dir, betaSkill) +} + +// installSkill installs a skill unrelated to the planted state, so the drive is +// an ordinary install that happens to run recovery first. +func installSkill(t *testing.T, dir string, content string) error { + t.Helper() + source := writeSourceSkill(t, filepath.Join(t.TempDir(), "source"), content) + _, err := Install(context.Background(), InstallOptions{Source: source, Dir: dir}) + return err +} + +func removeDriver(t *testing.T, dir string) error { + t.Helper() + return Remove(dir, "gamma") +} + +var recoveryDrivers = []struct { + name string + run func(t *testing.T, dir string) error +}{ + {"install", installDriver}, + {"remove", removeDriver}, +} + +// treeSnapshot records every path under dir and the content of every regular +// file, so a caller that must not have touched anything can be held to it. +func treeSnapshot(t *testing.T, dir string) map[string]string { + t.Helper() + snapshot := map[string]string{} + if _, err := os.Lstat(dir); os.IsNotExist(err) { + // An absent tree is a state like any other, and a caller that must not have + // touched anything must not have created it either. + return snapshot + } + err := filepath.WalkDir(dir, func(path string, entry os.DirEntry, err error) error { + if err != nil { + return err + } + rel, relErr := filepath.Rel(dir, path) + if relErr != nil { + return relErr + } + if entry.IsDir() { + snapshot[rel] = "" + return nil + } + data, readErr := os.ReadFile(path) + if readErr != nil { + return readErr + } + snapshot[rel] = string(data) + return nil + }) + if err != nil { + t.Fatalf("snapshot %s: %v", dir, err) + } + return snapshot +} + +func assertSnapshotUnchanged(t *testing.T, before map[string]string, after map[string]string) { + t.Helper() + for path, content := range before { + got, present := after[path] + if !present { + t.Errorf("%s was removed by a caller that must have aborted", path) + continue + } + if got != content { + t.Errorf("%s was rewritten by a caller that must have aborted: got %q want %q", path, got, content) + } + } + for path := range after { + if _, present := before[path]; !present { + t.Errorf("%s was created by a caller that must have aborted", path) + } + } +} + +// readLockBytes returns the lockfile exactly as it sits on disk, so a caller +// that must not have republished it can be held to the bytes. +func readLockBytes(t *testing.T, dir string) string { + t.Helper() + data, err := os.ReadFile(filepath.Join(dir, LockFileName)) + if err != nil { + if os.IsNotExist(err) { + return "" + } + t.Fatal(err) + } + return string(data) +} + +// Every step an interrupted update can be killed at, resolved through both entry +// points that take the install lock. The recorded hash is what decides: the tree +// the lockfile describes is the truthful one, and the tree it does not describe +// is the one recovery undoes. +func TestRecoveryResolvesEveryInterruptedUpdateStep(t *testing.T) { + steps := []struct { + step string + want recoveredAlpha + }{ + // The marker is down but no tree moved, so there is nothing to put back and + // the workspace belongs to whoever made it. + {"S1", recoveredAlpha{content: oldAlphaSkill, description: "old.", workspaceRetained: true}}, + // The target is gone and the backup is the only copy there is. + {"S2", recoveredAlpha{content: oldAlphaSkill, description: "old."}}, + // Both trees are on disk and only the lockfile can say which one it records: + // it still names the old source and hash, so the swap landed and the publish + // never did. + {"S3", recoveredAlpha{content: oldAlphaSkill, description: "old."}}, + // The lockfile records the new tree, so the publish committed and the backup + // beside it is superseded. + {"S4", recoveredAlpha{content: newAlphaSkill, description: "new.", fromNewSource: true}}, + // An interrupted rollback looks like an interrupted swap and is repaired the + // same way. + {"S5", recoveredAlpha{content: oldAlphaSkill, description: "old."}}, + } + for _, step := range steps { + for _, driver := range recoveryDrivers { + t.Run(step.step+"/"+driver.name, func(t *testing.T) { + update := plantInterruptedAlphaUpdate(t, step.step) + + if err := driver.run(t, update.dir); err != nil { + t.Fatalf("%s over a recoverable state: %v", driver.name, err) + } + + assertRecoveredAlpha(t, update, step.want) + // A second pass over an already resolved state must change nothing. + if err := installSkill(t, update.dir, deltaSkill); err != nil { + t.Fatalf("second recovery pass: %v", err) + } + assertRecoveredAlpha(t, update, step.want) + }) + t.Run(step.step+"/"+driver.name+"/removal-is-final", func(t *testing.T) { + update := plantInterruptedAlphaUpdate(t, step.step) + if err := driver.run(t, update.dir); err != nil { + t.Fatalf("%s over a recoverable state: %v", driver.name, err) + } + + if err := Remove(update.dir, "alpha"); err != nil { + t.Fatalf("remove alpha: %v", err) + } + // A later recovery reading a leftover backup would publish the removed + // skill again, and Get reads the directory rather than the lockfile, so + // the user would find it back. + if err := installSkill(t, update.dir, deltaSkill); err != nil { + t.Fatalf("install after removal: %v", err) + } + + if _, err := os.Stat(filepath.Join(update.dir, "alpha")); !os.IsNotExist(err) { + t.Errorf("a removed skill came back on disk: %v", err) + } + if _, ok := Get(update.dir, "alpha"); ok { + t.Error("a removed skill is loadable again") + } + lock, err := ReadLock(update.dir) + if err != nil { + t.Fatal(err) + } + if _, ok := lock["alpha"]; ok { + t.Error("the lockfile still names a removed skill") + } + }) + } + } +} + +// A recovery that could not be made is not the same as nothing to recover. +// Installing over or reporting the removal of a tree that is still owed a +// restore destroys the only copy of it, so the caller has to stop before it +// reads the lockfile or touches the target. +func TestCallersAbortWhenRecoveryCannotResolveAWorkspace(t *testing.T) { + injections := []struct { + name string + step string + // break renders the planted state unresolvable and returns a repair the test + // cleanup needs, so t.TempDir can still remove the tree. + breakState func(t *testing.T, update interruptedUpdate) + skipOnRoot bool + // wholeDirUntouched holds the injection to leaving every path in the skills + // dir exactly as it found it. A retirement that fails partway is the one + // exception: os.RemoveAll deletes what it can before the denied unlink, so + // the superseded backup it was clearing can be partly gone. That tree is + // already superseded by the committed target, which is why the failure is + // reported rather than acted on further. + wholeDirUntouched bool + }{ + { + // Neither tree is the one the lockfile records, so recovery cannot tell + // which one the user is owed and either guess destroys the other. + name: "neither tree matches the recorded hash", + step: "S3", + wholeDirUntouched: true, + breakState: func(t *testing.T, update interruptedUpdate) { + t.Helper() + writeSourceSkill(t, filepath.Join(update.workspace, "previous"), + "---\nname: alpha\ndescription: drifted.\n---\ndrifted alpha body\n") + }, + }, + { + // The restore is the whole point of the transaction, so a rename it + // cannot complete is the loudest failure recovery has. + name: "the backup cannot be restored", + step: "S2", + wholeDirUntouched: true, + breakState: func(t *testing.T, update interruptedUpdate) { + t.Helper() + t.Cleanup(func() { _ = os.Chmod(update.workspace, 0o755) }) + if err := os.Chmod(update.workspace, 0o555); err != nil { + t.Fatal(err) + } + }, + skipOnRoot: true, + }, + { + // A retirement that failed leaves a backup the next pass reads again, so + // it is reported rather than swallowed. + name: "the superseded workspace cannot be retired", + step: "S4", + breakState: func(t *testing.T, update interruptedUpdate) { + t.Helper() + t.Cleanup(func() { _ = os.Chmod(update.workspace, 0o755) }) + if err := os.Chmod(update.workspace, 0o555); err != nil { + t.Fatal(err) + } + }, + skipOnRoot: true, + }, + } + for _, injection := range injections { + for _, driver := range recoveryDrivers { + t.Run(injection.name+"/"+driver.name, func(t *testing.T) { + if injection.skipOnRoot { + 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") + } + } + update := plantInterruptedAlphaUpdate(t, injection.step) + injection.breakState(t, update) + target := filepath.Join(update.dir, "alpha") + lockBefore := readLockBytes(t, update.dir) + targetBefore := treeSnapshot(t, target) + dirBefore := treeSnapshot(t, update.dir) + + err := driver.run(t, update.dir) + + if err == nil { + t.Fatalf("%s continued over a transaction recovery could not resolve", driver.name) + } + assertSnapshotUnchanged(t, targetBefore, treeSnapshot(t, target)) + if got := readLockBytes(t, update.dir); got != lockBefore { + t.Errorf("the lockfile was republished by a caller that must have aborted: got %q want %q", got, lockBefore) + } + if injection.wholeDirUntouched { + assertSnapshotUnchanged(t, dirBefore, treeSnapshot(t, update.dir)) + } + }) + } + } +} + +// The workspace prefix is a public dot prefixed name and the loader enumerates +// dot prefixed directories, so a user authored skill can carry that name and +// hold the same entries a workspace does. Recovery acting on one would destroy +// installed content, so the marker is the only evidence that counts. +func TestRecoveryLeavesAUserSkillNamedLikeAWorkspaceAlone(t *testing.T) { + for _, driver := range recoveryDrivers { + t.Run(driver.name, func(t *testing.T) { + dir := t.TempDir() + gammaSource := writeSourceSkill(t, filepath.Join(t.TempDir(), "gamma"), gammaSkill) + if _, err := Install(context.Background(), InstallOptions{Source: gammaSource, Dir: dir}); err != nil { + t.Fatalf("seed gamma: %v", err) + } + notes := writeSourceSkill(t, filepath.Join(dir, ".zero-install-txn-notes"), + "---\nname: notes\ndescription: user authored.\n---\nnotes body\n") + if err := os.MkdirAll(filepath.Join(notes, "previous"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(notes, "target"), []byte("gamma"), 0o600); err != nil { + t.Fatal(err) + } + before := treeSnapshot(t, notes) + + if err := driver.run(t, dir); err != nil { + t.Fatalf("%s over a prefix colliding user skill: %v", driver.name, err) + } + + assertSnapshotUnchanged(t, before, treeSnapshot(t, notes)) + skill, ok := Get(dir, "notes") + if !ok { + t.Fatal("a user authored skill was made unloadable by recovery") + } + if skill.Description != "user authored." { + t.Errorf("the wrong tree loads as notes: %q", skill.Description) + } + }) + } +} diff --git a/internal/terminalpet/client.go b/internal/terminalpet/client.go index 53846e4af..3c9ecbd7c 100644 --- a/internal/terminalpet/client.go +++ b/internal/terminalpet/client.go @@ -290,8 +290,15 @@ 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) + // Put back anything an earlier run was killed mid-commit; see installtxn.Recover, + // and abort on a transaction it could not resolve rather than install over a + // tree that is still owed a restore. The reconciler is nil because this commit + // publishes no metadata beside the tree (the publish is a no-op), so there is + // nothing to reconcile a retained backup against and a live target is the + // committed one. + if err := installtxn.Recover(root, nil); err != nil { + return nil, err + } 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) From 005fff72c3f0035c944b94c9f2ae5fce8c927701 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Thu, 10 Sep 2026 11:31:37 -0500 Subject: [PATCH 4/6] fix(installtxn): do not read a target we could not probe as absent recoverWorkspace read any os.Lstat failure on the target as nothing being there, which enters the restore branch: the one branch that moves a tree, taken on the strength of a question that never got an answer. Only a not-exist error means the swap did not finish; every other probe failure leaves the phase unknown, so it is reported and the workspace is left as it was. The same distinction the reconcilers already draw between a missing file and a file that could not be read, applied to the probe that decides which branch recovery takes at all. --- internal/installtxn/installtxn.go | 6 ++++++ internal/installtxn/installtxn_test.go | 27 ++++++++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/internal/installtxn/installtxn.go b/internal/installtxn/installtxn.go index a5cbae824..891967e97 100644 --- a/internal/installtxn/installtxn.go +++ b/internal/installtxn/installtxn.go @@ -211,6 +211,12 @@ func Recover(dir string, reconcile Reconciler) error { // recoverWorkspace resolves one attributable workspace whose backup is present. func recoverWorkspace(workspace string, target string, backup string, name string, reconcile Reconciler) error { if _, err := os.Lstat(target); err != nil { + if !errors.Is(err, os.ErrNotExist) { + // A target we could not probe is not a target that is absent. Reading + // it as absent enters the restore branch, the one branch that moves a + // tree, on the strength of a question that never got an answer. + return fmt.Errorf("inspect install %s: %w", name, err) + } // Nothing at the target, so the swap never finished and the backup is the // only copy there is. There is no second tree to weigh it against, and // asking could only produce an answer that throws it away. diff --git a/internal/installtxn/installtxn_test.go b/internal/installtxn/installtxn_test.go index 1a3b30ebf..065927ae4 100644 --- a/internal/installtxn/installtxn_test.go +++ b/internal/installtxn/installtxn_test.go @@ -953,3 +953,30 @@ func TestRecoverKeepsAttributionWhenRetirementFailsPartway(t *testing.T) { t.Fatal("the second pass went silent on a workspace still holding a backup") } } + +// A target we could not probe is not a target that is absent. Reading a failed +// Lstat as "nothing there" sends recovery into the restore branch, which is the +// one branch that moves a tree, on the strength of a question it never got an +// answer to. The recorded name here is too long for the filesystem to resolve, +// which is the one probe failure a test can produce without a fault seam. +func TestRecoverReportsATargetItCannotProbe(t *testing.T) { + dir := t.TempDir() + name := strings.Repeat("a", 300) + workspace := plantInterruptedCommit(t, dir, "demo", name, "old") + reconciler := &recordingReconciler{phase: PhaseCommitted} + + err := Recover(dir, reconciler.reconcile) + + if err == nil { + t.Fatal("a target that could not be probed must be reported") + } + if strings.Contains(err.Error(), "restore interrupted install") { + t.Fatalf("recovery tried to restore over a target it never resolved: %v", err) + } + if !strings.Contains(err.Error(), "inspect install") { + t.Fatalf("error does not report the probe failure: %v", err) + } + if _, statErr := os.Stat(filepath.Join(workspace, "previous")); statErr != nil { + t.Fatalf("the retained backup must stay put: %v", statErr) + } +} From 9096330b50e10102e8ddcbd4af09157dd93263a9 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Thu, 10 Sep 2026 11:45:46 -0500 Subject: [PATCH 5/6] fix(installtxn): ask the reconciler about the name that resolved the path recoverableTarget trims the recorded name before resolving the path, but the raw line went to the reconciler, so the two halves of one decision ran on two different values. A marker recording "demo " resolves dir/demo and then asks the lockfile about "demo ". The lookup misses, a miss proves the publish never ran, and the committed target is replaced with the tree it superseded. Found by CodeRabbit on the round-2 push; the test observes that replacement before the fix. Also fixes the two CI failures on the new tests, neither of which was a product defect: macOS and Windows: the skills recovery matrix compared the lock source against the path the test handed the installer, while Install records canonicalSource(source). t.TempDir returns /var on macOS where the recorded value is /private/var, and an 8.3 short name on Windows where it is the long one. The expectation now comes from the same resolver the installer uses. Windows: the two plugins failure-injection tests make a directory unwritable to force a rename or removal to fail. Windows does not block either that way and root ignores the bits, so the injected failure never fired and the tests asserted against a success they never meant to produce. They now skip there, as the installtxn and skills equivalents already did. --- internal/installtxn/installtxn.go | 18 ++++++++----- internal/installtxn/installtxn_test.go | 35 ++++++++++++++++++++++++++ internal/plugins/install_test.go | 17 +++++++++++++ internal/skills/install_test.go | 8 ++++-- 4 files changed, 70 insertions(+), 8 deletions(-) diff --git a/internal/installtxn/installtxn.go b/internal/installtxn/installtxn.go index 891967e97..a419a83ed 100644 --- a/internal/installtxn/installtxn.go +++ b/internal/installtxn/installtxn.go @@ -187,7 +187,7 @@ func Recover(dir string, reconcile Reconciler) error { if !ok { continue } - target, ok := recoverableTarget(dir, name) + name, target, ok := recoverableTarget(dir, name) if !ok { continue } @@ -327,20 +327,26 @@ func markerTarget(workspace string) (string, bool) { } // 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 +// dir, and returns the normalized name alongside it. Both come back because the +// caller needs them to agree: the path is resolved from the trimmed name, so +// asking the reconciler about the raw one would split the decision across two +// values, and a lookup that missed on the untrimmed name would read as a publish +// that never ran and replace the committed target with the tree it superseded. +// +// A name that is not a single path element could name anything on the // filesystem, so it is refused rather than restored over. A name carrying the // workspace prefix is refused for the same reason: it names another // transaction, not an install, and restoring over one that is still in flight // would destroy it. -func recoverableTarget(dir string, name string) (string, bool) { +func recoverableTarget(dir string, name string) (string, string, bool) { name = strings.TrimSpace(name) if name == "" || name == "." || name == ".." || name != filepath.Base(name) { - return "", false + return "", "", false } if strings.HasPrefix(name, workspacePrefix) { - return "", false + return "", "", false } - return filepath.Join(dir, name), true + return name, filepath.Join(dir, name), true } func rollback(target string, backup string, hadPrevious bool, cause error) error { diff --git a/internal/installtxn/installtxn_test.go b/internal/installtxn/installtxn_test.go index 065927ae4..c5ab5a6ef 100644 --- a/internal/installtxn/installtxn_test.go +++ b/internal/installtxn/installtxn_test.go @@ -980,3 +980,38 @@ func TestRecoverReportsATargetItCannotProbe(t *testing.T) { t.Fatalf("the retained backup must stay put: %v", statErr) } } + +// The recorded name resolves the path after normalization, so it must be the +// same name the reconciler is asked about. Handing the reconciler the raw line +// while resolving the path from the trimmed one splits the decision across two +// values: the lookup misses, the miss reads as a publish that never ran, and the +// committed target is replaced with the tree it superseded. The reconciler here +// answers the way a lockfile does, by name. +func TestRecoverAsksTheReconcilerAboutTheNormalizedName(t *testing.T) { + dir := t.TempDir() + workspace := plantPublishedCommit(t, dir, "demo", "demo", "new", "old") + writeMarker(t, workspace, markerMagic+"\ntarget demo \n") + var asked string + reconcile := func(name string, target string, backup string) (Phase, error) { + asked = name + if name != "demo" { + return PhasePrePublish, nil + } + return PhaseCommitted, nil + } + + if err := Recover(dir, reconcile); err != nil { + t.Fatalf("Recover: %v", err) + } + + if asked != "demo" { + t.Errorf("reconciler asked about %q, want the normalized %q", asked, "demo") + } + data, err := os.ReadFile(filepath.Join(dir, "demo", "version")) + if err != nil { + t.Fatalf("read the live install: %v", err) + } + if string(data) != "new" { + t.Fatalf("the committed install was replaced with %q", data) + } +} diff --git a/internal/plugins/install_test.go b/internal/plugins/install_test.go index 55da42778..310b8c6b8 100644 --- a/internal/plugins/install_test.go +++ b/internal/plugins/install_test.go @@ -8,6 +8,7 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "testing" "github.com/Gitlawb/zero/internal/installtxn" @@ -973,7 +974,22 @@ func TestInterruptedUpdateWithAnUnreadableLockAbortsBothCallers(t *testing.T) { // A restore that cannot be carried out is the case where silence is worst: the // plugin is missing from the target and its only copy is in a workspace nothing // else reads. +// skipWithoutPermissionFaults skips a test that injects a failure by making a +// directory unwritable. Windows does not block renames or deletes that way, and +// root ignores the permission entirely, so on both the injected failure never +// fires and the test would assert against a success it never meant to produce. +func skipWithoutPermissionFaults(t *testing.T) { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("directory permissions do not block renames or removals on windows") + } + if os.Geteuid() == 0 { + t.Skip("root ignores the directory permissions this test relies on") + } +} + func TestAFailedRestoreAbortsBothCallers(t *testing.T) { + skipWithoutPermissionFaults(t) u := seedInterruptedUpdate(t) workspace := u.plantWorkspace(t) u.mustRename(t, u.target(), filepath.Join(workspace, "previous")) @@ -1075,6 +1091,7 @@ func interruptedCallers() []interruptedCaller { // ownership marker with it and leave the next pass a workspace it cannot // attribute. func TestAFailedRetirementAbortsBothCallers(t *testing.T) { + skipWithoutPermissionFaults(t) for _, caller := range interruptedCallers() { t.Run(caller.name, func(t *testing.T) { u := seedInterruptedUpdate(t) diff --git a/internal/skills/install_test.go b/internal/skills/install_test.go index 1d470212e..3f766aa11 100644 --- a/internal/skills/install_test.go +++ b/internal/skills/install_test.go @@ -604,7 +604,7 @@ func plantInterruptedAlphaUpdate(t *testing.T, step string) interruptedUpdate { case "S4": rename(target, previous) rename(staged, target) - publishLockEntry(t, dir, "alpha", LockEntry{Source: newSource, Hash: hashContent([]byte(newAlphaSkill))}) + publishLockEntry(t, dir, "alpha", LockEntry{Source: canonicalSource(newSource), Hash: hashContent([]byte(newAlphaSkill))}) case "S5": // An interrupted rollback: the failed tree was set aside and the backup is // still the only complete copy. @@ -616,7 +616,11 @@ func plantInterruptedAlphaUpdate(t *testing.T, step string) interruptedUpdate { default: t.Fatalf("unknown step %q", step) } - return interruptedUpdate{dir: dir, workspace: workspace, oldSource: oldSource, newSource: newSource} + // Compare against the source the installer records, not the path the test + // handed it. Install stores canonicalSource(source), so on macOS the recorded + // value is /private/var where t.TempDir returns /var, and on Windows it is the + // long user name where t.TempDir returns the 8.3 short one. + return interruptedUpdate{dir: dir, workspace: workspace, oldSource: canonicalSource(oldSource), newSource: canonicalSource(newSource)} } func publishLockEntry(t *testing.T, dir string, name string, entry LockEntry) { From a856de553816a672623d5e0dd2eae824e95315c0 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Thu, 10 Sep 2026 13:19:47 -0500 Subject: [PATCH 6/6] fix(installtxn): stop recovery acting on trees the lockfile never described An adversarial pass over the recovery protocol, driven end to end through the built binary rather than package tests, found that the protocol still acted on evidence it did not have. The one that loses data: a missing lock entry was read as proof the publish never ran, so recovery replaced whatever was at the target with the retained backup. A missing entry proves no such thing. The entry can be lost after a publish that did run, since a truncated or deleted lockfile reads as an empty one, and a directory with no entry is a state these packages already support (Load returns it, Remove handles it). Worse, the tree at the target may have nothing to do with the transaction: `zero tools make foo` scaffolds into the plugins root, and a hand-written skill directory is an ordinary thing to find in the skills root. Both were destroyed and replaced by a stale backup, and the install reported success. Reproduced through the CLI both ways before the fix. A missing entry is now PhaseUnknown, which stops every caller and touches nothing. The rest are the same class as the target probe fixed in 005fff72, a probe whose failure was read as a fact: - A marker that could not be read was skipped as somebody else's content, so the only copy of an install stayed stranded while the caller was told there was nothing to do. Present but unreadable is now reported. - The retained backup was probed with Stat, which follows symlinks and never checked the type, so a symlink there published content from outside the install root and a regular file published a file where a directory belongs. Only a directory is a tree this package set aside. - An entry at the target was read as an install at the target. A dangling symlink satisfies Lstat, and treating it as live retired the only real copy. - Two workspaces claiming one install were acted on in turn, which published one over the other and deleted the rest. Nothing ranks two claims, so both are reported. - A stale `failed` tree from an earlier rollback made the restore rename fail for good, and since every caller aborts on a recovery error that wedged every install and removal in the root. The workspace is proven ours, so the stale tree is cleared first. - A recorded name carrying a NUL or longer than a path element can be reached Lstat and came back with an error that is not not-exist, reported on every pass forever with no way for a caller to clear it. Such a name is not an install to recover and is skipped. Unresolved-transaction errors now name the workspace directory, because the only remedy is to delete it by hand and nothing in the CLI can reach it. Left alone deliberately: an interrupted RemoveDir still writes no marker, which the PR body already discloses as a pre-existing residual. --- internal/installtxn/installtxn.go | 101 +++++++++++-- internal/installtxn/installtxn_test.go | 202 +++++++++++++++++++++---- internal/plugins/install.go | 10 +- internal/plugins/install_test.go | 43 +++++- internal/skills/install.go | 9 +- internal/skills/install_test.go | 38 +++++ 6 files changed, 350 insertions(+), 53 deletions(-) diff --git a/internal/installtxn/installtxn.go b/internal/installtxn/installtxn.go index a419a83ed..b719c0991 100644 --- a/internal/installtxn/installtxn.go +++ b/internal/installtxn/installtxn.go @@ -178,30 +178,66 @@ func Recover(dir string, reconcile Reconciler) error { return fmt.Errorf("enumerate install workspaces: %w", err) } var unresolved []error + // Gather first, act second. A pass that acts as it walks cannot see that two + // workspaces claim one install, and acting on both in turn publishes one over + // the other and deletes the rest. + type claim struct { + workspace string + name string + target string + backup string + } + var claims []claim + claimants := map[string]int{} for _, entry := range entries { if !entry.IsDir() || !strings.HasPrefix(entry.Name(), workspacePrefix) { continue } workspace := filepath.Join(dir, entry.Name()) - name, ok := markerTarget(workspace) + recorded, ok, err := markerTarget(workspace) + if err != nil { + // A marker we could not read is not a marker that is absent. Skipping + // silently would tell the caller there was nothing to do while the only + // copy of an install stays in the workspace. + unresolved = append(unresolved, fmt.Errorf("read the transaction marker in %s: %w", workspace, err)) + continue + } if !ok { continue } - name, target, ok := recoverableTarget(dir, name) + name, target, ok := recoverableTarget(dir, recorded) if !ok { continue } backup := filepath.Join(workspace, "previous") - if _, err := os.Stat(backup); err != nil { + info, err := os.Lstat(backup) + if err != nil { if errors.Is(err, os.ErrNotExist) { // Mid-transaction: the trees never moved, so there is nothing to // put back and the workspace belongs to whoever made it. continue } - unresolved = append(unresolved, fmt.Errorf("inspect retained install %s: %w", name, err)) + unresolved = append(unresolved, fmt.Errorf("inspect the retained install in %s: %w", workspace, err)) continue } - if err := recoverWorkspace(workspace, target, backup, name, reconcile); err != nil { + if !info.IsDir() { + // Only a directory is something this package set aside. Renaming a + // symlink into the install root publishes whatever it points at, from + // anywhere on the filesystem. + unresolved = append(unresolved, fmt.Errorf("the retained install in %s is not a directory, so recovery will not publish it", workspace)) + continue + } + claims = append(claims, claim{workspace: workspace, name: name, target: target, backup: backup}) + claimants[name]++ + } + for _, c := range claims { + if claimants[c.name] > 1 { + // Nothing ranks two claims on one install, and publishing them in turn + // destroys every tree but the last. + unresolved = append(unresolved, fmt.Errorf("install %s is claimed by more than one interrupted transaction, including %s: resolve them by hand", c.name, c.workspace)) + continue + } + if err := recoverWorkspace(c.workspace, c.target, c.backup, c.name, reconcile); err != nil { unresolved = append(unresolved, err) } } @@ -210,11 +246,16 @@ func Recover(dir string, reconcile Reconciler) error { // recoverWorkspace resolves one attributable workspace whose backup is present. func recoverWorkspace(workspace string, target string, backup string, name string, reconcile Reconciler) error { - if _, err := os.Lstat(target); err != nil { + info, err := os.Lstat(target) + if err != nil { if !errors.Is(err, os.ErrNotExist) { // A target we could not probe is not a target that is absent. Reading // it as absent enters the restore branch, the one branch that moves a - // tree, on the strength of a question that never got an answer. + // tree, on the strength of a question that never got an answer. No test + // injects this on Linux: the names that used to reach here and fail are + // now refused by recoverableTarget, and a permission fault on the root + // stops at the marker read above. It is kept for the platforms that can + // still fail this call, Windows reserved device names among them. return fmt.Errorf("inspect install %s: %w", name, err) } // Nothing at the target, so the swap never finished and the backup is the @@ -226,6 +267,12 @@ func recoverWorkspace(workspace string, target string, backup string, name strin cleanupWorkspace(workspace) return nil } + if !info.IsDir() { + // An entry at the target is not an install at the target. A dangling + // symlink satisfies Lstat, and reading it as a live install retires the + // backup beside it, which is the only real copy there is. + return fmt.Errorf("install %s is not a directory, so recovery cannot tell what it is: resolve %s by hand", name, workspace) + } phase := PhaseCommitted if reconcile != nil { classified, err := reconcile(name, target, backup) @@ -266,7 +313,7 @@ func recoverWorkspace(workspace string, target string, backup string, name strin default: // Neither tree is the one the metadata records, so recovery cannot tell // which one the user is owed and either guess risks destroying the other. - return fmt.Errorf("interrupted install %s matches neither the live target nor the retained backup", name) + return fmt.Errorf("interrupted install %s matches neither the live target nor the retained backup: resolve %s by hand", name, workspace) } } @@ -281,6 +328,13 @@ func restoreOverTarget(workspace string, target string, backup string, name stri // order every instant has either a whole tree at the target or nothing there // and the backup intact, which is exactly what this pass can tell apart. failed := filepath.Join(workspace, "failed") + // An earlier rollback can have left a tree here. The workspace is proven ours + // by its marker, so clearing it is safe, and leaving it is not: the rename + // below fails for as long as it is there, and because every caller aborts on a + // recovery error that wedges every install and removal in this root. + if err := os.RemoveAll(failed); err != nil { + return fmt.Errorf("clear the superseded install set aside in %s: %w", workspace, err) + } if err := os.Rename(target, failed); err != nil { return fmt.Errorf("set aside superseded install %s: %w", name, err) } @@ -301,29 +355,37 @@ func restoreOverTarget(workspace string, target string, backup string, name stri // workspace does. Only the magic and version first line is evidence no ordinary // content produces by accident, so anything else is somebody's content and is // left alone. -func markerTarget(workspace string) (string, bool) { +func markerTarget(workspace string) (string, bool, error) { path := filepath.Join(workspace, markerFileName) info, err := os.Lstat(path) - if err != nil || !info.Mode().IsRegular() { - return "", false + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return "", false, nil + } + return "", false, err + } + if !info.Mode().IsRegular() { + return "", false, nil } data, err := os.ReadFile(path) if err != nil { - return "", false + // Present but unreadable is not absent, and the difference decides whether + // a retained tree is put back or stranded where nothing reads it. + return "", false, err } magic, rest, ok := strings.Cut(string(data), "\n") if !ok || magic != markerMagic { - return "", false + return "", false, nil } line, _, ok := strings.Cut(rest, "\n") if !ok { - return "", false + return "", false, nil } name, ok := strings.CutPrefix(line, "target ") if !ok { - return "", false + return "", false, nil } - return name, true + return name, true, nil } // recoverableTarget resolves a recorded target name to a path directly inside @@ -346,6 +408,13 @@ func recoverableTarget(dir string, name string) (string, string, bool) { if strings.HasPrefix(name, workspacePrefix) { return "", "", false } + // A name no filesystem can resolve is not an install to recover. Left to reach + // Lstat it answers with an error that is not not-exist, which would be reported + // as an unresolved transaction on every pass forever, with no way for a caller + // to clear it. + if strings.ContainsRune(name, 0) || len(name) > 255 { + return "", "", false + } return name, filepath.Join(dir, name), true } diff --git a/internal/installtxn/installtxn_test.go b/internal/installtxn/installtxn_test.go index c5ab5a6ef..28ceef9c7 100644 --- a/internal/installtxn/installtxn_test.go +++ b/internal/installtxn/installtxn_test.go @@ -2,6 +2,7 @@ package installtxn import ( "errors" + "fmt" "os" "path/filepath" "runtime" @@ -954,33 +955,6 @@ func TestRecoverKeepsAttributionWhenRetirementFailsPartway(t *testing.T) { } } -// A target we could not probe is not a target that is absent. Reading a failed -// Lstat as "nothing there" sends recovery into the restore branch, which is the -// one branch that moves a tree, on the strength of a question it never got an -// answer to. The recorded name here is too long for the filesystem to resolve, -// which is the one probe failure a test can produce without a fault seam. -func TestRecoverReportsATargetItCannotProbe(t *testing.T) { - dir := t.TempDir() - name := strings.Repeat("a", 300) - workspace := plantInterruptedCommit(t, dir, "demo", name, "old") - reconciler := &recordingReconciler{phase: PhaseCommitted} - - err := Recover(dir, reconciler.reconcile) - - if err == nil { - t.Fatal("a target that could not be probed must be reported") - } - if strings.Contains(err.Error(), "restore interrupted install") { - t.Fatalf("recovery tried to restore over a target it never resolved: %v", err) - } - if !strings.Contains(err.Error(), "inspect install") { - t.Fatalf("error does not report the probe failure: %v", err) - } - if _, statErr := os.Stat(filepath.Join(workspace, "previous")); statErr != nil { - t.Fatalf("the retained backup must stay put: %v", statErr) - } -} - // The recorded name resolves the path after normalization, so it must be the // same name the reconciler is asked about. Handing the reconciler the raw line // while resolving the path from the trimmed one splits the decision across two @@ -1015,3 +989,177 @@ func TestRecoverAsksTheReconcilerAboutTheNormalizedName(t *testing.T) { t.Fatalf("the committed install was replaced with %q", data) } } + +// A marker we could not read is not a marker that is absent. Skipping silently +// on a read error tells the caller there was nothing to do while the only copy +// of the install stays stranded in the workspace, which is the fail-open half of +// the distinction the target probe already draws. +func TestRecoverReportsAMarkerItCannotRead(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("file permissions do not block reads on windows") + } + if os.Geteuid() == 0 { + t.Skip("root ignores the file permissions this test relies on") + } + dir := t.TempDir() + workspace := plantInterruptedCommit(t, dir, "demo", "demo", "old") + marker := filepath.Join(workspace, markerFileName) + t.Cleanup(func() { _ = os.Chmod(marker, 0o600) }) + if err := os.Chmod(marker, 0o000); err != nil { + t.Fatal(err) + } + + err := Recover(dir, nil) + + if err == nil { + t.Fatal("a marker that could not be read must be reported, not skipped") + } + if _, statErr := os.Stat(filepath.Join(workspace, "previous")); statErr != nil { + t.Fatalf("the stranded copy must be left alone: %v", statErr) + } +} + +// The retained backup is a directory this package created. A symlink or a file +// standing in its place is not something recovery may publish: renaming a +// symlink into the install root installs whatever it points at, from anywhere on +// the filesystem. +func TestRecoverRefusesABackupThatIsNotADirectory(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation needs privileges on windows") + } + outside := t.TempDir() + if err := os.WriteFile(filepath.Join(outside, "payload"), []byte("elsewhere"), 0o644); err != nil { + t.Fatal(err) + } + for _, plant := range []struct { + name string + make func(t *testing.T, backup string) + }{ + {"symlink to a directory outside the root", func(t *testing.T, backup string) { + if err := os.Symlink(outside, backup); err != nil { + t.Fatal(err) + } + }}, + {"regular file", func(t *testing.T, backup string) { + if err := os.WriteFile(backup, []byte("not a tree"), 0o644); err != nil { + t.Fatal(err) + } + }}, + } { + t.Run(plant.name, func(t *testing.T) { + dir := t.TempDir() + staged, _, err := StageDir(dir) + if err != nil { + t.Fatal(err) + } + workspace := filepath.Dir(staged) + writeMarker(t, workspace, markerMagic+"\ntarget demo\n") + plant.make(t, filepath.Join(workspace, "previous")) + + if err := Recover(dir, nil); err == nil { + t.Fatal("a backup that is not a directory must be reported, not published") + } + if _, err := os.Lstat(filepath.Join(dir, "demo")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("nothing may be published at the target: %v", err) + } + }) + } +} + +// An entry at the target is not the same as an install at the target. A dangling +// symlink satisfies Lstat, and reading it as a live install retires the backup +// beside it, which is the only real copy there is. +func TestRecoverRefusesATargetThatIsNotADirectory(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation needs privileges on windows") + } + dir := t.TempDir() + workspace := plantInterruptedCommit(t, dir, "demo", "demo", "old") + if err := os.Symlink(filepath.Join(dir, "gone"), filepath.Join(dir, "demo")); err != nil { + t.Fatal(err) + } + + err := Recover(dir, nil) + + if err == nil { + t.Fatal("a target that is not a directory must be reported") + } + data, readErr := os.ReadFile(filepath.Join(workspace, "previous", "version")) + if readErr != nil { + t.Fatalf("the retained copy must survive: %v", readErr) + } + if string(data) != "old" { + t.Fatalf("retained copy = %q, want old", data) + } +} + +// A workspace of ours can hold a failed tree from an earlier rollback. Renaming +// the live target onto it fails for as long as it is there, and because every +// caller aborts on a recovery error, that wedges every install and removal in +// the root. The workspace is proven ours, so the stale tree is cleared. +func TestRecoverClearsAStaleFailedTreeBeforeRestoring(t *testing.T) { + dir := t.TempDir() + workspace := plantPublishedCommit(t, dir, "demo", "demo", "new", "old") + failed := filepath.Join(workspace, "failed") + if err := os.MkdirAll(failed, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(failed, "leftover"), []byte("stale"), 0o644); err != nil { + t.Fatal(err) + } + reconciler := &recordingReconciler{phase: PhasePrePublish} + + if err := Recover(dir, reconciler.reconcile); err != nil { + t.Fatalf("a stale failed tree must not wedge recovery: %v", err) + } + + data, err := os.ReadFile(filepath.Join(dir, "demo", "version")) + if err != nil { + t.Fatalf("read the restored install: %v", err) + } + if string(data) != "old" { + t.Fatalf("restored content = %q, want old", data) + } +} + +// A recorded name that cannot be a sane path element is not ours to act on. It +// used to reach Lstat, which answers with an error that is not not-exist, so the +// workspace reported an error no caller could ever clear. +func TestRecoverSkipsAMarkerNamingAnImpossibleTarget(t *testing.T) { + for _, name := range []string{"de\x00mo", strings.Repeat("a", 300)} { + t.Run(fmt.Sprintf("%q", name), func(t *testing.T) { + dir := t.TempDir() + workspace := plantInterruptedCommit(t, dir, "demo", name, "old") + + if err := Recover(dir, nil); err != nil { + t.Fatalf("an unusable recorded name must be skipped, not reported forever: %v", err) + } + if _, err := os.Stat(filepath.Join(workspace, "previous")); err != nil { + t.Fatalf("the workspace must be left intact: %v", err) + } + }) + } +} + +// Two workspaces claiming one install cannot both be right, and acting on them +// in turn publishes one over the other and deletes the rest. Recovery has no way +// to rank them, so it reports and touches nothing. +func TestRecoverRefusesTwoWorkspacesNamingOneTarget(t *testing.T) { + dir := t.TempDir() + first := plantInterruptedCommit(t, dir, "demo", "demo", "first") + second := plantInterruptedCommit(t, dir, "demo", "demo", "second") + + err := Recover(dir, nil) + + if err == nil { + t.Fatal("two workspaces naming one target must be reported") + } + for _, workspace := range []string{first, second} { + if _, statErr := os.Stat(filepath.Join(workspace, "previous")); statErr != nil { + t.Errorf("every retained copy must survive: %v", statErr) + } + } + if _, statErr := os.Lstat(filepath.Join(dir, "demo")); !errors.Is(statErr, os.ErrNotExist) { + t.Errorf("nothing may be published while the claim is ambiguous: %v", statErr) + } +} diff --git a/internal/plugins/install.go b/internal/plugins/install.go index ebe9b92e8..a180f679a 100644 --- a/internal/plugins/install.go +++ b/internal/plugins/install.go @@ -266,9 +266,13 @@ func reconcileInterrupted(dir string) installtxn.Reconciler { } entry, ok := lock[id] if !ok { - // Every publish writes the entry, so a missing one proves the publish - // never ran and the backup is still the tree the lockfile describes. - return installtxn.PhasePrePublish, nil + // A missing entry is not proof that the publish never ran. The entry can + // also be lost after a publish that did run: a truncated or deleted + // lockfile reads as an empty one, and a directory with no entry is a + // state this package supports (Load returns it, Remove handles it). The + // live tree here may be nothing to do with this transaction at all, so + // there is no phase to report and recovery must not pick between them. + return installtxn.PhaseUnknown, nil } if entry.Hash == "" { // A hand edited entry records no tree at all, so there is nothing to diff --git a/internal/plugins/install_test.go b/internal/plugins/install_test.go index 310b8c6b8..a4be8b559 100644 --- a/internal/plugins/install_test.go +++ b/internal/plugins/install_test.go @@ -1027,7 +1027,7 @@ func TestAFailedRestoreAbortsBothCallers(t *testing.T) { // A publish always writes the lockfile entry, so an interrupted update with no // entry naming it proves the publish never ran and the backup is still the tree // the lockfile describes, even though there is no hash left to match it against. -func TestInterruptedUpdateWithNoLockEntryRestoresTheBackup(t *testing.T) { +func TestInterruptedUpdateWithNoLockEntryAbortsTheCaller(t *testing.T) { u := seedInterruptedUpdate(t) workspace := u.plantWorkspace(t) u.mustRename(t, u.target(), filepath.Join(workspace, "previous")) @@ -1040,11 +1040,46 @@ func TestInterruptedUpdateWithNoLockEntryRestoresTheBackup(t *testing.T) { if err := writeLock(u.dir, lock); err != nil { t.Fatal(err) } + err = installOther(t, u.dir, "zero.other") - if err := installOther(t, u.dir, "zero.other"); err != nil { - t.Fatalf("install after the interrupted update: %v", err) + if err == nil { + t.Fatal("a live tree the lockfile does not describe must stop the caller") + } + assertUntouched(t, u, "new", LockEntry{}) +} + +// A directory at the target that the lockfile does not name is not proof that a +// publish was interrupted. Anything can have created it: `zero tools make`, a +// hand-written plugin, a restored file sync. Recovery used to read a missing +// entry as proof the publish never ran and replace that tree with the retained +// backup, which deleted whatever was there. +func TestAnUnrelatedTreeAtTheTargetIsNeverReplaced(t *testing.T) { + dir := t.TempDir() + workspace, err := os.MkdirTemp(dir, ".zero-install-txn-") + if err != nil { + t.Fatal(err) + } + marker := []byte("zero-install-txn v1\ntarget zero.demo\n") + if err := os.WriteFile(filepath.Join(workspace, ".zero-install-txn"), marker, 0o600); err != nil { + t.Fatal(err) + } + backup := filepath.Join(workspace, "previous") + writeSourcePlugin(t, backup, validManifest()) + // The user's own tree, which no lockfile entry names. + mine := filepath.Join(dir, "zero.demo") + writeSourcePlugin(t, mine, validManifest()) + if err := os.WriteFile(filepath.Join(mine, "mycode.txt"), []byte("my work"), 0o644); err != nil { + t.Fatal(err) + } + + err = installOther(t, dir, "zero.other") + + if err == nil { + t.Fatal("recovery must not act on a tree the lockfile does not describe") + } + if _, statErr := os.Stat(filepath.Join(mine, "mycode.txt")); statErr != nil { + t.Fatalf("the user's own tree was destroyed: %v", statErr) } - assertRecovered(t, u, workspace, recoveryWant{generation: "old", version: "0.1.0"}) } // An entry that records no hash describes no tree, so neither side can be diff --git a/internal/skills/install.go b/internal/skills/install.go index 0169a6dba..23f5a78ab 100644 --- a/internal/skills/install.go +++ b/internal/skills/install.go @@ -203,9 +203,12 @@ func lockReconciler(dir string) installtxn.Reconciler { } entry, locked := lock[name] if !locked { - // The publish writes the entry for every install, so no entry at all - // proves the publish never ran and the backup is what the user had. - return installtxn.PhasePrePublish, nil + // A missing entry is not proof that the publish never ran. The entry can + // also be lost after a publish that did run: a truncated or deleted + // lockfile reads as an empty one. A hand-written skill the lockfile never + // named is an ordinary thing to find here too, and replacing it with the + // retained backup would delete the user's own work. + return installtxn.PhaseUnknown, nil } if entry.Hash == "" { // Nothing to compare against, so neither tree can be shown to be the diff --git a/internal/skills/install_test.go b/internal/skills/install_test.go index 3f766aa11..412b30911 100644 --- a/internal/skills/install_test.go +++ b/internal/skills/install_test.go @@ -995,3 +995,41 @@ func TestRecoveryLeavesAUserSkillNamedLikeAWorkspaceAlone(t *testing.T) { }) } } + +// A directory at the target that the lockfile does not name is not proof that a +// publish was interrupted. Anything can have created it, and a hand-written +// skill is an ordinary thing to find in the skills directory. Recovery used to +// read a missing entry as proof the publish never ran and replace that tree with +// the retained backup, which deleted the user's own work. +func TestAnUnrelatedSkillAtTheTargetIsNeverReplaced(t *testing.T) { + dir := t.TempDir() + workspace, err := os.MkdirTemp(dir, ".zero-install-txn-") + if err != nil { + t.Fatal(err) + } + marker := []byte("zero-install-txn v1\ntarget notes\n") + if err := os.WriteFile(filepath.Join(workspace, ".zero-install-txn"), marker, 0o600); err != nil { + t.Fatal(err) + } + writeSourceSkill(t, filepath.Join(workspace, "previous"), + "---\nname: notes\ndescription: stale backup.\n---\nstale\n") + // The user's own skill, which no lockfile entry names. + mine := filepath.Join(dir, "notes") + writeSourceSkill(t, mine, "---\nname: notes\ndescription: my own.\n---\nmy careful notes\n") + if err := os.WriteFile(filepath.Join(mine, "research.md"), []byte("my research"), 0o644); err != nil { + t.Fatal(err) + } + + err = installSkill(t, dir, deltaSkill) + + if err == nil { + t.Fatal("recovery must not act on a tree the lockfile does not describe") + } + if _, statErr := os.Stat(filepath.Join(mine, "research.md")); statErr != nil { + t.Fatalf("the user's own skill was destroyed: %v", statErr) + } + data, readErr := os.ReadFile(filepath.Join(mine, skillFileName)) + if readErr != nil || !strings.Contains(string(data), "my own") { + t.Fatalf("the user's own SKILL.md was replaced: %q %v", data, readErr) + } +}