the window cannot pull the lock out from under a running apply

Close releases the directory lock and closes the log. The window could
reach it while an apply was still running - saving rules, saving
settings, adding a directory, or closing the window - and the engine
then went on moving files with the log shut underneath: a file moved
that no krino undo can see, the rest of the plan silently abandoned,
and the directory unlocked while krino was still working in it.

PlanTab and UndoTab refuse to close while their apply is in flight
(model.ErrApplying), the window's close request and reloadEngine honour
the refusal instead of ignoring it, and the tabs and Settings are greyed
out for the duration so a button that cannot work says so by being
unavailable rather than by an error afterwards.

The test starts an apply, calls Close from another goroutine while it
is in flight, and requires the refusal.

6 files changed, 157 insertions, 8 deletions

diff --git a/gui/internal/model/history.go b/gui/internal/model/history.go
index 7c166bb..87ab6e7 100644
--- a/gui/internal/model/history.go
+++ b/gui/internal/model/history.go
@@ -114,6 +114,10 @@ type UndoTab struct {
 	sess  *engine.Session
 	up    *engine.UndoPlan
 	locks []*lock.Lock
+
+	// applying is set while the reversal is in flight; Close refuses then,
+	// for the reason PlanTab.applying gives.
+	applying bool
 }
 
 // PlanUndo builds the reversal of runID and locks every directory it
@@ -208,6 +212,8 @@ func (t *UndoTab) SelectedCount() int {
 // said no to (spec §9). Refused files ride along unchanged, as they do on
 // the command line. The locks are released afterwards: the plan is history.
 func (t *UndoTab) Apply(ctx context.Context) (*engine.ApplyResult, error) {
+	t.applying = true
+	defer func() { t.applying = false }()
 	toApply := &engine.UndoPlan{Run: t.up.Run, Cleanup: t.up.Cleanup}
 	for i, f := range t.up.Files {
 		if f.Refused == "" && !t.Rows[i].Selected {
@@ -266,6 +272,9 @@ func undoOutcome(row UndoRow, fr engine.FileResult) string {
 // does. One lock's failure never stops the rest from being released, or the
 // session from being closed. Closing twice is not an error.
 func (t *UndoTab) Close() error {
+	if t.applying {
+		return ErrApplying
+	}
 	var first error
 	for _, l := range t.locks {
 		if err := l.Release(); err != nil && first == nil {
diff --git a/gui/internal/model/plan.go b/gui/internal/model/plan.go
index ccbf17b..c3e9654 100644
--- a/gui/internal/model/plan.go
+++ b/gui/internal/model/plan.go
@@ -7,6 +7,7 @@ package model
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"sort"
 	"time"
@@ -50,8 +51,24 @@ type PlanTab struct {
 	sess *engine.Session
 	dp   *engine.DirPlan
 	lock *lock.Lock
+
+	// applying is set while Apply is in flight. Close must refuse then:
+	// releasing the lock and closing the log under a running apply moves
+	// files the log never records, so undo cannot see them, and unlocks a
+	// directory krino is still working in. The window can reach Close from
+	// several places while an apply runs - saving rules or settings, adding
+	// a directory, closing the window - so the refusal lives here rather
+	// than in whichever of them remembers.
+	applying bool
+
+	// testBeforeApply, when set, is called just before the engine's Apply
+	// begins, so a test can act while the apply is in flight.
+	testBeforeApply func()
 }
 
+// ErrApplying is returned by Close while an apply is still running.
+var ErrApplying = errors.New("krino is still applying this plan")
+
 // Counts is the plan's summary line.
 type Counts struct {
 	Scanned, Acting, Excluded, Skipped, Unmatched, Warned int
@@ -95,6 +112,9 @@ func (t *PlanTab) Run() string { return t.sess.Run() }
 // Close releases the directory's lock and ends the run, which Apply also
 // does once the plan is history. Closing twice is not an error.
 func (t *PlanTab) Close() error {
+	if t.applying {
+		return ErrApplying
+	}
 	if t.lock == nil {
 		return nil
 	}
@@ -303,7 +323,12 @@ func (t *PlanTab) Apply(ctx context.Context) (*engine.ApplyResult, error) {
 			approved[r.Rel] = true
 		}
 	}
+	t.applying = true
+	if t.testBeforeApply != nil {
+		t.testBeforeApply()
+	}
 	res, err := t.sess.Apply(ctx, t.dp, approved)
+	t.applying = false
 	t.Applied = true
 	// An applied plan is history: the run is over and the directory free
 	// again, without closing the window (GUI design §3).
diff --git a/gui/internal/model/plan_test.go b/gui/internal/model/plan_test.go
index a36cf85..1bae2c5 100644
--- a/gui/internal/model/plan_test.go
+++ b/gui/internal/model/plan_test.go
@@ -462,3 +462,46 @@ func TestKeepThisCopyRefusesANonDuplicate(t *testing.T) {
 		t.Error("a row that does not exist was accepted")
 	}
 }
+
+// TestCloseDuringApplyIsRefused: Close releases the directory lock and
+// closes the log. Called while an apply is running - which the window
+// allows: saving rules, saving settings, adding a directory and closing the
+// window all reach it - the engine goes on moving files with the log shut
+// under it, so a file is moved that no krino undo can see, and the
+// directory is unlocked while krino is still working in it.
+func TestCloseDuringApplyIsRefused(t *testing.T) {
+	conf := "(path \"~/dl\")\n(rule \"pdfs\" (when (type pdf)) (move \"Docs\"))\n"
+	e, _ := sandboxDir(t, conf, map[string]string{"a.pdf": "one", "b.pdf": "two"})
+	tab, err := Plan(context.Background(), e, e.Dirs[0])
+	if err != nil {
+		t.Fatal(err)
+	}
+	for i := range tab.Rows {
+		tab.Rows[i].Selected = true
+	}
+
+	started := make(chan struct{})
+	done := make(chan struct{})
+	tab.testBeforeApply = func() {
+		close(started)
+		// Hold the apply open while Close is attempted.
+		<-done
+	}
+	var applyErr error
+	go func() {
+		_, applyErr = tab.Apply(context.Background())
+	}()
+	<-started
+
+	if err := tab.Close(); err == nil {
+		t.Error("Close during an apply was allowed: the lock and the log go out from under it")
+	}
+	close(done)
+	// Let the apply finish before the sandbox is torn down.
+	for i := 0; i < 200 && !tab.Applied; i++ {
+		time.Sleep(10 * time.Millisecond)
+	}
+	if applyErr != nil {
+		t.Errorf("the apply itself failed: %v", applyErr)
+	}
+}
diff --git a/gui/internal/ui/history.go b/gui/internal/ui/history.go
index f82f6e3..3a3166e 100644
--- a/gui/internal/ui/history.go
+++ b/gui/internal/ui/history.go
@@ -4,6 +4,7 @@ package ui
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"strings"
 
@@ -36,6 +37,10 @@ type historyView struct {
 	undo    *gtk.Button
 	cancel  *gtk.Button
 
+	// applying is set while a reversal is in flight; the window refuses
+	// anything that would take its lock or its log away.
+	applying bool
+
 	tab      *model.UndoTab
 	cancelOp context.CancelFunc
 }
@@ -349,6 +354,8 @@ func (h *historyView) onUndo() {
 		return
 	}
 	n := h.tab.SelectedCount()
+	h.applying = true
+	h.w.setApplying(true)
 	h.setBusy(true)
 	h.w.setStatus("reversing %d file(s)...", n)
 	var res *engine.ApplyResult
@@ -357,6 +364,8 @@ func (h *historyView) onUndo() {
 		res, err = h.tab.Apply(ctx)
 		return err
 	}, func(err error) {
+		h.applying = false
+		h.w.setApplying(false)
 		h.setBusy(false)
 		h.fillPlan()
 		h.undo.SetSensitive(false)
@@ -404,6 +413,9 @@ func (h *historyView) updateUndoButton() {
 }
 
 // setBusy turns the buttons on or off around a background operation.
+// busyApplying reports whether an undo is in flight.
+func (h *historyView) busyApplying() bool { return h.applying }
+
 func (h *historyView) setBusy(busy bool) {
 	h.reload.SetSensitive(!busy)
 	h.more.SetSensitive(!busy && len(h.runRows) >= h.limit)
@@ -419,15 +431,19 @@ func (h *historyView) setBusy(busy bool) {
 }
 
 // closeTab drops the open undo plan and releases its locks.
-func (h *historyView) closeTab() {
+func (h *historyView) closeTab() bool {
 	if h.tab == nil {
-		return
+		return true
 	}
 	if err := h.tab.Close(); err != nil {
 		h.w.setStatus("undo: %v", err)
+		if errors.Is(err, model.ErrApplying) {
+			return false
+		}
 	}
 	h.tab = nil
 	h.clearPlan()
+	return true
 }
 
 // clearList removes every row of a ListBox.
diff --git a/gui/internal/ui/plan.go b/gui/internal/ui/plan.go
index 1ee2a51..e59d4dc 100644
--- a/gui/internal/ui/plan.go
+++ b/gui/internal/ui/plan.go
@@ -4,6 +4,7 @@ package ui
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -63,6 +64,7 @@ type planView struct {
 	previewOff       bool
 	sortFollowsPrefs bool
 	startSelected    bool
+	applying         bool
 	menu             *gtk.Popover
 	keep             *gtk.Button
 	menuRow          int
@@ -555,6 +557,8 @@ func (p *planView) onApply() {
 		return
 	}
 	n := p.tab.SelectedCount()
+	p.applying = true
+	p.w.setApplying(true)
 	p.setBusy(true)
 	p.w.setStatus("applying %d file(s)...", n)
 	var res *engine.ApplyResult
@@ -563,6 +567,8 @@ func (p *planView) onApply() {
 		res, err = p.tab.Apply(ctx)
 		return err
 	}, func(err error) {
+		p.applying = false
+		p.w.setApplying(false)
 		p.setBusy(false)
 		p.apply.SetSensitive(false)
 		p.fillList()
@@ -576,17 +582,27 @@ func (p *planView) onApply() {
 }
 
 // closeTab drops the open plan and releases the directory's lock.
-func (p *planView) closeTab() {
+func (p *planView) closeTab() bool {
 	if p.tab == nil {
-		return
+		return true
 	}
 	if err := p.tab.Close(); err != nil {
 		p.w.setStatus("%s: %v", escape(p.tab.Dir.Name), err)
+		if errors.Is(err, model.ErrApplying) {
+			// The lock and the log must stay put until the apply is done,
+			// or files move with nothing recording them.
+			return false
+		}
 	}
 	p.tab = nil
 	p.fillList()
+	return true
 }
 
+// busyApplying reports whether an apply is in flight, so the window can
+// refuse anything that would take the lock or the log away from it.
+func (p *planView) busyApplying() bool { return p.applying }
+
 // setBusy turns the buttons on or off around a background operation.
 func (p *planView) setBusy(busy bool) {
 	p.scan.SetSensitive(!busy)
diff --git a/gui/internal/ui/window.go b/gui/internal/ui/window.go
index 3d40441..a8a087e 100644
--- a/gui/internal/ui/window.go
+++ b/gui/internal/ui/window.go
@@ -34,6 +34,8 @@ type Window struct {
 	plan      *planView
 	history   *historyView
 	rules     *rulesView
+	notebook  *gtk.Notebook
+	settings  *gtk.Button
 	status    *gtk.Label
 	prefs     model.Prefs
 	leaving   bool
@@ -50,6 +52,7 @@ func NewWindow(app *gtk.Application, e *engine.Engine) *Window {
 	w.win.SetDefaultSize(1200, 720)
 
 	notebook := gtk.NewNotebook()
+	w.notebook = notebook
 	w.plan = newPlanView(w)
 	notebook.AppendPage(w.plan.root, gtk.NewLabel("Plan"))
 	w.history = newHistoryView(w)
@@ -77,6 +80,7 @@ func NewWindow(app *gtk.Application, e *engine.Engine) *Window {
 	settings.SetTooltipText("krino's defaults, and how this window behaves")
 	settings.SetMarginEnd(6)
 	settings.ConnectClicked(func() { w.showSettings() })
+	w.settings = settings
 	notebook.SetActionWidget(settings, gtk.PackEnd)
 
 	w.status = gtk.NewLabel("")
@@ -105,9 +109,12 @@ func NewWindow(app *gtk.Application, e *engine.Engine) *Window {
 			w.confirmLeaving()
 			return true
 		}
-		w.plan.closeTab()
+		// An apply in flight keeps its lock and its log: closing the
+		// window under it would move files nothing records.
+		if !w.plan.closeTab() || !w.history.closeTab() {
+			return true
+		}
 		w.plan.closePreview()
-		w.history.closeTab()
 		return false
 	})
 	themeColours(w.win)
@@ -118,17 +125,50 @@ func NewWindow(app *gtk.Application, e *engine.Engine) *Window {
 // Show puts the window on screen.
 func (w *Window) Show() { w.win.Show() }
 
+// setApplying greys out everything that would pull the directory lock or
+// the log away from a running apply: the other tabs, and Settings. The
+// model refuses those anyway, but a button that cannot work should say so
+// by being unavailable rather than by an error afterwards.
+func (w *Window) setApplying(busy bool) {
+	if w.settings != nil {
+		w.settings.SetSensitive(!busy)
+	}
+	if w.notebook == nil {
+		return
+	}
+	current := int(w.notebook.CurrentPage())
+	for i := 0; i < int(w.notebook.NPages()); i++ {
+		if i == current {
+			continue
+		}
+		if page, ok := w.notebook.NthPage(i).(interface{ SetSensitive(bool) }); ok {
+			page.SetSensitive(!busy)
+		}
+	}
+}
+
 // reloadEngine re-reads the configuration, after the rules editor saves, so
 // every tab works from the rules the user just wrote. An open plan came
 // from the old ones, so it is closed and its lock released.
+//
+// It refuses while an apply is running. Saving rules, saving settings and
+// adding a directory all come through here, and every one of them is
+// reachable from the window while files are being moved; closing the plan
+// then releases the directory lock and shuts the log under the engine, so a
+// file moves that no krino undo can see and the rest of the plan is
+// abandoned.
 func (w *Window) reloadEngine() error {
+	if w.plan.busyApplying() || w.history.busyApplying() {
+		return model.ErrApplying
+	}
 	e, diags := engine.Load(w.engine.MainFile)
 	if len(diags) > 0 {
 		return diags[0]
 	}
 	e.CacheDir = w.engine.CacheDir
-	w.plan.closeTab()
-	w.history.closeTab()
+	if !w.plan.closeTab() || !w.history.closeTab() {
+		return model.ErrApplying
+	}
 	w.engine = e
 	return nil
 }