plan 9: an interrupted or failed undo can be finished

4 files changed, 153 insertions, 2 deletions

diff --git a/internal/engine/apply.go b/internal/engine/apply.go
index 94d65e4..e109aaf 100644
--- a/internal/engine/apply.go
+++ b/internal/engine/apply.go
@@ -343,6 +343,14 @@ func (e *Engine) PlanUndo(runID string) (*UndoPlan, error) {
 		return nil, fmt.Errorf("engine: plan undo: run %s is itself an undo and cannot be undone", runID)
 	}
 
+	// Reversals an earlier undo of this same run already completed are not
+	// offered again (review M10): an undo that stopped part way can be
+	// finished by undoing the run once more.
+	reversed, err := journal.ReversedSteps(e.Config.LogFile(), runID)
+	if err != nil {
+		return nil, fmt.Errorf("engine: plan undo: %w", err)
+	}
+
 	// Entries are grouped by directory and file together (review M7): one run
 	// spans every directory, and two directories can each hold a file of the
 	// same name.
@@ -362,7 +370,7 @@ func (e *Engine) PlanUndo(runID string) (*UndoPlan, error) {
 
 	up := &UndoPlan{Run: runID}
 	for _, k := range order {
-		uf := planUndoFile(k.dir, k.file, byFile[k])
+		uf := planUndoFile(k.dir, k.file, byFile[k], reversed)
 		// Critical finding, Task 8's review: a file every one of whose
 		// entries has Status != "ok" (declined by the ORIGINAL run's own
 		// review, or skipped, or failed before anything happened) yields
@@ -464,7 +472,7 @@ func isFileAffecting(action string) bool {
 // two steps of one file that touch the same path, and every un-contended
 // check keeps behaving exactly as before, since the projection only ever
 // overrides a real occupant that this same chain is itself about to clear.
-func planUndoFile(dir, file string, ents []journal.Entry) UndoFile {
+func planUndoFile(dir, file string, ents []journal.Entry, reversed map[journal.ReversedKey]int) UndoFile {
 	uf := UndoFile{File: file, Dir: dir}
 	proj := newUndoProjection()
 	for i := len(ents) - 1; i >= 0; i-- {
@@ -483,6 +491,13 @@ func planUndoFile(dir, file string, ents []journal.Entry) UndoFile {
 			break
 		}
 		step := reverseStep(en)
+		if k := (journal.ReversedKey{Dir: dir, File: file, Action: step.Action, Src: step.Src}); reversed[k] > 0 {
+			// An earlier undo of this run already reversed this step: the
+			// disk already shows it, and it is not offered again.
+			reversed[k]--
+			proj.record(step)
+			continue
+		}
 		if (en.Action == "move" || en.Action == "rename") && proj.occupied[en.Dst] {
 			// A reversal already queued for this same file puts it back at
 			// en.Dst before this one runs, and that reversal was checked
diff --git a/internal/engine/undo_identity_test.go b/internal/engine/undo_identity_test.go
index 9c93b3b..d4169be 100644
--- a/internal/engine/undo_identity_test.go
+++ b/internal/engine/undo_identity_test.go
@@ -223,3 +223,54 @@ func TestUndoLeavesNoDirectoriesBehind(t *testing.T) {
 		t.Errorf("dl/Out is left behind: %v", err)
 	}
 }
+
+// TestUndoCanBeFinishedAfterAFailure: an undo whose last reversal failed
+// (something took the original name) leaves the file part way back; once
+// the obstacle is gone, undoing the same run again offers only the step
+// that is left, and finishes it (review M10).
+func TestUndoCanBeFinishedAfterAFailure(t *testing.T) {
+	e, run, h, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one"}},
+		map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (rename \"r-{name}\") (move \"Out\"))\n"})
+	up, err := e.PlanUndo(run)
+	if err != nil {
+		t.Fatal(err)
+	}
+	blocker := filepath.Join(h, "dl", "a.pdf")
+	if err := os.WriteFile(blocker, []byte("in the way"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	undo := func(up *UndoPlan) *ApplyResult {
+		t.Helper()
+		j, err := journal.Open(logPath)
+		if err != nil {
+			t.Fatal(err)
+		}
+		defer j.Close()
+		res, err := e.ApplyUndo(context.Background(), up, j, journal.NewRunID(time.Now()))
+		if err != nil {
+			t.Fatal(err)
+		}
+		return res
+	}
+	if res := undo(up); res.Failed != 1 {
+		t.Fatalf("first undo: Failed = %d, want 1 (the rename back is blocked)", res.Failed)
+	}
+	if err := os.Remove(blocker); err != nil {
+		t.Fatal(err)
+	}
+	time.Sleep(10 * time.Millisecond) // a new run id
+	again, err := e.PlanUndo(run)
+	if err != nil {
+		t.Fatal(err)
+	}
+	f := undoFileNamed(t, again, "dl", "a.pdf")
+	if f.Refused != "" || len(f.Steps) != 1 || f.Steps[0].Action != "undo-rename" {
+		t.Fatalf("second plan: %+v; want one undo-rename step", f)
+	}
+	if res := undo(again); res.Failed != 0 || res.Applied != 1 {
+		t.Fatalf("second undo: %+v", res)
+	}
+	if b, err := os.ReadFile(filepath.Join(h, "dl", "a.pdf")); err != nil || string(b) != "one" {
+		t.Errorf("dl/a.pdf = %q, %v; want the original back", b, err)
+	}
+}
diff --git a/internal/journal/read.go b/internal/journal/read.go
index 3de15d2..de06422 100644
--- a/internal/journal/read.go
+++ b/internal/journal/read.go
@@ -43,6 +43,42 @@ type Run struct {
 	Dirs   []string
 	Counts map[string]int // action -> count of status "ok"
 	Undone bool           // a later run reversed this one
+	UndoOf string         // for an undo run, the run it reverses; "" otherwise
+}
+
+// ReversedKey identifies one reversal an undo run carried out: the file's
+// directory and name, the undo action and the path it started from - enough
+// to tell which step of the original run it reversed.
+type ReversedKey struct {
+	Dir, File, Action, Src string
+}
+
+// ReversedSteps counts, for runID, every reversal that earlier undo runs of
+// it completed ("ok" undo- entries of runs whose run-start says they undo
+// runID), so a later undo of the same run can offer only what is left
+// (review M10). An undo run's own unparsable lines are skipped; a missing
+// reversal is then offered again, where its own checks refuse it if it had
+// in fact happened.
+func ReversedSteps(path, runID string) (map[ReversedKey]int, error) {
+	lines, err := readLines(path)
+	if err != nil {
+		return nil, err
+	}
+	undoRuns := map[string]bool{}
+	for _, line := range lines {
+		if e, ok := parseLine(line); ok && e.Action == "run-start" && e.Detail == UndoOf(runID) {
+			undoRuns[e.Run] = true
+		}
+	}
+	out := map[ReversedKey]int{}
+	for _, line := range lines {
+		e, ok := parseLine(line)
+		if !ok || !undoRuns[e.Run] || e.Status != "ok" || !strings.HasPrefix(e.Action, "undo-") {
+			continue
+		}
+		out[ReversedKey{Dir: e.Dir, File: e.File, Action: e.Action, Src: e.Src}]++
+	}
+	return out, nil
 }
 
 // Entries returns every entry belonging to runID, in file order. A line
@@ -198,6 +234,7 @@ func Runs(path string, n int) ([]Run, error) {
 	sort.SliceStable(runs, func(i, j int) bool { return runs[i].Start.After(runs[j].Start) })
 	for i := range runs {
 		runs[i].Undone = undoes[runs[i].ID]
+		runs[i].UndoOf = pendingUndo[runs[i].ID]
 	}
 
 	if n > 0 && n < len(runs) {
diff --git a/internal/journal/read_test.go b/internal/journal/read_test.go
index 2f0f40f..2b46bfb 100644
--- a/internal/journal/read_test.go
+++ b/internal/journal/read_test.go
@@ -625,3 +625,51 @@ func TestEntriesFailsClosedOnMissingRunStart(t *testing.T) {
 		t.Errorf("entries = %+v", got)
 	}
 }
+
+// TestReversedStepsCountsEveryUndoOfARun: the steps earlier undo runs of a
+// run already reversed - only "ok" undo entries of runs that undo it - so
+// a later undo of the same run can offer just what is left (review M10).
+// Runs also names the run an undo run reversed.
+func TestReversedStepsCountsEveryUndoOfARun(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "krino.log")
+	w, err := Open(path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	at := time.Date(2026, 9, 11, 9, 0, 0, 0, time.UTC)
+	for _, e := range []Entry{
+		{Time: at, Run: "R", Action: "run-start", Status: "ok"},
+		{Time: at, Run: "R", Dir: "dl", File: "a.pdf", Step: 1, Action: "move", Status: "ok", Src: "/dl/a.pdf", Dst: "/w/a.pdf"},
+		{Time: at, Run: "R", Action: "run-end", Status: "ok"},
+		{Time: at.Add(time.Minute), Run: "U1", Action: "run-start", Status: "ok", Detail: UndoOf("R")},
+		{Time: at.Add(time.Minute), Run: "U1", Dir: "dl", File: "a.pdf", Step: 1, Action: "undo-move", Status: "ok", Src: "/w/a.pdf", Dst: "/dl/a.pdf"},
+		{Time: at.Add(time.Minute), Run: "U1", Dir: "dl", File: "b.pdf", Step: 1, Action: "undo-rename", Status: "failed", Src: "/dl/r-b.pdf", Dst: "/dl/b.pdf"},
+		{Time: at.Add(time.Minute), Run: "U1", Action: "run-end", Status: "ok"},
+		{Time: at.Add(2 * time.Minute), Run: "U2", Action: "run-start", Status: "ok", Detail: UndoOf("OTHER")},
+		{Time: at.Add(2 * time.Minute), Run: "U2", Dir: "dl", File: "a.pdf", Step: 1, Action: "undo-move", Status: "ok", Src: "/w/a.pdf", Dst: "/dl/a.pdf"},
+		{Time: at.Add(2 * time.Minute), Run: "U2", Action: "run-end", Status: "ok"},
+	} {
+		if err := w.Append(e); err != nil {
+			t.Fatal(err)
+		}
+	}
+	w.Close()
+	got, err := ReversedSteps(path, "R")
+	if err != nil {
+		t.Fatal(err)
+	}
+	want := map[ReversedKey]int{{Dir: "dl", File: "a.pdf", Action: "undo-move", Src: "/w/a.pdf"}: 1}
+	if len(got) != len(want) || got[ReversedKey{Dir: "dl", File: "a.pdf", Action: "undo-move", Src: "/w/a.pdf"}] != 1 {
+		t.Errorf("ReversedSteps = %v, want %v", got, want)
+	}
+	runs, err := Runs(path, 0)
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, r := range runs {
+		want := map[string]string{"R": "", "U1": "R", "U2": "OTHER"}[r.ID]
+		if r.UndoOf != want {
+			t.Errorf("run %s: UndoOf = %q, want %q", r.ID, r.UndoOf, want)
+		}
+	}
+}