From d6a280d87a274abc8d9cab9956d2af1c845f83d1 Mon Sep 17 00:00:00 2001 From: Lukasz Kasprzak Date: Wed, 16 Sep 2026 00:57:03 +0200 Subject: the engine owns a run: lock, log, run id, claims --- cmd/krino/sort.go | 92 ++++++------------------- internal/engine/session.go | 147 ++++++++++++++++++++++++++++++++++++++++ internal/engine/session_test.go | 119 ++++++++++++++++++++++++++++++++ 3 files changed, 285 insertions(+), 73 deletions(-) create mode 100644 internal/engine/session.go create mode 100644 internal/engine/session_test.go diff --git a/cmd/krino/sort.go b/cmd/krino/sort.go index aeeb1d4..4c0a0d8 100644 --- a/cmd/krino/sort.go +++ b/cmd/krino/sort.go @@ -18,8 +18,6 @@ import ( "golang.org/x/term" "krino/internal/engine" - "krino/internal/journal" - "krino/internal/lock" "krino/internal/plan" "krino/internal/scan" "krino/internal/xdg" @@ -50,8 +48,8 @@ const zeroOutcome = "0 applied · 0 failed · 0 declined" // cmdSort plans and, from Task 7, applies the included directories: flags // are checked before any config is read, a bad config stops the whole run -// before scanning (spec §11), and one journal.Writer, run id and -// plan.Claims cover every directory in the run. See docs/design.md +// before scanning (spec §11), and one engine.Session - its log, run id and +// claims - covers every directory in the run. See docs/design.md // §8.2-§8.4 and §11 for the flow this follows. func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int { if g.yes && g.dry { @@ -96,39 +94,25 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int { stopSignals := installSignalHandler(cancel) defer stopSignals() - // Ruling 2: journal.Open creates $XDG_STATE_HOME/krino/ and an empty - // krino.log as a side effect of merely being called, so a dry run must - // never call it at all - not open it and clean up afterwards. -n is - // known from the flags before the loop starts, so the gate is exactly - // that, nothing per-directory. One Writer and one run id cover every - // directory in the run (Task 5's undo depends on a single run id - // spanning all of them). - var j *journal.Writer - var run string - if !g.dry { - var err error - j, err = journal.Open(e.Config.LogFile()) - if err != nil { - fmt.Fprintf(stderr, "krino: %v\n", err) - return 1 - } - defer func() { - if cerr := j.Close(); cerr != nil { - fmt.Fprintf(stderr, "krino: %v\n", cerr) - } - }() - run = journal.NewRunID(e.Now()) + // The run itself - the log, its run id and the claims every directory + // shares - belongs to the engine, so the GUI runs a directory exactly + // as this does (GUI design §1.3). Ruling 2: a dry session opens no log, + // since journal.Open creates the state directory and an empty krino.log + // merely by being called. + sess, err := e.NewSession(g.dry) + if err != nil { + fmt.Fprintf(stderr, "krino: %v\n", err) + return 1 } + defer func() { + if cerr := sess.Close(); cerr != nil { + fmt.Fprintf(stderr, "krino: %v\n", cerr) + } + }() exit := 0 printed := false jsonDirs := []plan.JSONDir{} // never nil: the document's "dirs" must marshal as [], not null - // A3: in a dry run one Claims is shared across every directory's Plan - // call below, so two directories that both plan a move to the same - // destination resolve the collision at planning time instead of each - // independently believing it owns that path. A real run applies each - // directory before planning the next and starts fresh claims after it. - claims := plan.NewClaims() for _, d := range e.Dirs { // Spec §3/§11: a second krino on the same directory waits for the @@ -137,7 +121,7 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int { // is not unbounded in practice (fix round 2026-09-12/item 1): a // signal cancels it and Acquire returns ctx.Err() promptly instead // of polling forever. - l, err := lock.Acquire(ctx, e.Config.LockFile(d.Name), !g.yes) + l, err := sess.Lock(ctx, d, !g.yes) if err != nil { if interrupted(err) { // Interrupted while waiting for the lock: an interrupt, not @@ -151,7 +135,6 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int { continue } - var applied []string // where this directory's applied steps put files quit := func() bool { defer func() { if rerr := l.Release(); rerr != nil { @@ -164,7 +147,7 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int { exit = 1 return false } - dp, err := e.Plan(ctx, d, claims) + dp, err := sess.Plan(ctx, d) if err != nil { fmt.Fprintf(stderr, "krino: skipping %s: %v\n", d.Name, err) exit = 1 @@ -262,30 +245,7 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int { toApply = &reviewed notReviewed = len(actionable) - len(reviewedChains(actionable, approved)) } - res, aerr := e.Apply(ctx, toApply, approved, j, run) - if res != nil { - // Where files ended up: each copy, and the last place a - // move or rename put the file - not a path it passed - // through and left. - for _, fr := range res.Files { - final := "" - for _, sr := range fr.Steps { - switch { - case sr.Status != "ok": - case sr.Step.Kind == plan.DeletePermanent: - final = "" // gone: nothing is left anywhere - case sr.Dst == "": - case sr.Step.Kind == plan.Copy: - applied = append(applied, sr.Dst) - default: - final = sr.Dst - } - } - if final != "" { - applied = append(applied, final) - } - } - } + res, aerr := sess.Apply(ctx, toApply, approved) if aerr != nil { // Interrupted mid-apply (fix round 2026-09-12/item 2): // treated exactly like the cancelled lock wait above - not @@ -310,20 +270,6 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int { if quit { break } - if !g.dry { - // This directory is applied (or skipped) now, so the disk is the - // truth for the next one: its claims - sources it moved away, - // destinations it planned but declined or failed - must not - // block a later directory (triage 28i). What it did put - // somewhere stays claimed, so a later (on-conflict overwrite) - // takes a free name rather than trash this run's own result - // (plan 11 review M1). A dry run applies nothing, so there every - // claim carries over. - claims = plan.NewClaims() - for _, p := range applied { - claims.Claim(p) - } - } } if g.json { diff --git a/internal/engine/session.go b/internal/engine/session.go new file mode 100644 index 0000000..8d52027 --- /dev/null +++ b/internal/engine/session.go @@ -0,0 +1,147 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package engine + +import ( + "context" + "fmt" + + "krino/internal/journal" + "krino/internal/lock" + "krino/internal/plan" +) + +// Session is one run of krino over one or more directories: the log it +// writes, the run id every entry carries, and the claims its directories +// share. Both the command line and the GUI go through it, so the order a +// directory is locked, planned, applied and released in - and what a later +// directory may still claim - is written once (GUI design §1.3). +// +// A dry session opens no log and takes no run id: journal.Open creates the +// state directory and an empty krino.log merely by being called, and a dry +// run must not (spec §11). +type Session struct { + e *Engine + j *journal.Writer + run string + claims *plan.Claims + dry bool +} + +// NewSession starts a run. A real one opens the log; the caller closes the +// session when the run is over. +func (e *Engine) NewSession(dry bool) (*Session, error) { + s := &Session{e: e, claims: plan.NewClaims(), dry: dry} + if dry { + return s, nil + } + j, err := journal.Open(e.Config.LogFile()) + if err != nil { + return nil, err + } + s.j, s.run = j, journal.NewRunID(e.Now()) + return s, nil +} + +// Run is the run id every entry of this session carries; "" for a dry one. +func (s *Session) Run() string { return s.run } + +// Journal is the log this session writes, nil for a dry one. +func (s *Session) Journal() *journal.Writer { return s.j } + +// Lock takes d's lock (spec §3, §11). wait blocks until the holder is gone, +// or until ctx is cancelled; without it a held lock returns lock.ErrHeld at +// once, so a cron job never piles up behind a stuck run. The caller +// releases it. +func (s *Session) Lock(ctx context.Context, d *Dir, wait bool) (*lock.Lock, error) { + return lock.Acquire(ctx, s.e.Config.LockFile(d.Name), wait) +} + +// LockDirs takes the locks of several directories, in the order given, and +// releases every one it took if any of them cannot be had - so an undo +// spanning directories never holds half of them (spec §10). +func (s *Session) LockDirs(ctx context.Context, names []string, wait bool) ([]*lock.Lock, error) { + var held []*lock.Lock + for _, name := range names { + l, err := lock.Acquire(ctx, s.e.Config.LockFile(name), wait) + if err != nil { + for _, h := range held { + h.Release() + } + return nil, fmt.Errorf("%s: %w", name, err) + } + held = append(held, l) + } + return held, nil +} + +// Plan builds d's plan with the run's claims. +func (s *Session) Plan(ctx context.Context, d *Dir) (*DirPlan, error) { + return s.e.Plan(ctx, d, s.claims) +} + +// Apply carries out the approved files of dp and logs the run's steps. A +// real run applies each directory before the next is planned, so afterwards +// the disk is the truth for the next one: only the paths this directory's +// files ended up at stay claimed, which keeps a later (on-conflict +// overwrite) from displacing this run's own result (spec §7.4). A dry +// session keeps every claim, since it applies nothing. +func (s *Session) Apply(ctx context.Context, dp *DirPlan, approved map[string]bool) (*ApplyResult, error) { + res, err := s.e.Apply(ctx, dp, approved, s.j, s.run) + if !s.dry { + s.claims = plan.NewClaims() + for _, p := range landedAt(res) { + s.claims.Claim(p) + } + } + return res, err +} + +// landedAt is where res's files ended up: each copy, and the last place a +// move or rename put a file - not a path it passed through and left, and +// nothing at all for a file deleted for good. +func landedAt(res *ApplyResult) []string { + if res == nil { + return nil + } + var out []string + for _, fr := range res.Files { + final := "" + for _, sr := range fr.Steps { + switch { + case sr.Status != "ok": + case sr.Step.Kind == plan.DeletePermanent: + final = "" + case sr.Dst == "": + case sr.Step.Kind == plan.Copy: + out = append(out, sr.Dst) + default: + final = sr.Dst + } + } + if final != "" { + out = append(out, final) + } + } + return out +} + +// PlanUndo builds the reversal of runID (spec §10). +func (s *Session) PlanUndo(runID string) (*UndoPlan, error) { + return s.e.PlanUndo(runID) +} + +// ApplyUndo carries out up and logs it under this session's run id. +func (s *Session) ApplyUndo(ctx context.Context, up *UndoPlan) (*ApplyResult, error) { + return s.e.ApplyUndo(ctx, up, s.j, s.run) +} + +// Close closes the log. A dry session has nothing to close. +func (s *Session) Close() error { + if s.j == nil { + return nil + } + err := s.j.Close() + s.j = nil + return err +} diff --git a/internal/engine/session_test.go b/internal/engine/session_test.go new file mode 100644 index 0000000..ece69dd --- /dev/null +++ b/internal/engine/session_test.go @@ -0,0 +1,119 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package engine + +import ( + "context" + "os" + "path/filepath" + "testing" + "time" +) + +// twoOverwritingDirs builds a and b, each holding x.pdf, both moving it to +// ~/Out with (on-conflict overwrite). +func twoOverwritingDirs(t *testing.T, h string) *Engine { + t.Helper() + old := time.Now().Add(-2 * time.Hour) + dirs := map[string]string{} + for _, n := range []string{"a", "b"} { + p := filepath.Join(h, n, "x.pdf") + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte("from "+n), 0o644); err != nil { + t.Fatal(err) + } + os.Chtimes(p, old, old) + dirs[n] = "(path \"~/" + n + "\")\n(on-conflict overwrite)\n(rule \"out\" (move \"~/Out\"))\n" + } + main := writeConfig(t, h, `(include "a" "b")`, dirs) + e, errs := Load(main) + if len(errs) > 0 { + t.Fatal(errs) + } + return e +} + +// TestSessionKeepsOnlyAppliedDestinationsClaimed: one session covers a whole +// run - its log, run id and claims - and after applying a directory only the +// paths its files ended up at stay claimed, so a later directory's overwrite +// takes a free name instead of trashing this run's own result (spec §7.4). +func TestSessionKeepsOnlyAppliedDestinationsClaimed(t *testing.T) { + h := sandbox(t) + e := twoOverwritingDirs(t, h) + s, err := e.NewSession(false) + if err != nil { + t.Fatal(err) + } + defer s.Close() + if s.Run() == "" { + t.Error("a real session has no run id") + } + for _, d := range e.Dirs { + dp, err := s.Plan(context.Background(), d) + if err != nil { + t.Fatal(err) + } + if _, err := s.Apply(context.Background(), dp, map[string]bool{"x.pdf": true}); err != nil { + t.Fatal(err) + } + } + for rel, want := range map[string]string{"Out/x.pdf": "from a", "Out/x_1.pdf": "from b"} { + if b, err := os.ReadFile(filepath.Join(h, rel)); err != nil || string(b) != want { + t.Errorf("%s: %q, %v; want %q", rel, b, err, want) + } + } + if entries, _ := os.ReadDir(filepath.Join(h, ".local", "share", "Trash", "files")); len(entries) != 0 { + t.Errorf("the Trash holds %d entries; nothing should have been displaced", len(entries)) + } +} + +// TestDrySessionWritesNoLog: a dry session opens no log, so a dry run never +// creates the state directory's krino.log (spec §11). +func TestDrySessionWritesNoLog(t *testing.T) { + h := sandbox(t) + e := twoOverwritingDirs(t, h) + s, err := e.NewSession(true) + if err != nil { + t.Fatal(err) + } + defer s.Close() + if s.Run() != "" { + t.Errorf("a dry session took a run id: %q", s.Run()) + } + if _, err := s.Plan(context.Background(), e.Dirs[0]); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(e.Config.LogFile()); !os.IsNotExist(err) { + t.Errorf("a dry session touched the log: %v", err) + } +} + +// TestSessionLockIsTheDirectorysOwn: the session takes the same lock a +// second krino waits for, and releasing it lets the next one in. +func TestSessionLockIsTheDirectorysOwn(t *testing.T) { + h := sandbox(t) + e := twoOverwritingDirs(t, h) + s, err := e.NewSession(true) + if err != nil { + t.Fatal(err) + } + defer s.Close() + l, err := s.Lock(context.Background(), e.Dirs[0], false) + if err != nil { + t.Fatal(err) + } + if _, err := s.Lock(context.Background(), e.Dirs[0], false); err == nil { + t.Error("the directory was locked twice") + } + if err := l.Release(); err != nil { + t.Fatal(err) + } + l2, err := s.Lock(context.Background(), e.Dirs[0], false) + if err != nil { + t.Errorf("the lock was not released: %v", err) + } else { + l2.Release() + } +} -- cgit v1.3