Fix Go lookup dispatch ceilings

Author: ktsaou

.agents/sow/current/SOW-0021-20260613-netipc-at-scale.md

@@ -1044,6 +1044,11 @@ Tests or equivalent validation:
   - Focused POSIX validation passed: `cmake --build build-coverage --target test_service -j12 && /usr/bin/ctest --test-dir build-coverage --output-on-failure -R '^test_service$'`; `cd src/go && go test -count=1 -timeout=300s ./pkg/netipc/service/raw -run '^TestLookupLargeResponseSplit$'`; `cargo test --manifest-path src/crates/netipc/Cargo.toml large_response_split -- --nocapture`.
   - Focused Windows validation passed on `win11`: `NIPC_TEST_FILTER=large_response_split timeout 600 build-windows-focused/bin/test_win_service_extra.exe`; `cd src/go && "/c/Program Files/Go/bin/go.exe" test -count=1 -timeout=300s ./pkg/netipc/service/raw -run "^TestWinLookupLargeResponseSplit$"`; `/c/Users/costa/.cargo/bin/cargo.exe test --manifest-path src/crates/netipc/Cargo.toml large_response_split -- --nocapture`.
   - Protocol-level validation also passed: C `test_protocol`, Go `./pkg/netipc/protocol`, and Rust `protocol::lookup` tests.
+- Post-push Go reviewer fixes:
+  - External review found that Go zero-config raw servers treated the learned `MaxPayloadDefault` starting capacity as the growth ceiling, so default request/response payload learning stopped at `1024` bytes instead of the documented `MaxPayloadCap`. Fixed `src/go/pkg/netipc/service/raw/server.go` to compute growth ceilings from the original initialization config before replacing zero budgets with starting defaults.
+  - External review found that Go raw APPS_LOOKUP and CGROUPS_LOOKUP dispatch duplicated the protocol dispatch path and missed the protocol layer's `Finish()==0` overflow guard. Fixed `src/go/pkg/netipc/service/raw/apps_lookup.go` and `src/go/pkg/netipc/service/raw/cgroups_lookup.go` to delegate to protocol dispatchers while preserving raw-service `errHandlerFailed` semantics for typed handler failure.
+  - Removed the now-unused Go raw-service `lookupMinRequired()` helper after the protocol dispatchers became the single production path for lookup response minimum-size checks.
+  - Validation passed: POSIX `cd src/go && go test -count=1 -timeout=300s ./pkg/netipc/service/raw`; POSIX `cd src/go && go test -count=1 -timeout=300s ./pkg/netipc/protocol`; Windows `cd src/go && "/c/Program Files/Go/bin/go.exe" test -count=1 -timeout=300s ./pkg/netipc/service/raw`; Windows focused raw lookup/defaults tests also passed on `win11`.
 - Note: `ctest` on `$PATH` resolved to a broken local Python wrapper missing the `cmake` module; validation used `/usr/bin/ctest`.
 
 Real-use evidence:
@@ -1059,6 +1064,9 @@ Reviewer findings:
 - External reviewer finding: local oversized request-key synthesis could run before proving the client is connected. Handled by enforcing `READY` before logical lookup work in C, Go, and Rust.
 - External reviewer finding: cgroups all-local oversized handling could skip the zero-item probe path. Handled by continuing after a local oversized item only when more request items remain; otherwise the client sends the zero-item request and validates endpoint/generation behavior.
 - External reviewer concern: hidden fixed payload ceilings would contradict initialization-tunable budgets. Reviewed against code and docs. `NIPC_MAX_PAYLOAD_CAP` / `MaxPayloadCap` remains a named default growth ceiling; request/response payload budgets are exposed through initialization config. Server learned-capacity growth now also honors those configured ceilings.
+- External reviewer finding: Go zero-config raw-server growth ceilings were derived after applying `MaxPayloadDefault`, effectively preventing default servers from learning above `1024` bytes. Handled by deriving growth ceilings from the original config and validating default versus explicit ceiling behavior.
+- External reviewer finding: Go raw lookup dispatchers duplicated protocol dispatcher logic and could diverge from protocol overflow handling. Handled by delegating raw APPS_LOOKUP and CGROUPS_LOOKUP dispatch to protocol dispatchers, preserving raw typed-handler failure semantics, and deleting the now-dead local minimum-size helper.
+- One requested external review run timed out before returning a usable final report. The completed reviewer findings above were triaged against the codebase; concrete defects were fixed and validated, while non-blocking style or broader design suggestions remain subject to the existing SOW close gates.
 - External reviewer finding: coverage scripts and broader adversarial matrix still need updates before SOW completion. Coverage script expansion is now implemented; C/Rust/Go coverage gates pass; representative `8192` and `32768` logical-call tests now pass in C/Rust/Go on POSIX and Windows; POSIX and Windows baseline/SHM lookup-scale interop now pass across all C/Rust/Go directed pairs, including mixed-status lookup cases and heavier `65536` stress-only runs; lookup status codec interop now proves `PAYLOAD_EXCEEDED` and `OVERSIZED_ITEM` wire parity across C/Rust/Go; Rust malformed typed lookup response parity is now covered on POSIX and Windows; malformed follow-up responses after partial progress are now covered in C/Rust/Go on POSIX and Windows; reordered and duplicate response-item corruption is now covered in C/Rust/Go on POSIX and Windows; invalid status enum, invalid status-dependent field, and invalid label-table corruption are now covered in C/Rust/Go on POSIX and Windows; huge valid label isolation is now covered in C/Rust/Go on POSIX and Windows; endpoint absence before call, endpoint disappearance after partial progress, and endpoint disappearance before the first subcall are now covered in C/Rust/Go on POSIX and Windows; zero-item typed lookup calls are now covered in C/Rust/Go on POSIX and Windows; duplicate and unsorted request keys under request splitting are now covered in C/Rust/Go on POSIX and Windows; full POSIX and Windows benchmark regenerations now pass; downstream topology-containers post-vendor validation now passes. Broader adversarial matrix review remains open.
 
 Same-failure scan:

src/go/pkg/netipc/service/raw/apps_lookup.go

@@ -204,34 +204,17 @@ func AppsLookupDispatch(handle AppsLookupHandler) DispatchHandler {
 		return nil
 	}
 	return func(request []byte, responseBuf []byte) (int, error) {
-		req, err := protocol.DecodeAppsLookupRequest(request)
-		if err != nil {
-			return 0, err
-		}
-		minRequired, err := lookupMinRequired(protocol.AppsLookupRespHdr, req.ItemCount)
-		if err != nil {
-			return 0, err
-		}
-		if len(responseBuf) < minRequired {
-			return 0, protocol.ErrOverflow
-		}
-		builder := protocol.NewAppsLookupBuilder(responseBuf, req.ItemCount, 0)
-		if req.ItemCount > 0 {
-			lens := make([]int, req.ItemCount)
-			for i := range lens {
-				lens[i] = protocol.AppsLookupItemHdr + 3
-			}
-			builder.SetPayloadExceededItemLens(lens)
-		}
-		if !handle(req, builder) {
+		handlerFailed := false
+		n, err := protocol.DispatchAppsLookup(request, responseBuf, func(req *protocol.AppsLookupRequestView, builder *protocol.AppsLookupBuilder) bool {
+			ok := handle(req, builder)
+			if !ok && builder.Error() == nil {
+				handlerFailed = true
+			}
+			return ok
+		})
+		if handlerFailed && errors.Is(err, protocol.ErrBadLayout) {
 			return 0, errHandlerFailed
 		}
-		if builder.Error() != nil {
-			return 0, builder.Error()
-		}
-		if builder.ItemCount() != req.ItemCount {
-			return 0, protocol.ErrBadItemCount
-		}
-		return builder.Finish(), nil
+		return n, err
 	}
 }

src/go/pkg/netipc/service/raw/cgroups_lookup.go

@@ -2,6 +2,7 @@ package raw
 
 import (
 	"bytes"
+	"errors"
 
 	"github.com/netdata/plugin-ipc/go/pkg/netipc/protocol"
 )
@@ -268,39 +269,17 @@ func CgroupsLookupDispatch(handle CgroupsLookupHandler) DispatchHandler {
 		return nil
 	}
 	return func(request []byte, responseBuf []byte) (int, error) {
-		req, err := protocol.DecodeCgroupsLookupRequest(request)
-		if err != nil {
-			return 0, err
-		}
-		minRequired, err := lookupMinRequired(protocol.CgroupsLookupRespHdr, req.ItemCount)
-		if err != nil {
-			return 0, err
-		}
-		if len(responseBuf) < minRequired {
-			return 0, protocol.ErrOverflow
-		}
-		builder := protocol.NewCgroupsLookupBuilder(responseBuf, req.ItemCount, 0)
-		if req.ItemCount > 0 {
-			lens := make([]int, req.ItemCount)
-			for i := range lens {
-				item, err := req.Item(uint32(i)) // #nosec G115 -- i is bounded by req.ItemCount from the decoded uint32 header.
-				if err != nil {
-					return 0, err
-				}
-				size := protocol.CgroupsLookupItemHdr + len(item.Bytes()) + 2
-				lens[i] = size
+		handlerFailed := false
+		n, err := protocol.DispatchCgroupsLookup(request, responseBuf, func(req *protocol.CgroupsLookupRequestView, builder *protocol.CgroupsLookupBuilder) bool {
+			ok := handle(req, builder)
+			if !ok && builder.Error() == nil {
+				handlerFailed = true
 			}
-			builder.SetPayloadExceededItemLens(lens)
-		}
-		if !handle(req, builder) {
+			return ok
+		})
+		if handlerFailed && errors.Is(err, protocol.ErrBadLayout) {
 			return 0, errHandlerFailed
 		}
-		if builder.Error() != nil {
-			return 0, builder.Error()
-		}
-		if builder.ItemCount() != req.ItemCount {
-			return 0, protocol.ErrBadItemCount
-		}
-		return builder.Finish(), nil
+		return n, err
 	}
 }

src/go/pkg/netipc/service/raw/lookup_common.go

@@ -147,18 +147,6 @@ func checkedLookupU32(value int) (uint32, error) {
 	return uint32(value), nil // #nosec G115 -- value is bounded by the uint32 maximum above.
 }
 
-func lookupMinRequired(hdrSize int, itemCount uint32) (int, error) {
-	if hdrSize < 0 {
-		return 0, protocol.ErrOverflow
-	}
-	dirSize64 := uint64(itemCount) * uint64(protocol.LookupDirEntrySize)
-	min64 := uint64(hdrSize) + dirSize64
-	if min64 > uint64(int(^uint(0)>>1)) {
-		return 0, protocol.ErrOverflow
-	}
-	return int(min64), nil
-}
-
 func cloneLookupRawItem(item []byte) []byte {
 	cloned := make([]byte, len(item))
 	copy(cloned, item)

src/go/pkg/netipc/service/raw/lookup_common_test.go

@@ -68,12 +68,6 @@ func TestLookupCommonSizingHelpers(t *testing.T) {
 	if _, err := checkedLookupU32(int(uint64(^uint32(0)) + 1)); !errors.Is(err, protocol.ErrOverflow) {
 		t.Fatalf("checkedLookupU32 overflow error = %v", err)
 	}
-	if _, err := lookupMinRequired(-1, 1); !errors.Is(err, protocol.ErrOverflow) {
-		t.Fatalf("lookupMinRequired negative error = %v", err)
-	}
-	if _, err := lookupMinRequired(maxInt, 1); !errors.Is(err, protocol.ErrOverflow) {
-		t.Fatalf("lookupMinRequired overflow error = %v", err)
-	}
 }
 
 func TestLookupNextRequestSizingEdges(t *testing.T) {

src/go/pkg/netipc/service/raw/server.go

@@ -29,6 +29,8 @@ func (s *Server) initCommon(
 	if workerCount < 1 {
 		workerCount = 1
 	}
+	requestPayloadGrowthCeiling := payloadGrowthCeiling(maxRequestPayloadBytes)
+	responsePayloadGrowthCeiling := payloadGrowthCeiling(maxResponsePayloadBytes)
 	if maxRequestPayloadBytes == 0 {
 		maxRequestPayloadBytes = protocol.MaxPayloadDefault
 	}
@@ -41,8 +43,8 @@ func (s *Server) initCommon(
 	s.expectedMethodCode = expectedMethodCode
 	s.handler = handler
 	s.workerCount = workerCount
-	s.requestPayloadGrowthCeiling = payloadGrowthCeiling(maxRequestPayloadBytes)
-	s.responsePayloadGrowthCeiling = payloadGrowthCeiling(maxResponsePayloadBytes)
+	s.requestPayloadGrowthCeiling = requestPayloadGrowthCeiling
+	s.responsePayloadGrowthCeiling = responsePayloadGrowthCeiling
 	s.learnedRequestPayloadBytes.Store(maxRequestPayloadBytes)
 	s.learnedResponsePayloadBytes.Store(maxResponsePayloadBytes)
 }

src/go/pkg/netipc/service/raw/server_unit_test.go

@@ -93,6 +93,32 @@ func TestServerCommonInitDefaultsAndWorkerSlotGuards(t *testing.T) {
 			protocol.MaxPayloadDefault,
 			protocol.MaxPayloadDefault)
 	}
+	if server.requestPayloadGrowthCeiling != protocol.MaxPayloadCap ||
+		server.responsePayloadGrowthCeiling != protocol.MaxPayloadCap {
+		t.Fatalf("default growth ceilings = %d/%d, want %d/%d",
+			server.requestPayloadGrowthCeiling,
+			server.responsePayloadGrowthCeiling,
+			protocol.MaxPayloadCap,
+			protocol.MaxPayloadCap)
+	}
+
+	var configured Server
+	configured.initCommon("run", "svc", protocol.MethodIncrement, nil, 4, 4096, 8192)
+	if configured.workerCount != 4 {
+		t.Fatalf("configured worker count = %d, want 4", configured.workerCount)
+	}
+	if configured.learnedRequestPayloadBytes.Load() != 4096 ||
+		configured.learnedResponsePayloadBytes.Load() != 8192 {
+		t.Fatalf("configured learned capacities = %d/%d, want 4096/8192",
+			configured.learnedRequestPayloadBytes.Load(),
+			configured.learnedResponsePayloadBytes.Load())
+	}
+	if configured.requestPayloadGrowthCeiling != 4096 ||
+		configured.responsePayloadGrowthCeiling != 8192 {
+		t.Fatalf("configured growth ceilings = %d/%d, want 4096/8192",
+			configured.requestPayloadGrowthCeiling,
+			configured.responsePayloadGrowthCeiling)
+	}
 
 	var cleanupCalled atomic.Bool
 	if server.retryAcceptAfter(func() { cleanupCalled.Store(true) }) {