Skip to content

Commit 11ffe47

Browse files
committed
ui: fix data race on spinner Suffix between label updates and render goroutine
The spinner library (briandowns/spinner) reads s.Prefix and s.Suffix inside s.mu on every tick. renderProgress was writing u.spinner.Suffix directly from the calling goroutine without holding s.mu — a genuine data race. On amd64 a string is pointer+length (two 64-bit words), so a concurrent write can produce a torn read where pointer and length are from different updates, yielding garbage output or the wrong label text for one frame. go test -race confirms the race is now gone. Fix: add pendingSuffix to spinnerWriter. renderProgress writes it under sw.mu. A PreUpdate callback on the spinner (called while s.mu is held, before s.Suffix is read for the frame) applies pendingSuffix to s.Suffix atomically. Lock order is s.mu → sw.mu, consistent with how the spinner goroutine already calls sw.Write under s.mu. This eliminates the 'Resolving actions' / 'Planning pins' label text flickering at phase transitions: the label now only changes at tick boundaries, never mid-frame, and never races with the render goroutine.
1 parent efecf4b commit 11ffe47

1 file changed

Lines changed: 38 additions & 4 deletions

File tree

internal/ui/ui.go

Lines changed: 38 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -364,6 +364,15 @@ type spinnerWriter struct {
364364
// deferredHints mirrors deferredWrites for hint state so concurrent
365365
// stall-watcher updates aren't clobbered by printLine's restore.
366366
deferredHints map[int]string
367+
368+
// pendingSuffix holds the label text to apply to the spinner on the
369+
// next PreUpdate callback. PreUpdate fires inside s.mu (the spinner's
370+
// internal lock) so the s.Suffix assignment is race-free with the
371+
// spinner goroutine's concurrent read. renderProgress writes here
372+
// under sw.mu instead of writing s.Suffix directly; the lock order
373+
// s.mu → sw.mu is consistent with how Write is called from the
374+
// spinner goroutine.
375+
pendingSuffix string
367376
}
368377

369378
// workerSpinFrames is the rotating glyph shown next to each ACTIVE worker row
@@ -1054,6 +1063,16 @@ func (u *UI) StartProgress(label string) {
10541063
opts = append(opts, spinner.WithColor("fgCyan"))
10551064
}
10561065
sp := spinner.New(spinner.CharSets[11], 120*time.Millisecond, opts...)
1066+
// PreUpdate fires inside the spinner's internal lock (s.mu) immediately
1067+
// before Suffix is read for the frame. Applying pendingSuffix here
1068+
// makes label updates race-free: renderProgress writes pendingSuffix
1069+
// under sw.mu; PreUpdate reads it under s.mu→sw.mu, consistent with
1070+
// how Write is called from the same goroutine.
1071+
sp.PreUpdate = func(s *spinner.Spinner) {
1072+
sw.mu.Lock()
1073+
s.Suffix = sw.pendingSuffix
1074+
sw.mu.Unlock()
1075+
}
10571076
u.spinner = sp
10581077
u.progLabel = label
10591078
u.progDetail = ""
@@ -1308,11 +1327,26 @@ func (u *UI) renderProgress() {
13081327
suffix = label
13091328
}
13101329

1311-
u.spinner.Prefix = ""
1312-
if suffix != "" {
1313-
u.spinner.Suffix = " " + suffix
1330+
// Store the suffix in pendingSuffix; PreUpdate applies it to s.Suffix
1331+
// under the spinner's internal lock, eliminating the data race between
1332+
// this goroutine's write and the spinner goroutine's concurrent read.
1333+
if u.spinWriter != nil {
1334+
u.spinWriter.mu.Lock()
1335+
if suffix != "" {
1336+
u.spinWriter.pendingSuffix = " " + suffix
1337+
} else {
1338+
u.spinWriter.pendingSuffix = ""
1339+
}
1340+
u.spinWriter.mu.Unlock()
13141341
} else {
1315-
u.spinner.Suffix = ""
1342+
// spinWriter not yet set (shouldn't happen after StartProgress,
1343+
// but be defensive).
1344+
u.spinner.Prefix = ""
1345+
if suffix != "" {
1346+
u.spinner.Suffix = " " + suffix
1347+
} else {
1348+
u.spinner.Suffix = ""
1349+
}
13161350
}
13171351
}
13181352

0 commit comments

Comments
 (0)