From 805738034f727e246bebfc8f35e3adc721c6873a Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Fri, 3 Jul 2026 16:51:55 +0200 Subject: [PATCH] Remove refresh mutexes made redundant by bouncing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now that every refresh scope writes its model updates on the UI thread via onUIThreadUnlessRepoChanged, the per-scope mutexes that used to serialize concurrent worker-goroutine access are redundant: Model().Commits, .SubCommits, .Authors, the status view content, and .PullRequests/.PullRequestsMap are all now written only on the UI thread, and their readers already ran there. setSubCommits only existed to take the lock, so it's inlined to match refreshSubCommitsWithLimit, which writes Model().SubCommits directly. The worker phases still *read* some of these fields (the commit selection range, MergeRebasingCommits), but those reads race a concurrent refresh's bounced write regardless of the mutex — the write happens in the bounce, outside the locked region — so the mutex never protected them. That residual read race belongs to the broader -race effort, not to these locks. RefreshingBranchesMutex is deliberately kept. It is load-bearing for a reason unrelated to data races: at the INITIAL startup stage two refreshBranches run concurrently — an immediate one with an empty reflog (non-recency order) and an async one with the freshly-loaded reflog (recency order). The mutex serializes them so the recency write's bounce is enqueued last and wins. Without it the stale non-recency write can land last, reordering the branches list (caught by the recency-sort e2e tests: cherry_pick/*, branch/rebase_*). Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/gui/controllers/helpers/refresh_helper.go | 20 ------------------- .../controllers/helpers/sub_commits_helper.go | 9 +-------- pkg/gui/types/common.go | 13 ++++-------- 3 files changed, 5 insertions(+), 37 deletions(-) diff --git a/pkg/gui/controllers/helpers/refresh_helper.go b/pkg/gui/controllers/helpers/refresh_helper.go index 876ce0f30..75d5d2fd4 100644 --- a/pkg/gui/controllers/helpers/refresh_helper.go +++ b/pkg/gui/controllers/helpers/refresh_helper.go @@ -472,9 +472,6 @@ func (self *RefreshHelper) determineCheckedOutRef() models.Ref { } func (self *RefreshHelper) refreshCommitsWithLimit(commitSelection types.CommitSelectionBehavior) error { - self.c.Mutexes().LocalCommitsMutex.Lock() - defer self.c.Mutexes().LocalCommitsMutex.Unlock() - generation := self.c.State().GetRepoGeneration() var selectionRange *localCommitSelectionRange @@ -629,9 +626,6 @@ func (self *RefreshHelper) refreshSubCommitsWithLimit() error { return nil } - self.c.Mutexes().SubCommitsMutex.Lock() - defer self.c.Mutexes().SubCommitsMutex.Unlock() - generation := self.c.State().GetRepoGeneration() commits, err := self.c.Git().Loaders.CommitLoader.GetCommits( @@ -661,9 +655,6 @@ func (self *RefreshHelper) refreshSubCommitsWithLimit() error { } func (self *RefreshHelper) RefreshAuthors(commits []*models.Commit) { - self.c.Mutexes().AuthorsMutex.Lock() - defer self.c.Mutexes().AuthorsMutex.Unlock() - authors := self.c.Model().Authors for _, commit := range commits { if _, ok := authors[commit.AuthorEmail]; !ok { @@ -694,9 +685,6 @@ func (self *RefreshHelper) refreshCommitFilesContext() error { } func (self *RefreshHelper) refreshRebaseCommits() error { - self.c.Mutexes().LocalCommitsMutex.Lock() - defer self.c.Mutexes().LocalCommitsMutex.Unlock() - generation := self.c.State().GetRepoGeneration() updatedCommits, err := self.c.Git().Loaders.CommitLoader.MergeRebasingCommits(self.c.Model().HashPool, self.c.Model().Commits) @@ -795,9 +783,7 @@ func (self *RefreshHelper) refreshBranches(refreshWorktrees bool, keepBranchSele // Need to re-render the commits view because the visualization of local // branch heads might have changed - self.c.Mutexes().LocalCommitsMutex.Lock() self.c.Contexts().LocalCommits.HandleRender() - self.c.Mutexes().LocalCommitsMutex.Unlock() return nil }) @@ -1075,9 +1061,6 @@ func (self *RefreshHelper) refreshStashEntries() { // never call this on its own, it should only be called from within refreshCommits() func (self *RefreshHelper) refreshStatus() { - self.c.Mutexes().RefreshingStatusMutex.Lock() - defer self.c.Mutexes().RefreshingStatusMutex.Unlock() - generation := self.c.State().GetRepoGeneration() workingTreeState := self.c.Git().Status.WorkingTreeState() @@ -1147,9 +1130,6 @@ func (self *RefreshHelper) refreshView(context types.Context) { } func (self *RefreshHelper) refreshGithubPullRequests() { - self.c.Mutexes().RefreshingPullRequestsMutex.Lock() - defer self.c.Mutexes().RefreshingPullRequestsMutex.Unlock() - generation := self.c.State().GetRepoGeneration() clearPullRequests := func() { diff --git a/pkg/gui/controllers/helpers/sub_commits_helper.go b/pkg/gui/controllers/helpers/sub_commits_helper.go index fbf100e16..7bd928826 100644 --- a/pkg/gui/controllers/helpers/sub_commits_helper.go +++ b/pkg/gui/controllers/helpers/sub_commits_helper.go @@ -49,7 +49,7 @@ func (self *SubCommitsHelper) ViewSubCommits(opts ViewSubCommitsOpts) error { return err } - self.setSubCommits(commits) + self.c.Model().SubCommits = commits self.refreshHelper.RefreshAuthors(commits) subCommitsContext := self.c.Contexts().SubCommits @@ -71,10 +71,3 @@ func (self *SubCommitsHelper) ViewSubCommits(opts ViewSubCommitsOpts) error { self.c.Context().Push(self.c.Contexts().SubCommits, types.OnFocusOpts{}) return nil } - -func (self *SubCommitsHelper) setSubCommits(commits []*models.Commit) { - self.c.Mutexes().SubCommitsMutex.Lock() - defer self.c.Mutexes().SubCommitsMutex.Unlock() - - self.c.Model().SubCommits = commits -} diff --git a/pkg/gui/types/common.go b/pkg/gui/types/common.go index 2b7dc2312..35f38d21e 100644 --- a/pkg/gui/types/common.go +++ b/pkg/gui/types/common.go @@ -338,15 +338,10 @@ type Model struct { } type Mutexes struct { - RefreshingBranchesMutex deadlock.Mutex - RefreshingStatusMutex deadlock.Mutex - RefreshingPullRequestsMutex deadlock.Mutex - LocalCommitsMutex deadlock.Mutex - SubCommitsMutex deadlock.Mutex - AuthorsMutex deadlock.Mutex - SubprocessMutex deadlock.Mutex - PopupMutex deadlock.Mutex - PtyMutex deadlock.Mutex + RefreshingBranchesMutex deadlock.Mutex + SubprocessMutex deadlock.Mutex + PopupMutex deadlock.Mutex + PtyMutex deadlock.Mutex } // A long-running operation associated with an item. For example, we'll show