Skip to content

Commit 88371d6

Browse files
authored
Fix out-of-bounds panic in progress printer (#378)
* Fix out-of-bounds panic in progress printer We ran into an out-of-bounds panic when we had more tasks reporting as "currently running" than the number of status bars we had. That was due to a lack of defensive checks in the face of a possible race condition. The race condition leads the `PrintStatuses` method seeing more "currently running" tasks than there can technically be. The race condition is between the `PrintStatus` method that prints the slices of pointers to `*TaskStatus` and the tasks themselves updating the status non-atomatically. At least that's what it seems like. I'm not 100% sure about the cause yet, but I could reproduce the issue by simply injecting more statuses at the top of `PrintStatuses`. The code here fixes it. * Add changelog entry
1 parent b707fa5 commit 88371d6

File tree

2 files changed

+16
-4
lines changed

2 files changed

+16
-4
lines changed

CHANGELOG.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,14 +17,15 @@ All notable changes to `src-cli` are documented in this file.
1717

1818
### Fixed
1919

20+
- Fixed a bug that could cause `src campaign [apply|preview]` to crash in rare circumstances when executing a campaign spec due to a bug in the logic for the progress bar. [#378](https://github.com/sourcegraph/src-cli/pull/378)
21+
2022
### Removed
2123

2224
## 3.21.9
2325

2426
### Added
2527

2628
- Commands for campaigns no longer require the `-namespace` parameter. If omitted, campaigns will use the currently authenticated user as the namespace. [#372](https://github.com/sourcegraph/src-cli/pull/372)
27-
2829
- `src campaign [apply|preview]` now caches the result of running steps in a repository even if they didn't produce changes.
2930

3031
## 3.21.8

cmd/src/campaign_progress_printer.go

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ type campaignProgressPrinter struct {
3737
statusBarRepo map[int]string
3838
}
3939

40-
func (p *campaignProgressPrinter) initProgressBar(statuses []*campaigns.TaskStatus) {
40+
func (p *campaignProgressPrinter) initProgressBar(statuses []*campaigns.TaskStatus) int {
4141
numStatusBars := p.numParallelism
4242
if len(statuses) < numStatusBars {
4343
numStatusBars = len(statuses)
@@ -52,6 +52,8 @@ func (p *campaignProgressPrinter) initProgressBar(statuses []*campaigns.TaskStat
5252
Label: fmt.Sprintf("Executing steps in %d repositories", len(statuses)),
5353
Max: float64(len(statuses)),
5454
}}, statusBars, nil)
55+
56+
return numStatusBars
5557
}
5658

5759
func (p *campaignProgressPrinter) Complete() {
@@ -65,8 +67,9 @@ func (p *campaignProgressPrinter) PrintStatuses(statuses []*campaigns.TaskStatus
6567
return
6668
}
6769

70+
var numStatusBars int
6871
if p.progress == nil {
69-
p.initProgressBar(statuses)
72+
numStatusBars = p.initProgressBar(statuses)
7073
}
7174

7275
newlyCompleted := []*campaigns.TaskStatus{}
@@ -110,13 +113,21 @@ func (p *campaignProgressPrinter) PrintStatuses(statuses []*campaigns.TaskStatus
110113
newlyStarted[ts.RepoName] = ts
111114
p.runningTasks[ts.RepoName] = ts
112115

113-
// Find free slot
116+
// Find free status bar slot
114117
_, ok := p.statusBarRepo[statusBarIndex]
115118
for ok {
116119
statusBarIndex += 1
117120
_, ok = p.statusBarRepo[statusBarIndex]
118121
}
119122

123+
if statusBarIndex >= numStatusBars {
124+
// If the only free slot is past the number of status bars we
125+
// have, there's a race condition going on where we have more tasks
126+
// reporting as "currently executing" than could be executing, most
127+
// likely because one of them hasn't been updated yet.
128+
break
129+
}
130+
120131
p.statusBarRepo[statusBarIndex] = ts.RepoName
121132
p.repoStatusBar[ts.RepoName] = statusBarIndex
122133
}

0 commit comments

Comments
 (0)