Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions common/monitor/progress.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,10 +150,15 @@ func (s *Span) Progress() Progress {
parentTotal := s.total * childTotal
parentCount := s.count * childTotal
for _, child := range children {
// avoid divide by zero
// children only contains Running children (Complete / Failed /
// Suspended children are filtered out above and accounted for via
// the parent's own count). A Running child with Total == 0 has
// no measurable progress yet, so it contributes nothing - the
// previous "treat zero-total as complete" branch could push
// parentCount above parentTotal once the parent had already
// consumed its own count, producing >100% in the dashboard
// (#1241).
if child.Total == 0 {
// child with zero total means no work needed, consider it complete
parentCount += childTotal
continue
}
parentCount += childTotal * child.Count / child.Total
Expand Down
52 changes: 41 additions & 11 deletions common/monitor/progress_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,16 +66,20 @@ func TestProgress(t *testing.T) {
}
span.children.Store("child-zero-total", childSpan)

// This should not panic
// This should not panic. Running zero-total child contributes 0
// (#1241), so parentCount = 5 against parentTotal = 10.
progress := span.Progress()
assert.Equal(t, "parent", progress.Name)
assert.Equal(t, StatusRunning, progress.Status)
assert.Equal(t, 6, progress.Count)
assert.Equal(t, 5, progress.Count)
assert.Equal(t, 10, progress.Total)
})

t.Run("child with zero total", func(t *testing.T) {
// Test case: multiple children, one with zero Total
// Test case: multiple children, one with zero Total. The zero-total
// child is still Running, so it contributes 0 - it has no measurable
// progress and counting it as complete would inflate the parent
// past its real count (#1241).
span := &Span{
name: "parent",
status: StatusRunning,
Expand All @@ -91,7 +95,7 @@ func TestProgress(t *testing.T) {
count: 50,
start: time.Now(),
}
// Add a child with zero total
// Add a Running zero-total child. Contributes 0 until it Completes.
child2 := &Span{
name: "child2",
status: StatusRunning,
Expand All @@ -102,23 +106,24 @@ func TestProgress(t *testing.T) {
span.children.Store("child1", child1)
span.children.Store("child2", child2)

// This should not panic
// This should not panic.
// childTotal = max(100, 0) = 100. parentCount = 5*100 + 100*50/100 + 0 = 550.
progress := span.Progress()
assert.Equal(t, "parent", progress.Name)
assert.Equal(t, 650, progress.Count)
assert.Equal(t, 550, progress.Count)
assert.Equal(t, 1000, progress.Total)
})

t.Run("all children with zero total", func(t *testing.T) {
// Test case: all children have zero Total
// All children Running with Total=0: childTotal falls back to 1,
// children themselves contribute 0 (they have no progress yet).
span := &Span{
name: "parent",
status: StatusRunning,
total: 10,
count: 5,
start: time.Now(),
}
// All children have zero total
child1 := &Span{
name: "child1",
status: StatusRunning,
Expand All @@ -136,11 +141,36 @@ func TestProgress(t *testing.T) {
span.children.Store("child1", child1)
span.children.Store("child2", child2)

// This should not panic
// childTotal=1, so parentCount = 5 + 1 + 1 = 7, parentTotal = 10
// childTotal=1, parentCount = 5*1 + 0 + 0 = 5, parentTotal = 10.
progress := span.Progress()
assert.Equal(t, "parent", progress.Name)
assert.Equal(t, 7, progress.Count)
assert.Equal(t, 5, progress.Count)
assert.Equal(t, 10, progress.Total)
})

t.Run("zero-total running child does not exceed parent total", func(t *testing.T) {
// Regression for #1241. Parent has consumed all its own count and
// a Running zero-total child is registered. Old behaviour pushed
// parentCount to 3 against parentTotal of 2 (150%); fixed
// behaviour keeps Count <= Total.
span := &Span{
name: "parent",
status: StatusRunning,
total: 2,
count: 2,
start: time.Now(),
}
child := &Span{
name: "child",
status: StatusRunning,
total: 0,
count: 0,
start: time.Now(),
}
span.children.Store("child", child)

got := span.Progress()
assert.LessOrEqual(t, got.Count, got.Total,
"Count (%d) must not exceed Total (%d)", got.Count, got.Total)
})
}