diff --git a/common/monitor/progress.go b/common/monitor/progress.go index 9e2c032a7..23a97cffa 100644 --- a/common/monitor/progress.go +++ b/common/monitor/progress.go @@ -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 diff --git a/common/monitor/progress_test.go b/common/monitor/progress_test.go index dee08bc25..972517f95 100644 --- a/common/monitor/progress_test.go +++ b/common/monitor/progress_test.go @@ -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, @@ -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, @@ -102,15 +106,17 @@ 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, @@ -118,7 +124,6 @@ func TestProgress(t *testing.T) { count: 5, start: time.Now(), } - // All children have zero total child1 := &Span{ name: "child1", status: StatusRunning, @@ -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) + }) }