Skip to content

fix(monitor): clamp Span.Progress Count to Total#1243

Open
shaun0927 wants to merge 1 commit intogorse-io:masterfrom
shaun0927:fix/progress-clamp
Open

fix(monitor): clamp Span.Progress Count to Total#1243
shaun0927 wants to merge 1 commit intogorse-io:masterfrom
shaun0927:fix/progress-clamp

Conversation

@shaun0927
Copy link
Copy Markdown
Contributor

Problem

After #1208 removed the divide-by-zero panic in Span.Progress, the "zero-total child is complete" fallback can push parentCount above parentTotal when the parent has already consumed its own count and a running child is reporting zero total.

With a parent total=2, count=2 plus one running child with total=0, Span.Progress() returns Progress{Count: 3, Total: 2}. The dashboard progress bar at /api/dashboard/tasks renders this as 150%.

Root Cause

common/monitor/progress.go:138-169:

parentTotal := s.total * childTotal
parentCount := s.count * childTotal
for _, child := range children {
    if child.Total == 0 {
        parentCount += childTotal   // treat zero-total child as complete
        continue
    }
    parentCount += childTotal * child.Count / child.Total
}

When s.count * childTotal == parentTotal and the zero-total branch fires, parentCount exceeds parentTotal by one childTotal unit. There is no clamp before the Progress{} value is returned.

Solution

Clamp parentCount to parentTotal just before returning:

if parentCount > parentTotal {
    parentCount = parentTotal
}

This is the minimal change that keeps the "zero-total child is complete" semantics from #1208 while restoring the 0 <= Count <= Total invariant.

Testing

New pinning test in common/monitor/progress_overflow_test.go:

func TestProgress_ZeroTotalChildDoesNotExceed100Percent(t *testing.T) {
    _, parent := Start(context.Background(), "parent", 2)
    parent.Add(2)

    ctx := context.WithValue(context.Background(), spanKeyName, parent)
    _, _ = Start(ctx, "child", 0)

    got := parent.Progress()
    if got.Count > got.Total {
        t.Fatalf("Progress.Count (%d) exceeds Progress.Total (%d)", got.Count, got.Total)
    }
}
  • Before the clamp, this test fails with Count (3) exceeds Total (2).
  • After the clamp, all existing common/monitor tests (including TestProgress/divide_by_zero, TestProgress/child_with_zero_total, TestProgress/all_children_with_zero_total added in Fix divide by zero in progress calculation #1208) still pass.

Scope

Single-hunk fix plus a targeted regression test. No behavioural change for the non-overflowing cases; the clamp only engages on the exact scenario #1208 could not reach.

Fixes #1241

The zero-total child fallback introduced in gorse-io#1208 lets parentCount
exceed parentTotal when the parent has already consumed its own count
and a running child is reporting no work units. The resulting
Count > Total value surfaces as a >100% progress bar in the dashboard.

Add a single-line clamp so Count never exceeds Total, and pin the
behavior with TestProgress_ZeroTotalChildDoesNotExceed100Percent.
@shaun0927 shaun0927 force-pushed the fix/progress-clamp branch from 3d5b707 to 7af1a6a Compare April 17, 2026 05:14
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 17, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.09%. Comparing base (d7a249a) to head (7af1a6a).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1243      +/-   ##
==========================================
+ Coverage   73.05%   73.09%   +0.03%     
==========================================
  Files          89       89              
  Lines       16651    16653       +2     
==========================================
+ Hits        12165    12173       +8     
+ Misses       3247     3242       -5     
+ Partials     1239     1238       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Span.Progress returns Count > Total when a running child has Total == 0 (follow-up to #1208)

1 participant