Skip to content

fix(cli): DropSummary.Add silently accepts a negative count and understates other stages #1742

Description

@cristim

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.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions