From e53c72be52277509957070a27a7051719ed685c2 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Mon, 11 May 2026 17:46:57 +0200 Subject: [PATCH 1/3] Make ViewBufferManager.NewTask respect call order NewTask was incrementing newTaskID and reading taskID inside the spawned goroutine, so for two NewTask calls in quick succession the assignment was determined by goroutine scheduling order rather than call order. When the goroutines reordered, the first NewTask call could end up with the higher taskID and "win" the staleness check, superseding the second call's task even though the caller intended the second to be the latest. Worse, the staleness check ran after onNewKey, so a goroutine destined to bail as stale would still reset the view buffer first, potentially wiping the winning task's already-written output. Take newTaskID++ synchronously in NewTask so taskIDs follow call order, and move the first staleness check ahead of onNewKey so a stale task doesn't side-effect the view before exiting. Co-Authored-By: Claude Opus 4.7 (1M context) --- pkg/tasks/tasks.go | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index f534d01b4..7fadbb451 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -403,12 +403,29 @@ func (self *ViewBufferManager) NewTask(f func(TaskOpts) error, key string) error }) } + // Assign the taskID synchronously so it reflects NewTask call order + // rather than the order in which the spawned goroutines happen to be + // scheduled. Otherwise two NewTask calls in quick succession can have + // their goroutines race, with the later-called task ending up with the + // lower taskID and losing the staleness check below. + self.taskIDMutex.Lock() + self.newTaskID++ + taskID := self.newTaskID + self.taskIDMutex.Unlock() + go utils.Safe(func() { defer completeGocuiTask() self.taskIDMutex.Lock() - self.newTaskID++ - taskID := self.newTaskID + + // Bail out before touching shared view state if a newer task has + // already been queued: if we ran onNewKey here we'd reset the view + // for a task that's about to exit, potentially wiping output the + // winning task has already written. + if taskID < self.newTaskID { + self.taskIDMutex.Unlock() + return + } if self.GetTaskKey() != key && self.onNewKey != nil { self.onNewKey() @@ -419,6 +436,8 @@ func (self *ViewBufferManager) NewTask(f func(TaskOpts) error, key string) error self.waitingMutex.Lock() + // Re-check staleness after acquiring waitingMutex: a newer task + // may have arrived while we were blocked here. self.taskIDMutex.Lock() if taskID < self.newTaskID { self.waitingMutex.Unlock() From badf398a94fbd4fe39eb569ce95b8543c2289c8e Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Mon, 11 May 2026 20:42:05 +0200 Subject: [PATCH 2/3] Run moveMainContextPairToTop before queueing main-view tasks Copy the outgoing view's content into the target view (the flicker- prevention step) before queuing the render task, rather than after. The task writes the fresh content from a worker goroutine, so with the old order the worker write races the UI-thread copy, and the copy can land last and clobber the fresh content with stale output. This is only needed while view writes happen concurrently. Once view writes are serialized on the UI thread and the view write-mutex goes away, the synchronous copy always precedes the FIFO-queued write regardless of order, so the reorder becomes unnecessary. No code comment is added for it, since that comment would be obsoleted by that work and likely left behind. --- pkg/gui/main_panels.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/gui/main_panels.go b/pkg/gui/main_panels.go index 82f4fcac0..03b7469d2 100644 --- a/pkg/gui/main_panels.go +++ b/pkg/gui/main_panels.go @@ -117,6 +117,8 @@ func (gui *Gui) refreshMainViews(opts types.RefreshMainOpts) { } } + gui.moveMainContextPairToTop(opts.Pair) + if opts.Main != nil { gui.RefreshMainView(opts.Main, opts.Pair.Main) } @@ -127,8 +129,6 @@ func (gui *Gui) refreshMainViews(opts types.RefreshMainOpts) { opts.Pair.Secondary.GetView().Clear() } - gui.moveMainContextPairToTop(opts.Pair) - gui.splitMainPanel(opts.Secondary != nil) } From 873804a37b24e3c23c3c5f3374dd7965a6de4b98 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 2 Jul 2026 15:27:31 +0200 Subject: [PATCH 3/3] Make Gui.Update a synchronous FIFO enqueue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update spawned a goroutine per call that then sent on the user-events channel, so multiple Update calls from the same goroutine could be reordered by the scheduler — the doc comment even admitted "the order in which the user events will be handled is not guaranteed." That non-determinism is a latent source of flaky rendering: code that queues a model update and then a render in source order could see them run in the opposite order. Send on the channel directly instead, so same-goroutine calls arrive in source order. The send is non-blocking and panics on a full channel rather than blocking (a blocked send from the UI goroutine would deadlock against itself) or silently reordering; the buffer is sized generously so this is unreachable in normal use. UpdateAsync is now identical to Update and unused, so it's removed along with the shared updateAsyncAux helper. --- pkg/gocui/gui.go | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/pkg/gocui/gui.go b/pkg/gocui/gui.go index 558b9d619..ee1995911 100644 --- a/pkg/gocui/gui.go +++ b/pkg/gocui/gui.go @@ -238,7 +238,12 @@ func NewGui(opts NewGuiOpts) (*Gui, error) { g.stop = make(chan struct{}) g.gEvents = make(chan GocuiEvent, 20) - g.userEvents = make(chan userEvent, 20) + // Update does a non-blocking send and panics on a full channel rather than + // blocking (which would deadlock the UI goroutine against itself) or + // silently reordering. The buffer is sized well above the peak occupancy we + // see in practice, so the panic stays unreachable in normal use; if it ever + // fires, that's a real anomaly to investigate, not a cue to grow the buffer. + g.userEvents = make(chan userEvent, 256) g.taskManager = newTaskManager() if opts.PlayRecording { @@ -613,28 +618,23 @@ type userEvent struct { contentOnly bool } -// Update executes the passed function. This method can be called safely from a -// goroutine in order to update the GUI. It is important to note that the -// passed function won't be executed immediately, instead it will be added to -// the user events queue. Given that Update spawns a goroutine, the order in -// which the user events will be handled is not guaranteed. +// Update enqueues f on the user-events channel for the UI loop to run on its +// next iteration. Multiple Update calls from the same goroutine arrive in +// source order via the channel's FIFO. The send is non-blocking — if the +// channel is full we panic rather than block or silently reorder, since a +// blocked send from the UI goroutine would deadlock against itself and +// silently switching to inline execution would break the ordering guarantee +// callers rely on. The buffer is sized generously enough that this should +// never fire in practice; if it does, that's a signal to investigate, not +// to grow the buffer reflexively. func (g *Gui) Update(f func(*Gui) error) { task := g.NewTask() - go g.updateAsyncAux(f, task) -} - -// UpdateAsync is a version of Update that does not spawn a go routine, it can -// be a bit more efficient in cases where Update is called many times like when -// tailing a file. In general you should use Update() -func (g *Gui) UpdateAsync(f func(*Gui) error) { - task := g.NewTask() - - g.updateAsyncAux(f, task) -} - -func (g *Gui) updateAsyncAux(f func(*Gui) error, task Task) { - g.userEvents <- userEvent{f: f, task: task} + select { + case g.userEvents <- userEvent{f: f, task: task}: + default: + panic("gocui: userEvents channel full; refusing to block or reorder") + } } // Like Update, but signals that the callback only modifies content.