From 40ecbfae85c4c76a8ed944a91f8c000b4ec55fc5 Mon Sep 17 00:00:00 2001
From: Lukasz Kasprzak <lukas@labunix.xyz>
Date: Wed, 16 Sep 2026 00:59:08 +0200
Subject: undo runs through the same session

---
 internal/engine/session.go      | 36 ++++++++++++++++-------
 internal/engine/session_test.go | 63 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 89 insertions(+), 10 deletions(-)

(limited to 'internal')

diff --git a/internal/engine/session.go b/internal/engine/session.go
index 8d52027..33b6a76 100644
--- a/internal/engine/session.go
+++ b/internal/engine/session.go
@@ -28,22 +28,32 @@ type Session struct {
 	dry    bool
 }
 
-// NewSession starts a run. A real one opens the log; the caller closes the
-// session when the run is over.
+// NewSession starts a run. The log is opened by OpenLog, or by the first
+// Apply or ApplyUndo: sorting opens it before it plans anything, while undo
+// opens it only once it knows it will reverse something, and neither must
+// create a log a dry run would not. 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
+	return &Session{e: e, claims: plan.NewClaims(), dry: dry}, nil
+}
+
+// OpenLog opens the log and takes the run id, once. A dry session does
+// neither: journal.Open creates the state directory and an empty krino.log
+// merely by being called (spec §11).
+func (s *Session) OpenLog() error {
+	if s.dry || s.j != nil {
+		return nil
 	}
-	j, err := journal.Open(e.Config.LogFile())
+	j, err := journal.Open(s.e.Config.LogFile())
 	if err != nil {
-		return nil, err
+		return err
 	}
-	s.j, s.run = j, journal.NewRunID(e.Now())
-	return s, nil
+	s.j, s.run = j, journal.NewRunID(s.e.Now())
+	return nil
 }
 
-// Run is the run id every entry of this session carries; "" for a dry one.
+// Run is the run id every entry of this session carries; "" for a dry one,
+// and until the log is open.
 func (s *Session) Run() string { return s.run }
 
 // Journal is the log this session writes, nil for a dry one.
@@ -87,6 +97,9 @@ func (s *Session) Plan(ctx context.Context, d *Dir) (*DirPlan, error) {
 // 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) {
+	if err := s.OpenLog(); err != nil {
+		return nil, err
+	}
 	res, err := s.e.Apply(ctx, dp, approved, s.j, s.run)
 	if !s.dry {
 		s.claims = plan.NewClaims()
@@ -133,6 +146,9 @@ func (s *Session) PlanUndo(runID string) (*UndoPlan, error) {
 
 // ApplyUndo carries out up and logs it under this session's run id.
 func (s *Session) ApplyUndo(ctx context.Context, up *UndoPlan) (*ApplyResult, error) {
+	if err := s.OpenLog(); err != nil {
+		return nil, err
+	}
 	return s.e.ApplyUndo(ctx, up, s.j, s.run)
 }
 
diff --git a/internal/engine/session_test.go b/internal/engine/session_test.go
index ece69dd..8fd4593 100644
--- a/internal/engine/session_test.go
+++ b/internal/engine/session_test.go
@@ -47,6 +47,9 @@ func TestSessionKeepsOnlyAppliedDestinationsClaimed(t *testing.T) {
 		t.Fatal(err)
 	}
 	defer s.Close()
+	if err := s.OpenLog(); err != nil {
+		t.Fatal(err)
+	}
 	if s.Run() == "" {
 		t.Error("a real session has no run id")
 	}
@@ -79,6 +82,9 @@ func TestDrySessionWritesNoLog(t *testing.T) {
 		t.Fatal(err)
 	}
 	defer s.Close()
+	if err := s.OpenLog(); err != nil {
+		t.Fatal(err)
+	}
 	if s.Run() != "" {
 		t.Errorf("a dry session took a run id: %q", s.Run())
 	}
@@ -117,3 +123,60 @@ func TestSessionLockIsTheDirectorysOwn(t *testing.T) {
 		l2.Release()
 	}
 }
+
+// TestSessionUndoReversesTheRun: undo goes through the same session as
+// sorting - its locks, its log, its run id (GUI design §1.3).
+func TestSessionUndoReversesTheRun(t *testing.T) {
+	e, run, h, _ := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one"}},
+		map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (move \"Out\"))\n"})
+	s, err := e.NewSession(false)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer s.Close()
+	locks, err := s.LockDirs(context.Background(), []string{"dl"}, false)
+	if err != nil {
+		t.Fatal(err)
+	}
+	up, err := s.PlanUndo(run)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, err := s.ApplyUndo(context.Background(), up); err != nil {
+		t.Fatal(err)
+	}
+	for _, l := range locks {
+		l.Release()
+	}
+	if _, err := os.Stat(filepath.Join(h, "dl", "a.pdf")); err != nil {
+		t.Errorf("undo did not put the file back: %v", err)
+	}
+}
+
+// TestSessionLockDirsReleasesOnFailure: when one directory's lock cannot be
+// had, the locks already taken are released, so a failed undo leaves none
+// held.
+func TestSessionLockDirsReleasesOnFailure(t *testing.T) {
+	h := sandbox(t)
+	e := twoOverwritingDirs(t, h)
+	s, err := e.NewSession(true)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer s.Close()
+	held, err := s.Lock(context.Background(), e.Dirs[1], false)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, err := s.LockDirs(context.Background(), []string{"a", "b"}, false); err == nil {
+		t.Fatal("LockDirs took a lock another holder had")
+	}
+	held.Release()
+	locks, err := s.LockDirs(context.Background(), []string{"a", "b"}, false)
+	if err != nil {
+		t.Fatalf("the first directory's lock was left held: %v", err)
+	}
+	for _, l := range locks {
+		l.Release()
+	}
+}
-- 
cgit v1.3