feat(networkstream): process attribution on the network stream (SUB-7786)

[AlonLiwsky]

Summary

Puts process identity on the network-stream wire, so a malicious-endpoint finding can
aggregate onto the same incident as the exec, file and other findings from the same
process chain. Until now the stream carried no process data at all on the wire: the
sensor captured a process tree per connection and then stripped it before sending.

This also fixes silent data loss. The per-batch connection key was
address/port/protocol with first-writer-wins, so when two processes in one container
reached the same endpoint inside a flush interval, the second connection was discarded
entirely — it never reached the wire under any identity. The same held for two processes
resolving one domain. TestHandleNetworkEvent_TwoProcessesSameEndpoint pins that: before
the fix the batch holds one entry, not two.

What changed

• processRef per connection, plus processAttributionVersion per message
  (NetworkStreamProcessAttributionV1 = 1, set unconditionally — an empty processes
  map is not a capability signal).
• The batch key gained the ref, appended and never reordered: an unattributed key
  stays byte-identical to the old format. For IP connections the forms cannot collide
  (address/port/protocol are structured, so an attributed key carries exactly two more
  components); for DNS the name is unstructured, so a contrived name ending in
  /<pid>/<startTimeNs> can collide, costing one record — documented in the feature doc.
  First-writer-wins is retained per process.
• Trees ship once per process in the message-scoped processes map, not once per
  connection — trees dominate the payload, the duplicated ref bytes do not. On collision
  the deeper chain wins, because the tree cache TTL (1 min) is shorter than the flush
  interval (2 min) and first-wins would both discard the richer chain and make the
  payload depend on map iteration order.
• Command lines capped at 1024 bytes on the wire copy only, with a visible …,
  never splitting a rune. cmdline is ~40% of a tree's bytes and unbounded, so this is
  what bounds the payload tail rather than an optimisation.
• The flush snapshot is now independent of the live storage, which deletes a race
  rather than papering over it (below).
• A per-message budget on the process trees, because attribution changed what
  sizes the payload (below).
• go.mod: armoapi-go v0.0.696 → v0.0.742, the exact tag carrying the schema.
  v0.0.741 does not contain it, so a newer tag is not evidence.
• New feature doc: docs/features/network-stream-process-attribution.md.

The flush race, and lock discipline

The flush used to hand the notification channel the live storage struct, then — still
holding eventsStorageMutex — sleep 100 ms and strip every process tree from the maps
the consumer was reading. The sleep was the only thing standing between
private-node-agent's host network sensor and having its data erased mid-read. A test
showed worse: a connection recorded 300 ms after the flush appeared inside the
already-delivered snapshot, because the clear loop and the consumer shared the same maps.

snapshotNetworkStream now allocates its own event maps, so the delivered value is
immune to everything the producer does next. The 100 ms sleep is deleted rather than
shortened — there is no shared state left to race on.

This file has a prior mutex-stall fix in its history, so the bound on lock-held work is
part of the design:

	Inside eventsStorageMutex	Outside
Work	Allocate the snapshot's maps; copy each event by value; clear the live storage	Both sends; the wire copy's tree copies and command-line caps
Bound	O(entities + connections) small struct copies — a few hundred at the largest observed message (109 entities, 283 connections)	one capped tree copy per selected process — transient memory is bounded by the tree budget (2.5 MiB), not by the connection count, and freed after the send

Tree pointers are copied into the snapshot, never walked. That is safe because a tree
is immutable once attached: buildBranchToShim allocates fresh nodes per branch, so
event trees are not shared with the process tree's live map — only with its 1-minute LRU
entry, which nothing writes to after construction. The ref is also computed before
eventsStorageMutex is taken, so no storage→tree lock-order edge is added.

Payload budget — a gap in the approved plan, not only in the code

Attribution changed what sizes the payload. The old batch key collapsed every
process reaching one endpoint into a single entry carrying no tree, so size tracked
distinct endpoints. The new key splits per process and each distinct process
contributes a tree, so size tracks distinct processes that connected — and nothing
bounded that. SUB-7850's analysis modelled bytes per connection at a fixed count of
283 connections, which is exactly the quantity the key change stops holding fixed, so
the multiplier on connection count was never budgeted.

Breaching the broker's 5 MiB limit is not graceful: sendNetworkEvent gets a non-2xx,
Start() logs it and drops the snapshot — the node loses its entire interval of
traffic, no retry, no split. A node with heavy short-lived process churn (a cron loop
shelling out, a CI runner, exec-based probes) can reach that.

maxProcessTreeBytes bounds it at 2.5 MiB of estimated tree bytes, calibrated
against measured production traffic rather than guessed:

Input	Value	Source
Mean message on topic	29 KB (prod-us 32 KB)	pulsar_average_msg_size on network-stream-v1
Mean connections per batch	~42 (prod-us ~32)	network_reputation_events_in_total ÷ topic message count
⇒ bytes per connection, no tree	~530 B JSON	derived
Largest batch ever observed	283 connections	SUB-7850

Measured end to end against the worst case the budget exists for — every connection
from a distinct process, so trees scale 1:1 — with production-weight entries included:

Connections (all distinct)	Trees shipped	JSON	after base64
42 (the mean)	all 42	0.20 MiB	0.26 MiB
283 (observed worst)	all 283	1.33 MiB	1.77 MiB — 35% of limit
500	all 500	2.35 MiB	3.13 MiB
1000	513 (budget binds)	2.70 MiB	3.60 MiB
4000	513	4.50 MiB	6.01 MiB — over, on entries alone

Design points:

• A byte budget, not a tree count — tree size is not uniform (depth varies, fields
  are variable-length, JSON escaping inflates some content ~6×), so no count bounds the
  payload. The estimator overestimates realistic trees by ~19%, and two tests enforce
  that it never goes the other way (see below).
• Connections are never dropped — that is the data loss this PR fixes. Only trees
  are, and the refs stay on the connections, so pid identity survives and
  ProcessTreeFor returns nil for them as specified.
• Ranked smallest-tree-first, ties broken on the ref, which maximises the number of
  processes keeping a tree. Connection-count ranking was tried and rejected: a
  low-and-slow beacon opens exactly one connection per interval, so it sorted last and
  lost its tree first — the precise case reputation exists to catch.
• It must not bind on real traffic, or it degrades attribution on exactly the
  busiest nodes. A tighter 1.5 MiB was tried first and rejected — it clips 70 of 283
  trees while the payload there is barely a third of the limit.
  TestBuildWireStream_ObservedWorstCaseFitsBudget pins this and fails at 1.5 MiB.

The budget only became a real bound after a review caught this

The first version charged len(s) per string. That is not what encoding/json
emits: it escapes " and \, every control byte, and — because Marshal enables HTML
escaping — <, > and &, which real command lines are full of
(sh -c 'cmd > /dev/null 2>&1'). Invalid UTF-8 becomes the 6-byte \ufffd, and process
argv is arbitrary kernel bytes. Each such byte costs up to six where len() counted
one.

Measured: 629 processes with 1 KB of non-UTF-8 argv each estimated at 29% of the
budget, so nothing was dropped and nothing logged, while the real message was
5.37 MB after base64 — rejected by the broker, node loses its whole interval. It also
undercounted ordinary traffic (a realistic node: 372 estimated vs 395 marshalled).
Reachable deliberately by anyone able to exec in a container on the node, which made it
a detection-evasion primitive with a wider blast radius than the bug this PR fixes.

escapedLen now charges the true escaped cost, and the direction is enforced rather than
asserted: TestEstimateTreeBytes_NeverUnderestimates over 15 adversarial shapes (fails
on 10 of them with the old estimator) plus a 400-case randomised version checked against
real json.Marshal output.

How often this binds in reality is not knowable from current data, because today's
sensor strips trees and emits no process identity — distinct-processes-per-batch exists
in no message. So two log lines make it answerable after rollout: process tree budget exceeded (with processesWithTrees, treesShipped, treesDropped,
connectionsWithoutTree) and large payload above 2 MiB (size, entity, connection and
tree counts). Quiet at the ~30 KB fleet mean. If the first fires routinely, the budget
should become configurable rather than being raised blind.

Residual, deliberately not fixed here: with trees bounded, the connection entries
alone breach the limit at roughly 3,300 connections (11× the observed worst; measured
6.01 MiB at 4,000). No tree budget can help there — the fix for that regime is splitting
the message, as docs/features/container-profile-split-on-413.md does on HTTP 413, not
dropping connections.

Not Process.DeepCopy()

armotypes.Process.DeepCopy mutates its receiver: it calls MigrateToMap, which
allocates ChildrenMap and nils Children, and it does so on every child it recurses
into. Those nodes are shared with the process-tree manager's LRU cache, the legacy alert
paths and the channel consumer, so calling it from the flush goroutine would be a data
race that also strips the uncapped command lines those consumers rely on.
copyCappedProcess is read-only instead, normalising the deprecated Children slice
into the copy without touching the source. TestBuildWireStream_DoesNotMutateSharedTree
pins this so a future refactor cannot regress to DeepCopy. Every recursive walk is
depth-bounded (64).

StartTimeNs is boot-relative nanoseconds, emitted verbatim

Converted once on the way in from /proc (SUB-7845); nothing here rescales it. A
division would still join correctly within one message — key and ref share a producer —
while silently breaking identity across messages by seven orders of magnitude.
StartTimeNs appears in exactly one non-test statement in this diff and in no arithmetic
expression. Zero is legal and means unknown; the ref is emitted anyway, degrading to
pid-only identity. See docs/features/process-start-time.md.

Handoff for the host/ECS workstream

cmd/host, cmd/ecs, the KubernetesMode streaming gate and pkg/hostnetworksensor are
untouched. Inheriting from this change:

• The tree strip is gone, and with it the shared-map race the host sensor's 100 ms
  sleep existed to survive. The channel now delivers an independent snapshot, still
  with trees.
• The batch key gained a process suffix (format above).
• The channel send is still blocking (backpressure, not data loss) but now selects on
  context cancellation.
• Outside Kubernetes mode the HTTP export is disabled entirely, so that in-process
  channel is the only consumer today.
• In Kubernetes mode, pre-existing host (non-container) processes carry no start time.

Testing

pkg/networkstream cannot be built or tested on macOS (inspektor-gadget/pkg/utils/host
excludes darwin, imported transitively). Everything below ran in a Linux container:

docker run --rm -v "$PWD":/src -v "$(go env GOMODCACHE)":/go/pkg/mod -w /src \
  -e GOFLAGS=-mod=mod -e CGO_ENABLED=1 golang:1.25 \
  sh -c 'go build ./... && go vet ./pkg/networkstream/... && go test -race ./pkg/networkstream/... -count=1'

pkg/networkstream/v1 had two test functions before this change, so it brings its own
coverage. go test ./pkg/... shows the same pass/fail set as main: the only
failures are pre-existing and unrelated (pkg/containerwatcher/v2/tracers needs a
tracers.tar build artifact; pkg/validator TestCheckPrerequisites).

Two plan amendments this work implies

Neither is a code change; both are for the plan owner:

1. §2.3's payload paragraph reads as though the cmdline cap were the only lever.
   It is not — the connection-count multiplier introduced by the key change was never
   analysed, which is what the budget above addresses.
2. The two plan documents disagree on p90 tree size: the implementation plan says
   5.1 KB, while the epic plan's "~2 MB at 283 connections" implies ~7 KB. This work
   calibrated against 7 KB (the pessimistic figure).

CI note

build-and-push-image will fail on missing Quay secrets. That is structural for a
cross-fork PR — the job gets no secrets — not a defect in this change.

AI Review

Two independent fresh-context reviews by Claude Opus 5, each in a clean subagent
following armosec-shared-rules:code-review-standards, each with its own probe tests and
mutation passes.

Round 1 (commits through ad55206c) — no must-fix. It independently confirmed the
four highest-risk properties: no StartTimeNs scaling anywhere, no lock-order hazard, no
data race under -race with concurrent producers/flushers over a shared tree graph, and
that trees really are immutable once attached (so sharing pointers into the snapshot is
safe). It also confirmed the DeepCopy deviation is justified by reading the armoapi-go
source. Six should-fix/nit items were addressed in 82ba0dd2, including two of my own
tests that did not pin what they claimed — TestNoTickScaling passed with a
/10_000_000 injected into processRefFor, and the shutdown select was covered by
nothing because every channel test buffers so the producer never blocks. Both now fail
against those mutations.

Round 2 (the budget commits, which round 1 never saw) — one must-fix, now fixed.
The tree budget was not a bound: the estimator charged len(s) while encoding/json
escapes, so 629 processes with non-UTF-8 argv estimated at 29% of budget and produced a
5.37 MB message that the broker rejects, costing the node its whole interval — see the
section above. Fixed in 1e978ede with escapedLen, a raised per-node overhead, a
15-shape property test and a 400-case randomised one, both checked against real
json.Marshal output. The review also found the determinism contract, skip-vs-break
packing, the estimator's legacy-Children branch and the wrapper-field copy were all
unpinned (mutations survived the suite) — closed in the same commit — and that my
connection-count ranking rationale was backwards for the beacon case it named, which is
why ranking is now smallest-tree-first.

Verdict: approved after fixes. Everything else in both rounds was verified correct.
Round 2's closing note: "the test suite is unusually load-bearing: 5 of 13 mutations were
killed by named assertions."

Ticket

SUB-7786

AI-skills: none

Summary by CodeRabbit

• New Features

  • Network and DNS events now include process attribution with process-reference deduplication and connection counting.
  • Stream payloads retain process references while selecting deduplicated process-trees under a deterministic byte budget, with UTF-8 command-line truncation and warnings for dropped trees.

• Bug Fixes

  • Improved flush behavior with snapshot isolation, safer shutdown-aware notifications/backpressure, and more consistent event/payload handling.

• Documentation

  • Added detailed documentation on network-stream process attribution semantics and testing notes.

• Tests

  • Expanded coverage for attribution, snapshot/flush guarantees, budgeting statistics, and shutdown/cancellation scenarios.

• Chores

  • Updated the ArmoAPI Go dependency to a newer version.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Changes

The network stream now attributes network and DNS events to processes, isolates flush snapshots, and builds bounded wire payloads with deduplicated process trees. Tests cover attribution, cancellation, copying, serialization limits, deterministic selection, and UTF-8 handling. Documentation and the armoapi-go dependency were updated.

Network stream attribution

Layer / File(s)	Summary
Process-aware event attribution pkg/networkstream/v1/network_stream.go, pkg/networkstream/v1/network_stream_test.go, go.mod, docs/features/network-stream-process-attribution.md	Network and DNS keys and payloads include process references when available. PID-less events retain legacy keys. Tests cover process lookup, deduplication, and connection counting.
Snapshot-based flushing pkg/networkstream/v1/network_stream.go, pkg/networkstream/v1/network_stream_test.go, docs/features/network-stream-process-attribution.md	Flush operations snapshot and clear live maps under lock. Wire conversion and delivery occur outside the lock. Notification sends honor shutdown cancellation.
Bounded wire payload construction pkg/networkstream/v1/wire.go, pkg/networkstream/v1/wire_test.go, docs/features/network-stream-process-attribution.md	Wire conversion deduplicates process trees, preserves references, copies trees defensively, truncates command lines safely, estimates escaped JSON size, and selects trees within the payload budget.
Payload observability and operational documentation pkg/networkstream/v1/network_stream.go, docs/features/network-stream-process-attribution.md	Large payloads log serialized size and counts. Documentation describes handoff behavior, platform scope, residual limits, and testing.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Producer
  participant NetworkStream
  participant BuildWireStream
  participant NotificationChannel
  participant HTTP
  Producer->>NetworkStream: submit process-aware events
  NetworkStream->>NetworkStream: snapshot and clear live storage
  NetworkStream->>BuildWireStream: build bounded wire payload
  NetworkStream->>NotificationChannel: send intact snapshot or cancel on shutdown
  BuildWireStream->>HTTP: send events and selected process trees

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: adding process attribution to network-stream events.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

pkg/networkstream/v1/network_stream.go (1)

209-214: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Skip the wire copy when the HTTP exporter is disabled.

buildWireStream performs every tree deep-copy, command-line cap and size estimate for the flush. sendNetworkEvent then returns immediately when !ns.cfg.KubernetesMode || ns.cfg.Exporters.HTTPExporterConfig == nil (Line 484). Outside Kubernetes mode the HTTP export is disabled entirely, so this work is discarded on every interval on host and ECS deployments.

Gate the derivation on the same condition that sendNetworkEvent uses.

♻️ Proposed change

Add a helper next to sendNetworkEvent:

func (ns *NetworkStream) httpExportEnabled() bool {
	return ns.cfg.KubernetesMode && ns.cfg.Exporters.HTTPExporterConfig != nil
}

Then derive the wire copy conditionally:

-				wire := buildWireStream(snapshot)
+				var wire *armotypes.NetworkStream
+				if ns.httpExportEnabled() {
+					wire = buildWireStream(snapshot)
+				}

and guard the send:

-				if err := ns.sendNetworkEvent(wire); err != nil {
+				if wire != nil {
+					if err := ns.sendNetworkEvent(wire); err != nil {
 						logger.L().Error("NetworkStream - failed to send network events", helpers.Error(err))
+					}
 				}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/networkstream/v1/network_stream.go` around lines 209 - 214, Gate the
expensive buildWireStream call in the flush path using a new
NetworkStream.httpExportEnabled helper that returns true only when
KubernetesMode is enabled and HTTPExporterConfig is non-nil. Update
sendNetworkEvent to use the same helper, ensuring wire derivation and sending
share identical enablement conditions.

pkg/networkstream/v1/wire.go (1)

159-163: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Equal-depth collisions still select a tree by map iteration order.

The comment at Lines 144-145 states that the payload must not depend on Go's randomised map iteration order. The strict depth > candidate.depth comparison keeps the first-visited tree when two distinct trees for one ref have equal depth. The retained tree, and therefore estBytes, then varies between runs. Because estBytes feeds the budget ranking in selectProcessTrees, the shipped set can also vary in that case.

Add a deterministic secondary tie-break, for example the larger estBytes or the wider node count.

♻️ Proposed tie-break on estimated size

-			if depth := chainDepth(&event.ProcessTree.ProcessTree); !seen || depth > candidate.depth {
-				candidate.tree = event.ProcessTree
-				candidate.depth = depth
-				candidate.estBytes = estimateTreeBytes(event.ProcessTree)
-			}
+			depth := chainDepth(&event.ProcessTree.ProcessTree)
+			if !seen || depth > candidate.depth {
+				candidate.tree = event.ProcessTree
+				candidate.depth = depth
+				candidate.estBytes = estimateTreeBytes(event.ProcessTree)
+			} else if depth == candidate.depth {
+				// Equal depth: keep the larger estimate so the choice is
+				// independent of map iteration order.
+				if est := estimateTreeBytes(event.ProcessTree); est > candidate.estBytes {
+					candidate.tree = event.ProcessTree
+					candidate.estBytes = est
+				}
+			}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/networkstream/v1/wire.go` around lines 159 - 163, Update the candidate
selection condition in the process-tree handling block to deterministically
resolve equal-depth collisions, using a stable secondary criterion such as
larger estimateTreeBytes(event.ProcessTree). Ensure both the selected
candidate.tree and candidate.estBytes are updated when the secondary tie-break
wins, so selectProcessTrees produces map-order-independent results.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/networkstream/v1/wire.go`:
- Around line 273-278: Suppress SA1019 at the intentional legacy Children reads
without changing behavior. In pkg/networkstream/v1/wire.go#L273-L278,
`#L356-L371`, and `#L411-L415`, add targeted nolint:staticcheck annotations to the
specified node.Children, dst.Children, and src.Children accesses, documenting
charging, normalization, or depth-counting intent. In
pkg/networkstream/v1/wire_test.go#L164-L188, annotate
shared.ProcessTree.Children to document that the assertion verifies the source
slice remains populated.
- Around line 256-280: Update estimateProcessBytes to include each child’s
serialized ChildrenMap key cost during the ChildrenMap recursion, accounting for
the child Comm and PID text representation as escaped output. Apply the
equivalent child Comm key cost in the legacy Children recursion so both paths
include key overhead before recursing.

---

Nitpick comments:
In `@pkg/networkstream/v1/network_stream.go`:
- Around line 209-214: Gate the expensive buildWireStream call in the flush path
using a new NetworkStream.httpExportEnabled helper that returns true only when
KubernetesMode is enabled and HTTPExporterConfig is non-nil. Update
sendNetworkEvent to use the same helper, ensuring wire derivation and sending
share identical enablement conditions.

In `@pkg/networkstream/v1/wire.go`:
- Around line 159-163: Update the candidate selection condition in the
process-tree handling block to deterministically resolve equal-depth collisions,
using a stable secondary criterion such as larger
estimateTreeBytes(event.ProcessTree). Ensure both the selected candidate.tree
and candidate.estBytes are updated when the secondary tie-break wins, so
selectProcessTrees produces map-order-independent results.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: aff823fb-17b6-4ec6-9e56-5b30dcfd3ec0

📥 Commits

Reviewing files that changed from the base of the PR and between 8866b6c and 7d33da0.

⛔ Files ignored due to path filters (1)

• go.sum is excluded by !**/*.sum

📒 Files selected for processing (6)

• docs/features/network-stream-process-attribution.md
• go.mod
• pkg/networkstream/v1/network_stream.go
• pkg/networkstream/v1/network_stream_test.go
• pkg/networkstream/v1/wire.go
• pkg/networkstream/v1/wire_test.go

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (3)

pkg/networkstream/v1/wire_test.go (2)

284-292: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Assert that the truncated output keeps the original prefix.

The over-cap assertions check the length, the marker suffix and UTF-8 validity. They do not check that the retained bytes come from the start of the input. An implementation that returned only cmdlineTruncationMarker, or that kept the tail instead of the head, would still pass. Add a prefix assertion so the test pins which part of the command line survives.

♻️ Proposed prefix assertion

 	over := strings.Repeat("a", maxCmdlineBytes+1)
 	assert.LessOrEqual(t, len(capCmdline(over)), maxCmdlineBytes, "the marker is inside the budget")
 	assert.True(t, strings.HasSuffix(capCmdline(over), cmdlineTruncationMarker))
+	assert.True(t, strings.HasPrefix(over, strings.TrimSuffix(capCmdline(over), cmdlineTruncationMarker)),
+		"truncation keeps the head of the command line, not the tail")
 
 	// Truncation never splits a rune.
 	multibyte := strings.Repeat("é", maxCmdlineBytes) // 2 bytes per rune
 	capped := capCmdline(multibyte)
 	assert.LessOrEqual(t, len(capped), maxCmdlineBytes)
 	assert.True(t, utf8.ValidString(capped), "a cut mid-rune would emit invalid UTF-8")
+	assert.True(t, strings.HasPrefix(multibyte, strings.TrimSuffix(capped, cmdlineTruncationMarker)),
+		"the surviving runes are the leading ones")

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/networkstream/v1/wire_test.go` around lines 284 - 292, Add an assertion
in the capCmdline truncation tests to verify the output preserves the original
input prefix before the truncation marker. Cover the ASCII over-cap case and/or
multibyte case using the existing over, multibyte, and capped variables, while
retaining the current length, suffix, and UTF-8 assertions.

---

581-619: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Collapse the four child-attachment helpers into one.

escapeHeavyCommTree, escapeHeavyCommLegacyTree, unboundedCommTree and unboundedCommLegacyTree share the same body. They differ only in the comm value and in whether the child goes into ChildrenMap or into the Children slice. Parameterise those two choices. A future change to realisticNode or to the child shape then needs one edit, not four.

The unbounded-comm generators are the right addition. They stop the estimate from resting on the kernel TASK_COMM_LEN bound, which nothing in this repository enforces.

Based on learnings: size-estimation logic must not assume the 15-byte comm bound unless it explicitly enforces or validates the bound itself.

♻️ Proposed consolidation

-func escapeHeavyCommTree(children int) *armotypes.ProcessTree {
-	root := realisticNode(1, "sh")
-	for i := 0; i < children; i++ {
-		child := realisticNode(uint32(1000+i), "/usr/bin/curl https://example.com")
-		child.Comm = escapeHeavyComm
-		root.ChildrenMap[armotypes.CommPID{Comm: child.Comm, PID: child.PID}] = child
-	}
-	return treeOf("c1", root)
-}
-
-func escapeHeavyCommLegacyTree(children int) *armotypes.ProcessTree {
-	root := realisticNode(1, "sh")
-	for i := 0; i < children; i++ {
-		child := realisticNode(uint32(1000+i), "/usr/bin/curl https://example.com")
-		child.Comm = escapeHeavyComm
-		root.Children = append(root.Children, *child)
-	}
-	return treeOf("c1", root)
-}
-
-func unboundedCommTree(children, commBytes int) *armotypes.ProcessTree {
-	root := realisticNode(1, "sh")
-	for i := 0; i < children; i++ {
-		child := realisticNode(uint32(1000+i), "/usr/bin/curl https://example.com")
-		child.Comm = strings.Repeat("<", commBytes)
-		root.ChildrenMap[armotypes.CommPID{Comm: child.Comm, PID: child.PID}] = child
-	}
-	return treeOf("c1", root)
-}
-
-func unboundedCommLegacyTree(children, commBytes int) *armotypes.ProcessTree {
-	root := realisticNode(1, "sh")
-	for i := 0; i < children; i++ {
-		child := realisticNode(uint32(1000+i), "/usr/bin/curl https://example.com")
-		child.Comm = strings.Repeat("<", commBytes)
-		root.Children = append(root.Children, *child)
-	}
-	return treeOf("c1", root)
-}
+// commFanOutTree builds a root with `children` children, each carrying `comm`.
+// If legacy is true the children go into the deprecated Children slice instead
+// of ChildrenMap, which is the shape capTreeCopy must normalise.
+func commFanOutTree(children int, comm string, legacy bool) *armotypes.ProcessTree {
+	root := realisticNode(1, "sh")
+	for i := 0; i < children; i++ {
+		child := realisticNode(uint32(1000+i), "/usr/bin/curl https://example.com")
+		child.Comm = comm
+		if legacy {
+			root.Children = append(root.Children, *child)
+			continue
+		}
+		root.ChildrenMap[armotypes.CommPID{Comm: child.Comm, PID: child.PID}] = child
+	}
+	return treeOf("c1", root)
+}

Then update the table cases:

{"escape-heavy child comm, fan-out 5", commFanOutTree(5, escapeHeavyComm, false)},
{"escape-heavy child comm, fan-out 200", commFanOutTree(200, escapeHeavyComm, false)},
{"escape-heavy child comm, legacy Children", commFanOutTree(50, escapeHeavyComm, true)},
{"unbounded child comm, fan-out 200", commFanOutTree(200, strings.Repeat("<", 1024), false)},
{"unbounded child comm, legacy Children", commFanOutTree(200, strings.Repeat("<", 1024), true)},

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/networkstream/v1/wire_test.go` around lines 581 - 619, Replace the four
helpers escapeHeavyCommTree, escapeHeavyCommLegacyTree, unboundedCommTree, and
unboundedCommLegacyTree with one parameterized commFanOutTree helper that
accepts child count, comm value, and a legacy-Children flag, then attaches
children to either ChildrenMap or Children accordingly. Update the table cases
to use this helper, preserving both escape-heavy and unbounded comm scenarios,
including the 1024-byte comm value.

Source: Learnings

docs/features/network-stream-process-attribution.md (1)

252-262: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

The bullet list at Line 228 is interrupted by a heading and prose.

Line 228 opens a bullet list. Line 234 starts a #### heading, and lines 236-259 are prose. Line 260 then resumes bullets. Markdown renders this as two separate lists, so the reader loses the grouping of the budget properties. Move the escaping subsection after the bullets, or promote each bullet to its own subsection.

The accounting content itself is correct. It charges comm twice and does not rest the estimate on the kernel TASK_COMM_LEN bound.

Based on learnings: size-estimation logic must not assume the 15-byte comm bound unless it enforces or validates the bound itself.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/features/network-stream-process-attribution.md` around lines 252 - 262,
Reorganize the documentation around the bullet list beginning at line 228 so it
remains one continuous Markdown list: move the “escaping” subsection and its
prose after all bullets, including the content currently resumed at line 260.
Preserve the existing accounting guidance that charges comm twice and does not
rely on an unenforced TASK_COMM_LEN bound.

Source: Learnings

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/features/network-stream-process-attribution.md`:
- Around line 212-226: Clarify the 4000-row explanation to state that its 6.01
MiB estimate includes the 530 B per-entry production weighting, unlike the
events used by TestBuildWireStream_OverBudgetDropsTreesNotConnections, which
omit those connection fields and therefore remain under the 5 MiB test bound.

In `@pkg/networkstream/v1/wire_test.go`:
- Around line 773-792: Draw the child count once before the loop in the test
generator, then iterate using that fixed count instead of calling rng.Intn(6) in
the loop condition. Update the loop surrounding the child construction and
ChildrenMap insertion so fan-out is uniformly selected from 0 through 5,
preserving the existing child population logic.

---

Nitpick comments:
In `@docs/features/network-stream-process-attribution.md`:
- Around line 252-262: Reorganize the documentation around the bullet list
beginning at line 228 so it remains one continuous Markdown list: move the
“escaping” subsection and its prose after all bullets, including the content
currently resumed at line 260. Preserve the existing accounting guidance that
charges comm twice and does not rely on an unenforced TASK_COMM_LEN bound.

In `@pkg/networkstream/v1/wire_test.go`:
- Around line 284-292: Add an assertion in the capCmdline truncation tests to
verify the output preserves the original input prefix before the truncation
marker. Cover the ASCII over-cap case and/or multibyte case using the existing
over, multibyte, and capped variables, while retaining the current length,
suffix, and UTF-8 assertions.
- Around line 581-619: Replace the four helpers escapeHeavyCommTree,
escapeHeavyCommLegacyTree, unboundedCommTree, and unboundedCommLegacyTree with
one parameterized commFanOutTree helper that accepts child count, comm value,
and a legacy-Children flag, then attaches children to either ChildrenMap or
Children accordingly. Update the table cases to use this helper, preserving both
escape-heavy and unbounded comm scenarios, including the 1024-byte comm value.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: b9f3c69d-bc66-4fd8-a359-f5c8a75c041b

📥 Commits

Reviewing files that changed from the base of the PR and between 5b5ff1c and 86ea7a0.

⛔ Files ignored due to path filters (1)

• go.sum is excluded by !**/*.sum

📒 Files selected for processing (6)

• docs/features/network-stream-process-attribution.md
• go.mod
• pkg/networkstream/v1/network_stream.go
• pkg/networkstream/v1/network_stream_test.go
• pkg/networkstream/v1/wire.go
• pkg/networkstream/v1/wire_test.go

🚧 Files skipped from review as they are similar to previous changes (4)

• go.mod
• pkg/networkstream/v1/network_stream.go
• pkg/networkstream/v1/wire.go
• pkg/networkstream/v1/network_stream_test.go

[matthyx]

Reviewed this locally — build, go vet and go test -race ./pkg/networkstream/... all pass on go1.25.8. No correctness defect found, and the parts that carry the most risk hold up: I mutation-tested the suite and it killed StartTimeNs rescaling, the snapshot sharing the live maps again, capTreeCopy → Process.DeepCopy(), escapedLen → len(s), and dropping the childrenMap-key charge. I also checked the tree-immutability claim independently — buildBranchToShim allocates fresh nodes, and both MigrateToMap callers reachable on that graph (alert_bulk_manager.go:83, utils.CalculateProcessTreeDepth) are no-ops for branch-built trees, since every node gets a non-nil ChildrenMap and never a Children slice. So the new outside-the-lock walks add readers, not writers.

Four things below, none blocking.

1. The tree-ranking order isn't actually pinned

Flipping a.estBytes < b.estBytes to > in wire.go:159 leaves the whole suite green, TestSelectProcessTrees_PrefersSmallestTrees included. Every tree in that test is the same size, so ordering only decides which equal-sized trees ship — and the beacon's small tree lands in the leftover budget either way. Mixing two size populations makes the shipped count depend on ordering, which is the property the ranking exists for:

func TestSelectProcessTrees_MaximisesTreesKept(t *testing.T) {
	const smallBytes, largeBytes = 20_000, 80_000
	smallEst := estimateTreeBytes(bigTree(1, smallBytes))
	require.Greater(t, estimateTreeBytes(bigTree(1, largeBytes)), smallEst*3,
		"the two populations must be far enough apart to discriminate")

	events := map[string]armotypes.NetworkStreamEvent{}
	smallRefs := map[armotypes.ProcessRef]bool{}
	for pid := uint32(1); pid <= 200; pid++ {
		ref := &armotypes.ProcessRef{PID: pid, StartTimeNs: 10_000_000}
		smallRefs[*ref] = true
		events[fmt.Sprintf("10.1.0.%d/443/TCP/%d/10000000", pid%256, pid)] =
			armotypes.NetworkStreamEvent{ProcessRef: ref, ProcessTree: bigTree(pid, smallBytes)}
	}
	for pid := uint32(1000); pid < 1100; pid++ {
		ref := &armotypes.ProcessRef{PID: pid, StartTimeNs: 10_000_000}
		events[fmt.Sprintf("10.2.0.%d/443/TCP/%d/10000000", pid%256, pid)] =
			armotypes.NetworkStreamEvent{ProcessRef: ref, ProcessTree: bigTree(pid, largeBytes)}
	}

	wire := buildWireStream(outboundOnly("c1", events))

	// Greedy-smallest is optimal for "most processes keep a tree", so the expected
	// count is computable rather than a magic number.
	want := min(maxProcessTreeBytes/smallEst, 200)
	assert.Equal(t, want, len(wire.Processes),
		"ranking must maximise how many processes keep a tree")
	for ref := range wire.Processes {
		assert.True(t, smallRefs[ref], "a large tree displaced a small one: ranking is not smallest-first")
	}
}

Passes on 86ea7a08 (ships 126, exactly maxProcessTreeBytes/smallEst); fails with the sort inverted (ships 34, and the displacement assertion trips too).

2. The skip-vs-break branch is dead code, so the test named for it is vacuous

continue and break in the packing loop are provably equivalent given the ascending sort: if candidate i doesn't fit then used + est_i > budget, and for every j > i the sort guarantees est_j ≥ est_i while used never decreases, so used + est_j > budget too. Nothing after the first miss can ever fit. I ran 5000 randomised budget/size trials against both loop shapes — identical selected set every time.

So wire.go:170's "Skip rather than stop, so a small tree still ships after an oversized one" describes a scenario the sort makes unreachable, and TestSelectProcessTrees_OversizedTreeDoesNotBlockSmallOnes passes trivially because the five small trees sort ahead of the oversized one regardless. Worth either dropping the comment or keeping the defensive continue without claiming it's load-bearing. (It only became reachable in my mutation run because I had also inverted the sort — which is why neither mutation failed on its own.)

3. Derive the drop counters rather than accumulating them

The one real consequence of the loop shape is the warning log, and that's fixable by construction:

	totalBytes, totalConnections := 0, 0
	for _, candidate := range candidates {
		totalBytes += candidate.estBytes
		totalConnections += candidate.connections
	}
	...
	usedBytes, shippedConnections := 0, 0
	for _, candidate := range ordered {
		if usedBytes+candidate.estBytes > maxProcessTreeBytes {
			continue
		}
		processes[candidate.ref] = capTreeCopy(candidate.tree)
		usedBytes += candidate.estBytes
		shippedConnections += candidate.connections
	}
	// Derived, not accumulated in the loop above: correct whatever that loop does.
	droppedTrees := len(candidates) - len(processes)
	droppedConnections := totalConnections - shippedConnections

On 1000 over-budget candidates, both loop shapes then log treesShipped: 187; treesDropped: 813; connectionsWithoutTree: 813. As written today, a future break logs treesDropped: 0 while still dropping 813, and nothing asserts otherwise. Since the plan is "if this fires routinely, make the budget configurable rather than raising it blind", those two numbers are the whole decision input — making them independent of the loop body seems better than pinning the loop body.

4. The ref lookup is a new unconditional double-lock on the packet path

processRefFor runs before the dedup check, so it's on every network and DNS event including duplicates, and it takes ProcessTreeManagerImpl.mutex.RLock() then processTreeCreatorImpl.mutex.RLock() to read one map entry. ProcessTreeManagerImpl.ReportEvent write-locks that same manager mutex for every exec/fork/exit/procfs event. Before this change the network path never touched those locks at all — getProcessTreeByPid only inspects its argument.

Benchmarked the duplicate-connection path with a real ProcessTreeManagerImpl, 8 threads. The control is the identical benchmark with processRefFor short-circuited to nil, i.e. the pre-change hot path:

	ns/op
No process-tree churn, control	210
No process-tree churn, with ref lookup	244
4 goroutines of exec churn, control	254–271
4 goroutines of exec churn, with ref lookup	996–1035

Idle it's ~16%; under exec churn it's ~3.9× on that path. The cost is essentially all write-lock contention, so it lands hardest on exactly the fork-heavy nodes the tree budget was calibrated for (CI runners, cron shelling out). Inherent to the design since the batch key needs the ref — but a dedicated RWMutex for the pidStartTimeNs side map, or an atomic/sharded read, would keep packet handling off the process-tree writer's critical path.

---

Two smaller things while I was in there: processNodeOverheadBytes' comment says "~360 measured for a fully-populated node; generous on purpose" while the constant is 320 — I measured 176 actual bytes for a fully-populated node (every numeric field at max, strings empty), so the constant is genuinely generous and it's the 360 that's stale. And the tree budget bounds the wire but not the heap: with the process-aware key, networkEventsStorage retains one entry per process per endpoint for the whole 2-minute interval, each pinning a *ProcessTree. The residual note covers payload at ~3300 connections but not resident memory on the same shape.

Nice work on the escapedLen fix — the pre-fix estimator fails three named cases plus TestBuildWireStream_EscapeHeavyPayloadStaysUnderLimit, so that one is well pinned.

[AlonLiwsky]

Thanks @matthyx — this is a genuinely useful review, and the independent check on tree immutability (both MigrateToMap callers being no-ops for branch-built trees) is exactly the premise the whole outside-the-lock design rests on, so it's good to have that confirmed by someone else.

All four addressed in 24bf616f. Taking them in order:

1. Ranking order — accepted, your test added verbatim

You're right and I'd fooled myself. Reproduced exactly: inverting the comparator ships 34 instead of 126 and trips the displacement assertion, while TestSelectProcessTrees_PrefersSmallestTrees stays green — so it was pinning "the beacon survives", not the order. Two same-size populations was the missing ingredient. Landed as TestSelectProcessTrees_MaximisesTreesKept, with your min(maxProcessTreeBytes/smallEst, 200) derivation kept, since a computed expectation beats a magic number here.

2. Skip-vs-break is unreachable — accepted, the comment was wrong

Your proof holds: ascending sort ⇒ if candidate i misses then used + est_i > budget, and for j > i, est_j ≥ est_i while used never decreases, so nothing after the first miss can fit. I've kept the skip as cheap insurance against a future ordering change but stripped the claim that it's load-bearing, and renamed the test to TestSelectProcessTrees_OversizedTreeIsExcluded with an explicit note that it passes under either loop shape. A test whose name promises more than it delivers is worse than no test — that's the second one of mine you and CodeRabbit have caught between you, which is a useful signal about how I was writing them.

3. Derived counters — accepted, and this was the sharpest point

Your framing settled it: the counters are the decision input for "make the budget configurable rather than raising it blind", so they shouldn't depend on the loop body at all. Now derived as len(candidates) - len(processes) and totalConnections - shippedConnections. I extracted selectWithinBudget returning a budgetStats so they're assertable, and pinned the identities that must hold regardless of the loop: treesShipped + treesDropped == candidates, and connectionsWithoutTree following from the trees actually dropped.

4. Packet-path lock contention — accepted as real, deliberately not fixed here

Your numbers are the most consequential thing in the review, and I'm not going to hand-wave them: ~16% idle, ~3.9× under exec churn, landing hardest on precisely the fork-heavy nodes the budget was sized for.

Two things I'd add rather than dispute:

• The lookup itself is unavoidable in this design — the dedup key contains the ref, so the ref must exist before the key can be built. There's no reordering that makes it conditional on the dedup check.
• Caching the ref per pid across a flush interval, which is the obvious in-package mitigation, is not safe: a pid recycled mid-interval would inherit the dead process's start time, which is a confident falsehood in exactly the case the start time exists to prevent. So a cache trades the bug this field was added for against CPU. Not doing that.

That leaves your suggestion — a dedicated lock or atomic/sharded read for the creator's pidStartTimeNs side map — as the right fix, and it's in pkg/processtree/creator, which another workstream owns and is actively changing (exit-manager race, pid-reuse hardening). Touching it from here would collide. I've documented the measurement and your recommendation in docs/features/network-stream-process-attribution.md so it lands with the owner rather than being rediscovered under load. Happy to be told to fold it in here instead if you'd rather not split it.

The two smaller ones

• processNodeOverheadBytes: you're right that the comment is the stale part, not the constant. I measured 133 bytes for a node with every numeric at max and strings empty, plus ~16 for the childrenMap wrapper — so ~149 actual against a constant of 320. (Your 33-byte wrapper+key figure matches my processMapKeyOverheadBytes of 17 plus that ~16 exactly.) The comment now says 320 is deliberate headroom absorbing UniqueID and future fields, instead of pretending to be a measurement.
• Heap vs wire: correct, and it wasn't documented. The budget trims the payload on the way out but networkEventsStorage still retains one entry per (process, endpoint) for the whole interval, each pinning a tree. Capping that would mean dropping connections — the data loss this PR exists to fix — so the honest position is "unbounded and monitored", which the residual section now says explicitly rather than implying the budget covers it.

[matthyx]

Re-checked at 24bf616f. All four are addressed, and I verified the fixes rather than reading them — re-ran the mutation battery on the new head:

Mutation	on 86ea7a08	on 24bf616f
Ranking inverted to largest-first	survived	killed
treesDropped desynced from the result	—	killed
break instead of continue	survived	survived (now correct — see below)
escapedLen → len(s)	killed	killed
capTreeCopy → DeepCopy	killed	killed
Snapshot shares the live maps	killed	killed
StartTimeNs rescaled	killed	killed
Network key drops the ref	killed	killed

Build, go vet, gofmt and go test -race all clean, and nothing previously pinned regressed under the refactor.

On the break mutation still surviving: that's now the documented, correct state rather than a gap. I confirmed the point of the change holds — with the counters derived, continue and break both report treesShipped=187, treesDropped=813, connectionsWithoutTree=3252 on 1000 over-budget candidates, where the old accumulator reported 0 dropped under break. Renaming to TestSelectProcessTrees_OversizedTreeIsExcluded and spelling out what it does not prove is the honest fix.

Leaving the lock contention to pkg/processtree/creator seems right — it's that package's invariant to change, and documenting the measurement is what makes it actionable there.

I also reproduced the revised numbers independently: a 10-node realisticNode chain estimates 5633 B against 4345 B marshalled (29.6% over, vs the doc's ~33%), and the budget holds 465 of them (doc: ~461). Both check out. The ~149 overhead figure does too — I measure 138 B for a bare node with every numeric at max and strings empty, which is your 133 plus the timezone-offset form of startTime.

---

One correction, in the residual paragraph. This part isn't right as written:

the connection entries alone breach the message limit at roughly 3,000 connections … No tree budget can help there.

The 3,000 is correct, but it isn't entries alone — it's the combined payload with the tree budget saturated. Entries alone at ~530 B don't breach until ~7,400 connections: the 5 MiB limit is 3.75 MiB of raw JSON before base64, and 3,932,160 / 530 = 7,419. Your own 4,000 row is the check — 4.29 MiB total, of which entries are only 4,000 × 530 = 2.02 MiB, so the other 2.27 MiB is the saturated tree budget. Back that out and the remaining headroom is 3.75 − 2.27 = 1.48 MiB, i.e. ~2,930 entries. That's where the 3,000 comes from.

Which makes "No tree budget can help there" the part worth changing: trees occupy roughly 2.27 of the 3.75 MiB in that regime, so a tighter maxProcessTreeBytes moves the breach point out substantially — toward 7,400 as the tree budget approaches zero. Splitting the message is still the right long-term fix, but an operator staring at this during an incident would currently conclude the tree budget is not a lever, when it's about half the payload.

[AlonLiwsky]

You're right, and it's the useful kind of wrong — the number was fine and the advice attached to it wasn't. Fixed in 892df8b6.

I re-derived your decomposition rather than taking it: usable budget 3.75 MiB (5 MiB ÷ 4/3); at 4,000 connections entries are 4,000 × 530 = 2.02 MiB and the saturated trees ~1.91 MiB; back the trees out and 3.75 − 1.91 leaves room for 2,929 entries, which is the ~3,000. Entries alone: 3,932,160 ÷ 530 = 7,419. Both match yours.

The residual section now tabulates the two numbers with what each one means, and states plainly that the tree budget is a lever in that regime — trees are ~half the payload, tightening it moves the breach point out toward ~7,400, and it's the fastest thing to reach for during an incident since it needs no protocol change or backend coordination. Your operator framing is what made this worth changing rather than just rewording; thank you for putting it that way.

A second instance of the same error, which you didn't catch and I should have. The 4,000 row said 5.72 MiB. It double-counted the entries: the measured payload already contained ~94 B per synthetic entry, and I added a modelled 530 B on top, so I was charging ~624 B each. Honest figure is 3.93 MiB JSON → 5.24 MiB base64 — still over, so the row's verdict stands. Corrected, and the table now marks which figures are measured and which are modelled, because mixing the two without checking for overlap is precisely what produced both mistakes.

On splitting — one prerequisite worth settling before anyone starts. I've recorded it in the doc rather than leaving it implicit, because I think it's a trap for whoever picks up the ticket.

Your container-profile precedent works because storage's QueueManager does a Content-Length check and returns 413 synchronously — the sender learns immediately and can halve and retry. The network stream posts to HTTPExporterConfig.URL + "/v1/networkstreams", nothing in pkg/networkstream or pkg/exporters handles 413 at all (grepped: zero hits), and the 5 MiB cap is a broker limit whose enforcement point SUB-7850 explicitly lists as unverified:

Not verified: the production broker configs, and any namespace- or topic-level size policy.

So if the cap is enforced downstream at Pulsar rather than at the HTTP gateway, the sensor gets 200 OK, the message dies silently, and there is no signal to react to — reactive halving simply isn't available on this path, and the split has to be decided before the first send. Different design, same goal. Worth one question to a backend owner before any code.

Two smaller notes while I was in there:

• The natural split axis looks benign: cut on entity boundaries, and since a process belongs to exactly one container, its tree lands in exactly one chunk — no duplication. Trees only get duplicated if a single container is itself oversized and its connections have to straddle a cut.
• The precondition I'd want confirmed is that the ingester merges rows per message rather than replacing per message. The signals point that way (it logs "upserting network stream", and the traffic-view row identity carries no process field, which is why process-split entries already collapse), but if it replaces, splitting would make chunk 2 wipe chunk 1 — recreating the exact data loss this PR fixes. Not something to assume.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/features/network-stream-process-attribution.md`:
- Around line 321-325: Update the connection-breach arithmetic in the
network-stream-process-attribution documentation so the residual budget and
connection threshold are consistent with the stated 3.75 MiB, 1.91 MiB saturated
trees, and 530 B per entry values. Recompute the remaining budget from the
documented numbers, then either revise the ~3,000 threshold and related prose to
match that result or explicitly add the missing envelope overhead into the
formula; keep the threshold guidance aligned with the same calculation used near
the other ~3,000 references.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 41b1d78c-aa9a-4c4f-991b-5ede52153b72

📥 Commits

Reviewing files that changed from the base of the PR and between 24bf616 and 892df8b.

📒 Files selected for processing (1)

• docs/features/network-stream-process-attribution.md

[matthyx]

892df8b6 fixes the framing, and the new tree figure is right — but the threshold didn't move with it, and the ~3,000 was my bad arithmetic, not yours. CodeRabbit is correct to flag it.

I derived 2.27 MiB of saturated trees by subtracting modelled entries from the then-stated 4.29 MiB total, which was itself the inconsistent number. You've since measured trees properly at ~1.91 MiB. I measured it independently on this head and agree: 465 trees at budget saturation marshal to 2,033,576 B = 1.94 MiB.

Propagating that through your own formula:

usable JSON            = 5 MiB x 3/4        = 3,932,160 B  (3.75 MiB)
saturated trees        =                      2,033,576 B  (1.94 MiB, measured)
room left for entries  =                      1,898,584 B  (1.81 MiB)
                       / 530 B per entry    = ~3,580 connections

Using your 1.91 MiB rather than my measured 1.94 gives ~3,640. So the number is ~3,600, not ~3,000 — the 3.75 - 1.91 step leaves 1.84 MiB, which is ~3,640 entries, not ~2,900. The ~2,900 only follows from the old 2.27 MiB tree figure that the same commit correctly retired.

Three places to update: the table caption ("exceeds the limit at roughly 3,000 connections"), the residual table row, and the "which is where the ~3,000 comes from" sentence. The ~7,400 entries-alone figure is unaffected — I get 3,932,160 / 530 = 7,419.

Nothing else changed in 892df8b6 (docs only), so the code verification from my last pass stands, and go build ./... && go test -race ./pkg/networkstream/... is still clean on this head. The conclusion the paragraph now draws — that maxProcessTreeBytes is a real operator lever because trees are roughly half the payload — holds unchanged at ~3,600; it's only the threshold that's off.

[AlonLiwsky]

Good catch, and thanks for tracing it to the source rather than the symptom — you're right that 892df8b6 fixed the input and left the output stale. CodeRabbit landed on the same line independently.

Measured directly this time instead of deriving it from another derived figure, which is what produced this in the first place: a saturated tree map marshals to 1,978,505 B (1.89 MiB), leaving 3,932,160 − 1,978,505 = 1,953,655 B (1.86 MiB), so ~3,690 entries at 530 B. Your ~3,580 and CodeRabbit's ~3,600 land in the same band, so the doc now says ~3,600 — rounding low is the right direction for a breach threshold.

Fixed in 8bc4f41e, all three places: the table caption, the residual table row and the derivation sentence. I also wrote down the rule that produced both errors in this chain, since it's the actual lesson — derive the threshold from the measured tree total, never from a payload figure that already contains entries. That's what gave us a row counting entries twice, and then a threshold that outlived the figure it came from.

Unaffected, as you say: ~7,400 for entries alone (3,932,160 / 530 = 7,419), and the operator conclusion holds unchanged at ~3,600.