Address netipc review findings

Author: ktsaou

.agents/sow/current/SOW-0015-20260605-codacy-scope-and-maintainability.md

@@ -544,6 +544,55 @@ Plan:
 7. Validate Netdata build/tests for the vendored NetIPC surface, including Windows compile/unit validation if the Netdata workflow exposes it locally.
 8. Commit only explicit Netdata vendor/update files, push the branch, and open a PR against Netdata.
 
+### 2026-06-07 - PR 22649 Review And SonarCloud Follow-up
+
+Netdata PR #22649 review context was checked before editing:
+
+- GitHub review threads had two unresolved, non-outdated, actionable findings:
+  - `src/crates/netipc/src/service/raw/server.rs:95` equivalent vendored Windows raw server shutdown risk: only the initially stored listener handle was available to `stop()`, while later accepted pipe instances could keep blocking.
+  - `src/go/pkg/netipc/service/raw/cgroups_lookup.go:123` service dispatch had a dead `builder.Finish() == 0` check after builder error and item-count checks.
+- Same-pattern service dispatch review was checked in Go raw apps lookup; the same dead service-level guard existed there.
+- Protocol-level Go lookup dispatch was also checked. The guard looked similar, but `cd src/go && go test -count=1 ./...` proved it is required: `TestLookupDispatchCoverage` intentionally corrupts builder state and expects `ErrOverflow`. The protocol-level guards were restored.
+- SonarCloud PR query reported six open issues:
+  - Go parameter-count findings in `src/go/pkg/netipc/protocol/apps_lookup.go`.
+  - Go unnecessary recovered variable findings in `src/go/pkg/netipc/service/raw/server_unix.go` and `src/go/pkg/netipc/service/raw/server_windows.go`.
+  - C `openat()` descriptor finding in `src/libnetdata/netipc/src/transport/posix/netipc_shm.c`.
+  - C memcpy bounds finding in `src/libnetdata/netipc/src/transport/posix/netipc_uds_receive.c`.
+- GitHub AI quality findings also reported:
+  - ignored `clock_gettime` syscall result in `bench/drivers/go/main.go`.
+  - Go integer `range` compatibility concerns in Go service/raw and stale-dial code.
+  - missing rationale around the Go UDS 32-bit total-length limit.
+  - maintainability issues in `vendor-to-netdata.sh`.
+
+Implemented SDK follow-up:
+
+- Aligned all plugin-ipc Go modules to Netdata's Go version, `go 1.26.0`.
+- Ran Go's `go fix` modernization on the affected Go module surfaces, including benchmark driver loop and slice cleanups.
+- Changed Go benchmark CPU-time helper to check the `clock_gettime` syscall error path and return zero on failure.
+- Added the missing Go UDS comment explaining the protocol's 32-bit framed total-length contract.
+- Hardened `vendor-to-netdata.sh`:
+  - validates source `go.mod` presence and parses module paths from the `module` directive.
+  - avoids fragile status-line spacing.
+  - counts only relevant C, Rust, and Go source files.
+  - prints an import-rewrite review command after the suggested `sed` command.
+- Updated Rust Windows raw server to refresh the stored listener handle around each accept and clear it before joining session threads, so `stop()` can target the currently blocking listener handle.
+- Removed dead service-level Go lookup dispatch `builder.Finish() == 0` checks in raw apps lookup and raw cgroups lookup.
+- Reduced SonarCloud Go parameter count in apps lookup semantic validators by passing a private `appsLookupSemantics` struct.
+- Removed unnecessary recovered-value variables from Go raw server panic-recovery paths.
+- Avoided `openat()` with an invalid directory descriptor in POSIX SHM stale cleanup.
+- Added explicit chunk and first-packet bounds checks in POSIX UDS receive before copying chunk payload data into the assembled receive buffer.
+
+Validation completed for this follow-up:
+
+- `cd bench/drivers/go && go fix ./... && go test ./...`: passed.
+- `git diff --check`: passed.
+- `cd src/go && go test -count=1 ./...`: passed.
+- `cargo test --manifest-path src/crates/netipc/Cargo.toml`: 332 tests passed.
+- `cmake --build build`: passed.
+- `/usr/bin/ctest --test-dir build --output-on-failure`: 46/46 tests passed.
+- Win11 temp-copy Rust validation: `cargo test --manifest-path src/crates/netipc/Cargo.toml service::raw -- --test-threads=1`: 22 Windows raw-service tests passed.
+- Win11 temp-copy Go validation: `cd src/go && CGO_ENABLED=0 go test -count=1 ./pkg/netipc/service/raw ./pkg/netipc/transport/windows`: passed.
+
 ## Validation
 
 Acceptance criteria evidence:
@@ -555,6 +604,8 @@ Acceptance criteria evidence:
   - send chunk count no longer forms `(remaining + chunk_payload_budget - 1)`.
 - Same-pattern C transport issue was addressed in `src/libnetdata/netipc/src/transport/windows/netipc_named_pipe.c`.
 - Vendor script now copies the full vendored C include/source subtrees so split C files are not missed.
+- Netdata PR #22649 review and SonarCloud findings were verified against SDK source and addressed in the SDK before re-vendoring.
+- Plugin-ipc Go modules now match Netdata's Go version, `go 1.26.0`.
 
 Tests or equivalent validation:
 
@@ -563,6 +614,9 @@ Tests or equivalent validation:
 - `cargo test --manifest-path src/crates/netipc/Cargo.toml`: 332 passed.
 - `cd src/go && go test ./...`: passed.
 - `cd bench/drivers/go && go test ./...`: passed.
+- `cd src/go && go test -count=1 ./...`: passed after restoring the required protocol-level lookup dispatch guard.
+- Win11 temp-copy Rust raw-service validation: 22 tests passed.
+- Win11 temp-copy Go service/raw and transport/windows validation: passed.
 - `git diff --check`: passed.
 - `bash -n tests/run-windows-bench.sh tests/test_windows_bench_stability_policy.sh vendor-to-netdata.sh`: passed.
 - `codacy-analysis analyze --files ... --output-format json`: 0 issues; known local Revive adapter invocation error remains.
@@ -576,13 +630,17 @@ Real-use evidence:
 Reviewer findings:
 
 - GitHub AI findings for `netipc_uds_send.c` were manually verified and addressed.
+- GitHub review-thread findings from Netdata PR #22649 were manually verified and addressed where they were real.
+- SonarCloud PR findings for Netdata PR #22649 were queried and addressed in the SDK source before re-vendoring.
 - No external AI reviewer was used for this increment.
 
 Same-failure scan:
 
 - Same C total-message-width helper pattern was found and fixed in the Windows named-pipe transport.
 - Same chunk-count overflow-prone ceil pattern was found and fixed in POSIX C, Windows C, POSIX Rust, Windows Rust, POSIX Go, and Windows Go transport paths.
 - Same benchmark batch sizing nondeterminism pattern was found and fixed in POSIX and Windows Go benchmark drivers.
+- Same dead Go service-level lookup dispatch guard was found and removed in apps lookup and cgroups lookup.
+- Similar Go protocol-level lookup dispatch guards were tested and kept because regression coverage proves they still guard an overflow state.
 
 Sensitive data gate:
 

bench/drivers/go/go.mod

@@ -1,6 +1,6 @@
 module github.com/netdata/plugin-ipc/bench/drivers/go
 
-go 1.25.11
+go 1.26.0
 
 require github.com/netdata/plugin-ipc/go v0.0.0
 

bench/drivers/go/main.go

@@ -15,7 +15,7 @@ import (
 	"os"
 	"os/signal"
 	"runtime"
-	"sort"
+	"slices"
 	"strconv"
 	"strings"
 	"sync/atomic"
@@ -67,7 +67,10 @@ func cacheHashName(name string) uint32 {
 func cpuNS() uint64 {
 	var ts syscall.Timespec
 	// CLOCK_PROCESS_CPUTIME_ID = 2 on Linux
-	_, _, _ = syscall.Syscall(syscall.SYS_CLOCK_GETTIME, 2, uintptr(unsafe.Pointer(&ts)), 0) // #nosec G103 -- clock_gettime requires passing a Timespec pointer.
+	_, _, errno := syscall.Syscall(syscall.SYS_CLOCK_GETTIME, 2, uintptr(unsafe.Pointer(&ts)), 0) // #nosec G103 -- clock_gettime requires passing a Timespec pointer.
+	if errno != 0 {
+		return 0
+	}
 	if ts.Sec < 0 || ts.Nsec < 0 {
 		return 0
 	}
@@ -161,7 +164,7 @@ func (lr *latencyRecorder) percentile(pct float64) uint64 {
 	if len(lr.samples) == 0 {
 		return 0
 	}
-	sort.Slice(lr.samples, func(i, j int) bool { return lr.samples[i] < lr.samples[j] })
+	slices.Sort(lr.samples)
 	idx := int(pct / 100.0 * float64(len(lr.samples)-1))
 	if idx >= len(lr.samples) {
 		idx = len(lr.samples) - 1
@@ -291,7 +294,7 @@ func initSnapshotTemplate() bool {
 
 	snapshotNames = make([][]byte, 16)
 	snapshotPaths = make([][]byte, 16)
-	for i := uint32(0); i < 16; i++ {
+	for i := range uint32(16) {
 		name := fmt.Sprintf("cgroup-%d", i)
 		path := fmt.Sprintf("/sys/fs/cgroup/bench/cg-%d", i)
 		snapshotNames[i] = []byte(name)
@@ -313,7 +316,7 @@ func snapshotHandler() cgroups.Handler {
 				return false
 			}
 			builder.SetHeader(1, atomic.AddUint64(&snapshotGen, 1))
-			for i := uint32(0); i < 16; i++ {
+			for i := range uint32(16) {
 				if err := builder.Add(1000+i, 0, i%2, snapshotNames[i], snapshotPaths[i]); err != nil {
 					return false
 				}
@@ -426,7 +429,7 @@ func runBatchPingPongClient(runDir, service string, profiles uint32, durationSec
 	cfg := batchClientConfig(profiles)
 	session, err := posix.Connect(runDir, service, &cfg)
 	if err != nil {
-		for i := 0; i < 200; i++ {
+		for range 200 {
 			time.Sleep(10 * time.Millisecond)
 			session, err = posix.Connect(runDir, service, &cfg)
 			if err == nil {
@@ -444,7 +447,7 @@ func runBatchPingPongClient(runDir, service string, profiles uint32, durationSec
 	var shm *posix.ShmContext
 	if session.SelectedProfile == protocol.ProfileSHMHybrid ||
 		session.SelectedProfile == protocol.ProfileSHMFutex {
-		for i := 0; i < 200; i++ {
+		for range 200 {
 			shmCtx, serr := posix.ShmClientAttach(runDir, service, session.SessionID)
 			if serr == nil {
 				shm = shmCtx
@@ -490,7 +493,7 @@ func runBatchPingPongClient(runDir, service string, profiles uint32, durationSec
 		bb.Reset(reqBuf, batchSize)
 
 		buildOK := true
-		for i := uint32(0); i < batchSize; i++ {
+		for i := range batchSize {
 			val := counter + uint64(i)
 			protocol.IncrementEncode(val, itemBuf)
 			expected[i] = val + 1
@@ -578,7 +581,7 @@ func runBatchPingPongClient(runDir, service string, profiles uint32, durationSec
 					batchOK = false
 				}
 			} else {
-				for i := uint32(0); i < batchSize; i++ {
+				for i := range batchSize {
 					item, gerr := protocol.BatchItemGet(respPayload, batchSize, i)
 					if gerr != nil {
 						errors++
@@ -635,7 +638,7 @@ func runBatchPingPongClient(runDir, service string, profiles uint32, durationSec
 					batchOK = false
 				}
 			} else {
-				for i := uint32(0); i < batchSize; i++ {
+				for i := range batchSize {
 					item, gerr := protocol.BatchItemGet(payload, batchSize, i)
 					if gerr != nil {
 						errors++
@@ -693,7 +696,7 @@ func runPipelineClient(runDir, service string, durationSec int, targetRPS uint64
 	cfg := clientConfig(profileUDS)
 	session, err := posix.Connect(runDir, service, &cfg)
 	if err != nil {
-		for i := 0; i < 200; i++ {
+		for range 200 {
 			time.Sleep(10 * time.Millisecond)
 			session, err = posix.Connect(runDir, service, &cfg)
 			if err == nil {
@@ -727,7 +730,7 @@ func runPipelineClient(runDir, service string, durationSec int, targetRPS uint64
 
 		// Send `depth` requests with unique message IDs
 		sendOK := true
-		for d := 0; d < depth; d++ {
+		for d := range depth {
 			val := counter + uint64(d)
 			binary.NativeEndian.PutUint64(reqPayload[:], val)
 
@@ -753,7 +756,7 @@ func runPipelineClient(runDir, service string, durationSec int, targetRPS uint64
 		}
 
 		// Receive `depth` responses
-		for d := 0; d < depth; d++ {
+		for d := range depth {
 			_, payload, rerr := session.Receive(recvBuf)
 			if rerr != nil {
 				errors++
@@ -812,7 +815,7 @@ func runPipelineBatchClient(runDir, service string, durationSec int, targetRPS u
 	cfg := batchClientConfig(profileUDS)
 	session, err := posix.Connect(runDir, service, &cfg)
 	if err != nil {
-		for i := 0; i < 200; i++ {
+		for range 200 {
 			time.Sleep(10 * time.Millisecond)
 			session, err = posix.Connect(runDir, service, &cfg)
 			if err == nil {
@@ -852,14 +855,14 @@ func runPipelineBatchClient(runDir, service string, durationSec int, targetRPS u
 
 		// Build and send `depth` batch requests
 		sendOK := true
-		for d := 0; d < depth; d++ {
+		for d := range depth {
 			bs := randomBatchSize(&rng)
 			batchSizes[d] = bs
 
 			var bb protocol.BatchBuilder
 			bb.Reset(reqBufs[d], bs)
 
-			for i := uint32(0); i < bs; i++ {
+			for i := range bs {
 				protocol.IncrementEncode(counter+uint64(i), itemBuf)
 				if err := bb.Add(itemBuf); err != nil {
 					sendOK = false
@@ -897,7 +900,7 @@ func runPipelineBatchClient(runDir, service string, durationSec int, targetRPS u
 		}
 
 		// Receive `depth` batch responses
-		for d := 0; d < depth; d++ {
+		for d := range depth {
 			_, _, rerr := session.Receive(recvBuf)
 			if rerr != nil {
 				errors++
@@ -941,7 +944,7 @@ func runPingPongClient(runDir, service string, profiles uint32, durationSec int,
 	session, err := posix.Connect(runDir, service, &cfg)
 	if err != nil {
 		// Retry
-		for i := 0; i < 200; i++ {
+		for range 200 {
 			time.Sleep(10 * time.Millisecond)
 			session, err = posix.Connect(runDir, service, &cfg)
 			if err == nil {
@@ -959,7 +962,7 @@ func runPingPongClient(runDir, service string, profiles uint32, durationSec int,
 	var shm *posix.ShmContext
 	if session.SelectedProfile == protocol.ProfileSHMHybrid ||
 		session.SelectedProfile == protocol.ProfileSHMFutex {
-		for i := 0; i < 200; i++ {
+		for range 200 {
 			shmCtx, serr := posix.ShmClientAttach(runDir, service, session.SessionID)
 			if serr == nil {
 				shm = shmCtx
@@ -1093,7 +1096,7 @@ func runPingPongClient(runDir, service string, profiles uint32, durationSec int,
 func runSnapshotClient(runDir, service string, profiles uint32, durationSec int, targetRPS uint64, scenario, serverLang string) int {
 	client := cgroups.NewClient(runDir, service, typedClientConfig(profiles))
 
-	for i := 0; i < 200; i++ {
+	for range 200 {
 		client.Refresh()
 		if client.Ready() {
 			break
@@ -1315,8 +1318,8 @@ func runLookupMethodBench(durationSec int, scenario string, targetRPS uint64) in
 
 	paths := make([][]byte, itemCount)
 	pids := make([]uint32, itemCount)
-	for i := 0; i < itemCount; i++ {
-		paths[i] = []byte(fmt.Sprintf("/sys/fs/cgroup/bench/cg-%03d", i))
+	for i := range itemCount {
+		paths[i] = fmt.Appendf(nil, "/sys/fs/cgroup/bench/cg-%03d", i)
 		pids[i] = uint32(1000 + i)
 	}
 

src/crates/netipc/src/service/raw/server_windows.rs

@@ -18,7 +18,7 @@ impl ManagedServer {
         )
         .map_err(|_| NipcError::BadLayout)?;
 
-        *self.listener_handle.lock().unwrap() = Some(listener.handle() as usize);
+        self.remember_windows_listener_handle(&listener);
 
         self.running.store(true, Ordering::Release);
 
@@ -31,7 +31,11 @@ impl ManagedServer {
                 continue;
             }
 
-            let session = match listener.accept_with_config(session_id, accept_cfg) {
+            self.remember_windows_listener_handle(&listener);
+            let accepted = listener.accept_with_config(session_id, accept_cfg);
+            self.remember_windows_listener_handle(&listener);
+
+            let session = match accepted {
                 Ok(s) => s,
                 Err(_) => {
                     if let Some(mut prepared) = prepared_shm {
@@ -82,13 +86,25 @@ impl ManagedServer {
             session_threads.push(t);
         }
 
+        *self.listener_handle.lock().unwrap() = None;
+
         for t in session_threads {
             let _ = t.join();
         }
 
         Ok(())
     }
 
+    fn remember_windows_listener_handle(&self, listener: &NpListener) {
+        let handle = listener.handle();
+        let stored = if handle == 0 || handle == -1 {
+            None
+        } else {
+            Some(handle as usize)
+        };
+        *self.listener_handle.lock().unwrap() = stored;
+    }
+
     fn prepare_windows_accept(&mut self) -> (u64, ServerConfig, Option<PreparedWinShm>, bool) {
         let session_id = self.next_session_id;
         self.next_session_id += 1;

src/go/go.mod

@@ -1,3 +1,3 @@
 module github.com/netdata/plugin-ipc/go
 
-go 1.25.11
+go 1.26.0