diff --git a/cmd/create.go b/cmd/create.go index a5a20dc..a34d57a 100644 --- a/cmd/create.go +++ b/cmd/create.go @@ -41,6 +41,29 @@ var createCmd = &cobra.Command{ repos := discover.FindAllRepos(cfg.RepoDirs) repoMap := discover.RepoMap(repos) + // Resolve and validate branch and name BEFORE any source cloning, so a bad + // name, missing branch, or aborted prompt never triggers a network clone. + // (Cloning is source acquisition; a successful clone is intentionally + // retained even if workspace creation later fails.) + branch := createBranch + if branch == "" { + if console.IsTerminal(os.Stdin) { + branch = console.Prompt("Branch name") + } + if branch == "" { + exitError("Branch is required: --branch / -b") + } + } + var name string + if len(args) > 0 { + name = args[0] + } else { + name = deriveName(branch) + } + if err := workspace.ValidateWorkspaceName(name); err != nil { + exitError(err.Error()) + } + var repoNames []string // Resolve repos from preset @@ -59,6 +82,17 @@ var createCmd = &cobra.Command{ for i := range repoNames { repoNames[i] = strings.TrimSpace(repoNames[i]) } + // Validate non-URL repo names against known repos BEFORE cloning any + // URLs, so a mixed list with an unknown local repo is rejected without a + // network clone. + for _, name := range repoNames { + if gitops.IsGitURL(name) { + continue + } + if _, ok := repoMap[name]; !ok { + exitError("Unknown repo: " + name + ". Available: " + strings.Join(repoNamesList(repos), ", ")) + } + } // Clone any remote git URLs into the first repo_dir (mirrors add-repo). // This lets a resolver pass an unmatched repo as a clone URL. for i, name := range repoNames { @@ -145,31 +179,12 @@ var createCmd = &cobra.Command{ } // Validate repos exist - for _, name := range repoNames { - if _, ok := repoMap[name]; !ok { - exitError("Unknown repo: " + name + ". Available: " + strings.Join(repoNamesList(repos), ", ")) - } - } - - // Branch — prompt if omitted and in a terminal - branch := createBranch - if branch == "" { - if console.IsTerminal(os.Stdin) { - branch = console.Prompt("Branch name") - } - if branch == "" { - exitError("Branch is required: --branch / -b") + for _, rn := range repoNames { + if _, ok := repoMap[rn]; !ok { + exitError("Unknown repo: " + rn + ". Available: " + strings.Join(repoNamesList(repos), ", ")) } } - // Name - var name string - if len(args) > 0 { - name = args[0] - } else { - name = deriveName(branch) - } - // --replace: delete the current workspace (detected from cwd) before creating the new one. replacedName := "" if createReplace { @@ -226,11 +241,12 @@ var createCmd = &cobra.Command{ opts.BranchMode = workspace.BranchModeTrack } - if err := workspace.NewService().CreateWithOpts(name, opts); err != nil { + if res := workspace.NewService().CreateWithResult(name, opts); res.NonZeroExit() { + renderRepoOutcomes(res) if replacedName != "" { - exitError("failed to create new workspace (old workspace " + replacedName + " was already deleted): " + err.Error()) + exitError("failed to create new workspace (old workspace " + replacedName + " was already deleted): " + res.Message) } - exitError(err.Error()) + exitError(res.Message) } // Fire post_create hook if configured @@ -242,10 +258,10 @@ var createCmd = &cobra.Command{ vars.SourceTitle = source.Title } if err := lifecycle.Run("post_create", vars); err != nil && !errors.Is(err, lifecycle.ErrNoHook) { - if lifecycle.ShouldAbort(err) { - exitError(err.Error()) - } + // A post-create hook failure is a partial outcome: the workspace is + // valid but a lifecycle step failed, so exit non-zero. console.Warning(err.Error()) + os.Exit(1) } }, } diff --git a/cmd/create_cli_test.go b/cmd/create_cli_test.go new file mode 100644 index 0000000..95c7719 --- /dev/null +++ b/cmd/create_cli_test.go @@ -0,0 +1,109 @@ +package cmd + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// runGit is a tiny test helper. +func cmdTestGit(t *testing.T, dir string, args ...string) { + t.Helper() + c := exec.Command("git", args...) + c.Dir = dir + c.Env = append(os.Environ(), "GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_SYSTEM=/dev/null") + if out, err := c.CombinedOutput(); err != nil { + t.Fatalf("git %s: %v\n%s", strings.Join(args, " "), err, out) + } +} + +// TestCreateCommandExitsNonZeroAndRetainsClone builds the gw binary and runs a +// real `gw create` that clones a local file:// source and then fails (the target +// workspace name already exists). It proves two contract points at the command +// layer: (1) a failed create exits non-zero, and (2) the successfully cloned +// source repo is retained on disk despite the failure. +func TestCreateCommandExitsNonZeroAndRetainsClone(t *testing.T) { + if testing.Short() { + t.Skip("builds a binary; skipped in -short") + } + tmp := t.TempDir() + home := filepath.Join(tmp, "home") + repoDir := filepath.Join(tmp, "repos") + if err := os.MkdirAll(repoDir, 0o755); err != nil { + t.Fatal(err) + } + + // A local source repo to clone via file:// . + src := filepath.Join(tmp, "srcrepo") + cmdTestGit(t, tmp, "init", "-q", src) + cmdTestGit(t, src, "config", "user.email", "t@t.co") + cmdTestGit(t, src, "config", "user.name", "t") + if err := os.WriteFile(filepath.Join(src, "f"), []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + cmdTestGit(t, src, "add", ".") + cmdTestGit(t, src, "commit", "-qm", "init") + + // Build the gw binary. + bin := filepath.Join(tmp, "gw") + build := exec.Command("go", "build", "-o", bin, "./gw") + build.Dir = mustModuleCmdDir(t) + if out, err := build.CombinedOutput(); err != nil { + t.Fatalf("build gw: %v\n%s", err, out) + } + + env := append(os.Environ(), "HOME="+home) + + // Initialize grove with our repo dir. + initCmd := exec.Command(bin, "init", repoDir) + initCmd.Env = env + if out, err := initCmd.CombinedOutput(); err != nil { + t.Fatalf("gw init: %v\n%s", err, out) + } + + // Create the workspace name first so the later create collides. + url := "file://" + src + // Seed an existing workspace named "srcrepo" (derived name) via a normal + // create from a plain local repo so the second create hits "already exists". + plain := filepath.Join(repoDir, "plain") + cmdTestGit(t, repoDir, "init", "-q", "plain") + cmdTestGit(t, plain, "config", "user.email", "t@t.co") + cmdTestGit(t, plain, "config", "user.name", "t") + os.WriteFile(filepath.Join(plain, "f"), []byte("x"), 0o644) + cmdTestGit(t, plain, "add", ".") + cmdTestGit(t, plain, "commit", "-qm", "init") + + seed := exec.Command(bin, "create", "dup-ws", "-b", "feat/seed", "-r", "plain") + seed.Env = env + if out, err := seed.CombinedOutput(); err != nil { + t.Fatalf("seed create: %v\n%s", err, out) + } + + // Now create "dup-ws" again but with a clone URL — the clone should happen + // (source acquisition), then the create fails because dup-ws exists. + fail := exec.Command(bin, "create", "dup-ws", "-b", "feat/x", "-r", url) + fail.Env = env + out, err := fail.CombinedOutput() + if err == nil { + t.Fatalf("expected non-zero exit for duplicate create, got success:\n%s", out) + } + if _, ok := err.(*exec.ExitError); !ok { + t.Fatalf("expected exit error, got %T: %v", err, err) + } + // The cloned source must be retained despite the failure. + if _, statErr := os.Stat(filepath.Join(repoDir, "srcrepo")); statErr != nil { + t.Fatalf("cloned source repo must be retained after failed create: %v", statErr) + } +} + +// mustModuleCmdDir returns the cmd/ directory (this test file's directory). +func mustModuleCmdDir(t *testing.T) string { + t.Helper() + wd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + return wd // tests run in the package dir (cmd/) +} diff --git a/cmd/outcome.go b/cmd/outcome.go new file mode 100644 index 0000000..634a374 --- /dev/null +++ b/cmd/outcome.go @@ -0,0 +1,35 @@ +package cmd + +import ( + "fmt" + "io" + "os" + + "github.com/nicksenap/grove/internal/workspace" +) + +// renderRepoOutcomes prints each per-repository outcome to stderr so automation +// and humans both see complete detail before a non-zero exit. The public +// machine-readable envelope is a separate concern (issue #63). +func renderRepoOutcomes(res *workspace.OperationResult) { + fprintRepoOutcomes(os.Stderr, res) +} + +// fprintRepoOutcomes writes the ordered per-repository outcomes to w. +func fprintRepoOutcomes(w io.Writer, res *workspace.OperationResult) { + for _, r := range res.Repos { + line := fmt.Sprintf(" %-20s %s", r.RepoName, r.Status) + if r.Phase != "" { + line += " (" + r.Phase + ")" + } + if r.Err != nil { + line += ": " + r.Err.Error() + } else if r.Message != "" { + line += ": " + r.Message + } + fmt.Fprintln(w, line) + } + if res.RecordID != "" { + fmt.Fprintf(w, " recovery record: %s (run: gw doctor)\n", res.RecordID) + } +} diff --git a/cmd/outcome_test.go b/cmd/outcome_test.go new file mode 100644 index 0000000..b7b4994 --- /dev/null +++ b/cmd/outcome_test.go @@ -0,0 +1,48 @@ +package cmd + +import ( + "bytes" + "errors" + "strings" + "testing" + + "github.com/nicksenap/grove/internal/state" + "github.com/nicksenap/grove/internal/workspace" +) + +func TestFprintRepoOutcomesRendersAllRepos(t *testing.T) { + res := &workspace.OperationResult{ + Kind: state.OpCreate, + Workspace: "ws", + Status: workspace.OutcomePending, + RecordID: "op-123", + Repos: []workspace.RepoOutcome{ + {RepoName: "api", Status: state.RepoDone, Phase: "provision"}, + {RepoName: "web", Status: state.RepoFailed, Phase: "provision", Err: errors.New("boom")}, + }, + } + var buf bytes.Buffer + fprintRepoOutcomes(&buf, res) + out := buf.String() + for _, want := range []string{"api", "web", "boom", "op-123", "recovery record"} { + if !strings.Contains(out, want) { + t.Fatalf("output missing %q:\n%s", want, out) + } + } +} + +func TestOperationResultExitMapping(t *testing.T) { + cases := map[workspace.OutcomeStatus]bool{ + workspace.OutcomeSuccess: false, + workspace.OutcomeCancelled: false, + workspace.OutcomePartial: true, + workspace.OutcomeFailed: true, + workspace.OutcomePending: true, + } + for status, wantNonZero := range cases { + res := &workspace.OperationResult{Status: status} + if res.NonZeroExit() != wantNonZero { + t.Fatalf("status %s: NonZeroExit=%v want %v", status, res.NonZeroExit(), wantNonZero) + } + } +} diff --git a/internal/state/durable.go b/internal/state/durable.go new file mode 100644 index 0000000..4ea0bca --- /dev/null +++ b/internal/state/durable.go @@ -0,0 +1,98 @@ +package state + +import ( + "os" + "path/filepath" +) + +// durableSeams holds optional, test-only fault injectors for each durability +// phase. All fields are nil in production. +type durableSeams struct { + failWrite func() error + failSync func() error + failRename func() error + failDirSync func() error +} + +// writeFileDurableWith writes data to path with crash-consistent durability: it +// writes to a uniquely named temp file in the destination directory, fsyncs the +// file, atomically renames it into place, then fsyncs the parent directory so +// the rename itself is durable. The unique temp name avoids collisions between +// concurrent writers and never leaves a shared ".tmp" path behind. +func writeFileDurableWith(path string, data []byte, perm os.FileMode, seams durableSeams) (err error) { + dir := filepath.Dir(path) + f, err := os.CreateTemp(dir, "."+filepath.Base(path)+".*.tmp") + if err != nil { + return err + } + tmp := f.Name() + // Clean up the temp file on any failure before the rename succeeds. + defer func() { + if tmp != "" { + _ = os.Remove(tmp) + } + }() + + if err = f.Chmod(perm); err != nil { + _ = f.Close() + return err + } + if seams.failWrite != nil { + if err = seams.failWrite(); err != nil { + _ = f.Close() + return err + } + } + if _, err = f.Write(data); err != nil { + _ = f.Close() + return err + } + if seams.failSync != nil { + if err = seams.failSync(); err != nil { + _ = f.Close() + return err + } + } + if err = f.Sync(); err != nil { + _ = f.Close() + return err + } + if err = f.Close(); err != nil { + return err + } + if seams.failRename != nil { + if err = seams.failRename(); err != nil { + return err + } + } + if err = os.Rename(tmp, path); err != nil { + return err + } + tmp = "" // renamed successfully; nothing to clean up + + // fsync the parent directory so the rename survives a crash. + if seams.failDirSync != nil { + if err = seams.failDirSync(); err != nil { + return err + } + } + d, err := os.Open(dir) + if err != nil { + return err + } + if err = d.Sync(); err != nil { + _ = d.Close() + return err + } + return d.Close() +} + +// writeFileDurable is the Store's durable writer, wired to its test-only seams. +func (s *Store) writeFileDurable(path string, data []byte, perm os.FileMode) error { + return writeFileDurableWith(path, data, perm, durableSeams{ + failWrite: s.failWrite, + failSync: s.failSync, + failRename: s.failRename, + failDirSync: s.failDirSync, + }) +} diff --git a/internal/state/errors.go b/internal/state/errors.go new file mode 100644 index 0000000..6d532ce --- /dev/null +++ b/internal/state/errors.go @@ -0,0 +1,62 @@ +package state + +import ( + "errors" + "fmt" +) + +// Stable internal error codes for state operations. These are consumed by +// service-layer typed outcomes (issue #59) and are not part of any public +// machine envelope. +const ( + // CodeStateLockTimeout indicates the advisory state lock could not be + // acquired within the configured timeout. It is retryable. + CodeStateLockTimeout = "STATE_LOCK_TIMEOUT" + // CodeStateConflict indicates a same-record conflict revalidated under the + // lock (e.g. a workspace with the same name already exists). + CodeStateConflict = "STATE_CONFLICT" + // CodeStateNested indicates a nested state mutation was attempted from the + // same goroutine that already holds a Mutation handle. + CodeStateNested = "STATE_NESTED_MUTATION" + // CodeStateInactiveHandle indicates a Mutation handle was used after its + // WithMutation callback returned. + CodeStateInactiveHandle = "STATE_INACTIVE_HANDLE" +) + +// CodedError carries a stable internal error code alongside a human message. +type CodedError struct { + Code string + Message string + Err error + Retryable bool +} + +func (e *CodedError) Error() string { + if e.Err != nil { + return fmt.Sprintf("%s: %s: %v", e.Code, e.Message, e.Err) + } + return fmt.Sprintf("%s: %s", e.Code, e.Message) +} + +func (e *CodedError) Unwrap() error { return e.Err } + +// CodeOf returns the stable code for err if it is a *CodedError, else "". +func CodeOf(err error) string { + if err == nil { + return "" + } + var ce *CodedError + if errors.As(err, &ce) { + return ce.Code + } + return "" +} + +// IsRetryable reports whether err is a retryable coded error. +func IsRetryable(err error) bool { + var ce *CodedError + if errors.As(err, &ce) { + return ce.Retryable + } + return false +} diff --git a/internal/state/lock_test.go b/internal/state/lock_test.go new file mode 100644 index 0000000..9b29e00 --- /dev/null +++ b/internal/state/lock_test.go @@ -0,0 +1,422 @@ +package state + +import ( + "context" + "encoding/json" + "errors" + "os" + "os/exec" + "path/filepath" + "sync" + "testing" + "time" + + "github.com/nicksenap/grove/internal/models" +) + +// --- Subprocess helper --- +// +// These tests re-exec the test binary as helper processes to exercise real +// cross-process locking (a single-process mutex would not catch lost updates +// between separate `gw` invocations). + +const ( + envHelperMode = "GW_STATE_HELPER_MODE" + envHelperDir = "GW_STATE_HELPER_DIR" + envHelperName = "GW_STATE_HELPER_NAME" + envHelperFile = "GW_STATE_HELPER_READY" +) + +// TestStateSubprocessHelper is not a real test; it is the entry point used when +// the process is re-executed as a helper. It returns immediately in normal runs. +func TestStateSubprocessHelper(t *testing.T) { + mode := os.Getenv(envHelperMode) + if mode == "" { + return + } + store := NewStore(os.Getenv(envHelperDir)) + switch mode { + case "add": + name := os.Getenv(envHelperName) + err := store.WithMutation(context.Background(), func(m *Mutation) error { + if err := m.Add(models.NewWorkspace(name, "/tmp/"+name, "main")); err != nil { + return err + } + return m.Commit() + }) + if err != nil { + if CodeOf(err) == CodeStateConflict { + os.Exit(4) // deterministic conflict + } + os.Exit(3) + } + os.Exit(0) + case "hold": + // Acquire the lock and hold it (never releasing) until killed, to prove + // the OS releases advisory locks on process death. + lock, err := acquireLock(context.Background(), store.lockPath(), 5*time.Second) + if err != nil { + os.Exit(3) + } + _ = lock + if ready := os.Getenv(envHelperFile); ready != "" { + _ = os.WriteFile(ready, []byte("locked"), 0o644) + } + time.Sleep(60 * time.Second) + os.Exit(0) + case "writeop": + // Write a recovery record then exit abruptly without cleaning it up, to + // prove records survive a crashed helper process. + ops := NewOperationStore(os.Getenv(envHelperDir)) + rec := &OperationRecord{ + Kind: OpCreate, + Workspace: os.Getenv(envHelperName), + Phase: "provisioning", + Repos: []RepoOperation{ + {RepoName: "api", Status: RepoDone, BranchOwnership: OwnCreated, WorktreeOwnership: OwnCreated}, + }, + } + if err := ops.Write(rec); err != nil { + os.Exit(3) + } + if ready := os.Getenv(envHelperFile); ready != "" { + _ = os.WriteFile(ready, []byte(rec.ID), 0o644) + } + os.Exit(0) + } +} + +// runHelper starts the test binary in helper mode. +func runHelper(t *testing.T, mode string, extraEnv ...string) *exec.Cmd { + t.Helper() + cmd := exec.Command(os.Args[0], "-test.run=^TestStateSubprocessHelper$") + cmd.Env = append(os.Environ(), envHelperMode+"="+mode) + cmd.Env = append(cmd.Env, extraEnv...) + return cmd +} + +func TestStoreConcurrentSubprocessMutations(t *testing.T) { + dir := t.TempDir() + groveDir := filepath.Join(dir, ".grove") + if err := os.MkdirAll(groveDir, 0o755); err != nil { + t.Fatal(err) + } + store := NewStore(groveDir) + if err := os.WriteFile(store.Path, []byte("[]"), 0o644); err != nil { + t.Fatal(err) + } + + const n = 12 + var wg sync.WaitGroup + errs := make([]error, n) + for i := 0; i < n; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + name := "ws-" + string(rune('a'+i)) + cmd := runHelper(t, "add", + envHelperDir+"="+groveDir, + envHelperName+"="+name, + ) + errs[i] = cmd.Run() + }(i) + } + wg.Wait() + + for i, err := range errs { + if err != nil { + t.Fatalf("helper %d failed: %v", i, err) + } + } + + got, err := store.Load() + if err != nil { + t.Fatalf("load: %v", err) + } + if len(got) != n { + t.Fatalf("expected %d workspaces after concurrent adds, got %d: %v", n, len(got), names(got)) + } + seen := map[string]bool{} + for _, ws := range got { + if seen[ws.Name] { + t.Fatalf("duplicate workspace %q", ws.Name) + } + seen[ws.Name] = true + } +} + +func names(ws []models.Workspace) []string { + out := make([]string, len(ws)) + for i := range ws { + out[i] = ws[i].Name + } + return out +} + +func exitCode(err error) int { + if err == nil { + return 0 + } + var ee *exec.ExitError + if errors.As(err, &ee) { + return ee.ExitCode() + } + return -1 +} + +func TestStoreConcurrentSameNameConflict(t *testing.T) { + dir := t.TempDir() + groveDir := filepath.Join(dir, ".grove") + if err := os.MkdirAll(groveDir, 0o755); err != nil { + t.Fatal(err) + } + store := NewStore(groveDir) + if err := os.WriteFile(store.Path, []byte("[]"), 0o644); err != nil { + t.Fatal(err) + } + + const n = 5 + var wg sync.WaitGroup + codes := make([]int, n) + for i := 0; i < n; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + cmd := runHelper(t, "add", + envHelperDir+"="+groveDir, + envHelperName+"=dup", + ) + codes[i] = exitCode(cmd.Run()) + }(i) + } + wg.Wait() + + success, conflict := 0, 0 + for _, c := range codes { + switch c { + case 0: + success++ + case 4: + conflict++ + default: + t.Fatalf("unexpected helper exit code %d (codes=%v)", c, codes) + } + } + if success != 1 { + t.Fatalf("expected exactly one winner, got %d (codes=%v)", success, codes) + } + if conflict != n-1 { + t.Fatalf("expected %d conflicts, got %d (codes=%v)", n-1, conflict, codes) + } + + got, _ := store.Load() + if len(got) != 1 || got[0].Name != "dup" { + t.Fatalf("expected single 'dup' workspace, got %v", names(got)) + } +} + +func TestMutationCreatesGroveDir(t *testing.T) { + // The grove dir does not exist yet; the first mutation must create it. + dir := t.TempDir() + groveDir := filepath.Join(dir, "nonexistent", ".grove") + store := NewStore(groveDir) + + if err := store.AddWorkspace(models.NewWorkspace("first", "/tmp/first", "main")); err != nil { + t.Fatalf("first mutation should create grove dir: %v", err) + } + got, _ := store.Load() + if len(got) != 1 { + t.Fatalf("expected 1 workspace, got %v", names(got)) + } +} + +func TestEscapedMutationCannotCommit(t *testing.T) { + s := testStore(t) + var escaped *Mutation + if err := s.WithMutation(context.Background(), func(m *Mutation) error { + escaped = m + return nil + }); err != nil { + t.Fatalf("mutation: %v", err) + } + // After the callback returns the handle is invalid. + if err := escaped.Add(models.NewWorkspace("x", "/tmp/x", "main")); CodeOf(err) != CodeStateInactiveHandle { + t.Fatalf("expected inactive-handle error on Add, got %v", err) + } + if err := escaped.Commit(); CodeOf(err) != CodeStateInactiveHandle { + t.Fatalf("expected inactive-handle error on Commit, got %v", err) + } +} + +func TestNestedMutationRejected(t *testing.T) { + s := testStore(t) + err := s.WithMutation(context.Background(), func(m *Mutation) error { + // Calling a public lock-acquiring mutator from inside must fail fast. + return s.AddWorkspace(models.NewWorkspace("nested", "/tmp/nested", "main")) + }) + if CodeOf(err) != CodeStateNested { + t.Fatalf("expected nested-mutation error, got %v", err) + } +} + +func TestCommitWriteFailurePreservesState(t *testing.T) { + phases := []struct { + name string + set func(s *Store, fail func() error) + }{ + {"write", func(s *Store, f func() error) { s.failWrite = f }}, + {"sync", func(s *Store, f func() error) { s.failSync = f }}, + {"rename", func(s *Store, f func() error) { s.failRename = f }}, + {"dirsync", func(s *Store, f func() error) { s.failDirSync = f }}, + } + for _, ph := range phases { + t.Run(ph.name, func(t *testing.T) { + s := testStore(t) + if err := s.AddWorkspace(models.NewWorkspace("keep", "/tmp/keep", "main")); err != nil { + t.Fatalf("seed: %v", err) + } + before, _ := os.ReadFile(s.Path) + + boom := errors.New("inject " + ph.name) + ph.set(s, func() error { return boom }) + err := s.AddWorkspace(models.NewWorkspace("ghost", "/tmp/ghost", "main")) + if !errors.Is(err, boom) { + t.Fatalf("%s: expected injected error, got %v", ph.name, err) + } + ph.set(s, nil) + + after, _ := os.ReadFile(s.Path) + // rename/dirsync happen after (or during) the swap; for rename the old + // file must remain intact, for dirsync the write already succeeded so we + // only require valid JSON. In all cases the file must stay valid. + var ws []models.Workspace + if jerr := jsonUnmarshal(after, &ws); jerr != nil { + t.Fatalf("%s: state file corrupt: %v", ph.name, jerr) + } + if ph.name == "write" || ph.name == "sync" || ph.name == "rename" { + if string(before) != string(after) { + t.Fatalf("%s: state changed despite failure before rename\nbefore=%s\nafter=%s", ph.name, before, after) + } + } + + // No leaked temp files regardless of phase. + assertNoLeakedTemp(t, s) + + // Lock released: a subsequent clean mutation works. + if err := s.AddWorkspace(models.NewWorkspace("after", "/tmp/after", "main")); err != nil { + t.Fatalf("%s: mutation after failure should succeed: %v", ph.name, err) + } + }) + } +} + +func assertNoLeakedTemp(t *testing.T, s *Store) { + t.Helper() + matches, _ := filepath.Glob(filepath.Join(s.dir(), ".*.tmp")) + if len(matches) != 0 { + t.Fatalf("leaked temp files: %v", matches) + } +} + +func jsonUnmarshal(data []byte, v interface{}) error { + if len(data) == 0 { + return nil + } + return json.Unmarshal(data, v) +} + +func TestStoreMutationFailurePreservesState(t *testing.T) { + s := testStore(t) + if err := s.AddWorkspace(models.NewWorkspace("keep", "/tmp/keep", "main")); err != nil { + t.Fatalf("seed: %v", err) + } + before, err := os.ReadFile(s.Path) + if err != nil { + t.Fatal(err) + } + + // Callback edits then returns an error without committing. + sentinel := errors.New("boom") + err = s.WithMutation(context.Background(), func(m *Mutation) error { + _ = m.Add(models.NewWorkspace("ghost", "/tmp/ghost", "main")) + return sentinel + }) + if !errors.Is(err, sentinel) { + t.Fatalf("expected sentinel error, got %v", err) + } + + after, err := os.ReadFile(s.Path) + if err != nil { + t.Fatal(err) + } + if string(before) != string(after) { + t.Fatalf("state changed after failed mutation:\nbefore=%s\nafter=%s", before, after) + } + + // The lock must have been released — a subsequent mutation succeeds. + if err := s.AddWorkspace(models.NewWorkspace("second", "/tmp/second", "main")); err != nil { + t.Fatalf("mutation after failure should succeed (lock released): %v", err) + } + got, _ := s.Load() + if len(got) != 2 { + t.Fatalf("expected keep+second, got %v", names(got)) + } + for _, ws := range got { + if ws.Name == "ghost" { + t.Fatal("uncommitted ghost workspace leaked into state") + } + } +} + +func TestStateLockReleasedAfterProcessExit(t *testing.T) { + dir := t.TempDir() + groveDir := filepath.Join(dir, ".grove") + if err := os.MkdirAll(groveDir, 0o755); err != nil { + t.Fatal(err) + } + store := NewStore(groveDir) + ready := filepath.Join(dir, "ready") + + cmd := runHelper(t, "hold", + envHelperDir+"="+groveDir, + envHelperFile+"="+ready, + ) + if err := cmd.Start(); err != nil { + t.Fatalf("start helper: %v", err) + } + + // Wait until the helper reports it holds the lock. + deadline := time.Now().Add(10 * time.Second) + for { + if _, err := os.Stat(ready); err == nil { + break + } + if time.Now().After(deadline) { + _ = cmd.Process.Kill() + t.Fatal("helper never acquired the lock") + } + time.Sleep(20 * time.Millisecond) + } + + // Confirm the lock is genuinely held: a short-timeout acquisition fails. + if l, err := acquireLock(context.Background(), store.lockPath(), 300*time.Millisecond); err == nil { + _ = l.release() + _ = cmd.Process.Kill() + t.Fatal("expected lock to be held by helper") + } else if !errors.Is(err, errLockTimeout) { + _ = cmd.Process.Kill() + t.Fatalf("expected lock timeout, got %v", err) + } + + // Kill the helper (simulating a crash) and confirm the OS released the lock. + if err := cmd.Process.Kill(); err != nil { + t.Fatalf("kill helper: %v", err) + } + _, _ = cmd.Process.Wait() + + l, err := acquireLock(context.Background(), store.lockPath(), 10*time.Second) + if err != nil { + t.Fatalf("lock should be acquirable after holder died: %v", err) + } + _ = l.release() +} diff --git a/internal/state/lock_unix.go b/internal/state/lock_unix.go new file mode 100644 index 0000000..4b5c38c --- /dev/null +++ b/internal/state/lock_unix.go @@ -0,0 +1,65 @@ +//go:build !windows + +package state + +import ( + "context" + "errors" + "os" + "syscall" + "time" +) + +// errLockTimeout is the sentinel returned when the advisory lock cannot be +// acquired within the deadline. Callers translate it into a CodedError. +var errLockTimeout = errors.New("state lock timeout") + +// fileLock is a process-level advisory lock backed by flock(2). The lock is +// released when the file descriptor is closed or the process exits, which is +// what makes stranded locks self-healing after a crash. The lock file itself is +// never unlinked. +type fileLock struct { + f *os.File +} + +// acquireLock takes an exclusive advisory lock on path, polling until the +// deadline. It returns errLockTimeout if the lock cannot be acquired in time. +func acquireLock(ctx context.Context, path string, timeout time.Duration) (*fileLock, error) { + f, err := os.OpenFile(path, os.O_CREATE|os.O_RDWR, 0o644) + if err != nil { + return nil, err + } + deadline := time.Now().Add(timeout) + for { + err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB) + if err == nil { + return &fileLock{f: f}, nil + } + if !errors.Is(err, syscall.EWOULDBLOCK) { + _ = f.Close() + return nil, err + } + if ctx != nil { + select { + case <-ctx.Done(): + _ = f.Close() + return nil, ctx.Err() + default: + } + } + if time.Now().After(deadline) { + _ = f.Close() + return nil, errLockTimeout + } + time.Sleep(25 * time.Millisecond) + } +} + +// release unlocks and closes the descriptor without removing the lock file. +func (l *fileLock) release() error { + if l == nil || l.f == nil { + return nil + } + _ = syscall.Flock(int(l.f.Fd()), syscall.LOCK_UN) + return l.f.Close() +} diff --git a/internal/state/lock_workspace_unix.go b/internal/state/lock_workspace_unix.go new file mode 100644 index 0000000..e5b48a9 --- /dev/null +++ b/internal/state/lock_workspace_unix.go @@ -0,0 +1,45 @@ +//go:build !windows + +package state + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" +) + +// AcquireWorkspaceLock takes a cross-process advisory lock scoped to a single +// workspace name. It serializes same-name lifecycle operations (e.g. two +// concurrent creates) without holding the global state lock, so provisioning +// for different workspaces still runs concurrently. The returned release +// function must be called when the critical section ends. The lock file is +// never unlinked. +func (s *Store) AcquireWorkspaceLock(ctx context.Context, name string) (func(), error) { + return s.AcquireResourceLock(ctx, "ws-"+name) +} + +// AcquireResourceLock takes a cross-process advisory lock scoped to an arbitrary +// resource key (e.g. a workspace name, or a source-repo+branch pair). Distinct +// keys run concurrently; the same key serializes. The lock file is never +// unlinked. +func (s *Store) AcquireResourceLock(ctx context.Context, key string) (func(), error) { + dir := filepath.Join(s.dir(), "locks") + if err := os.MkdirAll(dir, 0o755); err != nil { + return nil, err + } + lp := filepath.Join(dir, sanitizeIDPart(key)+".lock") + lock, err := acquireLock(ctx, lp, lockTimeout) + if err != nil { + if errors.Is(err, errLockTimeout) { + return nil, &CodedError{ + Code: CodeStateLockTimeout, + Message: fmt.Sprintf("could not acquire lock for %q within %s", key, lockTimeout), + Retryable: true, + } + } + return nil, err + } + return func() { _ = lock.release() }, nil +} diff --git a/internal/state/operation.go b/internal/state/operation.go new file mode 100644 index 0000000..9344056 --- /dev/null +++ b/internal/state/operation.go @@ -0,0 +1,387 @@ +package state + +import ( + "crypto/rand" + "encoding/hex" + "encoding/json" + "fmt" + "os" + "path/filepath" + "sort" + "strings" + "sync/atomic" + "time" + + "github.com/nicksenap/grove/internal/models" +) + +// operationRecordVersion is the schema version for durable recovery records. +// Bump it only with an explicit migration; unknown newer versions are surfaced +// by doctor but never mutated. +const operationRecordVersion = 1 + +// OperationKind identifies which mutating operation a recovery record describes. +type OperationKind string + +const ( + OpCreate OperationKind = "create" + OpAdd OperationKind = "add" + OpRemove OperationKind = "remove" + OpDelete OperationKind = "delete" + OpSync OperationKind = "sync" + OpRename OperationKind = "rename" +) + +// KnownOperationKind reports whether k is a recognized recovery-record kind. +func KnownOperationKind(k OperationKind) bool { + switch k { + case OpCreate, OpAdd, OpRemove, OpDelete, OpSync, OpRename: + return true + } + return false +} + +// RepoStatus is the status of a single repository within an operation. +type RepoStatus string + +const ( + RepoPending RepoStatus = "pending" + RepoInProgress RepoStatus = "in_progress" + RepoDone RepoStatus = "done" + RepoFailed RepoStatus = "failed" + RepoCompensated RepoStatus = "compensated" + RepoSkipped RepoStatus = "skipped" +) + +// ResourceOwnership records whether a Git resource (branch or worktree) was +// created by this operation. It is deliberately three-valued so that repair can +// stay conservative: an empty (unknown) value must never be compensated +// destructively. +type ResourceOwnership string + +const ( + // OwnUnknown means ownership was not determined (e.g. the operation was + // interrupted before it recorded whether it created the resource). Repair + // must not destroy an unknown-ownership resource. + OwnUnknown ResourceOwnership = "" + // OwnPreexisting means the resource existed before this operation; it must + // never be compensated away. + OwnPreexisting ResourceOwnership = "preexisting" + // OwnCreated means this operation created the resource; it is safe to + // compensate. + OwnCreated ResourceOwnership = "created" +) + +// CommitStatus records the state of the single authoritative state commit. It +// lets repair distinguish "never committed" from "commit issued but the result +// is ambiguous" (e.g. a directory-sync error after a successful rename), so a +// repair reconciles authoritative state rather than blindly compensating. +type CommitStatus string + +const ( + // CommitPending means the state commit has not been attempted yet. + CommitPending CommitStatus = "" + // CommitAttempted means the commit was issued but its durability is + // uncertain; repair must reconcile against authoritative state. + CommitAttempted CommitStatus = "attempted" + // CommitDone means the commit is confirmed durable. + CommitDone CommitStatus = "committed" +) + +// ProvisionMode records how a repository worktree was (or must be) provisioned +// so a constructive retry reproduces the original semantics. +type ProvisionMode string + +const ( + // ProvisionFromBase creates a new branch from the resolved base branch. + ProvisionFromBase ProvisionMode = "" + // ProvisionTrack checks out an existing remote branch via a tracking + // worktree (e.g. a PR head). + ProvisionTrack ProvisionMode = "track" +) + +// RepoOperation captures per-repository progress, resource ownership, and the +// last coded error so repair can compensate exactly what this operation created +// and retain actionable detail for mixed outcomes. +type RepoOperation struct { + RepoName string `json:"repo_name"` + SourceRepo string `json:"source_repo,omitempty"` + WorktreePath string `json:"worktree_path,omitempty"` + Branch string `json:"branch,omitempty"` + // BaseBranch is the resolved base branch for this repo (may differ per repo + // via .grove.toml). Mode records how the worktree must be provisioned. + BaseBranch string `json:"base_branch,omitempty"` + Mode ProvisionMode `json:"mode,omitempty"` + Phase string `json:"phase,omitempty"` + Status RepoStatus `json:"status"` + + // Ownership of each resource this operation may compensate. + BranchOwnership ResourceOwnership `json:"branch_ownership,omitempty"` + WorktreeOwnership ResourceOwnership `json:"worktree_ownership,omitempty"` + + // Per-repository coded error for mixed-outcome operations. + ErrorCode string `json:"error_code,omitempty"` + Error string `json:"error,omitempty"` + Retryable bool `json:"retryable,omitempty"` +} + +// OperationRecord is a durable, operation-specific recovery journal entry. It is +// written before the first workspace Git/filesystem mutation and removed only +// after the state commit or complete compensation succeeds. +type OperationRecord struct { + Version int `json:"version"` + ID string `json:"id"` + Kind OperationKind `json:"kind"` + Workspace string `json:"workspace"` + + // Repair-critical target identity captured up front. + Path string `json:"path,omitempty"` // workspace root path + BaseBranch string `json:"base_branch,omitempty"` // resolved base branch + // RootOwnership records whether this operation created the workspace root + // directory, so repair can distinguish an operation-created root (safe to + // remove on compensation) from a pre-existing one. + RootOwnership ResourceOwnership `json:"root_ownership,omitempty"` + // Source preserves create provenance so a repaired/completed create can + // reconstruct the intended workspace exactly. + Source *models.WorkspaceSource `json:"source,omitempty"` + // Force records the destructive authorization granted to this operation. + // Repair must never widen force beyond what was recorded here. + Force bool `json:"force,omitempty"` + + // Rename identity (only meaningful for OpRename). Typed rather than stashed + // in Details because both sides are required to complete or revert a rename. + RenameFrom string `json:"rename_from,omitempty"` + RenameTo string `json:"rename_to,omitempty"` + RenameFromPath string `json:"rename_from_path,omitempty"` + RenameToPath string `json:"rename_to_path,omitempty"` + + // Phase is the high-level operation phase (operation-specific vocabulary). + Phase string `json:"phase"` + CommitStatus CommitStatus `json:"commit_status,omitempty"` + CreatedAt string `json:"created_at"` + UpdatedAt string `json:"updated_at"` + PID int `json:"pid"` + + Repos []RepoOperation `json:"repos,omitempty"` + + // LastError is the latest retryable error message, if any. + LastError string `json:"last_error,omitempty"` + Retryable bool `json:"retryable,omitempty"` + + // Details carries auxiliary operation-specific fields not modeled above. + // Repair-critical fields must use typed columns, never this map. + Details map[string]string `json:"details,omitempty"` +} + +// Supported reports whether this record can be safely repaired by the current +// binary. Only the exact current schema version with a known non-empty kind is +// supported; unversioned, newer, or malformed records are surfaced by doctor +// but must never be mutated. +func (r *OperationRecord) Supported() bool { + return r.Version == operationRecordVersion && KnownOperationKind(r.Kind) +} + +// OperationStore persists recovery records under ~/.grove/operations/. +type OperationStore struct { + Dir string + + // Test-only durability fault seams; nil in production. + failWrite func() error + failSync func() error + failRename func() error + failDirSync func() error + failDirEntrySync func() error +} + +// NewOperationStore creates an OperationStore for the given grove dir. +func NewOperationStore(groveDir string) *OperationStore { + return &OperationStore{Dir: filepath.Join(groveDir, "operations")} +} + +// opSeq is a process-local monotonic counter that guarantees ids created within +// the same clock tick still sort in creation order. +var opSeq uint64 + +// NewOperationID returns a strictly time-ordered, unique operation id. The +// leading nanosecond timestamp plus a monotonic sequence guarantees lexical +// ordering matches creation order even within the same clock tick. +func NewOperationID(kind OperationKind, workspace string) string { + var b [6]byte + _, _ = rand.Read(b[:]) + ts := time.Now().UTC().Format("20060102T150405.000000000") + seq := atomic.AddUint64(&opSeq, 1) + safe := sanitizeIDPart(workspace) + return fmt.Sprintf("%s-%012d-%s-%s-%s", ts, seq, kind, safe, hex.EncodeToString(b[:])) +} + +// sanitizeIDPart makes a string safe to embed in a filename component. +func sanitizeIDPart(s string) string { + return strings.Map(func(r rune) rune { + switch { + case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9': + return r + case r == '.' || r == '_': + return r + default: + return '-' + } + }, s) +} + +// validID guards against path escape via a crafted id. +func validID(id string) bool { + if id == "" || strings.ContainsAny(id, "/\\") || strings.Contains(id, "..") { + return false + } + return id == filepath.Base(id) +} + +func (o *OperationStore) path(id string) string { + return filepath.Join(o.Dir, id+".json") +} + +// Write durably persists rec, stamping version and timestamps. It returns the +// record id. +func (o *OperationStore) Write(rec *OperationRecord) error { + if !KnownOperationKind(rec.Kind) { + return fmt.Errorf("unknown or empty operation kind %q", rec.Kind) + } + if rec.Version == 0 { + rec.Version = operationRecordVersion + } else if rec.Version != operationRecordVersion { + return fmt.Errorf("refusing to write unsupported operation record version %d", rec.Version) + } + if rec.ID == "" { + rec.ID = NewOperationID(rec.Kind, rec.Workspace) + } + if !validID(rec.ID) { + return fmt.Errorf("invalid operation id %q", rec.ID) + } + now := time.Now().UTC().Format(time.RFC3339Nano) + if rec.CreatedAt == "" { + rec.CreatedAt = now + } + rec.UpdatedAt = now + if rec.PID == 0 { + rec.PID = os.Getpid() + } + + data, err := json.MarshalIndent(rec, "", " ") + if err != nil { + return err + } + if err := o.mkdirAllSync(o.Dir); err != nil { + return err + } + return writeFileDurableWith(o.path(rec.ID), data, 0o644, durableSeams{ + failWrite: o.failWrite, + failSync: o.failSync, + failRename: o.failRename, + failDirSync: o.failDirSync, + }) +} + +// Read loads a single record by id. +func (o *OperationStore) Read(id string) (*OperationRecord, error) { + if !validID(id) { + return nil, fmt.Errorf("invalid operation id %q", id) + } + data, err := os.ReadFile(o.path(id)) + if err != nil { + return nil, err + } + var rec OperationRecord + if err := json.Unmarshal(data, &rec); err != nil { + return nil, fmt.Errorf("corrupt operation record %s: %w", id, err) + } + return &rec, nil +} + +// List returns all recovery records sorted by id (time-ordered). Corrupt files +// are reported as errors alongside the records that did parse. +func (o *OperationStore) List() ([]OperationRecord, error) { + entries, err := os.ReadDir(o.Dir) + if err != nil { + if os.IsNotExist(err) { + return nil, nil + } + return nil, err + } + var recs []OperationRecord + var firstErr error + for _, e := range entries { + if e.IsDir() || !strings.HasSuffix(e.Name(), ".json") { + continue + } + if strings.HasPrefix(e.Name(), ".") { + continue // temp file in flight + } + id := strings.TrimSuffix(e.Name(), ".json") + rec, err := o.Read(id) + if err != nil { + if firstErr == nil { + firstErr = err + } + continue + } + // Guard against a record whose embedded ID disagrees with its filename; + // repair keys off the ID, so a mismatch is treated as corrupt. + if rec.ID != id { + if firstErr == nil { + firstErr = fmt.Errorf("operation record %s has mismatched id %q", id, rec.ID) + } + continue + } + recs = append(recs, *rec) + } + sort.Slice(recs, func(i, j int) bool { return recs[i].ID < recs[j].ID }) + return recs, firstErr +} + +// Delete removes a record by id and syncs the journal directory so the removal +// is durable. It is a no-op if the record (or the journal directory) is absent. +func (o *OperationStore) Delete(id string) error { + if !validID(id) { + return fmt.Errorf("invalid operation id %q", id) + } + err := os.Remove(o.path(id)) + if err != nil && !os.IsNotExist(err) { + return err + } + if _, statErr := os.Stat(o.Dir); statErr != nil { + if os.IsNotExist(statErr) { + return nil // nothing to sync + } + return statErr + } + return o.syncDir(o.Dir) +} + +// mkdirAllSync ensures dir exists and fsyncs its parent so the directory entry +// is durable. It always syncs the parent — even when dir already exists — so a +// retry after an earlier parent-sync failure still makes the entry durable. +func (o *OperationStore) mkdirAllSync(dir string) error { + if err := os.MkdirAll(dir, 0o755); err != nil { + return err + } + return o.syncDir(filepath.Dir(dir)) +} + +// syncDir fsyncs a directory so entry creation/removal is durable, honoring the +// test-only directory-sync fault seam. +func (o *OperationStore) syncDir(dir string) error { + if o.failDirEntrySync != nil { + if err := o.failDirEntrySync(); err != nil { + return err + } + } + d, err := os.Open(dir) + if err != nil { + return err + } + if err := d.Sync(); err != nil { + _ = d.Close() + return err + } + return d.Close() +} diff --git a/internal/state/operation_test.go b/internal/state/operation_test.go new file mode 100644 index 0000000..071b3be --- /dev/null +++ b/internal/state/operation_test.go @@ -0,0 +1,402 @@ +package state + +import ( + "errors" + "os" + "path/filepath" + "reflect" + "testing" + + "github.com/nicksenap/grove/internal/models" +) + +func TestOperationRecordRoundTrip(t *testing.T) { + dir := t.TempDir() + ops := NewOperationStore(dir) + + rec := &OperationRecord{ + Kind: OpCreate, + Workspace: "feat-login", + Phase: "provisioning", + Repos: []RepoOperation{ + { + RepoName: "api", + SourceRepo: "/src/api", + WorktreePath: "/ws/feat-login/api", + Branch: "feat/login", + Phase: "worktree_added", + Status: RepoDone, + BranchOwnership: OwnCreated, + WorktreeOwnership: OwnCreated, + }, + { + RepoName: "web", + Status: RepoPending, + }, + }, + LastError: "worktree add failed", + Retryable: true, + Details: map[string]string{"base_branch": "main"}, + } + + if err := ops.Write(rec); err != nil { + t.Fatalf("write: %v", err) + } + if rec.ID == "" { + t.Fatal("write should assign an ID") + } + if rec.Version != operationRecordVersion { + t.Fatalf("version: got %d want %d", rec.Version, operationRecordVersion) + } + if rec.CreatedAt == "" || rec.UpdatedAt == "" || rec.PID == 0 { + t.Fatalf("write should stamp timestamps and pid: %+v", rec) + } + + got, err := ops.Read(rec.ID) + if err != nil { + t.Fatalf("read: %v", err) + } + if !reflect.DeepEqual(got, rec) { + t.Fatalf("round-trip mismatch:\ngot %+v\nwant %+v", got, rec) + } + + // Ordered listing. + rec2 := &OperationRecord{Kind: OpDelete, Workspace: "old"} + if err := ops.Write(rec2); err != nil { + t.Fatalf("write 2: %v", err) + } + list, err := ops.List() + if err != nil { + t.Fatalf("list: %v", err) + } + if len(list) != 2 { + t.Fatalf("expected 2 records, got %d", len(list)) + } + if !(list[0].ID < list[1].ID) { + t.Fatalf("records not sorted by id: %v", []string{list[0].ID, list[1].ID}) + } + + // Delete is idempotent. + if err := ops.Delete(rec.ID); err != nil { + t.Fatalf("delete: %v", err) + } + if err := ops.Delete(rec.ID); err != nil { + t.Fatalf("second delete should be a no-op: %v", err) + } + if _, err := ops.Read(rec.ID); err == nil { + t.Fatal("expected read of deleted record to fail") + } +} + +func TestOperationRecordSurvivesProcessExit(t *testing.T) { + dir := t.TempDir() + groveDir := filepath.Join(dir, ".grove") + if err := os.MkdirAll(groveDir, 0o755); err != nil { + t.Fatal(err) + } + ready := filepath.Join(dir, "recid") + + cmd := runHelper(t, "writeop", + envHelperDir+"="+groveDir, + envHelperName+"=crashed-ws", + envHelperFile+"="+ready, + ) + if err := cmd.Run(); err != nil { + t.Fatalf("writeop helper failed: %v", err) + } + + idBytes, err := os.ReadFile(ready) + if err != nil { + t.Fatalf("read helper id: %v", err) + } + id := string(idBytes) + + ops := NewOperationStore(groveDir) + got, err := ops.Read(id) + if err != nil { + t.Fatalf("record should survive helper exit: %v", err) + } + if got.Workspace != "crashed-ws" || got.Kind != OpCreate { + t.Fatalf("unexpected record: %+v", got) + } + if len(got.Repos) != 1 || got.Repos[0].WorktreeOwnership != OwnCreated { + t.Fatalf("resource ownership not preserved: %+v", got.Repos) + } +} + +func TestOperationRecordUnsupportedVersion(t *testing.T) { + dir := t.TempDir() + ops := NewOperationStore(dir) + rec := &OperationRecord{Kind: OpCreate, Workspace: "future"} + if err := ops.Write(rec); err != nil { + t.Fatalf("write: %v", err) + } + got, _ := ops.Read(rec.ID) + // Only the exact current version with a known kind is supported. + for _, v := range []int{operationRecordVersion + 1, 0, -1} { + got.Version = v + if got.Supported() { + t.Fatalf("version %d must not be supported", v) + } + } + got.Version = operationRecordVersion + got.Kind = "bogus" + if got.Supported() { + t.Fatal("unknown-kind record must not be supported") + } +} + +func TestOperationStoreRejectsUnknownKindAndBadID(t *testing.T) { + dir := t.TempDir() + ops := NewOperationStore(dir) + if err := ops.Write(&OperationRecord{Kind: "nonsense", Workspace: "x"}); err == nil { + t.Fatal("expected error writing unknown kind") + } + if err := ops.Write(&OperationRecord{Kind: "", Workspace: "x"}); err == nil { + t.Fatal("expected error writing empty kind") + } + if _, err := ops.Read("../escape"); err == nil { + t.Fatal("expected error reading id with path escape") + } + if err := ops.Delete("a/b"); err == nil { + t.Fatal("expected error deleting id with separator") + } +} + +func TestOperationRecordProvisioningAndRenameRoundTrip(t *testing.T) { + dir := t.TempDir() + ops := NewOperationStore(dir) + rec := &OperationRecord{ + Kind: OpRename, + Workspace: "new-name", + RenameFrom: "old-name", + RenameTo: "new-name", + RenameFromPath: "/ws/old-name", + RenameToPath: "/ws/new-name", + Repos: []RepoOperation{ + {RepoName: "api", BaseBranch: "stage", Mode: ProvisionFromBase, Status: RepoDone}, + {RepoName: "web", BaseBranch: "main", Mode: ProvisionTrack, Status: RepoDone}, + }, + } + if err := ops.Write(rec); err != nil { + t.Fatalf("write: %v", err) + } + got, err := ops.Read(rec.ID) + if err != nil { + t.Fatalf("read: %v", err) + } + if got.RenameFrom != "old-name" || got.RenameToPath != "/ws/new-name" { + t.Fatalf("rename identity not preserved: %+v", got) + } + if got.Repos[0].BaseBranch != "stage" || got.Repos[1].Mode != ProvisionTrack { + t.Fatalf("per-repo base/mode not preserved: %+v", got.Repos) + } +} + +func TestOperationRecordRootOwnershipAndSourceRoundTrip(t *testing.T) { + dir := t.TempDir() + ops := NewOperationStore(dir) + rec := &OperationRecord{ + Kind: OpCreate, + Workspace: "feat", + Path: "/ws/feat", + RootOwnership: OwnCreated, + Source: &models.WorkspaceSource{ + Provider: "github", + URL: "https://github.com/o/r/pull/7", + Ref: "7", + Title: "Add login", + }, + } + if err := ops.Write(rec); err != nil { + t.Fatalf("write: %v", err) + } + got, err := ops.Read(rec.ID) + if err != nil { + t.Fatalf("read: %v", err) + } + if got.RootOwnership != OwnCreated { + t.Fatalf("root ownership not preserved: %+v", got) + } + if got.Source == nil || got.Source.Provider != "github" || got.Source.Ref != "7" { + t.Fatalf("source provenance not preserved: %+v", got.Source) + } +} + +func TestOperationRetryAfterDirSyncFailure(t *testing.T) { + dir := filepath.Join(t.TempDir(), ".grove") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + ops := NewOperationStore(dir) // operations/ absent + + // First write fails at the directory-entry sync (dir may be created). + boom := errors.New("dirsync") + ops.failDirEntrySync = func() error { return boom } + if err := ops.Write(&OperationRecord{Kind: OpCreate, Workspace: "x"}); err != boom { + t.Fatalf("expected dir-sync error, got %v", err) + } + + // Retry with the fault cleared must still sync the parent (mkdirAllSync must + // not skip syncing just because the directory now exists) and succeed. + ops.failDirEntrySync = nil + rec := &OperationRecord{Kind: OpCreate, Workspace: "x"} + if err := ops.Write(rec); err != nil { + t.Fatalf("retry write: %v", err) + } + if _, err := ops.Read(rec.ID); err != nil { + t.Fatalf("record should exist after retry: %v", err) + } +} + +func TestOperationDeleteAbsentDirIdempotent(t *testing.T) { + dir := filepath.Join(t.TempDir(), ".grove", "operations") + ops := &OperationStore{Dir: dir} // dir never created + if err := ops.Delete("20260101T000000.000000000-000000000001-create-x-abcdef012345"); err != nil { + t.Fatalf("delete on absent journal dir must be a no-op: %v", err) + } +} + +func TestOperationWriteRejectsUnsupportedVersion(t *testing.T) { + dir := t.TempDir() + ops := NewOperationStore(dir) + // A nonzero, non-current version must be refused, not silently rewritten. + if err := ops.Write(&OperationRecord{Version: operationRecordVersion + 1, Kind: OpCreate, Workspace: "x"}); err == nil { + t.Fatal("expected refusal to write unsupported version") + } + // Version 0 is defaulted to the current version. + rec := &OperationRecord{Kind: OpCreate, Workspace: "x"} + if err := ops.Write(rec); err != nil { + t.Fatalf("write v0: %v", err) + } + if rec.Version != operationRecordVersion { + t.Fatalf("version should default to %d, got %d", operationRecordVersion, rec.Version) + } +} + +func TestOperationDeletePropagatesStatError(t *testing.T) { + // A journal "directory" that is actually a file makes Stat succeed but is not + // a real absence; more importantly a permission error on the parent must + // surface. Simulate by pointing Dir at a path whose parent denies traversal. + if os.Getuid() == 0 { + t.Skip("root bypasses permission checks") + } + base := t.TempDir() + locked := filepath.Join(base, "locked") + if err := os.MkdirAll(filepath.Join(locked, "operations"), 0o755); err != nil { + t.Fatal(err) + } + ops := NewOperationStore(locked) + // Remove traversal permission on the parent so Stat on operations/ fails with + // EACCES rather than ENOENT. + if err := os.Chmod(locked, 0o000); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(locked, 0o755) }) + err := ops.Delete("20260101T000000.000000000-000000000001-create-x-abcdef012345") + if err == nil { + t.Fatal("expected non-absence stat error to propagate from Delete") + } + if os.IsNotExist(err) { + t.Fatalf("error should not be treated as absence: %v", err) + } +} + +func TestOperationDirSyncFailurePropagates(t *testing.T) { + // Creation-time directory sync failure surfaces from Write. + t.Run("create", func(t *testing.T) { + dir := filepath.Join(t.TempDir(), ".grove") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + ops := NewOperationStore(dir) // operations/ does not exist yet + boom := errors.New("dirsync") + ops.failDirEntrySync = func() error { return boom } + if err := ops.Write(&OperationRecord{Kind: OpCreate, Workspace: "x"}); err != boom { + t.Fatalf("expected dir-sync error from Write, got %v", err) + } + }) + // Delete-time directory sync failure surfaces from Delete. + t.Run("delete", func(t *testing.T) { + dir := t.TempDir() + ops := NewOperationStore(dir) + rec := &OperationRecord{Kind: OpSync, Workspace: "x"} + if err := ops.Write(rec); err != nil { + t.Fatalf("write: %v", err) + } + boom := errors.New("dirsync") + ops.failDirEntrySync = func() error { return boom } + if err := ops.Delete(rec.ID); err != boom { + t.Fatalf("expected dir-sync error from Delete, got %v", err) + } + }) +} + +func TestOperationRecordCommitStatusAndOwnershipRoundTrip(t *testing.T) { + dir := t.TempDir() + ops := NewOperationStore(dir) + rec := &OperationRecord{ + Kind: OpDelete, + Workspace: "doomed", + Path: "/ws/doomed", + Force: true, + CommitStatus: CommitAttempted, + Repos: []RepoOperation{{ + RepoName: "api", + Status: RepoFailed, + BranchOwnership: OwnPreexisting, + WorktreeOwnership: OwnCreated, + ErrorCode: "WORKTREE_DIRTY", + Error: "uncommitted changes", + Retryable: true, + }}, + } + if err := ops.Write(rec); err != nil { + t.Fatalf("write: %v", err) + } + got, err := ops.Read(rec.ID) + if err != nil { + t.Fatalf("read: %v", err) + } + if got.CommitStatus != CommitAttempted || !got.Force || got.Path != "/ws/doomed" { + t.Fatalf("top-level repair fields not preserved: %+v", got) + } + r := got.Repos[0] + if r.BranchOwnership != OwnPreexisting || r.WorktreeOwnership != OwnCreated || r.ErrorCode != "WORKTREE_DIRTY" { + t.Fatalf("per-repo ownership/error not preserved: %+v", r) + } +} + +func TestNewOperationIDIsTimeOrdered(t *testing.T) { + prev := "" + for i := 0; i < 50; i++ { + id := NewOperationID(OpCreate, "ws") + if id <= prev { + t.Fatalf("ids not strictly increasing: %q then %q", prev, id) + } + prev = id + } +} + +func TestOperationStoreNoLeakedTempAfterWrite(t *testing.T) { + dir := t.TempDir() + ops := NewOperationStore(dir) + if err := ops.Write(&OperationRecord{Kind: OpSync, Workspace: "x"}); err != nil { + t.Fatalf("write: %v", err) + } + matches, _ := filepath.Glob(filepath.Join(ops.Dir, ".*.tmp")) + if len(matches) != 0 { + t.Fatalf("leaked temp files: %v", matches) + } + // A record in flight (dotfile) must be ignored by List. + if err := os.WriteFile(filepath.Join(ops.Dir, ".partial.json"), []byte("{"), 0o644); err != nil { + t.Fatal(err) + } + list, err := ops.List() + if err != nil { + t.Fatalf("list: %v", err) + } + if len(list) != 1 { + t.Fatalf("expected in-flight dotfile ignored, got %d records", len(list)) + } +} diff --git a/internal/state/reentrancy.go b/internal/state/reentrancy.go new file mode 100644 index 0000000..363a8a8 --- /dev/null +++ b/internal/state/reentrancy.go @@ -0,0 +1,66 @@ +package state + +import ( + "runtime" + "strconv" + "strings" + "sync" +) + +// The state lock is a cross-process advisory (flock) lock. Within a single +// process, two goroutines opening it via separate file descriptors still +// contend, so they serialize correctly. The only case flock cannot distinguish +// is same-goroutine reentrancy, which would otherwise self-block for the full +// timeout. This registry tracks which goroutine currently holds each lock path +// so a nested call fails fast instead. + +var ( + heldMu sync.Mutex + held = map[string]int64{} // lockPath -> owning goroutine id +) + +func markHeld(lockPath string) { + id, ok := goid() + if !ok { + return + } + heldMu.Lock() + held[lockPath] = id + heldMu.Unlock() +} + +func clearHeld(lockPath string) { + heldMu.Lock() + delete(held, lockPath) + heldMu.Unlock() +} + +func heldByCurrentGoroutine(lockPath string) bool { + id, ok := goid() + if !ok { + return false // parsing failed; fall back to flock-only behavior + } + heldMu.Lock() + owner, held := held[lockPath] + heldMu.Unlock() + return held && owner == id +} + +// goid returns the current goroutine id by parsing the runtime stack header +// ("goroutine [running]:"). This is not an official API; on a parse +// failure it returns ok=false so callers fall back to flock-only serialization +// rather than risking a false reentrancy match. +func goid() (int64, bool) { + var buf [64]byte + n := runtime.Stack(buf[:], false) + s := string(buf[:n]) + s = strings.TrimPrefix(s, "goroutine ") + if i := strings.IndexByte(s, ' '); i >= 0 { + s = s[:i] + } + id, err := strconv.ParseInt(s, 10, 64) + if err != nil { + return 0, false + } + return id, true +} diff --git a/internal/state/state.go b/internal/state/state.go index b30444e..a1978f9 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -1,20 +1,34 @@ package state import ( + "context" "encoding/json" + "errors" "fmt" "os" "path/filepath" "strings" + "time" "github.com/nicksenap/grove/internal/config" "github.com/nicksenap/grove/internal/models" ) +// lockTimeout bounds how long a mutation waits for the advisory state lock +// before returning a retryable STATE_LOCK_TIMEOUT. +const lockTimeout = 30 * time.Second + // Store manages workspace state persistence. // Use NewStore for production; create directly in tests. type Store struct { Path string // path to state.json + + // Test-only fault-injection seams. They are unexported, so only same-package + // test code can set them; release binaries can never enable a failpoint. + failWrite func() error // injected before writing temp bytes + failSync func() error // injected before fsyncing the temp file + failRename func() error // injected before the atomic rename + failDirSync func() error // injected before fsyncing the parent directory } // NewStore creates a Store using the given grove dir. @@ -22,7 +36,15 @@ func NewStore(groveDir string) *Store { return &Store{Path: filepath.Join(groveDir, "state.json")} } -// Load reads all workspaces from state.json. +// dir returns the directory containing state.json. +func (s *Store) dir() string { return filepath.Dir(s.Path) } + +// lockPath is the advisory lock file guarding all state mutations. It is never +// unlinked so that flock ownership stays stable across processes. +func (s *Store) lockPath() string { return filepath.Join(s.dir(), "state.lock") } + +// Load reads all workspaces from state.json. Reads are lock-free: writes use an +// atomic rename so a reader always observes a complete snapshot. func (s *Store) Load() ([]models.Workspace, error) { data, err := os.ReadFile(s.Path) if err != nil { @@ -43,24 +65,211 @@ func (s *Store) Load() ([]models.Workspace, error) { return workspaces, nil } -// Save writes all workspaces to state.json atomically. -func (s *Store) Save(workspaces []models.Workspace) error { +// writeSnapshot durably writes workspaces to state.json. +func (s *Store) writeSnapshot(workspaces []models.Workspace) error { data, err := json.MarshalIndent(workspaces, "", " ") if err != nil { return err } + if err := os.MkdirAll(s.dir(), 0o755); err != nil { + return err + } + return s.writeFileDurable(s.Path, data, 0o644) +} + +// WithMutation acquires the advisory lock, loads the authoritative snapshot, +// and passes a caller-owned Mutation handle. The lock is held for the entire +// callback and released afterward. The handle is invalidated before the lock is +// released, so an escaped handle cannot commit outside serialization. Service +// code must never call a public lock-acquiring mutator while holding a handle; +// a nested call from the same goroutine is rejected immediately. +func (s *Store) WithMutation(ctx context.Context, fn func(*Mutation) error) (err error) { + if ctx == nil { + ctx = context.Background() + } + + // Reject same-goroutine reentrancy immediately rather than self-deadlocking + // on the advisory lock for the full timeout. + lp := s.lockPath() + if heldByCurrentGoroutine(lp) { + return &CodedError{ + Code: CodeStateNested, + Message: "nested state mutation is not allowed; use the existing Mutation handle", + } + } + + // The lock file lives in the grove dir, which may not exist on first use. + if err := os.MkdirAll(s.dir(), 0o755); err != nil { + return err + } + + lock, err := acquireLock(ctx, lp, lockTimeout) + if err != nil { + if errors.Is(err, errLockTimeout) { + return &CodedError{ + Code: CodeStateLockTimeout, + Message: fmt.Sprintf("could not acquire state lock within %s", lockTimeout), + Retryable: true, + } + } + return err + } + markHeld(lp) + defer func() { + clearHeld(lp) + if rerr := lock.release(); rerr != nil && err == nil { + err = rerr + } + }() + + workspaces, lerr := s.Load() + if lerr != nil { + return lerr + } + m := &Mutation{store: s, workspaces: workspaces, active: true} + defer func() { m.active = false }() // invalidate before lock release + return fn(m) +} + +// Mutation is a caller-owned handle to a locked state snapshot. All reads and +// edits operate on an in-memory copy until Commit durably persists it. A +// Mutation is only valid for the duration of the WithMutation callback. +type Mutation struct { + store *Store + workspaces []models.Workspace + active bool + committed bool +} + +// errInactive is returned when a Mutation is used outside its callback. +func (m *Mutation) checkActive() error { + if !m.active { + return &CodedError{ + Code: CodeStateInactiveHandle, + Message: "mutation handle used outside its WithMutation callback", + } + } + return nil +} + +// cloneWorkspace returns a deep copy so callers cannot mutate the locked +// snapshot through returned aliases. +func cloneWorkspace(ws models.Workspace) models.Workspace { + out := ws + out.Repos = append([]models.RepoWorktree(nil), ws.Repos...) + if ws.Source != nil { + src := *ws.Source + out.Source = &src + } + return out +} + +// Workspaces returns a deep copy of the current in-memory snapshot (post-edit). +func (m *Mutation) Workspaces() []models.Workspace { + out := make([]models.Workspace, len(m.workspaces)) + for i := range m.workspaces { + out[i] = cloneWorkspace(m.workspaces[i]) + } + return out +} + +// Get returns a deep copy of the workspace by name, or nil if absent. +func (m *Mutation) Get(name string) *models.Workspace { + for i := range m.workspaces { + if m.workspaces[i].Name == name { + ws := cloneWorkspace(m.workspaces[i]) + return &ws + } + } + return nil +} + +// Exists reports whether a workspace with the given name is present. +func (m *Mutation) Exists(name string) bool { + for i := range m.workspaces { + if m.workspaces[i].Name == name { + return true + } + } + return false +} + +// Add appends a workspace to the in-memory snapshot. It returns a conflict +// CodedError if a workspace with the same name already exists. +func (m *Mutation) Add(ws models.Workspace) error { + if err := m.checkActive(); err != nil { + return err + } + if m.Exists(ws.Name) { + return &CodedError{ + Code: CodeStateConflict, + Message: fmt.Sprintf("workspace %q already exists", ws.Name), + } + } + m.workspaces = append(m.workspaces, ws) + return nil +} + +// Update replaces a workspace matched by name. +func (m *Mutation) Update(ws models.Workspace) error { + return m.UpdateByName(ws, ws.Name) +} + +// UpdateByName replaces a workspace matched by matchName (enables renames where +// ws.Name is already the new name). If the new name differs from matchName and +// already belongs to another workspace, a conflict is returned. +func (m *Mutation) UpdateByName(ws models.Workspace, matchName string) error { + if err := m.checkActive(); err != nil { + return err + } + if ws.Name != matchName && m.Exists(ws.Name) { + return &CodedError{ + Code: CodeStateConflict, + Message: fmt.Sprintf("workspace %q already exists", ws.Name), + } + } + for i := range m.workspaces { + if m.workspaces[i].Name == matchName { + m.workspaces[i] = ws + return nil + } + } + return fmt.Errorf("workspace %s not found", matchName) +} - if err := os.MkdirAll(filepath.Dir(s.Path), 0o755); err != nil { +// Remove deletes a workspace by name (no error if absent). +func (m *Mutation) Remove(name string) error { + if err := m.checkActive(); err != nil { return err } + filtered := make([]models.Workspace, 0, len(m.workspaces)) + for _, ws := range m.workspaces { + if ws.Name != name { + filtered = append(filtered, ws) + } + } + m.workspaces = filtered + return nil +} - tmp := s.Path + ".tmp" - if err := os.WriteFile(tmp, data, 0o644); err != nil { +// Commit durably persists the in-memory snapshot. It is idempotent within a +// single mutation and fails if the handle is no longer active. +func (m *Mutation) Commit() error { + if err := m.checkActive(); err != nil { return err } - return os.Rename(tmp, s.Path) + if m.committed { + return nil + } + if err := m.store.writeSnapshot(m.workspaces); err != nil { + return err + } + m.committed = true + return nil } +// --- Public Store mutators: thin wrappers over WithMutation --- + // GetWorkspace finds a workspace by name. func (s *Store) GetWorkspace(name string) (*models.Workspace, error) { workspaces, err := s.Load() @@ -77,80 +286,69 @@ func (s *Store) GetWorkspace(name string) (*models.Workspace, error) { // AddWorkspace adds a workspace to state. func (s *Store) AddWorkspace(ws models.Workspace) error { - workspaces, err := s.Load() - if err != nil { - return err - } - workspaces = append(workspaces, ws) - return s.Save(workspaces) + return s.WithMutation(context.Background(), func(m *Mutation) error { + if err := m.Add(ws); err != nil { + return err + } + return m.Commit() + }) } // UpdateWorkspace replaces a workspace by name. func (s *Store) UpdateWorkspace(ws models.Workspace) error { - workspaces, err := s.Load() - if err != nil { - return err - } - for i := range workspaces { - if workspaces[i].Name == ws.Name { - workspaces[i] = ws - return s.Save(workspaces) + return s.WithMutation(context.Background(), func(m *Mutation) error { + if err := m.Update(ws); err != nil { + return err } - } - return fmt.Errorf("workspace %s not found", ws.Name) + return m.Commit() + }) } // RemoveWorkspace removes a workspace by name. func (s *Store) RemoveWorkspace(name string) error { - workspaces, err := s.Load() - if err != nil { - return err - } - filtered := make([]models.Workspace, 0, len(workspaces)) - for _, ws := range workspaces { - if ws.Name != name { - filtered = append(filtered, ws) + return s.WithMutation(context.Background(), func(m *Mutation) error { + if err := m.Remove(name); err != nil { + return err } - } - return s.Save(filtered) + return m.Commit() + }) } // UpdateWorkspaceByName replaces a workspace matched by matchName. // This enables atomic renames: ws.Name is already the new name, matchName is the old. func (s *Store) UpdateWorkspaceByName(ws models.Workspace, matchName string) error { - workspaces, err := s.Load() - if err != nil { - return err - } - for i := range workspaces { - if workspaces[i].Name == matchName { - workspaces[i] = ws - return s.Save(workspaces) + return s.WithMutation(context.Background(), func(m *Mutation) error { + if err := m.UpdateByName(ws, matchName); err != nil { + return err } - } - return fmt.Errorf("workspace %s not found", matchName) + return m.Commit() + }) } // RenameWorkspace renames a workspace in state and updates paths. func (s *Store) RenameWorkspace(oldName, newName, newPath string) error { - workspaces, err := s.Load() - if err != nil { - return err - } - for i := range workspaces { - if workspaces[i].Name == oldName { - oldPath := workspaces[i].Path - workspaces[i].Name = newName - workspaces[i].Path = newPath - for j := range workspaces[i].Repos { - workspaces[i].Repos[j].WorktreePath = strings.Replace( - workspaces[i].Repos[j].WorktreePath, oldPath, newPath, 1, - ) + return s.WithMutation(context.Background(), func(m *Mutation) error { + if oldName != newName && m.Exists(newName) { + return &CodedError{ + Code: CodeStateConflict, + Message: fmt.Sprintf("workspace %q already exists", newName), } - return s.Save(workspaces) } - } - return fmt.Errorf("workspace %s not found", oldName) + for i := range m.workspaces { + if m.workspaces[i].Name == oldName { + oldPath := m.workspaces[i].Path + m.workspaces[i].Name = newName + m.workspaces[i].Path = newPath + for j := range m.workspaces[i].Repos { + m.workspaces[i].Repos[j].WorktreePath = strings.Replace( + m.workspaces[i].Repos[j].WorktreePath, oldPath, newPath, 1, + ) + } + return m.Commit() + } + } + return fmt.Errorf("workspace %s not found", oldName) + }) } // FindWorkspaceByPath finds a workspace containing the given path. @@ -194,7 +392,6 @@ func defaultStore() *Store { // StatePath returns the path to state.json. func StatePath() string { return defaultStore().Path } func Load() ([]models.Workspace, error) { return defaultStore().Load() } -func Save(workspaces []models.Workspace) error { return defaultStore().Save(workspaces) } func GetWorkspace(name string) (*models.Workspace, error) { return defaultStore().GetWorkspace(name) } func AddWorkspace(ws models.Workspace) error { return defaultStore().AddWorkspace(ws) } func UpdateWorkspace(ws models.Workspace) error { return defaultStore().UpdateWorkspace(ws) } diff --git a/internal/state/state_test.go b/internal/state/state_test.go index a0d0890..aa9c860 100644 --- a/internal/state/state_test.go +++ b/internal/state/state_test.go @@ -278,6 +278,11 @@ func TestAtomicWrite(t *testing.T) { tmpPath := s.Path + ".tmp" if _, err := os.Stat(tmpPath); !os.IsNotExist(err) { - t.Error("temp file should be cleaned up") + t.Error("legacy temp file should not exist") + } + // No uniquely named temp files should leak either. + matches, _ := filepath.Glob(filepath.Join(filepath.Dir(s.Path), ".*.tmp")) + if len(matches) != 0 { + t.Errorf("leaked temp files: %v", matches) } } diff --git a/internal/workspace/backend_test.go b/internal/workspace/backend_test.go new file mode 100644 index 0000000..d27cdb4 --- /dev/null +++ b/internal/workspace/backend_test.go @@ -0,0 +1,135 @@ +package workspace + +import ( + "errors" + "os" + "path/filepath" + "testing" +) + +// faultBackend wraps a real mutationBackend and can inject a failure either +// before or after a named phase is applied, proving the seam supports both +// precondition failures and "applied then failed" post-mutation failures. +type faultBackend struct { + inner mutationBackend + failBefore map[string]error + failAfter map[string]error + appliedRepo []string // repos whose WorktreeAdd actually ran +} + +func newFaultBackend(inner mutationBackend) *faultBackend { + return &faultBackend{ + inner: inner, + failBefore: map[string]error{}, + failAfter: map[string]error{}, + } +} + +func (f *faultBackend) CreateBranch(repo, branch, start string) error { + if err := f.failBefore["CreateBranch:"+repo]; err != nil { + return err + } + if err := f.inner.CreateBranch(repo, branch, start); err != nil { + return err + } + return f.failAfter["CreateBranch:"+repo] +} +func (f *faultBackend) DeleteBranch(repo, branch string, force bool) error { + if err := f.failBefore["DeleteBranch:"+repo]; err != nil { + return err + } + return f.inner.DeleteBranch(repo, branch, force) +} +func (f *faultBackend) WorktreeAdd(repo, path, branch string) error { + if err := f.failBefore["WorktreeAdd:"+repo]; err != nil { + return err + } + if err := f.inner.WorktreeAdd(repo, path, branch); err != nil { + return err + } + f.appliedRepo = append(f.appliedRepo, repo) + return f.failAfter["WorktreeAdd:"+repo] +} +func (f *faultBackend) WorktreeAddTracking(repo, path, branch string) error { + if err := f.failBefore["WorktreeAddTracking:"+repo]; err != nil { + return err + } + return f.inner.WorktreeAddTracking(repo, path, branch) +} +func (f *faultBackend) WorktreeRemove(repo, path string) error { + if err := f.failBefore["WorktreeRemove:"+repo]; err != nil { + return err + } + return f.inner.WorktreeRemove(repo, path) +} +func (f *faultBackend) WorktreeRepair(repo, path string) error { + return f.inner.WorktreeRepair(repo, path) +} +func (f *faultBackend) Mkdir(path string, perm os.FileMode) error { + if err := f.failBefore["Mkdir:"+path]; err != nil { + return err + } + if err := f.inner.Mkdir(path, perm); err != nil { + return err + } + return f.failAfter["Mkdir:"+path] +} +func (f *faultBackend) RemoveAll(path string) error { + if err := f.failBefore["RemoveAll:"+path]; err != nil { + return err + } + return f.inner.RemoveAll(path) +} +func (f *faultBackend) Rename(o, n string) error { return f.inner.Rename(o, n) } + +// TestMutationBackendInstanceScoped proves failpoints are per-Service instance +// and can model both before- and after-mutation failures. +func TestMutationBackendInstanceScoped(t *testing.T) { + env := setupTestEnv(t) + + // A default service uses the production backend (no faults). + if _, ok := env.svc.mut().(prodBackend); !ok { + // setupTestEnv leaves backend nil, so mut() must default to prod. + t.Fatalf("expected default prod backend, got %T", env.svc.mut()) + } + + // Install a fault-injecting backend on this instance only. + fb := newFaultBackend(prodBackend{}) + env.svc.backend = fb + + repoPath := env.createRepo("api") + dst := filepath.Join(t.TempDir(), "wt") + if err := fb.CreateBranch(repoPath, "feat/x", ""); err != nil { + t.Fatalf("create branch: %v", err) + } + + // Before-failure: the mutation is not applied. + sentinel := errors.New("pre") + fb.failBefore["WorktreeAdd:"+repoPath] = sentinel + if err := env.svc.mut().WorktreeAdd(repoPath, dst, "feat/x"); !errors.Is(err, sentinel) { + t.Fatalf("expected before-failure, got %v", err) + } + if _, err := os.Stat(dst); !os.IsNotExist(err) { + t.Fatal("worktree should not exist after a before-failure") + } + + // After-failure: the mutation IS applied, then an error is returned. + delete(fb.failBefore, "WorktreeAdd:"+repoPath) + after := errors.New("post") + fb.failAfter["WorktreeAdd:"+repoPath] = after + if err := env.svc.mut().WorktreeAdd(repoPath, dst, "feat/x"); !errors.Is(err, after) { + t.Fatalf("expected after-failure, got %v", err) + } + if _, err := os.Stat(dst); err != nil { + t.Fatalf("worktree should exist after an after-failure (mutation applied): %v", err) + } + if len(fb.appliedRepo) != 1 { + t.Fatalf("expected exactly one applied WorktreeAdd, got %v", fb.appliedRepo) + } + + // A fresh service is unaffected — failpoints are instance-scoped. + other := setupTestEnv(t) + if _, ok := other.svc.mut().(prodBackend); !ok { + t.Fatalf("fresh service must not inherit faults, got %T", other.svc.mut()) + } +} diff --git a/internal/workspace/create.go b/internal/workspace/create.go new file mode 100644 index 0000000..bb0a131 --- /dev/null +++ b/internal/workspace/create.go @@ -0,0 +1,766 @@ +package workspace + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" + "strings" + "sync" + + "github.com/nicksenap/grove/internal/console" + "github.com/nicksenap/grove/internal/gitops" + "github.com/nicksenap/grove/internal/logging" + "github.com/nicksenap/grove/internal/models" + "github.com/nicksenap/grove/internal/state" +) + +// provisionOutcome records what provisioning a single repo actually did, so +// compensation touches only operation-created resources. +type provisionOutcome struct { + rw models.RepoWorktree + branchOwnership state.ResourceOwnership + worktreeOwnership state.ResourceOwnership + mode state.ProvisionMode + baseBranch string +} + +// CreateWithResult creates a workspace transactionally and returns an ordered, +// typed outcome. It writes a recovery record before the first Git/filesystem +// mutation, provisions every repo (tracking resource ownership), commits state +// exactly once under the state lock, and on any failure compensates in reverse +// order — deleting only operation-created branches/worktrees and preserving +// pre-existing ones. If compensation cannot complete, the recovery record is +// retained and the result is Pending. +func (s *Service) CreateWithResult(name string, opts CreateOpts) *OperationResult { + res := &OperationResult{Kind: state.OpCreate, Workspace: name} + + // 1. Validate inputs before any mutation or optional cloning. + sourcePaths, err := s.validateCreateInputs(name, opts) + if err != nil { + return res.fail(err) + } + + // Reserve this workspace name for the whole critical section (reconcile, + // provision, commit, compensate) so two same-name creates cannot corrupt a + // shared root or each other's recovery journal. Different names still run + // concurrently, and the global state lock is only taken briefly at commit. + release, err := s.State.AcquireWorkspaceLock(context.Background(), name) + if err != nil { + return res.fail(err) + } + released := false + releaseOnce := func() { + if !released { + released = true + release() + } + } + defer releaseOnce() + + // Re-check duplicate now that we hold the reservation, but resume a stale + // record FIRST so a crash after a prior commit is reconciled (record cleared) + // rather than masked by a bare "already exists". + if err := s.resumeStaleCreate(name); err != nil { + return res.fail(fmt.Errorf("resolving prior interrupted create: %w", err)) + } + if existing, err := s.State.GetWorkspace(name); err != nil { + return res.fail(err) + } else if existing != nil { + return res.fail(fmt.Errorf("workspace %s already exists", name)) + } + + wsPath := filepath.Join(opts.Cfg.WorkspaceDir, name) + + // 3. Write the recovery record before the first Git/filesystem mutation. + rec := newCreateRecord(name, wsPath, opts, sourcePaths) + if err := s.ops().Write(rec); err != nil { + return res.fail(fmt.Errorf("writing recovery record: %w", err)) + } + + logging.Info("creating workspace %q (branch=%s, repos=%v)", name, opts.Branch, opts.Repos) + + // 4. Create the workspace root using write-ahead ownership: record whether we + // intend to own the root BEFORE mkdir, so a crash mid-create is recoverable. + rootOwnership := state.OwnCreated + if _, err := os.Stat(wsPath); err == nil { + rootOwnership = state.OwnPreexisting + } + rec.RootOwnership = rootOwnership + if err := s.ops().Write(rec); err != nil { + _ = s.ops().Delete(rec.ID) + return res.fail(fmt.Errorf("persisting recovery record: %w", err)) + } + if err := s.mut().Mkdir(wsPath, 0o755); err != nil { + // Reconcile a possible applied-then-error mkdir: compensate (idempotent) + // then clear the record only if the root is gone. + return s.compensateCreateResult(res, rec, wsPath, rootOwnership, nil, + fmt.Errorf("creating workspace dir: %w", err)) + } + + // 5. Parallel fetch (outside the state lock — the slow network part). + s.fetchRepos(opts.Repos, sourcePaths) + + // 6. Provision every repo; on failure this compensates and returns. + outs, failed := s.provisionAll(res, rec, opts, sourcePaths, wsPath, rootOwnership) + if failed != nil { + return failed + } + + // 7. Commit state exactly once under the lock (authoritative dup check). + ws := models.NewWorkspace(name, wsPath, opts.Branch) + ws.Source = opts.Source + ws.Repos = collectWorktrees(outs) + rec.Phase = "commit" + rec.CommitStatus = state.CommitAttempted + if err := s.ops().Write(rec); err != nil { + // Do not commit if we cannot durably record the attempt. + return s.compensateCreateResult(res, rec, wsPath, rootOwnership, outs, + fmt.Errorf("persisting commit intent: %w", err)) + } + + if commitErr := s.withMutation(context.Background(), func(m *state.Mutation) error { + return m.Add(ws) + }); commitErr != nil { + // A genuine same-name conflict means another workspace owns the name; our + // provisioned resources must be compensated. + if state.CodeOf(commitErr) == state.CodeStateConflict { + return s.compensateCreateResult(res, rec, wsPath, rootOwnership, outs, commitErr) + } + // Otherwise the commit may have applied then returned an error (ambiguous, + // e.g. a directory-sync failure after rename). Reconcile against + // authoritative state while still holding the per-workspace reservation. + return s.reconcileAmbiguousCommit(res, rec, ws, wsPath, rootOwnership, outs, commitErr, releaseOnce) + } + rec.CommitStatus = state.CommitDone + _ = s.ops().Write(rec) + + return s.finishCreateSuccess(res, rec, ws, wsPath, releaseOnce) +} + +// reconcileAmbiguousCommit handles a state-commit that returned an error but may +// have applied. It verifies workspace identity under the reservation: a matching +// present workspace is treated as committed; a read error or identity mismatch +// retains the CommitAttempted record as Pending; a confirmed absence compensates. +func (s *Service) reconcileAmbiguousCommit(res *OperationResult, rec *state.OperationRecord, ws models.Workspace, + wsPath string, rootOwnership state.ResourceOwnership, outs []*provisionOutcome, cause error, releaseOnce func()) *OperationResult { + + existing, err := s.State.GetWorkspace(ws.Name) + if err != nil { + // Cannot prove state; keep the record for reconciliation rather than risk + // destroying a committed workspace. + rec.LastError = fmt.Sprintf("ambiguous commit; state unreadable: %v", cause) + rec.Retryable = true + _ = s.ops().Write(rec) + res.Status = OutcomePending + res.RecordID = rec.ID + res.Err = cause + res.Message = "state commit ambiguous and unreadable — run: gw doctor" + return res + } + if existing != nil && workspaceMatches(existing, ws) { + rec.CommitStatus = state.CommitDone + _ = s.ops().Write(rec) + return s.finishCreateSuccess(res, rec, ws, wsPath, releaseOnce) + } + if existing != nil { + // A different workspace holds the name; ours did not commit — compensate. + return s.compensateCreateResult(res, rec, wsPath, rootOwnership, outs, cause) + } + return s.compensateCreateResult(res, rec, wsPath, rootOwnership, outs, cause) +} + +// workspaceMatches reports whether the persisted workspace matches the one this +// operation intended to commit (path, branch, and full repo identity). +func workspaceMatches(got *models.Workspace, want models.Workspace) bool { + if got.Path != want.Path || got.Branch != want.Branch || len(got.Repos) != len(want.Repos) { + return false + } + type ident struct{ src, wt, br string } + have := map[string]ident{} + for _, r := range got.Repos { + have[r.RepoName] = ident{r.SourceRepo, r.WorktreePath, r.Branch} + } + for _, r := range want.Repos { + got, ok := have[r.RepoName] + if !ok || got.src != r.SourceRepo || got.wt != r.WorktreePath || got.br != r.Branch { + return false + } + } + return true +} + +// finishCreateSuccess clears the record, releases the workspace reservation, and +// runs post-commit best-effort steps (stats, mcp, and setup hooks). Setup hooks +// run AFTER the reservation is released so a hook that touches the same +// workspace cannot self-block. +func (s *Service) finishCreateSuccess(res *OperationResult, rec *state.OperationRecord, ws models.Workspace, wsPath string, releaseOnce func()) *OperationResult { + if err := s.ops().Delete(rec.ID); err != nil { + logging.Warn("workspace %q created but recovery record %s could not be cleared: %v", ws.Name, rec.ID, err) + } + + s.Stats.RecordCreated(ws) + writeMCPConfig(ws) + logging.Info("workspace %q created at %s", ws.Name, wsPath) + console.Successf("Workspace %s created at %s", ws.Name, wsPath) + if cdFile := os.Getenv("GROVE_CD_FILE"); cdFile != "" { + _ = os.WriteFile(cdFile, []byte(wsPath), 0o644) + } + + // Release the reservation before setup hooks (hooks run after commit and + // outside the lock, per the lifecycle contract). + releaseOnce() + if hookErr := s.runSetupHooksErr(ws); hookErr != nil { + res.Status = OutcomePartial + res.Err = hookErr + res.Message = "workspace created, but setup hooks failed: " + hookErr.Error() + return res + } + res.Status = OutcomeSuccess + res.Message = fmt.Sprintf("Workspace %s created at %s", ws.Name, wsPath) + return res +} + +// provisionAll provisions each repo sequentially. On the first failure it +// records the outcome, marks later repos skipped, compensates, and returns a +// non-nil terminal result. +func (s *Service) provisionAll(res *OperationResult, rec *state.OperationRecord, opts CreateOpts, + sourcePaths []string, wsPath string, rootOwnership state.ResourceOwnership) ([]*provisionOutcome, *OperationResult) { + + var outs []*provisionOutcome + for i, repoName := range opts.Repos { + console.Infof("[%d/%d] %s", i+1, len(opts.Repos), repoName) + mode := opts.BranchMode + if opts.TrackBranchRepo != "" && repoName != opts.TrackBranchRepo { + mode = BranchModeCreate + } + out, err := s.provisionRepo(rec, i, sourcePaths[i], repoName, wsPath, opts.Branch, mode) + if out != nil { + outs = append(outs, out) + } + if err != nil { + if out != nil { + rec.Repos[i] = repoOpFromOutcome(repoName, out) + } + rec.Repos[i].Status = state.RepoFailed + rec.Repos[i].Error = err.Error() + res.addRepo(RepoOutcome{RepoName: repoName, Status: state.RepoFailed, Phase: "provision", Err: err}) + // Every requested repository must appear in the result; later repos are + // skipped (not attempted). + for j := i + 1; j < len(opts.Repos); j++ { + rec.Repos[j].Status = state.RepoSkipped + res.addRepo(RepoOutcome{RepoName: opts.Repos[j], Status: state.RepoSkipped}) + } + return outs, s.compensateCreateResult(res, rec, wsPath, rootOwnership, outs, + fmt.Errorf("provisioning %s: %w", repoName, err)) + } + rec.Repos[i] = repoOpFromOutcome(repoName, out) + if werr := s.ops().Write(rec); werr != nil { + // A failed progress write means the durable journal no longer reflects + // reality; stop and compensate rather than continue with stale WAL. + res.addRepo(RepoOutcome{RepoName: repoName, Status: state.RepoDone, Phase: "provision"}) + for j := i + 1; j < len(opts.Repos); j++ { + rec.Repos[j].Status = state.RepoSkipped + res.addRepo(RepoOutcome{RepoName: opts.Repos[j], Status: state.RepoSkipped}) + } + return outs, s.compensateCreateResult(res, rec, wsPath, rootOwnership, outs, + fmt.Errorf("persisting recovery record: %w", werr)) + } + res.addRepo(RepoOutcome{RepoName: repoName, Status: state.RepoDone, Phase: "provision"}) + } + return outs, nil +} + +// validateCreateInputs validates inputs and resolves source paths, mutating +// nothing. It performs a non-authoritative early duplicate check for UX; the +// authoritative check happens under the lock at commit. +func (s *Service) validateCreateInputs(name string, opts CreateOpts) ([]string, error) { + if err := ValidateWorkspaceName(name); err != nil { + return nil, err + } + if strings.TrimSpace(opts.Branch) == "" { + return nil, errors.New("branch is required") + } + if len(opts.Repos) == 0 { + return nil, errors.New("at least one repo is required") + } + if opts.Cfg == nil { + return nil, errors.New("config is required") + } + sourcePaths := make([]string, len(opts.Repos)) + for i, repoName := range opts.Repos { + sp, ok := opts.RepoMap[repoName] + if !ok { + return nil, fmt.Errorf("repo %s not found", repoName) + } + sourcePaths[i] = sp + } + if existing, err := s.State.GetWorkspace(name); err != nil { + return nil, err + } else if existing != nil { + return nil, fmt.Errorf("workspace %s already exists", name) + } + return sourcePaths, nil +} + +// newCreateRecord builds the initial create recovery record. +func newCreateRecord(name, wsPath string, opts CreateOpts, sourcePaths []string) *state.OperationRecord { + rec := &state.OperationRecord{ + Kind: state.OpCreate, + Workspace: name, + Path: wsPath, + Source: opts.Source, + Phase: "provisioning", + } + for i, repoName := range opts.Repos { + rec.Repos = append(rec.Repos, state.RepoOperation{ + RepoName: repoName, + SourceRepo: sourcePaths[i], + Branch: opts.Branch, + Status: state.RepoPending, + }) + } + return rec +} + +// provisionRepo provisions one repo's worktree via the mutation backend using +// write-ahead ownership: it records the ownership it is about to take and the +// current phase in the recovery journal BEFORE each Git mutation, so a crash +// mid-mutation still leaves a durable record of what may have been created. +// Compensation (which checks actual existence) can then clean up exactly. +func (s *Service) provisionRepo(rec *state.OperationRecord, idx int, sourcePath, repoName, wsPath, branch string, mode BranchMode) (*provisionOutcome, error) { + wtPath := filepath.Join(wsPath, repoName) + out := &provisionOutcome{ + rw: models.RepoWorktree{RepoName: repoName, SourceRepo: sourcePath, WorktreePath: wtPath, Branch: branch}, + } + + // Serialize branch ownership for this exact (source repo, branch) pair so two + // creates for DIFFERENT workspace names that target the same branch cannot + // both classify it as operation-created and later delete each other's branch. + relBranch, err := s.State.AcquireResourceLock(context.Background(), "branch-"+sourcePath+"-"+branch) + if err != nil { + return out, err + } + defer relBranch() + + if hasWT, _ := gitops.WorktreeHasBranch(sourcePath, branch); hasWT { + return out, fmt.Errorf("branch %s already has a worktree in %s", branch, repoName) + } + + // Track mode: check out an existing remote branch (e.g. a PR head). + if mode == BranchModeTrack && !gitops.BranchExists(sourcePath, branch) { + if gitops.RemoteBranchExists(sourcePath, branch) { + logging.Info("tracking existing remote branch %q in %s", branch, repoName) + // Write-ahead: tracking creates both a local branch and a worktree. + out.mode = state.ProvisionTrack + out.branchOwnership = state.OwnCreated + out.worktreeOwnership = state.OwnCreated + if err := s.writeAhead(rec, idx, out, "track"); err != nil { + return out, err + } + if err := s.mut().WorktreeAddTracking(sourcePath, wtPath, branch); err != nil { + return out, fmt.Errorf("adding tracking worktree: %w", err) + } + return out, nil + } + console.Warningf("%s: remote branch %s not found — creating a new branch from base instead", repoName, branch) + } + + // Branch ownership: only compensate a branch this operation creates. + if gitops.BranchExists(sourcePath, branch) { + out.branchOwnership = state.OwnPreexisting + } else { + base, err := gitops.ResolveBaseBranch(sourcePath) + if err != nil { + base = "HEAD" + } + out.baseBranch = base + // Write-ahead: record that we intend to own this branch before creating it. + out.branchOwnership = state.OwnCreated + if err := s.writeAhead(rec, idx, out, "branch_create"); err != nil { + return out, err + } + if err := s.createBranchWithFallback(sourcePath, repoName, branch, base); err != nil { + return out, fmt.Errorf("creating branch: %w", err) + } + } + + // Write-ahead: record intended worktree ownership before adding it. + out.worktreeOwnership = state.OwnCreated + if err := s.writeAhead(rec, idx, out, "worktree_add"); err != nil { + return out, err + } + if err := s.mut().WorktreeAdd(sourcePath, wtPath, branch); err != nil { + return out, fmt.Errorf("adding worktree: %w", err) + } + return out, nil +} + +// writeAhead persists the repo's intended ownership/phase before a mutation. +func (s *Service) writeAhead(rec *state.OperationRecord, idx int, out *provisionOutcome, phase string) error { + op := repoOpFromOutcome(out.rw.RepoName, out) + op.Status = state.RepoInProgress + op.Phase = phase + rec.Repos[idx] = op + if err := s.ops().Write(rec); err != nil { + return fmt.Errorf("persisting recovery record: %w", err) + } + return nil +} + +// createBranchWithFallback reproduces the historical base → plain → HEAD +// fallback for branch creation via the mutation backend. +func (s *Service) createBranchWithFallback(sourcePath, repoName, branch, base string) error { + logging.Info("creating branch %q in %s from %s", branch, repoName, base) + if err := s.mut().CreateBranch(sourcePath, branch, base); err == nil { + return nil + } + plainBase := strings.TrimPrefix(base, "origin/") + if err := s.mut().CreateBranch(sourcePath, branch, plainBase); err == nil { + return nil + } + return s.mut().CreateBranch(sourcePath, branch, "HEAD") +} + +// compensateCreateResult rolls back provisioned resources in reverse order and +// finalizes the result as Failed (fully rolled back) or Pending (rollback +// incomplete — recovery record retained). +func (s *Service) compensateCreateResult(res *OperationResult, rec *state.OperationRecord, wsPath string, + rootOwnership state.ResourceOwnership, outs []*provisionOutcome, cause error) *OperationResult { + + logging.Error("workspace creation failed for %q: %v — rolling back", rec.Workspace, cause) + cerrs := s.compensateCreate(wsPath, rootOwnership, outs) + + res.Err = cause + if len(cerrs) == 0 { + if err := s.ops().Delete(rec.ID); err != nil { + logging.Warn("rollback complete but recovery record %s not cleared: %v", rec.ID, err) + } + res.Status = OutcomeFailed + res.Message = "creation failed and was rolled back: " + cause.Error() + return res + } + + rec.Phase = "compensation" + rec.Retryable = true + rec.LastError = joinErrors(cause, cerrs) + _ = s.ops().Write(rec) + res.Status = OutcomePending + res.RecordID = rec.ID + res.Message = "creation failed; automatic rollback incomplete — run: gw doctor" + return res +} + +// compensateCreate removes only operation-created resources, in reverse order, +// aggregating (not dropping) errors. It is idempotent: a resource marked as +// owned via write-ahead but never actually created (e.g. a crash before the Git +// call applied) is simply skipped, so retry converges. +func (s *Service) compensateCreate(wsPath string, rootOwnership state.ResourceOwnership, outs []*provisionOutcome) []error { + var errs []error + for i := len(outs) - 1; i >= 0; i-- { + o := outs[i] + if o == nil { + continue + } + if o.worktreeOwnership == state.OwnCreated { + if err := s.compensateWorktree(o.rw); err != nil { + errs = append(errs, err) + } + } + if o.branchOwnership == state.OwnCreated { + if gitops.BranchExists(o.rw.SourceRepo, o.rw.Branch) { + if err := s.mut().DeleteBranch(o.rw.SourceRepo, o.rw.Branch, true); err != nil { + // Only a still-present branch is a real failure. + if gitops.BranchExists(o.rw.SourceRepo, o.rw.Branch) { + errs = append(errs, fmt.Errorf("%s: branch delete: %w", o.rw.RepoName, err)) + } + } + } + } + } + if rootOwnership == state.OwnCreated { + if _, err := os.Stat(wsPath); err == nil || !os.IsNotExist(err) { + if rmErr := s.mut().RemoveAll(wsPath); rmErr != nil { + if _, st := os.Stat(wsPath); st == nil || !os.IsNotExist(st) { + errs = append(errs, fmt.Errorf("workspace root: %w", rmErr)) + } + } + } + } + return errs +} + +// compensateWorktree removes an operation-created worktree, reconciling against +// BOTH the filesystem and the Git worktree registration (a directory can be +// deleted while its worktree registration lingers as prunable). A registration +// that cannot be inspected is treated as still-present (returns an error so the +// caller keeps the record pending). +func (s *Service) compensateWorktree(rw models.RepoWorktree) error { + registered, listErr := s.worktreeRegistered(rw.SourceRepo, rw.WorktreePath) + if listErr != nil { + return fmt.Errorf("%s: worktree registration inspect: %w", rw.RepoName, listErr) + } + _, statErr := os.Stat(rw.WorktreePath) + exists := statErr == nil || !os.IsNotExist(statErr) + + switch { + case !registered && !exists: + return nil // nothing to do (write-ahead resource never actually created) + case registered: + // `git worktree remove --force` handles both the registration and the + // directory, and works even when the directory is already gone. It is + // path-scoped, so it never touches unrelated prunable registrations. + _ = s.mut().WorktreeRemove(rw.SourceRepo, rw.WorktreePath) + case exists: + // Directory present but not a registered worktree (e.g. mkdir'd then add + // failed): remove the directory. + _ = s.mut().RemoveAll(rw.WorktreePath) + } + + // Verify convergence: only a still-present worktree/registration is a failure. + reg, listErr := s.worktreeRegistered(rw.SourceRepo, rw.WorktreePath) + if listErr != nil { + return fmt.Errorf("%s: worktree registration inspect: %w", rw.RepoName, listErr) + } + if _, st := os.Stat(rw.WorktreePath); reg || st == nil || !os.IsNotExist(st) { + return fmt.Errorf("%s: worktree still present after removal", rw.RepoName) + } + return nil +} + +// worktreeRegistered reports whether path is a registered worktree of repo. +func (s *Service) worktreeRegistered(repo, path string) (bool, error) { + entries, err := gitops.WorktreeList(repo) + if err != nil { + return false, err + } + want := resolvePath(path) + for _, e := range entries { + if resolvePath(e.Path) == want { + return true, nil + } + } + return false, nil +} + +// resolvePath canonicalizes a path for comparison (symlinks + cleaning). When +// the leaf no longer exists (e.g. a deleted worktree directory), it resolves the +// parent directory's symlinks and re-appends the base, so a stale registration +// recorded under a resolved path (/private/var/...) still matches a caller path +// expressed via a symlinked prefix (/var/...). +func resolvePath(p string) string { + if r, err := filepath.EvalSymlinks(p); err == nil { + return filepath.Clean(r) + } + parent := filepath.Dir(p) + if rp, err := filepath.EvalSymlinks(parent); err == nil { + return filepath.Clean(filepath.Join(rp, filepath.Base(p))) + } + return filepath.Clean(p) +} + +// resumeStaleCreate conservatively resolves a create record left by a previous +// crashed run for the same workspace name. If state already contains the +// workspace, the earlier commit succeeded and the record is simply cleared. +// Otherwise the record's operation-created resources are compensated using the +// recorded ownership, then the record is removed so a fresh create can proceed. +func (s *Service) resumeStaleCreate(name string) error { + recs, err := s.ops().List() + if err != nil { + return err + } + for i := range recs { + rec := recs[i] + if rec.Kind != state.OpCreate || rec.Workspace != name { + continue + } + if !rec.Supported() { + return fmt.Errorf("unsupported prior create record %s; run gw doctor", rec.ID) + } + ws, err := s.State.GetWorkspace(name) + if err != nil { + return err + } + if ws != nil { + // Prior commit succeeded; just clear the stale record. + if err := s.ops().Delete(rec.ID); err != nil { + return err + } + continue + } + // Compensate operation-created resources from the record. If the record + // carries any unknown-ownership resource that might still exist, retain it + // rather than discarding the only repair evidence. + if recordHasUnknownOwnership(&rec) { + return fmt.Errorf("prior interrupted create for %q has unknown resource ownership; run gw doctor", name) + } + outs := outcomesFromRecord(&rec) + if cerrs := s.compensateCreate(rec.Path, rec.RootOwnership, outs); len(cerrs) > 0 { + return fmt.Errorf("could not roll back prior interrupted create: %s", joinErrors(nil, cerrs)) + } + if err := s.ops().Delete(rec.ID); err != nil { + return err + } + } + return nil +} + +// recordHasUnknownOwnership reports whether any operation-created resource in +// the record has undetermined ownership (so compensation cannot be exact). +func recordHasUnknownOwnership(rec *state.OperationRecord) bool { + for _, r := range rec.Repos { + // A repo that was attempted (in-progress or failed) but whose ownership was + // never determined cannot be compensated exactly. + if r.Status != state.RepoFailed && r.Status != state.RepoInProgress { + continue + } + if r.BranchOwnership == state.OwnUnknown && r.Branch != "" { + return true + } + if r.WorktreeOwnership == state.OwnUnknown && r.WorktreePath != "" { + return true + } + } + return false +} +func (s *Service) fetchRepos(repoNames, sourcePaths []string) { + console.Infof("fetching %d repos...", len(repoNames)) + var wg sync.WaitGroup + for i := range repoNames { + wg.Add(1) + go func(source, name string) { + defer wg.Done() + if err := gitops.Fetch(source); err != nil { + console.Warningf(" %s: fetch failed, using local state", name) + } + }(sourcePaths[i], repoNames[i]) + } + wg.Wait() +} + +// runSetupHooksErr runs setup hooks and returns an aggregated error (nil if all +// succeed or there are no hooks). +func (s *Service) runSetupHooksErr(ws models.Workspace) error { + var ( + mu sync.Mutex + errs []error + wg sync.WaitGroup + ) + for _, r := range ws.Repos { + groveCfg, _ := gitops.ReadGroveConfig(r.SourceRepo) + if groveCfg == nil || len(groveCfg.Setup) == 0 { + continue + } + wg.Add(1) + go func(repo models.RepoWorktree, cmds []string) { + defer wg.Done() + for _, cmdStr := range cmds { + if err := s.RunCmd(repo.WorktreePath, cmdStr); err != nil { + console.Warningf("setup hook failed in %s: %s", repo.RepoName, err) + mu.Lock() + errs = append(errs, fmt.Errorf("%s: %w", repo.RepoName, err)) + mu.Unlock() + } + } + }(r, []string(groveCfg.Setup)) + } + wg.Wait() + if len(errs) == 0 { + return nil + } + return joinErrs(errs) +} + +// --- helpers --- + +func repoOpFromOutcome(name string, out *provisionOutcome) state.RepoOperation { + return state.RepoOperation{ + RepoName: name, + SourceRepo: out.rw.SourceRepo, + WorktreePath: out.rw.WorktreePath, + Branch: out.rw.Branch, + BaseBranch: out.baseBranch, + Mode: out.mode, + Status: state.RepoDone, + BranchOwnership: out.branchOwnership, + WorktreeOwnership: out.worktreeOwnership, + } +} + +func outcomesFromRecord(rec *state.OperationRecord) []*provisionOutcome { + var outs []*provisionOutcome + for _, r := range rec.Repos { + // Include done, failed, and in-progress repos: any of them may have created + // resources (compensation checks actual existence and is idempotent). + if r.Status == state.RepoPending || r.Status == state.RepoSkipped { + continue + } + outs = append(outs, &provisionOutcome{ + rw: models.RepoWorktree{ + RepoName: r.RepoName, + SourceRepo: r.SourceRepo, + WorktreePath: r.WorktreePath, + Branch: r.Branch, + }, + branchOwnership: r.BranchOwnership, + worktreeOwnership: r.WorktreeOwnership, + mode: r.Mode, + baseBranch: r.BaseBranch, + }) + } + return outs +} + +func collectWorktrees(outs []*provisionOutcome) []models.RepoWorktree { + rws := make([]models.RepoWorktree, 0, len(outs)) + for _, o := range outs { + rws = append(rws, o.rw) + } + return rws +} + +func joinErrors(cause error, errs []error) string { + parts := make([]string, 0, len(errs)+1) + if cause != nil { + parts = append(parts, cause.Error()) + } + for _, e := range errs { + parts = append(parts, e.Error()) + } + return strings.Join(parts, "; ") +} + +func joinErrs(errs []error) error { + if len(errs) == 0 { + return nil + } + msgs := make([]string, len(errs)) + for i, e := range errs { + msgs[i] = e.Error() + } + return errors.New(strings.Join(msgs, "; ")) +} + +// ValidateWorkspaceName rejects names that would escape the workspace dir or +// collide with path handling. +func ValidateWorkspaceName(name string) error { + if strings.TrimSpace(name) == "" { + return errors.New("workspace name is required") + } + if strings.ContainsAny(name, "/\\") || strings.Contains(name, "..") { + return fmt.Errorf("invalid workspace name %q", name) + } + if name != filepath.Base(name) { + return fmt.Errorf("invalid workspace name %q", name) + } + return nil +} + +// fail marks the result as a precondition failure (nothing mutated) and returns it. +func (r *OperationResult) fail(err error) *OperationResult { + r.Status = OutcomeFailed + r.Err = err + r.Message = err.Error() + return r +} diff --git a/internal/workspace/create_fault_test.go b/internal/workspace/create_fault_test.go new file mode 100644 index 0000000..f8a7e80 --- /dev/null +++ b/internal/workspace/create_fault_test.go @@ -0,0 +1,608 @@ +package workspace + +import ( + "context" + "errors" + "os" + "path/filepath" + "testing" + + "github.com/nicksenap/grove/internal/models" + "github.com/nicksenap/grove/internal/state" +) + +// installCreateFaultBackend wraps the env's production backend so create tests +// can inject failures at specific phases. +func installCreateFaultBackend(env *testEnv) *faultBackend { + fb := newFaultBackend(prodBackend{}) + env.svc.backend = fb + return fb +} + +func recordFor(t *testing.T, env *testEnv, kind state.OperationKind, ws string) *state.OperationRecord { + t.Helper() + recs, err := env.svc.ops().List() + if err != nil { + t.Fatalf("list records: %v", err) + } + for i := range recs { + if recs[i].Kind == kind && recs[i].Workspace == ws { + return &recs[i] + } + } + return nil +} + +func branchExists(env *testEnv, repo, branch string) bool { + out := env.run(env.repoMap[repo], "git", "branch", "--list", branch) + return out != "" +} + +// TestCreateWorktreeFailureCompensatesBranch proves a worktree-add failure after +// a successful branch create rolls back the operation-created branch and leaves +// no state, worktree, or record behind. +func TestCreateWorktreeFailureCompensatesBranch(t *testing.T) { + env := setupTestEnv(t) + env.createRepo("api") + fb := installCreateFaultBackend(env) + // Branch is created, then the worktree add fails before applying. + fb.failBefore["WorktreeAdd:"+env.repoMap["api"]] = errors.New("disk full") + + res := env.svc.CreateWithResult("wt-fail", CreateOpts{ + Branch: "feat/x", Repos: []string{"api"}, RepoMap: env.repoMap, Cfg: env.cfg, + }) + if res.Status != OutcomeFailed { + t.Fatalf("expected failed outcome, got %s (%v)", res.Status, res.Err) + } + if !res.NonZeroExit() { + t.Fatal("failed create must exit non-zero") + } + // State: nothing committed. + if ws, _ := env.svc.State.GetWorkspace("wt-fail"); ws != nil { + t.Fatal("no workspace should be committed") + } + // Branch: operation-created branch removed. + if branchExists(env, "api", "feat/x") { + t.Fatal("operation-created branch should be deleted on compensation") + } + // Record cleared after full compensation. + if rec := recordFor(t, env, state.OpCreate, "wt-fail"); rec != nil { + t.Fatalf("recovery record should be cleared, got %+v", rec) + } + // Workspace root removed. + if _, err := os.Stat(filepath.Join(env.wsDir, "wt-fail")); !os.IsNotExist(err) { + t.Fatal("workspace root should be removed") + } +} + +// TestCreatePreservesPreexistingBranch proves compensation never deletes a +// branch that existed before the operation. +func TestCreatePreservesPreexistingBranch(t *testing.T) { + env := setupTestEnv(t) + env.createRepo("api") + // Pre-create the branch so the operation does NOT own it. + env.run(env.repoMap["api"], "git", "branch", "feat/keep") + + fb := installCreateFaultBackend(env) + fb.failAfter["WorktreeAdd:"+env.repoMap["api"]] = errors.New("boom") + + res := env.svc.CreateWithResult("keepbr", CreateOpts{ + Branch: "feat/keep", Repos: []string{"api"}, RepoMap: env.repoMap, Cfg: env.cfg, + }) + if res.Status != OutcomeFailed { + t.Fatalf("expected failed, got %s", res.Status) + } + if !branchExists(env, "api", "feat/keep") { + t.Fatal("pre-existing branch must be preserved on compensation") + } +} + +// TestCreateLaterRepoFailureRollsBackEarlier proves a failure on the second repo +// compensates the first repo's created resources. +func TestCreateLaterRepoFailureRollsBackEarlier(t *testing.T) { + env := setupTestEnv(t) + env.createRepo("api") + env.createRepo("web") + fb := installCreateFaultBackend(env) + fb.failBefore["WorktreeAdd:"+env.repoMap["web"]] = errors.New("web fails") + + res := env.svc.CreateWithResult("multi", CreateOpts{ + Branch: "feat/m", Repos: []string{"api", "web"}, RepoMap: env.repoMap, Cfg: env.cfg, + }) + if res.Status != OutcomeFailed { + t.Fatalf("expected failed, got %s (%v)", res.Status, res.Err) + } + // Ordered outcomes: api done, web failed. + if len(res.Repos) != 2 || res.Repos[0].RepoName != "api" || res.Repos[1].RepoName != "web" { + t.Fatalf("outcomes should preserve request order: %+v", res.Repos) + } + if res.Repos[1].Status != state.RepoFailed { + t.Fatalf("web should be failed: %+v", res.Repos[1]) + } + if branchExists(env, "api", "feat/m") { + t.Fatal("earlier repo's created branch must be rolled back") + } + if _, err := os.Stat(filepath.Join(env.wsDir, "multi", "api")); !os.IsNotExist(err) { + t.Fatal("earlier repo worktree must be removed") + } +} + +// TestCreateCommitFailureCompensates proves a final state-commit failure fully +// compensates provisioned resources. +func TestCreateCommitFailureCompensates(t *testing.T) { + env := setupTestEnv(t) + env.createRepo("api") + // Fail the commit itself (pre-commit: state not applied). + env.svc.commitFault = func(m *state.Mutation) error { return errors.New("commit boom") } + + res := env.svc.CreateWithResult("commitfail", CreateOpts{ + Branch: "feat/c", Repos: []string{"api"}, RepoMap: env.repoMap, Cfg: env.cfg, + }) + if res.Status != OutcomeFailed { + t.Fatalf("expected failed, got %s (%v)", res.Status, res.Err) + } + if ws, _ := env.svc.State.GetWorkspace("commitfail"); ws != nil { + t.Fatal("no workspace should be committed") + } + if branchExists(env, "api", "feat/c") { + t.Fatal("branch should be rolled back after commit failure") + } + if rec := recordFor(t, env, state.OpCreate, "commitfail"); rec != nil { + t.Fatal("record should be cleared after full compensation") + } +} + +// TestCreateRollbackFailureLeavesPendingRecord proves that when compensation +// itself fails, the operation is Pending and the recovery record is retained. +func TestCreateRollbackFailureLeavesPendingRecord(t *testing.T) { + env := setupTestEnv(t) + env.createRepo("api") + fb := installCreateFaultBackend(env) + // Branch created, worktree add fails, then the branch delete (compensation) + // also fails — leaving the operation unable to fully roll back. + fb.failBefore["WorktreeAdd:"+env.repoMap["api"]] = errors.New("provision boom") + fb.failBefore["DeleteBranch:"+env.repoMap["api"]] = errors.New("rollback boom") + + res := env.svc.CreateWithResult("pending-ws", CreateOpts{ + Branch: "feat/p", Repos: []string{"api"}, RepoMap: env.repoMap, Cfg: env.cfg, + }) + if res.Status != OutcomePending { + t.Fatalf("expected pending, got %s (%v)", res.Status, res.Err) + } + if res.RecordID == "" { + t.Fatal("pending result must carry a recovery record id") + } + rec := recordFor(t, env, state.OpCreate, "pending-ws") + if rec == nil { + t.Fatal("recovery record must be retained on incomplete rollback") + } + if rec.Phase != "compensation" || !rec.Retryable { + t.Fatalf("record should be marked compensation/retryable: %+v", rec) + } +} + +// TestCreateSetupHookFailureIsPartial proves setup-hook failure after commit +// yields a partial (non-zero) outcome without undoing the valid workspace. +func TestCreateSetupHookFailureIsPartial(t *testing.T) { + env := setupTestEnv(t) + repo := env.createRepo("hooked") + // A setup hook that fails. + if err := os.WriteFile(filepath.Join(repo, ".grove.toml"), []byte(`setup = "exit 1"`), 0o644); err != nil { + t.Fatal(err) + } + + res := env.svc.CreateWithResult("hook-partial", CreateOpts{ + Branch: "feat/h", Repos: []string{"hooked"}, RepoMap: env.repoMap, Cfg: env.cfg, + }) + if res.Status != OutcomePartial { + t.Fatalf("expected partial, got %s (%v)", res.Status, res.Err) + } + if !res.NonZeroExit() { + t.Fatal("partial create must exit non-zero") + } + // Workspace is valid and committed. + if ws, _ := env.svc.State.GetWorkspace("hook-partial"); ws == nil { + t.Fatal("workspace must remain committed despite hook failure") + } + // Record cleared (commit succeeded). + if rec := recordFor(t, env, state.OpCreate, "hook-partial"); rec != nil { + t.Fatal("record should be cleared after successful commit") + } +} + +// TestCreateSuccessClearsRecord proves the happy path leaves no recovery record +// and a consistent workspace. +func TestCreateSuccessClearsRecord(t *testing.T) { + env := setupTestEnv(t) + env.createRepo("api") + + res := env.svc.CreateWithResult("clean", CreateOpts{ + Branch: "feat/ok", Repos: []string{"api"}, RepoMap: env.repoMap, Cfg: env.cfg, + }) + if res.Status != OutcomeSuccess { + t.Fatalf("expected success, got %s (%v)", res.Status, res.Err) + } + if rec := recordFor(t, env, state.OpCreate, "clean"); rec != nil { + t.Fatal("record should be cleared on success") + } + if ws, _ := env.svc.State.GetWorkspace("clean"); ws == nil || len(ws.Repos) != 1 { + t.Fatal("workspace should be committed with its repo") + } +} + +// TestCreateResumesStalePendingRecord proves gw create can finish/roll back its +// own prior interrupted create for the same name. +func TestCreateResumesStalePendingRecord(t *testing.T) { + env := setupTestEnv(t) + env.createRepo("api") + + // Simulate a prior crash: an operation-created branch + a stale record, but + // no committed state. + env.run(env.repoMap["api"], "git", "branch", "feat/stale") + stale := &state.OperationRecord{ + Kind: state.OpCreate, + Workspace: "resumed", + Path: filepath.Join(env.wsDir, "resumed"), + RootOwnership: state.OwnUnknown, + Repos: []state.RepoOperation{{ + RepoName: "api", + SourceRepo: env.repoMap["api"], + Branch: "feat/stale", + Status: state.RepoDone, + BranchOwnership: state.OwnCreated, + }}, + } + if err := env.svc.ops().Write(stale); err != nil { + t.Fatal(err) + } + + // A fresh create for the same name resolves the stale record first. + res := env.svc.CreateWithResult("resumed", CreateOpts{ + Branch: "feat/new", Repos: []string{"api"}, RepoMap: env.repoMap, Cfg: env.cfg, + }) + if res.Status != OutcomeSuccess { + t.Fatalf("expected success after resume, got %s (%v)", res.Status, res.Err) + } + // Stale operation-created branch was rolled back during resume. + if branchExists(env, "api", "feat/stale") { + t.Fatal("stale operation-created branch should be rolled back on resume") + } + // Only one record-free, consistent workspace remains. + recs, _ := env.svc.ops().List() + if len(recs) != 0 { + t.Fatalf("no records should remain, got %d", len(recs)) + } +} + +// TestCreateInvalidInputsNoMutation proves precondition failures mutate nothing +// and write no recovery record. +func TestCreateInvalidInputsNoMutation(t *testing.T) { + env := setupTestEnv(t) + env.createRepo("api") + + cases := []struct { + name string + opts CreateOpts + }{ + {"", CreateOpts{Branch: "b", Repos: []string{"api"}, RepoMap: env.repoMap, Cfg: env.cfg}}, + {"bad/name", CreateOpts{Branch: "b", Repos: []string{"api"}, RepoMap: env.repoMap, Cfg: env.cfg}}, + {"no-branch", CreateOpts{Branch: "", Repos: []string{"api"}, RepoMap: env.repoMap, Cfg: env.cfg}}, + {"unknown-repo", CreateOpts{Branch: "b", Repos: []string{"ghost"}, RepoMap: env.repoMap, Cfg: env.cfg}}, + } + for _, c := range cases { + res := env.svc.CreateWithResult(c.name, c.opts) + if res.Status != OutcomeFailed || res.Err == nil { + t.Fatalf("case %q: expected precondition failure, got %s", c.name, res.Status) + } + } + recs, _ := env.svc.ops().List() + if len(recs) != 0 { + t.Fatalf("precondition failures must write no records, got %d", len(recs)) + } +} + +// TestCreateSameNameConcurrency proves two concurrent creates for the same name +// yield exactly one success and one deterministic conflict, with the loser fully +// compensated. +func TestCreateSameNameConcurrency(t *testing.T) { + env := setupTestEnv(t) + env.createRepo("api") + env.createRepo("web") + + type out struct { + res *OperationResult + } + ch := make(chan out, 2) + go func() { + e := &Service{State: env.svc.State, Stats: env.svc.Stats, Ops: env.svc.Ops, RunCmd: prodRunCmd, RunCmdSilent: prodRunCmdSilent, backend: prodBackend{}} + ch <- out{e.CreateWithResult("race", CreateOpts{Branch: "feat/a", Repos: []string{"api"}, RepoMap: env.repoMap, Cfg: env.cfg})} + }() + go func() { + e := &Service{State: env.svc.State, Stats: env.svc.Stats, Ops: env.svc.Ops, RunCmd: prodRunCmd, RunCmdSilent: prodRunCmdSilent, backend: prodBackend{}} + ch <- out{e.CreateWithResult("race", CreateOpts{Branch: "feat/b", Repos: []string{"web"}, RepoMap: env.repoMap, Cfg: env.cfg})} + }() + a := <-ch + b := <-ch + + successes := 0 + if a.res.Status == OutcomeSuccess { + successes++ + } + if b.res.Status == OutcomeSuccess { + successes++ + } + if successes != 1 { + t.Fatalf("expected exactly one winner, got %d (a=%s b=%s)", successes, a.res.Status, b.res.Status) + } + // Exactly one workspace named "race" in state, and its recorded repos match + // its on-disk worktrees (winner not corrupted by the loser's rollback). + ws, _ := env.svc.State.GetWorkspace("race") + if ws == nil { + t.Fatal("winner should be committed") + } + if _, err := os.Stat(ws.Path); err != nil { + t.Fatalf("winner workspace root must exist: %v", err) + } + for _, r := range ws.Repos { + if _, err := os.Stat(r.WorktreePath); err != nil { + t.Fatalf("winner worktree %s must exist: %v", r.WorktreePath, err) + } + if !branchExists(env, r.RepoName, r.Branch) { + t.Fatalf("winner branch %s must exist in %s", r.Branch, r.RepoName) + } + } + // No recovery records remain for either attempt. + recs, _ := env.svc.ops().List() + if len(recs) != 0 { + t.Fatalf("no records should remain after race, got %d", len(recs)) + } + _ = context.Background() +} + +// TestCreateAmbiguousCommitReconciledAsCommitted proves that when the state +// commit applies but returns an error (e.g. a post-rename dir-sync failure), the +// workspace is reconciled as committed rather than destroyed. +func TestCreateAmbiguousCommitReconciledAsCommitted(t *testing.T) { + env := setupTestEnv(t) + env.createRepo("api") + // commitFault commits, then returns an error (ambiguous commit). + env.svc.commitFault = func(m *state.Mutation) error { + if e := m.Commit(); e != nil { + return e + } + return errors.New("dir sync after rename") + } + + res := env.svc.CreateWithResult("ambi", CreateOpts{ + Branch: "feat/a", Repos: []string{"api"}, RepoMap: env.repoMap, Cfg: env.cfg, + }) + // The workspace persisted, so it must NOT be compensated; treat as committed. + if ws, _ := env.svc.State.GetWorkspace("ambi"); ws == nil { + t.Fatal("ambiguous-but-applied commit must keep the workspace") + } + if !branchExists(env, "api", "feat/a") { + t.Fatal("branch must be preserved for an applied commit") + } + if res.Status != OutcomeSuccess && res.Status != OutcomePartial { + t.Fatalf("expected success/partial for applied commit, got %s", res.Status) + } +} + +// TestCompensateWorktreeReconcilesStaleRegistration proves compensation +// reconciles against the Git worktree registration, not just the filesystem: +// when a worktree directory is deleted but its registration lingers (prunable), +// compensation prunes the registration and leaves the branch untouched. +func TestCompensateWorktreeReconcilesStaleRegistration(t *testing.T) { + env := setupTestEnv(t) + repo := env.createRepo("api") + wtPath := filepath.Join(t.TempDir(), "wt") + env.run(repo, "git", "branch", "feat/keep") + env.run(repo, "git", "worktree", "add", wtPath, "feat/keep") + + // Delete the directory out-of-band, leaving a prunable registration. + if err := os.RemoveAll(wtPath); err != nil { + t.Fatal(err) + } + if reg, _ := env.svc.worktreeRegistered(repo, wtPath); !reg { + t.Fatal("precondition: stale registration should still exist") + } + + rw := models.RepoWorktree{RepoName: "api", SourceRepo: repo, WorktreePath: wtPath, Branch: "feat/keep"} + if err := env.svc.compensateWorktree(rw); err != nil { + t.Fatalf("compensateWorktree: %v", err) + } + if reg, _ := env.svc.worktreeRegistered(repo, wtPath); reg { + t.Fatal("stale worktree registration must be pruned") + } + // compensateWorktree must not touch branches. + if !branchExists(env, "api", "feat/keep") { + t.Fatal("branch must remain after worktree compensation") + } +} + +// TestCompensateWorktreePreservesUnrelatedRegistrations proves worktree +// compensation is path-scoped: removing one stale registration must not disturb +// another workspace's prunable registration in the same repo. +func TestCompensateWorktreePreservesUnrelatedRegistrations(t *testing.T) { + env := setupTestEnv(t) + repo := env.createRepo("api") + mine := filepath.Join(t.TempDir(), "mine") + other := filepath.Join(t.TempDir(), "other") + env.run(repo, "git", "branch", "b-mine") + env.run(repo, "git", "branch", "b-other") + env.run(repo, "git", "worktree", "add", mine, "b-mine") + env.run(repo, "git", "worktree", "add", other, "b-other") + // Both directories vanish, leaving two prunable registrations. + os.RemoveAll(mine) + os.RemoveAll(other) + + rw := models.RepoWorktree{RepoName: "api", SourceRepo: repo, WorktreePath: mine, Branch: "b-mine"} + if err := env.svc.compensateWorktree(rw); err != nil { + t.Fatalf("compensateWorktree: %v", err) + } + if reg, _ := env.svc.worktreeRegistered(repo, mine); reg { + t.Fatal("my stale registration must be removed") + } + if reg, _ := env.svc.worktreeRegistered(repo, other); !reg { + t.Fatal("unrelated prunable registration must be preserved (scoped removal only)") + } +} + +// TestCreateMkdirFailureNoRecord proves a workspace-root mkdir failure leaves no +// state and no recovery record. +func TestCreateMkdirFailureNoRecord(t *testing.T) { + env := setupTestEnv(t) + env.createRepo("api") + fb := installCreateFaultBackend(env) + wsPath := filepath.Join(env.wsDir, "mkfail") + fb.failBefore["Mkdir:"+wsPath] = errors.New("permission denied") + + res := env.svc.CreateWithResult("mkfail", CreateOpts{ + Branch: "feat/m", Repos: []string{"api"}, RepoMap: env.repoMap, Cfg: env.cfg, + }) + if res.Status != OutcomeFailed { + t.Fatalf("expected failed, got %s", res.Status) + } + if recs, _ := env.svc.ops().List(); len(recs) != 0 { + t.Fatalf("mkdir failure should leave no record, got %d", len(recs)) + } +} + +// TestCreateMkdirAppliedThenErrorCompensatesRoot proves an applied-then-error +// mkdir (root created, then error) removes the operation-created root and +// clears the record. +func TestCreateMkdirAppliedThenErrorCompensatesRoot(t *testing.T) { + env := setupTestEnv(t) + env.createRepo("api") + fb := installCreateFaultBackend(env) + wsPath := filepath.Join(env.wsDir, "mkafter") + fb.failAfter["Mkdir:"+wsPath] = errors.New("post-mkdir boom") + + res := env.svc.CreateWithResult("mkafter", CreateOpts{ + Branch: "feat/m", Repos: []string{"api"}, RepoMap: env.repoMap, Cfg: env.cfg, + }) + if res.Status != OutcomeFailed { + t.Fatalf("expected failed, got %s (%v)", res.Status, res.Err) + } + if _, err := os.Stat(wsPath); !os.IsNotExist(err) { + t.Fatal("operation-created root must be removed after applied-then-error mkdir") + } + if recs, _ := env.svc.ops().List(); len(recs) != 0 { + t.Fatalf("record should be cleared, got %d", len(recs)) + } +} + +// TestCreateDifferentNameSameBranchConcurrency proves two creates for DIFFERENT +// workspace names targeting the same branch in the same repo do not delete each +// other's branch: the branch resource lock serializes them and the loser sees +// the branch as pre-existing (never compensating it). +func TestCreateDifferentNameSameBranchConcurrency(t *testing.T) { + env := setupTestEnv(t) + env.createRepo("api") + + type out struct{ res *OperationResult } + ch := make(chan out, 2) + mk := func(ws string) { + e := &Service{State: env.svc.State, Stats: env.svc.Stats, Ops: env.svc.Ops, RunCmd: prodRunCmd, RunCmdSilent: prodRunCmdSilent, backend: prodBackend{}} + ch <- out{e.CreateWithResult(ws, CreateOpts{Branch: "feat/shared", Repos: []string{"api"}, RepoMap: env.repoMap, Cfg: env.cfg})} + } + go mk("ws-a") + go mk("ws-b") + a := <-ch + b := <-ch + + successes := 0 + if a.res.Status == OutcomeSuccess { + successes++ + } + if b.res.Status == OutcomeSuccess { + successes++ + } + if successes != 1 { + t.Fatalf("exactly one should succeed (one worktree per branch), got %d", successes) + } + // The winner's branch must survive the loser's compensation. + if !branchExists(env, "api", "feat/shared") { + t.Fatal("shared branch must survive the loser's rollback") + } +} + +// TestCreatePendingRecordRetryConverges proves that after a rollback-incomplete +// pending record, re-running create for the same name resumes: it compensates +// the retained resources and then succeeds, leaving no record. +func TestCreatePendingRecordRetryConverges(t *testing.T) { + env := setupTestEnv(t) + env.createRepo("api") + fb := installCreateFaultBackend(env) + fb.failBefore["WorktreeAdd:"+env.repoMap["api"]] = errors.New("provision boom") + fb.failBefore["DeleteBranch:"+env.repoMap["api"]] = errors.New("rollback boom") + + if res := env.svc.CreateWithResult("conv", CreateOpts{ + Branch: "feat/c", Repos: []string{"api"}, RepoMap: env.repoMap, Cfg: env.cfg, + }); res.Status != OutcomePending { + t.Fatalf("setup: expected pending, got %s", res.Status) + } + + // Clear the faults and retry: resume should compensate then create cleanly. + env.svc.backend = prodBackend{} + res := env.svc.CreateWithResult("conv", CreateOpts{ + Branch: "feat/c", Repos: []string{"api"}, RepoMap: env.repoMap, Cfg: env.cfg, + }) + if res.Status != OutcomeSuccess { + t.Fatalf("retry should converge to success, got %s (%v)", res.Status, res.Err) + } + if recs, _ := env.svc.ops().List(); len(recs) != 0 { + t.Fatalf("no records should remain after convergent retry, got %d", len(recs)) + } + if ws, _ := env.svc.State.GetWorkspace("conv"); ws == nil { + t.Fatal("workspace should exist after convergent retry") + } +} + +// TestCreateRetainsSourceReposOnFailure proves compensation never deletes source +// repositories (an acquired/cloned source is intentionally retained even when a +// later workspace mutation fails). +func TestCreateRetainsSourceReposOnFailure(t *testing.T) { + env := setupTestEnv(t) + api := env.createRepo("api") + web := env.createRepo("web") + fb := installCreateFaultBackend(env) + fb.failBefore["WorktreeAdd:"+env.repoMap["web"]] = errors.New("web fails") + + res := env.svc.CreateWithResult("retain", CreateOpts{ + Branch: "feat/r", Repos: []string{"api", "web"}, RepoMap: env.repoMap, Cfg: env.cfg, + }) + if res.Status != OutcomeFailed { + t.Fatalf("expected failed, got %s", res.Status) + } + // Source repos must survive compensation. + for _, src := range []string{api, web} { + if _, err := os.Stat(src); err != nil { + t.Fatalf("source repo %s must be retained: %v", src, err) + } + } +} + +// TestCreateSkippedReposAppearInResult proves every requested repository appears +// in the ordered result even when an earlier repo fails. +func TestCreateSkippedReposAppearInResult(t *testing.T) { + env := setupTestEnv(t) + env.createRepo("api") + env.createRepo("web") + env.createRepo("worker") + fb := installCreateFaultBackend(env) + fb.failBefore["WorktreeAdd:"+env.repoMap["api"]] = errors.New("api fails") + + res := env.svc.CreateWithResult("skips", CreateOpts{ + Branch: "feat/s", Repos: []string{"api", "web", "worker"}, RepoMap: env.repoMap, Cfg: env.cfg, + }) + if len(res.Repos) != 3 { + t.Fatalf("all 3 repos must appear in result, got %d: %+v", len(res.Repos), res.Repos) + } + if res.Repos[0].Status != state.RepoFailed { + t.Fatalf("api should be failed: %+v", res.Repos[0]) + } + if res.Repos[1].Status != state.RepoSkipped || res.Repos[2].Status != state.RepoSkipped { + t.Fatalf("web/worker should be skipped: %+v", res.Repos[1:]) + } +} diff --git a/internal/workspace/doctor_recovery_test.go b/internal/workspace/doctor_recovery_test.go new file mode 100644 index 0000000..915325b --- /dev/null +++ b/internal/workspace/doctor_recovery_test.go @@ -0,0 +1,71 @@ +package workspace + +import ( + "strings" + "testing" + + "github.com/nicksenap/grove/internal/state" +) + +// TestDoctorReportsRecoveryRecord proves doctor surfaces a stranded recovery +// record without mutating it, and that clean state with no records is silent. +func TestDoctorReportsRecoveryRecord(t *testing.T) { + env := setupTestEnv(t) + + // No records, no workspaces → healthy. + issues, fixed, err := env.svc.Doctor(false) + if err != nil { + t.Fatalf("doctor: %v", err) + } + if len(issues) != 0 || fixed != 0 { + t.Fatalf("expected clean doctor, got issues=%v fixed=%d", issues, fixed) + } + + // Write a stranded recovery record as if a create crashed mid-flight. + rec := &state.OperationRecord{ + Kind: state.OpCreate, + Workspace: "feat-x", + Phase: "provisioning", + LastError: "worktree add failed", + Repos: []state.RepoOperation{ + {RepoName: "api", Status: state.RepoFailed}, + }, + } + if err := env.svc.ops().Write(rec); err != nil { + t.Fatalf("write record: %v", err) + } + + // Doctor reports it, even without --fix, and does not remove it. + issues, _, err = env.svc.Doctor(false) + if err != nil { + t.Fatalf("doctor: %v", err) + } + found := false + for _, iss := range issues { + if iss.Workspace == "feat-x" && strings.Contains(iss.Issue, "interrupted create") { + found = true + } + } + if !found { + t.Fatalf("expected interrupted-create issue, got %+v", issues) + } + + // The record must NOT be mutated by diagnosis, even with --fix (repair is a + // later task). + issues, _, err = env.svc.Doctor(true) + if err != nil { + t.Fatalf("doctor --fix: %v", err) + } + if got, _ := env.svc.ops().Read(rec.ID); got == nil { + t.Fatal("recovery record must survive diagnosis") + } + found = false + for _, iss := range issues { + if iss.Workspace == "feat-x" { + found = true + } + } + if !found { + t.Fatal("record should still be reported after diagnosis-only --fix") + } +} diff --git a/internal/workspace/faults.go b/internal/workspace/faults.go new file mode 100644 index 0000000..6899774 --- /dev/null +++ b/internal/workspace/faults.go @@ -0,0 +1,90 @@ +package workspace + +import ( + "context" + "os" + + "github.com/nicksenap/grove/internal/gitops" + "github.com/nicksenap/grove/internal/state" +) + +// mutationBackend performs the low-level Git and filesystem mutations that make +// up a workspace lifecycle operation. It is an instance-scoped seam so tests can +// wrap production behavior and inject failures either before or after a mutation +// is applied — the latter is essential for modeling "mutation succeeded, then +// the process died" cases that pure preconditions cannot represent. +// +// Release binaries always use prodBackend, selected by NewService. There is no +// production code path that installs a fault-injecting backend, so failpoints +// cannot be enabled outside tests. +// +// Lifecycle operations migrate onto this backend from Task 3 onward; Task 2 +// establishes the seam and its production implementation. +type mutationBackend interface { + CreateBranch(repo, branch, startPoint string) error + DeleteBranch(repo, branch string, force bool) error + WorktreeAdd(repo, path, branch string) error + WorktreeAddTracking(repo, path, branch string) error + WorktreeRemove(repo, path string) error + WorktreeRepair(repo, path string) error + Mkdir(path string, perm os.FileMode) error + RemoveAll(path string) error + Rename(oldPath, newPath string) error +} + +// prodBackend is the production mutation backend backed by real git and os +// calls. +type prodBackend struct{} + +func (prodBackend) CreateBranch(repo, branch, startPoint string) error { + return gitops.CreateBranch(repo, branch, startPoint) +} +func (prodBackend) DeleteBranch(repo, branch string, force bool) error { + return gitops.DeleteBranch(repo, branch, force) +} +func (prodBackend) WorktreeAdd(repo, path, branch string) error { + return gitops.WorktreeAdd(repo, path, branch) +} +func (prodBackend) WorktreeAddTracking(repo, path, branch string) error { + return gitops.WorktreeAddTracking(repo, path, branch) +} +func (prodBackend) WorktreeRemove(repo, path string) error { + return gitops.WorktreeRemove(repo, path) +} +func (prodBackend) WorktreeRepair(repo, path string) error { + return gitops.WorktreeRepair(repo, path) +} +func (prodBackend) Mkdir(path string, perm os.FileMode) error { + return os.MkdirAll(path, perm) +} +func (prodBackend) RemoveAll(path string) error { + return os.RemoveAll(path) +} +func (prodBackend) Rename(oldPath, newPath string) error { + return os.Rename(oldPath, newPath) +} + +// mut returns the Service's mutation backend, defaulting to production. +func (s *Service) mut() mutationBackend { + if s.backend != nil { + return s.backend + } + return prodBackend{} +} + +// withMutation is the single commit boundary lifecycle operations use. The +// callback performs in-memory edits on the locked snapshot but must NOT call +// Commit; withMutation owns the commit so tests can intercept it via an +// instance-scoped commit seam (commitFault) to model either a pre-commit +// failure (state not applied) or an applied-then-error ambiguous commit. +func (s *Service) withMutation(ctx context.Context, fn func(*state.Mutation) error) error { + return s.State.WithMutation(ctx, func(m *state.Mutation) error { + if err := fn(m); err != nil { + return err + } + if s.commitFault != nil { + return s.commitFault(m) + } + return m.Commit() + }) +} diff --git a/internal/workspace/result.go b/internal/workspace/result.go new file mode 100644 index 0000000..8f3e860 --- /dev/null +++ b/internal/workspace/result.go @@ -0,0 +1,77 @@ +package workspace + +import "github.com/nicksenap/grove/internal/state" + +// OutcomeStatus is the overall status of a lifecycle operation. +type OutcomeStatus string + +const ( + // OutcomeSuccess: every requested target completed and state is consistent. + OutcomeSuccess OutcomeStatus = "success" + // OutcomePartial: the core operation committed but a non-critical step + // (e.g. setup/post-create hooks) failed. The workspace is valid. + OutcomePartial OutcomeStatus = "partial" + // OutcomeFailed: the operation did not commit; it was fully compensated. + OutcomeFailed OutcomeStatus = "failed" + // OutcomePending: the operation could not complete or fully compensate and + // left a durable recovery record for retry/repair. + OutcomePending OutcomeStatus = "pending" + // OutcomeCancelled: the user explicitly cancelled; nothing was mutated. + OutcomeCancelled OutcomeStatus = "cancelled" +) + +// RepoOutcome is the ordered per-repository result of an operation. +type RepoOutcome struct { + RepoName string + Status state.RepoStatus + Phase string + Message string + Err error +} + +// OperationResult is the typed, ordered outcome of a lifecycle operation. It is +// rendered to stderr by the human CLI; the public machine envelope (issue #63) +// is a separate concern. +type OperationResult struct { + Kind state.OperationKind + Workspace string + Status OutcomeStatus + Repos []RepoOutcome + // RecordID is the recovery-record id when Status is Pending. + RecordID string + // Message is a human summary; Err is the terminal error, if any. + Message string + Err error +} + +// NonZeroExit reports whether the CLI should exit non-zero for this result. +// Exit 0 only for complete success or explicit cancellation. +func (r *OperationResult) NonZeroExit() bool { + switch r.Status { + case OutcomeSuccess, OutcomeCancelled: + return false + default: + return true + } +} + +// toError synthesizes an error for the legacy error-returning method shims. +func (r *OperationResult) toError() error { + if !r.NonZeroExit() { + return nil + } + if r.Err != nil { + return r.Err + } + if r.Message != "" { + return &resultError{msg: r.Message} + } + return &resultError{msg: string(r.Status) + " " + string(r.Kind)} +} + +type resultError struct{ msg string } + +func (e *resultError) Error() string { return e.msg } + +// addRepo appends an ordered repository outcome. +func (r *OperationResult) addRepo(o RepoOutcome) { r.Repos = append(r.Repos, o) } diff --git a/internal/workspace/service.go b/internal/workspace/service.go index 7e64fd1..373c7c9 100644 --- a/internal/workspace/service.go +++ b/internal/workspace/service.go @@ -1,20 +1,58 @@ package workspace import ( + "context" "os" "os/exec" "github.com/nicksenap/grove/internal/config" + "github.com/nicksenap/grove/internal/models" "github.com/nicksenap/grove/internal/state" "github.com/nicksenap/grove/internal/stats" ) +// stateStore is the authoritative workspace-state boundary used by the service. +// It is an interface so tests can wrap the real *state.Store and inject commit +// failures (including "commit applied, then error") without a global failpoint. +type stateStore interface { + Load() ([]models.Workspace, error) + GetWorkspace(name string) (*models.Workspace, error) + FindWorkspaceByPath(path string) (*models.Workspace, error) + AddWorkspace(ws models.Workspace) error + UpdateWorkspace(ws models.Workspace) error + UpdateWorkspaceByName(ws models.Workspace, matchName string) error + RemoveWorkspace(name string) error + RenameWorkspace(oldName, newName, newPath string) error + WithMutation(ctx context.Context, fn func(*state.Mutation) error) error + AcquireWorkspaceLock(ctx context.Context, name string) (func(), error) + AcquireResourceLock(ctx context.Context, key string) (func(), error) +} + +// journalStore is the recovery-journal boundary. An interface so tests can +// inject record write/delete failures per Service instance. +type journalStore interface { + Write(rec *state.OperationRecord) error + Read(id string) (*state.OperationRecord, error) + List() ([]state.OperationRecord, error) + Delete(id string) error +} + // Service orchestrates workspace operations with injectable dependencies. type Service struct { - State *state.Store + State stateStore Stats *stats.Tracker + Ops journalStore RunCmd func(dir, cmd string) error RunCmdSilent func(dir, cmd string) error + + // backend performs low-level Git/filesystem mutations. It is unexported so + // only same-package test code can install a fault-injecting wrapper; release + // binaries always use the production backend. + backend mutationBackend + + // commitFault is a test-only seam intercepting the state commit boundary + // (see withMutation). nil in production. + commitFault func(*state.Mutation) error } // NewService creates a Service with production dependencies. @@ -22,9 +60,20 @@ func NewService() *Service { return &Service{ State: state.NewStore(config.GroveDir), Stats: stats.NewTracker(config.GroveDir), + Ops: state.NewOperationStore(config.GroveDir), RunCmd: prodRunCmd, RunCmdSilent: prodRunCmdSilent, + backend: prodBackend{}, + } +} + +// ops returns the recovery journal. It is always set by NewService and by test +// constructors; a nil journal is a programming error surfaced eagerly. +func (s *Service) ops() journalStore { + if s.Ops == nil { + panic("workspace.Service: Ops journal not configured") } + return s.Ops } func prodRunCmd(dir, cmdStr string) error { diff --git a/internal/workspace/service_fault_test.go b/internal/workspace/service_fault_test.go new file mode 100644 index 0000000..fc25aff --- /dev/null +++ b/internal/workspace/service_fault_test.go @@ -0,0 +1,131 @@ +package workspace + +import ( + "context" + "errors" + "os" + "path/filepath" + "testing" + + "github.com/nicksenap/grove/internal/models" + "github.com/nicksenap/grove/internal/state" +) + +// faultJournal injects recovery-journal write/delete failures per instance. +type faultJournal struct { + inner journalStore + failWrite error + failDelete error +} + +func (f *faultJournal) Write(r *state.OperationRecord) error { + if f.failWrite != nil { + return f.failWrite + } + return f.inner.Write(r) +} +func (f *faultJournal) Read(id string) (*state.OperationRecord, error) { return f.inner.Read(id) } +func (f *faultJournal) List() ([]state.OperationRecord, error) { return f.inner.List() } +func (f *faultJournal) Delete(id string) error { + if f.failDelete != nil { + return f.failDelete + } + return f.inner.Delete(id) +} + +// TestServiceCommitSeamInjectable proves the commit boundary can be +// failure-injected per Service instance, covering both a pre-commit failure +// (edits made but state NOT applied) and an applied-then-error ambiguous commit. +func TestServiceCommitSeamInjectable(t *testing.T) { + env := setupTestEnv(t) + + // Pre-commit failure: the callback runs and edits the snapshot, but the + // commit seam returns before m.Commit(), so nothing is persisted. + callbackRan := false + boom := errors.New("pre-commit") + env.svc.commitFault = func(m *state.Mutation) error { return boom } + err := env.svc.withMutation(context.Background(), func(m *state.Mutation) error { + callbackRan = true + return m.Add(models.NewWorkspace("x", "/tmp/x", "main")) + }) + if !errors.Is(err, boom) { + t.Fatalf("expected pre-commit failure, got %v", err) + } + if !callbackRan { + t.Fatal("callback must run (and edit) before the pre-commit failure") + } + if ws, _ := env.svc.State.Load(); len(ws) != 0 { + t.Fatalf("pre-commit failure must not persist state, got %d", len(ws)) + } + + // Applied-then-error: the seam commits, then returns an ambiguous error. + after := errors.New("post-commit") + env.svc.commitFault = func(m *state.Mutation) error { + if e := m.Commit(); e != nil { + return e + } + return after + } + err = env.svc.withMutation(context.Background(), func(m *state.Mutation) error { + return m.Add(models.NewWorkspace("y", "/tmp/y", "main")) + }) + if !errors.Is(err, after) { + t.Fatalf("expected applied-then-error, got %v", err) + } + if ws, _ := env.svc.State.Load(); len(ws) != 1 { + t.Fatalf("applied-then-error must persist state, got %d", len(ws)) + } + + // Journal write failure is injectable per instance. + fj := &faultJournal{inner: env.svc.Ops, failWrite: errors.New("journal")} + env.svc.Ops = fj + if err := env.svc.ops().Write(&state.OperationRecord{Kind: state.OpCreate, Workspace: "z"}); err == nil { + t.Fatal("expected journal write failure") + } +} + +// TestDoctorPendingRecordSuppressesStaleFix proves a pending recovery record +// prevents the destructive "remove stale state entry" fix for that workspace. +func TestDoctorPendingRecordSuppressesStaleFix(t *testing.T) { + env := setupTestEnv(t) + + ws := models.NewWorkspace("ghosted", filepath.Join(env.wsDir, "ghosted-missing"), "main") + if err := env.svc.State.AddWorkspace(ws); err != nil { + t.Fatal(err) + } + if err := env.svc.ops().Write(&state.OperationRecord{Kind: state.OpCreate, Workspace: "ghosted"}); err != nil { + t.Fatal(err) + } + + if _, _, err := env.svc.Doctor(true); err != nil { + t.Fatalf("doctor --fix: %v", err) + } + if got, _ := env.svc.State.GetWorkspace("ghosted"); got == nil { + t.Fatal("pending record must prevent stale-state removal") + } +} + +// TestDoctorCorruptJournalSuppressesAllFixes proves an unreadable journal +// disables destructive fixes entirely (the affected workspace is unknown). +func TestDoctorCorruptJournalSuppressesAllFixes(t *testing.T) { + env := setupTestEnv(t) + + ws := models.NewWorkspace("ghosted", filepath.Join(env.wsDir, "ghosted-missing"), "main") + if err := env.svc.State.AddWorkspace(ws); err != nil { + t.Fatal(err) + } + opsDir := filepath.Join(env.groveDir, "operations") + if err := os.MkdirAll(opsDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(opsDir, "broken.json"), []byte("{not json"), 0o644); err != nil { + t.Fatal(err) + } + + if _, _, err := env.svc.Doctor(true); err != nil { + t.Fatalf("doctor --fix: %v", err) + } + if got, _ := env.svc.State.GetWorkspace("ghosted"); got == nil { + t.Fatal("corrupt journal must suppress all destructive fixes") + } +} diff --git a/internal/workspace/workspace.go b/internal/workspace/workspace.go index 2dabc43..5a3358a 100644 --- a/internal/workspace/workspace.go +++ b/internal/workspace/workspace.go @@ -12,6 +12,7 @@ import ( "github.com/nicksenap/grove/internal/gitops" "github.com/nicksenap/grove/internal/logging" "github.com/nicksenap/grove/internal/models" + "github.com/nicksenap/grove/internal/state" ) // BranchMode determines how a worktree's branch is provisioned. @@ -64,109 +65,12 @@ func (s *Service) Create(name, branch string, repoNames []string, repoMap map[st }) } -// CreateWithOpts creates a new workspace from the given options. It is the full -// implementation behind Create, additionally supporting per-repo branch tracking -// (BranchMode/TrackBranchRepo) and a persisted Source link. +// CreateWithOpts creates a new workspace from the given options. It is a +// backward-compatible shim over CreateWithResult that returns a synthesized +// error for non-success outcomes; new callers should prefer CreateWithResult to +// render ordered per-repository outcomes and meaningful exit codes. func (s *Service) CreateWithOpts(name string, opts CreateOpts) error { - branch := opts.Branch - repoNames := opts.Repos - repoMap := opts.RepoMap - cfg := opts.Cfg - - // Check duplicate - existing, err := s.State.GetWorkspace(name) - if err != nil { - return err - } - if existing != nil { - return fmt.Errorf("workspace %s already exists", name) - } - - logging.Info("creating workspace %q (branch=%s, repos=%v)", name, branch, repoNames) - - wsPath := filepath.Join(cfg.WorkspaceDir, name) - if err := os.MkdirAll(wsPath, 0o755); err != nil { - return fmt.Errorf("creating workspace dir: %w", err) - } - - ws := models.NewWorkspace(name, wsPath, branch) - ws.Source = opts.Source - - // Validate all repo names first - sourcePaths := make([]string, len(repoNames)) - for i, repoName := range repoNames { - sourcePath, ok := repoMap[repoName] - if !ok { - os.RemoveAll(wsPath) - return fmt.Errorf("repo %s not found", repoName) - } - sourcePaths[i] = sourcePath - } - - // Phase 1: parallel fetch (the slow network part) - console.Infof("fetching %d repos...", len(repoNames)) - var fetchWg sync.WaitGroup - for i, repoName := range repoNames { - fetchWg.Add(1) - go func(source, name string) { - defer fetchWg.Done() - if err := gitops.Fetch(source); err != nil { - console.Warningf(" %s: fetch failed, using local state", name) - } - }(sourcePaths[i], repoName) - } - fetchWg.Wait() - - // Phase 2: sequential worktree creation (for rollback safety) - var created []models.RepoWorktree - for i, repoName := range repoNames { - console.Infof("[%d/%d] %s", i+1, len(repoNames), repoName) - // Resolve the per-repo mode. With no TrackBranchRepo set, opts.BranchMode - // applies to every repo; otherwise only the named repo uses it. - mode := opts.BranchMode - if opts.TrackBranchRepo != "" && repoName != opts.TrackBranchRepo { - mode = BranchModeCreate - } - rw, err := provisionWorktreeNoFetch(sourcePaths[i], repoName, wsPath, branch, mode) - if err != nil { - logging.Error("workspace creation failed for %q — rolled back", name) - rollback(created) - os.RemoveAll(wsPath) - return fmt.Errorf("provisioning %s: %w", repoName, err) - } - created = append(created, *rw) - } - - ws.Repos = created - - // Run setup hooks (parallel) - if hasSetupHooks(ws) { - console.Infof("running setup hooks...") - } - s.runSetupHooks(ws) - - // Save state - if err := s.State.AddWorkspace(ws); err != nil { - rollback(created) - os.RemoveAll(wsPath) - return err - } - - // Record stats - s.Stats.RecordCreated(ws) - - // Write .mcp.json - writeMCPConfig(ws) - - logging.Info("workspace %q created at %s", name, wsPath) - console.Successf("Workspace %s created at %s", name, wsPath) - - // Write GROVE_CD_FILE if set - if cdFile := os.Getenv("GROVE_CD_FILE"); cdFile != "" { - os.WriteFile(cdFile, []byte(wsPath), 0o644) - } - - return nil + return s.CreateWithResult(name, opts).toError() } func provisionWorktree(sourcePath, repoName, wsPath, branch string) (*models.RepoWorktree, error) { @@ -235,22 +139,6 @@ func provisionWorktreeNoFetch(sourcePath, repoName, wsPath, branch string, mode }, nil } -func rollback(repos []models.RepoWorktree) { - for _, r := range repos { - gitops.WorktreeRemove(r.SourceRepo, r.WorktreePath) - } -} - -func hasSetupHooks(ws models.Workspace) bool { - for _, r := range ws.Repos { - groveCfg, _ := gitops.ReadGroveConfig(r.SourceRepo) - if groveCfg != nil && len(groveCfg.Setup) > 0 { - return true - } - } - return false -} - func (s *Service) runSetupHooks(ws models.Workspace) { var wg sync.WaitGroup for _, r := range ws.Repos { @@ -958,11 +846,18 @@ func (s *Service) Doctor(fix bool) ([]models.DoctorIssue, int, error) { return nil, 0, err } + // Scan recovery records first so legacy fixes can be suppressed for any + // workspace with a pending operation — its state may be mid-reconciliation + // and must not be treated as merely stale. A corrupt/unreadable journal + // disables all destructive fixes, since the affected workspace is unknown. + recIssues, pending, journalOK := s.checkRecoveryRecords() + var issues []models.DoctorIssue fixed := 0 for _, ws := range workspaces { - f, iss := s.checkWorkspaceExists(ws, fix) + allowFix := fix && journalOK && !pending[ws.Name] + f, iss := s.checkWorkspaceExists(ws, allowFix) if f > 0 { fixed += f } @@ -971,14 +866,85 @@ func (s *Service) Doctor(fix bool) ([]models.DoctorIssue, int, error) { continue } - f, iss = s.checkWorkspaceRepos(&ws, fix) + f, iss = s.checkWorkspaceRepos(&ws, allowFix) fixed += f issues = append(issues, iss...) } + // Recovery records are reported but never mutated here; repair of each record + // kind is delivered by later tasks (doctor --fix). + issues = append(issues, recIssues...) + return issues, fixed, nil } +// checkRecoveryRecords surfaces durable operation records left behind by an +// interrupted or crashed mutation. It is read-only and returns the set of +// workspace names that have a pending record plus whether the journal was fully +// readable, so callers can suppress unsafe legacy fixes. +func (s *Service) checkRecoveryRecords() ([]models.DoctorIssue, map[string]bool, bool) { + recs, err := s.ops().List() + pending := map[string]bool{} + journalOK := err == nil + var issues []models.DoctorIssue + if err != nil { + issues = append(issues, models.DoctorIssue{ + Workspace: "", + Repo: nil, + Issue: "corrupt recovery record: " + err.Error(), + SuggestedAction: "inspect ~/.grove/operations", + }) + } + for i := range recs { + rec := recs[i] + if rec.Workspace != "" { + pending[rec.Workspace] = true + } + issue := "interrupted " + string(rec.Kind) + " operation" + if rec.Phase != "" { + issue += " (phase " + rec.Phase + ")" + } + if rec.LastError != "" { + issue += ": " + rec.LastError + } + // Repair of recovery records is not yet implemented (Tasks 10-13); guide + // the user to resume the original command rather than over-promising + // doctor --fix. + action := "re-run the original " + resumeCommand(rec.Kind) + " to resume" + if !rec.Supported() { + action = "unsupported recovery record; inspect ~/.grove/operations" + } + issues = append(issues, models.DoctorIssue{ + Workspace: rec.Workspace, + Repo: nil, + Issue: issue, + SuggestedAction: action, + }) + } + return issues, pending, journalOK +} + +// resumeCommand maps an operation kind to the gw command a user re-runs to +// resume it. +func resumeCommand(k state.OperationKind) string { + switch k { + case state.OpCreate: + return "gw create" + case state.OpAdd: + return "gw add-repo" + case state.OpRemove: + return "gw remove-repo" + case state.OpDelete: + return "gw delete" + case state.OpSync: + return "gw sync" + case state.OpRename: + return "gw rename" + default: + return "gw command" + } +} + func (s *Service) checkWorkspaceExists(ws models.Workspace, fix bool) (int, []models.DoctorIssue) { if _, err := os.Stat(ws.Path); err == nil { return 0, nil diff --git a/internal/workspace/workspace_test.go b/internal/workspace/workspace_test.go index 2855ce4..f84bbb2 100644 --- a/internal/workspace/workspace_test.go +++ b/internal/workspace/workspace_test.go @@ -52,6 +52,7 @@ func setupTestEnv(t *testing.T) *testEnv { svc := &Service{ State: store, Stats: &stats.Tracker{StatsPath: filepath.Join(groveDir, "stats.json"), NowFn: time.Now}, + Ops: state.NewOperationStore(groveDir), RunCmd: prodRunCmd, RunCmdSilent: prodRunCmdSilent, } diff --git a/tasks/plan.md b/tasks/plan.md new file mode 100644 index 0000000..749cbf1 --- /dev/null +++ b/tasks/plan.md @@ -0,0 +1,467 @@ +# Implementation Plan: Transactional and Recoverable Workspace Operations + +**Target:** [GitHub issue #59](https://github.com/nicksenap/grove/issues/59) +**Status:** Approved for implementation; no implementation has started. +**Related boundary:** [Issue #63](https://github.com/nicksenap/grove/issues/63) owns the public machine-output envelope and global `--format json` contract. + +## Overview + +Grove must keep `~/.grove/state.json`, workspace paths, Git worktree registrations, and workspace branches consistent across failures and concurrent `gw` processes. Each mutating operation will complete, compensate everything it created, or leave a durable recovery record that the original command and `gw doctor` can explain and repair. + +This plan delivers complete operation paths rather than implementing all storage, all Git wrappers, then all CLI work. The two foundation tasks are limited to an end-to-end state mutation boundary and recovery-record lifecycle; each later task carries one user operation through service behavior, failure recovery, CLI exit handling, and tests. + +## Scope + +- Cross-process serialization and lost-update-safe state mutations. +- Durable, operation-specific recovery records for interrupted mutations. +- Transactional `create` and multi-repo `add-repo`. +- Safe, retryable `remove-repo` and `delete` with explicit destructive force. +- Aggregated per-repository and per-workspace outcomes with meaningful exits. +- Four-way reconciliation of state, filesystem, Git worktree registration, and branch state. +- Idempotent repair for every recovery-record type, including rename and sync cleanup. +- Fault-injection, race, deterministic e2e, and documentation coverage. + +## Non-goals + +- Distributed or cloud state. +- A general-purpose transaction framework. +- The public versioned CLI envelope/global machine mode; that belongs to issue #63. +- New task/PR intake, blueprint, Oven, MCP, or agent features. +- Hiding Git failures that require user action. +- Locking unrelated best-effort files such as `stats.json`. +- Redefining successful branch-cleanup ownership semantics beyond what safe compensation and repair require. + +## Current Evidence + +- State mutators are unlocked `Load` → edit → `Save` sequences (`internal/state/state.go`), so concurrent processes can lose updates. +- `Save` uses one shared `.tmp` path and does not implement the fsync guarantees claimed by OpenWiki. +- Create rollback removes worktrees but not newly created branches and drops rollback errors (`internal/workspace/workspace.go`). +- `AddRepos` validates and provisions incrementally without compensation. +- Worktree removal is always forced; delete/remove fall back to unchecked `os.RemoveAll` and suppress failures (`internal/gitops/gitops.go`, `internal/workspace/workspace.go`). +- Doctor only checks path existence and can discard repair evidence. +- Sync reports repository failures to the console but returns `nil`; rename ignores worktree-repair failures. +- Existing real-Git fixtures are useful, but current rollback and partial-delete tests do not inject post-mutation failures. +- Planning baseline: `go test ./...` and `go test -race ./internal/state ./internal/workspace` pass. This does not exercise cross-process lost updates. + +## Conditional Lifecycle Contract for Human Approval + +The task criteria below assume approval of this contract. If any item changes, update the affected tasks before implementation. + +1. **Mutation ownership API:** `Store.WithMutation(ctx, func(*state.Mutation) error)` acquires one stable advisory lock, loads the authoritative snapshot, and passes a caller-owned handle with locked `Get/Add/Update/Remove`, recovery-record CRUD, and `Commit` methods. Service code never calls a lock-acquiring public mutator while holding this handle. Raw full-snapshot `Save` becomes internal or revision-checked so stale snapshots cannot overwrite newer state. +2. **Lock behavior:** wait up to 30 seconds, then return retryable `STATE_LOCK_TIMEOUT`; same-name/same-workspace conflicts are revalidated under the lock and return deterministic conflict outcomes. The lock file is never unlinked. +3. **Durable state writes:** use a unique temp file in the destination directory, sync it, atomically rename it, and sync the parent directory. +4. **Small recovery journal:** versioned, operation-specific records live under `~/.grove/operations/`. A record is written before the first **workspace** Git/filesystem mutation, captures repository phases/resource ownership, and is removed only after state commit or complete compensation. Supported kinds are create, add, remove, delete, sync cleanup, and rename. +5. **Source cloning boundary:** remote-URL cloning is source-repository acquisition, not workspace mutation. Validate command inputs before cloning; successful clones are intentionally retained as reusable local sources if later workspace creation/addition fails. Clone failures are reported as preflight failures and tested, but clones are not compensated. +6. **Typed service outcomes:** issue #59 introduces ordered `OperationResult`/`RepoOutcome` values and stable internal error codes. Human CLI commands render all repository details to stderr and return non-zero on failed/partial/pending outcomes. Public machine envelopes/`--format json` are deferred to issue #63. Existing `doctor --json` remains compatible and gains additive stable diagnosis/repair fields required by #59. +7. **Force behavior:** add `--yes/-y` for confirmation-only bypass. `--force/-f` implies yes and authorizes dirty-worktree or validated filesystem fallback. Force never bypasses canonical workspace-boundary, source identity, or expected worktree-path checks. +8. **Hook boundary:** global `pre_delete` and per-repo teardown run outside the state lock; the service acquires the lock and repeats complete identity/registration/dirty preflight immediately afterward. An aborting global hook stops that workspace; warning hooks are included in aggregate outcomes. Teardown failure blocks that repository and remains retryable. Setup/post-create run after commit/lock release; failure yields a partial/non-zero outcome without undoing a valid workspace. +9. **Exit behavior:** exit 0 only for complete success or explicit cancellation. Precondition failure, partial success, cleanup pending, and repair failure exit non-zero after all requested targets are attempted. +10. **Doctor behavior:** existing `doctor --json` stays a bare array for compatibility. Diagnosis reports recovery records from the first lifecycle slice onward. `doctor --fix` applies versioned actions with precondition checks; actionable diagnosis and repair failures return non-zero. + +## Dependency Graph + +```text +Locked Mutation handle + durable writer + │ + ▼ +Recovery records + injected mutation seams + doctor visibility + │ + ┌─────────┴─────────┐ + ▼ ▼ +Transactional create Safe remove preflight + │ │ + ▼ ▼ +Atomic add-repo Retryable remove-repo + │ + ▼ + Retryable workspace delete + │ + ▼ + Multi-workspace aggregation + └──────────────┬──────────────┘ + ▼ + Four-way doctor plan + ┌──────┴──────┐ + ▼ ▼ + Constructive repair Destructive repair + └──────┬──────┘ + ▼ + Sync + rename recovery + │ + ▼ + Race/e2e gates + docs +``` + +## Verification Convention + +Each implementation task names the exact tests it must add. Before running a targeted command, verify they exist with `go test -list '' | grep -q .`; a zero-test `go test -run` is not acceptance evidence. Fault injection must be instance-scoped/test-only; filesystem-permission tricks are not the primary seam. + +--- + +## Phase 1: State and Recovery Foundations + +### Task 1: Serialize state mutations across processes + +**Description:** Implement one complete lock-backed state mutation path from lock acquisition through authoritative read, revision-safe edits, durable commit, and release. Migrate public Store mutators to wrappers over that path without allowing nested acquisition. + +**Acceptance criteria:** +- [ ] `Mutation` owns locked reads/edits/commit; raw stale snapshot replacement cannot overwrite a newer revision. +- [ ] Concurrent helper processes preserve independent updates; a same-record conflict has one deterministic winner and one conflict result. +- [ ] Callback/write/sync/rename/process-death failures preserve the last valid JSON, release the lock, and leave no shared temp collision. + +**Verification:** +- [ ] Add and run `TestStoreConcurrentSubprocessMutations`, `TestStoreMutationFailurePreservesState`, and `TestStateLockReleasedAfterProcessExit` repeatedly under `-race`. +- [ ] Cross-compile the state package/test binary for released Darwin/Linux targets. + +**Dependencies:** None +**Files likely touched:** `internal/state/state.go`, `internal/state/state_test.go`, new `internal/state/lock_unix.go`, `go.mod`, `go.sum` +**Estimated scope:** Medium (4–5 files) + +### Task 2: Persist and surface recovery records + +**Description:** Add the fixed recovery-record schema/store, instance-scoped Git/filesystem/state fault seams, and generic doctor visibility. This task proves a record can survive interruption and be reported before any operation-specific mutation is migrated. + +**Acceptance criteria:** +- [ ] Records for create/add/remove/delete/sync/rename atomically capture intent, phase, resource ownership, ordered repository outcomes, and the latest retryable error. +- [ ] The workspace service receives a narrow injectable mutation backend; release binaries cannot enable test failpoints. +- [ ] `gw doctor` reports incomplete/stale records without mutation, and interrupted helper-process records survive restart. + +**Verification:** +- [ ] Add and run `TestOperationRecordRoundTrip`, `TestOperationRecordSurvivesProcessExit`, and `TestDoctorReportsRecoveryRecord`. +- [ ] Confirm old/no-operations state loads unchanged and no temporary record files remain after successful completion. + +**Dependencies:** Task 1 +**Files likely touched:** new `internal/state/operation.go`, operation tests, `internal/workspace/service.go`, a focused workspace fault-test helper, `internal/workspace/workspace.go` or new doctor helper +**Estimated scope:** Medium (4–5 files) + +## Checkpoint: State Foundation (after Tasks 1–2) + +- [ ] Concurrent state updates are lossless across processes. +- [ ] Recovery intent and test seams exist before lifecycle work begins. +- [ ] Doctor can identify a stranded record. +- [ ] Existing state fixtures still load unchanged. +- [ ] Human approves the conditional lifecycle contract before Task 3. + +--- + +## Phase 2: Constructive Operations + +### Task 3: Make workspace creation transactional and recoverable + +**Description:** Carry create from validated CLI inputs through source resolution, operation record, authoritative duplicate check, branch/worktree provisioning, one state commit, reverse-order compensation, original-command retry, and typed CLI outcome. + +**Acceptance criteria:** +- [ ] Validate branch/name/repo inputs before optional cloning; retained-clone behavior is explicit. Failures at mkdir, branch create, worktree add, a later repo, or state commit fully compensate or remain recoverable. +- [ ] Compensation deletes only operation-created resources, preserves pre-existing branches, aggregates rollback errors, and handles track mode. +- [ ] Success aligns state/path/registration/branch and clears the record; `gw create` can safely resume/finish its own pending record and reports setup/post-create failures as partial after commit. + +**Verification:** +- [ ] Add exact real-Git/fault tests for branch-before-worktree failure, later-repo failure, final commit failure, rollback failure, track mode, retained clone, and same-name concurrency. +- [ ] Add command tests proving all repo outcomes render and partial/pending create exits non-zero. + +**Dependencies:** Tasks 1–2 +**Files likely touched:** `internal/workspace/service.go`, `internal/workspace/workspace.go` or new `internal/workspace/create.go`, new `internal/workspace/result.go`, workspace tests, `cmd/create.go` +**Estimated scope:** Medium (4–5 files) + +### Task 4: Make multi-repository additions atomic and recoverable + +**Description:** Apply the constructive transaction to `add-repo`: validate all inputs, retain explicitly acquired clones, provision every requested repo, commit once, compensate all additions on failure, and resume a pending add safely. + +**Acceptance criteria:** +- [ ] Invalid/later failing input produces no committed addition and no operation-created branch/worktree leak; retained source clones are documented outcomes. +- [ ] Final state-update failure restores the original workspace exactly or leaves a record that `gw add-repo` and doctor can explain/retry. +- [ ] Duplicate/already-present inputs are deterministic, setup runs only after commit, and every requested repository appears in the typed result. + +**Verification:** +- [ ] Add exact tests for later unknown repo preflight, repo-2 branch/worktree failure, commit failure, rollback failure, retained clone, retry, and idempotent duplicates. +- [ ] Add command tests proving partial/pending add exits non-zero after rendering all outcomes. + +**Dependencies:** Task 3 +**Files likely touched:** `internal/workspace/workspace.go` or new `internal/workspace/add.go`, workspace tests, `cmd/addrepo.go`, command tests +**Estimated scope:** Medium (3–4 files) + +## Checkpoint: Constructive Operations (after Tasks 3–4) + +- [ ] Create/add success and every injected failure satisfy state/filesystem/Git invariants. +- [ ] The original command can resume its pending operation; doctor reports it as a fallback. +- [ ] Recovery records clear after complete success/compensation. +- [ ] `just check`, `just build`, and focused race tests pass. + +--- + +## Phase 3: Destructive Operations + +### Task 5: Refuse unsafe repository removal by default + +**Description:** Deliver a complete default `remove-repo` path that runs hooks outside the lock, then performs authoritative all-target preflight and clean Git removal without unchecked filesystem fallback. + +**Acceptance criteria:** +- [ ] After teardown, immediately revalidate canonical source/path identity, workspace containment, symlink policy, exact Git registration/branch, and dirty state before any removal. +- [ ] If any target fails preflight, mutate none; dirty, unexpected, unregistered, detached/mismatched, or source-missing targets remain intact and in state. +- [ ] Clean expected targets remove successfully with typed per-repo outcomes and no unconditional `RemoveAll`. + +**Verification:** +- [ ] Add exact gitops tests for non-forced dirty refusal and porcelain identity parsing. +- [ ] Add exact workspace/command tests for dirty, unexpected path, symlink escape, registration mismatch, branch mismatch, teardown-induced dirty state, and clean success. + +**Dependencies:** Tasks 1–3 (independent of Task 4) +**Files likely touched:** `internal/gitops/gitops.go`, gitops tests, new `internal/workspace/cleanup.go`, cleanup tests, `cmd/removerepo.go` +**Estimated scope:** Medium (5 files) + +### Task 6: Make forced and failed repository removal retryable + +**Description:** Extend the safe removal path with `--yes`/bounded `--force`, durable phase progress, aggregated runtime failures, and idempotent retry through both `remove-repo` and doctor visibility. + +**Acceptance criteria:** +- [ ] Force may remove a dirty expected target or use filesystem fallback only after canonical identity/boundary checks; it never authorizes an unexpected path. +- [ ] Teardown, worktree removal, forced filesystem removal, branch deletion, or state commit failure remains recorded with repair inputs and a non-zero result. +- [ ] Mixed outcomes preserve failed entries; retry treats already-absent completed stages as success and converges. + +**Verification:** +- [ ] Add exact fault tests for every removal phase, mixed two-repo outcomes, state-write failure, retry, and second-call idempotence. +- [ ] Add command tests for confirmation-only `--yes`, destructive `--force`, aggregate rendering, and exit status. + +**Dependencies:** Task 5 +**Files likely touched:** `internal/workspace/cleanup.go`, cleanup/workspace tests, `cmd/removerepo.go`, command tests +**Estimated scope:** Medium (4 files) + +### Task 7: Make workspace deletion safe and retryable + +**Description:** Route one workspace deletion through the cleanup engine, preserving state/recovery data until repository, workspace-root, branch, and state-removal stages complete; original-command retry must finish interrupted deletion. + +**Acceptance criteria:** +- [ ] Global `pre_delete`/repo teardown outcomes are collected outside the lock; service preflight repeats afterward before MCP/path mutation. +- [ ] Per-repo, root-removal, branch, hook, and state-write failures yield cleanup-pending/non-zero without a false success banner, premature state removal, or premature stats event. +- [ ] Retry resumes idempotently and clears workspace/recovery state only when every required stage succeeds. + +**Verification:** +- [ ] Add exact tests for dirty/unexpected refusal, hook-induced changes, each runtime phase failure, root-removal failure, commit failure, and retry convergence. +- [ ] Local e2e: default dirty delete preserves data; forced retry completes. + +**Dependencies:** Task 6 +**Files likely touched:** `internal/workspace/workspace.go` or new `internal/workspace/delete.go`, deletion tests, `cmd/delete.go`, command tests, `e2e/run.sh` +**Estimated scope:** Medium (4–5 files) + +### Task 8: Aggregate multi-workspace deletion + +**Description:** Make interactive multi-select (and, if approved, multiple names) attempt every workspace with one service instance, aggregate hook/service outcomes, and exit only after all targets finish. + +**Acceptance criteria:** +- [ ] Failure/abort for one workspace does not prevent later selected workspaces from being attempted. +- [ ] Final output preserves request order and includes each workspace/repository failure with one non-zero exit. +- [ ] Completed workspaces remain completed while pending workspaces retain recovery data. + +**Verification:** +- [ ] Add exact command tests for continue-after-hook-failure, continue-after-service-failure, ordering, and aggregate exit. +- [ ] E2E: one dirty and one clean selected workspace leaves the dirty one discoverable and deletes the clean one. + +**Dependencies:** Task 7 +**Files likely touched:** `cmd/delete.go`, new `cmd/delete_test.go`, optionally shared human-result rendering +**Estimated scope:** Small (2–3 files) + +## Checkpoint: Destructive Safety (after Tasks 5–8) + +- [ ] Default delete/remove cannot erase dirty or unexpected paths. +- [ ] Force is path-bounded and tested. +- [ ] Original commands can resume partial cleanup; doctor reports pending records. +- [ ] Every selected workspace is attempted before aggregate exit. +- [ ] Human reviews changed `--yes`/`--force` behavior. + +--- + +## Phase 4: Reconciliation and Repair + +### Task 9: Produce four-way reconciliation plans + +**Description:** Upgrade diagnosis from path existence to a read-only plan comparing state entries, canonical filesystem paths, `git worktree list --porcelain`, branches, and all recovery-record kinds. + +**Acceptance criteria:** +- [ ] Doctor distinguishes healthy, state-only, path-only, registration-only, branch-mismatched, dirty/unexpected, and operation-pending conditions without deleting evidence. +- [ ] Additive JSON fields provide stable issue/action codes, preconditions, destructive flag, and ordered workspace/repo context while preserving the existing array shape. +- [ ] Diagnosis-only runs have no side effects and return the approved healthy/actionable exit status. + +**Verification:** +- [ ] Add exact tests for every four-way mismatch and each create/add/remove/delete/sync/rename record kind. +- [ ] Add compatibility tests that existing `doctor --json | jq 'length'` consumers still work. + +**Dependencies:** Tasks 2–8 +**Files likely touched:** `internal/gitops/gitops.go`, gitops tests, new `internal/workspace/reconcile.go`, reconciliation tests, `cmd/doctor.go` +**Estimated scope:** Medium (5 files) + +### Task 10: Repair constructive recovery records idempotently + +**Description:** Make `doctor --fix` apply create/add compensation or completion through the same constructive primitives, with precondition checks and repeatable outcomes. + +**Acceptance criteria:** +- [ ] Pending create/add can complete or compensate without manual branch/worktree/state edits. +- [ ] Unknown ownership or changed preconditions stop destructive action conservatively and retain the record with a non-zero result. +- [ ] A second fix after convergence performs zero mutations. + +**Verification:** +- [ ] Add exact tests for interrupted branch creation, worktree creation, final state commit, failed compensation, stale precondition, and second-fix no-op. +- [ ] E2E: injected pending create/add → doctor diagnosis → fix → healthy second fix. + +**Dependencies:** Tasks 3–4, 9 +**Files likely touched:** new `internal/workspace/repair.go`, constructive repair tests, `cmd/doctor.go`, command tests, transactional e2e +**Estimated scope:** Medium (5 files) + +### Task 11: Repair destructive recovery records idempotently + +**Description:** Extend `doctor --fix` to resume remove/delete through the same bounded cleanup engine and persist applied/skipped/failed action results. + +**Acceptance criteria:** +- [ ] Pending remove/delete completes safely when preconditions still match and never widens force beyond the recorded authorization. +- [ ] Stale/failed repair remains pending with actionable per-repo detail and non-zero exit. +- [ ] Reapplying a completed destructive plan is a no-op. + +**Verification:** +- [ ] Add exact tests for each pending cleanup stage, stale paths/registrations, mixed outcomes, repair failure, and second-fix no-op. +- [ ] E2E: partial delete → plan → fix → healthy second fix. + +**Dependencies:** Tasks 6–10 +**Files likely touched:** `internal/workspace/repair.go`, destructive repair tests, `cmd/doctor.go`, command tests, transactional e2e +**Estimated scope:** Medium (5 files) + +## Checkpoint: Core Recovery (after Tasks 9–11) + +- [ ] Create/add/remove/delete failures are diagnosable and repairable without Git surgery. +- [ ] Doctor never discards the only repair inputs. +- [ ] Fix is idempotent and emits compatible machine-readable issue/action results. + +--- + +## Phase 5: Remaining Mutation Paths + +### Task 12: Return structured sync outcomes and repair failed aborts + +**Description:** Replace console-only sync failures with ordered per-repository outcomes while retaining parallel execution, and add doctor diagnosis/repair for the only durable sync recovery state: a rebase that could not be aborted. + +**Acceptance criteria:** +- [ ] Every repository returns a stable phase/status for fetch, dirty precondition, base resolution, hooks, rebase, and abort; aggregate failure is non-zero after all repos finish. +- [ ] Rebase conflict with successful abort is a normal failed outcome; abort failure writes a sync recovery record visible to doctor. +- [ ] `doctor --fix` retries/validates abort cleanup, clears the record on success, and is idempotent. + +**Verification:** +- [ ] Add exact tests for every sync phase, mixed success/failure ordering, conflict+abort success, abort failure record, repair, and second-fix no-op. +- [ ] E2E: one successful and one failing repo both appear before non-zero exit. + +**Dependencies:** Tasks 2–3, 9–11 +**Files likely touched:** `internal/workspace/workspace.go` or new `internal/workspace/sync.go`, sync/repair tests, `cmd/sync_cmd.go`, `cmd/doctor.go`, e2e tests +**Estimated scope:** Medium (5 files) + +### Task 13: Harden rename and replace with repair support + +**Description:** Bring rename under the same lock/record/compensation contract and extend doctor for incomplete rename. Keep replace explicitly composed: old deletion commits before new creation begins, and no creation follows blocked/pending deletion. + +**Acceptance criteria:** +- [ ] Rename failures at state update, filesystem rename, worktree repair, or compensation leave original/new consistency or a doctor-visible repair record. +- [ ] Doctor can conservatively finish/revert rename and a second fix is a no-op. +- [ ] Replace never creates the new workspace unless old deletion fully completes and clearly reports when deletion committed before create failed. + +**Verification:** +- [ ] Add exact tests for every rename phase, concurrent rename/create/delete, repair, and second-fix no-op. +- [ ] Add command/e2e tests for blocked dirty replace and delete-committed/create-failed reporting. + +**Dependencies:** Tasks 1–3, 7, 9–11 +**Files likely touched:** `internal/workspace/workspace.go` or new `internal/workspace/rename.go`, rename/repair tests, `cmd/create.go`, `cmd/rename.go`, command/e2e tests +**Estimated scope:** Medium (5 files) + +## Checkpoint: Complete Mutation Contract (after Tasks 12–13) + +- [ ] Create/add/remove/delete/sync return typed ordered outcomes and meaningful exits. +- [ ] Rename/replace follow documented transaction boundaries. +- [ ] Every recovery-record kind has doctor diagnosis and idempotent repair. +- [ ] No mutating service path bypasses the `Mutation` handle. + +--- + +## Phase 6: Validation and Documentation + +### Task 14: Add race and transactional e2e gates + +**Description:** Wire the already-working fault tests into required CI and run the full deterministic local e2e lifecycle with a race-instrumented binary. Optional external-network clone checks remain a separately documented ordinary-e2e exclusion. + +**Acceptance criteria:** +- [ ] CI runs `go test -race ./...` plus repeated cross-process contention tests. +- [ ] Every deterministic/local e2e section runs with `GW_BIN` built using `-race`, covering dirty refusal, partial failure, recovery, aggregate exits, and convergent repair. +- [ ] Existing optional external-network tests run in ordinary e2e and are the only documented race-e2e exclusion. + +**Verification:** +- [ ] `just check && go test -race ./...` +- [ ] `go build -race -o /tmp/gw-race ./cmd/gw && GW_E2E_SKIP_NETWORK=1 GW_BIN=/tmp/gw-race bash e2e/run.sh` +- [ ] `just e2e` + +**Dependencies:** Tasks 1–13 +**Files likely touched:** `.github/workflows/ci.yml`, `Justfile`, `e2e/run.sh` and/or new `e2e/transactional.sh` +**Estimated scope:** Small (3–4 files) + +### Task 15: Align lifecycle documentation and release notes + +**Description:** Update architecture, workflows, and operations to match the approved/implemented boundaries, then document shipping behavior without claiming issue #63's public machine contract. + +**Acceptance criteria:** +- [ ] Docs describe the `Mutation` boundary, source-clone retention, recovery records, hook timing/revalidation, force/exit behavior, and doctor diagnosis/fix in timeless language. +- [ ] Correct stale claims about mutex protection, fsync, state JSON shape, force skipping cleanup, and current doctor capabilities. +- [ ] Release notes identify behavior changes to `--yes`/`--force`, actionable doctor exits, and surfaced partial failures. + +**Verification:** +- [ ] Review examples against `gw --help` and deterministic e2e output. +- [ ] `just check && just build && just e2e` + +**Dependencies:** Tasks 1–14 +**Files likely touched:** `openwiki/architecture.md`, `openwiki/workflows.md`, `openwiki/operations.md`, `CHANGELOG.md` at release time +**Estimated scope:** Medium (3–4 files) + +## Checkpoint: Complete (after Tasks 14–15) + +- [ ] Fault-injection coverage exists for every mutation phase and expected test names are present. +- [ ] Concurrent mutations cannot lose state updates. +- [ ] Interrupted operations are diagnosable and repairable without manual Git surgery. +- [ ] Default deletion preserves dirty/unexpected paths. +- [ ] Automation receives non-zero exit plus complete per-repository details; issue #63 can consume typed results. +- [ ] Unit/full race, build, race-instrumented deterministic e2e, and ordinary full e2e gates pass. +- [ ] Documentation/release notes match runtime behavior. +- [ ] Human approval is recorded before merge/release. + +## Fault-Injection Matrix + +| Area | Required phases | +|---|---| +| State | lock, load, callback, temp write, file sync, rename, directory sync, holder process death | +| Source acquisition | input validation, clone failure, retained successful clone followed by workspace failure | +| Create | root mkdir, branch create, worktree add/track per repo, state commit, worktree rollback, branch rollback | +| Add | all-input preflight, branch/worktree per repo, state update, rollback | +| Remove | post-hook identity/dirty preflight, worktree remove, forced filesystem remove, branch delete, state update | +| Delete | hook aggregation/revalidation, remove phases, root removal, MCP/stats policy, state removal, next workspace | +| Sync | fetch, status, base, counts, hooks, rebase, abort, abort repair | +| Rename | state transition, filesystem rename, each worktree repair, compensation, doctor repair | +| Doctor | scan each source, plan, precondition recheck, apply, result persistence | + +Every case asserts state JSON validity, filesystem paths, `git worktree list --porcelain`, relevant branches, typed result, CLI exit, recovery visibility, and convergence. + +## Risks and Mitigations + +| Risk | Impact | Mitigation | +|---|---|---| +| Global lock blocks during slow local mutation | Medium | Keep fetch/source acquisition and hooks outside; repeat authoritative checks inside; use a 30-second actionable timeout. | +| Nested mutation deadlocks | High | Explicit non-reentrant `Mutation` handle; no public lock-acquiring calls inside; fail nested/test misuse immediately. | +| Hook changes target after preflight | High | Run hooks outside lock, then perform the complete authoritative preflight immediately before mutation. | +| Recovery deletes a pre-existing branch | High | Persist operation ownership before mutation; repair is conservative when ownership is unknown. | +| `--force` meaning changes | High | Add `--yes`, make implications explicit in help/results/docs, and add compatibility tests. | +| Public JSON work leaks into #59 | Medium | Keep typed results internal/human-rendered; preserve only additive doctor JSON; defer global envelope to #63. | +| Journal grows stale | Medium | Doctor reports stale records; command retry/fix removes completed records; repair is idempotent. | +| Permission-based tests are flaky | Medium | Add instance-scoped seams in Task 2; keep real Git for invariant verification. | +| Setup/teardown shell side effects cannot roll back | Medium | Keep hooks outside core resource atomicity, revalidate after destructive hooks, and surface failures. | + +## Human Review Questions + +1. Approve the explicit `Mutation` handle with a 30-second lock timeout? +2. Approve `~/.grove/operations/` as the minimal crash-recovery journal? +3. Approve `--yes` for confirmation bypass and `--force` as `--yes` plus bounded destructive authorization? +4. Approve hook behavior: destructive hooks run outside the lock followed by full revalidation; teardown failure blocks that repo; setup/post-create failure returns partial after commit? +5. Approve actionable `gw doctor` findings returning non-zero while preserving the current `doctor --json` array shape? +6. Approve successful remote source clones being retained when later workspace mutation fails? + +## Definition of Done + +For every task: acceptance criteria pass; named tests exist and fail without the change; existing tests remain green; edge/error paths are covered; formatting/static analysis pass; no unrelated refactor is included; and user-facing behavior is documented. For the epic: every recovery kind is repaired at runtime, integration/race/e2e gates pass, backward compatibility is reviewed, and a human approves both this plan and the final implementation. diff --git a/tasks/todo.md b/tasks/todo.md new file mode 100644 index 0000000..a4f1678 --- /dev/null +++ b/tasks/todo.md @@ -0,0 +1,96 @@ +# Issue #59 Task List + +Source plan: [`tasks/plan.md`](plan.md) +Target: [Make workspace operations transactional and recoverable](https://github.com/nicksenap/grove/issues/59) + +## Human Review Gate + +- [x] Approve explicit `Mutation` handle and 30-second lock timeout. +- [x] Approve minimal `~/.grove/operations/` recovery journal. +- [x] Approve `--yes` and bounded destructive `--force` semantics. +- [x] Approve hook timing, revalidation, and failure policy. +- [x] Approve actionable `gw doctor` findings returning non-zero. +- [x] Approve retention of successfully cloned source repos after later workspace failure. + +Approved by the human reviewer. Update the plan before implementation if any decision changes. + +## Phase 1: State and Recovery Foundations + +- [x] **Task 1:** Serialize state mutations across processes. *(Dependencies: none)* +- [x] **Task 2:** Persist and surface recovery records. *(Dependencies: Task 1)* + +### Checkpoint: State Foundation + +- [x] Concurrent subprocess updates are lossless. +- [x] Recovery records and fault seams survive interruption. +- [x] Doctor can identify a stranded record. +- [x] Existing state JSON remains compatible. +- [x] Human approves the lifecycle contract. + +## Phase 2: Constructive Operations + +- [x] **Task 3:** Make workspace creation transactional and recoverable. *(Dependencies: Tasks 1–2)* +- [ ] **Task 4:** Make multi-repository additions atomic and recoverable. *(Dependencies: Task 3)* + +### Checkpoint: Constructive Operations + +- [ ] Create/add failures compensate fully or remain recoverable. +- [ ] Original commands can safely resume pending operations. +- [ ] No operation-created branch/worktree leaks after successful compensation. +- [ ] Focused race tests, `just check`, and `just build` pass. + +## Phase 3: Destructive Operations + +- [ ] **Task 5:** Refuse unsafe repository removal by default. *(Dependencies: Tasks 1–3)* +- [ ] **Task 6:** Make forced and failed repository removal retryable. *(Dependencies: Task 5)* +- [ ] **Task 7:** Make workspace deletion safe and retryable. *(Dependencies: Task 6)* +- [ ] **Task 8:** Aggregate multi-workspace deletion. *(Dependencies: Task 7)* + +### Checkpoint: Destructive Safety + +- [ ] Default cleanup preserves dirty and unexpected paths. +- [ ] Force remains bounded by canonical path/source checks. +- [ ] Original commands can resume partial cleanup; doctor reports it. +- [ ] Every selected workspace is attempted before aggregate exit. +- [ ] Human reviews CLI behavior changes. + +## Phase 4: Reconciliation and Repair + +- [ ] **Task 9:** Produce four-way reconciliation plans. *(Dependencies: Tasks 2–8)* +- [ ] **Task 10:** Repair constructive recovery records idempotently. *(Dependencies: Tasks 3–4, 9)* +- [ ] **Task 11:** Repair destructive recovery records idempotently. *(Dependencies: Tasks 6–10)* + +### Checkpoint: Core Recovery + +- [ ] Create/add/remove/delete failures are diagnosable and repairable. +- [ ] Doctor preserves repair inputs and compatibility. +- [ ] Reapplying completed repair is a no-op. +- [ ] Recovery succeeds without manual Git surgery. + +## Phase 5: Remaining Mutation Paths + +- [ ] **Task 12:** Return structured sync outcomes and repair failed aborts. *(Dependencies: Tasks 2–3, 9–11)* +- [ ] **Task 13:** Harden rename and replace with repair support. *(Dependencies: Tasks 1–3, 7, 9–11)* + +### Checkpoint: Complete Mutation Contract + +- [ ] Create/add/remove/delete/sync return ordered typed outcomes and meaningful exits. +- [ ] Rename/replace follow documented transaction boundaries. +- [ ] Every recovery-record kind has doctor diagnosis and repair. +- [ ] No mutating service path bypasses the `Mutation` handle. + +## Phase 6: Validation and Documentation + +- [ ] **Task 14:** Add race and transactional e2e gates. *(Dependencies: Tasks 1–13)* +- [ ] **Task 15:** Align lifecycle documentation and release notes. *(Dependencies: Tasks 1–14)* + +### Checkpoint: Complete + +- [ ] Named fault-injection tests cover every mutation phase. +- [ ] `just check` passes. +- [ ] `go test -race ./...` passes. +- [ ] `just build` passes. +- [ ] Full deterministic e2e passes with a race binary. +- [ ] Ordinary full `just e2e` passes. +- [ ] OpenWiki and release notes match runtime behavior. +- [ ] Human reviews and approves the completed implementation.