SIGN IN SIGN UP

test+refactor(networkstream): pin the ranking order, derive the budget counters (SUB-7786)

Addresses matthyx's review.

1. The ranking order was not pinned. Inverting the estBytes comparator left the whole
suite green, TestSelectProcessTrees_PrefersSmallestTrees included, because every tree
in it is the same size -- ordering only decided WHICH equal-sized trees shipped, and
the beacon's small tree landed in the leftover budget either way. His test mixes two
size populations so the shipped COUNT depends on the order, which is the property
smallest-first exists for. Verified: ships 126 (= maxProcessTreeBytes/smallEst) as
written, 34 with the sort inverted, and the displacement assertion trips too.

2. The skip-vs-break branch is unreachable, so the comment claiming otherwise was
wrong. His proof holds: with the ascending sort, if candidate i does not fit then
used+est_i > budget, and for every j>i we have est_j >= est_i while used never
decreases, so nothing after the first miss can fit. Kept the skip as defensive against
a future ordering change, but it no longer claims to be load-bearing, and the test
named for it now states what it does and does not prove.

3. The drop counters are now derived -- len(candidates) minus len(processes), and total
connections minus shipped -- rather than accumulated in the packing loop. They are the
decision input for whether the budget needs raising, so they should not depend on the
loop body: as written before, a future early exit would have logged treesDropped 0
while dropping hundreds, with nothing asserting otherwise. Extracted selectWithinBudget
so the counters are testable, and pinned the identities that must hold regardless.

4. processNodeOverheadBytes' comment claimed "~360 measured" while the constant is 320.
He was right that the comment is the stale part: measured 133 bytes for a node with
every numeric at max and strings empty, plus ~16 for the childrenMap wrapper, so ~149
actual. The comment now says 320 is deliberate headroom -- absorbing UniqueID and
future fields -- rather than pretending to be a measurement.

Also documented two costs he raised that are real but out of scope here. The ref lookup
is on the packet path for every event including duplicates (unavoidable: the dedup key
contains the ref), and under exec churn his benchmark puts it at ~3.9x on that path --
write-lock contention on the manager mutex. The fix is a dedicated lock or atomic read
for the creator's pidStartTimeNs side map, which lives in pkg/processtree/creator and
belongs to the workstream that owns it. And the budget bounds the wire but not the
heap: storage retains one entry per process per endpoint per interval, each pinning a
tree, which cannot be capped without reintroducing the connection drops this fixes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Alon <alon@armosec.io>
A
Alon committed
24bf616f0872aa1dba5c00c6bdf24ebdc0ade98e
Parent: 86ea7a0