DropSummary.Add accepts a negative n and silently subtracts it from the running total for that key. Since stages share the summary, a negative from one stage understates another, and the printed total no longer describes the run.
Found while reviewing PR #1725, where a latent path can produce exactly that.
The latent path
reportInstanceLimit's case kept == rec.Count: continue misfires when rec.Count == 0 and the rec is past len(after): the rec is skipped from dropped but still counted in belowMinCount, so drops.Add(DropMaxInstances, dropped-belowMinCount) receives -1.
Reproduced by the reviewer: the summary printed --max-instances=-1 with an understated total. The same zero-count rec also survives the tail-only floor loop in a non-tail position.
Why it is not urgent
Unreachable today, and the barrier was verified rather than assumed. pkg/scorer/scorer.go:88 filters Count <= 0 when MinCount > 0, and scoreLimitAndDisplay is the sole caller, feeding the same cfg.MinCount to both. So no zero-count recommendation reaches reportInstanceLimit on any live path.
Why it is still worth doing
The guard belongs on Add, not on the one caller that can currently reach it. A negative count is never meaningful for a drop tally, and Add is called from several stages — a guard there protects every one of them, including callers that do not exist yet. The reportInstanceLimit arithmetic is one way to produce a negative; it will not be the last.
Suggested: reject or clamp a negative n in Add, loudly enough that a caller producing one is discovered rather than silently absorbed. Given this codebase's recent history with silent-absorption defects, erroring is likely better than clamping.
Optionally also fix the kept == rec.Count condition so the zero-count case cannot reach Add negative in the first place. Defence in depth, but the Add guard is the load-bearing half.
Test note
A test asserting only the printed total will pass with the bug present whenever the negative happens to cancel out. Assert the per-key tallies, and include a case where a negative would be produced without the guard.
DropSummary.Addaccepts a negativenand silently subtracts it from the running total for that key. Since stages share the summary, a negative from one stage understates another, and the printed total no longer describes the run.Found while reviewing PR #1725, where a latent path can produce exactly that.
The latent path
reportInstanceLimit'scase kept == rec.Count: continuemisfires whenrec.Count == 0and the rec is pastlen(after): the rec is skipped fromdroppedbut still counted inbelowMinCount, sodrops.Add(DropMaxInstances, dropped-belowMinCount)receives -1.Reproduced by the reviewer: the summary printed
--max-instances=-1with an understated total. The same zero-count rec also survives the tail-only floor loop in a non-tail position.Why it is not urgent
Unreachable today, and the barrier was verified rather than assumed.
pkg/scorer/scorer.go:88filtersCount <= 0whenMinCount > 0, andscoreLimitAndDisplayis the sole caller, feeding the samecfg.MinCountto both. So no zero-count recommendation reachesreportInstanceLimiton any live path.Why it is still worth doing
The guard belongs on
Add, not on the one caller that can currently reach it. A negative count is never meaningful for a drop tally, andAddis called from several stages — a guard there protects every one of them, including callers that do not exist yet. ThereportInstanceLimitarithmetic is one way to produce a negative; it will not be the last.Suggested: reject or clamp a negative
ninAdd, loudly enough that a caller producing one is discovered rather than silently absorbed. Given this codebase's recent history with silent-absorption defects, erroring is likely better than clamping.Optionally also fix the
kept == rec.Countcondition so the zero-count case cannot reachAddnegative in the first place. Defence in depth, but theAddguard is the load-bearing half.Test note
A test asserting only the printed total will pass with the bug present whenever the negative happens to cancel out. Assert the per-key tallies, and include a case where a negative would be produced without the guard.