diff --git a/internal/installtxn/installtxn.go b/internal/installtxn/installtxn.go index da4f2ad45..b719c0991 100644 --- a/internal/installtxn/installtxn.go +++ b/internal/installtxn/installtxn.go @@ -8,10 +8,27 @@ 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-" + +// 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 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. func Lock(dir string) (func(), error) { @@ -28,7 +45,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 +62,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, 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 { return fmt.Errorf("retain previous install: %w", err) } @@ -76,7 +99,7 @@ func CommitDir(target string, staged string, publish func() error) error { // // The caller must hold the install-root lock returned by Lock. func RemoveDir(target string, publish func() error) error { - workspace, err := os.MkdirTemp(filepath.Dir(target), ".zero-install-txn-") + workspace, err := os.MkdirTemp(filepath.Dir(target), workspacePrefix) if err != nil { return fmt.Errorf("create removal staging dir: %w", err) } @@ -95,15 +118,339 @@ func RemoveDir(target string, publish func() error) error { return nil } -func rollback(target string, backup string, hadPrevious bool, cause error) error { - if err := os.RemoveAll(target); err != nil { - return errors.Join(cause, fmt.Errorf("remove failed install: %w", err)) +// 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 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 { + // 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) } - if hadPrevious { + 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()) + 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, recorded) + if !ok { + continue + } + backup := filepath.Join(workspace, "previous") + 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 the retained install in %s: %w", workspace, err)) + continue + } + 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) + } + } + 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 { + 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. 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 + // 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 { - return errors.Join(cause, fmt.Errorf("restore previous install: %w", err)) + return fmt.Errorf("restore interrupted install %s: %w", name, err) + } + 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) + 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: resolve %s by hand", name, workspace) + } +} + +// 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") + // 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) + } + 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, error) { + path := filepath.Join(workspace, markerFileName) + info, err := os.Lstat(path) + 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 { + // 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, nil + } + line, _, ok := strings.Cut(rest, "\n") + if !ok { + return "", false, nil + } + name, ok := strings.CutPrefix(line, "target ") + if !ok { + return "", false, nil + } + return name, true, nil +} + +// recoverableTarget resolves a recorded target name to a path directly inside +// 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, string, bool) { + name = strings.TrimSpace(name) + if name == "" || name == "." || name == ".." || name != filepath.Base(name) { + return "", "", false + } + 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 +} + +func rollback(target string, backup string, hadPrevious bool, cause error) error { + if !hadPrevious { + // A first install has no backup to protect and recorded no target, so + // recovery never looks here and deleting in place costs nothing. + if err := os.RemoveAll(target); err != nil { + return errors.Join(cause, fmt.Errorf("remove failed install: %w", err)) + } + return cause + } + // Move the failed install aside before restoring rather than deleting it in + // place. A process killed partway through an in-place delete would leave a + // half removed tree at the target while the backup was still the only + // complete copy, and recovery reads a target that is there as a committed + // publish and retires the backup beside it. With the renames in this order + // every instant of the rollback has either a whole tree at the target or + // nothing there and the backup intact, which is exactly what recovery's two + // branches are able to tell apart. + failed := filepath.Join(filepath.Dir(backup), "failed") + if err := os.Rename(target, failed); err != nil { + // The move aside can fail too, and then the failed install stays live at + // the target while the backup is still the only copy of what it replaced. + // 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 { + return errors.Join(cause, fmt.Errorf("restore previous install: %w", err)) } + _ = os.RemoveAll(failed) return cause } diff --git a/internal/installtxn/installtxn_test.go b/internal/installtxn/installtxn_test.go index fdd27e4c9..28ceef9c7 100644 --- a/internal/installtxn/installtxn_test.go +++ b/internal/installtxn/installtxn_test.go @@ -2,8 +2,11 @@ package installtxn import ( "errors" + "fmt" "os" "path/filepath" + "runtime" + "strings" "testing" ) @@ -76,3 +79,1087 @@ 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, markerFileName)) + 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 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) + } +} + +// 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) + writeMarker(t, workspace, "zero-install-txn v1\ntarget "+recorded+"\n") + 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") + + 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" { + 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 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"}, + } { + 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) + } + } + + if err := Recover(dir, nil); err != nil { + t.Fatalf("Recover: %v", err) + } + + data, err := os.ReadFile(filepath.Join(live, "version")) + if tc.live == "" { + if err == nil { + t.Fatalf("an existing install was replaced by a backup: version = %q", data) + } + } else if err != nil || string(data) != tc.live { + t.Fatalf("the live install must win: got %q err %v", data, err) + } + if _, err := os.Stat(workspace); !os.IsNotExist(err) { + t.Errorf("the superseded workspace should be retired, got %v", err) + } + }) + } +} + +// The recorded target names a directory inside the install root and nothing +// else. A name that could resolve anywhere is refused, not restored over. +func TestRecoverRefusesATargetOutsideTheInstallRoot(t *testing.T) { + for _, recorded := range []string{"..", ".", "", " ", "../escape", "a/b", string(filepath.Separator) + "etc"} { + t.Run(recorded, func(t *testing.T) { + root := t.TempDir() + dir := filepath.Join(root, "installs") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + outside := filepath.Join(root, "escape") + workspace := plantInterruptedCommit(t, dir, "demo", recorded, "old") + + 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) + } + 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, markerFileName)); err != nil { + t.Fatal(err) + } + + 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) + } + 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) + } +} + +// 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, 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, "target"), []byte("elsewhere"), 0o600); err != nil { + t.Fatal(err) + } + + 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) + } + if _, err := os.Stat(filepath.Join(dir, "elsewhere")); !os.IsNotExist(err) { + t.Errorf("recovery published from a directory that is not its workspace: %v", err) + } +} + +// plantPublishedCommit builds what a process killed after CommitDir's second +// rename leaves in dir: the replacement live at the target, and the tree it +// replaced still sitting in the workspace beside it. +func plantPublishedCommit(t *testing.T, dir, name, recorded, published, superseded string) string { + t.Helper() + workspace := plantInterruptedCommit(t, dir, name, recorded, superseded) + target := filepath.Join(dir, name) + if err := os.MkdirAll(target, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(target, "version"), []byte(published), 0o644); err != nil { + t.Fatal(err) + } + return workspace +} + +// A backup left beside a target that the publish rename already replaced is +// superseded, not pending. Keeping it made a removal reversible by accident: +// the removal deleted the live target, and the next recovery then read the +// absent target as an interrupted swap and published the stale tree again. +func TestRecoverRetiresABackupASuccessfulPublishSuperseded(t *testing.T) { + dir := t.TempDir() + workspace := plantPublishedCommit(t, dir, "demo", "demo", "new", "old") + target := filepath.Join(dir, "demo") + + 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" { + 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) + } + 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) + } +} + +// 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) + } + + 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" { + 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) + } + + 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" { + 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) + } +} + +// 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") + } +} + +// 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) + } +} + +// 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 1335b67be..a180f679a 100644 --- a/internal/plugins/install.go +++ b/internal/plugins/install.go @@ -153,6 +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, 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. @@ -205,6 +211,13 @@ func Remove(dir string, id string) error { return err } defer unlock() + // 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 { @@ -236,6 +249,70 @@ 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 { + // 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 + // 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 ce1b4b597..a4be8b559 100644 --- a/internal/plugins/install_test.go +++ b/internal/plugins/install_test.go @@ -8,7 +8,10 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "testing" + + "github.com/Gitlawb/zero/internal/installtxn" ) // initGitPluginRepo creates a real local git repo holding a plugin and returns a @@ -403,3 +406,778 @@ 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) + writeWorkspaceMarker(t, workspace, "zero.demo") + 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) + writeWorkspaceMarker(t, workspace, "zero.demo") + if err := os.Rename(filepath.Join(dir, "zero.demo"), filepath.Join(workspace, "previous")); err != nil { + t.Fatal(err) + } + + if err := Remove(dir, "zero.demo"); err != nil { + t.Fatalf("Remove: %v", err) + } + + other := validManifest() + other["id"] = "zero.other" + src2 := writeSourcePlugin(t, filepath.Join(t.TempDir(), "src2"), other) + if _, err := Install(context.Background(), InstallOptions{Source: src2, Dir: dir}); err != nil { + t.Fatalf("later install: %v", err) + } + + if _, err := os.Stat(filepath.Join(dir, "zero.demo")); !os.IsNotExist(err) { + t.Errorf("a removed plugin was put back on disk: %v", err) + } + loaded, err := Load(LoadOptions{Roots: []Root{{Source: SourceUser, Path: dir}}}) + if err != nil { + t.Fatal(err) + } + for _, p := range loaded.Plugins { + if p.ID == "zero.demo" { + t.Errorf("a removed plugin is loadable again") + } + } +} + +// A commit killed after its publish rename but before it cleared the workspace +// leaves the tree the install replaced in a backup beside the live plugin. +// Removing that plugin deletes the live tree and its lockfile entry, so a +// backup that outlived it would be published again by the next install's +// recovery, and Load enumerates directories rather than the lockfile: the +// removed plugin would be live again with no entry naming it. +func TestRemoveLeavesNoSupersededBackupARecoveryCanResurrect(t *testing.T) { + dir := t.TempDir() + src := writeSourcePlugin(t, filepath.Join(t.TempDir(), "src"), validManifest()) + if _, err := Install(context.Background(), InstallOptions{Source: src, Dir: dir}); err != nil { + t.Fatalf("seeding the install: %v", err) + } + + // The state a kill in that window leaves: the plugin live at its target, + // with the tree it replaced still retained in the workspace beside it. + staged, _, err := installtxn.StageDir(dir) + if err != nil { + t.Fatal(err) + } + workspace := filepath.Dir(staged) + writeWorkspaceMarker(t, workspace, "zero.demo") + 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") + } +} + +// 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. +// 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")) + // 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 TestInterruptedUpdateWithNoLockEntryAbortsTheCaller(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) + } + err = installOther(t, u.dir, "zero.other") + + 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) + } +} + +// 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) { + skipWithoutPermissionFaults(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 3438857c1..23f5a78ab 100644 --- a/internal/skills/install.go +++ b/internal/skills/install.go @@ -150,6 +150,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. + // 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. @@ -184,6 +190,65 @@ 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 { + // 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 + // 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 { @@ -201,6 +266,13 @@ func Remove(dir string, name string) error { return err } defer unlock() + // Put back anything an earlier run was killed mid-commit; see installtxn.Recover. + // 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 10f99cb8f..412b30911 100644 --- a/internal/skills/install_test.go +++ b/internal/skills/install_test.go @@ -7,8 +7,11 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "strings" "testing" + + "github.com/Gitlawb/zero/internal/installtxn" ) // initGitSkillRepo creates a real local git repo holding a skill and returns a @@ -72,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"), @@ -432,3 +447,589 @@ 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) + writeWorkspaceMarker(t, workspace, "alpha") + if err := os.Rename(filepath.Join(dir, "alpha"), filepath.Join(workspace, "previous")); err != nil { + t.Fatal(err) + } + + src2 := writeSourceSkill(t, filepath.Join(t.TempDir(), "src2"), + "---\nname: beta\ndescription: second.\n---\nbeta body\n") + if _, err := Install(context.Background(), InstallOptions{Source: src2, Dir: dir}); err != nil { + t.Fatalf("later install: %v", err) + } + + got, ok := Get(dir, "alpha") + if !ok || !strings.Contains(got.Content, "alpha body") { + t.Fatalf("the interrupted skill install was not put back: ok=%v skill=%+v", ok, got) + } + if _, err := os.Stat(workspace); !os.IsNotExist(err) { + t.Errorf("the recovered workspace should be cleared, got %v", err) + } +} + +// skills.Remove carries the same recovery call as plugins.Remove, so it needs +// the same proof. A commit killed after its publish rename leaves the tree the +// install replaced in a backup beside the live skill; removing the skill must +// not leave that backup for the next install's recovery to publish, since Get +// reads the directory rather than the lockfile and would find the removed skill +// loadable again. +func TestRemoveLeavesNoSupersededBackupARecoveryCanResurrect(t *testing.T) { + dir := t.TempDir() + src := writeSourceSkill(t, filepath.Join(t.TempDir(), "src"), + "---\nname: alpha\ndescription: first.\n---\nalpha body\n") + if _, err := Install(context.Background(), InstallOptions{Source: src, Dir: dir}); err != nil { + t.Fatalf("seeding the install: %v", err) + } + + // The state a kill after CommitDir's second rename leaves behind. + staged, _, err := installtxn.StageDir(dir) + if err != nil { + t.Fatal(err) + } + workspace := filepath.Dir(staged) + writeWorkspaceMarker(t, workspace, "alpha") + 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") + } +} + +// 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: 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. + 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) + } + // 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) { + 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) + } + }) + } +} + +// 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) + } +} diff --git a/internal/terminalpet/client.go b/internal/terminalpet/client.go index a01a8aec5..3c9ecbd7c 100644 --- a/internal/terminalpet/client.go +++ b/internal/terminalpet/client.go @@ -290,6 +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, + // 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)