Don't block repo switching on slow network (#5829)

Fix two problems that would prevent switching repos or worktrees while a
background fetch was running, especially when the network is very slow
and the fetch takes long. See commit messages for details.

Labelling as ignore-for-release because it fixes a regression that was
introduced after the last release.

pkg/commands/git_commands/github.go

@@ -210,7 +210,10 @@ func (self *GitHubCommands) fetchRecentPRsAux(endpoint string, repoOwner string,
 	req.Header.Set("Authorization", "token "+token)
 	req.Header.Set("Content-Type", "application/json")
 
-	client := &http.Client{}
+	// Bound the request so that a dead or extremely slow network can't leave
+	// the pull-request refresh in flight for minutes. The data is auxiliary,
+	// so giving up and retrying on the next refresh beats waiting.
+	client := &http.Client{Timeout: 10 * time.Second}
 	resp, err := client.Do(req)
 	if err != nil {
 		return nil, err

pkg/gui/background.go

@@ -119,7 +119,7 @@ func (self *BackgroundRoutineMgr) startBackgroundFetch() {
 		if self.gui.UserConfig().Gui.ShowBottomLine || firstTimeOrRetriggered {
 			return self.gui.helpers.AppStatus.WithWaitingStatusImpl(self.gui.Tr.FetchingStatus, func(gocui.Task) error {
 				return self.backgroundFetch()
-			}, nil, true)
+			}, nil)
 		}
 
 		return self.backgroundFetch()

pkg/gui/controllers/helpers/app_status_helper.go

@@ -34,12 +34,7 @@ func (self *AppStatusHelper) Toast(message string, kind types.ToastKind) {
 
 	self.statusMgr().AddToastStatus(message, kind)
 
-	// Render the toast in the background: it's a transient notification, not
-	// lazygit driving an operation, so it must not count towards being busy —
-	// otherwise a toast (e.g. the "can't switch, operation in progress" one)
-	// would itself block a repo switch until it faded. A real operation showing
-	// a toast still keeps its own foreground task busy independently.
-	self.renderAppStatus(true)
+	self.renderAppStatus()
 }
 
 // A custom task for WithWaitingStatus calls; it wraps the original one and
@@ -66,14 +61,15 @@ func (self appStatusHelperTask) Continue() {
 // WithWaitingStatus wraps a function and shows a waiting status while the function is still executing
 func (self *AppStatusHelper) WithWaitingStatus(message string, f func(gocui.Task) error) {
 	self.c.OnWorker(func(task gocui.Task) error {
-		return self.WithWaitingStatusImpl(message, f, task, false)
+		return self.WithWaitingStatusImpl(message, f, task)
 	})
 }
 
-// background reports whether this waiting status belongs to a background routine
-// (the auto-fetch poller); when it does, the spinner it drives must not count
-// towards lazygit being busy, or it'd block repo switches while a fetch runs.
-func (self *AppStatusHelper) WithWaitingStatusImpl(message string, f func(gocui.Task) error, task gocui.Task, background bool) error {
+// WithWaitingStatusImpl is WithWaitingStatus for callers that already run on a
+// goroutine of their own (e.g. the auto-fetch poller) rather than wanting the
+// work dispatched to a worker. task is used to hide the status while the task
+// is paused; it may be nil for callers whose f ignores its task.
+func (self *AppStatusHelper) WithWaitingStatusImpl(message string, f func(gocui.Task) error, task gocui.Task) error {
 	// A waiting status means lazygit is driving a git operation itself (often
 	// one that internally runs a rebase and continues it). Pause the background
 	// routines for its duration so they don't refresh from an intermediate
@@ -81,7 +77,7 @@ func (self *AppStatusHelper) WithWaitingStatusImpl(message string, f func(gocui.
 	self.c.PauseBackgroundRefreshes(true)
 	defer self.c.PauseBackgroundRefreshes(false)
 
-	return self.statusMgr().WithWaitingStatus(message, func() { self.renderAppStatus(background) }, func(waitingStatusHandle *status.WaitingStatusHandle) error {
+	return self.statusMgr().WithWaitingStatus(message, self.renderAppStatus, func(waitingStatusHandle *status.WaitingStatusHandle) error {
 		return f(appStatusHelperTask{task, waitingStatusHandle})
 	})
 }
@@ -112,7 +108,7 @@ func (self *AppStatusHelper) WithWaitingStatusBlockingInput(message string, f fu
 			self.modeHelper.SetSuppressRebasingMode(false)
 			return self.c.GocuiGui().EndBlockingEvents()
 		})
-		return self.WithWaitingStatusImpl(message, f, task, false)
+		return self.WithWaitingStatusImpl(message, f, task)
 	})
 }
 
@@ -125,33 +121,36 @@ func (self *AppStatusHelper) GetStatusString() string {
 	return appStatus
 }
 
-func (self *AppStatusHelper) renderAppStatus(background bool) {
-	// A background waiting status (auto-fetch) must not count towards lazygit
-	// being busy, so its spinner worker and per-frame UI updates go through the
-	// background variants.
-	onWorker := self.c.OnWorker
-	onUIThread := self.c.OnUIThread
-	onUIThreadContentOnly := self.c.OnUIThreadContentOnly
-	if background {
-		onWorker = self.c.OnWorkerBackground
-		onUIThread = self.c.OnUIThreadBackground
-		onUIThreadContentOnly = self.c.OnUIThreadContentOnlyBackground
+// renderAppStatus ensures the render loop that keeps the app-status view up to
+// date is running. There is one loop for the whole status stack, no matter how
+// many statuses are showing: it draws whatever the top status currently is,
+// and exits after drawing a final empty frame once the last status is removed.
+//
+// The loop always runs as a background task, regardless of what kind of
+// operation owns a status: rendering runs no git commands, so it must never
+// count towards lazygit being busy — otherwise it would block repo switching
+// for as long as anything is showing (e.g. for the whole duration of a hung
+// background fetch, or of a toast fading). A foreground operation's busy-ness
+// is carried by its own worker task, not by the renderer.
+func (self *AppStatusHelper) renderAppStatus() {
+	if !self.statusMgr().ClaimRenderLoop() {
+		return
 	}
 
-	onWorker(func(_ gocui.Task) error {
+	self.c.OnWorkerBackground(func(_ gocui.Task) error {
 		ticker := time.NewTicker(time.Millisecond * time.Duration(self.c.UserConfig().Gui.Spinner.Rate))
 		defer ticker.Stop()
 		prevAppStatus := ""
 		for range ticker.C {
 			appStatus, color := self.statusMgr().GetStatusString(self.c.UserConfig())
 
-			update := onUIThreadContentOnly
+			update := self.c.OnUIThreadContentOnlyBackground
 			if utils.StringWidth(appStatus) != utils.StringWidth(prevAppStatus) {
 				// Need a full layout whenever the width of the status string changes. This can't
 				// happen during normal spinning because we validate that all spinner frames have
 				// the same width, so typically this will only be triggered at the beginning and end
 				// of a status, or if the status string changes midway for some reason.
-				update = onUIThread
+				update = self.c.OnUIThreadBackground
 			}
 			update(func() error {
 				self.c.Views().AppStatus.FgColor = color
@@ -160,7 +159,9 @@ func (self *AppStatusHelper) renderAppStatus(background bool) {
 			})
 			prevAppStatus = appStatus
 
-			if appStatus == "" {
+			// Checked after rendering, so that the frame which clears the view
+			// has already been drawn when we exit.
+			if self.statusMgr().ReleaseRenderLoopIfEmpty() {
 				break
 			}
 		}

pkg/gui/controllers/helpers/refresh_helper.go

@@ -365,7 +365,17 @@ func (self *RefreshHelper) performRefresh(options types.RefreshOptions, calledFr
 	}
 
 	if scopeSet.Includes(types.PULL_REQUESTS) {
-		self.onWorker(env.background, func(gocui.Task) error {
+		// Fetching pull requests talks to the GitHub API over the network; on
+		// a bad connection that request can stall for a long time. It runs no
+		// git commands against the repo, and its model writes are guarded by
+		// the repo generation (a repo switch mid-fetch simply drops the
+		// result), so it is safe to run as a background task even when the
+		// enclosing refresh is a foreground one — a foreground task would
+		// block repo switching for as long as the request takes. The env copy
+		// makes the downstream UI-thread bounces background as well.
+		prEnv := env
+		prEnv.background = true
+		self.c.OnWorkerBackground(func(gocui.Task) error {
 			branchesAndRemotesWg.Wait()
 
 			t := time.Now()
@@ -373,7 +383,7 @@ func (self *RefreshHelper) performRefresh(options types.RefreshOptions, calledFr
 			// Model().Branches/Remotes: those writes are bounced onto the
 			// UI thread and may not have landed on this worker yet. The
 			// wait above orders us after both loads have stashed theirs.
-			self.refreshGithubPullRequests(loadedBranches, loadedRemotes, env)
+			self.refreshGithubPullRequests(loadedBranches, loadedRemotes, prEnv)
 			self.c.Log.Infof("refreshed pull requests in %s", time.Since(t))
 			return nil
 		})

pkg/gui/status/status_manager.go

@@ -17,6 +17,11 @@ type StatusManager struct {
 	statuses []appStatus
 	nextId   int
 	mutex    deadlock.Mutex
+
+	// Whether a render loop is currently drawing the statuses. Guarded by
+	// mutex, so that claiming and releasing the loop stay atomic with the
+	// changes to statuses; see ClaimRenderLoop and ReleaseRenderLoopIfEmpty.
+	renderLoopRunning bool
 }
 
 // Can be used to manipulate a waiting status while it is running (e.g. pause
@@ -90,6 +95,39 @@ func (self *StatusManager) HasStatus() bool {
 	return len(self.statuses) > 0
 }
 
+// ClaimRenderLoop is called by whoever just added a status; it reports whether
+// they must start the render loop. When it returns false, a loop is already
+// running and will pick the new status up on its next tick.
+func (self *StatusManager) ClaimRenderLoop() bool {
+	self.mutex.Lock()
+	defer self.mutex.Unlock()
+
+	if self.renderLoopRunning {
+		return false
+	}
+
+	self.renderLoopRunning = true
+	return true
+}
+
+// ReleaseRenderLoopIfEmpty is called by the render loop after each frame it
+// draws; a true result releases the loop's claim and tells it to exit, because
+// there are no statuses left to draw. The emptiness check and the release are
+// atomic with respect to ClaimRenderLoop, so a status added around this moment
+// either sees the still-running loop or starts a fresh one — it can't end up
+// unrendered.
+func (self *StatusManager) ReleaseRenderLoopIfEmpty() bool {
+	self.mutex.Lock()
+	defer self.mutex.Unlock()
+
+	if len(self.statuses) > 0 {
+		return false
+	}
+
+	self.renderLoopRunning = false
+	return true
+}
+
 func (self *StatusManager) addStatus(message string, statusType string, kind types.ToastKind) int {
 	self.mutex.Lock()
 	defer self.mutex.Unlock()