From 21339a73f5cd592ebd92937a1440aa9f13e75bbb Mon Sep 17 00:00:00 2001 From: Lukasz Kasprzak Date: Mon, 14 Sep 2026 22:26:23 +0200 Subject: plan 10: a damaged log line refuses only its file, not the run --- docs/design.md | 4 +++ internal/engine/apply.go | 8 +++++ internal/engine/undo_identity_test.go | 42 +++++++++++++++++++++++++ internal/journal/read.go | 39 ++++++++++++++++++----- internal/journal/read_test.go | 59 ++++++++++++++++++++++++++--------- 5 files changed, 130 insertions(+), 22 deletions(-) diff --git a/docs/design.md b/docs/design.md index a232baf..99e5b55 100644 --- a/docs/design.md +++ b/docs/design.md @@ -642,6 +642,10 @@ not logged; declined files are. way (a refused or failed step, an interrupt) is finished by undoing the run once more; when the most recent run is itself an undo, plain `krino undo` does exactly that for the run it undid. +- A damaged log line (a crash or a full disk cutting it) refuses only the + file it names, when that file can still be read from the line; the rest + of the run is reversed. A line of another run is ignored. Only a line + whose file cannot be read refuses the whole run. - A file is identified by its directory and its path within it, so two directories' files of the same name are reversed apart. A file that one directory's rules move into another included directory, which sorts it diff --git a/internal/engine/apply.go b/internal/engine/apply.go index cc87f30..ed2fbe5 100644 --- a/internal/engine/apply.go +++ b/internal/engine/apply.go @@ -496,6 +496,14 @@ func isFileAffecting(action string) bool { // overrides a real occupant that this same chain is itself about to clear. func planUndoFile(dir, file string, ents []journal.Entry, reversed map[journal.ReversedKey]int) UndoFile { uf := UndoFile{File: file, Dir: dir} + for _, en := range ents { + if en.Action == "damaged" { + // A log line of this file is cut or damaged (journal.Entries): a + // step may be missing from its chain, so none of it is reversed. + uf.Refused = fmt.Sprintf("its log is damaged (%s); a step may not be recorded", en.Detail) + return uf + } + } proj := newUndoProjection() for i := len(ents) - 1; i >= 0; i-- { en := ents[i] diff --git a/internal/engine/undo_identity_test.go b/internal/engine/undo_identity_test.go index 23e0bf9..5040ece 100644 --- a/internal/engine/undo_identity_test.go +++ b/internal/engine/undo_identity_test.go @@ -317,3 +317,45 @@ func TestApplyReportsAStepThatCouldNotBeLogged(t *testing.T) { } } } + +// TestUndoRefusesOnlyTheFileWithADamagedLine: a crash that cuts one file's +// log line refuses that file; the other files of the run are still undone +// (re-review N1). +func TestUndoRefusesOnlyTheFileWithADamagedLine(t *testing.T) { + e, run, h, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one", "b.pdf": "two"}}, + map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (move \"Out\"))\n"}) + raw, err := os.ReadFile(logPath) + if err != nil { + t.Fatal(err) + } + lines := strings.Split(strings.TrimRight(string(raw), "\n"), "\n") + for i, l := range lines { + if strings.Contains(l, "\tb.pdf\t") && strings.Contains(l, "\tmove\t") { + lines[i] = l[:len(l)/2] // cut mid-write + } + } + if err := os.WriteFile(logPath, []byte(strings.Join(lines, "\n")+"\n"), 0o644); err != nil { + t.Fatal(err) + } + up, err := e.PlanUndo(run) + if err != nil { + t.Fatalf("PlanUndo refused the whole run: %v", err) + } + if f := undoFileNamed(t, up, "dl", "b.pdf"); !strings.Contains(f.Refused, "damaged") { + t.Errorf("b.pdf: Refused %q; want its damaged log named", f.Refused) + } + if f := undoFileNamed(t, up, "dl", "a.pdf"); f.Refused != "" { + t.Errorf("a.pdf refused: %q", f.Refused) + } + j, err := journal.Open(logPath) + if err != nil { + t.Fatal(err) + } + defer j.Close() + if _, err := e.ApplyUndo(context.Background(), up, j, journal.NewRunID(time.Now())); err != nil { + t.Fatal(err) + } + if b, err := os.ReadFile(filepath.Join(h, "dl", "a.pdf")); err != nil || string(b) != "one" { + t.Errorf("a.pdf not restored: %q %v", b, err) + } +} diff --git a/internal/journal/read.go b/internal/journal/read.go index de06422..48cd6b0 100644 --- a/internal/journal/read.go +++ b/internal/journal/read.go @@ -81,9 +81,12 @@ func ReversedSteps(path, runID string) (map[ReversedKey]int, error) { return out, nil } -// Entries returns every entry belonging to runID, in file order. A line -// that fails to parse is skipped, but Entries fails closed within the run's -// own window - from its run-start line to its run-end line, or to end of +// Entries returns every entry belonging to runID, in file order. Since plan +// 10 (re-review N1), an unparsable line whose run column names another run +// is ignored, and one of this run whose directory and file columns are still +// readable is returned as a "damaged" entry for that file, so undo refuses +// that file alone. Otherwise a line that fails to parse is skipped, but +// Entries fails closed within the run's own window - from its run-start line to its run-end line, or to end of // file when there is no run-end (a crashed run, which is precisely when // corruption is likely): any unparsable line found inside that window sets // the returned error, whether or not the line's own Run column can still be @@ -136,14 +139,25 @@ func Entries(path, runID string) ([]Entry, error) { } continue } - if badLine != 0 { + run, runFound := runFieldOf(line) + if runFound && run != runID { + // Another run's damaged line: runs of different directories can + // interleave, and it says nothing about this one (re-review N1). continue } - if inWindow { - badLine = i + 1 + ours := inWindow || (runFound && run == runID) + if !ours { + continue + } + if dir, file, ok := fileFieldsOf(line); ok { + // A line of this run cut or damaged where its file is still + // readable: that file's chain may be missing a step, so it is + // returned as damaged and PlanUndo refuses just that file; the + // rest of the run stays undoable (re-review N1). + out = append(out, Entry{Run: runID, Dir: dir, File: file, Action: "damaged", Status: "damaged", Detail: fmt.Sprintf("line %d", i+1)}) continue } - if run, found := runFieldOf(line); found && run == runID { + if badLine == 0 { badLine = i + 1 } } @@ -167,6 +181,17 @@ func runFieldOf(line string) (string, bool) { return unescape(f[1]), true } +// fileFieldsOf best-effort extracts a line's directory and file columns when +// the line otherwise fails to parse; ok is false when the line is cut before +// them or names no file (a run-start or run-end line). +func fileFieldsOf(line string) (dir, file string, ok bool) { + f := strings.SplitN(line, "\t", 5) + if len(f) < 5 || f[3] == "" { + return "", "", false + } + return unescape(f[2]), unescape(f[3]), true +} + // Runs summarises every run found in the log, newest first. n <= 0 means // all. As with Entries, an unparsable line is skipped rather than failing // the read - here silently and always, even when it belonged to the run diff --git a/internal/journal/read_test.go b/internal/journal/read_test.go index 2b46bfb..fb6f77f 100644 --- a/internal/journal/read_test.go +++ b/internal/journal/read_test.go @@ -303,15 +303,18 @@ func TestEntriesBothFailureModesReportsBadLineFirst(t *testing.T) { t.Fatal(err) } + // Since plan 10 a damaged line whose file is readable refuses only that + // file (a "damaged" entry); the run as a whole still fails closed here, + // on its missing run-start. got, err := Entries(path, "A") if err == nil { - t.Fatal("Entries returned no error with both a bad line and a missing run-start present") + t.Fatal("Entries returned no error with a missing run-start") } - if !strings.Contains(err.Error(), "unparsable line 2") { - t.Errorf("error = %q, want it to report the unparsable line (line 2), not the missing run-start", err) + if !strings.Contains(err.Error(), "no readable run-start") { + t.Errorf("error = %q, want it to name the missing run-start", err) } - if len(got) != 1 || got[0].Action != "run-end" { - t.Errorf("entries = %+v, want just the surviving run-end", got) + if len(got) != 2 || got[0].Action != "damaged" || got[0].File != "x.pdf" || got[1].Action != "run-end" { + t.Errorf("entries = %+v, want x.pdf damaged, then run-end", got) } } @@ -412,8 +415,10 @@ func TestEntriesAdjacentRunStartsOneCorrupted(t *testing.T) { // TestEntriesReportsAMangledLine: a corrupt line that is not the log's // final line must not be silently dropped by Entries the way Runs drops it -// - PlanUndo needs to know a step went missing so it can refuse the whole -// run rather than half-undo a file (spec ยง10). +// - PlanUndo needs to know a step went missing. Since plan 10 (re-review +// N1) a line whose directory and file columns are readable is returned as a +// "damaged" entry for that file, so only that file is refused and the rest +// of the run can still be undone. func TestEntriesReportsAMangledLine(t *testing.T) { path := filepath.Join(t.TempDir(), "krino.log") w, _ := Open(path) @@ -451,17 +456,41 @@ func TestEntriesReportsAMangledLine(t *testing.T) { } got, err := Entries(path, "A") - if err == nil { - t.Fatal("Entries did not report the mangled line") + if err != nil { + t.Fatalf("Entries = %v; a damaged line with a readable file must not refuse the run", err) } - if !strings.Contains(err.Error(), "line 2") { - t.Errorf("error %q does not name line 2", err) + if len(got) != 3 || got[1].Action != "damaged" || got[1].Dir != "dl" || got[1].File != "x.pdf" || !strings.Contains(got[1].Detail, "line 2") { + t.Fatalf("entries = %+v; want run-start, x.pdf damaged (line 2), run-end", got) } - if len(got) != 2 { - t.Fatalf("got %d entries, want the 2 surviving (run-start, run-end): %+v", len(got), got) +} + +// TestEntriesIgnoresAnotherRunsDamagedLine: a damaged line whose run column +// names another run - two directories' runs can interleave in one log - does +// not refuse this run, even inside its window (re-review N1). +func TestEntriesIgnoresAnotherRunsDamagedLine(t *testing.T) { + path := filepath.Join(t.TempDir(), "krino.log") + w, _ := Open(path) + at := time.Date(2026, 9, 11, 10, 2, 3, 0, time.UTC) + for _, e := range []Entry{ + {Time: at, Run: "A", Action: "run-start", Status: "ok"}, + {Time: at, Run: "A", Dir: "dl", File: "x.pdf", Step: 1, Action: "move", Status: "ok", Src: "/a/x.pdf", Dst: "/b/x.pdf"}, + {Time: at, Run: "A", Action: "run-end", Status: "ok"}, + } { + if err := w.Append(e); err != nil { + t.Fatal(err) + } } - if got[0].Action != "run-start" || got[1].Action != "run-end" { - t.Errorf("entries = %+v", got) + w.Close() + raw, _ := os.ReadFile(path) + lines := strings.Split(strings.TrimRight(string(raw), "\n"), "\n") + cut := "2026-09-11T10:02:04Z\tB\tscans\ty.pdf\t1\tmo" + lines = append(lines[:2], append([]string{cut}, lines[2:]...)...) + if err := os.WriteFile(path, []byte(strings.Join(lines, "\n")+"\n"), 0o644); err != nil { + t.Fatal(err) + } + got, err := Entries(path, "A") + if err != nil || len(got) != 3 { + t.Errorf("Entries(A) = %+v, %v; want A's three entries and no error", got, err) } } -- cgit v1.3