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/2] 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/2] 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") + } +}