From 7e1073a0ee1d8338bb1c3d39d3cbbd7eedbdacee Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Wed, 15 Jul 2026 14:32:48 +0200 Subject: [PATCH 1/3] Extract the tcell-to-gocui event conversion out of pollEvent A following commit needs pollEvent to attach information from the replayed-event wrappers to the GocuiEvent it returns. With the conversion inlined there is no seam to do that in, because every branch of the type switch returns directly. Co-Authored-By: Claude Fable 5 --- pkg/gocui/tcell_driver.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/gocui/tcell_driver.go b/pkg/gocui/tcell_driver.go index 226ee0580..857e35bfc 100644 --- a/pkg/gocui/tcell_driver.go +++ b/pkg/gocui/tcell_driver.go @@ -300,6 +300,10 @@ func (g *Gui) pollEvent() GocuiEvent { tev = <-Screen.EventQ() } + return gocuiEventFromTcellEvent(tev) +} + +func gocuiEventFromTcellEvent(tev tcell.Event) GocuiEvent { switch tev := tev.(type) { case *tcell.EventInterrupt: return GocuiEvent{Type: eventInterrupt} From 664a65d5843765b6a0e89f2267c276c6e1bc1aab Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Wed, 15 Jul 2026 14:45:52 +0200 Subject: [PATCH 2/3] Track replayed test input as busy from the moment it is submitted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Integration tests synchronize with lazygit through the task manager: after submitting an input event, the test driver waits until the program goes idle before asserting. But a submitted event only got its task once the main loop picked it up from the events channel; while it was still in flight (handed to the poller goroutine, or sitting in the channel), no task existed for it, so the program could look idle even though input was still pending. The edge-triggered idle protocol mostly papers over this: each wait is satisfied by the *next* busy-to-idle transition, which in practice is the one produced by processing the submitted event. It only goes wrong when some other task (e.g. a background refresh) completes in that window, producing an edge the waiting test mistakes for its own — a rare source of test flakes. The next commit replaces that protocol with a level-triggered one, for which the window would be fatal rather than rare: a wait falling into the gap would return immediately. Close the gap by creating the task on the test goroutine before the event is submitted, and carrying it through the poller into the main loop, which uses it instead of creating its own. The new Replay* methods own this invariant, and the replayed-events channels are no longer exported, so tests can't submit an untracked event. Co-Authored-By: Claude Fable 5 --- pkg/gocui/gui.go | 46 ++++++++++++++++++++++++++++++++++----- pkg/gocui/tcell_driver.go | 28 +++++++++++++++++++----- pkg/gui/gui_driver.go | 16 +++++++------- 3 files changed, 72 insertions(+), 18 deletions(-) diff --git a/pkg/gocui/gui.go b/pkg/gocui/gui.go index ceb570c59..e5588e262 100644 --- a/pkg/gocui/gui.go +++ b/pkg/gocui/gui.go @@ -125,8 +125,11 @@ type clickInfo struct { // and keybindings. type Gui struct { RecordingConfig - // ReplayedEvents is for passing pre-recorded input events, for the purposes of testing - ReplayedEvents replayedEvents + // replayedEvents is for passing simulated input events, for the purposes + // of testing. Events must be submitted through the Replay* methods, which + // attach a task to each event; pushing into the channels directly would + // bypass the busy-tracking that integration tests rely on. + replayedEvents replayedEvents playRecording bool tabClickBindings []*tabClickBinding @@ -255,7 +258,7 @@ func NewGui(opts NewGuiOpts) (*Gui, error) { g.taskManager = newTaskManager() if opts.PlayRecording { - g.ReplayedEvents = replayedEvents{ + g.replayedEvents = replayedEvents{ Keys: make(chan *TcellKeyEventWrapper), Resizes: make(chan *TcellResizeEventWrapper), MouseEvents: make(chan *TcellMouseEventWrapper), @@ -291,6 +294,30 @@ func (g *Gui) NewBackgroundTask() *TaskImpl { return g.taskManager.NewTask(true) } +// ReplayKeyEvent simulates a key press, as if the user had typed it. It's used +// by integration tests. The event carries a task, so that the program counts +// as busy from before the event is submitted until the main loop has fully +// processed it; the test driver relies on this when it waits for the program +// to go idle after submitting an event. (If the task were only created once +// the main loop picks the event up, there would be a window in which the event +// is still in flight but nothing counts as busy.) +func (g *Gui) ReplayKeyEvent(ev *TcellKeyEventWrapper) { + ev.task = g.NewTask() + g.replayedEvents.Keys <- ev +} + +// ReplayMouseEvent is like ReplayKeyEvent, but for mouse events. +func (g *Gui) ReplayMouseEvent(ev *TcellMouseEventWrapper) { + ev.task = g.NewTask() + g.replayedEvents.MouseEvents <- ev +} + +// ReplayFocusEvent is like ReplayKeyEvent, but for focus events. +func (g *Gui) ReplayFocusEvent(ev *TcellFocusEventWrapper) { + ev.task = g.NewTask() + g.replayedEvents.FocusEvents <- ev +} + // Busy reports whether any foreground work is in flight, ignoring the event // currently being processed on the main goroutine (see currentTask). Background // routines (auto-fetch etc.) don't count. It's used to decide whether it's safe @@ -948,7 +975,12 @@ func (g *Gui) processEvent() error { // are always the primary event here. select { case ev := <-g.gEvents: - task := g.NewTask() + // Replayed test events already carry their task (see ReplayKeyEvent); + // organic events get theirs here. + task := ev.task + if task == nil { + task = g.NewTask() + } g.currentTask = task defer func() { g.currentTask = nil; task.Done() }() @@ -992,7 +1024,11 @@ func (g *Gui) processRemainingEvents() (bool, error) { select { case ev := <-g.gEvents: contentOnly = false - if err := g.handleError(g.handleEvent(&ev)); err != nil { + err := g.handleError(g.handleEvent(&ev)) + if ev.task != nil { + ev.task.Done() + } + if err != nil { return false, err } default: diff --git a/pkg/gocui/tcell_driver.go b/pkg/gocui/tcell_driver.go index 857e35bfc..885bcbabb 100644 --- a/pkg/gocui/tcell_driver.go +++ b/pkg/gocui/tcell_driver.go @@ -172,6 +172,12 @@ type GocuiEvent struct { Focused bool Start bool N int + + // task tracks the processing of this event for idle detection. Events + // replayed by integration tests carry a task from the moment they are + // submitted (see Gui.ReplayKeyEvent); for organic events it is nil, and + // the main loop creates a task when it picks the event up. + task Task } // Event types. @@ -208,6 +214,8 @@ type TcellKeyEventWrapper struct { Mod tcell.ModMask Key tcell.Key Ch string + + task Task // see GocuiEvent.task } func NewTcellKeyEventWrapper(event *tcell.EventKey, timestamp int64) *TcellKeyEventWrapper { @@ -229,6 +237,8 @@ type TcellMouseEventWrapper struct { Y int ButtonMask tcell.ButtonMask ModMask tcell.ModMask + + task Task // see GocuiEvent.task } func NewTcellMouseEventWrapper(event *tcell.EventMouse, timestamp int64) *TcellMouseEventWrapper { @@ -269,6 +279,8 @@ func (wrapper TcellResizeEventWrapper) toTcellEvent() tcell.Event { type TcellFocusEventWrapper struct { Timestamp int64 Focused bool + + task Task // see GocuiEvent.task } func NewTcellFocusEventWrapper(event *tcell.EventFocus, timestamp int64) *TcellFocusEventWrapper { @@ -285,22 +297,28 @@ func (wrapper TcellFocusEventWrapper) toTcellEvent() tcell.Event { // pollEvent get tcell.Event and transform it into gocuiEvent func (g *Gui) pollEvent() GocuiEvent { var tev tcell.Event + var task Task if g.playRecording { select { - case ev := <-g.ReplayedEvents.Keys: + case ev := <-g.replayedEvents.Keys: tev = (ev).toTcellEvent() - case ev := <-g.ReplayedEvents.Resizes: + task = ev.task + case ev := <-g.replayedEvents.Resizes: tev = (ev).toTcellEvent() - case ev := <-g.ReplayedEvents.MouseEvents: + case ev := <-g.replayedEvents.MouseEvents: tev = (ev).toTcellEvent() - case ev := <-g.ReplayedEvents.FocusEvents: + task = ev.task + case ev := <-g.replayedEvents.FocusEvents: tev = (ev).toTcellEvent() + task = ev.task } } else { tev = <-Screen.EventQ() } - return gocuiEventFromTcellEvent(tev) + event := gocuiEventFromTcellEvent(tev) + event.task = task + return event } func gocuiEventFromTcellEvent(tev tcell.Event) GocuiEvent { diff --git a/pkg/gui/gui_driver.go b/pkg/gui/gui_driver.go index fef33bf66..a54530791 100644 --- a/pkg/gui/gui_driver.go +++ b/pkg/gui/gui_driver.go @@ -33,10 +33,10 @@ func (self *GuiDriver) PressKey(keyStr string) { self.Fail("Unrecognized key: " + keyStr) } - self.gui.g.ReplayedEvents.Keys <- gocui.NewTcellKeyEventWrapper( + self.gui.g.ReplayKeyEvent(gocui.NewTcellKeyEventWrapper( tcell.NewEventKey(tcell.Key(key.KeyName()), key.Str(), tcell.ModMask(key.Mod())), 0, - ) + )) self.waitTillIdle() } @@ -44,15 +44,15 @@ func (self *GuiDriver) PressKey(keyStr string) { func (self *GuiDriver) Click(x, y int) { self.CheckAllToastsAcknowledged() - self.gui.g.ReplayedEvents.MouseEvents <- gocui.NewTcellMouseEventWrapper( + self.gui.g.ReplayMouseEvent(gocui.NewTcellMouseEventWrapper( tcell.NewEventMouse(x, y, tcell.ButtonPrimary, 0), 0, - ) + )) self.waitTillIdle() - self.gui.g.ReplayedEvents.MouseEvents <- gocui.NewTcellMouseEventWrapper( + self.gui.g.ReplayMouseEvent(gocui.NewTcellMouseEventWrapper( tcell.NewEventMouse(x, y, tcell.ButtonNone, 0), 0, - ) + )) self.waitTillIdle() } @@ -60,10 +60,10 @@ func (self *GuiDriver) Click(x, y int) { // learns to reload changed config files. Tests use it to exercise the live // config-reload path. func (self *GuiDriver) FocusIn() { - self.gui.g.ReplayedEvents.FocusEvents <- gocui.NewTcellFocusEventWrapper( + self.gui.g.ReplayFocusEvent(gocui.NewTcellFocusEventWrapper( tcell.NewEventFocus(true), 0, - ) + )) self.waitTillIdle() } From 0ce857c717b391172261cb2f6498a07dccf6c076 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Wed, 15 Jul 2026 14:56:27 +0200 Subject: [PATCH 3/3] Fix a deadlock between task.Done() and the integration test's idle wait MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Running the integration tests in a loop under the race detector eventually hung in demo/bisect. The goroutine dump shows the cycle: a background worker's task.Done() held the task manager's mutex while blocking on the unbuffered idle-listener channel send, and the test runner goroutine — the only reader of that channel — was itself blocked in NewTask on that same mutex, on its way to enqueueing a caption render (SetCaption -> Render -> OnUIThread). Neither side could proceed: the notification couldn't be delivered until the test goroutine got the mutex, and the mutex couldn't be released until the notification was delivered. The root problem is that the busy-to-idle notification is a blocking rendezvous performed while holding the mutex, so it needs the waiter's cooperation at a moment where the waiter may legitimately need the mutex first. Make the notification fire-and-forget instead: WaitUntilIdle waits on a condition variable and re-checks "is any task busy?" under the mutex, and the busy-to-idle transition broadcasts, which never blocks. Waiting is now level-triggered rather than edge-triggered, which is also more robust: a wait can no longer be satisfied by a stale idle transition produced by an unrelated background task, because the predicate is evaluated against the current state. This relies on the previous commit having made replayed input events carry their task from submission; without that, the wait could return in the window where an event is in flight but not yet picked up by the main loop. Co-Authored-By: Claude Fable 5 --- pkg/gocui/gui.go | 10 +++--- pkg/gocui/task_manager.go | 54 ++++++++++++++++++---------- pkg/gocui/task_manager_test.go | 66 ++++++++++++++++++++++++++++++++++ pkg/gui/gui_driver.go | 9 +++-- pkg/gui/test_mode.go | 8 ++--- 5 files changed, 112 insertions(+), 35 deletions(-) diff --git a/pkg/gocui/gui.go b/pkg/gocui/gui.go index e5588e262..9da5225c0 100644 --- a/pkg/gocui/gui.go +++ b/pkg/gocui/gui.go @@ -326,11 +326,11 @@ func (g *Gui) Busy() bool { return g.taskManager.hasBusyForegroundTaskExcept(g.currentTask) } -// An idle listener listens for when the program is idle. This is useful for -// integration tests which can wait for the program to be idle before taking -// the next step in the test. -func (g *Gui) AddIdleListener(c chan struct{}) { - g.taskManager.addIdleListener(c) +// WaitUntilIdle blocks until the program is idle (no busy tasks). This is +// useful for integration tests which want to wait for the program to finish +// processing before taking the next step in the test. +func (g *Gui) WaitUntilIdle() { + g.taskManager.WaitUntilIdle() } // Close finalizes the library. It should be called after a successful diff --git a/pkg/gocui/task_manager.go b/pkg/gocui/task_manager.go index 23ef0f77e..8d6daaa20 100644 --- a/pkg/gocui/task_manager.go +++ b/pkg/gocui/task_manager.go @@ -6,20 +6,23 @@ import "sync" // the main goroutine or a worker goroutine). Used by integration tests // to wait until the program is idle before progressing. type TaskManager struct { - // each of these listeners will be notified when the program goes from busy to idle - idleListeners []chan struct{} - tasks map[int]Task + tasks map[int]Task // auto-incrementing id for new tasks nextId int mutex sync.Mutex + // signalled whenever the program transitions from busy to idle; used by + // WaitUntilIdle + idleCond *sync.Cond } func newTaskManager() *TaskManager { - return &TaskManager{ - tasks: make(map[int]Task), - idleListeners: []chan struct{}{}, + self := &TaskManager{ + tasks: make(map[int]Task), } + self.idleCond = sync.NewCond(&self.mutex) + + return self } func (self *TaskManager) NewTask(background bool) *TaskImpl { @@ -58,8 +61,26 @@ func (self *TaskManager) hasBusyForegroundTaskExcept(ignore Task) bool { return false } -func (self *TaskManager) addIdleListener(c chan struct{}) { - self.idleListeners = append(self.idleListeners, c) +// WaitUntilIdle blocks until no task is busy. Integration tests use it to wait +// for the program to finish processing before taking the next step. +func (self *TaskManager) WaitUntilIdle() { + self.mutex.Lock() + defer self.mutex.Unlock() + + for self.hasBusyTask() { + self.idleCond.Wait() + } +} + +// caller must hold self.mutex +func (self *TaskManager) hasBusyTask() bool { + for _, task := range self.tasks { + if task.isBusy() { + return true + } + } + + return false } func (self *TaskManager) withMutex(f func()) { @@ -68,17 +89,12 @@ func (self *TaskManager) withMutex(f func()) { f() - // Check if all tasks are done - for _, task := range self.tasks { - if task.isBusy() { - return - } - } - - // If we get here, all tasks are done, so - // notify listeners that the program is idle - for _, listener := range self.idleListeners { - listener <- struct{}{} + // Wake up any goroutine blocked in WaitUntilIdle. This must not block on + // the waiter (we hold the mutex, and the waiter may itself be trying to + // acquire it, e.g. by creating a task, before it next waits) — which is + // exactly what Broadcast guarantees. + if !self.hasBusyTask() { + self.idleCond.Broadcast() } } diff --git a/pkg/gocui/task_manager_test.go b/pkg/gocui/task_manager_test.go index 7fe706d7a..b83b678ea 100644 --- a/pkg/gocui/task_manager_test.go +++ b/pkg/gocui/task_manager_test.go @@ -2,6 +2,7 @@ package gocui import ( "testing" + "time" "github.com/stretchr/testify/assert" ) @@ -61,3 +62,68 @@ func TestTaskManagerHasBusyForegroundTaskExcept(t *testing.T) { assert.False(t, tm.hasBusyForegroundTaskExcept(current)) }) } + +func TestTaskManagerWaitUntilIdle(t *testing.T) { + // returnsWithin reports whether f returns within the given duration. + returnsWithin := func(d time.Duration, f func()) bool { + done := make(chan struct{}) + go func() { + f() + close(done) + }() + select { + case <-done: + return true + case <-time.After(d): + return false + } + } + + t.Run("returns immediately when no task was ever created", func(t *testing.T) { + tm := newTaskManager() + assert.True(t, returnsWithin(time.Second, tm.WaitUntilIdle)) + }) + + t.Run("blocks while a task is busy", func(t *testing.T) { + tm := newTaskManager() + tm.NewTask(false) + assert.False(t, returnsWithin(50*time.Millisecond, tm.WaitUntilIdle)) + }) + + t.Run("wakes up when the last busy task completes", func(t *testing.T) { + tm := newTaskManager() + task := tm.NewTask(false) + go func() { + time.Sleep(10 * time.Millisecond) + task.Done() + }() + assert.True(t, returnsWithin(time.Second, tm.WaitUntilIdle)) + }) + + t.Run("a paused task counts as idle", func(t *testing.T) { + tm := newTaskManager() + task := tm.NewTask(false) + task.Pause() + assert.True(t, returnsWithin(time.Second, tm.WaitUntilIdle)) + }) + + t.Run("a task completing while nobody waits must not block", func(t *testing.T) { + // This is the deadlock case: the waiter (the integration-test runner) + // is between waits, and itself needs the task manager's mutex (it + // creates a task whenever it enqueues work) before it waits again. The + // idle notification must neither block the completing task while it + // holds the mutex, nor get lost. + tm := newTaskManager() + assert.True(t, returnsWithin(time.Second, func() { + // the program goes idle with nobody waiting... + tm.NewTask(true).Done() + + // ...and creating and completing more tasks afterwards must still + // be possible + task := tm.NewTask(false) + tm.NewTask(false).Done() + task.Done() + })) + assert.True(t, returnsWithin(time.Second, tm.WaitUntilIdle)) + }) +} diff --git a/pkg/gui/gui_driver.go b/pkg/gui/gui_driver.go index a54530791..31094b253 100644 --- a/pkg/gui/gui_driver.go +++ b/pkg/gui/gui_driver.go @@ -17,10 +17,9 @@ import ( // this gives our integration test a way of interacting with the gui for sending keypresses // and reading state. type GuiDriver struct { - gui *Gui - isIdleChan chan struct{} - toastChan chan string - headless bool + gui *Gui + toastChan chan string + headless bool } var _ integrationTypes.GuiDriver = &GuiDriver{} @@ -79,7 +78,7 @@ func (self *GuiDriver) PretendMergeOrRebaseStartedInLazygit() { // wait until lazygit is idle (i.e. all processing is done) before continuing func (self *GuiDriver) waitTillIdle() { - <-self.isIdleChan + self.gui.g.WaitUntilIdle() } func (self *GuiDriver) CheckAllToastsAcknowledged() { diff --git a/pkg/gui/test_mode.go b/pkg/gui/test_mode.go index 2ba381078..2d5958fbb 100644 --- a/pkg/gui/test_mode.go +++ b/pkg/gui/test_mode.go @@ -23,12 +23,8 @@ func (gui *Gui) handleTestMode() { } if test != nil { - isIdleChan := make(chan struct{}) - - gui.c.GocuiGui().AddIdleListener(isIdleChan) - waitUntilIdle := func() { - <-isIdleChan + gui.c.GocuiGui().WaitUntilIdle() } go func() { @@ -38,7 +34,7 @@ func (gui *Gui) handleTestMode() { gui.PopupHandler.(*popup.PopupHandler).SetToastFunc( func(message string, kind types.ToastKind) { toastChan <- message }) - test.Run(&GuiDriver{gui: gui, isIdleChan: isIdleChan, toastChan: toastChan, headless: Headless()}) + test.Run(&GuiDriver{gui: gui, toastChan: toastChan, headless: Headless()}) gui.g.Update(func(*gocui.Gui) error { return gocui.ErrQuit