From 40dbf18893c33bc40c8617d0f3e8de16ce2947e2 Mon Sep 17 00:00:00 2001 From: Lukasz Kasprzak Date: Thu, 17 Sep 2026 14:19:23 +0200 Subject: the applying flag is atomic; its test waits instead of polling The race detector found my own new flag: Apply writes it on a worker and Close reads it from the main loop. The test was polling a plain field too, where the real window hears about the apply on the main loop, which orders the writes. --- gui/internal/model/history.go | 11 ++++++----- gui/internal/model/plan.go | 13 ++++++++----- gui/internal/model/plan_test.go | 20 ++++++++++++-------- 3 files changed, 26 insertions(+), 18 deletions(-) (limited to 'gui') diff --git a/gui/internal/model/history.go b/gui/internal/model/history.go index 87ab6e7..d86984b 100644 --- a/gui/internal/model/history.go +++ b/gui/internal/model/history.go @@ -7,6 +7,7 @@ import ( "fmt" "sort" "strings" + "sync/atomic" "time" "git.labunix.xyz/krino/internal/engine" @@ -116,8 +117,8 @@ type UndoTab struct { locks []*lock.Lock // applying is set while the reversal is in flight; Close refuses then, - // for the reason PlanTab.applying gives. - applying bool + // for the reason PlanTab.applying gives. Atomic for the same reason. + applying atomic.Bool } // PlanUndo builds the reversal of runID and locks every directory it @@ -212,8 +213,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 }() + t.applying.Store(true) + defer t.applying.Store(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 { @@ -272,7 +273,7 @@ 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 { + if t.applying.Load() { return ErrApplying } var first error diff --git a/gui/internal/model/plan.go b/gui/internal/model/plan.go index 31a783b..31c12dd 100644 --- a/gui/internal/model/plan.go +++ b/gui/internal/model/plan.go @@ -10,6 +10,7 @@ import ( "errors" "fmt" "sort" + "sync/atomic" "time" "git.labunix.xyz/krino/internal/engine" @@ -53,14 +54,16 @@ type PlanTab struct { dp *engine.DirPlan lock *lock.Lock - // applying is set while Apply is in flight. Close must refuse then: + // applying is set while Apply is in flight, and read from the main loop + // while the apply runs on a worker, so it is atomic. Close must refuse + // while it is set: // 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 + applying atomic.Bool // testBeforeApply, when set, is called just before the engine's Apply // begins, so a test can act while the apply is in flight. @@ -113,7 +116,7 @@ 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 { + if t.applying.Load() { return ErrApplying } if t.lock == nil { @@ -341,12 +344,12 @@ func (t *PlanTab) Apply(ctx context.Context) (*engine.ApplyResult, error) { approved[r.Rel] = true } } - t.applying = true + t.applying.Store(true) if t.testBeforeApply != nil { t.testBeforeApply() } res, err := t.sess.Apply(ctx, t.dp, approved) - t.applying = false + t.applying.Store(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 241a049..3b3d7b9 100644 --- a/gui/internal/model/plan_test.go +++ b/gui/internal/model/plan_test.go @@ -487,9 +487,10 @@ func TestCloseDuringApplyIsRefused(t *testing.T) { // Hold the apply open while Close is attempted. <-done } - var applyErr error + finished := make(chan error, 1) go func() { - _, applyErr = tab.Apply(context.Background()) + _, err := tab.Apply(context.Background()) + finished <- err }() <-started @@ -497,12 +498,15 @@ func TestCloseDuringApplyIsRefused(t *testing.T) { 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) + // Wait for the apply rather than polling its fields: the real window + // hears about it on the main loop, which orders the writes. + select { + case err := <-finished: + if err != nil { + t.Errorf("the apply itself failed: %v", err) + } + case <-time.After(30 * time.Second): + t.Fatal("the apply never finished") } } -- cgit v1.3