From f9b790a1f916883a82b323e30917f3feb589b14c Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Wed, 12 Aug 2026 11:04:53 +0200 Subject: [PATCH 1/4] Let OnUIThreadAndWait's error be about the wait, not about f Every caller passes an f that unconditionally returns nil, so f's error return has never carried anything: the value is dead weight, and it occupies the one channel the wait itself needs to report that it couldn't run f at all. Drop it, so that the error the wait returns can only ever mean that. Work that can fail hands its error back through a captured variable, the way the background fetch already hands back four values, which keeps the two outcomes distinguishable at a call site that has both. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/gocui/gui.go | 22 ++++++++++++------- pkg/gui/background.go | 6 ++--- .../helpers/merge_and_rebase_helper.go | 3 +-- pkg/gui/controllers/helpers/refresh_helper.go | 8 ++----- .../controllers/patch_building_controller.go | 3 +-- pkg/gui/gui_driver.go | 2 +- pkg/gui/tasks_adapter.go | 9 +++----- pkg/tasks/tasks.go | 14 ++++-------- pkg/tasks/tasks_test.go | 6 ++--- 9 files changed, 31 insertions(+), 42 deletions(-) diff --git a/pkg/gocui/gui.go b/pkg/gocui/gui.go index be52c8584b6..43841b826e0 100644 --- a/pkg/gocui/gui.go +++ b/pkg/gocui/gui.go @@ -893,36 +893,42 @@ func (g *Gui) EndBlockingEvents() error { } // OnUIThreadAndWait runs f on the main event-loop goroutine and blocks the -// caller until f has run, returning f's error. Use it to read UI-thread-owned -// state (the model, contexts) from a worker without racing the UI thread. +// caller until f has run. Use it to read UI-thread-owned state (the model, +// contexts) from a worker without racing the UI thread. +// +// The error it returns is the wait's own, never f's: it reports that f was not +// run at all. f doesn't report an error because what callers want on the UI +// thread — reading and mutating state — doesn't fail. // // It must be called from a worker goroutine, never from the UI thread itself: // the UI thread would block waiting for a callback only it can run, which // deadlocks. Callers arrange this by construction (see the refresh helper's // RefreshFromWorker); a debug-only assertion there guards against getting it // wrong. -func (g *Gui) OnUIThreadAndWait(f func() error) error { +func (g *Gui) OnUIThreadAndWait(f func()) error { return g.onUIThreadAndWait(f, false) } // Like OnUIThreadAndWait, but the enqueued work belongs to a background routine, // so it doesn't count towards the program being busy (see UpdateBackground). -func (g *Gui) OnUIThreadAndWaitBackground(f func() error) error { +func (g *Gui) OnUIThreadAndWaitBackground(f func()) error { return g.onUIThreadAndWait(f, true) } -func (g *Gui) onUIThreadAndWait(f func() error, background bool) error { +func (g *Gui) onUIThreadAndWait(f func(), background bool) error { enqueue := g.Update if background { enqueue = g.UpdateBackground } - result := make(chan error, 1) + ran := make(chan struct{}) enqueue(func(*Gui) error { - result <- f() + f() + close(ran) return nil }) - return <-result + <-ran + return nil } // Calls a function in a goroutine. Handles panics gracefully and tracks diff --git a/pkg/gui/background.go b/pkg/gui/background.go index 180b2d44545..6a4c529a141 100644 --- a/pkg/gui/background.go +++ b/pkg/gui/background.go @@ -119,13 +119,12 @@ func (self *BackgroundRoutineMgr) startBackgroundFetch() { var appStatusHelper *helpers.AppStatusHelper var branchesHelper *helpers.BranchesHelper var fetchGeneration int - if err := self.gui.g.OnUIThreadAndWaitBackground(func() error { + if err := self.gui.g.OnUIThreadAndWaitBackground(func() { git = self.gui.git appStatusHelper = self.gui.helpers.AppStatus branchesHelper = self.gui.helpers.BranchesHelper fetchGeneration = self.gui.c.State().GetRepoGeneration() self.gui.State.LastBackgroundFetchTime = time.Now() - return nil }); err != nil { return err } @@ -184,10 +183,9 @@ func (self *BackgroundRoutineMgr) checkForExternalChanges() { // reading them from this background goroutine would race the reassignment. var git *commands.GitCommand var refreshHelper *helpers.RefreshHelper - if err := self.gui.g.OnUIThreadAndWaitBackground(func() error { + if err := self.gui.g.OnUIThreadAndWaitBackground(func() { git = self.gui.git refreshHelper = self.gui.helpers.Refresh - return nil }); err != nil { return } diff --git a/pkg/gui/controllers/helpers/merge_and_rebase_helper.go b/pkg/gui/controllers/helpers/merge_and_rebase_helper.go index 7c7ab3e9a49..8f27efa08d4 100644 --- a/pkg/gui/controllers/helpers/merge_and_rebase_helper.go +++ b/pkg/gui/controllers/helpers/merge_and_rebase_helper.go @@ -188,9 +188,8 @@ func (self *MergeAndRebaseHelper) hasExecTodos(calledFromWorker bool) bool { } result := false - _ = self.c.GocuiGui().OnUIThreadAndWait(func() error { + _ = self.c.GocuiGui().OnUIThreadAndWait(func() { result = check() - return nil }) return result } diff --git a/pkg/gui/controllers/helpers/refresh_helper.go b/pkg/gui/controllers/helpers/refresh_helper.go index 53505ba6c8c..ce0cfe25ef4 100644 --- a/pkg/gui/controllers/helpers/refresh_helper.go +++ b/pkg/gui/controllers/helpers/refresh_helper.go @@ -1254,14 +1254,10 @@ func (self *RefreshHelper) captureOnUIThread(calledFromWorker bool, background b return } - wrapped := func() error { - fn() - return nil - } if background { - _ = self.c.GocuiGui().OnUIThreadAndWaitBackground(wrapped) + _ = self.c.GocuiGui().OnUIThreadAndWaitBackground(fn) } else { - _ = self.c.GocuiGui().OnUIThreadAndWait(wrapped) + _ = self.c.GocuiGui().OnUIThreadAndWait(fn) } } diff --git a/pkg/gui/controllers/patch_building_controller.go b/pkg/gui/controllers/patch_building_controller.go index f3e26e3031f..e1405463a2d 100644 --- a/pkg/gui/controllers/patch_building_controller.go +++ b/pkg/gui/controllers/patch_building_controller.go @@ -230,9 +230,8 @@ func (self *PatchBuildingController) discardSelectionFromCommit() error { err := self.c.Git().Patch.DeletePatchesFromCommit(commits, commitIndex) // Escape pops the patch-building context, so run it on the UI thread // before the refresh below. - _ = self.c.GocuiGui().OnUIThreadAndWait(func() error { + _ = self.c.GocuiGui().OnUIThreadAndWait(func() { self.c.Helpers().PatchBuilding.Escape() - return nil }) return self.c.Helpers().MergeAndRebase.CheckMergeOrRebaseWithRefreshOptions( err, types.RefreshOptions{}) diff --git a/pkg/gui/gui_driver.go b/pkg/gui/gui_driver.go index 25bfcf2d51e..673d9d726c7 100644 --- a/pkg/gui/gui_driver.go +++ b/pkg/gui/gui_driver.go @@ -82,7 +82,7 @@ func (self *GuiDriver) WaitUntilIdle() { } func (self *GuiDriver) OnUIThreadAndWait(f func()) { - _ = self.gui.g.OnUIThreadAndWait(func() error { f(); return nil }) + _ = self.gui.g.OnUIThreadAndWait(f) } func (self *GuiDriver) replayMouseEvent(x, y int, buttons tcell.ButtonMask) { diff --git a/pkg/gui/tasks_adapter.go b/pkg/gui/tasks_adapter.go index 3dce93874aa..ccc0b308cbb 100644 --- a/pkg/gui/tasks_adapter.go +++ b/pkg/gui/tasks_adapter.go @@ -80,9 +80,8 @@ func (gui *Gui) newStringTaskWithoutScroll(view *gocui.View, str string) error { manager := gui.getManager(view) f := func(tasks.TaskOpts) error { - return gui.g.OnUIThreadAndWaitBackground(func() error { + return gui.g.OnUIThreadAndWaitBackground(func() { gui.c.SetViewContent(view, str) - return nil }) } @@ -97,10 +96,9 @@ func (gui *Gui) newStringTaskWithScroll(view *gocui.View, str string, originX in manager := gui.getManager(view) f := func(tasks.TaskOpts) error { - return gui.g.OnUIThreadAndWaitBackground(func() error { + return gui.g.OnUIThreadAndWaitBackground(func() { gui.c.SetViewContent(view, str) view.SetOrigin(originX, originY) - return nil }) } @@ -115,10 +113,9 @@ func (gui *Gui) newStringTaskWithKey(view *gocui.View, str string, key string) e manager := gui.getManager(view) f := func(tasks.TaskOpts) error { - return gui.g.OnUIThreadAndWaitBackground(func() error { + return gui.g.OnUIThreadAndWaitBackground(func() { gui.c.ResetViewOrigin(view) gui.c.SetViewContent(view, str) - return nil }) } diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index 3768e0c1936..df7791aafc6 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -87,7 +87,7 @@ type ViewBufferManager struct { // of the view happen through this, so that the view is only ever touched on // the UI thread (where it is also laid out and drawn), never on the task's // own goroutine. - onUIThread func(f func() error) error + onUIThread func(f func()) error // if the user flicks through a heap of items, with each one // spawning a process to render something to the main view, @@ -126,7 +126,7 @@ func NewViewBufferManager( onEndOfInput func(), onNewKey func(), newGocuiTask func() gocui.Task, - onUIThread func(f func() error) error, + onUIThread func(f func()) error, ) *ViewBufferManager { return &ViewBufferManager{ Log: log, @@ -358,10 +358,7 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix // onEndOfInput reads the view's dimensions (to decide // whether to scroll) and sets the origin, both of which // are UI-thread-only, so run it there. - _ = self.onUIThread(func() error { - self.onEndOfInput() - return nil - }) + _ = self.onUIThread(self.onEndOfInput) callThen() break outer } @@ -502,10 +499,7 @@ func (self *ViewBufferManager) NewTask(f func(TaskOpts) error, key string) error // must happen after releasing taskIDMutex: it blocks until the UI // thread runs it, and a NewTask call on the UI thread takes // taskIDMutex, so holding it here would deadlock. - _ = self.onUIThread(func() error { - self.onNewKey() - return nil - }) + _ = self.onUIThread(self.onNewKey) } self.waitingMutex.Lock() diff --git a/pkg/tasks/tasks_test.go b/pkg/tasks/tasks_test.go index 2cea139e8b3..dec476ba4ff 100644 --- a/pkg/tasks/tasks_test.go +++ b/pkg/tasks/tasks_test.go @@ -40,7 +40,7 @@ func TestNewCmdTaskInstantStop(t *testing.T) { onNewKey, newTask, // no UI thread in the test; run the view mutations inline - func(f func() error) error { return f() }, + func(f func()) error { f(); return nil }, ) stop := make(chan struct{}) @@ -107,7 +107,7 @@ func TestNewCmdTask(t *testing.T) { onNewKey, newTask, // no UI thread in the test; run the view mutations inline - func(f func() error) error { return f() }, + func(f func()) error { f(); return nil }, ) stop := make(chan struct{}) @@ -242,7 +242,7 @@ func TestNewCmdTaskRefresh(t *testing.T) { func() {}, newTask, // no UI thread in the test; run the view mutations inline - func(f func() error) error { return f() }, + func(f func()) error { f(); return nil }, ) stop := make(chan struct{}) From 2f06724b802f12cf71ea8cab34f882be2f45fb95 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Wed, 12 Aug 2026 12:59:48 +0200 Subject: [PATCH 2/4] Make RefreshHelper pay attention to the error returned from OnUIThreadAndWait Right now the function always returns nil, but this will change later in this branch, so handle errors properly. Without that, the first capture that assigns env.git would not run, leave env.git nil, and subsequent code would crash. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/gui/controllers/helpers/refresh_helper.go | 61 ++++++++++++------- 1 file changed, 40 insertions(+), 21 deletions(-) diff --git a/pkg/gui/controllers/helpers/refresh_helper.go b/pkg/gui/controllers/helpers/refresh_helper.go index ce0cfe25ef4..6672fab2656 100644 --- a/pkg/gui/controllers/helpers/refresh_helper.go +++ b/pkg/gui/controllers/helpers/refresh_helper.go @@ -223,10 +223,12 @@ func (self *RefreshHelper) performRefresh(options types.RefreshOptions, calledFr background: options.Background || options.DontBlockRepoSwitch, backgroundRoutine: options.Background, } - self.captureOnUIThread(calledFromWorker, env.background, func() { + if !self.captureOnUIThread(calledFromWorker, env.background, func() { env.generation = self.c.State().GetRepoGeneration() env.git = self.c.Git() - }) + }) { + return + } if options.BatchUIUpdates { env.batch = &refreshBounceBatch{} } @@ -321,11 +323,13 @@ func (self *RefreshHelper) performRefresh(options types.RefreshOptions, calledFr var capturedCommits capturedCommitState var capturedReflog capturedReflogState var capturedBranches capturedBranchState - self.captureOnUIThread(calledFromWorker, env.background, func() { + if !self.captureOnUIThread(calledFromWorker, env.background, func() { capturedCommits = self.captureCommitsState() capturedReflog = self.captureReflogState() capturedBranches = self.captureBranchState() - }) + }) { + return + } refresh("commits and commit files", func() { self.refreshCommitsAndCommitFiles(capturedCommits, options.CommitSelection, env) }) @@ -355,35 +359,43 @@ func (self *RefreshHelper) performRefresh(options types.RefreshOptions, calledFr // if we've asked specifically for rebase commits and not those other things var rebaseHashPool *utils.StringPool var rebaseCommits []*models.Commit - self.captureOnUIThread(calledFromWorker, env.background, func() { + if !self.captureOnUIThread(calledFromWorker, env.background, func() { rebaseHashPool, rebaseCommits = self.captureRebaseCommitState() - }) + }) { + return + } refresh("rebase commits", func() { _ = self.refreshRebaseCommits(rebaseHashPool, rebaseCommits, env) }) } if scopeSet.Includes(types.SUB_COMMITS) { var capturedSubCommits capturedSubCommitState - self.captureOnUIThread(calledFromWorker, env.background, func() { + if !self.captureOnUIThread(calledFromWorker, env.background, func() { capturedSubCommits = self.captureSubCommitState() - }) + }) { + return + } refresh("sub commits", func() { _ = self.refreshSubCommitsWithLimit(capturedSubCommits, env) }) } // reason we're not doing this if the COMMITS type is included is that if the COMMITS type _is_ included we will refresh the commit files context anyway if scopeSet.Includes(types.COMMIT_FILES) && !scopeSet.Includes(types.COMMITS) { var capturedCommitFiles capturedCommitFilesState - self.captureOnUIThread(calledFromWorker, env.background, func() { + if !self.captureOnUIThread(calledFromWorker, env.background, func() { capturedCommitFiles = self.captureCommitFilesState() - }) + }) { + return + } refresh("commit files", func() { _ = self.refreshCommitFilesContext(capturedCommitFiles, env) }) } fileWg := sync.WaitGroup{} if scopeSet.Includes(types.FILES) { var capturedFiles capturedFilesState - self.captureOnUIThread(calledFromWorker, env.background, func() { + if !self.captureOnUIThread(calledFromWorker, env.background, func() { capturedFiles = self.captureFilesState() - }) + }) { + return + } fileWg.Add(1) refresh("files", func() { _ = self.refreshFilesAndSubmodules(capturedFiles, env) @@ -393,9 +405,11 @@ func (self *RefreshHelper) performRefresh(options types.RefreshOptions, calledFr if scopeSet.Includes(types.STASH) { var stashFilterPath string - self.captureOnUIThread(calledFromWorker, env.background, func() { + if !self.captureOnUIThread(calledFromWorker, env.background, func() { stashFilterPath = self.c.Modes().Filtering.GetPath() - }) + }) { + return + } refresh("stash", func() { self.refreshStashEntries(stashFilterPath, env) }) } @@ -408,9 +422,11 @@ func (self *RefreshHelper) performRefresh(options types.RefreshOptions, calledFr // needs it to keep the remote-branches selection valid, and reading // the Remotes context off the UI thread races its render. var prevSelectedRemote *models.Remote - self.captureOnUIThread(calledFromWorker, env.background, func() { + if !self.captureOnUIThread(calledFromWorker, env.background, func() { prevSelectedRemote = self.c.Contexts().Remotes.GetSelected() - }) + }) { + return + } branchesAndRemotesWg.Add(1) refresh("remotes", func() { loadedRemotes, _ = self.refreshRemotes(prevSelectedRemote, env) @@ -1248,17 +1264,20 @@ func (self *RefreshHelper) onUIThread(background bool, f func() error) { // waiting for a callback that only it can run), and capturing inline also // guarantees the snapshot reflects the state at the moment Refresh was called, // before the calling handler regains control and can mutate it. -func (self *RefreshHelper) captureOnUIThread(calledFromWorker bool, background bool, fn func()) { +// +// It returns false when fn didn't run because the app is shutting down, in +// which case the caller must abandon the refresh rather than compute from a +// snapshot that was never taken. +func (self *RefreshHelper) captureOnUIThread(calledFromWorker bool, background bool, fn func()) bool { if !calledFromWorker { fn() - return + return true } if background { - _ = self.c.GocuiGui().OnUIThreadAndWaitBackground(fn) - } else { - _ = self.c.GocuiGui().OnUIThreadAndWait(fn) + return self.c.GocuiGui().OnUIThreadAndWaitBackground(fn) == nil } + return self.c.GocuiGui().OnUIThreadAndWait(fn) == nil } // capturedFilesState holds the files refresh's context/model inputs, gathered From 70427c8ff5a3ed4fff3a1ad1284148dfc267d93f Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Wed, 12 Aug 2026 11:12:40 +0200 Subject: [PATCH 3/4] Add a test for waiting on the UI thread after the loop has exited Nothing dequeues user events once MainLoop has returned, so a worker blocked in OnUIThreadAndWait is blocked for good. The assertion records that; the next commit makes the wait give up instead. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/gocui/ui_thread_test.go | 46 +++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 pkg/gocui/ui_thread_test.go diff --git a/pkg/gocui/ui_thread_test.go b/pkg/gocui/ui_thread_test.go new file mode 100644 index 00000000000..cc5396d4a7a --- /dev/null +++ b/pkg/gocui/ui_thread_test.go @@ -0,0 +1,46 @@ +package gocui + +import ( + "errors" + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +// errStillWaiting stands in for the result of a wait that hasn't produced one. +var errStillWaiting = errors.New("still waiting") + +// resultOrTimeout reports what a wait returned, or errStillWaiting if it hasn't +// returned by the time we give up on it. +func resultOrTimeout(result chan error) error { + select { + case err := <-result: + return err + case <-time.After(time.Second): + return errStillWaiting + } +} + +// A worker waiting for the UI thread must not be left parked there once the +// main loop has stopped: nothing will ever run its callback, and the shutdown +// that follows blocks until such workers have finished (see +// tasks.ViewBufferManager.Close). +func TestOnUIThreadAndWaitGivesUpWhenTheLoopExits(t *testing.T) { + g := newTestGui(t) + + // Closing this is what MainLoop returning does. From here on nothing + // dequeues user events, so the callback below is never going to run. + close(g.loopExited) + + result := make(chan error, 1) + go func() { + result <- g.OnUIThreadAndWait(func() {}) + }() + + err := resultOrTimeout(result) + /* EXPECTED: + assert.ErrorIs(t, err, ErrLoopExited) + ACTUAL: */ + assert.ErrorIs(t, err, errStillWaiting) +} From ec577f1afa7ff0e9933f3e511ed65d941ab039a5 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Wed, 12 Aug 2026 11:17:03 +0200 Subject: [PATCH 4/4] Give up waiting for the UI thread once the main loop has exited MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Quitting with confirmOnQuit set hung for three seconds and printed "cannot kill child process", but only with a clean working tree. Closing the confirmation pops the context before running its handler, so the files panel is re-focused and re-renders the main view, and only then does the handler return ErrQuit. With no changed files that render is a string task, whose whole body is one hop to the UI thread — a hop that is never served, because the handler's ErrQuit has meanwhile brought the main loop down. The task can't finish, so the ViewBufferManager.Close that follows waits for it until it times out. (With changed files it's a command task instead, and every blocking point in one of those selects on the stop channel, so Close gets through.) A wait for the UI thread now ends when the loop does. That also covers the command task's own hops, which are stopped only in between them. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/gocui/gui.go | 21 +++++++++++++++++---- pkg/gocui/ui_thread_test.go | 3 --- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/pkg/gocui/gui.go b/pkg/gocui/gui.go index 43841b826e0..a9b68dc5c8d 100644 --- a/pkg/gocui/gui.go +++ b/pkg/gocui/gui.go @@ -38,6 +38,11 @@ var ( // ErrKeybindingNotHandled is returned when a keybinding is not handled, so that the key can be dispatched further ErrKeybindingNotHandled = standardErrors.New("keybinding not handled") + + // ErrLoopExited is returned by OnUIThreadAndWait when MainLoop has already + // returned. Nothing dequeues user events after that, so the callback it was + // asked to run on the main goroutine never will be. + ErrLoopExited = standardErrors.New("main loop exited") ) const ( @@ -897,8 +902,9 @@ func (g *Gui) EndBlockingEvents() error { // contexts) from a worker without racing the UI thread. // // The error it returns is the wait's own, never f's: it reports that f was not -// run at all. f doesn't report an error because what callers want on the UI -// thread — reading and mutating state — doesn't fail. +// run at all, which happens when the main loop has exited (ErrLoopExited). f +// doesn't report an error because what callers want on the UI thread — reading +// and mutating state — doesn't fail. // // It must be called from a worker goroutine, never from the UI thread itself: // the UI thread would block waiting for a callback only it can run, which @@ -927,8 +933,15 @@ func (g *Gui) onUIThreadAndWait(f func(), background bool) error { close(ran) return nil }) - <-ran - return nil + + select { + case <-ran: + return nil + case <-g.loopExited: + // The queue we just enqueued onto is no longer being served, so waiting + // on `ran` here would mean waiting for the rest of the process's life. + return ErrLoopExited + } } // Calls a function in a goroutine. Handles panics gracefully and tracks diff --git a/pkg/gocui/ui_thread_test.go b/pkg/gocui/ui_thread_test.go index cc5396d4a7a..d76bfaf9c8c 100644 --- a/pkg/gocui/ui_thread_test.go +++ b/pkg/gocui/ui_thread_test.go @@ -39,8 +39,5 @@ func TestOnUIThreadAndWaitGivesUpWhenTheLoopExits(t *testing.T) { }() err := resultOrTimeout(result) - /* EXPECTED: assert.ErrorIs(t, err, ErrLoopExited) - ACTUAL: */ - assert.ErrorIs(t, err, errStillWaiting) }