Address static analysis cleanup findings

Author: ktsaou

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

@@ -424,6 +424,45 @@ Open decisions:
   - `codacy-analysis analyze . --output-format json`: exit status 0, 0 issues, 1 known Revive adapter invocation error:
     - Revive error: `Failed to run revive: findings is not iterable`.
 
+### 2026-06-06 - GitHub AI Findings Triage
+
+- Opened `https://github.com/netdata/plugin-ipc/security/quality/ai-findings` with the authenticated Playwright browser.
+- GitHub displayed 15 AI findings across 5 files, not 5 individual findings:
+  - `src/crates/netipc/src/service/raw/server_windows.rs`: 2 findings.
+  - `src/go/pkg/netipc/protocol/lookup_common.go`: 5 findings.
+  - `src/go/pkg/netipc/transport/posix/uds_stale.go`: 3 findings.
+  - `src/go/pkg/netipc/transport/windows/pipe_session.go`: 2 findings.
+  - `src/libnetdata/netipc/src/transport/posix/netipc_uds_send.c`: 3 findings.
+- Local verification found stale or false-positive protocol findings:
+  - `src/go/pkg/netipc/protocol/lookup_common.go:19` already has the `LookupLabelView` doc comment.
+  - `src/go/pkg/netipc/protocol/lookup_common.go:452` already uses `count := int(itemCount)` after the prior overflow check.
+  - `src/go/pkg/netipc/protocol/frame.go:75` defines package-level `ne = binary.NativeEndian`.
+  - `src/go/pkg/netipc/protocol/frame.go:100` defines package-level `Align8`.
+- Accepted real low-risk findings:
+  - Simplify redundant Rust Windows SHM finalization match.
+  - Document the Rust Windows SHM cleanup order.
+  - Add a doc comment for unexported Go `maxIntValue` to satisfy the scanner without behavior change.
+  - Cache effective UID before POSIX stale-unlink directory stat.
+  - Use a bounded UDS stale-candidate dial timeout and retry transient `ECONNREFUSED` before unlinking.
+  - Add idiomatic Windows Go `Role()` accessors while preserving deprecated `GetRole()` wrappers.
+  - Replace the Windows named-pipe spin-loop magic number with a named constant.
+  - Split C UDS send compound conditions and local min expressions without changing send behavior.
+- Same-pattern search found `src/go/pkg/netipc/transport/windows/shm.go:163` also exposed only `GetRole()` while POSIX SHM exposes `Role()`; added the same additive `Role()` API there and kept `GetRole()` for compatibility.
+- Validation for this AI-findings pass:
+  - `gofmt` on touched Go files: passed.
+  - `cargo fmt --manifest-path src/crates/netipc/Cargo.toml`: passed.
+  - `git diff --check`: passed.
+  - `cd src/go && go test ./pkg/netipc/protocol ./pkg/netipc/transport/posix`: passed.
+  - `cd src/go && go test ./...`: passed.
+  - `cd src/go && GOOS=windows GOARCH=amd64 go test -c -o /tmp/netipc-transport-windows.test.exe ./pkg/netipc/transport/windows`: passed.
+  - `ssh win11 'cd /tmp/plugin-ipc-ai-findings/src/go && MSYSTEM=MSYS go test ./pkg/netipc/transport/windows'`: passed.
+  - `cargo test --manifest-path src/crates/netipc/Cargo.toml`: passed, 332 Rust unit tests.
+  - `cmake --build build`: passed.
+  - `/usr/bin/ctest --test-dir build --output-on-failure`: passed, 46/46 tests.
+  - `bash .agents/sow/audit.sh`: passed.
+  - `codacy-analysis analyze --files ... --output-format json`: exit status 0, 0 issues, 1 known Revive adapter error:
+    - Revive error: `Failed to run revive: findings is not iterable`.
+
 ## Validation
 
 Acceptance criteria evidence:

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

@@ -54,16 +54,14 @@ impl ManagedServer {
                 continue;
             }
 
-            let shm = match self.finalize_windows_shm(&session, prepared_shm) {
-                Some(shm) => Some(shm),
-                None if session.selected_profile == WIN_SHM_PROFILE_HYBRID
-                    || session.selected_profile == WIN_SHM_PROFILE_BUSYWAIT =>
-                {
-                    drop(session);
-                    continue;
-                }
-                None => None,
-            };
+            let shm = self.finalize_windows_shm(&session, prepared_shm);
+            if shm.is_none()
+                && (session.selected_profile == WIN_SHM_PROFILE_HYBRID
+                    || session.selected_profile == WIN_SHM_PROFILE_BUSYWAIT)
+            {
+                drop(session);
+                continue;
+            }
 
             let expected_method_code = self.expected_method_code;
             let handler = self.handler.clone();
@@ -154,6 +152,7 @@ impl ManagedServer {
             return None;
         }
         let mut prepared = prepared?;
+        // Keep the negotiated context and destroy every unused prepared context.
         let selected = prepared.take(profile);
         prepared.destroy_all();
         selected

src/go/pkg/netipc/protocol/lookup_common.go

@@ -26,6 +26,7 @@ func invalidSourceString(data []byte, requireNonEmpty bool) bool {
 	return (requireNonEmpty && len(data) == 0) || bytes.IndexByte(data, 0) >= 0
 }
 
+// maxIntValue returns the maximum value representable by int on this platform.
 func maxIntValue() int {
 	return int(^uint(0) >> 1)
 }

src/go/pkg/netipc/transport/posix/uds_stale.go

@@ -7,6 +7,7 @@ import (
 	"net"
 	"os"
 	"syscall"
+	"time"
 )
 
 type staleResult int
@@ -17,27 +18,52 @@ const (
 	staleLiveServer staleResult = 2
 )
 
+const (
+	staleDialAttempts   = 3
+	staleDialRetryDelay = 50 * time.Millisecond
+	staleDialTimeout    = 1 * time.Second
+)
+
 func runDirAllowsStaleUnlink(runDir string) bool {
+	euid := uint32(os.Geteuid())
 	info, err := os.Stat(runDir)
 	if err != nil || !info.IsDir() {
 		return false
 	}
 	st, ok := info.Sys().(*syscall.Stat_t)
-	if !ok || st.Uid != uint32(os.Geteuid()) {
+	if !ok || st.Uid != euid {
 		return false
 	}
 	return info.Mode().Perm()&0022 == 0
 }
 
+func dialStaleCandidate(path string) error {
+	var err error
+	for attempt := 0; attempt < staleDialAttempts; attempt++ {
+		var conn net.Conn
+		conn, err = net.DialTimeout("unixpacket", path, staleDialTimeout)
+		if err == nil {
+			_ = conn.Close()
+			return nil
+		}
+		if !errors.Is(err, syscall.ECONNREFUSED) {
+			return err
+		}
+		if attempt+1 < staleDialAttempts {
+			time.Sleep(staleDialRetryDelay)
+		}
+	}
+	return err
+}
+
 func checkAndRecoverStale(path string, allowStaleUnlink bool) staleResult {
 	_, err := os.Stat(path)
 	if err != nil {
 		return staleNotExist
 	}
 
-	conn, err := net.Dial("unixpacket", path)
+	err = dialStaleCandidate(path)
 	if err == nil {
-		_ = conn.Close()
 		return staleLiveServer
 	}
 

src/go/pkg/netipc/transport/windows/pipe_integration_test.go

@@ -1073,8 +1073,8 @@ func TestPipeHandleAndRole(t *testing.T) {
 	if client.Handle() == syscall.InvalidHandle || server.Handle() == syscall.InvalidHandle {
 		t.Fatal("session handles should be valid")
 	}
-	if client.GetRole() != RoleClient || server.GetRole() != RoleServer {
-		t.Fatalf("unexpected roles client=%d server=%d", client.GetRole(), server.GetRole())
+	if client.Role() != RoleClient || server.Role() != RoleServer {
+		t.Fatalf("unexpected roles client=%d server=%d", client.Role(), server.Role())
 	}
 }
 

src/go/pkg/netipc/transport/windows/pipe_session.go

@@ -15,6 +15,9 @@ const (
 	RoleServer Role = 2
 )
 
+// spinWaitIterations limits cooperative polling before falling back to sleep.
+const spinWaitIterations = 256
+
 // ClientConfig configures a client connection.
 type ClientConfig struct {
 	SupportedProfiles       uint32
@@ -72,10 +75,16 @@ func (s *Session) Handle() syscall.Handle {
 }
 
 // Role returns the session role.
-func (s *Session) GetRole() Role {
+func (s *Session) Role() Role {
 	return s.role
 }
 
+// GetRole returns the session role.
+// Deprecated: use Role.
+func (s *Session) GetRole() Role {
+	return s.Role()
+}
+
 // WaitReadable waits until bytes are available to read or the timeout expires.
 func (s *Session) WaitReadable(timeoutMs uint32) (bool, error) {
 	if s.handle == syscall.InvalidHandle {
@@ -101,7 +110,7 @@ func (s *Session) WaitReadable(timeoutMs uint32) (bool, error) {
 		}
 		if !yielded {
 			yielded = true
-			for i := 0; i < 256; i++ {
+			for i := 0; i < spinWaitIterations; i++ {
 				procSwitchToThread.Call()
 				available, err = peekNamedPipeAvailable(s.handle)
 				if err != nil {

src/go/pkg/netipc/transport/windows/shm.go

@@ -160,7 +160,11 @@ type WinShmContext struct {
 }
 
 // Role returns the context role.
-func (c *WinShmContext) GetRole() WinShmRole { return c.role }
+func (c *WinShmContext) Role() WinShmRole { return c.role }
+
+// GetRole returns the context role.
+// Deprecated: use Role.
+func (c *WinShmContext) GetRole() WinShmRole { return c.Role() }
 
 // ---------------------------------------------------------------------------
 //  Server API

src/go/pkg/netipc/transport/windows/shm_test.go

@@ -111,11 +111,11 @@ func TestWinShmCreateAttachAndCloseValidation(t *testing.T) {
 	}
 	defer client.WinShmClose()
 
-	if server.GetRole() != WinShmRoleServer {
-		t.Fatalf("server role = %d, want %d", server.GetRole(), WinShmRoleServer)
+	if server.Role() != WinShmRoleServer {
+		t.Fatalf("server role = %d, want %d", server.Role(), WinShmRoleServer)
 	}
-	if client.GetRole() != WinShmRoleClient {
-		t.Fatalf("client role = %d, want %d", client.GetRole(), WinShmRoleClient)
+	if client.Role() != WinShmRoleClient {
+		t.Fatalf("client role = %d, want %d", client.Role(), WinShmRoleClient)
 	}
 }
 

src/libnetdata/netipc/src/transport/posix/netipc_uds_send.c

@@ -2,6 +2,11 @@
 
 #include <string.h>
 
+static size_t min_size(size_t a, size_t b)
+{
+    return a < b ? a : b;
+}
+
 static bool tracks_client_request(const nipc_uds_session_t *session,
                                   const nipc_header_t *hdr)
 {
@@ -14,7 +19,10 @@ static void update_inflight_after_send(nipc_uds_session_t *session,
                                        bool tracked,
                                        nipc_uds_error_t err)
 {
-    if (!tracked || err == NIPC_UDS_OK)
+    if (!tracked)
+        return;
+
+    if (err == NIPC_UDS_OK)
         return;
 
     if (err == NIPC_UDS_ERR_SEND)
@@ -66,9 +74,11 @@ static nipc_uds_error_t validate_outbound_limits(nipc_uds_session_t *session,
     uint32_t max_batch;
     outbound_limits(session, hdr, &max_payload, &max_batch);
 
-    if (payload_len <= UINT32_MAX &&
-        (max_payload == 0 || payload_len <= max_payload) &&
-        (max_batch == 0 || hdr->item_count <= max_batch))
+    bool payload_fits_u32 = payload_len <= UINT32_MAX;
+    bool payload_within_limit = max_payload == 0 || payload_len <= max_payload;
+    bool batch_within_limit = max_batch == 0 || hdr->item_count <= max_batch;
+
+    if (payload_fits_u32 && payload_within_limit && batch_within_limit)
         return NIPC_UDS_OK;
 
     if (tracked)
@@ -154,9 +164,7 @@ static nipc_uds_error_t send_chunked(nipc_uds_session_t *session,
         return NIPC_UDS_ERR_BAD_PARAM;
 
     size_t remaining = payload_len;
-    size_t first_chunk_payload = remaining < chunk_payload_budget
-        ? remaining
-        : chunk_payload_budget;
+    size_t first_chunk_payload = min_size(remaining, chunk_payload_budget);
     remaining -= first_chunk_payload;
 
     uint32_t continuation_chunks = 0;
@@ -175,9 +183,7 @@ static nipc_uds_error_t send_chunked(nipc_uds_session_t *session,
     remaining = payload_len - first_chunk_payload;
 
     for (uint32_t ci = 1; ci < chunk_count; ci++) {
-        size_t this_chunk = remaining < chunk_payload_budget
-            ? remaining
-            : chunk_payload_budget;
+        size_t this_chunk = min_size(remaining, chunk_payload_budget);
 
         err = send_continuation_chunk(session, hdr, src, this_chunk, ci,
                                       chunk_count, (uint32_t)total_msg,