Synchronize async view rendering

pkg/commands/patch/patch_builder.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/jesseduffield/generics/maps"
 	"github.com/samber/lo"
+	"github.com/sasha-s/go-deadlock"
 	"github.com/sirupsen/logrus"
 )
 
@@ -50,6 +51,13 @@ type PatchBuilder struct {
 	fileInfoMap map[string]*fileInfo
 	Log         *logrus.Entry
 
+	// mutex guards the fields that a git worker can mutate (via Reset, at the
+	// end of a patch-consuming operation) while the UI thread reads them to
+	// render — chiefly To and the fileInfoMap pointer. The map's *entries* are
+	// only ever touched on the UI thread, so we only hold the lock long enough
+	// to read or swap the fields, never across the git I/O in getFileInfo.
+	mutex deadlock.Mutex
+
 	// loadFileDiff loads the diff of a file, for a given to (typically a commit hash)
 	loadFileDiff loadFileDiffFunc
 }
@@ -62,17 +70,31 @@ func NewPatchBuilder(log *logrus.Entry, loadFileDiff loadFileDiffFunc) *PatchBui
 }
 
 func (p *PatchBuilder) Start(from, to string, reverse bool, canRebase bool) {
+	p.mutex.Lock()
+	defer p.mutex.Unlock()
+
 	p.To = to
 	p.From = from
 	p.reverse = reverse
 	p.CanRebase = canRebase
 	p.fileInfoMap = map[string]*fileInfo{}
 }
 
+// snapshotFileInfoMap returns the current fileInfoMap under the lock. The map's
+// entries are only mutated on the UI thread, so callers can read the returned
+// map without holding the lock; the lock only serializes the pointer swap that
+// Reset/Start do (potentially from a git worker) against these reads.
+func (p *PatchBuilder) snapshotFileInfoMap() map[string]*fileInfo {
+	p.mutex.Lock()
+	defer p.mutex.Unlock()
+
+	return p.fileInfoMap
+}
+
 func (p *PatchBuilder) PatchToApply(reverse bool, turnAddedFilesIntoDiffAgainstEmptyFile bool) string {
 	var patch strings.Builder
 
-	for filename, info := range p.fileInfoMap {
+	for filename, info := range p.snapshotFileInfoMap() {
 		if info.mode == UNSELECTED {
 			continue
 		}
@@ -130,12 +152,17 @@ func (p *PatchBuilder) RemoveFile(filename string, previousPath string) error {
 }
 
 func (p *PatchBuilder) getFileInfo(filename string, previousPath string) (*fileInfo, error) {
-	info, ok := p.fileInfoMap[filename]
+	p.mutex.Lock()
+	fileInfoMap := p.fileInfoMap
+	from, to, reverse := p.From, p.To, p.reverse
+	p.mutex.Unlock()
+
+	info, ok := fileInfoMap[filename]
 	if ok {
 		return info, nil
 	}
 
-	diff, err := p.loadFileDiff(p.From, p.To, p.reverse, filename, previousPath, true)
+	diff, err := p.loadFileDiff(from, to, reverse, filename, previousPath, true)
 	if err != nil {
 		return nil, err
 	}
@@ -145,7 +172,7 @@ func (p *PatchBuilder) getFileInfo(filename string, previousPath string) (*fileI
 		previousPath: previousPath,
 	}
 
-	p.fileInfoMap[filename] = info
+	fileInfoMap[filename] = info
 
 	return info, nil
 }
@@ -220,14 +247,16 @@ func (p *PatchBuilder) RenderPatchForFile(opts RenderPatchForFileOpts) string {
 }
 
 func (p *PatchBuilder) renderEachFilePatch(plain bool) []string {
+	fileInfoMap := p.snapshotFileInfoMap()
+
 	// sort files by name then iterate through and render each patch
-	filenames := maps.Keys(p.fileInfoMap)
+	filenames := maps.Keys(fileInfoMap)
 
 	sort.Strings(filenames)
 	patches := lo.Map(filenames, func(filename string, _ int) string {
 		return p.RenderPatchForFile(RenderPatchForFileOpts{
 			Filename:                               filename,
-			PreviousPath:                           p.fileInfoMap[filename].previousPath,
+			PreviousPath:                           fileInfoMap[filename].previousPath,
 			Plain:                                  plain,
 			Reverse:                                false,
 			TurnAddedFilesIntoDiffAgainstEmptyFile: true,
@@ -245,11 +274,16 @@ func (p *PatchBuilder) RenderAggregatedPatch(plain bool) string {
 }
 
 func (p *PatchBuilder) GetFileStatus(filename string, parent string) PatchStatus {
-	if parent != p.To {
+	p.mutex.Lock()
+	to := p.To
+	fileInfoMap := p.fileInfoMap
+	p.mutex.Unlock()
+
+	if parent != to {
 		return UNSELECTED
 	}
 
-	info, ok := p.fileInfoMap[filename]
+	info, ok := fileInfoMap[filename]
 	if !ok {
 		return UNSELECTED
 	}
@@ -267,16 +301,22 @@ func (p *PatchBuilder) GetFileIncLineIndices(filename string, previousPath strin
 
 // clears the patch
 func (p *PatchBuilder) Reset() {
+	p.mutex.Lock()
+	defer p.mutex.Unlock()
+
 	p.To = ""
 	p.fileInfoMap = map[string]*fileInfo{}
 }
 
 func (p *PatchBuilder) Active() bool {
+	p.mutex.Lock()
+	defer p.mutex.Unlock()
+
 	return p.To != ""
 }
 
 func (p *PatchBuilder) IsEmpty() bool {
-	for _, fileInfo := range p.fileInfoMap {
+	for _, fileInfo := range p.snapshotFileInfoMap() {
 		if fileInfo.mode == WHOLE || (fileInfo.mode == PART && len(fileInfo.includedLineIndices) > 0) {
 			return false
 		}
@@ -287,9 +327,12 @@ func (p *PatchBuilder) IsEmpty() bool {
 
 // if any of these things change we'll need to reset and start a new patch
 func (p *PatchBuilder) NewPatchRequired(from string, to string, reverse bool) bool {
+	p.mutex.Lock()
+	defer p.mutex.Unlock()
+
 	return from != p.From || to != p.To || reverse != p.reverse
 }
 
 func (p *PatchBuilder) AllFilesInPatch() []string {
-	return lo.Keys(p.fileInfoMap)
+	return lo.Keys(p.snapshotFileInfoMap())
 }

pkg/gocui/escape.go

@@ -34,6 +34,14 @@ type escapeInterpreter struct {
 	// modelled — we don't track the col argument of CUPs, and most
 	// pager-style emitters use col 1 anyway.
 	screenRow, screenCol int
+
+	// The screen width that soft-wraps are counted against (see
+	// notifyCellsWritten). It's a snapshot of the view's InnerWidth taken on
+	// the UI thread (in NewView, and refreshed per render via
+	// View.SetContentWidth), rather than read live from the view's dimensions:
+	// a view's output is written from a task goroutine, and reading the live
+	// dimensions there would race the UI thread updating them during layout.
+	screenColMax int
 }
 
 type (
@@ -175,8 +183,8 @@ func (ei *escapeInterpreter) notifyColumnReset() {
 // columns; if that crosses the right edge of a `screenColMax`-wide pty
 // screen, the corresponding number of soft-wraps are added to screenRow
 // so subsequent CUPs land on the right line.
-func (ei *escapeInterpreter) notifyCellsWritten(width, screenColMax int) {
-	if screenColMax <= 0 {
+func (ei *escapeInterpreter) notifyCellsWritten(width int) {
+	if ei.screenColMax <= 0 {
 		return
 	}
 	// One column at a time: matches ConPTY's "pending wrap" semantics
@@ -185,7 +193,7 @@ func (ei *escapeInterpreter) notifyCellsWritten(width, screenColMax int) {
 	// columns rather than doing the math in one shot so wide cells on a
 	// row boundary still wrap cleanly.
 	for range width {
-		if ei.screenCol > screenColMax {
+		if ei.screenCol > ei.screenColMax {
 			ei.screenRow++
 			ei.screenCol = 1
 		}

pkg/gocui/gui.go

@@ -148,6 +148,10 @@ type Gui struct {
 	maxX, maxY               int
 	outputMode               OutputMode
 	stop                     chan struct{}
+	// loopExited is closed when MainLoop returns, so callers (e.g. the
+	// integration-test harness) can wait for the event loop to actually finish
+	// rather than polling or sleeping a fixed interval.
+	loopExited chan struct{}
 
 	// BgColor and FgColor allow to configure the background and foreground
 	// colors of the GUI.
@@ -260,6 +264,7 @@ func NewGui(opts NewGuiOpts) (*Gui, error) {
 	g.outputMode = opts.OutputMode
 
 	g.stop = make(chan struct{})
+	g.loopExited = make(chan struct{})
 
 	g.gEvents = make(chan GocuiEvent, 20)
 	g.userEvents = newUserEventQueue()
@@ -288,6 +293,12 @@ func NewGui(opts NewGuiOpts) (*Gui, error) {
 
 	g.playRecording = opts.PlayRecording
 
+	// Record the UI thread here, at construction. This assumes NewGui is called
+	// on the same goroutine that will run MainLoop, which holds for all our
+	// callers -- and it means IsUIThread is already correct for the UI work that
+	// runs during startup, before we reach MainLoop.
+	g.uiThreadID.Store(goid.Get())
+
 	return g, nil
 }
 
@@ -348,6 +359,11 @@ func (g *Gui) Close() {
 	Screen.Fini()
 }
 
+// LoopExited returns a channel that is closed once MainLoop has returned.
+func (g *Gui) LoopExited() <-chan struct{} {
+	return g.loopExited
+}
+
 // Size returns the terminal's size.
 func (g *Gui) Size() (x, y int) {
 	return g.maxX, g.maxY
@@ -385,7 +401,7 @@ func (g *Gui) SetView(name string, x0, y0, x1, y1 int, overlaps byte) (*View, er
 		v.y1 = y1
 
 		if sizeChanged {
-			v.clearViewLines()
+			v.ClearViewLines()
 
 			if v.Editable {
 				cursorX, cursorY := v.TextArea.GetCursorXY()
@@ -965,7 +981,7 @@ func (g *Gui) SetManagerFunc(manager func(*Gui) error) {
 // MainLoop runs the main loop until an error is returned. A successful
 // finish should return ErrQuit.
 func (g *Gui) MainLoop() error {
-	g.uiThreadID.Store(goid.Get())
+	defer close(g.loopExited)
 
 	go func() {
 		for {
@@ -1461,7 +1477,7 @@ func (g *Gui) flush() error {
 	// if GUI's size has changed, we need to redraw all views
 	if maxX != g.maxX || maxY != g.maxY {
 		for _, v := range g.views {
-			v.clearViewLines()
+			v.ClearViewLines()
 		}
 	}
 	g.maxX, g.maxY = maxX, maxY
@@ -1500,7 +1516,7 @@ func viewsToRedrawContentOnly(views []*View) []*View {
 	redrawIndexes := set.New[int]()
 
 	for i, v := range views {
-		if !v.tainted && !redrawIndexes.Includes(i) {
+		if !v.IsTainted() && !redrawIndexes.Includes(i) {
 			continue
 		}
 

pkg/gocui/view.go

@@ -7,6 +7,7 @@ package gocui
 import (
 	"fmt"
 	"io"
+	"slices"
 	"strings"
 	"sync"
 	"unicode"
@@ -214,6 +215,16 @@ func (v *View) clearViewLines() {
 	v.clearHover()
 }
 
+// ClearViewLines is clearViewLines guarded by writeMutex. It's for callers on
+// the UI thread (the layout pass) that touch a view whose content a task
+// goroutine may be writing concurrently: viewLines/tainted/hover are all
+// buffer state that writeMutex protects.
+func (v *View) ClearViewLines() {
+	v.writeMutex.Lock()
+	defer v.writeMutex.Unlock()
+	v.clearViewLines()
+}
+
 type searcher struct {
 	searchString       string
 	searchPositions    []SearchPosition
@@ -536,9 +547,20 @@ func NewView(name string, x0, y0, x1, y1 int, mode OutputMode) *View {
 	v.SelFgColor, v.SelBgColor = ColorDefault, ColorDefault
 	v.InactiveViewSelBgColor = ColorDefault
 	v.TitleColor, v.FrameColor = ColorDefault, ColorDefault
+	v.ei.screenColMax = v.InnerWidth()
 	return v
 }
 
+// SetContentWidth tells the view the screen width that content written to it
+// should count soft-wraps against (see escapeInterpreter.notifyCellsWritten).
+// Callers pass the view's InnerWidth; it's a separate call, made on the UI
+// thread when a render starts, so that the task goroutine that streams the
+// content can consult this snapshot instead of reading the view's live
+// dimensions (which the UI thread mutates during layout).
+func (v *View) SetContentWidth(width int) {
+	v.ei.screenColMax = width
+}
+
 // Dimensions returns the dimensions of the View
 func (v *View) Dimensions() (int, int, int, int) {
 	return v.x0, v.y0, v.x1, v.y1
@@ -907,7 +929,7 @@ func (v *View) write(p []byte) {
 				for _, c := range cells {
 					totalWidth += c.width
 				}
-				v.ei.notifyCellsWritten(totalWidth, v.InnerWidth())
+				v.ei.notifyCellsWritten(totalWidth)
 			}
 		}
 	}
@@ -1125,10 +1147,25 @@ func (v *View) CopyContent(from *View) {
 	v.writeMutex.Lock()
 	defer v.writeMutex.Unlock()
 
+	// A background task may be streaming output into the source view's buffer
+	// via Write, so read it under its own lock. The source is always a
+	// different view than the destination (see the sole caller,
+	// moveMainContextToTop), and no other code holds two view write locks at
+	// once, so this can't deadlock.
+	from.writeMutex.Lock()
+	defer from.writeMutex.Unlock()
+
 	v.clear()
 
-	v.lines = from.lines
-	v.viewLines = from.viewLines
+	// Clone the row slices rather than sharing them: the source view stays
+	// live (its streaming task keeps appending rows, and refreshViewLinesIfNeeded
+	// fills each row's wrapping cache in place via &lines[i]), so sharing the
+	// backing arrays would race those writes against this view's own rendering.
+	// This is a shallow clone -- the per-row cell data is immutable once written
+	// and stays shared, so the cost is proportional to the number of rows, not
+	// their contents.
+	v.lines = slices.Clone(from.lines)
+	v.viewLines = slices.Clone(from.viewLines)
 	v.ox = from.ox
 	v.oy = from.oy
 	v.cx = from.cx
@@ -1276,6 +1313,8 @@ func (v *View) updateSearchPositions() {
 
 // IsTainted tells us if the view is tainted
 func (v *View) IsTainted() bool {
+	v.writeMutex.Lock()
+	defer v.writeMutex.Unlock()
 	return v.tainted
 }
 
@@ -1524,6 +1563,9 @@ func (v *View) BufferLines() []string {
 // Buffer returns a string with the contents of the view's internal
 // buffer.
 func (v *View) Buffer() string {
+	v.writeMutex.Lock()
+	defer v.writeMutex.Unlock()
+
 	return linesToString(v.lines)
 }
 

pkg/gui/context/suggestions_context.go

@@ -81,10 +81,17 @@ func (self *SuggestionsContext) SetSuggestions(suggestions []*types.Suggestion)
 }
 
 func (self *SuggestionsContext) RefreshSuggestions() {
+	// Capture the suggestions function and the prompt input here, on the UI
+	// thread, rather than inside the worker below: the main thread rewrites both
+	// (State.FindSuggestions and the prompt's TextArea) when it (re)creates a
+	// prompt panel, so reading them from the worker races those writes. It's
+	// also more correct -- we search for the input as it was when dispatched,
+	// which is what this request's AsyncHandler id corresponds to.
+	findSuggestionsFn := self.State.FindSuggestions
+	promptInput := self.c.GetPromptInput()
 	self.State.AsyncHandler.Do(func() func() {
-		findSuggestionsFn := self.State.FindSuggestions
 		if findSuggestionsFn != nil {
-			suggestions := findSuggestionsFn(self.c.GetPromptInput())
+			suggestions := findSuggestionsFn(promptInput)
 			return func() { self.SetSuggestions(suggestions) }
 		}
 		return func() {}