diff options
| author | Lukasz Kasprzak <lukas@labunix.xyz> | 2026-09-13 02:31:32 +0200 |
|---|---|---|
| committer | Lukasz Kasprzak <lukas@labunix.xyz> | 2026-09-13 02:31:32 +0200 |
| commit | 26c94eb3db62ec6eebbf8d22c11afe691d9520c4 (patch) | |
| tree | 165e5bf69234b4f96c9b74deb4898d7143ddf120 /internal/engine | |
| parent | a6e442a645902011b2081c216daaec052cdc6ce6 (diff) | |
| download | krino-26c94eb3db62ec6eebbf8d22c11afe691d9520c4.tar.gz krino-26c94eb3db62ec6eebbf8d22c11afe691d9520c4.zip | |
krino: release 0.0.1 — man pages, install, examples, cross and release, README, changelogv0.0.1
Also: undo removes the directories its run created; a hardlink is never a
duplicate of its own other name; a flag written before "undo" is honoured;
--version prints no leading v. Duplicate conditions with different scopes
not sharing an original is documented as a known limitation.
Diffstat (limited to 'internal/engine')
| -rw-r--r-- | internal/engine/apply.go | 118 | ||||
| -rw-r--r-- | internal/engine/apply_test.go | 243 | ||||
| -rw-r--r-- | internal/engine/plan_bench_test.go | 108 |
3 files changed, 469 insertions, 0 deletions
diff --git a/internal/engine/apply.go b/internal/engine/apply.go index c74414d..878cb18 100644 --- a/internal/engine/apply.go +++ b/internal/engine/apply.go @@ -9,6 +9,7 @@ import ( "io" "os" "path/filepath" + "sort" "strings" "syscall" "time" @@ -660,6 +661,50 @@ func refuseIfSrcExists(us UndoStep, proj *undoProjection) string { // declined - a single pass, not two, so the two kinds of file interleave in // the log exactly as the run touched them, the same as Apply's own // approved-and-declined chains do. +// +// Task 1 (plan 5): after every file's reversal has been attempted, a second, +// run-wide pass retries the directory removals that were refused as +// non-empty. planUndoFile puts the undo-mkdir step for a shared destination +// on whichever file's chain first created it (spec §9: only the step that +// actually created a directory logs a "mkdir" entry, so only that file's +// reversal carries the matching undo-mkdir); when that file reverses first, +// its siblings are usually still inside, the removal is correctly refused as +// non-empty (spec §10), and - without this pass - nothing ever retries it, +// leaving empty directories behind even though every file came back. This +// mirrors planUndoFile's own undoProjection insight (see its comment) one +// level up: a removal judged too early is judging the wrong world, whether +// that "too early" is mid-file (what the projection fixes) or mid-run (what +// this retry fixes). +// +// The retry is a run-level tidy-up, never a re-run of a step: it does not +// touch what the first undo-mkdir attempt already logged (that entry, ok or +// failed, stands exactly as it was written), and a directory the retry does +// manage to remove gets an ADDITIONAL journal entry - never a rewrite - so +// the log never disagrees with reality (my ruling on the point the brief +// left open: spec §9 logs every step, and a directory removed while the log +// still says its removal was refused would be a false record). Because +// journal.ranAnyUndoStep already excludes "undo-mkdir" from what marks a run +// "(undone)", this extra "ok" entry cannot change that marking either - +// TestApplyUndoRetryLogsBothMkdirEntriesAndStillMarksOriginalRunUndone pins +// it rather than assuming it. A retried removal is likewise never folded +// into ApplyResult: it is +// collected from candidates whose first attempt already went through +// tallyFile once (via isFileAffecting's exemption), and counting it again +// here would double-count a directory that failed once and then quietly +// tidied itself away. +// +// Candidates are collected only from directories this run's own reversal +// created - by construction, since every candidate comes from an undo-mkdir +// step, and an undo-mkdir step exists only for a directory the forward run's +// Made recorded - never a directory the retry merely happens to find empty. +// They are retried deepest path first (retryDirRemovals), so a nested +// directory - e.g. Work/Sub under Work - is removed before its +// now-possibly-empty parent, the same outermost-created/innermost-removed +// discipline logStep and undoFile already keep within one file's own chain, +// applied here across files. A directory still non-empty at retry time +// genuinely holds something else (or the retry runs before every sibling +// happens to have reversed, on a later undo of a different run) and simply +// stays, with its original refusal the only record of it. func (e *Engine) ApplyUndo(ctx context.Context, up *UndoPlan, j *journal.Writer, run string) (*ApplyResult, error) { result := &ApplyResult{} @@ -695,6 +740,7 @@ func (e *Engine) ApplyUndo(ctx context.Context, up *UndoPlan, j *journal.Writer, return result, fmt.Errorf("engine: apply undo: %w", err) } + var retries []dirRetry for _, f := range actionable { if err := ctx.Err(); err != nil { return result, err @@ -714,6 +760,15 @@ func (e *Engine) ApplyUndo(ctx context.Context, up *UndoPlan, j *journal.Writer, } result.Files = append(result.Files, fr) tallyFile(result, fr.Steps, func(i int) bool { return isFileAffecting(f.Steps[i].Action) }) + for i, us := range f.Steps { + if us.Action == "undo-mkdir" && fr.Steps[i].Status == "failed" { + retries = append(retries, dirRetry{dir: us.Src, dirName: f.Dir, file: f.File, step: i + 1}) + } + } + } + + if err := e.retryDirRemovals(j, run, retries); err != nil { + return result, fmt.Errorf("engine: apply undo: %w", err) } if err := j.Append(journal.Entry{Time: e.Now(), Run: run, Action: "run-end", Status: "ok"}); err != nil { @@ -722,6 +777,69 @@ func (e *Engine) ApplyUndo(ctx context.Context, up *UndoPlan, j *journal.Writer, return result, nil } +// dirRetry names one directory whose undo-mkdir was refused (as non-empty) +// during ApplyUndo's main pass, kept for the run-wide retry once every +// file's reversal has been attempted. file and dirName are the file and +// config directory name that owned the original undo-mkdir step, carried +// forward so retryDirRemovals's journal entry - if the retry succeeds - +// names the same file and directory the original refusal did, not an +// arbitrary one; step is that same step's 1-based index, so the two entries +// (the original "failed" and, if the retry succeeds, this "ok") read +// together under the same File/Step in the log. +type dirRetry struct { + dir string + dirName string + file string + step int +} + +// retryDirRemovals is ApplyUndo's run-wide second pass (Task 1, plan 5): once +// every file's reversal has run, some directories an undo-mkdir step could +// not remove earlier may now be empty, because a sibling file that shared +// the directory has since reversed too. candidates is sorted deepest path +// first (by descending path-segment count) so a nested directory is removed +// before its parent, exactly the order a real cleanup needs; a directory +// still non-empty at its turn genuinely holds something else and is left +// exactly as its first attempt recorded it - no second entry, no error. +// +// This never rewrites or removes the original undo-mkdir entry (ok or +// failed, whichever the first attempt logged): a directory the retry does +// manage to remove gets one ADDITIONAL entry instead (my ruling on the point +// the brief left open - see ApplyUndo's comment), so the log always agrees +// with what is actually on disk. The new entry's own Action is still +// "undo-mkdir", so journal.ranAnyUndoStep - which excludes that action on +// principle, not by accident (see its own comment) - continues to treat this +// exactly like any other undo-mkdir for the purpose of marking a run +// "(undone)": tidying up an empty directory, on the first attempt or the +// retry, is still not a restoration. +func (e *Engine) retryDirRemovals(j *journal.Writer, run string, candidates []dirRetry) error { + sort.SliceStable(candidates, func(i, j int) bool { + return pathDepth(candidates[i].dir) > pathDepth(candidates[j].dir) + }) + for _, c := range candidates { + if err := os.Remove(c.dir); err != nil { + // Still not empty (or gone, or otherwise unremovable): the + // original refusal already recorded this, and it stands. + continue + } + if err := j.Append(journal.Entry{ + Time: e.Now(), Run: run, Dir: c.dirName, File: c.file, Step: c.step, + Action: "undo-mkdir", Status: "ok", Src: c.dir, + }); err != nil { + return err + } + } + return nil +} + +// pathDepth counts path's separators after cleaning it, so retryDirRemovals +// can sort deepest first: a nested directory (more separators) is always +// removed before the parent it sits under, whatever the two paths' common +// root. +func pathDepth(path string) int { + return strings.Count(filepath.Clean(path), string(filepath.Separator)) +} + // declineUndoFile logs f's reversal as declined without carrying out any of // it - spec §9's "declined files are logged even though nothing happens to // them", extended to undo (fix round 2026-09-12, item 2 of Task 8's review): diff --git a/internal/engine/apply_test.go b/internal/engine/apply_test.go index 42724a0..a54bffc 100644 --- a/internal/engine/apply_test.go +++ b/internal/engine/apply_test.go @@ -1180,3 +1180,246 @@ func TestTallyFileCountsMixedOutcomesOnceEach(t *testing.T) { t.Errorf("result = %+v, want one of each", result) } } + +// --- Plan 5, Task 1 --- + +// sharedDestUndoFixture builds a downloads directory with three pdf files +// (a.pdf, b.pdf, c.pdf) and a single rule moving all of them into dest, +// applies the move, and returns the sandbox home, the loaded engine, the +// journal's path and the forward run's ID. +// +// All three files landing on one destination that this one run creates is +// the shape that exercises the run-wide directory retry (Task 1, plan 5): +// whichever file's chain first creates dest carries its undo-mkdir step(s), +// and that file's own reversal typically runs while its siblings still +// occupy dest - refusing the removal correctly, at first. dest may name a +// nested path ("Work/Sub"): apply.mkdirAllTracked then records every +// directory the move had to create, outermost first, and every one of them +// still lands on that same first file's chain. +// +// Extracted per fix round 1 (Important 2): TestApplyUndoRemovesSharedDirectoryAfterEveryFileReverses +// and TestApplyUndoRetryLogsBothMkdirEntriesAndStillMarksOriginalRunUndone +// used to duplicate this setup verbatim; TestApplyUndoRetryRemovesNestedDirectoriesDeepestFirst +// needed the identical shape with only dest varying, which is what named the +// parameter rather than hard-coding "Filed" here. +func sharedDestUndoFixture(t *testing.T, dest string) (h string, e *Engine, logPath string, run string) { + t.Helper() + h = sandbox(t) + dl := filepath.Join(h, "dl") + if err := os.MkdirAll(dl, 0o755); err != nil { + t.Fatal(err) + } + // Three files that all move into ONE created destination. The bug this + // task fixes is that only the file whose chain first creates it ever + // carries the undo-mkdir step, and that step is attempted while its + // siblings are still inside. + for _, n := range []string{"a.pdf", "b.pdf", "c.pdf"} { + if err := os.WriteFile(filepath.Join(dl, n), []byte(n), 0o640); err != nil { + t.Fatal(err) + } + } + main := writeConfig(t, h, `(include "dl")`, map[string]string{"dl": ` +(path "~/dl") +(min-age 0s) +(rule "pdfs" (when (type pdf)) (move "` + dest + `")) +`}) + loaded, errs := Load(main) + if len(errs) > 0 { + t.Fatal(errs) + } + e = loaded + dp, err := e.Plan(context.Background(), e.Dirs[0], plan.NewClaims()) + if err != nil { + t.Fatal(err) + } + approved := map[string]bool{} + for _, c := range dp.Chains { + approved[c.File.Rel] = true + } + logPath = filepath.Join(h, ".local", "state", "krino", "krino.log") + j, err := journal.Open(logPath) + if err != nil { + t.Fatal(err) + } + run = journal.NewRunID(time.Now()) + if _, err := e.Apply(context.Background(), dp, approved, j, run); err != nil { + t.Fatal(err) + } + j.Close() + return h, e, logPath, run +} + +func TestApplyUndoRemovesSharedDirectoryAfterEveryFileReverses(t *testing.T) { + h, e, logPath, run := sharedDestUndoFixture(t, "Filed") + dl := filepath.Join(h, "dl") + filed := filepath.Join(dl, "Filed") + if _, err := os.Stat(filed); err != nil { + t.Fatalf("apply did not create the directory: %v", err) + } + + up, err := e.PlanUndo(run) + if err != nil { + t.Fatal(err) + } + j2, err := journal.Open(logPath) + if err != nil { + t.Fatal(err) + } + res, err := e.ApplyUndo(context.Background(), up, j2, journal.NewRunID(time.Now())) + if err != nil { + t.Fatal(err) + } + j2.Close() + if res.Failed != 0 { + t.Fatalf("undo reported %d failures: %+v", res.Failed, res) + } + if _, err := os.Stat(filed); !os.IsNotExist(err) { + t.Errorf("undo left the created directory behind: %v", err) + } + for _, n := range []string{"a.pdf", "b.pdf", "c.pdf"} { + if _, err := os.Stat(filepath.Join(dl, n)); err != nil { + t.Errorf("%s did not come back: %v", n, err) + } + } +} + +// TestApplyUndoRetryRemovesNestedDirectoriesDeepestFirst pins fix round 1's +// Important 1: retryDirRemovals must retry deepest path first. All three +// files move into Work/Sub, so Apply's single mkdirAllTracked call creates +// both Work and Work/Sub on the FIRST file's own chain (outermost first), +// which means that one file's reversal carries two undo-mkdir steps, one for +// each directory - and both are refused on that file's own turn, since the +// other two files still sit in Work/Sub at that point. +// +// Retried deepest first, Work/Sub empties out and is removed, and Work - +// now itself empty - is removed right after. Retried shallowest first +// instead, Work is tried while Work/Sub (now empty, but not yet removed) +// still sits inside it, so Work is refused as non-empty and never retried +// again in this run; Work/Sub is then removed, leaving the outer Work +// directory behind. So end state alone - no directory left over - already +// distinguishes correct (deepest-first) ordering from inverted or dropped +// ordering; unlike the flat-destination tests above, where only one +// directory ever entered `retries`, this is the case built to tell the two +// apart. +func TestApplyUndoRetryRemovesNestedDirectoriesDeepestFirst(t *testing.T) { + h, e, logPath, run := sharedDestUndoFixture(t, "Work/Sub") + dl := filepath.Join(h, "dl") + work := filepath.Join(dl, "Work") + sub := filepath.Join(work, "Sub") + if _, err := os.Stat(sub); err != nil { + t.Fatalf("apply did not create the nested directory: %v", err) + } + + up, err := e.PlanUndo(run) + if err != nil { + t.Fatal(err) + } + j2, err := journal.Open(logPath) + if err != nil { + t.Fatal(err) + } + res, err := e.ApplyUndo(context.Background(), up, j2, journal.NewRunID(time.Now())) + if err != nil { + t.Fatal(err) + } + j2.Close() + if res.Failed != 0 { + t.Fatalf("undo reported %d failures: %+v", res.Failed, res) + } + if _, err := os.Stat(sub); !os.IsNotExist(err) { + t.Errorf("undo left the nested directory behind: %v", err) + } + if _, err := os.Stat(work); !os.IsNotExist(err) { + t.Errorf("undo left the outer directory behind - retryDirRemovals is not retrying deepest path first: %v", err) + } + for _, n := range []string{"a.pdf", "b.pdf", "c.pdf"} { + if _, err := os.Stat(filepath.Join(dl, n)); err != nil { + t.Errorf("%s did not come back: %v", n, err) + } + } +} + +// TestApplyUndoRetryLogsBothMkdirEntriesAndStillMarksOriginalRunUndone pins +// the journal half of the ruling on the point Task 1's brief left open: when +// retryDirRemovals succeeds in removing a directory, it appends an +// ADDITIONAL journal entry for it - the original "failed" undo-mkdir entry, +// recorded on whichever file's chain first created the directory, is never +// rewritten or removed - and, because that entry's Action is still +// "undo-mkdir" like the first, journal.ranAnyUndoStep continues to exclude +// it from what marks a run "(undone)" (the brief's constraint: it "cannot +// change whether a run shows as (undone); confirm that rather than assume +// it"). +func TestApplyUndoRetryLogsBothMkdirEntriesAndStillMarksOriginalRunUndone(t *testing.T) { + _, e, logPath, run := sharedDestUndoFixture(t, "Filed") + + up, err := e.PlanUndo(run) + if err != nil { + t.Fatal(err) + } + j2, err := journal.Open(logPath) + if err != nil { + t.Fatal(err) + } + undoRun := journal.NewRunID(time.Now()) + res, err := e.ApplyUndo(context.Background(), up, j2, undoRun) + if err != nil { + t.Fatal(err) + } + j2.Close() + if res.Failed != 0 { + t.Fatalf("undo reported %d failures: %+v", res.Failed, res) + } + + entries, err := journal.Entries(logPath, undoRun) + if err != nil { + t.Fatal(err) + } + var failed, ok *journal.Entry + var mkdirCount int + for i := range entries { + en := &entries[i] + if en.Action != "undo-mkdir" { + continue + } + mkdirCount++ + switch en.Status { + case "failed": + failed = en + case "ok": + ok = en + } + } + if mkdirCount != 2 { + t.Fatalf("undo-mkdir entries = %d, want exactly 2 (the original refusal plus the retry's addition): %+v", mkdirCount, entries) + } + if failed == nil { + t.Fatal("the original refused undo-mkdir entry is missing - it must never be rewritten or removed") + } + if ok == nil { + t.Fatal("no successful undo-mkdir entry was appended for the retry") + } + if failed.Src != ok.Src { + t.Errorf("failed.Src = %q, ok.Src = %q; want the same directory", failed.Src, ok.Src) + } + if failed.File != ok.File { + t.Errorf("failed.File = %q, ok.File = %q; want the retry entry to carry the file that owned the original undo-mkdir", failed.File, ok.File) + } + if failed.Dir != ok.Dir { + t.Errorf("failed.Dir = %q, ok.Dir = %q; want the same directory name", failed.Dir, ok.Dir) + } + + runs, err := e.Runs(0) + if err != nil { + t.Fatal(err) + } + byID := map[string]bool{} + for _, r := range runs { + byID[r.ID] = r.Undone + } + if !byID[run] { + t.Errorf("original run %q not marked Undone, though every file came back", run) + } + if byID[undoRun] { + t.Errorf("the undo run %q itself must never read as Undone", undoRun) + } +} diff --git a/internal/engine/plan_bench_test.go b/internal/engine/plan_bench_test.go new file mode 100644 index 0000000..a3d08bb --- /dev/null +++ b/internal/engine/plan_bench_test.go @@ -0,0 +1,108 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package engine + +import ( + "context" + "fmt" + "os" + "path/filepath" + "testing" + "time" + + "krino/internal/plan" +) + +// BenchmarkPlan measures krino's own cost of planning: walking a tree, +// matching rules and building action chains (Engine.Plan, which wraps +// Engine.Match and plan.Build). It does NOT measure krino's real-world +// throughput - spec §13 is explicit that there is no performance target +// for 0.0.1, because a full run's wall time is dominated by the external +// extractors (pdftotext and friends), not by krino itself. This benchmark +// therefore uses a config with no (content ...) test, so no extractor +// ever runs and the result is identical on any machine, with or without +// poppler installed. +// +// The tree is built once in b.TempDir(), before the timer starts; each +// iteration re-plans the same on-disk tree with a fresh plan.Claims, so +// iterations are independent and repeatable. +func BenchmarkPlan(b *testing.B) { + root := b.TempDir() + scanRoot := filepath.Join(root, "Filed") + buildBenchTree(b, scanRoot) + + mainFile := filepath.Join(root, "krino.conf") + if err := os.WriteFile(mainFile, []byte(`(include "dl")`), 0o644); err != nil { + b.Fatal(err) + } + dirsDir := filepath.Join(root, "dirs") + if err := os.MkdirAll(dirsDir, 0o755); err != nil { + b.Fatal(err) + } + // Type-only rules (no content test), one per group present in the + // generated tree, mirroring examples/by-type.conf; "dat" files match + // none of them and take the unmatched path through Match. + dirConf := fmt.Sprintf(` +(path %q) +(recursive yes) +(min-age 0s) +(rule "images" (when (type image)) (move "Sorted/Images") (stop)) +(rule "documents" (when (type document)) (move "Sorted/Documents") (stop)) +(rule "spreadsheets" (when (type spreadsheet)) (move "Sorted/Spreadsheets") (stop)) +(rule "archives" (when (type archive)) (move "Sorted/Archives") (stop)) +(rule "media" (when (or (type audio) (type video))) (move "Sorted/Media") (stop)) +`, scanRoot) + if err := os.WriteFile(filepath.Join(dirsDir, "dl.conf"), []byte(dirConf), 0o644); err != nil { + b.Fatal(err) + } + + e, errs := Load(mainFile, "dl") + if len(errs) > 0 { + b.Fatalf("config errors: %v", errs) + } + + ctx := context.Background() + b.ResetTimer() + for i := 0; i < b.N; i++ { + if _, err := e.Plan(ctx, e.Dirs[0], plan.NewClaims()); err != nil { + b.Fatal(err) + } + } +} + +// benchTreeDirs * benchFilesPerDir files are generated, spread over +// nested directories so the walk itself is exercised, not just a single +// flat directory read. +const ( + benchTreeDirs = 12 + benchFilesPerDir = 150 +) + +// buildBenchTree creates a synthetic tree under root for BenchmarkPlan: +// nested "Sub" directories holding files that cycle through extensions +// spanning several of Appendix A's type groups, plus one extension +// ("dat") that matches no rule. Names are neutral (Sub, a<N>.<ext>) - +// never anything from a real folder, per the leak-check patterns. +func buildBenchTree(b *testing.B, root string) { + b.Helper() + exts := []string{"pdf", "jpg", "xlsx", "zip", "mp3", "dat"} + old := time.Now().Add(-time.Hour) + n := 0 + for d := 0; d < benchTreeDirs; d++ { + dir := filepath.Join(root, fmt.Sprintf("Sub%d", d), "Nested") + if err := os.MkdirAll(dir, 0o755); err != nil { + b.Fatal(err) + } + for f := 0; f < benchFilesPerDir; f++ { + ext := exts[n%len(exts)] + p := filepath.Join(dir, fmt.Sprintf("a%04d.%s", n, ext)) + if err := os.WriteFile(p, []byte("x"), 0o644); err != nil { + b.Fatal(err) + } + if err := os.Chtimes(p, old, old); err != nil { + b.Fatal(err) + } + n++ + } + } +} |
